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

> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> 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'?