3 ms·

I usually use switch with fork: if (daemon && !test_mode) { int pid; switch (pid = fork()) { case -1: /* Error */ fatal

I usually use switch with fork: if (daemon && !test_mode) { int pid; switch (pid = fork()) { case -1: /* Error */ fatal_error("Failed to fork"); case 0: /* In child */ break; default: /* In parent */ write_pid(pid_file, pid, !test_mode); exit(0); } } else { write_pid(pid_file, getpid(), !test_mode); }

jgrahamc 12y ago
Yes. That's rather nice. Although I'm not a fan of the (pid = fork()) inline assignment and condition. But that's a matter of taste, not technology.
- cperciva 12y ago
  Sure, you could have pid = fork(); switch (pid) if you prefer. I find that the inline assignment-and-condition style is clearer since it reads to me as "switch on the result of fork, and cache that value somewhere" whereas the separate statements read to me as "call fork" and "switch on the process ID".
colanderman 12y ago
For daemonization, daemon(3) is better (EDIT: assuming you only care about Linux). (It also chdirs to /, closes STD*, and detaches from the terminal.)
- cperciva 12y ago
  Alas, daemon(3) is not POSIX.
- mindajar 12y ago
  daemon(3) isn't part of POSIX, and the OS X man page says "the use of this API is discouraged in favor of using launchd(8)", so who knows what might happen here in the future. If you don't care about portability, it's an easier call to make.
cliveowen 12y ago
You should always put a break statement at the end of a default branch. The default case is put at the end as a convention but no one (certainly not the C standard) prevents you from adding a new case after, which will be promptly executed even though probably it's what you want. I think this is mentioned a best practice in the K&R.
- cperciva 12y ago
  You mean, after exit(0) returns?
  - AnimalMuppet 12y ago
    Not after exit(0) returns, no. Instead, you're going to want it after some programmer removes exit(0) six months from now.
    - loup-vaillant 12y ago
      I'm going to write a pre-processor which adds the appropriate `break` statements automatically. No more fall-through bug. (And if you really want to fall through, I could add a `fallthrough` keyword.)
      - cperciva 12y ago
        For what it's worth, it's a style bug in FreeBSD to have a fallthrough which does not have a /* FALLTHROUGH */ comment.
      - scintill76 12y ago
        Yep, could even re-use the "continue" keyword.
    - hamburglar 12y ago
      smart-ass comment: you should put two breaks in for when some programmer removes one of them six months from now. ;) edit: i know it's not totally analogous since removing the exit and not noticing there's no break is a lot more likely than just randomly removing a break, but the "let's prevent someone clumsy from screwing this code up in the future" argument always makes me laugh a little.
rosser 12y ago
I'm kinda surprised that the assignment-during-test thing doesn't generate a warning in the case of a switch statement. It does in other cases (if and while do under gcc and clang, at least).
- cperciva 12y ago
  If it creates warnings, you can silence them by adding extra parentheses, e.g., if ((buf = malloc(buflen)) == NULL) goto outofmemory;
  - rosser 12y ago
    I was originally going to comment saying exactly that — under gcc and clang, at least, just the extra parens will disable the warning; no comparison necessary — but thought to test that the warning is generated in case of a switch statement (and is then disabled by the extra set of parens), which led to realizing the behavior is different. Just surprised by the inconsistency.
  - loup-vaillant 12y ago
    Careful with that example. The parentheses here don't just remove the warning. They remove a bug. This code: if (buf = malloc(buflen) == NULL) goto outofmemory; is actually equivalent to that code: if (buf = (malloc(buflen) == NULL)) goto outofmemory; So, malloc gives you a pointer, which is compared to the null pointer, giving you either 0 or 1. And that is assigned to buf. Hopefully your compiler will warn you about the type error that spawns from such dark magic.
    - deleted 12y ago
      [deleted]
    - cperciva 12y ago
      Oops, quite right. I was originally thinking of if ((rc = pthread_mutex_lock(mtx))) err("mutex_lock failed: %s", strerror(rc)); but decided to switch to a better-known function at the last minute and completely lost the point.
ChuckMcM 12y ago
Somewhat pedantically, I discovered a bug where a daemon (which did this very similar code) started killing random processes. The issue turned out to be that it was test run as root once and the pid_file was owned by root, group root. So the write_pid() function failed (silently) and the cleanup script always took the pid that was in pid_file and sent it a kill 9, which was now stuck. Sometimes kill 9 would return invalid pid, sometimes it would kill some process. Fixed by removing the pid_file, letting the daemon create it, and checking that the write succeeded.

jgrahamc 12y ago

Yes. That's rather nice. Although I'm not a fan of the (pid = fork()) inline assignment and condition. But that's a matter of taste, not technology.

cperciva 12y ago
Sure, you could have pid = fork(); switch (pid) if you prefer. I find that the inline assignment-and-condition style is clearer since it reads to me as "switch on the result of fork, and cache that value somewhere" whereas the separate statements read to me as "call fork" and "switch on the process ID".

colanderman 12y ago

For daemonization, daemon(3) is better (EDIT: assuming you only care about Linux). (It also chdirs to /, closes STD*, and detaches from the terminal.)

cperciva 12y ago
Alas, daemon(3) is not POSIX.
mindajar 12y ago
daemon(3) isn't part of POSIX, and the OS X man page says "the use of this API is discouraged in favor of using launchd(8)", so who knows what might happen here in the future. If you don't care about portability, it's an easier call to make.

cliveowen 12y ago

You should always put a break statement at the end of a default branch. The default case is put at the end as a convention but no one (certainly not the C standard) prevents you from adding a new case after, which will be promptly executed even though probably it's what you want. I think this is mentioned a best practice in the K&R.

cperciva 12y ago
You mean, after exit(0) returns?
- AnimalMuppet 12y ago
  Not after exit(0) returns, no. Instead, you're going to want it after some programmer removes exit(0) six months from now.
  - loup-vaillant 12y ago
    I'm going to write a pre-processor which adds the appropriate `break` statements automatically. No more fall-through bug. (And if you really want to fall through, I could add a `fallthrough` keyword.)
    - cperciva 12y ago
      For what it's worth, it's a style bug in FreeBSD to have a fallthrough which does not have a /* FALLTHROUGH */ comment.
    - scintill76 12y ago
      Yep, could even re-use the "continue" keyword.
  - hamburglar 12y ago
    smart-ass comment: you should put two breaks in for when some programmer removes one of them six months from now. ;) edit: i know it's not totally analogous since removing the exit and not noticing there's no break is a lot more likely than just randomly removing a break, but the "let's prevent someone clumsy from screwing this code up in the future" argument always makes me laugh a little.

