6 ms·

My first C program: simple process supervisor like daemontools/runit in 359 LoC

48 points by uggedal 14y ago

antirez 14y ago
congrats for your first C project! A few reamarks: 1) // style commands are C99 only. If you don't have other C99 dependencies it's somewhat a shame it could not compile in older systems just for this. 2) inline should rarely be used if not into performance critical code. 3) two space indent is not C-ish ;) 4) While 'bool' is valid C99 code, it is more or less never used in C code.
- maybird 14y ago
  (3) You bring up an interesting issue. Why is there so much variance out there? Google uses 2 spaces, the linux kernel uses 8, and most people I know use 4. Are people using such a wide range of font sizes, and font aspect ratios, that 4 spaces doesn't make sense for everyone?
  - udp 14y ago
    I used 3 on my last open source C project, just to troll every faction of the indent war. I actually think it looks pretty nice. https://raw.github.com/udp/json-parser/master/json.c https://raw.github.com/udp/json-parser/master/json.c
    - dschulz 14y ago
      indent -kr json.c problem solved ;-)
  - graue 14y ago
    When I was writing a lot of C, I settled on 8-column tabs (a style I copied from OpenBSD). Combined with an 80-column wide terminal, it prevents you from nesting too far down — 4 or 5 levels and you keep having to wrap, which is an automatic sign that "something is wrong, I need to refactor this."
    - tptacek 14y ago
      I was an 8spcr (default tabstop, tab-to-indent, learned C in pico). I've gone a year or two writing nothing but Ruby (where 2spc is the rule) and recently had to build a small C program, and oh my god is 8spc terrible. But I'm also a single-return fetishist, so I tend to nest deeper than normal.
  - deleted 14y ago
    [deleted]
  - jonathansizz 14y ago
    It's funny how so many coders take offence at such trivial things. I always get annoyed by cuddled elses, although I don't suppose I could give a rational explanation for this!
    - pivo 14y ago
      I also feel annoyed by cuddled elses. My reason (rationalization?) is that because the else clause is visually further indented it seems to suggest that the clause is somehow less important, and it's also harder to see the structure of the code when the if and else don't align.
- rcfox 14y ago
  > While 'bool' is valid C99 code, it is more or less never used in C code. I disagree. It's used quite a bit in the code I work on. Little things like an explicit statement that this variable is only true or false significantly reduce the cognitive load when reading code. Anyway, even if it were true that nobody uses bool, there's absolutely no reason to discourage its use.
  - zvrba 14y ago
    > Little things like an explicit statement that this variable is only true or false significantly reduce the cognitive load when reading code. Since it's allowed to use bool in arithmetic context (some_int + some_bool will compile fine, for example), having a bool type is useless. (Useless in C and C++ that is; Java, for example, has sane bool handling.) I have no idea why C99 introduced _Bool type when they could have just inserted typedef char bool; in stdbool.h. Has anybody ever used for something useful the fact that memory representation of _Bool is constrained to read as 0 or 1?
    - VMG 14y ago
      it still is a useful annotation
    - fusiongyro 14y ago
      1. It's semantically valuable, or you wouldn't see it defined in everyone's code 2. C lets you commit all kinds of atrocities in arithmetic contexts with or without it. 3. Having a standard typedef saves us from all having to make our own 4. None of your points support the case of "don't use stdbool"
      - zvrba 14y ago
        > 4. None of your points support the case of "don't use stdbool" Huh? I was saying that the language did not have to be extended to get a bool typedef. C99 defines a new type, _Bool (name from the reserved identifier space), which is different from existing types. stdbool is a simple typedef of _Bool to bool. Also, introducing bool would have had meaning if implicit conversion to int wasn't allowed. But since it is, it's a useless type. My viewpoint is that types exist for enforcing semantic constraints; what use of a bool type that allows performing semantically nonsense operation, such as implicit conversion to int in arithmetic context? The C99 committee messed it up big with bool.
- ajross 14y ago
  I'm aware of no "older systems" still in reasonable use that don't support the // comment convention. That got added to pretty much every C toolchain as soon as anyone started shipping a C++ preprocessor. I believe it is disabled by the -ansi switch to gcc though; use -std=c99 instead if you care about that stuff (or better: don't use -ansi, it doesn't actually make your code more portable, it just whines a lot).
