patch, 3 partsdaemon: explicitly allow EINTR during poll()

> Consolidate that, so it will be easier to use in a future commit.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  compat/posix.h   | 7 +++++++
>  config.mak.uname | 3 ---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/compat/posix.h b/compat/posix.h
> index 067a00f33b83..2612a8515897 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -250,6 +250,13 @@ char *gitdirname(char *);
>  #define NAME_MAX 255
>  #endif
>  
> +/* On most systems <signal.h> would have given us this, but
> + * not on some systems (e.g. NonStop, QNX).
> + */
> +#ifndef SA_RESTART
> +#define SA_RESTART 0	/* disabled for sigaction() */
> +#endif

>  typedef uintmax_t timestamp_t;
>  #define PRItime PRIuMAX
>  #define parse_timestamp strtoumax
> diff --git a/config.mak.uname b/config.mak.uname
> index b1c5c4d5e8ed..52160ef5cb07 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -654,8 +654,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  
>  	# Not detected (nor checked for) by './configure'.
> -	# We don't have SA_RESTART on NonStop, unfortunalety.
> -	COMPAT_CFLAGS += -DSA_RESTART=0
>  	# Apparently needed in compat/fnmatch/fnmatch.c.
>  	COMPAT_CFLAGS += -DHAVE_STRING_H=1
>  	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
> @@ -782,7 +780,6 @@ ifeq ($(uname_S),MINGW)
>          endif
>  endif
>  ifeq ($(uname_S),QNX)
> -	COMPAT_CFLAGS += -DSA_RESTART=0
>  	EXPAT_NEEDS_XMLPARSE_H = YesPlease
>  	HAVE_STRINGS_H = YesPlease
>  	NEEDS_SOCKET = YesPlease

> 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().
>
> Replace the call, which hs 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index d1be61fd5789..d870ad2f63c1 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)
>  	/*
>  	 * Otherwise empty handler because systemcalls will get interrupted
>  	 * upon signal receipt
> -	 * SysV needs the handler to be rearmed
>  	 */
> -	signal(SIGCHLD, child_handler);
>  }
>  
>  static int set_reuse_addr(int sockfd)
> @@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>  static int service_loop(struct socketlist *socklist)
>  {
>  	struct pollfd *pfd;
> +	struct sigaction sa;
>  
>  	CALLOC_ARRAY(pfd, socklist->nr);
>  
> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
>  		pfd[i].events = POLLIN;
>  	}
>  
> -	signal(SIGCHLD, child_handler);
> +	sigemptyset(&sa.sa_mask);
> +	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> +	sa.sa_handler = child_handler;
> +	sigaction(SIGCHLD, &sa, NULL);
>  
>  	for (;;) {
>  		check_dead_children();

> "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().
> >
> > Replace the call, which hs the added benefit of using BSD semantics
> > reliably and therefore not needing the rearming call.
> 
> "hs" -> "has"
> 
> Hmph, if we do not have to rearm, do we even need to have the
> handler at all, now it is a completely empty function?  Presumably
> we'll see the answer to this question in the next step?

> 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().
>
> Replace the call, which hs 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

> diff --git a/daemon.c b/daemon.c
> index d1be61fd5789..d870ad2f63c1 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)
>  	/*
>  	 * Otherwise empty handler because systemcalls will get interrupted
>  	 * upon signal receipt
> -	 * SysV needs the handler to be rearmed
>  	 */
> -	signal(SIGCHLD, child_handler);
>  }
>  
>  static int set_reuse_addr(int sockfd)
> @@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>  static int service_loop(struct socketlist *socklist)
>  {
>  	struct pollfd *pfd;
> +	struct sigaction sa;
>  
>  	CALLOC_ARRAY(pfd, socklist->nr);
>  
> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
>  		pfd[i].events = POLLIN;
>  	}
>  
> -	signal(SIGCHLD, child_handler);
> +	sigemptyset(&sa.sa_mask);
> +	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> +	sa.sa_handler = child_handler;
> +	sigaction(SIGCHLD, &sa, NULL);
>  
>  	for (;;) {
>  		check_dead_children();

> "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().
> >
> > Replace the call, which hs 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 | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Hmph.  Wouldn't it a much smaller change and fix to discard 2/3 and
> most of the 3/3 and instead make a siginterrupt() call to tell the
> system to interrupt us when SIGCHLD is received only on platforms
> where siginterrupt() is available?  Use of sigaction() does not seem
> to be buying us anything for the purpose of this series.

>> Hmph.  Wouldn't it a much smaller change and fix to discard 2/3 and
>> most of the 3/3 and instead make a siginterrupt() call to tell the
>> system to interrupt us when SIGCHLD is received only on platforms
>> where siginterrupt() is available?  Use of sigaction() does not seem
>> to be buying us anything for the purpose of this series.
>
> Using siginterrupt() would work (at least it did when I tested it in
> OpenBSD), but its use is discouraged as it has been obsoleted by the
> last two versions of POSIX (since 2018).

> Most systems seem to be implementing `signal()` with `sigaction()`
> nowadays, but in the ones that are not (ex: Solaris) calling the later
> to get a `struct sigaction` with the flags being used, doesn't work
> and therefore it would seem, that the only way to do this reliably is
> by using sigaction everywhere for this signal, as implemented in 2/3.

> -static void child_handler(int signo UNUSED)
> +static void child_handler(int signo)
>  {
>  	/*
> -	 * Otherwise empty handler because systemcalls will get interrupted
> -	 * upon signal receipt
> +	 * Empty handler because systemcalls should get interrupted
> +	 * upon signal receipt.
>  	 */
> +#ifdef NO_SIGINTERRUPT
> +	/* SysV needs the handler to be rearmed */
> +	signal(signo, child_handler);
> +#endif
>  }

>  static int set_reuse_addr(int sockfd)
> @@ -1118,8 +1122,10 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>  
>  static int service_loop(struct socketlist *socklist)
>  {
> -	struct pollfd *pfd;
> +#ifndef NO_SIGINTERRUPT
>  	struct sigaction sa;
> +#endif
> +	struct pollfd *pfd;
>  	CALLOC_ARRAY(pfd, socklist->nr);
>  
> @@ -1128,14 +1134,22 @@ static int service_loop(struct socketlist *socklist)
>  		pfd[i].events = POLLIN;
>  	}
>  
> +#ifdef NO_SIGINTERRUPT
> +	signal(SIGCHLD, child_handler);
> +#else
>  	sigemptyset(&sa.sa_mask);
>  	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
>  	sa.sa_handler = child_handler;
>  	sigaction(SIGCHLD, &sa, NULL);
> +#endif
>  
>  	for (;;) {
>  		check_dead_children();
>  
> +#ifndef NO_SIGINTERRUPT
> +		sa.sa_flags &= ~SA_RESTART;
> +		sigaction(SIGCHLD, &sa, NULL);
> +#endif
>  		if (poll(pfd, socklist->nr, -1) < 0) {
>  			if (errno != EINTR) {
>  				logerror("Poll failed, resuming: %s",
> @@ -1144,6 +1158,10 @@ static int service_loop(struct socketlist *socklist)
>  			}
>  			continue;
>  		}
> +#ifndef NO_SIGINTERRUPT
> +		sa.sa_flags |= SA_RESTART;
> +		sigaction(SIGCHLD, &sa, NULL);
> +#endif

> +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> +# prefer using ANSI C signal() over POSIX sigaction()
> +
> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> +	AC_COMPILE_IFELSE(
> +		[AC_LANG_PROGRAM([#include <signal.h>], [[
> +		#ifdef SA_RESTART
> +		#endif
> +		siginterrupt(SIGCHLD, 1)

> +		]])],[ac_cv_siginterrupt=yes],[
> +			ac_cv_siginterrupt=no
> +			USE_NON_POSIX_SIGNAL=UnfortunatelyYes
> +		]
> +	)
> +])
> +GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])

