Subject: Re: [PATCH v5 02/10] daemon: libify child process handling functions

From: Victoria Dye <[email-masked]>
Date: Thu, 12 Jan 2023 11:35:25 -0800

Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <[email-masked]>
> 
> 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.