codebeaker 14y ago
I adore that you used literate style documentation for your source, as someone who is also learning to love programming efficient tools in C I very much appreciate that. I question the need for yet-another-process-monitor, but I acknowledge that upstart, monit, etc are by and large overcomplicated and messy. They provide more features, of course, but they violate my interpretation of the unix philosophy. I'm sure many people on HN will condemn you for writing this but, I for one am very excited to try it. I had a similar idea of a daemon that would allow inter-process signaling for users to signal processes which shared their GID, to simplify web application deployment without relying on sudo privilege restrictions. You may have just inspired me to take your lead, and just do it - a few hundred lines of C, and smart documentation have a lot of value as learning, experience, proof-of-concept and a discussion point; and that in itself is reason enough for this to exist!
peterwwillis 14y ago
I can't really comment on code since i'm no master of C programming myself, but there are really way too many comments. It actually makes it harder to read code if there are comments on every new line, and harder still if you have multiple lines of comments to explain one line of code. My recommendation is to have one comment to explain a large chunk of code and let the programmer discern what part is doing what instead of annotating it piece by piece.
- lshevtsov 14y ago
  This. Having a paragraph of explanation text for each line is not a replacement for source code that is readable by itself. I understand, though, that explaining each and every line of code was done for the sake of learning C.
  - tptacek 14y ago
    This is very readable C code, as C code goes. The documentation isn't there as a substitute for that.
- advisedwang 14y ago
  Literate programming tools like CWEB often have a tool (in CWEB called CTANGLE) that extracts just the machine-readable code. Perhaps there is some tool you can use to decomment this code (maybe even just grep :D)
- m_for_monkey 14y ago
  It is meant as a literate program: http://uggedal.github.com/going/going.c.html http://uggedal.github.com/going/going.c.html. (Scroll down on this page!)
  - Camillo 14y ago
    It's actually a pretty nice example of the problems with overcommenting. For example, redundancy: "atexit(cleanup_children)" is perfectly self-explanatory, yet he repeats the same thing with the circumlocution "We setup our cleanup function as an exit handler which will be called at normal process termination." That's just a waste of both the writer and the reader's time. And of course, redundancy spawns inconsistency: the code says "spawn_unquarantined_children()", but the comment says "All quarantined (a newly initialized child structure is quarantined by default) children is spawned for the first time." Which is it, quarantined or unquarantined? And the comment breaks number agreement, too. I find it remarkably illustrative that comment rot has already set in into version 1.0 of a "359 LoC" program (actually 797, according to github).