rosser 12y ago

I'm kinda surprised that the assignment-during-test thing doesn't generate a warning in the case of a switch statement. It does in other cases (if and while do under gcc and clang, at least).

cperciva 12y ago
If it creates warnings, you can silence them by adding extra parentheses, e.g., if ((buf = malloc(buflen)) == NULL) goto outofmemory;
- rosser 12y ago
  I was originally going to comment saying exactly that — under gcc and clang, at least, just the extra parens will disable the warning; no comparison necessary — but thought to test that the warning is generated in case of a switch statement (and is then disabled by the extra set of parens), which led to realizing the behavior is different. Just surprised by the inconsistency.
- loup-vaillant 12y ago
  Careful with that example. The parentheses here don't just remove the warning. They remove a bug. This code: if (buf = malloc(buflen) == NULL) goto outofmemory; is actually equivalent to that code: if (buf = (malloc(buflen) == NULL)) goto outofmemory; So, malloc gives you a pointer, which is compared to the null pointer, giving you either 0 or 1. And that is assigned to buf. Hopefully your compiler will warn you about the type error that spawns from such dark magic.
  - deleted 12y ago
    [deleted]
  - cperciva 12y ago
    Oops, quite right. I was originally thinking of if ((rc = pthread_mutex_lock(mtx))) err("mutex_lock failed: %s", strerror(rc)); but decided to switch to a better-known function at the last minute and completely lost the point.

ChuckMcM 12y ago

Somewhat pedantically, I discovered a bug where a daemon (which did this very similar code) started killing random processes. The issue turned out to be that it was test run as root once and the pid_file was owned by root, group root. So the write_pid() function failed (silently) and the cleanup script always took the pid that was in pid_file and sent it a kill 9, which was now stuck. Sometimes kill 9 would return invalid pid, sometimes it would kill some process. Fixed by removing the pid_file, letting the daemon create it, and checking that the write succeeded.