
My first C program: simple process supervisor like daemontools/runit  in 359 LoC - uggedal
http://uggedal.github.com/going/
======
lxfontes
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
          ........

------
fallenhitokiri
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>

//edit: fixed typo

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

~~~
m_for_monkey
It is meant as a literate program:
<http://uggedal.github.com/going/going.c.html>. (Scroll down on this page!)

~~~
peterwwillis
I saw that, and I still think it's quite excessive. Perl modules often come
with POD documentation in-line with code, but it's usually just to document
something like a function or method with maybe an example or two. (And yes,
POD is not literate programming, but it's more like the comments you'd see in
a normal program source code)

I suppose this is up to the individual, but I don't like literate programming
partly because it interferes with a dynamic that coders around the world have.
Most people include comments only when it's necessary. It signifies something
important to take note of and clarifies unusual behavior. Literate programming
partly strips that away without giving you a really long-term benefit... It
seems useful to me mostly for the initial implementation and ends up making
maintenance a chore.

If I can modify my original comment a bit for literate programming: Make a
comment at the beginning of your function with a bullet list of how the
function will work, step by step, and then just write the code. Easier to read
and you still have your [mostly-]literate program.

~~~
tptacek
If you keep commenting like this, the metadiscussion about comments might
become competitively long.

I liked the comments, not because I needed them to help understand what the
program is doing (this is a bog standard C program that I think most Unix
developers have written several times over) but as document of someone
learning C, and as something I can show other people who want to learn C.

~~~
azelfrath
I'm a sub-par C programmer and the comments definitely helped me out. Instead
of assuming the reader knows what each function does, they are explained in
enough detail that the average reader could at least Google for more info
based on keywords.

------
antirez
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
(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
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>

~~~
dschulz
indent -kr json.c

problem solved ;-)

------
basugasubaku
Your get_tail_child always returns NULL

    
    
      for (tail_ch = head_ch; tail_ch; tail_ch = tail_ch->next);
    
      return tail_ch;

~~~
uggedal
That was embarrasing. Should be fixed now:
[https://github.com/uggedal/going/commit/15a83397076ffbebe585...](https://github.com/uggedal/going/commit/15a83397076ffbebe58550c9fb210ee5f0be82a6)

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

~~~
xyzzyz
_… but they'd also leave off the "inline" and use "int" instead of bool._

Why use int, when there's bool in C99 that clearly communicates the intent?

~~~
jws
I like the human communication aspect of "bool", but dislike the compiler
language dependency.

The C cultural expectation is that anything named like a predicate is going to
return an int for true or false. In another language with a different culture
I would use a Boolean or whatever it had.

~~~
xyzzyz
Every modern C compiler (not counting Microsoft one with terribly lame support
for C99) supports bool, so there's no compiler dependency.

------
utf8guy
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
Gasp! I'm sure your first program is meant to read:

#include <stdio.h>

main() { printf("hello, world\n"); }

;)