fallenhitokiri 14y ago
I, as most developers, appreciate well commented code. But as others have mentioned you are going a bit too far. // The user has given and illegal number or type of arguments. The program // usage is printed to the standard error stream and we exit abnormally. fprintf(stderr, USAGE); exit(EX_USAGE); What else is it supposed to be? This is, even for me who does not use C(++) on a regular basis, obvious. It looks like you are interested in writing well documented code. I think a good example you can learn a lot from is Redis[1]. Also written in C. [1] https://github.com/antirez/redis https://github.com/antirez/redis //edit: fixed typo
lxfontes 14y ago
Good job !! Consistent coding convention :) As a constructive note, think about reversing your conditions (check if something NOT happened) and break huge chunks of code into functions. This will guarantee a 'sane' amount of nested 'ifs' and improve code readability. Ex: for (int i = dn - 1; i >= 0; i--) { if (!has_child(dlist[i]->d_name)) { if ((fp = fopen(path, "r")) != NULL) { ............. } else { slog(LOG_ERR, "Can't read %s: %m", path); } } } Notice how the error message is 20 lines far from the actual condition. You can rework that as: if ((fp = fopen(path, "r")) != NULL) { do_some_work_with(fp); } else { slog(LOG_ERR, "Can't read %s: %m", path); } or if (!(fp = fopen(path, "r")) { slog(LOG_ERR, "Can't read %s: %m", path); continue; } //removed 1 nesting level ........
basugasubaku 14y ago
Your get_tail_child always returns NULL for (tail_ch = head_ch; tail_ch; tail_ch = tail_ch->next); return tail_ch;
- jws 14y ago
  Well spotted! Going to want to add a test case with more than one child at a time. I think as is it loses all but the most recent child.
- uggedal 14y ago
  That was embarrasing. Should be fixed now: https://github.com/uggedal/going/commit/15a83397076ffbebe58550c9fb210ee5f0be82a6 https://github.com/uggedal/going/commit/15a83397076ffbebe585...
jws 14y ago
Congratulations! You have written completely non-terrifying C code. This would not inspire any sense of dread in me if I had to go in and look for a problem, so I will. Comments – I see you have alarmed the HN crowd, but I was reading the annotated page and it makes sense in that context. As a data point, I only looked at the lefthand column paragraphs about twice, related to the quarantine handling in the forever loop. There is an odd effect if children are dying faster than the quarantine period, but I convinced myself that there was a finite number of children and they would eventually get quarantined if they kept that up allowing the period to finally expire. I looked at the titles though. Design – I'd lose the fixed size string buffers in the child_t. 256 sounds like a lot for a command until someone uses a wildcard in a directory. strdup() and free() are your friends here. I see you have a centralized free function for the child_t and are using valgrind. I trust you will not leak or shoot off a body part. Portability – You are using "%m" to get the strerror(errno) into your log messages. This is a GNU libc extension. I don't have a pure BSD machine around, but it doesn't work on OS X. This will likely also crop up for very small Linux systems that use alternative C libraries for space reasons, such as OpenWRT. strncmp() and the strnXXX() functions in general – These are not really what you want. Think of them as functions for fixed length string fields with optional NUL terminators. Then never use them. Consider… strncmp("foo", "foobar", 3) It returns 0, that means equal. That is seldom what you actually intended. In your case, strcmp() is fine because you are working with guaranteed NUL terminated strings. I see a strnlen()… inline bool str_not_empty(char *str) { return strnlen(str, 1) == 1; } Most C programmers would rather see… inline bool str_not_empty(char *str) { return *str != 0; } … but they'd also leave off the "inline" and use "int" instead of bool. "inline" is the compiler's problem, let it take care of that. The part you might do for performance is to mark the argument as "const" since you aren't going to change it. That gives the optimizer more freedom around the calls, though in this case the optimizer will probably inline it and it won't matter. "const" is also a nice hint to people reading your code. I suppose summing up strings, "char *" is not an opaque type to C programmers. Signals and file descriptors across forks – Signals are generally a bit tricky, and what to do with open file descriptors across a fork is also fussy. I'll leave that to you. (Frequently in cases like this, closing all file descriptors is the right answer. A shame there isn't a portable way to do that. Remember you might have something open from the syslog functions.)
utf8guy 14y ago
Is this really you're first C program. That's very, very impressive! My first C program was something like: main() { printf("Hello, world\n"); }
travelstacker 14y ago
Gasp! I'm sure your first program is meant to read: #include <stdio.h> main() { printf("hello, world\n"); } ;)

48 points by uggedal 14y ago

congrats for your first C project! A few reamarks: 1) // style commands are C99 only. If you don't have other C99 dependencies it's somewhat a shame it could not compile in older systems just for this. 2) inline should rarely be used if not into performance critical code. 3) two space indent is not C-ish ;) 4) While 'bool' is valid C99 code, it is more or less never used in C code.

maybird 14y ago
(3) You bring up an interesting issue. Why is there so much variance out there? Google uses 2 spaces, the linux kernel uses 8, and most people I know use 4. Are people using such a wide range of font sizes, and font aspect ratios, that 4 spaces doesn't make sense for everyone?
- udp 14y ago
  I used 3 on my last open source C project, just to troll every faction of the indent war. I actually think it looks pretty nice. https://raw.github.com/udp/json-parser/master/json.c https://raw.github.com/udp/json-parser/master/json.c
  - dschulz 14y ago
    indent -kr json.c problem solved ;-)
- graue 14y ago
  When I was writing a lot of C, I settled on 8-column tabs (a style I copied from OpenBSD). Combined with an 80-column wide terminal, it prevents you from nesting too far down — 4 or 5 levels and you keep having to wrap, which is an automatic sign that "something is wrong, I need to refactor this."
  - tptacek 14y ago
    I was an 8spcr (default tabstop, tab-to-indent, learned C in pico). I've gone a year or two writing nothing but Ruby (where 2spc is the rule) and recently had to build a small C program, and oh my god is 8spc terrible. But I'm also a single-return fetishist, so I tend to nest deeper than normal.
- deleted 14y ago
  [deleted]
- jonathansizz 14y ago
  It's funny how so many coders take offence at such trivial things. I always get annoyed by cuddled elses, although I don't suppose I could give a rational explanation for this!
  - pivo 14y ago
    I also feel annoyed by cuddled elses. My reason (rationalization?) is that because the else clause is visually further indented it seems to suggest that the clause is somehow less important, and it's also harder to see the structure of the code when the if and else don't align.
rcfox 14y ago
> While 'bool' is valid C99 code, it is more or less never used in C code. I disagree. It's used quite a bit in the code I work on. Little things like an explicit statement that this variable is only true or false significantly reduce the cognitive load when reading code. Anyway, even if it were true that nobody uses bool, there's absolutely no reason to discourage its use.
- zvrba 14y ago
  > Little things like an explicit statement that this variable is only true or false significantly reduce the cognitive load when reading code. Since it's allowed to use bool in arithmetic context (some_int + some_bool will compile fine, for example), having a bool type is useless. (Useless in C and C++ that is; Java, for example, has sane bool handling.) I have no idea why C99 introduced _Bool type when they could have just inserted typedef char bool; in stdbool.h. Has anybody ever used for something useful the fact that memory representation of _Bool is constrained to read as 0 or 1?
  - VMG 14y ago
    it still is a useful annotation
  - fusiongyro 14y ago
    1. It's semantically valuable, or you wouldn't see it defined in everyone's code 2. C lets you commit all kinds of atrocities in arithmetic contexts with or without it. 3. Having a standard typedef saves us from all having to make our own 4. None of your points support the case of "don't use stdbool"
    - zvrba 14y ago
      > 4. None of your points support the case of "don't use stdbool" Huh? I was saying that the language did not have to be extended to get a bool typedef. C99 defines a new type, _Bool (name from the reserved identifier space), which is different from existing types. stdbool is a simple typedef of _Bool to bool. Also, introducing bool would have had meaning if implicit conversion to int wasn't allowed. But since it is, it's a useless type. My viewpoint is that types exist for enforcing semantic constraints; what use of a bool type that allows performing semantically nonsense operation, such as implicit conversion to int in arithmetic context? The C99 committee messed it up big with bool.
ajross 14y ago
I'm aware of no "older systems" still in reasonable use that don't support the // comment convention. That got added to pretty much every C toolchain as soon as anyone started shipping a C++ preprocessor. I believe it is disabled by the -ansi switch to gcc though; use -std=c99 instead if you care about that stuff (or better: don't use -ansi, it doesn't actually make your code more portable, it just whines a lot).

codebeaker 14y ago

I adore that you used literate style documentation for your source, as someone who is also learning to love programming efficient tools in C I very much appreciate that. I question the need for yet-another-process-monitor, but I acknowledge that upstart, monit, etc are by and large overcomplicated and messy. They provide more features, of course, but they violate my interpretation of the unix philosophy. I'm sure many people on HN will condemn you for writing this but, I for one am very excited to try it. I had a similar idea of a daemon that would allow inter-process signaling for users to signal processes which shared their GID, to simplify web application deployment without relying on sudo privilege restrictions. You may have just inspired me to take your lead, and just do it - a few hundred lines of C, and smart documentation have a lot of value as learning, experience, proof-of-concept and a discussion point; and that in itself is reason enough for this to exist!

peterwwillis 14y ago

I can't really comment on code since i'm no master of C programming myself, but there are really way too many comments. It actually makes it harder to read code if there are comments on every new line, and harder still if you have multiple lines of comments to explain one line of code. My recommendation is to have one comment to explain a large chunk of code and let the programmer discern what part is doing what instead of annotating it piece by piece.

lshevtsov 14y ago
This. Having a paragraph of explanation text for each line is not a replacement for source code that is readable by itself. I understand, though, that explaining each and every line of code was done for the sake of learning C.
- tptacek 14y ago
  This is very readable C code, as C code goes. The documentation isn't there as a substitute for that.
advisedwang 14y ago
Literate programming tools like CWEB often have a tool (in CWEB called CTANGLE) that extracts just the machine-readable code. Perhaps there is some tool you can use to decomment this code (maybe even just grep :D)
m_for_monkey 14y ago
It is meant as a literate program: http://uggedal.github.com/going/going.c.html http://uggedal.github.com/going/going.c.html. (Scroll down on this page!)
- Camillo 14y ago
  It's actually a pretty nice example of the problems with overcommenting. For example, redundancy: "atexit(cleanup_children)" is perfectly self-explanatory, yet he repeats the same thing with the circumlocution "We setup our cleanup function as an exit handler which will be called at normal process termination." That's just a waste of both the writer and the reader's time. And of course, redundancy spawns inconsistency: the code says "spawn_unquarantined_children()", but the comment says "All quarantined (a newly initialized child structure is quarantined by default) children is spawned for the first time." Which is it, quarantined or unquarantined? And the comment breaks number agreement, too. I find it remarkably illustrative that comment rot has already set in into version 1.0 of a "359 LoC" program (actually 797, according to github).

fallenhitokiri 14y ago

I, as most developers, appreciate well commented code. But as others have mentioned you are going a bit too far. // The user has given and illegal number or type of arguments. The program // usage is printed to the standard error stream and we exit abnormally. fprintf(stderr, USAGE); exit(EX_USAGE); What else is it supposed to be? This is, even for me who does not use C(++) on a regular basis, obvious. It looks like you are interested in writing well documented code. I think a good example you can learn a lot from is Redis[1]. Also written in C. [1] https://github.com/antirez/redis https://github.com/antirez/redis //edit: fixed typo

lxfontes 14y ago

Good job !! Consistent coding convention :) As a constructive note, think about reversing your conditions (check if something NOT happened) and break huge chunks of code into functions. This will guarantee a 'sane' amount of nested 'ifs' and improve code readability. Ex: for (int i = dn - 1; i >= 0; i--) { if (!has_child(dlist[i]->d_name)) { if ((fp = fopen(path, "r")) != NULL) { ............. } else { slog(LOG_ERR, "Can't read %s: %m", path); } } } Notice how the error message is 20 lines far from the actual condition. You can rework that as: if ((fp = fopen(path, "r")) != NULL) { do_some_work_with(fp); } else { slog(LOG_ERR, "Can't read %s: %m", path); } or if (!(fp = fopen(path, "r")) { slog(LOG_ERR, "Can't read %s: %m", path); continue; } //removed 1 nesting level ........

basugasubaku 14y ago

Your get_tail_child always returns NULL for (tail_ch = head_ch; tail_ch; tail_ch = tail_ch->next); return tail_ch;

jws 14y ago
Well spotted! Going to want to add a test case with more than one child at a time. I think as is it loses all but the most recent child.
uggedal 14y ago
That was embarrasing. Should be fixed now: https://github.com/uggedal/going/commit/15a83397076ffbebe58550c9fb210ee5f0be82a6 https://github.com/uggedal/going/commit/15a83397076ffbebe585...

jws 14y ago

Congratulations! You have written completely non-terrifying C code. This would not inspire any sense of dread in me if I had to go in and look for a problem, so I will. Comments – I see you have alarmed the HN crowd, but I was reading the annotated page and it makes sense in that context. As a data point, I only looked at the lefthand column paragraphs about twice, related to the quarantine handling in the forever loop. There is an odd effect if children are dying faster than the quarantine period, but I convinced myself that there was a finite number of children and they would eventually get quarantined if they kept that up allowing the period to finally expire. I looked at the titles though. Design – I'd lose the fixed size string buffers in the child_t. 256 sounds like a lot for a command until someone uses a wildcard in a directory. strdup() and free() are your friends here. I see you have a centralized free function for the child_t and are using valgrind. I trust you will not leak or shoot off a body part. Portability – You are using "%m" to get the strerror(errno) into your log messages. This is a GNU libc extension. I don't have a pure BSD machine around, but it doesn't work on OS X. This will likely also crop up for very small Linux systems that use alternative C libraries for space reasons, such as OpenWRT. strncmp() and the strnXXX() functions in general – These are not really what you want. Think of them as functions for fixed length string fields with optional NUL terminators. Then never use them. Consider… strncmp("foo", "foobar", 3) It returns 0, that means equal. That is seldom what you actually intended. In your case, strcmp() is fine because you are working with guaranteed NUL terminated strings. I see a strnlen()… inline bool str_not_empty(char *str) { return strnlen(str, 1) == 1; } Most C programmers would rather see… inline bool str_not_empty(char *str) { return *str != 0; } … but they'd also leave off the "inline" and use "int" instead of bool. "inline" is the compiler's problem, let it take care of that. The part you might do for performance is to mark the argument as "const" since you aren't going to change it. That gives the optimizer more freedom around the calls, though in this case the optimizer will probably inline it and it won't matter. "const" is also a nice hint to people reading your code. I suppose summing up strings, "char *" is not an opaque type to C programmers. Signals and file descriptors across forks – Signals are generally a bit tricky, and what to do with open file descriptors across a fork is also fussy. I'll leave that to you. (Frequently in cases like this, closing all file descriptors is the right answer. A shame there isn't a portable way to do that. Remember you might have something open from the syslog functions.)

utf8guy 14y ago

Is this really you're first C program. That's very, very impressive! My first C program was something like: main() { printf("Hello, world\n"); }

travelstacker 14y ago

Gasp! I'm sure your first program is meant to read: #include <stdio.h> main() { printf("hello, world\n"); } ;)