On 2023-01-12 11:35, Victoria Dye wrote:
> 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.
Ah! You are correct; my bad. Will correct this in v6.
> 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.
Will call this out in the commit message in v6.
> 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'?
I removed it initially as it was referencing things like `max_connections`
which no longer existed in the context of `daemon-utils.{c,h}`.
Next iteration I can restore the spirit of the comment, that this should be
called when the maximimum number of connections has been reached, in order
to kill the newest connection from a duplicate IP.
> Everything else here looks good.
Thanks,
Matthew
