Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>
> Extract functions and structures for managing child processes started
> from the parent daemon-like process from `daemon.c` to the new shared
> `daemon-utils.{c,h}` files.
As with patch 1, it looks like the main changes here are changing global
references to function arguments. Specifically, those variables are
'firstborn', 'live_children', and 'loginfo':
> -static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)
> +void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
> + struct child *firstborn , unsigned int *live_children)
> -static void kill_some_child(void)
> +void kill_some_child(struct child *firstborn)
> -static void check_dead_children(void)
> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
> + log_fn loginfo)
Those values are provided by the callers in 'daemon.c'. The major change
here is that 'live_children' is passed as a pointer, since its value is
updated by difference is passing 'live_children' as a pointer, since its
value is updated by 'check_dead_children()' and 'add_child()':
> @@ -879,9 +797,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> struct child_process cld = CHILD_PROCESS_INIT;
>
> if (max_connections && live_children >= max_connections) {
> - kill_some_child();
> + kill_some_child(firstborn);
> sleep(1); /* give it some time to die */
> - check_dead_children();
> + check_dead_children(firstborn, &live_children, loginfo);
> if (live_children >= max_connections) {
> close(incoming);
> logerror("Too many children, dropping connection");
> @@ -914,7 +832,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> if (start_command(&cld))
> logerror("unable to fork");
> else
> - add_child(&cld, addr, addrlen);
> + add_child(&cld, addr, addrlen, firstborn, &live_children);
> }
>
> static void child_handler(int signo)
> @@ -944,7 +862,7 @@ static int service_loop(struct socketlist *socklist)
> for (;;) {
> int i;
>
> - check_dead_children();
> + check_dead_children(firstborn, &live_children, loginfo);
>
> if (poll(pfd, socklist->nr, -1) < 0) {
> if (errno != EINTR) {
However, I think that change to 'live_children' may have caused a bug. In
'check_dead_children()', you decrement the 'live_children' *pointer*. That
changes its address, not its value:
> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
> + log_fn loginfo)
> +{
...
> + live_children--;
...
> +}
Same thing in 'add_child()':
> +void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
> + struct child *firstborn , unsigned int *live_children)
> +{
...
> + live_children++;
...
> +}
These should be changed to '(*live_children)--' and '(*live_children)++',
respectively.
There's also one minor functional change in 'check_dead_children()', where
an 'if (loginfo)' check is added guarding the call to 'loginfo()':
> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
> + log_fn loginfo)
> +{
...
> + if (loginfo) {
> + const char *dead = "";
> + if (status)
> + dead = " (with error)";
> + loginfo("[%"PRIuMAX"] Disconnected%s",
> + (uintmax_t)pid, dead);
> + }
...
> +}
I'm guessing this is done because a caller later in the series won't provide
a 'loginfo', but if that's the case, it would help to note that in this
patch's commit message.
The one other thing I noticed is that you removed the function documentation
for 'kill_some_child()':
> -/*
> - * This gets called if the number of connections grows
> - * past "max_connections".
> - *
> - * We kill the newest connection from a duplicate IP.
> - */
Is there a reason why you removed it? Otherwise, it should be added back in
- probably in 'daemon-utils.h'?
Everything else here looks good.
