Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make systemd config generation more resilient #74

Merged
1 commit merged into from Nov 15, 2015
Merged

Make systemd config generation more resilient #74

1 commit merged into from Nov 15, 2015

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2015

We wanna make sure that it really really generates a config, or fails otherwise

cc @Shnatsel @kpcyrd

@Shnatsel
Copy link

Looks good to me. The && everywhere is not necessary because "/bin/sh -e" already takes care of that, but you might as well keep it for readability.

The same changes should be applied to the Upstart script: https://github.com/hyperboria/cjdns/blob/master/contrib/upstart/cjdns.conf

@ghost
Copy link
Author

ghost commented Nov 15, 2015

Right, you said that on IRC when I brought it up -- bringing the semicolons back.

ghost pushed a commit that referenced this pull request Nov 15, 2015
Make systemd config generation more resilient
@ghost ghost merged commit 6479122 into hyperboria:master Nov 15, 2015
@ghost ghost deleted the systemd-genconf branch November 15, 2015 21:41
@Shnatsel
Copy link

Now that I look at the updated diff, this line looks fishy:

echo $conf > /etc/cjdroute.conf

Multi-line variables in shell scripts, like $conf, should be quouted. So it should end up like this eventually:

echo "$conf" > /etc/cjdroute.conf

But you're already quoting the entire thing to get it out of systemd's parser, so you'll have to look up how to escape quotes in systemd docs.

@Kubuxu
Copy link

Kubuxu commented Nov 15, 2015

Also if $conf gets too long echo will fail. It would be better do gen it once, check if it worked and then gen it straight into the file.

EDIT: Might not be an issue in most systems (2MB in case of my Arch).

@Shnatsel
Copy link

Now that I think about it, "test -s" is all that's needed to fix that particular bug. We don't have to do any of this fishy store-and-echo stuff. You can just swap "test -e" for "test -s" and call it a day, and I think we really ought to do that.

@ghost
Copy link
Author

ghost commented Nov 15, 2015

@Shnatsel you make an excellent point! Wanna fix it for systemd too in #76?

@Shnatsel
Copy link

Not really. I've found a git recipe for rolling back a single file, should be this:

git checkout 01b2990 contrib/systemd/cjdns.service

And then swap "-e" for "-s" and poof, done.

But you'd have to tell the the correct branch-switching and your-changes-pulling and this-branch-pushing sequence too because last time I did that on git instead of bzr there were fireworks and I don't really want to go through it all again right now.

ghost pushed a commit that referenced this pull request Nov 16, 2015
Fix config generation failing permanently with a blank config file if cjdroute binary is not in $PATH the first time around.
Should never happen in .deb packages (where this config is most used), but it's a reasonable robustness improvement nevertheless.
ghost pushed a commit that referenced this pull request Nov 16, 2015
Propagate the changes from #74 to upstart script
@ghost ghost mentioned this pull request Nov 16, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants