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

[Junio C Hamano <gitster@pobox.com>]

"Carlo Marcelo Arenas Belón via GitGitGadget"
<gitgitgadget@gmail.com> writes:

> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
>
> In a future change, the flags used for processing SIGCHLD will need to
> be updated, which is only possible by using sigaction().
>
> Factor out the call to set the signal handler and use sigaction instead
> of signal for the systems that allow that, which has the added benefit
> of using BSD semantics reliably and therefore not needing the rearming
> call.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  daemon.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index d1be61fd5789..8133bd902157 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -912,14 +912,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>  		add_child(&cld, addr, addrlen);
>  }
>  
> -static void child_handler(int signo UNUSED)
> +static void child_handler(int signo MAYBE_UNUSED)
>  {
>  	/*
> -	 * Otherwise empty handler because systemcalls will get interrupted
> -	 * upon signal receipt
> -	 * SysV needs the handler to be rearmed
> +	 * Otherwise empty handler because systemcalls should get interrupted
> +	 * upon signal receipt.
>  	 */
> -	signal(SIGCHLD, child_handler);
> +#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);
> +#endif
>  }
>  
>  static int set_reuse_addr(int sockfd)
> @@ -1118,8 +1123,26 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>  	}
>  }
>  
> +#ifndef USE_NON_POSIX_SIGNAL

Does this #ifndef block with #else lack its #endif probably before
the service_loop() is defined ...

> +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);
> +}
> +
> +#else
> +
> +static void set_signal_handler(struct sigaction *psa UNUSED)
> +{
> +	signal(SIGCHLD, child_handler);
> +}

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

>  static int service_loop(struct socketlist *socklist)
>  {
> +	struct sigaction sa;
>  	struct pollfd *pfd;
>  
>  	CALLOC_ARRAY(pfd, socklist->nr);
> @@ -1129,7 +1152,7 @@ static int service_loop(struct socketlist *socklist)
>  		pfd[i].events = POLLIN;
>  	}
>  
> -	signal(SIGCHLD, child_handler);
> +	set_signal_handler(&sa);
>  
>  	for (;;) {
>  		check_dead_children();