Re: [PATCH v5 02/10] daemon: libify child process handling functions
- From
Victoria Dye <vdye@github.com>
- Date
- Jan 12, 2023, 19:35 UTC
- Message-ID
- <3a8d1b66-ed06-16a3-5459-9381faa69420@github.com>
- In-Reply-To
- <bc972fc8d3d3a028d3d160aac354d2a13bad37ae.1673475190.git.gitgitgadget@gmail.com>
Matthew John Cheetham via GitGitGadget wrote:
Show 5 quoted lines
> 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()':
Show 30 quoted lines
> @@ -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)
> +{...
Show 7 quoted lines
> + 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()':
Show 6 quoted lines
> -/* > - * 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.