Re: [PATCH v2 2/3] daemon: use sigaction() to install child_handler()

[Junio C Hamano <gitster@xxxxxxxxx>]

Junio C Hamano <gitster@xxxxxxxxx> writes:

> Does this #ifndef block with #else lack its #endif probably before
> the service_loop() is defined ...
> ...
> ... around here?
>
> I am wondering if it would make it even cleaner to also define
> rearm_signal_handler() in this "if sigaction() can be used, do this,
> otherwise do that" block, and move the whole thing a bit earlier in
> this file.  That way, the primary code paths do not have to see much
> of the #ifdef conditionals.  With IPV6 related #ifdef noise already
> contaminating the file, it may not be a huge deal, but these crufts
> tend to build up unless we tightly control them with discipline.

Applying this on top of your series would illustrate what I meant.

I didn't spend enough braincycles on the naming issues for the
conditional compilation so I left it as USE_NON_POSIX_SIGNAL even
though I know it has to be fixed before we move forward.

Thanks.

 daemon.c | 81 +++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 39 deletions(-)

diff --git c/daemon.c w/daemon.c
index 01337fcfed..8a371518b8 100644
--- c/daemon.c
+++ w/daemon.c
@@ -912,19 +912,54 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 		add_child(&cld, addr, addrlen);
 }
 
-static void child_handler(int signo MAYBE_UNUSED)
+#ifndef USE_NON_POSIX_SIGNAL
+
+static void set_signal_handler(struct sigaction *psa, void (*child_handler)(int))
+{
+	sigemptyset(&psa->sa_mask);
+	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
+	psa->sa_handler = child_handler;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
+static void rearm_signal_handler(int signo UNUSED, void (*child_handler)(int) UNUSED)
+{
+}
+
+static void set_sa_restart(struct sigaction *psa, int enable)
+{
+	if (enable)
+		psa->sa_flags |= SA_RESTART;
+	else
+		psa->sa_flags &= ~SA_RESTART;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
+#else
+
+static void set_signal_handler(struct sigaction *psa UNUSED, void (*child_handler)(int))
+{
+	signal(SIGCHLD, child_handler);
+}
+
+static void rearm_signal_handler(int signo, void (*child_handler)(int))
 {
-	/*
-	 * Otherwise empty handler because systemcalls should get interrupted
-	 * upon signal receipt.
-	 */
-#ifdef USE_NON_POSIX_SIGNAL
 	/*
 	 * SysV needs the handler to be rearmed, but this is known
 	 * to trigger infinite recursion crashes at least in AIX.
 	 */
 	signal(signo, child_handler);
+}
+
+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
+{
+}
+
 #endif
+
+static void child_handler(int signo)
+{
+	rearm_signal_handler(signo, child_handler);
 }
 
 static int set_reuse_addr(int sockfd)
@@ -1123,38 +1158,6 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 	}
 }
 
-#ifndef USE_NON_POSIX_SIGNAL
-
-static void set_signal_handler(struct sigaction *psa)
-{
-	sigemptyset(&psa->sa_mask);
-	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
-	psa->sa_handler = child_handler;
-	sigaction(SIGCHLD, psa, NULL);
-}
-
-static void set_sa_restart(struct sigaction *psa, int enable)
-{
-	if (enable)
-		psa->sa_flags |= SA_RESTART;
-	else
-		psa->sa_flags &= ~SA_RESTART;
-	sigaction(SIGCHLD, psa, NULL);
-}
-
-#else
-
-static void set_signal_handler(struct sigaction *psa UNUSED)
-{
-	signal(SIGCHLD, child_handler);
-}
-
-static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
-{
-}
-
-#endif
-
 static int service_loop(struct socketlist *socklist)
 {
 	struct sigaction sa;
@@ -1167,7 +1170,7 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	set_signal_handler(&sa);
+	set_signal_handler(&sa, child_handler);
 
 	for (;;) {
 		check_dead_children();