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@xxxxxxxxx>
+ ## 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@xxxxxxxxx>
## 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@xxxxxxxxx>
- ## 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'],
- }
