Merge branch 'cb/daemon-reap-children' into seen

Futz with SIGCHLD handling in "git daemon".

Comments?

* cb/daemon-reap-children:
  daemon: explicitly allow EINTR during poll()
  daemon: use sigaction() to install child_handler()
  compat/posix.h: track SA_RESTART fallback

Author: gitster

Makefile

@@ -37,6 +37,9 @@ 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).
 #
@@ -1810,6 +1813,9 @@ 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

@@ -95,7 +95,6 @@ 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

@@ -250,6 +250,14 @@ 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

config.mak.uname

@@ -480,6 +480,7 @@ 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:
@@ -648,8 +649,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
@@ -658,6 +657,7 @@ 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
@@ -693,6 +693,7 @@ 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.
@@ -776,7 +777,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
@@ -788,4 +788,5 @@ ifeq ($(uname_S),QNX)
 	NO_PTHREADS = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_STRLCPY = YesPlease
+	USE_NON_POSIX_SIGNAL = UnfortunatelyYes
 endif

configure.ac

@@ -862,6 +862,23 @@ 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])
 #

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,41 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 	}
 }
 
+#ifndef USE_NON_POSIX_SIGNAL
+
+static void set_signal_handler(struct sigaction *psa)
+{
+	sigemptyset(&psa->sa_mask);
+	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
+	psa->sa_handler = child_handler;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
+static void set_sa_restart(struct sigaction *psa, int enable)
+{
+	if (enable)
+		psa->sa_flags |= SA_RESTART;
+	else
+		psa->sa_flags &= ~SA_RESTART;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
+#else
+
+static void set_signal_handler(struct sigaction *psa UNUSED)
+{
+	signal(SIGCHLD, child_handler);
+}
+
+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
+{
+}
+
+#endif
+
 static int service_loop(struct socketlist *socklist)
 {
+	struct sigaction sa;
 	struct pollfd *pfd;
 
 	CALLOC_ARRAY(pfd, socklist->nr);
@@ -1129,11 +1167,12 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	set_signal_handler(&sa);
 
 	for (;;) {
 		check_dead_children();
 
+		set_sa_restart(&sa, 0);
 		if (poll(pfd, socklist->nr, -1) < 0) {
 			if (errno != EINTR) {
 				logerror("Poll failed, resuming: %s",
@@ -1142,6 +1181,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) {

meson.build

@@ -1094,6 +1094,10 @@ 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