diff mbox series

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

Message ID bc972fc8d3d3a028d3d160aac354d2a13bad37ae.1673475190.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enhance credential helper protocol to include auth headers | expand

Commit Message

Matthew John Cheetham Jan. 11, 2023, 10:13 p.m. UTC 
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.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 daemon-utils.c | 77 ++++++++++++++++++++++++++++++++++++++++++
 daemon-utils.h | 15 ++++++++
 daemon.c       | 92 +++-----------------------------------------------
 3 files changed, 97 insertions(+), 87 deletions(-)

Comments

Victoria Dye Jan. 12, 2023, 7:35 p.m. UTC | #1 
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'?

Everything else here looks good.
Matthew John Cheetham Jan. 17, 2023, 9:14 p.m. UTC | #2 
On 2023-01-12 11:35, Victoria Dye wrote:

> 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.

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
diff mbox series

Patch

diff --git a/daemon-utils.c b/daemon-utils.c
index b96b55962db..3804bc60973 100644
--- a/daemon-utils.c
+++ b/daemon-utils.c
@@ -207,3 +207,80 @@  void socksetup(struct string_list *listen_addr, int listen_port,
 		}
 	}
 }
+
+static int addrcmp(const struct sockaddr_storage *s1,
+    const struct sockaddr_storage *s2)
+{
+	const struct sockaddr *sa1 = (const struct sockaddr*) s1;
+	const struct sockaddr *sa2 = (const struct sockaddr*) s2;
+
+	if (sa1->sa_family != sa2->sa_family)
+		return sa1->sa_family - sa2->sa_family;
+	if (sa1->sa_family == AF_INET)
+		return memcmp(&((struct sockaddr_in *)s1)->sin_addr,
+		    &((struct sockaddr_in *)s2)->sin_addr,
+		    sizeof(struct in_addr));
+#ifndef NO_IPV6
+	if (sa1->sa_family == AF_INET6)
+		return memcmp(&((struct sockaddr_in6 *)s1)->sin6_addr,
+		    &((struct sockaddr_in6 *)s2)->sin6_addr,
+		    sizeof(struct in6_addr));
+#endif
+	return 0;
+}
+
+void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
+	       struct child *firstborn , unsigned int *live_children)
+{
+	struct child *newborn, **cradle;
+
+	CALLOC_ARRAY(newborn, 1);
+	live_children++;
+	memcpy(&newborn->cld, cld, sizeof(*cld));
+	memcpy(&newborn->address, addr, addrlen);
+	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
+		if (!addrcmp(&(*cradle)->address, &newborn->address))
+			break;
+	newborn->next = *cradle;
+	*cradle = newborn;
+}
+
+void kill_some_child(struct child *firstborn)
+{
+	const struct child *blanket, *next;
+
+	if (!(blanket = firstborn))
+		return;
+
+	for (; (next = blanket->next); blanket = next)
+		if (!addrcmp(&blanket->address, &next->address)) {
+			kill(blanket->cld.pid, SIGTERM);
+			break;
+		}
+}
+
+void check_dead_children(struct child *firstborn, unsigned int *live_children,
+			 log_fn loginfo)
+{
+	int status;
+	pid_t pid;
+
+	struct child **cradle, *blanket;
+	for (cradle = &firstborn; (blanket = *cradle);)
+		if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
+			if (loginfo) {
+				const char *dead = "";
+				if (status)
+					dead = " (with error)";
+				loginfo("[%"PRIuMAX"] Disconnected%s",
+					(uintmax_t)pid, dead);
+			}
+
+			/* remove the child */
+			*cradle = blanket->next;
+			live_children--;
+			child_process_clear(&blanket->cld);
+			free(blanket);
+		} else
+			cradle = &blanket->next;
+}
diff --git a/daemon-utils.h b/daemon-utils.h
index 6710a2a6dc0..fe8d9d05256 100644
--- a/daemon-utils.h
+++ b/daemon-utils.h
@@ -2,6 +2,7 @@ 
 #define DAEMON_UTILS_H
 
 #include "git-compat-util.h"
+#include "run-command.h"
 #include "string-list.h"
 
 typedef void (*log_fn)(const char *msg, ...);
@@ -20,4 +21,18 @@  void socksetup(struct string_list *listen_addr, int listen_port,
 	       struct socketlist *socklist, int reuseaddr,
 	       log_fn logerror);
 
+struct child {
+	struct child *next;
+	struct child_process cld;
+	struct sockaddr_storage address;
+};
+
+void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
+	       struct child *firstborn, unsigned int *live_children);
+
+void kill_some_child(struct child *firstborn);
+
+void check_dead_children(struct child *firstborn, unsigned int *live_children,
+			 log_fn loginfo);
+
 #endif
diff --git a/daemon.c b/daemon.c
index 1ed4e705680..ec3b407ecbc 100644
--- a/daemon.c
+++ b/daemon.c
@@ -785,93 +785,11 @@  static int execute(void)
 	return -1;
 }
 
-static int addrcmp(const struct sockaddr_storage *s1,
-    const struct sockaddr_storage *s2)
-{
-	const struct sockaddr *sa1 = (const struct sockaddr*) s1;
-	const struct sockaddr *sa2 = (const struct sockaddr*) s2;
-
-	if (sa1->sa_family != sa2->sa_family)
-		return sa1->sa_family - sa2->sa_family;
-	if (sa1->sa_family == AF_INET)
-		return memcmp(&((struct sockaddr_in *)s1)->sin_addr,
-		    &((struct sockaddr_in *)s2)->sin_addr,
-		    sizeof(struct in_addr));
-#ifndef NO_IPV6
-	if (sa1->sa_family == AF_INET6)
-		return memcmp(&((struct sockaddr_in6 *)s1)->sin6_addr,
-		    &((struct sockaddr_in6 *)s2)->sin6_addr,
-		    sizeof(struct in6_addr));
-#endif
-	return 0;
-}
-
 static int max_connections = 32;
 
 static unsigned int live_children;
 
-static struct child {
-	struct child *next;
-	struct child_process cld;
-	struct sockaddr_storage address;
-} *firstborn;
-
-static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)
-{
-	struct child *newborn, **cradle;
-
-	CALLOC_ARRAY(newborn, 1);
-	live_children++;
-	memcpy(&newborn->cld, cld, sizeof(*cld));
-	memcpy(&newborn->address, addr, addrlen);
-	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
-		if (!addrcmp(&(*cradle)->address, &newborn->address))
-			break;
-	newborn->next = *cradle;
-	*cradle = newborn;
-}
-
-/*
- * This gets called if the number of connections grows
- * past "max_connections".
- *
- * We kill the newest connection from a duplicate IP.
- */
-static void kill_some_child(void)
-{
-	const struct child *blanket, *next;
-
-	if (!(blanket = firstborn))
-		return;
-
-	for (; (next = blanket->next); blanket = next)
-		if (!addrcmp(&blanket->address, &next->address)) {
-			kill(blanket->cld.pid, SIGTERM);
-			break;
-		}
-}
-
-static void check_dead_children(void)
-{
-	int status;
-	pid_t pid;
-
-	struct child **cradle, *blanket;
-	for (cradle = &firstborn; (blanket = *cradle);)
-		if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
-			const char *dead = "";
-			if (status)
-				dead = " (with error)";
-			loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
-
-			/* remove the child */
-			*cradle = blanket->next;
-			live_children--;
-			child_process_clear(&blanket->cld);
-			free(blanket);
-		} else
-			cradle = &blanket->next;
-}
+static struct child *firstborn;
 
 static struct strvec cld_argv = STRVEC_INIT;
 static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
@@ -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) {