> "Carlo Marcelo Arenas Belón via GitGitGadget"
> <gitgitgadget@gmail.com> writes:
> 
> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> > +# prefer using ANSI C signal() over POSIX sigaction()
> > +
> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> > +	AC_COMPILE_IFELSE(
> > +		[AC_LANG_PROGRAM([#include <signal.h>], [[
> > +		#ifdef SA_RESTART
> > +		#endif
> > +		siginterrupt(SIGCHLD, 1)
> 
> This is curious.  What is this #ifdef/#endif doing that does not
> have anything in it?

> On Wed, Jun 25, 2025 at 09:07:15AM -0800, Junio C Hamano wrote:
>> "Carlo Marcelo Arenas Belón via GitGitGadget"
>> <gitgitgadget@gmail.com> writes:
>> 
>> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>> > +# prefer using ANSI C signal() over POSIX sigaction()
>> > +
>> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
>> > +	AC_COMPILE_IFELSE(
>> > +		[AC_LANG_PROGRAM([#include <signal.h>], [[
>> > +		#ifdef SA_RESTART
>> > +		#endif
>> > +		siginterrupt(SIGCHLD, 1)
>> 
>> This is curious.  What is this #ifdef/#endif doing that does not
>> have anything in it?
>
> It checks that `SA_RESTART` is defined in `signal.h`, which should
> fail at least in QNX, NonStop and Windows.

> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> 
> > On Wed, Jun 25, 2025 at 09:07:15AM -0800, Junio C Hamano wrote:
> >> "Carlo Marcelo Arenas Belón via GitGitGadget"
> >> <gitgitgadget@gmail.com> writes:
> >> 
> >> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> >> > +# prefer using ANSI C signal() over POSIX sigaction()
> >> > +
> >> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> >> > +	AC_COMPILE_IFELSE(
> >> > +		[AC_LANG_PROGRAM([#include <signal.h>], [[
> >> > +		#ifdef SA_RESTART
> >> > +		#endif
> >> > +		siginterrupt(SIGCHLD, 1)
> >> 
> >> This is curious.  What is this #ifdef/#endif doing that does not
> >> have anything in it?
> >
> > It checks that `SA_RESTART` is defined in `signal.h`, which should
> > fail at least in QNX, NonStop and Windows.
> 
> The above roughly expands to
> 
>         #include <signal.h>
>         int main(void)
>         {
>                 #ifdef SA_RESTART
>                 #endif
>                 siginterrupt(SIGCHLD, 1);
>                 return 0;
>         }
> 
> Are you saying that a preprocessor macro SA_RESTART, which may or
> may not be defined, when asked by "#ifdef", causes what is left in
> the preprocessed source change in any meaningful way to cause the
> compilation to fail?

> 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

> +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);
> +}

>  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();

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

> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
>
> If the setup for the SIGCHLD signal handler sets SA_RESTART, poll()
> might not return with -1 and set errno to EINTR when a signal is
> received.
>
> Since the logic to reap zombie childs relies on those interruptions
> make sure to explicitly disable SA_RESTART around this function.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  daemon.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/daemon.c b/daemon.c
> index 8133bd902157..01337fcfedab 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1133,6 +1133,15 @@ static void set_signal_handler(struct sigaction *psa)
>  	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)
> @@ -1140,6 +1149,12 @@ 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)

> The underlying problem is that when this code was originally introduced,
> SA_RESTART was not widely implemented, and the signal() call usually
> implemented SysV like semantics, at least until it started being
> reimplemented by calling sigaction() internally.
> 
> Changes since v1
> 
>   * Almost all references to siginterrupt has been removed and a better named
>     variable used instead
>   * Changes had been anstracted to minimize ifdefs and their introduction
>     staged more naturally
> 
> Carlo Marcelo Arenas Belón (3):
>    compat/posix.h: track SA_RESTART fallback
>    daemon: use sigaction() to install child_handler()
>    daemon: explicitly allow EINTR during poll()
> 
>   Makefile             |  6 +++++
>   compat/mingw-posix.h |  1 -
>   compat/posix.h       |  8 +++++++
>   config.mak.uname     |  7 +++---
>   configure.ac         | 17 +++++++++++++++
>   daemon.c             | 52 +++++++++++++++++++++++++++++++++++++++-----
>   meson.build          |  4 ++++
>   7 files changed, 85 insertions(+), 10 deletions(-)
> 
> 
> base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v2
> Pull-Request: https://github.com/git/git/pull/2002
> 
> Range-diff vs v1:
> 
>   1:  2b5a58e53ac ! 1:  e82b7425bbc compat/posix.h: track SA_RESTART fallback
>       @@ Metadata
>         ## Commit message ##
>            compat/posix.h: track SA_RESTART fallback
>        
>       -    Systems without SA_RESTART where using custom CFLAGS instead of
>       -    the standard header file.
>       +    Systems without SA_RESTART are using custom CFLAGS or headers
>       +    instead of the standard header file.
>        
>       -    Consolidate that, so it will be easier to use in a future commit.
>       +    Correct that, and invent a Makefile variable to track the
>       +    exceptions which will become handy in the next commits.
>        
>            Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>        
>       + ## Makefile ##
>       +@@ Makefile: include shared.mak
>       + # when attempting to read from an fopen'ed directory (or even to fopen
>       + # it at all).
>       + #
>       ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>       ++# prefer to use ANSI C signal() over POSIX sigaction()
>       ++#
>       + # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
>       + # when a signal is received (as opposed to restarting).
>       + #
>       +@@ Makefile: ifdef FREAD_READS_DIRECTORIES
>       + 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
>       + 	COMPAT_OBJS += compat/fopen.o
>       + endif
>       ++ifdef USE_NON_POSIX_SIGNAL
>       ++	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
>       ++endif
>       + ifdef OPEN_RETURNS_EINTR
>       + 	COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
>       + endif
>       +
>       + ## compat/mingw-posix.h ##
>       +@@ compat/mingw-posix.h: struct sigaction {
>       + 	sig_handler_t sa_handler;
>       + 	unsigned sa_flags;
>       + };
>       +-#define SA_RESTART 0
>       +
>       + struct itimerval {
>       + 	struct timeval it_value, it_interval;
>       +
>         ## compat/posix.h ##
>        @@ compat/posix.h: char *gitdirname(char *);
>         #define NAME_MAX 255
>         #endif
>         
>       -+/* On most systems <signal.h> would have given us this, but
>       ++/*
>       ++ * On most systems <signal.h> would have given us this, but
>        + * not on some systems (e.g. NonStop, QNX).
>        + */
>        +#ifndef SA_RESTART
>       -+#define SA_RESTART 0	/* disabled for sigaction() */
>       ++# define SA_RESTART 0	/* disabled for sigaction() */
>        +#endif
>        +
>         typedef uintmax_t timestamp_t;
>       @@ compat/posix.h: char *gitdirname(char *);
>         #define parse_timestamp strtoumax
>        
>         ## config.mak.uname ##
>       +@@ config.mak.uname: ifeq ($(uname_S),Windows)
>       + 	NO_STRTOUMAX = YesPlease
>       + 	NO_MKDTEMP = YesPlease
>       + 	NO_INTTYPES_H = YesPlease
>       ++	USE_NON_POSIX_SIGNAL = YesPlease
>       + 	CSPRNG_METHOD = rtlgenrandom
>       + 	# VS2015 with UCRT claims that snprintf and friends are C99 compliant,
>       + 	# so we don't need this:
>        @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
>         	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>         
>       @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
>         	# Apparently needed in compat/fnmatch/fnmatch.c.
>         	COMPAT_CFLAGS += -DHAVE_STRING_H=1
>         	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
>       +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
>       + 	NO_MMAP = YesPlease
>       + 	NO_POLL = YesPlease
>       + 	NO_INTPTR_T = UnfortunatelyYes
>       ++	USE_NON_POSIX_SIGNAL = UnfortunatelyYes
>       + 	CSPRNG_METHOD = openssl
>       + 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
>       + 	SHELL_PATH = /usr/coreutils/bin/bash
>       +@@ config.mak.uname: ifeq ($(uname_S),MINGW)
>       + 	NEEDS_LIBICONV = YesPlease
>       + 	NO_STRTOUMAX = YesPlease
>       + 	NO_MKDTEMP = YesPlease
>       ++	USE_NON_POSIX_SIGNAL = YesPlease
>       + 	NO_SVN_TESTS = YesPlease
>       +
>       + 	# The builtin FSMonitor requires Named Pipes and Threads on Windows.
>        @@ config.mak.uname: ifeq ($(uname_S),MINGW)
>                 endif
>         endif
>       @@ config.mak.uname: ifeq ($(uname_S),MINGW)
>         	EXPAT_NEEDS_XMLPARSE_H = YesPlease
>         	HAVE_STRINGS_H = YesPlease
>         	NEEDS_SOCKET = YesPlease
>       +@@ config.mak.uname: ifeq ($(uname_S),QNX)
>       + 	NO_PTHREADS = YesPlease
>       + 	NO_STRCASESTR = YesPlease
>       + 	NO_STRLCPY = YesPlease
>       ++	USE_NON_POSIX_SIGNAL = UnfortunatelyYes
>       + endif
>       +
>       + ## configure.ac ##
>       +@@ configure.ac: fi
>       + GIT_CONF_SUBST([ICONV_OMITS_BOM])
>       + fi
>       +
>       ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>       ++# prefer using ANSI C signal() over POSIX sigaction()
>       ++
>       ++AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
>       ++	AC_COMPILE_IFELSE(
>       ++		[AC_LANG_PROGRAM([#include <signal.h>], [[
>       ++		#ifdef SA_RESTART
>       ++		#endif
>       ++		siginterrupt(SIGCHLD, 1)
>       ++		]])],[ac_cv_siginterrupt=yes],[
>       ++			ac_cv_siginterrupt=no
>       ++			USE_NON_POSIX_SIGNAL=UnfortunatelyYes
>       ++		]
>       ++	)
>       ++])
>       ++GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])
>       ++
>       + ## Checks for typedefs, structures, and compiler characteristics.
>       + AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
>       + #
>       +
>       + ## meson.build ##
>       +@@ meson.build: else
>       +   build_options_config.set('NO_EXPAT', '1')
>       + endif
>       +
>       ++if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
>       ++  libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
>       ++endif
>       ++
>       + if not compiler.has_header('sys/select.h')
>       +   libgit_c_args += '-DNO_SYS_SELECT_H'
>       + endif
>   2:  2e8c4643a60 ! 2:  05d945aa1e5 daemon: use sigaction() to install child_handler()
>       @@ Commit message
>            In a future change, the flags used for processing SIGCHLD will need to
>            be updated, which is only possible by using sigaction().
>        
>       -    Replace the call, which hs the added benefit of using BSD semantics
>       -    reliably and therefore not needing the rearming call.
>       +    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 ##
>       -@@ daemon.c: static void child_handler(int signo UNUSED)
>       +@@ daemon.c: 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
>       +-	 * 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)
>        @@ daemon.c: 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);
>       ++}
>       ++
>       ++#else
>       ++
>       ++static void set_signal_handler(struct sigaction *psa UNUSED)
>       ++{
>       ++	signal(SIGCHLD, child_handler);
>       ++}
>       ++
>         static int service_loop(struct socketlist *socklist)
>         {
>       - 	struct pollfd *pfd;
>        +	struct sigaction sa;
>       + 	struct pollfd *pfd;
>         
>         	CALLOC_ARRAY(pfd, socklist->nr);
>       -
>        @@ daemon.c: static int service_loop(struct socketlist *socklist)
>         		pfd[i].events = POLLIN;
>         	}
>         
>        -	signal(SIGCHLD, child_handler);
>       -+	sigemptyset(&sa.sa_mask);
>       -+	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
>       -+	sa.sa_handler = child_handler;
>       -+	sigaction(SIGCHLD, &sa, NULL);
>       ++	set_signal_handler(&sa);
>         
>         	for (;;) {
>         		check_dead_children();
>   3:  a450bdb0066 ! 3:  b737e0389df daemon: explicitly allow EINTR during poll()
>       @@ Commit message
>            might not return with -1 and set errno to EINTR when a signal is
>            received.
>        
>       -    Since the logic to reap zombie childs relies om those interruptions
>       +    Since the logic to reap zombie childs relies on those interruptions
>            make sure to explicitly disable SA_RESTART around this function.
>        
>       -    Add a Makefile flag for portability to systems that don't have the
>       -    functionality to change those flags or where it is not needed.
>       -
>            Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>        
>       - ## Makefile ##
>       -@@ Makefile: include shared.mak
>       - # Define NO_PREAD if you have a problem with pread() system call (e.g.
>       - # cygwin1.dll before v1.5.22).
>       - #
>       -+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART
>       -+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.
>       -+#
>       - # Define NO_SETITIMER if you don't have setitimer()
>       - #
>       - # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
>       -@@ Makefile: ifdef NO_PREAD
>       - 	COMPAT_CFLAGS += -DNO_PREAD
>       - 	COMPAT_OBJS += compat/pread.o
>       - endif
>       -+ifdef NO_SIGINTERRUPT
>       -+	COMPAT_CFLAGS += -DNO_SIGINTERRUPT
>       -+endif
>       - ifdef NO_FAST_WORKING_DIRECTORY
>       - 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
>       - endif
>       -
>       - ## config.mak.uname ##
>       -@@ config.mak.uname: ifeq ($(uname_S),Windows)
>       - 	NO_STRTOUMAX = YesPlease
>       - 	NO_MKDTEMP = YesPlease
>       - 	NO_INTTYPES_H = YesPlease
>       -+	NO_SIGINTERRUPT = YesPlease
>       - 	CSPRNG_METHOD = rtlgenrandom
>       - 	# VS2015 with UCRT claims that snprintf and friends are C99 compliant,
>       - 	# so we don't need this:
>       -@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
>       - 	NO_PREAD = YesPlease
>       - 	NO_MMAP = YesPlease
>       - 	NO_POLL = YesPlease
>       -+	NO_SIGINTERRUPT = UnfortunatelyYes
>       - 	NO_INTPTR_T = UnfortunatelyYes
>       - 	CSPRNG_METHOD = openssl
>       - 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
>       -@@ config.mak.uname: ifeq ($(uname_S),MINGW)
>       - 	NEEDS_LIBICONV = YesPlease
>       - 	NO_STRTOUMAX = YesPlease
>       - 	NO_MKDTEMP = YesPlease
>       -+	NO_SIGINTERRUPT = YesPlease
>       - 	NO_SVN_TESTS = YesPlease
>       -
>       - 	# The builtin FSMonitor requires Named Pipes and Threads on Windows.
>       -@@ config.mak.uname: ifeq ($(uname_S),QNX)
>       - 	NO_PTHREADS = YesPlease
>       - 	NO_STRCASESTR = YesPlease
>       - 	NO_STRLCPY = YesPlease
>       -+	NO_SIGINTERRUPT = UnfortunatelyYes
>       - endif
>       -
>       - ## configure.ac ##
>       -@@ configure.ac: GIT_CHECK_FUNC(getdelim,
>       - [HAVE_GETDELIM=])
>       - GIT_CONF_SUBST([HAVE_GETDELIM])
>       - #
>       -+# Define NO_SIGINTERRUPT if you don't have siginterrupt.
>       -+GIT_CHECK_FUNC(siginterrupt,
>       -+[NO_SIGINTERRUPT=],
>       -+[NO_SIGINTERRUPT=YesPlease])
>       -+GIT_CONF_SUBST([NO_SIGINTERRUPT])
>       - #
>       - # Define NO_MMAP if you want to avoid mmap.
>       - #
>       -
>         ## daemon.c ##
>       -@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>       - 		add_child(&cld, addr, addrlen);
>       +@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
>       + 	sigaction(SIGCHLD, psa, NULL);
>         }
>         
>       --static void child_handler(int signo UNUSED)
>       -+static void child_handler(int signo)
>       - {
>       - 	/*
>       --	 * Otherwise empty handler because systemcalls will get interrupted
>       --	 * upon signal receipt
>       -+	 * Empty handler because systemcalls should get interrupted
>       -+	 * upon signal receipt.
>       - 	 */
>       -+#ifdef NO_SIGINTERRUPT
>       -+	/* SysV needs the handler to be rearmed */
>       -+	signal(signo, child_handler);
>       -+#endif
>       ++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)
>       +@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
>       + 	signal(SIGCHLD, child_handler);
>         }
>         
>       - static int set_reuse_addr(int sockfd)
>       -@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>       -
>       ++static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
>       ++{
>       ++}
>       ++
>       ++#endif
>       ++
>         static int service_loop(struct socketlist *socklist)
>         {
>       --	struct pollfd *pfd;
>       -+#ifndef NO_SIGINTERRUPT
>         	struct sigaction sa;
>       -+#endif
>       -+	struct pollfd *pfd;
>       -
>       - 	CALLOC_ARRAY(pfd, socklist->nr);
>       -
>        @@ daemon.c: static int service_loop(struct socketlist *socklist)
>       - 		pfd[i].events = POLLIN;
>       - 	}
>       -
>       -+#ifdef NO_SIGINTERRUPT
>       -+	signal(SIGCHLD, child_handler);
>       -+#else
>       - 	sigemptyset(&sa.sa_mask);
>       - 	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
>       - 	sa.sa_handler = child_handler;
>       - 	sigaction(SIGCHLD, &sa, NULL);
>       -+#endif
>       -
>         	for (;;) {
>         		check_dead_children();
>         
>       -+#ifndef NO_SIGINTERRUPT
>       -+		sa.sa_flags &= ~SA_RESTART;
>       -+		sigaction(SIGCHLD, &sa, NULL);
>       -+#endif
>       ++		set_sa_restart(&sa, 0);
>         		if (poll(pfd, socklist->nr, -1) < 0) {
>         			if (errno != EINTR) {
>         				logerror("Poll failed, resuming: %s",
>       @@ daemon.c: static int service_loop(struct socketlist *socklist)
>         			}
>         			continue;
>         		}
>       -+#ifndef NO_SIGINTERRUPT
>       -+		sa.sa_flags |= SA_RESTART;
>       -+		sigaction(SIGCHLD, &sa, NULL);
>       -+#endif
>       ++		set_sa_restart(&sa, 1);
>         
>         		for (size_t i = 0; i < socklist->nr; i++) {
>         			if (pfd[i].revents & POLLIN) {
>       -
>       - ## meson.build ##
>       -@@ meson.build: checkfuncs = {
>       -   'setenv' : ['setenv.c'],
>       -   'mkdtemp' : ['mkdtemp.c'],
>       -   'initgroups' : [],
>       -+  'siginterrupt' : [],
>       -   'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
>       -   'pread' : ['pread.c'],
>       - }
> 

> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>> This series addresses and ambiguity that is at least visible in OpenBSD,
>> where zombie proceses would only be cleared after a new connection is
>> received.
>
> There is still a race where a child that exits after it has been
> checked in check_dead_children() but before we call poll() will not be
> collected until a new connection is received or a child exits while
> we're polling. If we used the self-pipe trick described on the
> select(2) man page [1] we would avoid that race and would not need to
> mess with SA_RESTART and so would not need to introduce
> USE_NON_POSIX_SIGNAL.
>
> Best Wishes
>
> Phillip
>
> [1] https://www.man7.org/linux/man-pages/man2/select.2.html

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>>> This series addresses and ambiguity that is at least visible in OpenBSD,
>>> where zombie proceses would only be cleared after a new connection is
>>> received.
>>
>> There is still a race where a child that exits after it has been
>> checked in check_dead_children() but before we call poll() will not be
>> collected until a new connection is received or a child exits while
>> we're polling. If we used the self-pipe trick described on the
>> select(2) man page [1] we would avoid that race and would not need to
>> mess with SA_RESTART and so would not need to introduce
>> USE_NON_POSIX_SIGNAL.
>>
>> Best Wishes
>>
>> Phillip
>>
>> [1] https://www.man7.org/linux/man-pages/man2/select.2.html
> 
> The principle should apply equally to poll-based service loop, I
> presume.

> An obvious disadvantage (at least of this implementation), is that it
> actually doubles the number of events that need to be handled for each
> children process on most cases (ex: when `poll()` gets interrupted)
> 
> I suspect that if fixing that last race condition is so important with
> the current foundation, it might be better to reintroduce some sort of
> timeout to poll(), so that they will be cleared periodically.
> 
> I had a prototype (only the bare minimum) that I thought was more
> efficient and that would instead remove completely the need for a
> signal handler which I would post (only for RFC) later.

>   daemon.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/daemon.c b/daemon.c
> index d1be61fd57..d3b9421575 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>   		add_child(&cld, addr, addrlen);
>   }
>   
> -static void child_handler(int signo UNUSED)
> +int poll_pipe[2] = { -1, -1 };

> +
> +static void child_handler(int signo)
>   {
>   	/*
> -	 * Otherwise empty handler because systemcalls will get interrupted
> -	 * upon signal receipt
>   	 * SysV needs the handler to be rearmed
>   	 */
>   	signal(SIGCHLD, child_handler);

>   }
>   
>   static int set_reuse_addr(int sockfd)
> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>   static int service_loop(struct socketlist *socklist)
>   {
>   	struct pollfd *pfd;
> +	unsigned long nfds = 1 + socklist->nr;
> +
> +	ALLOC_ARRAY(pfd, nfds);
> +	if (!pipe(poll_pipe)) {

> +			int flags;
> +
> +			flags = fcntl(poll_pipe[i], F_GETFD, 0);
> +			if (flags >= 0)
> +				fcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);

> +
> +			flags = fcntl(poll_pipe[i], F_GETFL, 0);
> +			if (flags < 0 || fcntl(poll_pipe[i], F_SETFL,
> +					       flags | O_NONBLOCK) == -1) {
> +				close(poll_pipe[0]);
> +				close(poll_pipe[1]);
> +				poll_pipe[0] = poll_pipe[1] = -1;
> +				break;
> +			}
> +		}
> +	}
> +	pfd[0].fd = poll_pipe[0];
> +	pfd[0].events = POLLIN;
>   
> -	CALLOC_ARRAY(pfd, socklist->nr);
> -
> -	for (size_t i = 0; i < socklist->nr; i++) {
> -		pfd[i].fd = socklist->list[i];
> +	for (size_t i = 1; i < nfds; i++) {
> +		pfd[i].fd = socklist->list[i - 1];
>   		pfd[i].events = POLLIN;
>   	}
>   
>   	signal(SIGCHLD, child_handler);
>   
>   	for (;;) {
> +		int nevents, scratch;
> +
>   		check_dead_children();
>   
> -		if (poll(pfd, socklist->nr, -1) < 0) {
> +		if ((nevents = poll(pfd, nfds, -1)) <= 0) {
>   			if (errno != EINTR) {
>   				logerror("Poll failed, resuming: %s",
>   				      strerror(errno));

> @@ -1143,7 +1169,17 @@ static int service_loop(struct socketlist *socklist)
>   			continue;
>   		}
>   
> -		for (size_t i = 0; i < socklist->nr; i++) {
> +		if ((pfd[0].revents & POLLIN) && pfd[0].fd != -1) {
> +			if (nevents == 1 && read(pfd[0].fd, &scratch, 1) > 0)

> +				continue;
> +			else if (!read(pfd[0].fd, &scratch, 1)) {
> +				close(pfd[0].fd);
> +				pfd[0].fd = -1;
> +			}
> +			nevents--;
> +		}
> +
> +		for (size_t i = 1; nevents && i < nfds; i++) {
>   			if (pfd[i].revents & POLLIN) {
>   				union {
>   					struct sockaddr sa;
> @@ -1165,6 +1201,7 @@ static int service_loop(struct socketlist *socklist)
>   					}
>   				}
>   				handle(incoming, &ss.sa, sslen);
> +				nevents--;
>   			}
>   		}
>   	}

> 
> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
> > 
> > I had a prototype (only the bare minimum) that I thought was more
> > efficient and that would instead remove completely the need for a
> > signal handler which I would post (only for RFC) later.
> 
> I'm not sure injecting an fd into each child process is a good direction.

> > diff --git a/daemon.c b/daemon.c
> > index d1be61fd57..d3b9421575 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> >   		add_child(&cld, addr, addrlen);
> >   }
> > -static void child_handler(int signo UNUSED)
> > +int poll_pipe[2] = { -1, -1 };
> Maybe call this signal_pipe? I'm not sure what poll_pipe means.
> 
> > +
> > +static void child_handler(int signo)
> >   {
> >   	/*
> > -	 * Otherwise empty handler because systemcalls will get interrupted
> > -	 * upon signal receipt
> >   	 * SysV needs the handler to be rearmed
> >   	 */
> >   	signal(SIGCHLD, child_handler);
> 
> I think from Chris' email that it is conventional to do this at the end of
> the handler.

> > +
> > +	if (poll_pipe[1] >= 0)
> > +		write(poll_pipe[1], &signo, 1);
> 
> write() might fail so we should save errno around it. Conventionally one
> would re-try on EINTR as well though in this case the most likely reason for
> that is another child exiting which means the pipe would be written to
> anyway.

> >   }
> >   static int set_reuse_addr(int sockfd)
> > @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> >   static int service_loop(struct socketlist *socklist)
> >   {
> >   	struct pollfd *pfd;
> > +	unsigned long nfds = 1 + socklist->nr;
> > +
> > +	ALLOC_ARRAY(pfd, nfds);
> > +	if (!pipe(poll_pipe)) {
> 
> If we cannot create a pipe here then things have gone pretty badly wrong and
> I think it is unlikely we're going to be able to accept incoming connections
> so it would be best to die().

> > +			int flags;
> > +
> > +			flags = fcntl(poll_pipe[i], F_GETFD, 0);
> > +			if (flags >= 0)
> > +				fcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);
> I think we should probably close the pipes if we do not set FD_CLOEXEC.

> On Fri, Jun 27, 2025 at 09:38:36AM -0800, Phillip Wood wrote:
>>
>> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
>>>
>>> diff --git a/daemon.c b/daemon.c
>>> index d1be61fd57..d3b9421575 100644
>>> --- a/daemon.c
>>> +++ b/daemon.c
>>> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>>>    		add_child(&cld, addr, addrlen);
>>>    }
>>> -static void child_handler(int signo UNUSED)
>>> +int poll_pipe[2] = { -1, -1 };
>> Maybe call this signal_pipe? I'm not sure what poll_pipe means.
>>
>>> +
>>> +static void child_handler(int signo)
>>>    {
>>>    	/*
>>> -	 * Otherwise empty handler because systemcalls will get interrupted
>>> -	 * upon signal receipt
>>>    	 * SysV needs the handler to be rearmed
>>>    	 */
>>>    	signal(SIGCHLD, child_handler);
>>
>> I think from Chris' email that it is conventional to do this at the end of
>> the handler.
> 
> It really depends on which flags the signal was expected to use.  For maximum
> portability it would seem better to have it at the beginning to minimize the
> inherent race condition from when SA_RESETHAND is on effect (which was more
> of a SysV signal() tradition).
> 
> Will move it to the end, regardless, as it won't be needed once the call is
> setup using sigaction().
> 
>> As I said above we could add an additional commit that moves
>> this into the event loop to fix the infinite recursion on AIX.
> 
> Not sure I understant what you mean by this.

>>> +
>>> +	if (poll_pipe[1] >= 0)
>>> +		write(poll_pipe[1], &signo, 1);
>>
>> write() might fail so we should save errno around it. Conventionally one
>> would re-try on EINTR as well though in this case the most likely reason for
>> that is another child exiting which means the pipe would be written to
>> anyway.
> 
> EINTR shouldn't be possible here from SIGCHLD, because that signal is
> blocked here, 

>>>    }
>>>    static int set_reuse_addr(int sockfd)
>>> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>>>    static int service_loop(struct socketlist *socklist)
>>>    {
>>>    	struct pollfd *pfd;
>>> +	unsigned long nfds = 1 + socklist->nr;
>>> +
>>> +	ALLOC_ARRAY(pfd, nfds);
>>> +	if (!pipe(poll_pipe)) {
>>
>> If we cannot create a pipe here then things have gone pretty badly wrong and
>> I think it is unlikely we're going to be able to accept incoming connections
>> so it would be best to die().
> 
> True, but since this is "optional", there is no harm on letting this continue
> even in failure.

>> An obvious disadvantage (at least of this implementation), is that it
>> actually doubles the number of events that need to be handled for each
>> children process on most cases (ex: when `poll()` gets interrupted)
>> I suspect that if fixing that last race condition is so important
>> with
>> the current foundation, it might be better to reintroduce some sort of
>> timeout to poll(), so that they will be cleared periodically.
>> I had a prototype (only the bare minimum) that I thought was more
>> efficient and that would instead remove completely the need for a
>> signal handler which I would post (only for RFC) later.
>
> I'm not sure injecting an fd into each child process is a good direction.

>      +@@ Makefile: include shared.mak
>      + # when attempting to read from an fopen'ed directory (or even to fopen
>      + # it at all).
>      + #
>      ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>      ++# prefer to use ANSI C signal() over POSIX sigaction()
>      ++#
> ...
>      ++ifdef USE_NON_POSIX_SIGNAL
>      ++	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
>      ++endif

> "Carlo Marcelo Arenas Belón via GitGitGadget"
> <gitgitgadget@gmail.com> writes:
> 
> >      +@@ Makefile: include shared.mak
> >      + # when attempting to read from an fopen'ed directory (or even to fopen
> >      + # it at all).
> >      + #
> >      ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> >      ++# prefer to use ANSI C signal() over POSIX sigaction()
> >      ++#
> > ...
> >      ++ifdef USE_NON_POSIX_SIGNAL
> >      ++	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
> >      ++endif
> 
> The new symbol sounds like "POSIX does not have signal(2) but on
> this platform we have a usable signal(2), so we use it here", but I
> do not think that it is what we want to say (as POSIX inherits this
> from ANSI C anyway).  More importantly, this "USE_X" sounds as if we
> allow builders to set it and magically we stop using sigaction(2),
> which is not what is going on.  We have tons of calls to both
> signal(2) and sigaction(2), and we turn calls to signal(2) we have
> in daemon.c to sigaction(2) but on some platforms their sigaction(2)
> cannot do what we ask it to do, so we are stuck with signal(2) on
> these platforms only for these calls in daemon.c.  It may be obvious
> to those who develop and review this series, but not for anybody else.
> 
> Isn't the situation more like:
> 
>     We use sigaction(2) everywhere and have been happy with it in
>     our code, but this topic discovered that on some platforms,
>     their sigaction(2) does not do XYZ that everybody else's
>     sigaction(2) does, so on them we need to fall back on the plain
>     old signal(2) on selected code paths that we need XYZ out of the
>     signal handling interface.
> 
> What is this XYZ that describes the characteristics of
> signal/sigaction implementation on these platforms?  A name
> constructed more like SIGACTION_LACKS_XYZ (hence we have to resort
> to signal), possibly with a more appropriate verb than "lack", would
> be less confusing.

> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> +	AC_COMPILE_IFELSE(
> +		[AC_LANG_PROGRAM([#include <signal.h>], [[
> +			#ifdef SA_RESTART
> +			restartable signals supported
> +			#endif

> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> 
> A future change will start using sigaction to setup a SIGCHLD signal
> handler.
> 
> The current code uses signal() which returns SIG_ERR (but doesn't
> seem to set errno) so instruct sigaction() to do the same.

> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   compat/mingw-posix.h | 1 +
>   compat/mingw.c       | 4 +++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
> index a0dca756d104..847d558c9b2d 100644
> --- a/compat/mingw-posix.h
> +++ b/compat/mingw-posix.h
> @@ -95,6 +95,7 @@ struct sigaction {
>   	sig_handler_t sa_handler;
>   	unsigned sa_flags;
>   };
> +#define SA_NOCLDSTOP 1
>   
>   struct itimerval {
>   	struct timeval it_value, it_interval;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 8a9972a1ca19..5d69ae32f4b9 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
>   
>   int sigaction(int sig, struct sigaction *in, struct sigaction *out)
>   {
> -	if (sig != SIGALRM)
> +	if (sig == SIGCHLD)
> +		return -1;
> +	else if (sig != SIGALRM)
>   		return errno = EINVAL,
>   			error("sigaction only implemented for SIGALRM");
>   	if (out)

> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> > 
> > A future change will start using sigaction to setup a SIGCHLD signal
> > handler.
> > 
> > The current code uses signal() which returns SIG_ERR (but doesn't
> > seem to set errno) so instruct sigaction() to do the same.
> 
> Why are we returning -1 below instead of SIG_ERR if we want the behavior to
> match?

> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
>>>
>>> A future change will start using sigaction to setup a SIGCHLD signal
>>> handler.
>>>
>>> The current code uses signal() which returns SIG_ERR (but doesn't
>>> seem to set errno) so instruct sigaction() to do the same.
>>
>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to
>> match?
> 
> By "match", I mean that in both cases we will get an error return value
> and errno won't be set to EINVAL (which is what POSIX requires)
> 
> In our codebase since we ignore the return code anyway, it wouldn't make
> a difference, either way.
> 
> signal() returns a pointer, and sigaction() returns and int, 

> so you can
> have the later be literally SIG_ERR, eventhough it will be ironically
> equivalent it casted into an int.
> 
> Csrlo
> 

> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
> > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
> > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> > > > 
> > > > A future change will start using sigaction to setup a SIGCHLD signal
> > > > handler.
> > > > 
> > > > The current code uses signal() which returns SIG_ERR (but doesn't
> > > > seem to set errno) so instruct sigaction() to do the same.
> > > 
> > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to
> > > match?
> > 
> > By "match", I mean that in both cases we will get an error return value
> > and errno won't be set to EINVAL (which is what POSIX requires)
> > 
> > In our codebase since we ignore the return code anyway, it wouldn't make
> > a difference, either way.
> > 
> > signal() returns a pointer, and sigaction() returns and int,
> 
> Oh right, I'd forgotten they have different return types. I think we should
> probably be setting errno = EINVAL before returning -1 to match what this
> function does with other signals it does not support - just because our
> current callers ignore the return value doesn't mean that future callers
> will and they might want check errno if they see the function fail.

> On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:
>> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
>>> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
>>>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>>>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
>>>>>
>>>>> A future change will start using sigaction to setup a SIGCHLD signal
>>>>> handler.
>>>>>
>>>>> The current code uses signal() which returns SIG_ERR (but doesn't
>>>>> seem to set errno) so instruct sigaction() to do the same.
>>>>
>>>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to
>>>> match?
>>>
>>> By "match", I mean that in both cases we will get an error return value
>>> and errno won't be set to EINVAL (which is what POSIX requires)
>>>
>>> In our codebase since we ignore the return code anyway, it wouldn't make
>>> a difference, either way.
>>>
>>> signal() returns a pointer, and sigaction() returns and int,
>>
>> Oh right, I'd forgotten they have different return types. I think we should
>> probably be setting errno = EINVAL before returning -1 to match what this
>> function does with other signals it does not support - just because our
>> current callers ignore the return value doesn't mean that future callers
>> will and they might want check errno if they see the function fail.
> 
> I agree, and indeed had to triple check and change my implementation after I
> confirmed that signal(SIGCHLD) does not change errno on Windows (not our
> version, neither of the windows libc or mingw, even if it is documented[1] to
> do so.
> 
> It might be because the signal number itself is bogus (there is none for
> SIGCHLD in their headers, and git uses their own numbers in compat), but
> either way, I would rather be consistent with signal() at least originally.

> On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:
> > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:
> > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
> > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
> > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> > > > > > 
> > > > > > A future change will start using sigaction to setup a SIGCHLD signal
> > > > > > handler.
> > > > > > 
> > > > > > The current code uses signal() which returns SIG_ERR (but doesn't
> > > > > > seem to set errno) so instruct sigaction() to do the same.
> > > > > 
> > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to
> > > > > match?
> > > > 
> > > > By "match", I mean that in both cases we will get an error return value
> > > > and errno won't be set to EINVAL (which is what POSIX requires)
> > > > 
> > > > In our codebase since we ignore the return code anyway, it wouldn't make
> > > > a difference, either way.
> > > > 
> > > > signal() returns a pointer, and sigaction() returns and int,
> > > 
> > > Oh right, I'd forgotten they have different return types. I think we should
> > > probably be setting errno = EINVAL before returning -1 to match what this
> > > function does with other signals it does not support - just because our
> > > current callers ignore the return value doesn't mean that future callers
> > > will and they might want check errno if they see the function fail.
> > 
> > I agree, and indeed had to triple check and change my implementation after I
> > confirmed that signal(SIGCHLD) does not change errno on Windows (not our
> > version, neither of the windows libc or mingw, even if it is documented[1] to
> > do so.
> > 
> > It might be because the signal number itself is bogus (there is none for
> > SIGCHLD in their headers, and git uses their own numbers in compat), but
> > either way, I would rather be consistent with signal() at least originally.
> 
> I'm not sure I understand - don't we want the sigaction() wrapper to behave
> like sigaction() would?

> On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote:
>> On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:
>>> On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:
>>>> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
>>>>> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
>>>>>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>>>>>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
>>>>>>>
>>>>>>> A future change will start using sigaction to setup a SIGCHLD signal
>>>>>>> handler.
>>>>>>>
>>>>>>> The current code uses signal() which returns SIG_ERR (but doesn't
>>>>>>> seem to set errno) so instruct sigaction() to do the same.
>>>>>>
>>>>>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to
>>>>>> match?
>>>>>
>>>>> By "match", I mean that in both cases we will get an error return value
>>>>> and errno won't be set to EINVAL (which is what POSIX requires)
>>>>>
>>>>> In our codebase since we ignore the return code anyway, it wouldn't make
>>>>> a difference, either way.
>>>>>
>>>>> signal() returns a pointer, and sigaction() returns and int,
>>>>
>>>> Oh right, I'd forgotten they have different return types. I think we should
>>>> probably be setting errno = EINVAL before returning -1 to match what this
>>>> function does with other signals it does not support - just because our
>>>> current callers ignore the return value doesn't mean that future callers
>>>> will and they might want check errno if they see the function fail.
>>>
>>> I agree, and indeed had to triple check and change my implementation after I
>>> confirmed that signal(SIGCHLD) does not change errno on Windows (not our
>>> version, neither of the windows libc or mingw, even if it is documented[1] to
>>> do so.
>>>
>>> It might be because the signal number itself is bogus (there is none for
>>> SIGCHLD in their headers, and git uses their own numbers in compat), but
>>> either way, I would rather be consistent with signal() at least originally.
>>
>> I'm not sure I understand - don't we want the sigaction() wrapper to behave
>> like sigaction() would?
> 
> for at least the first iteration, I would rather have sigaction() behave
> like signal(), so that the change doesn't introduce any regressions.

> eventually, sigaction() should behave like any other sigaction(), but to
> do so, I suspect the windows emulation might need to change their SIGCHLD
> to match.
> 
> just confirmed with MSVC that if I use 20 instead of 17, errno gets updated
> just like the documentation says it should.

> Carlo
> 
> PS. Maybe we should get dscho involved?
>>>
>>> [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

> On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote:
> > On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote:
> > > On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:
> > > > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:
> > > > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
> > > > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
> > > > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > > > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> > > > > > > > 
> > > > > > > > A future change will start using sigaction to setup a SIGCHLD signal
> > > > > > > > handler.
> > > > > > > > 
> > > > > > > > The current code uses signal() which returns SIG_ERR (but doesn't
> > > > > > > > seem to set errno) so instruct sigaction() to do the same.
> > > > > > > 
> > > > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to
> > > > > > > match?
> > > > > > 
> > > > > > By "match", I mean that in both cases we will get an error return value
> > > > > > and errno won't be set to EINVAL (which is what POSIX requires)
> > > > > > 
> > > > > > In our codebase since we ignore the return code anyway, it wouldn't make
> > > > > > a difference, either way.
> > > > > > 
> > > > > > signal() returns a pointer, and sigaction() returns and int,
> > > > > 
> > > > > Oh right, I'd forgotten they have different return types. I think we should
> > > > > probably be setting errno = EINVAL before returning -1 to match what this
> > > > > function does with other signals it does not support - just because our
> > > > > current callers ignore the return value doesn't mean that future callers
> > > > > will and they might want check errno if they see the function fail.
> > > > 
> > > > I agree, and indeed had to triple check and change my implementation after I
> > > > confirmed that signal(SIGCHLD) does not change errno on Windows (not our
> > > > version, neither of the windows libc or mingw, even if it is documented[1] to
> > > > do so.
> > > > 
> > > > It might be because the signal number itself is bogus (there is none for
> > > > SIGCHLD in their headers, and git uses their own numbers in compat), but
> > > > either way, I would rather be consistent with signal() at least originally.
> > > 
> > > I'm not sure I understand - don't we want the sigaction() wrapper to behave
> > > like sigaction() would?
> > 
> > for at least the first iteration, I would rather have sigaction() behave
> > like signal(), so that the change doesn't introduce any regressions.
> 
> What regressions are you worried about?

> We're talking about changing a
> single call from signal() to sigaction(). I'd have thought we're far more
> likely to introduce regressions if we change the behavior of the windows
> implementation of sigaction() to behave like signal() as that introduces
> more variation between different platforms.

> > eventually, sigaction() should behave like any other sigaction(), but to
> > do so, I suspect the windows emulation might need to change their SIGCHLD
> > to match.
> > 
> > just confirmed with MSVC that if I use 20 instead of 17, errno gets updated
> > just like the documentation says it should.
> 
> Oh not so setting errno as you want to do would not actually match signal()
> on Windows in that case?

> > Carlo
> > 
> > PS. Maybe we should get dscho involved?
> > > > 
> > > > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

> 
> Replace signal() with an equivalent invocation of sigaction(), which
> has the added benefit of using BSD semantics reliably and therefore
> not needing the rearming call in the signal handler.
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   daemon.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/daemon.c b/daemon.c
> index d1be61fd5789..155b2e180167 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>   static void child_handler(int signo 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);
>   }
>   
>   static int set_reuse_addr(int sockfd)
> @@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>   
>   static int service_loop(struct socketlist *socklist)
>   {
> +	struct sigaction sa;
>   	struct pollfd *pfd;
>   
>   	CALLOC_ARRAY(pfd, socklist->nr);
> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
>   		pfd[i].events = POLLIN;
>   	}
>   
> -	signal(SIGCHLD, child_handler);
> +	sigemptyset(&sa.sa_mask);
> +	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> +	sa.sa_handler = child_handler;
> +	sigaction(SIGCHLD, &sa, NULL);
>   
>   	for (;;) {
>   		check_dead_children();

> Only because you have chosen to use SA_RESTART here. I think it would
> be better to drop that and instead say something like
>
>
> POSIX leaves several aspects of the behavior of signal() up to the
> implementer. It is unspecified whether long running syscalls are
> restarted after an interrupt is received.  The event loop in "git
> daemon" assumes that poll() will return with EINTR if a child exits
> while it is polling but if poll() is restarted after an interrupt is
> received that does not happen which can lead to a build up of
> uncollected zombie processes.
>
> It is also unspecified whether the handler is reset after an interrupt
> is received. In order to work correctly on operating systems such as
> Solaris where the handler for SIGCLD is reset when a signal is
> received "git daemon" calls signal() from the SIGCLD handler to
> re-establish a handler for that signal. Unfortunately this causes
> infinite recursion on other operating systems such as AIX.
>
> Both of these problems can be addressed by using sigaction() instead
> of signal() which has much stricter semantics and so, by setting the
> appropriate flags, we can rely on poll() being interrupted and the
> SIGCLD handler not being reset. This change means that all long
> running system calls could fail with EINTR not just pall() but rest of
> the event loop code is designed to gracefully handle that.
>
> After this change there is still a race where a child that exits after
> it has been checked in check_dead_children() but before we call poll()
> will not be collected until a new connection is received or a child
> exits while we're polling. We could fix this by using the "self-pipe
> trick" but do not because ....
>
>
> Then we can drop patches 1 and 4.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > Only because you have chosen to use SA_RESTART here. I think it would
> > be better to drop that and instead say something like
> >
> >
> > POSIX leaves several aspects of the behavior of signal() up to the
> > implementer. It is unspecified whether long running syscalls are
> > restarted after an interrupt is received.  The event loop in "git
> > daemon" assumes that poll() will return with EINTR if a child exits
> > while it is polling but if poll() is restarted after an interrupt is
> > received that does not happen which can lead to a build up of
> > uncollected zombie processes.
> >
> > It is also unspecified whether the handler is reset after an interrupt
> > is received. In order to work correctly on operating systems such as
> > Solaris where the handler for SIGCLD is reset when a signal is
> > received "git daemon" calls signal() from the SIGCLD handler to
> > re-establish a handler for that signal. Unfortunately this causes
> > infinite recursion on other operating systems such as AIX.
> >
> > Both of these problems can be addressed by using sigaction() instead
> > of signal() which has much stricter semantics and so, by setting the
> > appropriate flags, we can rely on poll() being interrupted and the
> > SIGCLD handler not being reset. This change means that all long
> > running system calls could fail with EINTR not just pall() but rest of
> > the event loop code is designed to gracefully handle that.
> >
> > After this change there is still a race where a child that exits after
> > it has been checked in check_dead_children() but before we call poll()
> > will not be collected until a new connection is received or a child
> > exits while we're polling. We could fix this by using the "self-pipe
> > trick" but do not because ....
> >
> >
> > Then we can drop patches 1 and 4.
> 
> Thanks for a suggestion.  I really like the "everybody in these code
> paths are prepared to receive and handle EINTR just fine, so we do
> not have to do the SA_RESTART" observation very much.

> On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>> Thanks for a suggestion.  I really like the "everybody in these code
>> paths are prepared to receive and handle EINTR just fine, so we do
>> not have to do the SA_RESTART" observation very much.
> 
> Except that is unlikely to be true, as the code has changed a lot on
> those 20 years (including that it now uses run_command) and that we
> had been effectively using SA_RESTART under the covers for most of
> that time because signal() behaviour changed. An obvious bug:
> (ex: 20250626161038.85966-1-carenas@gmail.com)

> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> 
> If the setup for the SIGCHLD signal handler sets SA_RESTART, poll()
> might not return with -1 and set errno to EINTR when a signal is
> received.
> 
> Since the logic to reap zombie childs relies on those interruptions
> make sure to explicitly disable SA_RESTART around this function.

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   daemon.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/daemon.c b/daemon.c
> index 155b2e180167..7e29c03e313f 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1116,6 +1116,25 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>   	}
>   }
>   
> +#ifndef NO_RESTARTABLE_SIGNALS
> +
> +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_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
> +{
> +}
> +
> +#endif
> +
>   static int service_loop(struct socketlist *socklist)
>   {
>   	struct sigaction sa;
> @@ -1136,6 +1155,7 @@ static int service_loop(struct socketlist *socklist)
>   	for (;;) {
>   		check_dead_children();
>   
> +		set_sa_restart(&sa, 0);
>   		if (poll(pfd, socklist->nr, -1) < 0) {
>   			if (errno != EINTR) {
>   				logerror("Poll failed, resuming: %s",
> @@ -1144,6 +1164,7 @@ static int service_loop(struct socketlist *socklist)
>   			}
>   			continue;
>   		}
> +		set_sa_restart(&sa, 1);
>   
>   		for (size_t i = 0; i < socklist->nr; i++) {
>   			if (pfd[i].revents & POLLIN) {

> This series addresses and ambiguity that is at least visible in OpenBSD,
> where zombie proceses would only be cleared after a new connection is
> received.
> 
> The underlying problem is that when this code was originally introduced,
> SA_RESTART was not widely implemented, and the signal() call usually
> implemented SysV like semantics, at least until it started being
> reimplemented by calling sigaction() internally.

> Changes since v2
> 
>   * Add a new patch 2 that modifies windows' sigaction so there is no more
>     need for a fallback
>   * Hopefully no more silly mistakes and a variable that finally makes sense
> 
> Changes since v1
> 
>   * Almost all references to siginterrupt has been removed and a better named
>     variable used instead
>   * Changes had been abstracted to minimize ifdefs and their introduction
>     staged more naturally
> 
> Carlo Marcelo Arenas Belón (4):
>    compat/posix.h: track SA_RESTART fallback
>    compat/mingw: allow sigaction(SIGCHLD)
>    daemon: use sigaction() to install child_handler()
>    daemon: explicitly allow EINTR during poll()
> 
>   Makefile             |  5 +++++
>   compat/mingw-posix.h |  2 +-
>   compat/mingw.c       |  4 +++-
>   compat/posix.h       |  8 ++++++++
>   config.mak.uname     |  7 ++++---
>   configure.ac         | 16 ++++++++++++++++
>   daemon.c             | 33 ++++++++++++++++++++++++++++-----
>   meson.build          |  4 ++++
>   8 files changed, 69 insertions(+), 10 deletions(-)
> 
> 
> base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v3
> Pull-Request: https://github.com/git/git/pull/2002
> 
> Range-diff vs v2:
> 
>   1:  e82b7425bbc ! 1:  ae1ca6bb2b2 compat/posix.h: track SA_RESTART fallback
>       @@ Makefile: include shared.mak
>         # when attempting to read from an fopen'ed directory (or even to fopen
>         # it at all).
>         #
>       -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>       -+# prefer to use ANSI C signal() over POSIX sigaction()
>       ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
>        +#
>         # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
>         # when a signal is received (as opposed to restarting).
>       @@ Makefile: ifdef FREAD_READS_DIRECTORIES
>         	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
>         	COMPAT_OBJS += compat/fopen.o
>         endif
>       -+ifdef USE_NON_POSIX_SIGNAL
>       -+	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
>       ++ifdef NO_RESTARTABLE_SIGNALS
>       ++	COMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS
>        +endif
>         ifdef OPEN_RETURNS_EINTR
>         	COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
>       @@ compat/posix.h: char *gitdirname(char *);
>        + * not on some systems (e.g. NonStop, QNX).
>        + */
>        +#ifndef SA_RESTART
>       -+# define SA_RESTART 0	/* disabled for sigaction() */
>       ++# define SA_RESTART 0 /* disabled for sigaction() */
>        +#endif
>        +
>         typedef uintmax_t timestamp_t;
>       @@ config.mak.uname: ifeq ($(uname_S),Windows)
>         	NO_STRTOUMAX = YesPlease
>         	NO_MKDTEMP = YesPlease
>         	NO_INTTYPES_H = YesPlease
>       -+	USE_NON_POSIX_SIGNAL = YesPlease
>       ++	NO_RESTARTABLE_SIGNALS = YesPlease
>         	CSPRNG_METHOD = rtlgenrandom
>         	# VS2015 with UCRT claims that snprintf and friends are C99 compliant,
>         	# so we don't need this:
>       @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
>         	NO_MMAP = YesPlease
>         	NO_POLL = YesPlease
>         	NO_INTPTR_T = UnfortunatelyYes
>       -+	USE_NON_POSIX_SIGNAL = UnfortunatelyYes
>       ++	NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
>         	CSPRNG_METHOD = openssl
>         	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
>         	SHELL_PATH = /usr/coreutils/bin/bash
>       @@ config.mak.uname: ifeq ($(uname_S),MINGW)
>         	NEEDS_LIBICONV = YesPlease
>         	NO_STRTOUMAX = YesPlease
>         	NO_MKDTEMP = YesPlease
>       -+	USE_NON_POSIX_SIGNAL = YesPlease
>       ++	NO_RESTARTABLE_SIGNALS = YesPlease
>         	NO_SVN_TESTS = YesPlease
>         
>         	# The builtin FSMonitor requires Named Pipes and Threads on Windows.
>       @@ config.mak.uname: ifeq ($(uname_S),QNX)
>         	NO_PTHREADS = YesPlease
>         	NO_STRCASESTR = YesPlease
>         	NO_STRLCPY = YesPlease
>       -+	USE_NON_POSIX_SIGNAL = UnfortunatelyYes
>       ++	NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
>         endif
>        
>         ## configure.ac ##
>       @@ configure.ac: fi
>         GIT_CONF_SUBST([ICONV_OMITS_BOM])
>         fi
>         
>       -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
>       -+# prefer using ANSI C signal() over POSIX sigaction()
>       ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
>        +
>        +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
>        +	AC_COMPILE_IFELSE(
>        +		[AC_LANG_PROGRAM([#include <signal.h>], [[
>       -+		#ifdef SA_RESTART
>       -+		#endif
>       -+		siginterrupt(SIGCHLD, 1)
>       -+		]])],[ac_cv_siginterrupt=yes],[
>       ++			#ifdef SA_RESTART
>       ++			restartable signals supported
>       ++			#endif
>       ++		]])],[
>        +			ac_cv_siginterrupt=no
>       -+			USE_NON_POSIX_SIGNAL=UnfortunatelyYes
>       -+		]
>       ++			NO_RESTARTABLE_SIGNALS=UnfortunatelyYes
>       ++		], [ac_cv_siginterrupt=yes]
>        +	)
>        +])
>       -+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])
>       ++GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])
>        +
>         ## Checks for typedefs, structures, and compiler characteristics.
>         AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
>       @@ meson.build: else
>         endif
>         
>        +if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
>       -+  libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
>       ++  libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'
>        +endif
>        +
>         if not compiler.has_header('sys/select.h')
>   -:  ----------- > 2:  3f63479119f compat/mingw: allow sigaction(SIGCHLD)
>   2:  05d945aa1e5 ! 3:  c66bda461f4 daemon: use sigaction() to install child_handler()
>       @@ Commit message
>            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.
>       +    Replace signal() with an equivalent invocation of sigaction(), which
>       +    has the added benefit of using BSD semantics reliably and therefore
>       +    not needing the rearming call in the signal handler.
>        
>            Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>        
>         ## daemon.c ##
>        @@ daemon.c: 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)
>       + static void child_handler(int signo UNUSED)
>         {
>         	/*
>        -	 * Otherwise empty handler because systemcalls will get interrupted
>       @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr
>        +	 * 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)
>        @@ daemon.c: 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);
>       -+}
>       -+
>       -+#else
>       -+
>       -+static void set_signal_handler(struct sigaction *psa UNUSED)
>       -+{
>       -+	signal(SIGCHLD, child_handler);
>       -+}
>       -+
>         static int service_loop(struct socketlist *socklist)
>         {
>        +	struct sigaction sa;
>       @@ daemon.c: static int service_loop(struct socketlist *socklist)
>         	}
>         
>        -	signal(SIGCHLD, child_handler);
>       -+	set_signal_handler(&sa);
>       ++	sigemptyset(&sa.sa_mask);
>       ++	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
>       ++	sa.sa_handler = child_handler;
>       ++	sigaction(SIGCHLD, &sa, NULL);
>         
>         	for (;;) {
>         		check_dead_children();
>   3:  b737e0389df ! 4:  851d663be0b daemon: explicitly allow EINTR during poll()
>       @@ Commit message
>            Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>        
>         ## daemon.c ##
>       -@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
>       - 	sigaction(SIGCHLD, psa, NULL);
>       +@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>       + 	}
>         }
>         
>       ++#ifndef NO_RESTARTABLE_SIGNALS
>       ++
>        +static void set_sa_restart(struct sigaction *psa, int enable)
>        +{
>        +	if (enable)
>       @@ daemon.c: static void set_signal_handler(struct sigaction *psa)
>        +	sigaction(SIGCHLD, psa, NULL);
>        +}
>        +
>       - #else
>       -
>       - static void set_signal_handler(struct sigaction *psa UNUSED)
>       -@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
>       - 	signal(SIGCHLD, child_handler);
>       - }
>       -
>       ++#else
>       ++
>        +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
>        +{
>        +}
> 

> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > This series addresses and ambiguity that is at least visible in OpenBSD,
> > where zombie proceses would only be cleared after a new connection is
> > received.
> > 
> > The underlying problem is that when this code was originally introduced,
> > SA_RESTART was not widely implemented, and the signal() call usually
> > implemented SysV like semantics, at least until it started being
> > reimplemented by calling sigaction() internally.
> 
> I'm all in favor of using sigaction() but I think the SA_RESTART parts of
> this series are an unnecessary complication that has the potential to hide
> bugs as we support platforms without SA_RESTART.

> A future change will start using sigaction to setup a SIGCHLD signal
> handler.
>
> The current code uses signal(), which returns SIG_ERR (but doesn't
> seem to set errno) so instruct sigaction() to do the same.
>
> A new SA flag will be needed, so copy the one from Cygwinr; note that
> the sigacgtion() implementation that is provided won't use it, so
> its value is otherwise irrelevant.

> I understand that the self-pipe to wake ourselves up is left outside
> this topic on purpose, which I agree with.
> 
> Thanks, Carlo, for putting this together from weeks' long
> discussions, and thanks Phillip for pushing for simpler and smaller
> set of changes.
> 
> Will queue.
> 
> 
> 1:  30773a76ce ! 1:  ef03aa432a compat/mingw: allow sigaction(SIGCHLD)
>      @@ Commit message
>           The current code uses signal(), which returns SIG_ERR (but doesn't
>           seem to set errno) so instruct sigaction() to do the same.
>       
>      -    A new SA flag will be needed, so copy the one from Cygwinr; note that
>      -    the sigacgtion() implementation that is provided won't use it, so
>      +    A new SA flag will be needed, so copy the one from Cygwin; note that
>      +    the sigaction() implementation that is provided won't use it, so
>           its value is otherwise irrelevant.
>       
>           Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> 2:  7a33d7a646 = 2:  d83e1eef3b daemon: use sigaction() to install child_handler()

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> Let's queue this round and then patch the errno issue up on top
> after the dust settles.  "might break something" may then happen,
> at which time it is easier to see where that breakage came from,
> and we can go from there.