Hacker News Reader

Comment by jgrahamc | Hacker News Reader

Quietly goes to check the last piece of C I wrote containing a fork():

    if (daemon && !test_mode) {
      int pid = fork();
      if (pid == -1) {
        fatal_error("Failed to fork");
      }
      if (pid != 0) {
        write_pid(pid_file, pid, !test_mode);
        exit(0);
      }
    } else {
      write_pid(pid_file, getpid(), !test_mode);
    }

Phew!

View on HN

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);
    }

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.

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.

You mean, after exit(0) returns?

Not after exit(0) returns, no. Instead, you're going to want it after some programmer removes exit(0) six months from now.

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.

Of course, you can always take it too far. Two breaks is too far, obviously (I'm pretty sure you agree with me on that).

Is a break after an exit too far? It may well be, but it's less clearly too far than two breaks are.

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.)

For what it's worth, it's a style bug in FreeBSD to have a fallthrough which does not have a /* FALLTHROUGH */ comment.

Yep, could even re-use the "continue" keyword.

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.

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".

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

Alas, daemon(3) is not POSIX.

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.

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).

Assignment-during-test is flagged because it's common for a typo to conflate assignment and equality-testing. Equality-testing is very common in an if or while, so an assignment inside an if or while has a fairly high likelihood of actually being a mis-typed equality-test. Switch, on the other hand, is very rarely used with an equality-test (since equality-test only returns true or false, why would you use a switch when an if would suffice?). So an assignment inside a switch is much less likely to be a mis-typed equality-test.

If it creates warnings, you can silence them by adding extra parentheses, e.g.,

    if ((buf = malloc(buflen)) == NULL)
        goto outofmemory;

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.

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.

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.

I was curious too about my own code, so I looked at the last publicly available C code I wrote with fork in it[1], and yep I checked for the error case too :)

I think this just came from being drilled in school on the importance of checking error return values. No matter how unlikely never assume that something can't happen. If it really can't you should at the very least assert on it.

[1] - https://github.com/jervisfm/W4118_HW1/blob/master/shell/shel...

Reply on news.ycombinator.com