Re: [PATCH v3 0/4] daemon: explicitly allow EINTR during poll()

[Phillip Wood <phillip.wood123@gmail.com>]

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. Your EINTR patches 
have all been about efficiency rather than correctness so I think my 
initial assessment that the existing code handles EINTR correctly was 
probably true. As I said in my comments on patch 3 I think we can 
happily drop patches 1 and 4.

Thanks

Phillip


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