Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()

> 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'],
>       - }
>