{"thread":{"id":"63688","subject":"[PATCH 0/3] daemon: explicitly allow EINTR during poll()","startedAt":"2025-06-24T14:08:45Z","lastAt":"2025-07-15T09:29:53Z","messageCount":70,"participants":["Carlo Marcelo Arenas Belón via GitGitGadget","Carlo Marcelo Arenas Belón","Junio C Hamano","Chris Torek","Phillip Wood","phillip.wood123@gmail.com","Eric Sunshine","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"520642","messageId":"pull.2002.git.git.1750774122.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":null,"subject":"[PATCH 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-24T14:08:39Z","receivedAt":"2025-06-24T14:08:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series addresses and ambiguity that is at least visible in OpenBSD,\nwhere zombie proceses would only be cleared after a new connection is\nreceived.\n\nThe underlying problem is that when this code was originally introduced,\nSA_RESTART was not widely implemented, and the signal() call usually\nimplemented SysV like semantics, at least until it started being\nreimplemented by calling sigaction() internally.\n\nThe main change is implemented in the third patch, but the changes to\nprepare for it that were done in the second patch, also solve a know crasher\nin AIX.\n\nCarlo Marcelo Arenas Belón (3):\n compat/posix.h: track SA_RESTART fallback\n daemon: use sigaction() to install child_handler()\n daemon: explicitly allow EINTR during poll()\n\n Makefile | 6 ++++++\n compat/posix.h | 7 +++++++\n config.mak.uname | 7 ++++---\n configure.ac | 5 +++++\n daemon.c | 30 +++++++++++++++++++++++++-----\n meson.build | 1 +\n 6 files changed, 48 insertions(+), 8 deletions(-)\n\n\nbase-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v1\nPull-Request: https://github.com/git/git/pull/2002\n-- \ngitgitgadget\n"},{"id":"520643","messageId":"2b5a58e53ac68e39a72e23bb40b386366ff03485.1750774122.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.git.git.1750774122.gitgitgadget@gmail.com","subject":"[PATCH 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-24T14:08:40Z","receivedAt":"2025-06-24T14:08:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nSystems without SA_RESTART where using custom CFLAGS instead of\nthe standard header file.\n\nConsolidate that, so it will be easier to use in a future commit.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n compat/posix.h | 7 +++++++\n config.mak.uname | 3 ---\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 067a00f33b83..2612a8515897 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -250,6 +250,13 @@ char *gitdirname(char *);\n #define NAME_MAX 255\n #endif\n \n+/* On most systems would have given us this, but\n+ * not on some systems (e.g. NonStop, QNX).\n+ */\n+#ifndef SA_RESTART\n+#define SA_RESTART 0\t/* disabled for sigaction() */\n+#endif\n+\n typedef uintmax_t timestamp_t;\n #define PRItime PRIuMAX\n #define parse_timestamp strtoumax\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b1c5c4d5e8ed..52160ef5cb07 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -654,8 +654,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \n \t# Not detected (nor checked for) by './configure'.\n-\t# We don't have SA_RESTART on NonStop, unfortunalety.\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \t# Apparently needed in compat/fnmatch/fnmatch.c.\n \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n@@ -782,7 +780,6 @@ ifeq ($(uname_S),MINGW)\n endif\n endif\n ifeq ($(uname_S),QNX)\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n \tHAVE_STRINGS_H = YesPlease\n \tNEEDS_SOCKET = YesPlease\n-- \ngitgitgadget\n\n"},{"id":"520644","messageId":"2e8c4643a60e354d24bda9bf364e1b34ce1c45ae.1750774122.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.git.git.1750774122.gitgitgadget@gmail.com","subject":"[PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-24T14:08:41Z","receivedAt":"2025-06-24T14:08:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIn a future change, the flags used for processing SIGCHLD will need to\nbe updated, which is only possible by using sigaction().\n\nReplace the call, which hs the added benefit of using BSD semantics\nreliably and therefore not needing the rearming call.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd5789..d870ad2f63c1 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)\n \t/*\n \t * Otherwise empty handler because systemcalls will get interrupted\n \t * upon signal receipt\n-\t * SysV needs the handler to be rearmed\n \t */\n-\tsignal(SIGCHLD, child_handler);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct pollfd *pfd;\n+\tstruct sigaction sa;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n \n@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n+\tsa.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sa, NULL);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n-- \ngitgitgadget\n\n"},{"id":"520645","messageId":"a450bdb0066912d135dd242090b012de0bc18180.1750774122.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.git.git.1750774122.gitgitgadget@gmail.com","subject":"[PATCH 3/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-24T14:08:42Z","receivedAt":"2025-06-24T14:08:48Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIf the setup for the SIGCHLD signal handler sets SA_RESTART, poll()\nmight not return with -1 and set errno to EINTR when a signal is\nreceived.\n\nSince the logic to reap zombie childs relies om those interruptions\nmake sure to explicitly disable SA_RESTART around this function.\n\nAdd a Makefile flag for portability to systems that don't have the\nfunctionality to change those flags or where it is not needed.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 6 ++++++\n config.mak.uname | 4 ++++\n configure.ac | 5 +++++\n daemon.c | 26 ++++++++++++++++++++++----\n meson.build | 1 +\n 5 files changed, 38 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 70d1543b6b86..d489f94dc6f0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -144,6 +144,9 @@ include shared.mak\n # Define NO_PREAD if you have a problem with pread() system call (e.g.\n # cygwin1.dll before v1.5.22).\n #\n+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART\n+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.\n+#\n # Define NO_SETITIMER if you don't have setitimer()\n #\n # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval\n@@ -1902,6 +1905,9 @@ ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n endif\n+ifdef NO_SIGINTERRUPT\n+\tCOMPAT_CFLAGS += -DNO_SIGINTERRUPT\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 52160ef5cb07..e824b45a4020 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -486,6 +486,7 @@ ifeq ($(uname_S),Windows)\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_INTTYPES_H = YesPlease\n+\tNO_SIGINTERRUPT = YesPlease\n \tCSPRNG_METHOD = rtlgenrandom\n \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n \t# so we don't need this:\n@@ -661,6 +662,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tNO_PREAD = YesPlease\n \tNO_MMAP = YesPlease\n \tNO_POLL = YesPlease\n+\tNO_SIGINTERRUPT = UnfortunatelyYes\n \tNO_INTPTR_T = UnfortunatelyYes\n \tCSPRNG_METHOD = openssl\n \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n@@ -697,6 +699,7 @@ ifeq ($(uname_S),MINGW)\n \tNEEDS_LIBICONV = YesPlease\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tNO_SIGINTERRUPT = YesPlease\n \tNO_SVN_TESTS = YesPlease\n \n \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n@@ -791,4 +794,5 @@ ifeq ($(uname_S),QNX)\n \tNO_PTHREADS = YesPlease\n \tNO_STRCASESTR = YesPlease\n \tNO_STRLCPY = YesPlease\n+\tNO_SIGINTERRUPT = UnfortunatelyYes\n endif\ndiff --git a/configure.ac b/configure.ac\nindex f6caab919a3e..2abb2a32cd1e 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -1192,6 +1192,11 @@ GIT_CHECK_FUNC(getdelim,\n [HAVE_GETDELIM=])\n GIT_CONF_SUBST([HAVE_GETDELIM])\n #\n+# Define NO_SIGINTERRUPT if you don't have siginterrupt.\n+GIT_CHECK_FUNC(siginterrupt,\n+[NO_SIGINTERRUPT=],\n+[NO_SIGINTERRUPT=YesPlease])\n+GIT_CONF_SUBST([NO_SIGINTERRUPT])\n #\n # Define NO_MMAP if you want to avoid mmap.\n #\ndiff --git a/daemon.c b/daemon.c\nindex d870ad2f63c1..542e63822391 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -912,12 +912,16 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tadd_child(&cld, addr, addrlen);\n }\n \n-static void child_handler(int signo UNUSED)\n+static void child_handler(int signo)\n {\n \t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n+\t * Empty handler because systemcalls should get interrupted\n+\t * upon signal receipt.\n \t */\n+#ifdef NO_SIGINTERRUPT\n+\t/* SysV needs the handler to be rearmed */\n+\tsignal(signo, child_handler);\n+#endif\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1118,8 +1122,10 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \n static int service_loop(struct socketlist *socklist)\n {\n-\tstruct pollfd *pfd;\n+#ifndef NO_SIGINTERRUPT\n \tstruct sigaction sa;\n+#endif\n+\tstruct pollfd *pfd;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n \n@@ -1128,14 +1134,22 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n+#ifdef NO_SIGINTERRUPT\n+\tsignal(SIGCHLD, child_handler);\n+#else\n \tsigemptyset(&sa.sa_mask);\n \tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n \tsa.sa_handler = child_handler;\n \tsigaction(SIGCHLD, &sa, NULL);\n+#endif\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n \n+#ifndef NO_SIGINTERRUPT\n+\t\tsa.sa_flags &= ~SA_RESTART;\n+\t\tsigaction(SIGCHLD, &sa, NULL);\n+#endif\n \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n@@ -1144,6 +1158,10 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+#ifndef NO_SIGINTERRUPT\n+\t\tsa.sa_flags |= SA_RESTART;\n+\t\tsigaction(SIGCHLD, &sa, NULL);\n+#endif\n \n \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\ndiff --git a/meson.build b/meson.build\nindex 7fea4a34d684..54942db151e8 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1361,6 +1361,7 @@ checkfuncs = {\n 'setenv' : ['setenv.c'],\n 'mkdtemp' : ['mkdtemp.c'],\n 'initgroups' : [],\n+ 'siginterrupt' : [],\n 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n 'pread' : ['pread.c'],\n }\n-- \ngitgitgadget\n"},{"id":"520646","messageId":"ixvokc6osmpxbhsla2s22yldtf6kuyxptnx2f6jh3cchteett4@ubvbyxpb527c","threadId":"63688","inReplyTo":"a450bdb0066912d135dd242090b012de0bc18180.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-24T14:33:22Z","receivedAt":"2025-06-24T14:33:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This will need the following fixup on top:\n\n---- >8 ----\ndiff --git a/daemon.c b/daemon.c\nindex 542e638223..f5f0c426c3 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -912,7 +912,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tadd_child(&cld, addr, addrlen);\n }\n \n-static void child_handler(int signo)\n+static void child_handler(int signo UNUSED)\n {\n \t/*\n \t * Empty handler because systemcalls should get interrupted\n"},{"id":"520648","messageId":"xmqq34bp2lws.fsf@gitster.g","threadId":"63688","inReplyTo":"2b5a58e53ac68e39a72e23bb40b386366ff03485.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T15:34:43Z","receivedAt":"2025-06-24T15:34:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>\n> Systems without SA_RESTART where using custom CFLAGS instead of\n> the standard header file.\n\nThis is not a sentence, isn't it? \"where\" -> \"are\"??? I dunno.\n\n> Consolidate that, so it will be easier to use in a future commit.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> compat/posix.h | 7 +++++++\n> config.mak.uname | 3 ---\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/compat/posix.h b/compat/posix.h\n> index 067a00f33b83..2612a8515897 100644\n> --- a/compat/posix.h\n> +++ b/compat/posix.h\n> @@ -250,6 +250,13 @@ char *gitdirname(char *);\n> #define NAME_MAX 255\n> #endif\n> \n> +/* On most systems would have given us this, but\n> + * not on some systems (e.g. NonStop, QNX).\n> + */\n> +#ifndef SA_RESTART\n> +#define SA_RESTART 0\t/* disabled for sigaction() */\n> +#endif\n\nSo not just on QNX and NonStop, we have SA_RESTART defined\neverywhere. I do not know offhand what the ramifications of this\nchange is, but we seem to use the symbol in places outside #ifdef\nmeaning that anybody other than QNX and NonStop that lack SA_RESTART\nwouldn't have been able to build Git before this change, and now\nthey would be. The resulting binary may not work for them at all\nyet but that is not any worse than before.\n\n> typedef uintmax_t timestamp_t;\n> #define PRItime PRIuMAX\n> #define parse_timestamp strtoumax\n> diff --git a/config.mak.uname b/config.mak.uname\n> index b1c5c4d5e8ed..52160ef5cb07 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -654,8 +654,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n> \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n> \n> \t# Not detected (nor checked for) by './configure'.\n> -\t# We don't have SA_RESTART on NonStop, unfortunalety.\n> -\tCOMPAT_CFLAGS += -DSA_RESTART=0\n> \t# Apparently needed in compat/fnmatch/fnmatch.c.\n> \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n> \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n> @@ -782,7 +780,6 @@ ifeq ($(uname_S),MINGW)\n> endif\n> endif\n> ifeq ($(uname_S),QNX)\n> -\tCOMPAT_CFLAGS += -DSA_RESTART=0\n> \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n> \tHAVE_STRINGS_H = YesPlease\n> \tNEEDS_SOCKET = YesPlease\n"},{"id":"520649","messageId":"xmqqv7ol177p.fsf@gitster.g","threadId":"63688","inReplyTo":"2e8c4643a60e354d24bda9bf364e1b34ce1c45ae.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T15:37:30Z","receivedAt":"2025-06-24T15:37:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>\n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n>\n> Replace the call, which hs the added benefit of using BSD semantics\n> reliably and therefore not needing the rearming call.\n\n\"hs\" -> \"has\"\n\nHmph, if we do not have to rearm, do we even need to have the\nhandler at all, now it is a completely empty function? Presumably\nwe'll see the answer to this question in the next step?\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index d1be61fd5789..d870ad2f63c1 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)\n> \t/*\n> \t * Otherwise empty handler because systemcalls will get interrupted\n> \t * upon signal receipt\n> -\t * SysV needs the handler to be rearmed\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> static int service_loop(struct socketlist *socklist)\n> {\n> \tstruct pollfd *pfd;\n> +\tstruct sigaction sa;\n> \n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> \n> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> +\tsigemptyset(&sa.sa_mask);\n> +\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> +\tsa.sa_handler = child_handler;\n> +\tsigaction(SIGCHLD, &sa, NULL);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n"},{"id":"520650","messageId":"xmqqqzz916xu.fsf@gitster.g","threadId":"63688","inReplyTo":"a450bdb0066912d135dd242090b012de0bc18180.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T15:43:25Z","receivedAt":"2025-06-24T15:43:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> -static void child_handler(int signo UNUSED)\n> +static void child_handler(int signo)\n> {\n> \t/*\n> -\t * Otherwise empty handler because systemcalls will get interrupted\n> -\t * upon signal receipt\n> +\t * Empty handler because systemcalls should get interrupted\n> +\t * upon signal receipt.\n> \t */\n> +#ifdef NO_SIGINTERRUPT\n> +\t/* SysV needs the handler to be rearmed */\n> +\tsignal(signo, child_handler);\n> +#endif\n> }\n\nOn NO_SIGINTERRUPT systems, signo is UNUSED, isn't it?\n\nCan we abstract this a bit better? #if/#else/#endif sprinkled\neverywhere is simply an eyesore.\n\n> static int set_reuse_addr(int sockfd)\n> @@ -1118,8 +1122,10 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> \n> static int service_loop(struct socketlist *socklist)\n> {\n> -\tstruct pollfd *pfd;\n> +#ifndef NO_SIGINTERRUPT\n> \tstruct sigaction sa;\n> +#endif\n> +\tstruct pollfd *pfd;\n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> \n> @@ -1128,14 +1134,22 @@ static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> +#ifdef NO_SIGINTERRUPT\n> +\tsignal(SIGCHLD, child_handler);\n> +#else\n> \tsigemptyset(&sa.sa_mask);\n> \tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> \tsa.sa_handler = child_handler;\n> \tsigaction(SIGCHLD, &sa, NULL);\n> +#endif\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n> \n> +#ifndef NO_SIGINTERRUPT\n> +\t\tsa.sa_flags &= ~SA_RESTART;\n> +\t\tsigaction(SIGCHLD, &sa, NULL);\n> +#endif\n> \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> \t\t\tif (errno != EINTR) {\n> \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n> @@ -1144,6 +1158,10 @@ static int service_loop(struct socketlist *socklist)\n> \t\t\t}\n> \t\t\tcontinue;\n> \t\t}\n> +#ifndef NO_SIGINTERRUPT\n> +\t\tsa.sa_flags |= SA_RESTART;\n> +\t\tsigaction(SIGCHLD, &sa, NULL);\n> +#endif\n"},{"id":"520658","messageId":"xmqq5xgl1589.fsf@gitster.g","threadId":"63688","inReplyTo":"2e8c4643a60e354d24bda9bf364e1b34ce1c45ae.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T16:20:22Z","receivedAt":"2025-06-24T16:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>\n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n>\n> Replace the call, which hs the added benefit of using BSD semantics\n> reliably and therefore not needing the rearming call.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n\nHmph. Wouldn't it a much smaller change and fix to discard 2/3 and\nmost of the 3/3 and instead make a siginterrupt() call to tell the\nsystem to interrupt us when SIGCHLD is received only on platforms\nwhere siginterrupt() is available? Use of sigaction() does not seem\nto be buying us anything for the purpose of this series.\n\n> diff --git a/daemon.c b/daemon.c\n> index d1be61fd5789..d870ad2f63c1 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)\n> \t/*\n> \t * Otherwise empty handler because systemcalls will get interrupted\n> \t * upon signal receipt\n> -\t * SysV needs the handler to be rearmed\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> static int service_loop(struct socketlist *socklist)\n> {\n> \tstruct pollfd *pfd;\n> +\tstruct sigaction sa;\n> \n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> \n> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> +\tsigemptyset(&sa.sa_mask);\n> +\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> +\tsa.sa_handler = child_handler;\n> +\tsigaction(SIGCHLD, &sa, NULL);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n"},{"id":"520659","messageId":"xmqqy0thyuhf.fsf@gitster.g","threadId":"63688","inReplyTo":"2b5a58e53ac68e39a72e23bb40b386366ff03485.1750774122.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T16:28:28Z","receivedAt":"2025-06-24T16:28:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\nTwo references to Documentation/CodingGuidelines\n\n> +/* On most systems would have given us this, but\n> + * not on some systems (e.g. NonStop, QNX).\n> + */\n\n - Multi-line comments include their delimiters on separate lines from\n the text.\n\n> +#ifndef SA_RESTART\n> +#define SA_RESTART 0\t/* disabled for sigaction() */\n> +#endif\n\n - Nested C preprocessor directives are indented after the hash by one\n space per nesting level.\n\n"},{"id":"520661","messageId":"7f3ac4djbbhskbryzr754kdjdiyauiiy5dduv7h2uaa7mvafsr@chntkatmbbcb","threadId":"63688","inReplyTo":"xmqq5xgl1589.fsf@gitster.g","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-24T21:28:37Z","receivedAt":"2025-06-24T21:28:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jun 24, 2025 at 09:20:22AM -0800, Junio C Hamano wrote:\n> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> writes:\n> \n> > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> >\n> > In a future change, the flags used for processing SIGCHLD will need to\n> > be updated, which is only possible by using sigaction().\n> >\n> > Replace the call, which hs the added benefit of using BSD semantics\n> > reliably and therefore not needing the rearming call.\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belón \n> > ---\n> > daemon.c | 8 +++++---\n> > 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> Hmph. Wouldn't it a much smaller change and fix to discard 2/3 and\n> most of the 3/3 and instead make a siginterrupt() call to tell the\n> system to interrupt us when SIGCHLD is received only on platforms\n> where siginterrupt() is available? Use of sigaction() does not seem\n> to be buying us anything for the purpose of this series.\n\nUsing siginterrupt() would work (at least it did when I tested it in\nOpenBSD), but its use is discouraged as it has been obsoleted by the\nlast two versions of POSIX (since 2018).\n\nIndeed that code fails to build[1] in recent Linux with :\n\n daemon.c: In function ‘service_loop’:\n daemon.c:1138:17: error: ‘siginterrupt’ is deprecated: Use sigaction with SA_RESTART instead [-Werror=deprecated-declarations]\n 1138 | siginterrupt(SIGCHLD, 1);\n | ^~~~~~~~~~~~\n In file included from compat/posix.h:112,\n from git-compat-util.h:26,\n from daemon.c:3:\n /usr/include/signal.h:324:12: note: declared here\n 324 | extern int siginterrupt (int __sig, int __interrupt) __THROW\n | ^~~~~~~~~~~~\n\nMost systems seem to be implementing `signal()` with `sigaction()`\nnowadays, but in the ones that are not (ex: Solaris) calling the later\nto get a `struct sigaction` with the flags being used, doesn't work\nand therefore it would seem, that the only way to do this reliably is\nby using sigaction everywhere for this signal, as implemented in 2/3.\n\nCarlo\n\n[1] https://github.com/git/git/actions/runs/15849572148\n"},{"id":"520662","messageId":"xmqqms9wzuz2.fsf@gitster.g","threadId":"63688","inReplyTo":"7f3ac4djbbhskbryzr754kdjdiyauiiy5dduv7h2uaa7mvafsr@chntkatmbbcb","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-24T21:32:33Z","receivedAt":"2025-06-24T21:32:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n>> Hmph. Wouldn't it a much smaller change and fix to discard 2/3 and\n>> most of the 3/3 and instead make a siginterrupt() call to tell the\n>> system to interrupt us when SIGCHLD is received only on platforms\n>> where siginterrupt() is available? Use of sigaction() does not seem\n>> to be buying us anything for the purpose of this series.\n>\n> Using siginterrupt() would work (at least it did when I tested it in\n> OpenBSD), but its use is discouraged as it has been obsoleted by the\n> last two versions of POSIX (since 2018).\n\nOK, but it feels a bit funny to base the conditional compilation to\nuse sigaction() (as opposed to signal()) on a symbol whose name was\nderived from that deprecated interface, doesn't it, then?\n\n> Most systems seem to be implementing `signal()` with `sigaction()`\n> nowadays, but in the ones that are not (ex: Solaris) calling the later\n> to get a `struct sigaction` with the flags being used, doesn't work\n> and therefore it would seem, that the only way to do this reliably is\n> by using sigaction everywhere for this signal, as implemented in 2/3.\n\nStill the many #ifdef's sprinkled all over looked really ugly. Can\nwe abstract this out a bit better?\n\nThanks.\n"},{"id":"520663","messageId":"neeiqdzggdukyfd5metm56nq6tnperhcnzvgvt4e6idw52rxeg@qrwzjoexs35e","threadId":"63688","inReplyTo":"xmqqv7ol177p.fsf@gitster.g","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-24T22:29:25Z","receivedAt":"2025-06-24T22:29:27Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jun 24, 2025 at 08:37:30AM -0800, Junio C Hamano wrote:\n> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> writes:\n> \n> > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> >\n> > In a future change, the flags used for processing SIGCHLD will need to\n> > be updated, which is only possible by using sigaction().\n> >\n> > Replace the call, which hs the added benefit of using BSD semantics\n> > reliably and therefore not needing the rearming call.\n> \n> \"hs\" -> \"has\"\n> \n> Hmph, if we do not have to rearm, do we even need to have the\n> handler at all, now it is a completely empty function? Presumably\n> we'll see the answer to this question in the next step?\n\nI didn'r address it because I didn't knew where to put it, but removing\nthe signal handler isn't possible, because as soon as we do, EINTR is no\nlonger \"returned\" by `poll()` on the systems that allowed that as an \nexception to SA_RESTART rules, and even trying to force it with\n`siginterrupt()` no longer works, not even returning an error.\n\nCarlo\n"},{"id":"520665","messageId":"CAPx1GveWK55UWksUXNUf2wU3o88SLyXF0LGFHAX4_b3-cBa=Bg@mail.gmail.com","threadId":"63688","inReplyTo":"neeiqdzggdukyfd5metm56nq6tnperhcnzvgvt4e6idw52rxeg@qrwzjoexs35e","subject":"Re: [PATCH 2/3] daemon: use sigaction() to install child_handler()","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2025-06-24T23:27:46Z","receivedAt":"2025-06-24T23:28:01Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Tue, Jun 24, 2025 at 3:29 PM Carlo Marcelo Arenas Belón\n wrote:\n> I didn'r address it because I didn't knew where to put it, but removing\n> the signal handler isn't possible, because as soon as we do, EINTR is no\n> longer \"returned\" by `poll()` on the systems that allowed that as an\n> exception to SA_RESTART rules ...\n\nYes, the signal handling stuff in POSIX is messy for Hysterical Raisins.\n\nIf we go back to V6/V7 Unix we only had three options:\n\n * signal is default-handled, or\n * signal is ignored, or\n * signal is caught: receipt of signal calls user-supplied function.\n\nand the disposition of the signal was always reset to default if\ncaught, so the user function had to call signal() again. Which was\nsort of OK but had the obvious race flaw, if the signal is delivered\ntwice, before the user function could be re-armed, well, Too Bad.\n\n4.1BSD and 2.9BSD Unix added the \"reliable signal\" mechanism through a\nmagic library (-ljobs), with several new system calls internally,\nwhich morphed into the more-standard-ish 4.2BSD `sigvec` system call,\nwhich in turn morphed into the POSIX `sigaction` call. But this still\nretained the three basic options: default, ignore, or\ncatch-at-user-function. So you have to have a function, even if it\ndoesn't do anything.\n\nMeanwhile System III programmers discovered the problem with\nchild-is-ready-to-be-reaped signals (SIGCLD in that variant) getting\nlost because a SIGCLD handler could not re-catch the signal in time,\nand \"solved\" it by a horrible hack that (they claimed) required no\nuser intervention. Given that a user program using SIGCLD already had\na handler -- let's call it `child_exited` -- that called\n`signal(SIGCLD, child_exited)` somewhere within the handler, the hack\nwas to have the OS *re-generate* the signal if there was currently a\ncollectable zombie.\n\nThis of course depended strongly on the actual code in `child_exited`,\nwhich *had to* call `wait` once *before* calling `signal` to re-arm\nthe signal. Otherwise you get infinite recursion. But in fact the\nprogram(s?) that they cared about did call `signal` at the end, rather\nthan at the start, of their handler(s).\n\nPOSIX took `sigvec` and added flags, like `SA_RESTART` and\n`SA_RESETHAND`, but otherwise maintained a lot of backwards\ncompatibility with both schemes by making a lot of behavior optional.\nSo now you have to over-specify things.\n\nPeople keep trying to fix the klunky interface over time, but history\nis just too messy...\n\nChris\n"},{"id":"520672","messageId":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.git.git.1750774122.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-25T07:35:25Z","receivedAt":"2025-06-25T07:35:32Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series addresses and ambiguity that is at least visible in OpenBSD,\nwhere zombie proceses would only be cleared after a new connection is\nreceived.\n\nThe underlying problem is that when this code was originally introduced,\nSA_RESTART was not widely implemented, and the signal() call usually\nimplemented SysV like semantics, at least until it started being\nreimplemented by calling sigaction() internally.\n\nChanges since v1\n\n * Almost all references to siginterrupt has been removed and a better named\n variable used instead\n * Changes had been anstracted to minimize ifdefs and their introduction\n staged more naturally\n\nCarlo Marcelo Arenas Belón (3):\n compat/posix.h: track SA_RESTART fallback\n daemon: use sigaction() to install child_handler()\n daemon: explicitly allow EINTR during poll()\n\n Makefile | 6 +++++\n compat/mingw-posix.h | 1 -\n compat/posix.h | 8 +++++++\n config.mak.uname | 7 +++---\n configure.ac | 17 +++++++++++++++\n daemon.c | 52 +++++++++++++++++++++++++++++++++++++++-----\n meson.build | 4 ++++\n 7 files changed, 85 insertions(+), 10 deletions(-)\n\n\nbase-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v2\nPull-Request: https://github.com/git/git/pull/2002\n\nRange-diff vs v1:\n\n 1: 2b5a58e53ac ! 1: e82b7425bbc compat/posix.h: track SA_RESTART fallback\n @@ Metadata\n ## Commit message ##\n compat/posix.h: track SA_RESTART fallback\n \n - Systems without SA_RESTART where using custom CFLAGS instead of\n - the standard header file.\n + Systems without SA_RESTART are using custom CFLAGS or headers\n + instead of the standard header file.\n \n - Consolidate that, so it will be easier to use in a future commit.\n + Correct that, and invent a Makefile variable to track the\n + exceptions which will become handy in the next commits.\n \n Signed-off-by: Carlo Marcelo Arenas Belón \n \n + ## Makefile ##\n +@@ Makefile: include shared.mak\n + # when attempting to read from an fopen'ed directory (or even to fopen\n + # it at all).\n + #\n ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n ++# prefer to use ANSI C signal() over POSIX sigaction()\n ++#\n + # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n + # when a signal is received (as opposed to restarting).\n + #\n +@@ Makefile: ifdef FREAD_READS_DIRECTORIES\n + \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n + \tCOMPAT_OBJS += compat/fopen.o\n + endif\n ++ifdef USE_NON_POSIX_SIGNAL\n ++\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n ++endif\n + ifdef OPEN_RETURNS_EINTR\n + \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n + endif\n +\n + ## compat/mingw-posix.h ##\n +@@ compat/mingw-posix.h: struct sigaction {\n + \tsig_handler_t sa_handler;\n + \tunsigned sa_flags;\n + };\n +-#define SA_RESTART 0\n + \n + struct itimerval {\n + \tstruct timeval it_value, it_interval;\n +\n ## compat/posix.h ##\n @@ compat/posix.h: char *gitdirname(char *);\n #define NAME_MAX 255\n #endif\n \n -+/* On most systems would have given us this, but\n ++/*\n ++ * On most systems would have given us this, but\n + * not on some systems (e.g. NonStop, QNX).\n + */\n +#ifndef SA_RESTART\n -+#define SA_RESTART 0\t/* disabled for sigaction() */\n ++# define SA_RESTART 0\t/* disabled for sigaction() */\n +#endif\n +\n typedef uintmax_t timestamp_t;\n @@ compat/posix.h: char *gitdirname(char *);\n #define parse_timestamp strtoumax\n \n ## config.mak.uname ##\n +@@ config.mak.uname: ifeq ($(uname_S),Windows)\n + \tNO_STRTOUMAX = YesPlease\n + \tNO_MKDTEMP = YesPlease\n + \tNO_INTTYPES_H = YesPlease\n ++\tUSE_NON_POSIX_SIGNAL = YesPlease\n + \tCSPRNG_METHOD = rtlgenrandom\n + \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n + \t# so we don't need this:\n @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \n @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n \t# Apparently needed in compat/fnmatch/fnmatch.c.\n \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n + \tNO_MMAP = YesPlease\n + \tNO_POLL = YesPlease\n + \tNO_INTPTR_T = UnfortunatelyYes\n ++\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n + \tCSPRNG_METHOD = openssl\n + \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n + \tSHELL_PATH = /usr/coreutils/bin/bash\n +@@ config.mak.uname: ifeq ($(uname_S),MINGW)\n + \tNEEDS_LIBICONV = YesPlease\n + \tNO_STRTOUMAX = YesPlease\n + \tNO_MKDTEMP = YesPlease\n ++\tUSE_NON_POSIX_SIGNAL = YesPlease\n + \tNO_SVN_TESTS = YesPlease\n + \n + \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n endif\n endif\n @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n \tHAVE_STRINGS_H = YesPlease\n \tNEEDS_SOCKET = YesPlease\n +@@ config.mak.uname: ifeq ($(uname_S),QNX)\n + \tNO_PTHREADS = YesPlease\n + \tNO_STRCASESTR = YesPlease\n + \tNO_STRLCPY = YesPlease\n ++\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n + endif\n +\n + ## configure.ac ##\n +@@ configure.ac: fi\n + GIT_CONF_SUBST([ICONV_OMITS_BOM])\n + fi\n + \n ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n ++# prefer using ANSI C signal() over POSIX sigaction()\n ++\n ++AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n ++\tAC_COMPILE_IFELSE(\n ++\t\t[AC_LANG_PROGRAM([#include ], [[\n ++\t\t#ifdef SA_RESTART\n ++\t\t#endif\n ++\t\tsiginterrupt(SIGCHLD, 1)\n ++\t\t]])],[ac_cv_siginterrupt=yes],[\n ++\t\t\tac_cv_siginterrupt=no\n ++\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n ++\t\t]\n ++\t)\n ++])\n ++GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n ++\n + ## Checks for typedefs, structures, and compiler characteristics.\n + AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n + #\n +\n + ## meson.build ##\n +@@ meson.build: else\n + build_options_config.set('NO_EXPAT', '1')\n + endif\n + \n ++if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n ++ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'\n ++endif\n ++\n + if not compiler.has_header('sys/select.h')\n + libgit_c_args += '-DNO_SYS_SELECT_H'\n + endif\n 2: 2e8c4643a60 ! 2: 05d945aa1e5 daemon: use sigaction() to install child_handler()\n @@ Commit message\n In a future change, the flags used for processing SIGCHLD will need to\n be updated, which is only possible by using sigaction().\n \n - Replace the call, which hs the added benefit of using BSD semantics\n - reliably and therefore not needing the rearming call.\n + Factor out the call to set the signal handler and use sigaction instead\n + of signal for the systems that allow that, which has the added benefit\n + of using BSD semantics reliably and therefore not needing the rearming\n + call.\n \n Signed-off-by: Carlo Marcelo Arenas Belón \n \n ## daemon.c ##\n -@@ daemon.c: static void child_handler(int signo UNUSED)\n +@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n + \t\tadd_child(&cld, addr, addrlen);\n + }\n + \n +-static void child_handler(int signo UNUSED)\n ++static void child_handler(int signo MAYBE_UNUSED)\n + {\n \t/*\n - \t * Otherwise empty handler because systemcalls will get interrupted\n - \t * upon signal receipt\n +-\t * Otherwise empty handler because systemcalls will get interrupted\n +-\t * upon signal receipt\n -\t * SysV needs the handler to be rearmed\n ++\t * Otherwise empty handler because systemcalls should get interrupted\n ++\t * upon signal receipt.\n \t */\n -\tsignal(SIGCHLD, child_handler);\n ++#ifdef USE_NON_POSIX_SIGNAL\n ++\t/*\n ++\t * SysV needs the handler to be rearmed, but this is known\n ++\t * to trigger infinite recursion crashes at least in AIX.\n ++\t */\n ++\tsignal(signo, child_handler);\n ++#endif\n }\n \n static int set_reuse_addr(int sockfd)\n @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n + \t}\n + }\n + \n ++#ifndef USE_NON_POSIX_SIGNAL\n ++\n ++static void set_signal_handler(struct sigaction *psa)\n ++{\n ++\tsigemptyset(&psa->sa_mask);\n ++\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n ++\tpsa->sa_handler = child_handler;\n ++\tsigaction(SIGCHLD, psa, NULL);\n ++}\n ++\n ++#else\n ++\n ++static void set_signal_handler(struct sigaction *psa UNUSED)\n ++{\n ++\tsignal(SIGCHLD, child_handler);\n ++}\n ++\n static int service_loop(struct socketlist *socklist)\n {\n - \tstruct pollfd *pfd;\n +\tstruct sigaction sa;\n + \tstruct pollfd *pfd;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n - \n @@ daemon.c: static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n -\tsignal(SIGCHLD, child_handler);\n -+\tsigemptyset(&sa.sa_mask);\n -+\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n -+\tsa.sa_handler = child_handler;\n -+\tsigaction(SIGCHLD, &sa, NULL);\n ++\tset_signal_handler(&sa);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n 3: a450bdb0066 ! 3: b737e0389df daemon: explicitly allow EINTR during poll()\n @@ Commit message\n might not return with -1 and set errno to EINTR when a signal is\n received.\n \n - Since the logic to reap zombie childs relies om those interruptions\n + Since the logic to reap zombie childs relies on those interruptions\n make sure to explicitly disable SA_RESTART around this function.\n \n - Add a Makefile flag for portability to systems that don't have the\n - functionality to change those flags or where it is not needed.\n -\n Signed-off-by: Carlo Marcelo Arenas Belón \n \n - ## Makefile ##\n -@@ Makefile: include shared.mak\n - # Define NO_PREAD if you have a problem with pread() system call (e.g.\n - # cygwin1.dll before v1.5.22).\n - #\n -+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART\n -+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.\n -+#\n - # Define NO_SETITIMER if you don't have setitimer()\n - #\n - # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval\n -@@ Makefile: ifdef NO_PREAD\n - \tCOMPAT_CFLAGS += -DNO_PREAD\n - \tCOMPAT_OBJS += compat/pread.o\n - endif\n -+ifdef NO_SIGINTERRUPT\n -+\tCOMPAT_CFLAGS += -DNO_SIGINTERRUPT\n -+endif\n - ifdef NO_FAST_WORKING_DIRECTORY\n - \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n - endif\n -\n - ## config.mak.uname ##\n -@@ config.mak.uname: ifeq ($(uname_S),Windows)\n - \tNO_STRTOUMAX = YesPlease\n - \tNO_MKDTEMP = YesPlease\n - \tNO_INTTYPES_H = YesPlease\n -+\tNO_SIGINTERRUPT = YesPlease\n - \tCSPRNG_METHOD = rtlgenrandom\n - \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n - \t# so we don't need this:\n -@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n - \tNO_PREAD = YesPlease\n - \tNO_MMAP = YesPlease\n - \tNO_POLL = YesPlease\n -+\tNO_SIGINTERRUPT = UnfortunatelyYes\n - \tNO_INTPTR_T = UnfortunatelyYes\n - \tCSPRNG_METHOD = openssl\n - \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n -@@ config.mak.uname: ifeq ($(uname_S),MINGW)\n - \tNEEDS_LIBICONV = YesPlease\n - \tNO_STRTOUMAX = YesPlease\n - \tNO_MKDTEMP = YesPlease\n -+\tNO_SIGINTERRUPT = YesPlease\n - \tNO_SVN_TESTS = YesPlease\n - \n - \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n -@@ config.mak.uname: ifeq ($(uname_S),QNX)\n - \tNO_PTHREADS = YesPlease\n - \tNO_STRCASESTR = YesPlease\n - \tNO_STRLCPY = YesPlease\n -+\tNO_SIGINTERRUPT = UnfortunatelyYes\n - endif\n -\n - ## configure.ac ##\n -@@ configure.ac: GIT_CHECK_FUNC(getdelim,\n - [HAVE_GETDELIM=])\n - GIT_CONF_SUBST([HAVE_GETDELIM])\n - #\n -+# Define NO_SIGINTERRUPT if you don't have siginterrupt.\n -+GIT_CHECK_FUNC(siginterrupt,\n -+[NO_SIGINTERRUPT=],\n -+[NO_SIGINTERRUPT=YesPlease])\n -+GIT_CONF_SUBST([NO_SIGINTERRUPT])\n - #\n - # Define NO_MMAP if you want to avoid mmap.\n - #\n -\n ## daemon.c ##\n -@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n - \t\tadd_child(&cld, addr, addrlen);\n +@@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n + \tsigaction(SIGCHLD, psa, NULL);\n }\n \n --static void child_handler(int signo UNUSED)\n -+static void child_handler(int signo)\n - {\n - \t/*\n --\t * Otherwise empty handler because systemcalls will get interrupted\n --\t * upon signal receipt\n -+\t * Empty handler because systemcalls should get interrupted\n -+\t * upon signal receipt.\n - \t */\n -+#ifdef NO_SIGINTERRUPT\n -+\t/* SysV needs the handler to be rearmed */\n -+\tsignal(signo, child_handler);\n -+#endif\n ++static void set_sa_restart(struct sigaction *psa, int enable)\n ++{\n ++\tif (enable)\n ++\t\tpsa->sa_flags |= SA_RESTART;\n ++\telse\n ++\t\tpsa->sa_flags &= ~SA_RESTART;\n ++\tsigaction(SIGCHLD, psa, NULL);\n ++}\n ++\n + #else\n + \n + static void set_signal_handler(struct sigaction *psa UNUSED)\n +@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)\n + \tsignal(SIGCHLD, child_handler);\n }\n \n - static int set_reuse_addr(int sockfd)\n -@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n - \n ++static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n ++{\n ++}\n ++\n ++#endif\n ++\n static int service_loop(struct socketlist *socklist)\n {\n --\tstruct pollfd *pfd;\n -+#ifndef NO_SIGINTERRUPT\n \tstruct sigaction sa;\n -+#endif\n -+\tstruct pollfd *pfd;\n - \n - \tCALLOC_ARRAY(pfd, socklist->nr);\n - \n @@ daemon.c: static int service_loop(struct socketlist *socklist)\n - \t\tpfd[i].events = POLLIN;\n - \t}\n - \n -+#ifdef NO_SIGINTERRUPT\n -+\tsignal(SIGCHLD, child_handler);\n -+#else\n - \tsigemptyset(&sa.sa_mask);\n - \tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n - \tsa.sa_handler = child_handler;\n - \tsigaction(SIGCHLD, &sa, NULL);\n -+#endif\n - \n \tfor (;;) {\n \t\tcheck_dead_children();\n \n -+#ifndef NO_SIGINTERRUPT\n -+\t\tsa.sa_flags &= ~SA_RESTART;\n -+\t\tsigaction(SIGCHLD, &sa, NULL);\n -+#endif\n ++\t\tset_sa_restart(&sa, 0);\n \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n @@ daemon.c: static int service_loop(struct socketlist *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n -+#ifndef NO_SIGINTERRUPT\n -+\t\tsa.sa_flags |= SA_RESTART;\n -+\t\tsigaction(SIGCHLD, &sa, NULL);\n -+#endif\n ++\t\tset_sa_restart(&sa, 1);\n \n \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n -\n - ## meson.build ##\n -@@ meson.build: checkfuncs = {\n - 'setenv' : ['setenv.c'],\n - 'mkdtemp' : ['mkdtemp.c'],\n - 'initgroups' : [],\n -+ 'siginterrupt' : [],\n - 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n - 'pread' : ['pread.c'],\n - }\n\n-- \ngitgitgadget\n"},{"id":"520673","messageId":"e82b7425bbc2540fa5ef3fd4584e6f902485d064.1750836928.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-25T07:35:26Z","receivedAt":"2025-06-25T07:35:33Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nSystems without SA_RESTART are using custom CFLAGS or headers\ninstead of the standard header file.\n\nCorrect that, and invent a Makefile variable to track the\nexceptions which will become handy in the next commits.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 6 ++++++\n compat/mingw-posix.h | 1 -\n compat/posix.h | 8 ++++++++\n config.mak.uname | 7 ++++---\n configure.ac | 17 +++++++++++++++++\n meson.build | 4 ++++\n 6 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 70d1543b6b86..c4e6fc7bfd13 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -37,6 +37,9 @@ include shared.mak\n # when attempting to read from an fopen'ed directory (or even to fopen\n # it at all).\n #\n+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n+# prefer to use ANSI C signal() over POSIX sigaction()\n+#\n # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n # when a signal is received (as opposed to restarting).\n #\n@@ -1811,6 +1814,9 @@ ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n+ifdef USE_NON_POSIX_SIGNAL\n+\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n+endif\n ifdef OPEN_RETURNS_EINTR\n \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n endif\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex 88e0cf92924b..a0dca756d104 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -95,7 +95,6 @@ struct sigaction {\n \tsig_handler_t sa_handler;\n \tunsigned sa_flags;\n };\n-#define SA_RESTART 0\n \n struct itimerval {\n \tstruct timeval it_value, it_interval;\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 067a00f33b83..84ad2c9647aa 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -250,6 +250,14 @@ char *gitdirname(char *);\n #define NAME_MAX 255\n #endif\n \n+/*\n+ * On most systems would have given us this, but\n+ * not on some systems (e.g. NonStop, QNX).\n+ */\n+#ifndef SA_RESTART\n+# define SA_RESTART 0\t/* disabled for sigaction() */\n+#endif\n+\n typedef uintmax_t timestamp_t;\n #define PRItime PRIuMAX\n #define parse_timestamp strtoumax\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b1c5c4d5e8ed..1914982eb13e 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -486,6 +486,7 @@ ifeq ($(uname_S),Windows)\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_INTTYPES_H = YesPlease\n+\tUSE_NON_POSIX_SIGNAL = YesPlease\n \tCSPRNG_METHOD = rtlgenrandom\n \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n \t# so we don't need this:\n@@ -654,8 +655,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \n \t# Not detected (nor checked for) by './configure'.\n-\t# We don't have SA_RESTART on NonStop, unfortunalety.\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \t# Apparently needed in compat/fnmatch/fnmatch.c.\n \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n@@ -664,6 +663,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tNO_MMAP = YesPlease\n \tNO_POLL = YesPlease\n \tNO_INTPTR_T = UnfortunatelyYes\n+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n \tCSPRNG_METHOD = openssl\n \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n \tSHELL_PATH = /usr/coreutils/bin/bash\n@@ -699,6 +699,7 @@ ifeq ($(uname_S),MINGW)\n \tNEEDS_LIBICONV = YesPlease\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tUSE_NON_POSIX_SIGNAL = YesPlease\n \tNO_SVN_TESTS = YesPlease\n \n \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n@@ -782,7 +783,6 @@ ifeq ($(uname_S),MINGW)\n endif\n endif\n ifeq ($(uname_S),QNX)\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n \tHAVE_STRINGS_H = YesPlease\n \tNEEDS_SOCKET = YesPlease\n@@ -794,4 +794,5 @@ ifeq ($(uname_S),QNX)\n \tNO_PTHREADS = YesPlease\n \tNO_STRCASESTR = YesPlease\n \tNO_STRLCPY = YesPlease\n+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n endif\ndiff --git a/configure.ac b/configure.ac\nindex f6caab919a3e..8c52fd129f31 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -862,6 +862,23 @@ fi\n GIT_CONF_SUBST([ICONV_OMITS_BOM])\n fi\n \n+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n+# prefer using ANSI C signal() over POSIX sigaction()\n+\n+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n+\tAC_COMPILE_IFELSE(\n+\t\t[AC_LANG_PROGRAM([#include ], [[\n+\t\t#ifdef SA_RESTART\n+\t\t#endif\n+\t\tsiginterrupt(SIGCHLD, 1)\n+\t\t]])],[ac_cv_siginterrupt=yes],[\n+\t\t\tac_cv_siginterrupt=no\n+\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n+\t\t]\n+\t)\n+])\n+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n+\n ## Checks for typedefs, structures, and compiler characteristics.\n AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n #\ndiff --git a/meson.build b/meson.build\nindex 7fea4a34d684..f27c37e430ae 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1094,6 +1094,10 @@ else\n build_options_config.set('NO_EXPAT', '1')\n endif\n \n+if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n+ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'\n+endif\n+\n if not compiler.has_header('sys/select.h')\n libgit_c_args += '-DNO_SYS_SELECT_H'\n endif\n-- \ngitgitgadget\n\n"},{"id":"520674","messageId":"05d945aa1e546bcc028e215c8e4d174b5e0c32ad.1750836928.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-25T07:35:27Z","receivedAt":"2025-06-25T07:35:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIn a future change, the flags used for processing SIGCHLD will need to\nbe updated, which is only possible by using sigaction().\n\nFactor out the call to set the signal handler and use sigaction instead\nof signal for the systems that allow that, which has the added benefit\nof using BSD semantics reliably and therefore not needing the rearming\ncall.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 35 +++++++++++++++++++++++++++++------\n 1 file changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd5789..8133bd902157 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -912,14 +912,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tadd_child(&cld, addr, addrlen);\n }\n \n-static void child_handler(int signo UNUSED)\n+static void child_handler(int signo MAYBE_UNUSED)\n {\n \t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n-\t * SysV needs the handler to be rearmed\n+\t * Otherwise empty handler because systemcalls should get interrupted\n+\t * upon signal receipt.\n \t */\n-\tsignal(SIGCHLD, child_handler);\n+#ifdef USE_NON_POSIX_SIGNAL\n+\t/*\n+\t * SysV needs the handler to be rearmed, but this is known\n+\t * to trigger infinite recursion crashes at least in AIX.\n+\t */\n+\tsignal(signo, child_handler);\n+#endif\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1118,8 +1123,26 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \t}\n }\n \n+#ifndef USE_NON_POSIX_SIGNAL\n+\n+static void set_signal_handler(struct sigaction *psa)\n+{\n+\tsigemptyset(&psa->sa_mask);\n+\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n+\tpsa->sa_handler = child_handler;\n+\tsigaction(SIGCHLD, psa, NULL);\n+}\n+\n+#else\n+\n+static void set_signal_handler(struct sigaction *psa UNUSED)\n+{\n+\tsignal(SIGCHLD, child_handler);\n+}\n+\n static int service_loop(struct socketlist *socklist)\n {\n+\tstruct sigaction sa;\n \tstruct pollfd *pfd;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n@@ -1129,7 +1152,7 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tset_signal_handler(&sa);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n-- \ngitgitgadget\n\n"},{"id":"520675","messageId":"b737e0389dfc280994e118736bf4452ed80ebcd6.1750836928.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-25T07:35:28Z","receivedAt":"2025-06-25T07:35:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIf the setup for the SIGCHLD signal handler sets SA_RESTART, poll()\nmight not return with -1 and set errno to EINTR when a signal is\nreceived.\n\nSince the logic to reap zombie childs relies on those interruptions\nmake sure to explicitly disable SA_RESTART around this function.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/daemon.c b/daemon.c\nindex 8133bd902157..01337fcfedab 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1133,6 +1133,15 @@ static void set_signal_handler(struct sigaction *psa)\n \tsigaction(SIGCHLD, psa, NULL);\n }\n \n+static void set_sa_restart(struct sigaction *psa, int enable)\n+{\n+\tif (enable)\n+\t\tpsa->sa_flags |= SA_RESTART;\n+\telse\n+\t\tpsa->sa_flags &= ~SA_RESTART;\n+\tsigaction(SIGCHLD, psa, NULL);\n+}\n+\n #else\n \n static void set_signal_handler(struct sigaction *psa UNUSED)\n@@ -1140,6 +1149,12 @@ static void set_signal_handler(struct sigaction *psa UNUSED)\n \tsignal(SIGCHLD, child_handler);\n }\n \n+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n+{\n+}\n+\n+#endif\n+\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct sigaction sa;\n@@ -1157,6 +1172,7 @@ static int service_loop(struct socketlist *socklist)\n \tfor (;;) {\n \t\tcheck_dead_children();\n \n+\t\tset_sa_restart(&sa, 0);\n \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n@@ -1165,6 +1181,7 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\tset_sa_restart(&sa, 1);\n \n \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n-- \ngitgitgadget\n"},{"id":"520676","messageId":"907a79d1-da2e-4c8e-963f-05c6e313643f@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-25T08:39:25Z","receivedAt":"2025-06-25T08:39:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> This series addresses and ambiguity that is at least visible in OpenBSD,\n> where zombie proceses would only be cleared after a new connection is\n> received.\n\nThere is still a race where a child that exits after it has been checked \nin check_dead_children() but before we call poll() will not be collected \nuntil a new connection is received or a child exits while we're polling. \nIf we used the self-pipe trick described on the select(2) man page [1] \nwe would avoid that race and would not need to mess with SA_RESTART and \nso would not need to introduce USE_NON_POSIX_SIGNAL.\n\nBest Wishes\n\nPhillip\n\n[1] https://www.man7.org/linux/man-pages/man2/select.2.html\n> The underlying problem is that when this code was originally introduced,\n> SA_RESTART was not widely implemented, and the signal() call usually\n> implemented SysV like semantics, at least until it started being\n> reimplemented by calling sigaction() internally.\n> \n> Changes since v1\n> \n> * Almost all references to siginterrupt has been removed and a better named\n> variable used instead\n> * Changes had been anstracted to minimize ifdefs and their introduction\n> staged more naturally\n> \n> Carlo Marcelo Arenas Belón (3):\n> compat/posix.h: track SA_RESTART fallback\n> daemon: use sigaction() to install child_handler()\n> daemon: explicitly allow EINTR during poll()\n> \n> Makefile | 6 +++++\n> compat/mingw-posix.h | 1 -\n> compat/posix.h | 8 +++++++\n> config.mak.uname | 7 +++---\n> configure.ac | 17 +++++++++++++++\n> daemon.c | 52 +++++++++++++++++++++++++++++++++++++++-----\n> meson.build | 4 ++++\n> 7 files changed, 85 insertions(+), 10 deletions(-)\n> \n> \n> base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v2\n> Pull-Request: https://github.com/git/git/pull/2002\n> \n> Range-diff vs v1:\n> \n> 1: 2b5a58e53ac ! 1: e82b7425bbc compat/posix.h: track SA_RESTART fallback\n> @@ Metadata\n> ## Commit message ##\n> compat/posix.h: track SA_RESTART fallback\n> \n> - Systems without SA_RESTART where using custom CFLAGS instead of\n> - the standard header file.\n> + Systems without SA_RESTART are using custom CFLAGS or headers\n> + instead of the standard header file.\n> \n> - Consolidate that, so it will be easier to use in a future commit.\n> + Correct that, and invent a Makefile variable to track the\n> + exceptions which will become handy in the next commits.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> \n> + ## Makefile ##\n> +@@ Makefile: include shared.mak\n> + # when attempting to read from an fopen'ed directory (or even to fopen\n> + # it at all).\n> + #\n> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> ++# prefer to use ANSI C signal() over POSIX sigaction()\n> ++#\n> + # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n> + # when a signal is received (as opposed to restarting).\n> + #\n> +@@ Makefile: ifdef FREAD_READS_DIRECTORIES\n> + \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n> + \tCOMPAT_OBJS += compat/fopen.o\n> + endif\n> ++ifdef USE_NON_POSIX_SIGNAL\n> ++\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n> ++endif\n> + ifdef OPEN_RETURNS_EINTR\n> + \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n> + endif\n> +\n> + ## compat/mingw-posix.h ##\n> +@@ compat/mingw-posix.h: struct sigaction {\n> + \tsig_handler_t sa_handler;\n> + \tunsigned sa_flags;\n> + };\n> +-#define SA_RESTART 0\n> +\n> + struct itimerval {\n> + \tstruct timeval it_value, it_interval;\n> +\n> ## compat/posix.h ##\n> @@ compat/posix.h: char *gitdirname(char *);\n> #define NAME_MAX 255\n> #endif\n> \n> -+/* On most systems would have given us this, but\n> ++/*\n> ++ * On most systems would have given us this, but\n> + * not on some systems (e.g. NonStop, QNX).\n> + */\n> +#ifndef SA_RESTART\n> -+#define SA_RESTART 0\t/* disabled for sigaction() */\n> ++# define SA_RESTART 0\t/* disabled for sigaction() */\n> +#endif\n> +\n> typedef uintmax_t timestamp_t;\n> @@ compat/posix.h: char *gitdirname(char *);\n> #define parse_timestamp strtoumax\n> \n> ## config.mak.uname ##\n> +@@ config.mak.uname: ifeq ($(uname_S),Windows)\n> + \tNO_STRTOUMAX = YesPlease\n> + \tNO_MKDTEMP = YesPlease\n> + \tNO_INTTYPES_H = YesPlease\n> ++\tUSE_NON_POSIX_SIGNAL = YesPlease\n> + \tCSPRNG_METHOD = rtlgenrandom\n> + \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n> + \t# so we don't need this:\n> @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n> \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n> \n> @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n> \t# Apparently needed in compat/fnmatch/fnmatch.c.\n> \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n> \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n> +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n> + \tNO_MMAP = YesPlease\n> + \tNO_POLL = YesPlease\n> + \tNO_INTPTR_T = UnfortunatelyYes\n> ++\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n> + \tCSPRNG_METHOD = openssl\n> + \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n> + \tSHELL_PATH = /usr/coreutils/bin/bash\n> +@@ config.mak.uname: ifeq ($(uname_S),MINGW)\n> + \tNEEDS_LIBICONV = YesPlease\n> + \tNO_STRTOUMAX = YesPlease\n> + \tNO_MKDTEMP = YesPlease\n> ++\tUSE_NON_POSIX_SIGNAL = YesPlease\n> + \tNO_SVN_TESTS = YesPlease\n> +\n> + \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n> @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n> endif\n> endif\n> @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n> \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n> \tHAVE_STRINGS_H = YesPlease\n> \tNEEDS_SOCKET = YesPlease\n> +@@ config.mak.uname: ifeq ($(uname_S),QNX)\n> + \tNO_PTHREADS = YesPlease\n> + \tNO_STRCASESTR = YesPlease\n> + \tNO_STRLCPY = YesPlease\n> ++\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n> + endif\n> +\n> + ## configure.ac ##\n> +@@ configure.ac: fi\n> + GIT_CONF_SUBST([ICONV_OMITS_BOM])\n> + fi\n> +\n> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> ++# prefer using ANSI C signal() over POSIX sigaction()\n> ++\n> ++AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> ++\tAC_COMPILE_IFELSE(\n> ++\t\t[AC_LANG_PROGRAM([#include ], [[\n> ++\t\t#ifdef SA_RESTART\n> ++\t\t#endif\n> ++\t\tsiginterrupt(SIGCHLD, 1)\n> ++\t\t]])],[ac_cv_siginterrupt=yes],[\n> ++\t\t\tac_cv_siginterrupt=no\n> ++\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n> ++\t\t]\n> ++\t)\n> ++])\n> ++GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n> ++\n> + ## Checks for typedefs, structures, and compiler characteristics.\n> + AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n> + #\n> +\n> + ## meson.build ##\n> +@@ meson.build: else\n> + build_options_config.set('NO_EXPAT', '1')\n> + endif\n> +\n> ++if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n> ++ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'\n> ++endif\n> ++\n> + if not compiler.has_header('sys/select.h')\n> + libgit_c_args += '-DNO_SYS_SELECT_H'\n> + endif\n> 2: 2e8c4643a60 ! 2: 05d945aa1e5 daemon: use sigaction() to install child_handler()\n> @@ Commit message\n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n> \n> - Replace the call, which hs the added benefit of using BSD semantics\n> - reliably and therefore not needing the rearming call.\n> + Factor out the call to set the signal handler and use sigaction instead\n> + of signal for the systems that allow that, which has the added benefit\n> + of using BSD semantics reliably and therefore not needing the rearming\n> + call.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> \n> ## daemon.c ##\n> -@@ daemon.c: static void child_handler(int signo UNUSED)\n> +@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> + \t\tadd_child(&cld, addr, addrlen);\n> + }\n> +\n> +-static void child_handler(int signo UNUSED)\n> ++static void child_handler(int signo MAYBE_UNUSED)\n> + {\n> \t/*\n> - \t * Otherwise empty handler because systemcalls will get interrupted\n> - \t * upon signal receipt\n> +-\t * Otherwise empty handler because systemcalls will get interrupted\n> +-\t * upon signal receipt\n> -\t * SysV needs the handler to be rearmed\n> ++\t * Otherwise empty handler because systemcalls should get interrupted\n> ++\t * upon signal receipt.\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> ++#ifdef USE_NON_POSIX_SIGNAL\n> ++\t/*\n> ++\t * SysV needs the handler to be rearmed, but this is known\n> ++\t * to trigger infinite recursion crashes at least in AIX.\n> ++\t */\n> ++\tsignal(signo, child_handler);\n> ++#endif\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> + \t}\n> + }\n> +\n> ++#ifndef USE_NON_POSIX_SIGNAL\n> ++\n> ++static void set_signal_handler(struct sigaction *psa)\n> ++{\n> ++\tsigemptyset(&psa->sa_mask);\n> ++\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> ++\tpsa->sa_handler = child_handler;\n> ++\tsigaction(SIGCHLD, psa, NULL);\n> ++}\n> ++\n> ++#else\n> ++\n> ++static void set_signal_handler(struct sigaction *psa UNUSED)\n> ++{\n> ++\tsignal(SIGCHLD, child_handler);\n> ++}\n> ++\n> static int service_loop(struct socketlist *socklist)\n> {\n> - \tstruct pollfd *pfd;\n> +\tstruct sigaction sa;\n> + \tstruct pollfd *pfd;\n> \n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> -\n> @@ daemon.c: static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> -+\tsigemptyset(&sa.sa_mask);\n> -+\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> -+\tsa.sa_handler = child_handler;\n> -+\tsigaction(SIGCHLD, &sa, NULL);\n> ++\tset_signal_handler(&sa);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n> 3: a450bdb0066 ! 3: b737e0389df daemon: explicitly allow EINTR during poll()\n> @@ Commit message\n> might not return with -1 and set errno to EINTR when a signal is\n> received.\n> \n> - Since the logic to reap zombie childs relies om those interruptions\n> + Since the logic to reap zombie childs relies on those interruptions\n> make sure to explicitly disable SA_RESTART around this function.\n> \n> - Add a Makefile flag for portability to systems that don't have the\n> - functionality to change those flags or where it is not needed.\n> -\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> \n> - ## Makefile ##\n> -@@ Makefile: include shared.mak\n> - # Define NO_PREAD if you have a problem with pread() system call (e.g.\n> - # cygwin1.dll before v1.5.22).\n> - #\n> -+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART\n> -+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.\n> -+#\n> - # Define NO_SETITIMER if you don't have setitimer()\n> - #\n> - # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval\n> -@@ Makefile: ifdef NO_PREAD\n> - \tCOMPAT_CFLAGS += -DNO_PREAD\n> - \tCOMPAT_OBJS += compat/pread.o\n> - endif\n> -+ifdef NO_SIGINTERRUPT\n> -+\tCOMPAT_CFLAGS += -DNO_SIGINTERRUPT\n> -+endif\n> - ifdef NO_FAST_WORKING_DIRECTORY\n> - \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n> - endif\n> -\n> - ## config.mak.uname ##\n> -@@ config.mak.uname: ifeq ($(uname_S),Windows)\n> - \tNO_STRTOUMAX = YesPlease\n> - \tNO_MKDTEMP = YesPlease\n> - \tNO_INTTYPES_H = YesPlease\n> -+\tNO_SIGINTERRUPT = YesPlease\n> - \tCSPRNG_METHOD = rtlgenrandom\n> - \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n> - \t# so we don't need this:\n> -@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n> - \tNO_PREAD = YesPlease\n> - \tNO_MMAP = YesPlease\n> - \tNO_POLL = YesPlease\n> -+\tNO_SIGINTERRUPT = UnfortunatelyYes\n> - \tNO_INTPTR_T = UnfortunatelyYes\n> - \tCSPRNG_METHOD = openssl\n> - \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n> -@@ config.mak.uname: ifeq ($(uname_S),MINGW)\n> - \tNEEDS_LIBICONV = YesPlease\n> - \tNO_STRTOUMAX = YesPlease\n> - \tNO_MKDTEMP = YesPlease\n> -+\tNO_SIGINTERRUPT = YesPlease\n> - \tNO_SVN_TESTS = YesPlease\n> -\n> - \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n> -@@ config.mak.uname: ifeq ($(uname_S),QNX)\n> - \tNO_PTHREADS = YesPlease\n> - \tNO_STRCASESTR = YesPlease\n> - \tNO_STRLCPY = YesPlease\n> -+\tNO_SIGINTERRUPT = UnfortunatelyYes\n> - endif\n> -\n> - ## configure.ac ##\n> -@@ configure.ac: GIT_CHECK_FUNC(getdelim,\n> - [HAVE_GETDELIM=])\n> - GIT_CONF_SUBST([HAVE_GETDELIM])\n> - #\n> -+# Define NO_SIGINTERRUPT if you don't have siginterrupt.\n> -+GIT_CHECK_FUNC(siginterrupt,\n> -+[NO_SIGINTERRUPT=],\n> -+[NO_SIGINTERRUPT=YesPlease])\n> -+GIT_CONF_SUBST([NO_SIGINTERRUPT])\n> - #\n> - # Define NO_MMAP if you want to avoid mmap.\n> - #\n> -\n> ## daemon.c ##\n> -@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> - \t\tadd_child(&cld, addr, addrlen);\n> +@@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n> + \tsigaction(SIGCHLD, psa, NULL);\n> }\n> \n> --static void child_handler(int signo UNUSED)\n> -+static void child_handler(int signo)\n> - {\n> - \t/*\n> --\t * Otherwise empty handler because systemcalls will get interrupted\n> --\t * upon signal receipt\n> -+\t * Empty handler because systemcalls should get interrupted\n> -+\t * upon signal receipt.\n> - \t */\n> -+#ifdef NO_SIGINTERRUPT\n> -+\t/* SysV needs the handler to be rearmed */\n> -+\tsignal(signo, child_handler);\n> -+#endif\n> ++static void set_sa_restart(struct sigaction *psa, int enable)\n> ++{\n> ++\tif (enable)\n> ++\t\tpsa->sa_flags |= SA_RESTART;\n> ++\telse\n> ++\t\tpsa->sa_flags &= ~SA_RESTART;\n> ++\tsigaction(SIGCHLD, psa, NULL);\n> ++}\n> ++\n> + #else\n> +\n> + static void set_signal_handler(struct sigaction *psa UNUSED)\n> +@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)\n> + \tsignal(SIGCHLD, child_handler);\n> }\n> \n> - static int set_reuse_addr(int sockfd)\n> -@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> -\n> ++static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n> ++{\n> ++}\n> ++\n> ++#endif\n> ++\n> static int service_loop(struct socketlist *socklist)\n> {\n> --\tstruct pollfd *pfd;\n> -+#ifndef NO_SIGINTERRUPT\n> \tstruct sigaction sa;\n> -+#endif\n> -+\tstruct pollfd *pfd;\n> -\n> - \tCALLOC_ARRAY(pfd, socklist->nr);\n> -\n> @@ daemon.c: static int service_loop(struct socketlist *socklist)\n> - \t\tpfd[i].events = POLLIN;\n> - \t}\n> -\n> -+#ifdef NO_SIGINTERRUPT\n> -+\tsignal(SIGCHLD, child_handler);\n> -+#else\n> - \tsigemptyset(&sa.sa_mask);\n> - \tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> - \tsa.sa_handler = child_handler;\n> - \tsigaction(SIGCHLD, &sa, NULL);\n> -+#endif\n> -\n> \tfor (;;) {\n> \t\tcheck_dead_children();\n> \n> -+#ifndef NO_SIGINTERRUPT\n> -+\t\tsa.sa_flags &= ~SA_RESTART;\n> -+\t\tsigaction(SIGCHLD, &sa, NULL);\n> -+#endif\n> ++\t\tset_sa_restart(&sa, 0);\n> \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> \t\t\tif (errno != EINTR) {\n> \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n> @@ daemon.c: static int service_loop(struct socketlist *socklist)\n> \t\t\t}\n> \t\t\tcontinue;\n> \t\t}\n> -+#ifndef NO_SIGINTERRUPT\n> -+\t\tsa.sa_flags |= SA_RESTART;\n> -+\t\tsigaction(SIGCHLD, &sa, NULL);\n> -+#endif\n> ++\t\tset_sa_restart(&sa, 1);\n> \n> \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n> \t\t\tif (pfd[i].revents & POLLIN) {\n> -\n> - ## meson.build ##\n> -@@ meson.build: checkfuncs = {\n> - 'setenv' : ['setenv.c'],\n> - 'mkdtemp' : ['mkdtemp.c'],\n> - 'initgroups' : [],\n> -+ 'siginterrupt' : [],\n> - 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n> - 'pread' : ['pread.c'],\n> - }\n> \n\n"},{"id":"520707","messageId":"xmqqfrfnyff1.fsf@gitster.g","threadId":"63688","inReplyTo":"05d945aa1e546bcc028e215c8e4d174b5e0c32ad.1750836928.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:06:10Z","receivedAt":"2025-06-25T16:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>\n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n>\n> Factor out the call to set the signal handler and use sigaction instead\n> of signal for the systems that allow that, which has the added benefit\n> of using BSD semantics reliably and therefore not needing the rearming\n> call.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 35 +++++++++++++++++++++++++++++------\n> 1 file changed, 29 insertions(+), 6 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index d1be61fd5789..8133bd902157 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -912,14 +912,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> \t\tadd_child(&cld, addr, addrlen);\n> }\n> \n> -static void child_handler(int signo UNUSED)\n> +static void child_handler(int signo MAYBE_UNUSED)\n> {\n> \t/*\n> -\t * Otherwise empty handler because systemcalls will get interrupted\n> -\t * upon signal receipt\n> -\t * SysV needs the handler to be rearmed\n> +\t * Otherwise empty handler because systemcalls should get interrupted\n> +\t * upon signal receipt.\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> +#ifdef USE_NON_POSIX_SIGNAL\n> +\t/*\n> +\t * SysV needs the handler to be rearmed, but this is known\n> +\t * to trigger infinite recursion crashes at least in AIX.\n> +\t */\n> +\tsignal(signo, child_handler);\n> +#endif\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ -1118,8 +1123,26 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> \t}\n> }\n> \n> +#ifndef USE_NON_POSIX_SIGNAL\n\nDoes this #ifndef block with #else lack its #endif probably before\nthe service_loop() is defined ...\n\n> +static void set_signal_handler(struct sigaction *psa)\n> +{\n> +\tsigemptyset(&psa->sa_mask);\n> +\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> +\tpsa->sa_handler = child_handler;\n> +\tsigaction(SIGCHLD, psa, NULL);\n> +}\n> +\n> +#else\n> +\n> +static void set_signal_handler(struct sigaction *psa UNUSED)\n> +{\n> +\tsignal(SIGCHLD, child_handler);\n> +}\n\n... around here?\n\nI am wondering if it would make it even cleaner to also define\nrearm_signal_handler() in this \"if sigaction() can be used, do this,\notherwise do that\" block, and move the whole thing a bit earlier in\nthis file. That way, the primary code paths do not have to see much\nof the #ifdef conditionals. With IPV6 related #ifdef noise already\ncontaminating the file, it may not be a huge deal, but these crufts\ntend to build up unless we tightly control them with discipline.\n\n> static int service_loop(struct socketlist *socklist)\n> {\n> +\tstruct sigaction sa;\n> \tstruct pollfd *pfd;\n> \n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> @@ -1129,7 +1152,7 @@ static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> +\tset_signal_handler(&sa);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n"},{"id":"520708","messageId":"xmqqa55vyfdc.fsf@gitster.g","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:07:11Z","receivedAt":"2025-06-25T16:07:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> +@@ Makefile: include shared.mak\n> + # when attempting to read from an fopen'ed directory (or even to fopen\n> + # it at all).\n> + #\n> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> ++# prefer to use ANSI C signal() over POSIX sigaction()\n> ++#\n> ...\n> ++ifdef USE_NON_POSIX_SIGNAL\n> ++\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n> ++endif\n\nThe new symbol sounds like \"POSIX does not have signal(2) but on\nthis platform we have a usable signal(2), so we use it here\", but I\ndo not think that it is what we want to say (as POSIX inherits this\nfrom ANSI C anyway). More importantly, this \"USE_X\" sounds as if we\nallow builders to set it and magically we stop using sigaction(2),\nwhich is not what is going on. We have tons of calls to both\nsignal(2) and sigaction(2), and we turn calls to signal(2) we have\nin daemon.c to sigaction(2) but on some platforms their sigaction(2)\ncannot do what we ask it to do, so we are stuck with signal(2) on\nthese platforms only for these calls in daemon.c. It may be obvious\nto those who develop and review this series, but not for anybody else.\n\nIsn't the situation more like:\n\n We use sigaction(2) everywhere and have been happy with it in\n our code, but this topic discovered that on some platforms,\n their sigaction(2) does not do XYZ that everybody else's\n sigaction(2) does, so on them we need to fall back on the plain\n old signal(2) on selected code paths that we need XYZ out of the\n signal handling interface.\n\nWhat is this XYZ that describes the characteristics of\nsignal/sigaction implementation on these platforms? A name\nconstructed more like SIGACTION_LACKS_XYZ (hence we have to resort\nto signal), possibly with a more appropriate verb than \"lack\", would\nbe less confusing.\n\nI think the topic is moving in the right direction with cleaner code\nthan the previous round. Thanks for investing time in it.\n"},{"id":"520709","messageId":"xmqq4iw3yfd8.fsf@gitster.g","threadId":"63688","inReplyTo":"e82b7425bbc2540fa5ef3fd4584e6f902485d064.1750836928.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:07:15Z","receivedAt":"2025-06-25T16:07:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> +# prefer using ANSI C signal() over POSIX sigaction()\n> +\n> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> +\tAC_COMPILE_IFELSE(\n> +\t\t[AC_LANG_PROGRAM([#include ], [[\n> +\t\t#ifdef SA_RESTART\n> +\t\t#endif\n> +\t\tsiginterrupt(SIGCHLD, 1)\n\nThis is curious. What is this #ifdef/#endif doing that does not\nhave anything in it?\n\n> +\t\t]])],[ac_cv_siginterrupt=yes],[\n> +\t\t\tac_cv_siginterrupt=no\n> +\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n> +\t\t]\n> +\t)\n> +])\n> +GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n"},{"id":"520710","messageId":"xmqqzfdvx0lb.fsf@gitster.g","threadId":"63688","inReplyTo":"b737e0389dfc280994e118736bf4452ed80ebcd6.1750836928.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:11:44Z","receivedAt":"2025-06-25T16:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>\n> If the setup for the SIGCHLD signal handler sets SA_RESTART, poll()\n> might not return with -1 and set errno to EINTR when a signal is\n> received.\n>\n> Since the logic to reap zombie childs relies on those interruptions\n> make sure to explicitly disable SA_RESTART around this function.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 8133bd902157..01337fcfedab 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1133,6 +1133,15 @@ static void set_signal_handler(struct sigaction *psa)\n> \tsigaction(SIGCHLD, psa, NULL);\n> }\n> \n> +static void set_sa_restart(struct sigaction *psa, int enable)\n> +{\n> +\tif (enable)\n> +\t\tpsa->sa_flags |= SA_RESTART;\n> +\telse\n> +\t\tpsa->sa_flags &= ~SA_RESTART;\n> +\tsigaction(SIGCHLD, psa, NULL);\n> +}\n> +\n> #else\n> \n> static void set_signal_handler(struct sigaction *psa UNUSED)\n> @@ -1140,6 +1149,12 @@ static void set_signal_handler(struct sigaction *psa UNUSED)\n> \tsignal(SIGCHLD, child_handler);\n> }\n> \n> +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n> +{\n> +}\n> +\n> +#endif\n> +\n> static int service_loop(struct socketlist *socklist)\n\nOK, this is the answer to the question I was puzzled with while\nreviewing the [2/3].\n"},{"id":"520711","messageId":"xmqqsejnx02l.fsf@gitster.g","threadId":"63688","inReplyTo":"xmqqfrfnyff1.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:22:58Z","receivedAt":"2025-06-25T16:23:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Does this #ifndef block with #else lack its #endif probably before\n> the service_loop() is defined ...\n> ...\n> ... around here?\n>\n> I am wondering if it would make it even cleaner to also define\n> rearm_signal_handler() in this \"if sigaction() can be used, do this,\n> otherwise do that\" block, and move the whole thing a bit earlier in\n> this file. That way, the primary code paths do not have to see much\n> of the #ifdef conditionals. With IPV6 related #ifdef noise already\n> contaminating the file, it may not be a huge deal, but these crufts\n> tend to build up unless we tightly control them with discipline.\n\nApplying this on top of your series would illustrate what I meant.\n\nI didn't spend enough braincycles on the naming issues for the\nconditional compilation so I left it as USE_NON_POSIX_SIGNAL even\nthough I know it has to be fixed before we move forward.\n\nThanks.\n\n daemon.c | 81 +++++++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 42 insertions(+), 39 deletions(-)\n\ndiff --git c/daemon.c w/daemon.c\nindex 01337fcfed..8a371518b8 100644\n--- c/daemon.c\n+++ w/daemon.c\n@@ -912,19 +912,54 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tadd_child(&cld, addr, addrlen);\n }\n \n-static void child_handler(int signo MAYBE_UNUSED)\n+#ifndef USE_NON_POSIX_SIGNAL\n+\n+static void set_signal_handler(struct sigaction *psa, void (*child_handler)(int))\n+{\n+\tsigemptyset(&psa->sa_mask);\n+\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n+\tpsa->sa_handler = child_handler;\n+\tsigaction(SIGCHLD, psa, NULL);\n+}\n+\n+static void rearm_signal_handler(int signo UNUSED, void (*child_handler)(int) UNUSED)\n+{\n+}\n+\n+static void set_sa_restart(struct sigaction *psa, int enable)\n+{\n+\tif (enable)\n+\t\tpsa->sa_flags |= SA_RESTART;\n+\telse\n+\t\tpsa->sa_flags &= ~SA_RESTART;\n+\tsigaction(SIGCHLD, psa, NULL);\n+}\n+\n+#else\n+\n+static void set_signal_handler(struct sigaction *psa UNUSED, void (*child_handler)(int))\n+{\n+\tsignal(SIGCHLD, child_handler);\n+}\n+\n+static void rearm_signal_handler(int signo, void (*child_handler)(int))\n {\n-\t/*\n-\t * Otherwise empty handler because systemcalls should get interrupted\n-\t * upon signal receipt.\n-\t */\n-#ifdef USE_NON_POSIX_SIGNAL\n \t/*\n \t * SysV needs the handler to be rearmed, but this is known\n \t * to trigger infinite recursion crashes at least in AIX.\n \t */\n \tsignal(signo, child_handler);\n+}\n+\n+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n+{\n+}\n+\n #endif\n+\n+static void child_handler(int signo)\n+{\n+\trearm_signal_handler(signo, child_handler);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1123,38 +1158,6 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \t}\n }\n \n-#ifndef USE_NON_POSIX_SIGNAL\n-\n-static void set_signal_handler(struct sigaction *psa)\n-{\n-\tsigemptyset(&psa->sa_mask);\n-\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n-\tpsa->sa_handler = child_handler;\n-\tsigaction(SIGCHLD, psa, NULL);\n-}\n-\n-static void set_sa_restart(struct sigaction *psa, int enable)\n-{\n-\tif (enable)\n-\t\tpsa->sa_flags |= SA_RESTART;\n-\telse\n-\t\tpsa->sa_flags &= ~SA_RESTART;\n-\tsigaction(SIGCHLD, psa, NULL);\n-}\n-\n-#else\n-\n-static void set_signal_handler(struct sigaction *psa UNUSED)\n-{\n-\tsignal(SIGCHLD, child_handler);\n-}\n-\n-static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n-{\n-}\n-\n-#endif\n-\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct sigaction sa;\n@@ -1167,7 +1170,7 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tset_signal_handler(&sa);\n+\tset_signal_handler(&sa, child_handler);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n"},{"id":"520712","messageId":"xmqqo6ubx00s.fsf@gitster.g","threadId":"63688","inReplyTo":"907a79d1-da2e-4c8e-963f-05c6e313643f@gmail.com","subject":"Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-25T16:24:03Z","receivedAt":"2025-06-25T16:24:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n>> This series addresses and ambiguity that is at least visible in OpenBSD,\n>> where zombie proceses would only be cleared after a new connection is\n>> received.\n>\n> There is still a race where a child that exits after it has been\n> checked in check_dead_children() but before we call poll() will not be\n> collected until a new connection is received or a child exits while\n> we're polling. If we used the self-pipe trick described on the\n> select(2) man page [1] we would avoid that race and would not need to\n> mess with SA_RESTART and so would not need to introduce\n> USE_NON_POSIX_SIGNAL.\n>\n> Best Wishes\n>\n> Phillip\n>\n> [1] https://www.man7.org/linux/man-pages/man2/select.2.html\n\nThe principle should apply equally to poll-based service loop, I\npresume.\n\nThanks.\n"},{"id":"520728","messageId":"c314cd2d-8fdd-4386-bda0-881ff87d9204@gmail.com","threadId":"63688","inReplyTo":"xmqqo6ubx00s.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-25T19:35:27Z","receivedAt":"2025-06-25T19:35:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/06/2025 17:24, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n>>> This series addresses and ambiguity that is at least visible in OpenBSD,\n>>> where zombie proceses would only be cleared after a new connection is\n>>> received.\n>>\n>> There is still a race where a child that exits after it has been\n>> checked in check_dead_children() but before we call poll() will not be\n>> collected until a new connection is received or a child exits while\n>> we're polling. If we used the self-pipe trick described on the\n>> select(2) man page [1] we would avoid that race and would not need to\n>> mess with SA_RESTART and so would not need to introduce\n>> USE_NON_POSIX_SIGNAL.\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>> [1] https://www.man7.org/linux/man-pages/man2/select.2.html\n> \n> The principle should apply equally to poll-based service loop, I\n> presume.\n\nYes, you create a pipe, add the read end to the set of file descriptors \nmonitored by poll() and write to the other end of the pipe when a signal \nis received.\n\nThanks\n\nPhillip\n> Thanks.\n\n"},{"id":"520734","messageId":"4oh4eatsp4wo4ur6rluy6ickfy5jfpuarg435vplrqzvk3eaiz@jbtnnwqnz2yi","threadId":"63688","inReplyTo":"xmqq4iw3yfd8.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-25T22:24:55Z","receivedAt":"2025-06-25T22:24:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Jun 25, 2025 at 09:07:15AM -0800, Junio C Hamano wrote:\n> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> writes:\n> \n> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> > +# prefer using ANSI C signal() over POSIX sigaction()\n> > +\n> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> > +\tAC_COMPILE_IFELSE(\n> > +\t\t[AC_LANG_PROGRAM([#include ], [[\n> > +\t\t#ifdef SA_RESTART\n> > +\t\t#endif\n> > +\t\tsiginterrupt(SIGCHLD, 1)\n> \n> This is curious. What is this #ifdef/#endif doing that does not\n> have anything in it?\n\nIt checks that `SA_RESTART` is defined in `signal.h`, which should\nfail at least in QNX, NonStop and Windows.\n\nCarlo\n"},{"id":"520736","messageId":"xmqqzfdvqr3a.fsf@gitster.g","threadId":"63688","inReplyTo":"4oh4eatsp4wo4ur6rluy6ickfy5jfpuarg435vplrqzvk3eaiz@jbtnnwqnz2yi","subject":"Re: [PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-26T00:33:29Z","receivedAt":"2025-06-26T00:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> On Wed, Jun 25, 2025 at 09:07:15AM -0800, Junio C Hamano wrote:\n>> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n>> writes:\n>> \n>> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n>> > +# prefer using ANSI C signal() over POSIX sigaction()\n>> > +\n>> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n>> > +\tAC_COMPILE_IFELSE(\n>> > +\t\t[AC_LANG_PROGRAM([#include ], [[\n>> > +\t\t#ifdef SA_RESTART\n>> > +\t\t#endif\n>> > +\t\tsiginterrupt(SIGCHLD, 1)\n>> \n>> This is curious. What is this #ifdef/#endif doing that does not\n>> have anything in it?\n>\n> It checks that `SA_RESTART` is defined in `signal.h`, which should\n> fail at least in QNX, NonStop and Windows.\n\nThe above roughly expands to\n\n #include \n int main(void)\n {\n #ifdef SA_RESTART\n #endif\n siginterrupt(SIGCHLD, 1);\n return 0;\n }\n\nAre you saying that a preprocessor macro SA_RESTART, which may or\nmay not be defined, when asked by \"#ifdef\", causes what is left in\nthe preprocessed source change in any meaningful way to cause the\ncompilation to fail?\n\nI understand that these platforms may fail to compile the above due\nto siginterrupt() not declared in , but I cannot quite see\nhow the empty #ifdef NO_SUCH_SYMBOL/#endif block that comes before\nit changes anything.\n\nOn a debian-derived host I happen to have access to, compiling the\nabove fails, with or without \"s/SA_RESTART/NO_SUCH_SYMBOL/\", not\nbecause of the empty #ifdef/#endif block, but because use of\nsiginterrupt() gets deprecation warning.\n"},{"id":"520737","messageId":"xmqqplerqqjk.fsf@gitster.g","threadId":"63688","inReplyTo":"e82b7425bbc2540fa5ef3fd4584e6f902485d064.1750836928.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-26T00:45:19Z","receivedAt":"2025-06-26T00:45:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> +# prefer to use ANSI C signal() over POSIX sigaction()\n> +#\n\nI may or may not have mentioned this, but this is not helpful enough\nfor folks, as there is no clue for users to decide if they \"prefer\".\n\nWe need to state how they need to decide between the use of\n\"signal()\" and \"sigaction()\" in the affected codepath, especially\nwhen they have both.\n\nIf their system lacks sigaction() at all, the decision may be\ntrivial, but the conditional compilation you are trying to achieve\nin daemon.c is _not_ like \"my system has both, so I can use either\none and the resulting binary works just fine\". Rather, \"Even though\nmy platform has both, the sigaction() my system has is not quite\nright in such and such way and I have to use signal(), unlike\neverybody else, unfortunately\".\n\nIt may cause us to rethink the name USE_NON_POSIX_SIGNAL; I though\nI've already discussed this point in my response to the cover\nletter.\n\nThanks.\n"},{"id":"520740","messageId":"ilvhfzdfqcwkjt5h336ldrimnzpz7kwkkdst6egyohwxppo2gn@fpkulngdty23","threadId":"63688","inReplyTo":"xmqqzfdvqr3a.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] compat/posix.h: track SA_RESTART fallback","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T01:35:00Z","receivedAt":"2025-06-26T01:35:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Jun 25, 2025 at 05:33:29PM -0800, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> \n> > On Wed, Jun 25, 2025 at 09:07:15AM -0800, Junio C Hamano wrote:\n> >> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> >> writes:\n> >> \n> >> > +# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> >> > +# prefer using ANSI C signal() over POSIX sigaction()\n> >> > +\n> >> > +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> >> > +\tAC_COMPILE_IFELSE(\n> >> > +\t\t[AC_LANG_PROGRAM([#include ], [[\n> >> > +\t\t#ifdef SA_RESTART\n> >> > +\t\t#endif\n> >> > +\t\tsiginterrupt(SIGCHLD, 1)\n> >> \n> >> This is curious. What is this #ifdef/#endif doing that does not\n> >> have anything in it?\n> >\n> > It checks that `SA_RESTART` is defined in `signal.h`, which should\n> > fail at least in QNX, NonStop and Windows.\n> \n> The above roughly expands to\n> \n> #include \n> int main(void)\n> {\n> #ifdef SA_RESTART\n> #endif\n> siginterrupt(SIGCHLD, 1);\n> return 0;\n> }\n> \n> Are you saying that a preprocessor macro SA_RESTART, which may or\n> may not be defined, when asked by \"#ifdef\", causes what is left in\n> the preprocessed source change in any meaningful way to cause the\n> compilation to fail?\n\nLack of judgement on my part; I apologize and will correct it.\n\nCarlo\n"},{"id":"520745","messageId":"ypopyf663bxcj3lakoa3vmxinyj7ipcjtuwrbu3i4uhga7ono3@ubxgvqpntk7u","threadId":"63688","inReplyTo":"xmqqa55vyfdc.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T08:50:48Z","receivedAt":"2025-06-26T08:50:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Jun 25, 2025 at 09:07:11AM -0800, Junio C Hamano wrote:\n> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> writes:\n> \n> > +@@ Makefile: include shared.mak\n> > + # when attempting to read from an fopen'ed directory (or even to fopen\n> > + # it at all).\n> > + #\n> > ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> > ++# prefer to use ANSI C signal() over POSIX sigaction()\n> > ++#\n> > ...\n> > ++ifdef USE_NON_POSIX_SIGNAL\n> > ++\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n> > ++endif\n> \n> The new symbol sounds like \"POSIX does not have signal(2) but on\n> this platform we have a usable signal(2), so we use it here\", but I\n> do not think that it is what we want to say (as POSIX inherits this\n> from ANSI C anyway). More importantly, this \"USE_X\" sounds as if we\n> allow builders to set it and magically we stop using sigaction(2),\n> which is not what is going on. We have tons of calls to both\n> signal(2) and sigaction(2), and we turn calls to signal(2) we have\n> in daemon.c to sigaction(2) but on some platforms their sigaction(2)\n> cannot do what we ask it to do, so we are stuck with signal(2) on\n> these platforms only for these calls in daemon.c. It may be obvious\n> to those who develop and review this series, but not for anybody else.\n> \n> Isn't the situation more like:\n> \n> We use sigaction(2) everywhere and have been happy with it in\n> our code, but this topic discovered that on some platforms,\n> their sigaction(2) does not do XYZ that everybody else's\n> sigaction(2) does, so on them we need to fall back on the plain\n> old signal(2) on selected code paths that we need XYZ out of the\n> signal handling interface.\n> \n> What is this XYZ that describes the characteristics of\n> signal/sigaction implementation on these platforms? A name\n> constructed more like SIGACTION_LACKS_XYZ (hence we have to resort\n> to signal), possibly with a more appropriate verb than \"lack\", would\n> be less confusing.\n\nsigaction(2) doesn't have any issues and it is indeed a better option\nevery time as it behaves the same in all platforms that have it.\n\nthe problems we have come from our codebase:\n\n1) we have a hack to workaround the lack of support for SA_RESTART in\n some platforms, which sets it to 0 and allow us to compile as if it\n works.\n2) Windows doesn't have sigaction() and their compability version needs\n updating if we would use it for this new code.\n\nKeeping the fallback to use signal() isn't really needed, and is indeed\nproblematic, as it crashes in AIX and probably other SysIII derived\nsystems because of the hack Chris described for non BSD signal().\n\nCarlo\n"},{"id":"520746","messageId":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-26T08:53:04Z","receivedAt":"2025-06-26T08:53:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series addresses and ambiguity that is at least visible in OpenBSD,\nwhere zombie proceses would only be cleared after a new connection is\nreceived.\n\nThe underlying problem is that when this code was originally introduced,\nSA_RESTART was not widely implemented, and the signal() call usually\nimplemented SysV like semantics, at least until it started being\nreimplemented by calling sigaction() internally.\n\nChanges since v2\n\n * Add a new patch 2 that modifies windows' sigaction so there is no more\n need for a fallback\n * Hopefully no more silly mistakes and a variable that finally makes sense\n\nChanges since v1\n\n * Almost all references to siginterrupt has been removed and a better named\n variable used instead\n * Changes had been abstracted to minimize ifdefs and their introduction\n staged more naturally\n\nCarlo Marcelo Arenas Belón (4):\n compat/posix.h: track SA_RESTART fallback\n compat/mingw: allow sigaction(SIGCHLD)\n daemon: use sigaction() to install child_handler()\n daemon: explicitly allow EINTR during poll()\n\n Makefile | 5 +++++\n compat/mingw-posix.h | 2 +-\n compat/mingw.c | 4 +++-\n compat/posix.h | 8 ++++++++\n config.mak.uname | 7 ++++---\n configure.ac | 16 ++++++++++++++++\n daemon.c | 33 ++++++++++++++++++++++++++++-----\n meson.build | 4 ++++\n 8 files changed, 69 insertions(+), 10 deletions(-)\n\n\nbase-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v3\nPull-Request: https://github.com/git/git/pull/2002\n\nRange-diff vs v2:\n\n 1: e82b7425bbc ! 1: ae1ca6bb2b2 compat/posix.h: track SA_RESTART fallback\n @@ Makefile: include shared.mak\n # when attempting to read from an fopen'ed directory (or even to fopen\n # it at all).\n #\n -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n -+# prefer to use ANSI C signal() over POSIX sigaction()\n ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n +#\n # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n # when a signal is received (as opposed to restarting).\n @@ Makefile: ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n -+ifdef USE_NON_POSIX_SIGNAL\n -+\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n ++ifdef NO_RESTARTABLE_SIGNALS\n ++\tCOMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS\n +endif\n ifdef OPEN_RETURNS_EINTR\n \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n @@ compat/posix.h: char *gitdirname(char *);\n + * not on some systems (e.g. NonStop, QNX).\n + */\n +#ifndef SA_RESTART\n -+# define SA_RESTART 0\t/* disabled for sigaction() */\n ++# define SA_RESTART 0 /* disabled for sigaction() */\n +#endif\n +\n typedef uintmax_t timestamp_t;\n @@ config.mak.uname: ifeq ($(uname_S),Windows)\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_INTTYPES_H = YesPlease\n -+\tUSE_NON_POSIX_SIGNAL = YesPlease\n ++\tNO_RESTARTABLE_SIGNALS = YesPlease\n \tCSPRNG_METHOD = rtlgenrandom\n \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n \t# so we don't need this:\n @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n \tNO_MMAP = YesPlease\n \tNO_POLL = YesPlease\n \tNO_INTPTR_T = UnfortunatelyYes\n -+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n ++\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n \tCSPRNG_METHOD = openssl\n \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n \tSHELL_PATH = /usr/coreutils/bin/bash\n @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n \tNEEDS_LIBICONV = YesPlease\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n -+\tUSE_NON_POSIX_SIGNAL = YesPlease\n ++\tNO_RESTARTABLE_SIGNALS = YesPlease\n \tNO_SVN_TESTS = YesPlease\n \n \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n @@ config.mak.uname: ifeq ($(uname_S),QNX)\n \tNO_PTHREADS = YesPlease\n \tNO_STRCASESTR = YesPlease\n \tNO_STRLCPY = YesPlease\n -+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n ++\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n endif\n \n ## configure.ac ##\n @@ configure.ac: fi\n GIT_CONF_SUBST([ICONV_OMITS_BOM])\n fi\n \n -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n -+# prefer using ANSI C signal() over POSIX sigaction()\n ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n +\n +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n +\tAC_COMPILE_IFELSE(\n +\t\t[AC_LANG_PROGRAM([#include ], [[\n -+\t\t#ifdef SA_RESTART\n -+\t\t#endif\n -+\t\tsiginterrupt(SIGCHLD, 1)\n -+\t\t]])],[ac_cv_siginterrupt=yes],[\n ++\t\t\t#ifdef SA_RESTART\n ++\t\t\trestartable signals supported\n ++\t\t\t#endif\n ++\t\t]])],[\n +\t\t\tac_cv_siginterrupt=no\n -+\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n -+\t\t]\n ++\t\t\tNO_RESTARTABLE_SIGNALS=UnfortunatelyYes\n ++\t\t], [ac_cv_siginterrupt=yes]\n +\t)\n +])\n -+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n ++GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])\n +\n ## Checks for typedefs, structures, and compiler characteristics.\n AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n @@ meson.build: else\n endif\n \n +if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n -+ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'\n ++ libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'\n +endif\n +\n if not compiler.has_header('sys/select.h')\n -: ----------- > 2: 3f63479119f compat/mingw: allow sigaction(SIGCHLD)\n 2: 05d945aa1e5 ! 3: c66bda461f4 daemon: use sigaction() to install child_handler()\n @@ Commit message\n In a future change, the flags used for processing SIGCHLD will need to\n be updated, which is only possible by using sigaction().\n \n - Factor out the call to set the signal handler and use sigaction instead\n - of signal for the systems that allow that, which has the added benefit\n - of using BSD semantics reliably and therefore not needing the rearming\n - call.\n + Replace signal() with an equivalent invocation of sigaction(), which\n + has the added benefit of using BSD semantics reliably and therefore\n + not needing the rearming call in the signal handler.\n \n Signed-off-by: Carlo Marcelo Arenas Belón \n \n ## daemon.c ##\n @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n - \t\tadd_child(&cld, addr, addrlen);\n - }\n - \n --static void child_handler(int signo UNUSED)\n -+static void child_handler(int signo MAYBE_UNUSED)\n + static void child_handler(int signo UNUSED)\n {\n \t/*\n -\t * Otherwise empty handler because systemcalls will get interrupted\n @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr\n +\t * upon signal receipt.\n \t */\n -\tsignal(SIGCHLD, child_handler);\n -+#ifdef USE_NON_POSIX_SIGNAL\n -+\t/*\n -+\t * SysV needs the handler to be rearmed, but this is known\n -+\t * to trigger infinite recursion crashes at least in AIX.\n -+\t */\n -+\tsignal(signo, child_handler);\n -+#endif\n }\n \n static int set_reuse_addr(int sockfd)\n @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n - \t}\n - }\n \n -+#ifndef USE_NON_POSIX_SIGNAL\n -+\n -+static void set_signal_handler(struct sigaction *psa)\n -+{\n -+\tsigemptyset(&psa->sa_mask);\n -+\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n -+\tpsa->sa_handler = child_handler;\n -+\tsigaction(SIGCHLD, psa, NULL);\n -+}\n -+\n -+#else\n -+\n -+static void set_signal_handler(struct sigaction *psa UNUSED)\n -+{\n -+\tsignal(SIGCHLD, child_handler);\n -+}\n -+\n static int service_loop(struct socketlist *socklist)\n {\n +\tstruct sigaction sa;\n @@ daemon.c: static int service_loop(struct socketlist *socklist)\n \t}\n \n -\tsignal(SIGCHLD, child_handler);\n -+\tset_signal_handler(&sa);\n ++\tsigemptyset(&sa.sa_mask);\n ++\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n ++\tsa.sa_handler = child_handler;\n ++\tsigaction(SIGCHLD, &sa, NULL);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n 3: b737e0389df ! 4: 851d663be0b daemon: explicitly allow EINTR during poll()\n @@ Commit message\n Signed-off-by: Carlo Marcelo Arenas Belón \n \n ## daemon.c ##\n -@@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n - \tsigaction(SIGCHLD, psa, NULL);\n +@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n + \t}\n }\n \n ++#ifndef NO_RESTARTABLE_SIGNALS\n ++\n +static void set_sa_restart(struct sigaction *psa, int enable)\n +{\n +\tif (enable)\n @@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n +\tsigaction(SIGCHLD, psa, NULL);\n +}\n +\n - #else\n - \n - static void set_signal_handler(struct sigaction *psa UNUSED)\n -@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)\n - \tsignal(SIGCHLD, child_handler);\n - }\n - \n ++#else\n ++\n +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n +{\n +}\n\n-- \ngitgitgadget\n"},{"id":"520747","messageId":"ae1ca6bb2b258fc3c18c627aed2159dbb8f8c268.1750927989.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] compat/posix.h: track SA_RESTART fallback","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-26T08:53:05Z","receivedAt":"2025-06-26T08:53:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nSystems without SA_RESTART are using custom CFLAGS or headers\ninstead of the standard header file.\n\nCorrect that, and invent a Makefile variable to track the\nexceptions which will become handy in the next commits.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 5 +++++\n compat/mingw-posix.h | 1 -\n compat/posix.h | 8 ++++++++\n config.mak.uname | 7 ++++---\n configure.ac | 16 ++++++++++++++++\n meson.build | 4 ++++\n 6 files changed, 37 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 70d1543b6b86..f0839356199c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -37,6 +37,8 @@ include shared.mak\n # when attempting to read from an fopen'ed directory (or even to fopen\n # it at all).\n #\n+# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n+#\n # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n # when a signal is received (as opposed to restarting).\n #\n@@ -1811,6 +1813,9 @@ ifdef FREAD_READS_DIRECTORIES\n \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n \tCOMPAT_OBJS += compat/fopen.o\n endif\n+ifdef NO_RESTARTABLE_SIGNALS\n+\tCOMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS\n+endif\n ifdef OPEN_RETURNS_EINTR\n \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n endif\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex 88e0cf92924b..a0dca756d104 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -95,7 +95,6 @@ struct sigaction {\n \tsig_handler_t sa_handler;\n \tunsigned sa_flags;\n };\n-#define SA_RESTART 0\n \n struct itimerval {\n \tstruct timeval it_value, it_interval;\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 067a00f33b83..b514e67902d2 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -250,6 +250,14 @@ char *gitdirname(char *);\n #define NAME_MAX 255\n #endif\n \n+/*\n+ * On most systems would have given us this, but\n+ * not on some systems (e.g. NonStop, QNX).\n+ */\n+#ifndef SA_RESTART\n+# define SA_RESTART 0 /* disabled for sigaction() */\n+#endif\n+\n typedef uintmax_t timestamp_t;\n #define PRItime PRIuMAX\n #define parse_timestamp strtoumax\ndiff --git a/config.mak.uname b/config.mak.uname\nindex b1c5c4d5e8ed..435078dc620b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -486,6 +486,7 @@ ifeq ($(uname_S),Windows)\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_INTTYPES_H = YesPlease\n+\tNO_RESTARTABLE_SIGNALS = YesPlease\n \tCSPRNG_METHOD = rtlgenrandom\n \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n \t# so we don't need this:\n@@ -654,8 +655,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \n \t# Not detected (nor checked for) by './configure'.\n-\t# We don't have SA_RESTART on NonStop, unfortunalety.\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \t# Apparently needed in compat/fnmatch/fnmatch.c.\n \tCOMPAT_CFLAGS += -DHAVE_STRING_H=1\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n@@ -664,6 +663,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tNO_MMAP = YesPlease\n \tNO_POLL = YesPlease\n \tNO_INTPTR_T = UnfortunatelyYes\n+\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n \tCSPRNG_METHOD = openssl\n \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n \tSHELL_PATH = /usr/coreutils/bin/bash\n@@ -699,6 +699,7 @@ ifeq ($(uname_S),MINGW)\n \tNEEDS_LIBICONV = YesPlease\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n+\tNO_RESTARTABLE_SIGNALS = YesPlease\n \tNO_SVN_TESTS = YesPlease\n \n \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n@@ -782,7 +783,6 @@ ifeq ($(uname_S),MINGW)\n endif\n endif\n ifeq ($(uname_S),QNX)\n-\tCOMPAT_CFLAGS += -DSA_RESTART=0\n \tEXPAT_NEEDS_XMLPARSE_H = YesPlease\n \tHAVE_STRINGS_H = YesPlease\n \tNEEDS_SOCKET = YesPlease\n@@ -794,4 +794,5 @@ ifeq ($(uname_S),QNX)\n \tNO_PTHREADS = YesPlease\n \tNO_STRCASESTR = YesPlease\n \tNO_STRLCPY = YesPlease\n+\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n endif\ndiff --git a/configure.ac b/configure.ac\nindex f6caab919a3e..631ca67dd1aa 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -862,6 +862,22 @@ fi\n GIT_CONF_SUBST([ICONV_OMITS_BOM])\n fi\n \n+# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n+\n+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n+\tAC_COMPILE_IFELSE(\n+\t\t[AC_LANG_PROGRAM([#include ], [[\n+\t\t\t#ifdef SA_RESTART\n+\t\t\trestartable signals supported\n+\t\t\t#endif\n+\t\t]])],[\n+\t\t\tac_cv_siginterrupt=no\n+\t\t\tNO_RESTARTABLE_SIGNALS=UnfortunatelyYes\n+\t\t], [ac_cv_siginterrupt=yes]\n+\t)\n+])\n+GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])\n+\n ## Checks for typedefs, structures, and compiler characteristics.\n AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n #\ndiff --git a/meson.build b/meson.build\nindex 7fea4a34d684..b6711393a5fa 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1094,6 +1094,10 @@ else\n build_options_config.set('NO_EXPAT', '1')\n endif\n \n+if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n+ libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'\n+endif\n+\n if not compiler.has_header('sys/select.h')\n libgit_c_args += '-DNO_SYS_SELECT_H'\n endif\n-- \ngitgitgadget\n\n"},{"id":"520748","messageId":"3f63479119ffe6fdcf694dac3cb47cd7838564b7.1750927989.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-26T08:53:06Z","receivedAt":"2025-06-26T08:53:14Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nA future change will start using sigaction to setup a SIGCHLD signal\nhandler.\n\nThe current code uses signal() which returns SIG_ERR (but doesn't\nseem to set errno) so instruct sigaction() to do the same.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n compat/mingw-posix.h | 1 +\n compat/mingw.c | 4 +++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex a0dca756d104..847d558c9b2d 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -95,6 +95,7 @@ struct sigaction {\n \tsig_handler_t sa_handler;\n \tunsigned sa_flags;\n };\n+#define SA_NOCLDSTOP 1\n \n struct itimerval {\n \tstruct timeval it_value, it_interval;\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 8a9972a1ca19..5d69ae32f4b9 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n \n int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n {\n-\tif (sig != SIGALRM)\n+\tif (sig == SIGCHLD)\n+\t\treturn -1;\n+\telse if (sig != SIGALRM)\n \t\treturn errno = EINVAL,\n \t\t\terror(\"sigaction only implemented for SIGALRM\");\n \tif (out)\n-- \ngitgitgadget\n\n"},{"id":"520749","messageId":"c66bda461f45791d278779fb0021f1e0369fe889.1750927989.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-26T08:53:07Z","receivedAt":"2025-06-26T08:53:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIn a future change, the flags used for processing SIGCHLD will need to\nbe updated, which is only possible by using sigaction().\n\nReplace signal() with an equivalent invocation of sigaction(), which\nhas the added benefit of using BSD semantics reliably and therefore\nnot needing the rearming call in the signal handler.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd5789..155b2e180167 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n static void child_handler(int signo UNUSED)\n {\n \t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n-\t * SysV needs the handler to be rearmed\n+\t * Otherwise empty handler because systemcalls should get interrupted\n+\t * upon signal receipt.\n \t */\n-\tsignal(SIGCHLD, child_handler);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \n static int service_loop(struct socketlist *socklist)\n {\n+\tstruct sigaction sa;\n \tstruct pollfd *pfd;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n+\tsa.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sa, NULL);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n-- \ngitgitgadget\n\n"},{"id":"520750","messageId":"851d663be0bda9ecb0e267ab0f85cc1f14cff10e.1750927989.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-26T08:53:08Z","receivedAt":"2025-06-26T08:53:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nIf the setup for the SIGCHLD signal handler sets SA_RESTART, poll()\nmight not return with -1 and set errno to EINTR when a signal is\nreceived.\n\nSince the logic to reap zombie childs relies on those interruptions\nmake sure to explicitly disable SA_RESTART around this function.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/daemon.c b/daemon.c\nindex 155b2e180167..7e29c03e313f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1116,6 +1116,25 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \t}\n }\n \n+#ifndef NO_RESTARTABLE_SIGNALS\n+\n+static void set_sa_restart(struct sigaction *psa, int enable)\n+{\n+\tif (enable)\n+\t\tpsa->sa_flags |= SA_RESTART;\n+\telse\n+\t\tpsa->sa_flags &= ~SA_RESTART;\n+\tsigaction(SIGCHLD, psa, NULL);\n+}\n+\n+#else\n+\n+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n+{\n+}\n+\n+#endif\n+\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct sigaction sa;\n@@ -1136,6 +1155,7 @@ static int service_loop(struct socketlist *socklist)\n \tfor (;;) {\n \t\tcheck_dead_children();\n \n+\t\tset_sa_restart(&sa, 0);\n \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n@@ -1144,6 +1164,7 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\tset_sa_restart(&sa, 1);\n \n \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n-- \ngitgitgadget\n"},{"id":"520753","messageId":"49cf7749-fc86-4829-8e94-c1f1e87803aa@gmail.com","threadId":"63688","inReplyTo":"3f63479119ffe6fdcf694dac3cb47cd7838564b7.1750927989.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T12:52:47Z","receivedAt":"2025-06-26T12:52:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> \n> A future change will start using sigaction to setup a SIGCHLD signal\n> handler.\n> \n> The current code uses signal() which returns SIG_ERR (but doesn't\n> seem to set errno) so instruct sigaction() to do the same.\n\nWhy are we returning -1 below instead of SIG_ERR if we want the behavior \nto match?\n\nThanks\n\nPhillip\n\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> compat/mingw-posix.h | 1 +\n> compat/mingw.c | 4 +++-\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\n> index a0dca756d104..847d558c9b2d 100644\n> --- a/compat/mingw-posix.h\n> +++ b/compat/mingw-posix.h\n> @@ -95,6 +95,7 @@ struct sigaction {\n> \tsig_handler_t sa_handler;\n> \tunsigned sa_flags;\n> };\n> +#define SA_NOCLDSTOP 1\n> \n> struct itimerval {\n> \tstruct timeval it_value, it_interval;\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 8a9972a1ca19..5d69ae32f4b9 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n> \n> int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n> {\n> -\tif (sig != SIGALRM)\n> +\tif (sig == SIGCHLD)\n> +\t\treturn -1;\n> +\telse if (sig != SIGALRM)\n> \t\treturn errno = EINVAL,\n> \t\t\terror(\"sigaction only implemented for SIGALRM\");\n> \tif (out)\n\n"},{"id":"520754","messageId":"d7c86948-e0b0-4864-88f8-fd1222e0dffe@gmail.com","threadId":"63688","inReplyTo":"c66bda461f45791d278779fb0021f1e0369fe889.1750927989.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T13:11:52Z","receivedAt":"2025-06-26T13:11:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> \n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n\nOnly because you have chosen to use SA_RESTART here. I think it would be \nbetter to drop that and instead say something like\n\n\nPOSIX leaves several aspects of the behavior of signal() up to the \nimplementer. It is unspecified whether long running syscalls are \nrestarted after an interrupt is received. The event loop in \"git \ndaemon\" assumes that poll() will return with EINTR if a child exits \nwhile it is polling but if poll() is restarted after an interrupt is \nreceived that does not happen which can lead to a build up of \nuncollected zombie processes.\n\nIt is also unspecified whether the handler is reset after an interrupt \nis received. In order to work correctly on operating systems such as \nSolaris where the handler for SIGCLD is reset when a signal is received \n\"git daemon\" calls signal() from the SIGCLD handler to re-establish a \nhandler for that signal. Unfortunately this causes infinite recursion on \nother operating systems such as AIX.\n\nBoth of these problems can be addressed by using sigaction() instead of \nsignal() which has much stricter semantics and so, by setting the \nappropriate flags, we can rely on poll() being interrupted and the \nSIGCLD handler not being reset. This change means that all long running \nsystem calls could fail with EINTR not just pall() but rest of the event \nloop code is designed to gracefully handle that.\n\nAfter this change there is still a race where a child that exits after \nit has been checked in check_dead_children() but before we call poll() \nwill not be collected until a new connection is received or a child \nexits while we're polling. We could fix this by using the \"self-pipe \ntrick\" but do not because ....\n\n\nThen we can drop patches 1 and 4.\n\nBest Wishes\n\nPhillip\n\n> \n> Replace signal() with an equivalent invocation of sigaction(), which\n> has the added benefit of using BSD semantics reliably and therefore\n> not needing the rearming call in the signal handler.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/daemon.c b/daemon.c\n> index d1be61fd5789..155b2e180167 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> static void child_handler(int signo UNUSED)\n> {\n> \t/*\n> -\t * Otherwise empty handler because systemcalls will get interrupted\n> -\t * upon signal receipt\n> -\t * SysV needs the handler to be rearmed\n> +\t * Otherwise empty handler because systemcalls should get interrupted\n> +\t * upon signal receipt.\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> \n> static int service_loop(struct socketlist *socklist)\n> {\n> +\tstruct sigaction sa;\n> \tstruct pollfd *pfd;\n> \n> \tCALLOC_ARRAY(pfd, socklist->nr);\n> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> +\tsigemptyset(&sa.sa_mask);\n> +\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> +\tsa.sa_handler = child_handler;\n> +\tsigaction(SIGCHLD, &sa, NULL);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n\n"},{"id":"520755","messageId":"dba9ae0d-1e43-4345-a7ec-b57a07d45a07@gmail.com","threadId":"63688","inReplyTo":"851d663be0bda9ecb0e267ab0f85cc1f14cff10e.1750927989.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/4] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T13:14:42Z","receivedAt":"2025-06-26T13:14:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> \n> If the setup for the SIGCHLD signal handler sets SA_RESTART, poll()\n> might not return with -1 and set errno to EINTR when a signal is\n> received.\n> \n> Since the logic to reap zombie childs relies on those interruptions\n> make sure to explicitly disable SA_RESTART around this function.\n\nGiven \"git daemon\" seems to have been designed with the assumption that \nlong running system calls can fail with EINTR I don't know why this \nseries is trying to use SA_RESTART in the first place.\n\nThanks\n\nPhillip\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> daemon.c | 21 +++++++++++++++++++++\n> 1 file changed, 21 insertions(+)\n> \n> diff --git a/daemon.c b/daemon.c\n> index 155b2e180167..7e29c03e313f 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1116,6 +1116,25 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> \t}\n> }\n> \n> +#ifndef NO_RESTARTABLE_SIGNALS\n> +\n> +static void set_sa_restart(struct sigaction *psa, int enable)\n> +{\n> +\tif (enable)\n> +\t\tpsa->sa_flags |= SA_RESTART;\n> +\telse\n> +\t\tpsa->sa_flags &= ~SA_RESTART;\n> +\tsigaction(SIGCHLD, psa, NULL);\n> +}\n> +\n> +#else\n> +\n> +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n> +{\n> +}\n> +\n> +#endif\n> +\n> static int service_loop(struct socketlist *socklist)\n> {\n> \tstruct sigaction sa;\n> @@ -1136,6 +1155,7 @@ static int service_loop(struct socketlist *socklist)\n> \tfor (;;) {\n> \t\tcheck_dead_children();\n> \n> +\t\tset_sa_restart(&sa, 0);\n> \t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> \t\t\tif (errno != EINTR) {\n> \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n> @@ -1144,6 +1164,7 @@ static int service_loop(struct socketlist *socklist)\n> \t\t\t}\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tset_sa_restart(&sa, 1);\n> \n> \t\tfor (size_t i = 0; i < socklist->nr; i++) {\n> \t\t\tif (pfd[i].revents & POLLIN) {\n\n"},{"id":"520756","messageId":"qizh636elher65bsdzkiqohzyo23tmon7hxcl4jcuftculbtm6@nupmqjy3igja","threadId":"63688","inReplyTo":"49cf7749-fc86-4829-8e94-c1f1e87803aa@gmail.com","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T13:15:55Z","receivedAt":"2025-06-26T13:15:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> > \n> > A future change will start using sigaction to setup a SIGCHLD signal\n> > handler.\n> > \n> > The current code uses signal() which returns SIG_ERR (but doesn't\n> > seem to set errno) so instruct sigaction() to do the same.\n> \n> Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n> match?\n\nBy \"match\", I mean that in both cases we will get an error return value\nand errno won't be set to EINVAL (which is what POSIX requires)\n\nIn our codebase since we ignore the return code anyway, it wouldn't make\na difference, either way.\n\nsignal() returns a pointer, and sigaction() returns and int, so you can\nhave the later be literally SIG_ERR, eventhough it will be ironically\nequivalent it casted into an int.\n\nCsrlo\n"},{"id":"520759","messageId":"a1fb8c27-6ddf-42d5-a062-a9710f6cc1cd@gmail.com","threadId":"63688","inReplyTo":"qizh636elher65bsdzkiqohzyo23tmon7hxcl4jcuftculbtm6@nupmqjy3igja","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T13:56:22Z","receivedAt":"2025-06-26T13:56:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>>>\n>>> A future change will start using sigaction to setup a SIGCHLD signal\n>>> handler.\n>>>\n>>> The current code uses signal() which returns SIG_ERR (but doesn't\n>>> seem to set errno) so instruct sigaction() to do the same.\n>>\n>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n>> match?\n> \n> By \"match\", I mean that in both cases we will get an error return value\n> and errno won't be set to EINVAL (which is what POSIX requires)\n> \n> In our codebase since we ignore the return code anyway, it wouldn't make\n> a difference, either way.\n> \n> signal() returns a pointer, and sigaction() returns and int, \n\nOh right, I'd forgotten they have different return types. I think we \nshould probably be setting errno = EINVAL before returning -1 to match \nwhat this function does with other signals it does not support - just \nbecause our current callers ignore the return value doesn't mean that \nfuture callers will and they might want check errno if they see the \nfunction fail.\n\nThanks\n\nPhillip\n\n> so you can\n> have the later be literally SIG_ERR, eventhough it will be ironically\n> equivalent it casted into an int.\n> \n> Csrlo\n> \n\n"},{"id":"520763","messageId":"o6cihjnfj4q6uiks3syovjun3fcijvsqto444osw7tgtpkttvt@42r37athz2tw","threadId":"63688","inReplyTo":"a1fb8c27-6ddf-42d5-a062-a9710f6cc1cd@gmail.com","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T14:58:13Z","receivedAt":"2025-06-26T14:58:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:\n> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n> > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n> > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> > > > \n> > > > A future change will start using sigaction to setup a SIGCHLD signal\n> > > > handler.\n> > > > \n> > > > The current code uses signal() which returns SIG_ERR (but doesn't\n> > > > seem to set errno) so instruct sigaction() to do the same.\n> > > \n> > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n> > > match?\n> > \n> > By \"match\", I mean that in both cases we will get an error return value\n> > and errno won't be set to EINVAL (which is what POSIX requires)\n> > \n> > In our codebase since we ignore the return code anyway, it wouldn't make\n> > a difference, either way.\n> > \n> > signal() returns a pointer, and sigaction() returns and int,\n> \n> Oh right, I'd forgotten they have different return types. I think we should\n> probably be setting errno = EINVAL before returning -1 to match what this\n> function does with other signals it does not support - just because our\n> current callers ignore the return value doesn't mean that future callers\n> will and they might want check errno if they see the function fail.\n\nI agree, and indeed had to triple check and change my implementation after I\nconfirmed that signal(SIGCHLD) does not change errno on Windows (not our\nversion, neither of the windows libc or mingw, even if it is documented[1] to\ndo so.\n\nIt might be because the signal number itself is bogus (there is none for\nSIGCHLD in their headers, and git uses their own numbers in compat), but\neither way, I would rather be consistent with signal() at least originally.\n\nCarlo\n\n[1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal\n"},{"id":"520764","messageId":"0dd51eab-8869-46be-beca-238a616dd6f3@gmail.com","threadId":"63688","inReplyTo":"o6cihjnfj4q6uiks3syovjun3fcijvsqto444osw7tgtpkttvt@42r37athz2tw","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T15:19:11Z","receivedAt":"2025-06-26T15:19:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:\n> On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:\n>> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n>>> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n>>>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n>>>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>>>>>\n>>>>> A future change will start using sigaction to setup a SIGCHLD signal\n>>>>> handler.\n>>>>>\n>>>>> The current code uses signal() which returns SIG_ERR (but doesn't\n>>>>> seem to set errno) so instruct sigaction() to do the same.\n>>>>\n>>>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n>>>> match?\n>>>\n>>> By \"match\", I mean that in both cases we will get an error return value\n>>> and errno won't be set to EINVAL (which is what POSIX requires)\n>>>\n>>> In our codebase since we ignore the return code anyway, it wouldn't make\n>>> a difference, either way.\n>>>\n>>> signal() returns a pointer, and sigaction() returns and int,\n>>\n>> Oh right, I'd forgotten they have different return types. I think we should\n>> probably be setting errno = EINVAL before returning -1 to match what this\n>> function does with other signals it does not support - just because our\n>> current callers ignore the return value doesn't mean that future callers\n>> will and they might want check errno if they see the function fail.\n> \n> I agree, and indeed had to triple check and change my implementation after I\n> confirmed that signal(SIGCHLD) does not change errno on Windows (not our\n> version, neither of the windows libc or mingw, even if it is documented[1] to\n> do so.\n> \n> It might be because the signal number itself is bogus (there is none for\n> SIGCHLD in their headers, and git uses their own numbers in compat), but\n> either way, I would rather be consistent with signal() at least originally.\n\nI'm not sure I understand - don't we want the sigaction() wrapper to \nbehave like sigaction() would?\n\nThanks\n\nPhillip\n\n> Carlo\n> \n> [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal\n\n"},{"id":"520765","messageId":"xmqqy0teplfa.fsf@gitster.g","threadId":"63688","inReplyTo":"d7c86948-e0b0-4864-88f8-fd1222e0dffe@gmail.com","subject":"Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-26T15:33:29Z","receivedAt":"2025-06-26T15:33:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Only because you have chosen to use SA_RESTART here. I think it would\n> be better to drop that and instead say something like\n>\n>\n> POSIX leaves several aspects of the behavior of signal() up to the\n> implementer. It is unspecified whether long running syscalls are\n> restarted after an interrupt is received. The event loop in \"git\n> daemon\" assumes that poll() will return with EINTR if a child exits\n> while it is polling but if poll() is restarted after an interrupt is\n> received that does not happen which can lead to a build up of\n> uncollected zombie processes.\n>\n> It is also unspecified whether the handler is reset after an interrupt\n> is received. In order to work correctly on operating systems such as\n> Solaris where the handler for SIGCLD is reset when a signal is\n> received \"git daemon\" calls signal() from the SIGCLD handler to\n> re-establish a handler for that signal. Unfortunately this causes\n> infinite recursion on other operating systems such as AIX.\n>\n> Both of these problems can be addressed by using sigaction() instead\n> of signal() which has much stricter semantics and so, by setting the\n> appropriate flags, we can rely on poll() being interrupted and the\n> SIGCLD handler not being reset. This change means that all long\n> running system calls could fail with EINTR not just pall() but rest of\n> the event loop code is designed to gracefully handle that.\n>\n> After this change there is still a race where a child that exits after\n> it has been checked in check_dead_children() but before we call poll()\n> will not be collected until a new connection is received or a child\n> exits while we're polling. We could fix this by using the \"self-pipe\n> trick\" but do not because ....\n>\n>\n> Then we can drop patches 1 and 4.\n\nThanks for a suggestion. I really like the \"everybody in these code\npaths are prepared to receive and handle EINTR just fine, so we do\nnot have to do the SA_RESTART\" observation very much.\n\n"},{"id":"520772","messageId":"wanrtwacxrjmmpfnjwxhgdfhlo4uvnktijnc2rxdzlnkpe5r4a@3b2e2onpqakp","threadId":"63688","inReplyTo":"xmqqy0teplfa.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T16:36:01Z","receivedAt":"2025-06-26T16:36:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > Only because you have chosen to use SA_RESTART here. I think it would\n> > be better to drop that and instead say something like\n> >\n> >\n> > POSIX leaves several aspects of the behavior of signal() up to the\n> > implementer. It is unspecified whether long running syscalls are\n> > restarted after an interrupt is received. The event loop in \"git\n> > daemon\" assumes that poll() will return with EINTR if a child exits\n> > while it is polling but if poll() is restarted after an interrupt is\n> > received that does not happen which can lead to a build up of\n> > uncollected zombie processes.\n> >\n> > It is also unspecified whether the handler is reset after an interrupt\n> > is received. In order to work correctly on operating systems such as\n> > Solaris where the handler for SIGCLD is reset when a signal is\n> > received \"git daemon\" calls signal() from the SIGCLD handler to\n> > re-establish a handler for that signal. Unfortunately this causes\n> > infinite recursion on other operating systems such as AIX.\n> >\n> > Both of these problems can be addressed by using sigaction() instead\n> > of signal() which has much stricter semantics and so, by setting the\n> > appropriate flags, we can rely on poll() being interrupted and the\n> > SIGCLD handler not being reset. This change means that all long\n> > running system calls could fail with EINTR not just pall() but rest of\n> > the event loop code is designed to gracefully handle that.\n> >\n> > After this change there is still a race where a child that exits after\n> > it has been checked in check_dead_children() but before we call poll()\n> > will not be collected until a new connection is received or a child\n> > exits while we're polling. We could fix this by using the \"self-pipe\n> > trick\" but do not because ....\n> >\n> >\n> > Then we can drop patches 1 and 4.\n> \n> Thanks for a suggestion. I really like the \"everybody in these code\n> paths are prepared to receive and handle EINTR just fine, so we do\n> not have to do the SA_RESTART\" observation very much.\n\nExcept that is unlikely to be true, as the code has changed a lot on\nthose 20 years (including that it now uses run_command) and that we\nhad been effectively using SA_RESTART under the covers for most of\nthat time because signal() behaviour changed. An obvious bug:\n(ex: 20250626161038.85966-1-carenas@gmail.com)\n\nI think using SA_RESTART by default might be safer, specially considering\nthat patch 4 already exist and it is not that complicated now that we\nhave 2.\n\nCarlo\n"},{"id":"520776","messageId":"087d437f-3163-4c63-b6f5-e5d726016359@gmail.com","threadId":"63688","inReplyTo":"wanrtwacxrjmmpfnjwxhgdfhlo4uvnktijnc2rxdzlnkpe5r4a@3b2e2onpqakp","subject":"Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-26T18:04:30Z","receivedAt":"2025-06-26T18:04:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 26/06/2025 17:36, Carlo Marcelo Arenas Belón wrote:\n> On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>>\n>> Thanks for a suggestion. I really like the \"everybody in these code\n>> paths are prepared to receive and handle EINTR just fine, so we do\n>> not have to do the SA_RESTART\" observation very much.\n> \n> Except that is unlikely to be true, as the code has changed a lot on\n> those 20 years (including that it now uses run_command) and that we\n> had been effectively using SA_RESTART under the covers for most of\n> that time because signal() behaviour changed. An obvious bug:\n> (ex: 20250626161038.85966-1-carenas@gmail.com)\n\nI don't think the syscall handling has changed much since 695605b508 \n(git-daemon: Simplify dead-children reaping logic, 2008-08-14) when we \nstarted relying on poll() returning EINTR to collect children that exit \nwhile we are waiting for a new connection. In any case my comment was \nbased on reading the code. You're right that we started using \nrun_command() after 695605b508 but when I read the code in run-command.c \nit looked to me like it is pretty careful in the way it handles signals \nand EINTR - what part of the code are you concerned about? As git \nsupports operating systems that do not provide SA_RESTART we should to \nmake sure we handle EINTR correctly in this code anyway. As far as the \npatch you link to goes, it may not have been handling EINTR as \nefficiently as you'd like but it would still accept the client on the \nnext iteration of the loop which would happen immediately because the \nconnection would be pending when we call poll().\n\n> I think using SA_RESTART by default might be safer,\n\nThe counter argument to this is that it is masking bugs that happen on \nplatforms without SA_RESTART.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"520777","messageId":"20250626182432.87523-1-carenas@gmail.com","threadId":"63688","inReplyTo":"c314cd2d-8fdd-4386-bda0-881ff87d9204@gmail.com","subject":"[RFC PATCH] daemon: add a self pipe to trigger reaping of children","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T18:24:32Z","receivedAt":"2025-06-26T18:24:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"There has always been a small race condition between the time a\nchildren status is checked, and the arrival of a SIGCHLD that\nwould alert us of their demise, hopefully interrupt poll().\n\nTo close the gap, add the reading side of a pipe to the poll and\nuse the signal handler to write to it for each child.\n\nSuggested=by: Phillip Wood \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nImplements the \"self pipe\" trick Hillip suggested.\n\nI tried to make self healing and optional, as I also presume the race\ncondition it is meant to address is very unlikely.\n\nAn obvious disadvantage (at least of this implementation), is that it\nactually doubles the number of events that need to be handled for each\nchildren process on most cases (ex: when `poll()` gets interrupted)\n\nI suspect that if fixing that last race condition is so important with\nthe current foundation, it might be better to reintroduce some sort of\ntimeout to poll(), so that they will be cleared periodically.\n\nI had a prototype (only the bare minimum) that I thought was more\nefficient and that would instead remove completely the need for a\nsignal handler which I would post (only for RFC) later.\n\n daemon.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 46 insertions(+), 9 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd57..d3b9421575 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tadd_child(&cld, addr, addrlen);\n }\n \n-static void child_handler(int signo UNUSED)\n+int poll_pipe[2] = { -1, -1 };\n+\n+static void child_handler(int signo)\n {\n \t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n \t * SysV needs the handler to be rearmed\n \t */\n \tsignal(SIGCHLD, child_handler);\n+\n+\tif (poll_pipe[1] >= 0)\n+\t\twrite(poll_pipe[1], &signo, 1);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct pollfd *pfd;\n+\tunsigned long nfds = 1 + socklist->nr;\n+\n+\tALLOC_ARRAY(pfd, nfds);\n+\tif (!pipe(poll_pipe)) {\n+\t\tfor (int i = 0; i < 2; i++) {\n+\t\t\tint flags;\n+\n+\t\t\tflags = fcntl(poll_pipe[i], F_GETFD, 0);\n+\t\t\tif (flags >= 0)\n+\t\t\t\tfcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);\n+\n+\t\t\tflags = fcntl(poll_pipe[i], F_GETFL, 0);\n+\t\t\tif (flags < 0 || fcntl(poll_pipe[i], F_SETFL,\n+\t\t\t\t\t flags | O_NONBLOCK) == -1) {\n+\t\t\t\tclose(poll_pipe[0]);\n+\t\t\t\tclose(poll_pipe[1]);\n+\t\t\t\tpoll_pipe[0] = poll_pipe[1] = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tpfd[0].fd = poll_pipe[0];\n+\tpfd[0].events = POLLIN;\n \n-\tCALLOC_ARRAY(pfd, socklist->nr);\n-\n-\tfor (size_t i = 0; i < socklist->nr; i++) {\n-\t\tpfd[i].fd = socklist->list[i];\n+\tfor (size_t i = 1; i < nfds; i++) {\n+\t\tpfd[i].fd = socklist->list[i - 1];\n \t\tpfd[i].events = POLLIN;\n \t}\n \n \tsignal(SIGCHLD, child_handler);\n \n \tfor (;;) {\n+\t\tint nevents, scratch;\n+\n \t\tcheck_dead_children();\n \n-\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n+\t\tif ((nevents = poll(pfd, nfds, -1)) <= 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n \t\t\t\t strerror(errno));\n@@ -1143,7 +1169,17 @@ static int service_loop(struct socketlist *socklist)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tfor (size_t i = 0; i < socklist->nr; i++) {\n+\t\tif ((pfd[0].revents & POLLIN) && pfd[0].fd != -1) {\n+\t\t\tif (nevents == 1 && read(pfd[0].fd, &scratch, 1) > 0)\n+\t\t\t\tcontinue;\n+\t\t\telse if (!read(pfd[0].fd, &scratch, 1)) {\n+\t\t\t\tclose(pfd[0].fd);\n+\t\t\t\tpfd[0].fd = -1;\n+\t\t\t}\n+\t\t\tnevents--;\n+\t\t}\n+\n+\t\tfor (size_t i = 1; nevents && i < nfds; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n \t\t\t\tunion {\n \t\t\t\t\tstruct sockaddr sa;\n@@ -1165,6 +1201,7 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\thandle(incoming, &ss.sa, sslen);\n+\t\t\t\tnevents--;\n \t\t\t}\n \t\t}\n \t}\n-- \n2.50.0.131.gcf6f63ea6b.dirty\n\n"},{"id":"520782","messageId":"p6xegxqqq4wzi6gnokypy3k5auxk3d2wxmj4pj45ugfomace3q@y5q3e2al42oj","threadId":"63688","inReplyTo":"0dd51eab-8869-46be-beca-238a616dd6f3@gmail.com","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T20:09:12Z","receivedAt":"2025-06-26T20:09:14Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote:\n> On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:\n> > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:\n> > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n> > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n> > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> > > > > > \n> > > > > > A future change will start using sigaction to setup a SIGCHLD signal\n> > > > > > handler.\n> > > > > > \n> > > > > > The current code uses signal() which returns SIG_ERR (but doesn't\n> > > > > > seem to set errno) so instruct sigaction() to do the same.\n> > > > > \n> > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n> > > > > match?\n> > > > \n> > > > By \"match\", I mean that in both cases we will get an error return value\n> > > > and errno won't be set to EINVAL (which is what POSIX requires)\n> > > > \n> > > > In our codebase since we ignore the return code anyway, it wouldn't make\n> > > > a difference, either way.\n> > > > \n> > > > signal() returns a pointer, and sigaction() returns and int,\n> > > \n> > > Oh right, I'd forgotten they have different return types. I think we should\n> > > probably be setting errno = EINVAL before returning -1 to match what this\n> > > function does with other signals it does not support - just because our\n> > > current callers ignore the return value doesn't mean that future callers\n> > > will and they might want check errno if they see the function fail.\n> > \n> > I agree, and indeed had to triple check and change my implementation after I\n> > confirmed that signal(SIGCHLD) does not change errno on Windows (not our\n> > version, neither of the windows libc or mingw, even if it is documented[1] to\n> > do so.\n> > \n> > It might be because the signal number itself is bogus (there is none for\n> > SIGCHLD in their headers, and git uses their own numbers in compat), but\n> > either way, I would rather be consistent with signal() at least originally.\n> \n> I'm not sure I understand - don't we want the sigaction() wrapper to behave\n> like sigaction() would?\n\nfor at least the first iteration, I would rather have sigaction() behave\nlike signal(), so that the change doesn't introduce any regressions.\n\neventually, sigaction() should behave like any other sigaction(), but to\ndo so, I suspect the windows emulation might need to change their SIGCHLD\nto match.\n\njust confirmed with MSVC that if I use 20 instead of 17, errno gets updated\njust like the documentation says it should.\n\nCarlo\n\nPS. Maybe we should get dscho involved?\n> > \n> > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal\n"},{"id":"520783","messageId":"20250626212234.88570-1-carenas@gmail.com","threadId":"63688","inReplyTo":"20250626182432.87523-1-carenas@gmail.com","subject":"[RFC PATCH 0/2] daemon: tracking childs without signals","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T21:22:32Z","receivedAt":"2025-06-26T21:22:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This is a minimum POC of what could be done to remove signals when\nhandling children in git-daemon.\n\nThe advantage is that it is at least portable to *NIX systems, unlike\nusing a Linux specific API like pidfd which also allows collecting\nstatus from children through fds.\n\nIt has to do some innefficient management of the fds array that is\ngiven to poll, and wouldn't work as a general solution, since the\nchildren could close the pipe() used for tracking unintentionally,\nbut since we control both sides in this case, that might not be a\nproblem.\n\nCarlo Marcelo Arenas Belón (2):\n run-command: add a pipe() write end to childs\n daemon: poor man's pidfd like POC\n\n daemon.c | 74 ++++++++++++++++++++++++++++++++++++---------------\n run-command.c | 3 +++\n run-command.h | 2 ++\n 3 files changed, 57 insertions(+), 22 deletions(-)\n\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"520784","messageId":"20250626212234.88570-2-carenas@gmail.com","threadId":"63688","inReplyTo":"20250626212234.88570-1-carenas@gmail.com","subject":"[RFC PATCH 1/2] run-command: add a pipe() write end to childs","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T21:22:33Z","receivedAt":"2025-06-26T21:22:54Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Instruct the child to close the read side of a pipe provided by\nthe parent for notification that would be used in a future patch.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n run-command.c | 3 +++\n run-command.h | 2 ++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8833b23367..0156d3a01d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -796,6 +796,9 @@ int start_command(struct child_process *cmd)\n \t\tset_error_routine(child_error_fn);\n \t\tset_warn_routine(child_warn_fn);\n \n+\t\tif (cmd->parent_ipc_in != -1)\n+\t\t\tclose(cmd->parent_ipc_in);\n+\n \t\tclose(notify_pipe[0]);\n \t\tset_cloexec(notify_pipe[1]);\n \t\tchild_notifier = notify_pipe[1];\ndiff --git a/run-command.h b/run-command.h\nindex 0df25e445f..d731b400b9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -81,6 +81,7 @@ struct child_process {\n \tconst char *trace2_child_class;\n \tconst char *trace2_hook_name;\n \n+\tint parent_ipc_in;\n \t/*\n \t * Using .in, .out, .err:\n \t * - Specify 0 for no redirections. No new file descriptor is allocated.\n@@ -147,6 +148,7 @@ struct child_process {\n #define CHILD_PROCESS_INIT { \\\n \t.args = STRVEC_INIT, \\\n \t.env = STRVEC_INIT, \\\n+\t.parent_ipc_in = -1, \\\n }\n \n /**\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"520785","messageId":"20250626212234.88570-3-carenas@gmail.com","threadId":"63688","inReplyTo":"20250626212234.88570-1-carenas@gmail.com","subject":"[RFC PATCH 2/2] daemon: poor man's pidfd like POC","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-26T21:22:34Z","receivedAt":"2025-06-26T21:22:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Create a pipe for each child, and let the write side leak into\nthe children, then add the read side to the poll and wait for\nthe pipe to close (presumed to happen when the child is done).\n\nThere is no need to change the children code, since the OS (at\nleast in *NIX) will close all fds on termination.\n\nThis implementation is suboptimal, as it shouldn't need to\nscan all children once a notification is received, but does so\nto minimize changes.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 74 +++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 52 insertions(+), 22 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd57..c78556be97 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -811,9 +811,19 @@ static struct child {\n \tstruct sockaddr_storage address;\n } *firstborn;\n \n-static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)\n+static void add_child(nfds_t *nfds, struct pollfd **pfd,\n+\t\t\tstruct child_process *cld,\n+\t\t\tstruct sockaddr *addr, socklen_t addrlen)\n {\n \tstruct child *newborn, **cradle;\n+\tstruct pollfd newfd;\n+\tnfds_t size = *nfds;\n+\n+\tnewfd.fd = cld->parent_ipc_in;\n+\tnewfd.events = POLL_HUP;\n+\t*nfds = size + 1;\n+\tREALLOC_ARRAY(*pfd, *nfds);\n+\tmemcpy(*pfd + size, &newfd, sizeof(newfd));\n \n \tCALLOC_ARRAY(newborn, 1);\n \tlive_children++;\n@@ -846,10 +856,11 @@ static void kill_some_child(void)\n \t\t}\n }\n \n-static void check_dead_children(void)\n+static void check_dead_children(nfds_t *nfds, struct pollfd *pfd, nfds_t base)\n {\n \tint status;\n \tpid_t pid;\n+\tnfds_t size = *nfds;\n \n \tstruct child **cradle, *blanket;\n \tfor (cradle = &firstborn; (blanket = *cradle);)\n@@ -859,6 +870,20 @@ static void check_dead_children(void)\n \t\t\t\tdead = \" (with error)\";\n \t\t\tloginfo(\"[%\"PRIuMAX\"] Disconnected%s\", (uintmax_t)pid, dead);\n \n+\t\t\tfor (nfds_t i = base; i < size; i++)\n+\t\t\t\tif (pfd[i].fd == blanket->cld.parent_ipc_in) {\n+\t\t\t\t\tclose(blanket->cld.parent_ipc_in);\n+\t\t\t\t\tsize--;\n+\t\t\t\t\tif (size - i) {\n+\t\t\t\t\t\tMOVE_ARRAY(pfd + i,\n+\t\t\t\t\t\t\t\tpfd + i + 1,\n+\t\t\t\t\t\t\t\tsize - i);\n+\t\t\t\t\t}\n+\n+\t\t\t\t\t*nfds = size;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\n \t\t\t/* remove the child */\n \t\t\t*cradle = blanket->next;\n \t\t\tlive_children--;\n@@ -869,14 +894,16 @@ static void check_dead_children(void)\n }\n \n static struct strvec cld_argv = STRVEC_INIT;\n-static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n+static void handle(int incoming, nfds_t *nfds, struct pollfd **pfd, nfds_t base,\n+\t\t\tstruct sockaddr *addr, socklen_t addrlen)\n {\n+\tint ipc[2] = { -1, -1 };\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n \n \tif (max_connections && live_children >= max_connections) {\n \t\tkill_some_child();\n \t\tsleep(1); /* give it some time to die */\n-\t\tcheck_dead_children();\n+\t\tcheck_dead_children(nfds, *pfd, base);\n \t\tif (live_children >= max_connections) {\n \t\t\tclose(incoming);\n \t\t\tlogerror(\"Too many children, dropping connection\");\n@@ -902,24 +929,22 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n #endif\n \t}\n \n+#ifndef GIT_WINDOWS_NATIVE\n+\tpipe(ipc);\n+\tcld.parent_ipc_in = ipc[0];\n+#endif\n+\n \tstrvec_pushv(&cld.args, cld_argv.v);\n \tcld.in = incoming;\n \tcld.out = dup(incoming);\n \n \tif (start_command(&cld))\n \t\tlogerror(\"unable to fork\");\n-\telse\n-\t\tadd_child(&cld, addr, addrlen);\n-}\n-\n-static void child_handler(int signo UNUSED)\n-{\n-\t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n-\t * SysV needs the handler to be rearmed\n-\t */\n-\tsignal(SIGCHLD, child_handler);\n+\telse {\n+\t\tif (ipc[1] != -1)\n+\t\t\tclose(ipc[1]);\n+\t\tadd_child(nfds, pfd, &cld, addr, addrlen);\n+\t}\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1121,6 +1146,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n static int service_loop(struct socketlist *socklist)\n {\n \tstruct pollfd *pfd;\n+\tnfds_t nfds = socklist->nr;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n \n@@ -1129,12 +1155,10 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n-\n \tfor (;;) {\n-\t\tcheck_dead_children();\n+\t\tsize_t i;\n \n-\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n+\t\tif (poll(pfd, nfds, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n \t\t\t\t strerror(errno));\n@@ -1143,7 +1167,12 @@ static int service_loop(struct socketlist *socklist)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tfor (size_t i = 0; i < socklist->nr; i++) {\n+\t\tfor (i = socklist->nr; i < nfds; i++) {\n+\t\t\tif (pfd[i].revents & POLLHUP)\n+\t\t\t\tcheck_dead_children(&nfds, pfd, socklist->nr);\n+\t\t}\n+\n+\t\tfor (i = 0; i < socklist->nr; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n \t\t\t\tunion {\n \t\t\t\t\tstruct sockaddr sa;\n@@ -1164,7 +1193,8 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t\t\t\tdie_errno(\"accept returned\");\n \t\t\t\t\t}\n \t\t\t\t}\n-\t\t\t\thandle(incoming, &ss.sa, sslen);\n+\t\t\t\thandle(incoming, &nfds, &pfd, socklist->nr,\n+\t\t\t\t\t&ss.sa, sslen);\n \t\t\t}\n \t\t}\n \t}\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"520791","messageId":"xmqq4iw29d11.fsf@gitster.g","threadId":"63688","inReplyTo":"ae1ca6bb2b258fc3c18c627aed2159dbb8f8c268.1750927989.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/4] compat/posix.h: track SA_RESTART fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-27T01:41:30Z","receivedAt":"2025-06-27T01:41:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> +\tAC_COMPILE_IFELSE(\n> +\t\t[AC_LANG_PROGRAM([#include ], [[\n> +\t\t\t#ifdef SA_RESTART\n> +\t\t\trestartable signals supported\n> +\t\t\t#endif\n\nSo, where SA_RESTART is defined, we fail the compilation.\n\n> +\t\t]])],[\n> +\t\t\tac_cv_siginterrupt=no\n> +\t\t\tNO_RESTARTABLE_SIGNALS=UnfortunatelyYes\n\nAs this is IFELSE, we know the condition that did not fail the\ncompilation is where we did not see SA_RESTART. So we set the\nNO_RESTARTABLE_SIGNALS=UnfortunatelyYes, which makes sense.\n\n> +\t\t], [ac_cv_siginterrupt=yes]\n> +\t)\n> +])\n> +GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])\n\nIt is curious that throughout the two renames, the cached variable\nused by autoconf hasn't changed its name. Is it because it is\ntotally invisible to the end-users/builders?\n"},{"id":"520801","messageId":"59087d2d-6034-44d4-9fa0-c51d4bd60683@gmail.com","threadId":"63688","inReplyTo":"20250626182432.87523-1-carenas@gmail.com","subject":"Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-27T08:38:36Z","receivedAt":"2025-06-27T08:38:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:\n> There has always been a small race condition between the time a\n> children status is checked, and the arrival of a SIGCHLD that\n> would alert us of their demise, hopefully interrupt poll().\n\nThe race was introduced by 695605b5080 (git-daemon: Simplify \ndead-children reaping logic, 2008-08-14). Before that children were \nreaped inside the signal handler so there was no race.\n> To close the gap, add the reading side of a pipe to the poll and\n> use the signal handler to write to it for each child.\n\nAs well as closing the race this fixes the issue with poll() not being \ninterrupted when a child exits. It also allows us to move the re-arming \nof the handler into the event loop which would fix the infinite \nrecursion on AIX.\n\n> Suggested=by: Phillip Wood \n\ns/=/-/\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Implements the \"self pipe\" trick Hillip suggested.\n\nI've never been called that before :-/\n\n> \n> I tried to make self healing and optional, as I also presume the race\n> condition it is meant to address is very unlikely.\n\nI see this as an alternative fix for the other problems you've been \nworking on as well.\n\n> An obvious disadvantage (at least of this implementation), is that it\n> actually doubles the number of events that need to be handled for each\n> children process on most cases (ex: when `poll()` gets interrupted)\n> \n> I suspect that if fixing that last race condition is so important with\n> the current foundation, it might be better to reintroduce some sort of\n> timeout to poll(), so that they will be cleared periodically.\n> \n> I had a prototype (only the bare minimum) that I thought was more\n> efficient and that would instead remove completely the need for a\n> signal handler which I would post (only for RFC) later.\n\nI'm not sure injecting an fd into each child process is a good direction.\n\n> daemon.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------\n> 1 file changed, 46 insertions(+), 9 deletions(-)\n> \n> diff --git a/daemon.c b/daemon.c\n> index d1be61fd57..d3b9421575 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> \t\tadd_child(&cld, addr, addrlen);\n> }\n> \n> -static void child_handler(int signo UNUSED)\n> +int poll_pipe[2] = { -1, -1 };\nMaybe call this signal_pipe? I'm not sure what poll_pipe means.\n\n> +\n> +static void child_handler(int signo)\n> {\n> \t/*\n> -\t * Otherwise empty handler because systemcalls will get interrupted\n> -\t * upon signal receipt\n> \t * SysV needs the handler to be rearmed\n> \t */\n> \tsignal(SIGCHLD, child_handler);\n\nI think from Chris' email that it is conventional to do this at the end \nof the handler. As I said above we could add an additional commit that \nmoves this into the event loop to fix the infinite recursion on AIX.\n\n> +\n> +\tif (poll_pipe[1] >= 0)\n> +\t\twrite(poll_pipe[1], &signo, 1);\n\nwrite() might fail so we should save errno around it. Conventionally one \nwould re-try on EINTR as well though in this case the most likely reason \nfor that is another child exiting which means the pipe would be written \nto anyway.\n\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> static int service_loop(struct socketlist *socklist)\n> {\n> \tstruct pollfd *pfd;\n> +\tunsigned long nfds = 1 + socklist->nr;\n> +\n> +\tALLOC_ARRAY(pfd, nfds);\n> +\tif (!pipe(poll_pipe)) {\n\nIf we cannot create a pipe here then things have gone pretty badly wrong \nand I think it is unlikely we're going to be able to accept incoming \nconnections so it would be best to die(). Relying on the signal pipe \nwould solve the problem of children not being collected until we receive \nan new connection as well.\n\n> +\t\tfor (int i = 0; i < 2; i++) {\n\nThe body of this loop is quite indented - I think it would be better to \nturn it into a function.\n\n> +\t\t\tint flags;\n> +\n> +\t\t\tflags = fcntl(poll_pipe[i], F_GETFD, 0);\n> +\t\t\tif (flags >= 0)\n> +\t\t\t\tfcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);\nI think we should probably close the pipes if we do not set FD_CLOEXEC.\n\n> +\n> +\t\t\tflags = fcntl(poll_pipe[i], F_GETFL, 0);\n> +\t\t\tif (flags < 0 || fcntl(poll_pipe[i], F_SETFL,\n> +\t\t\t\t\t flags | O_NONBLOCK) == -1) {\n> +\t\t\t\tclose(poll_pipe[0]);\n> +\t\t\t\tclose(poll_pipe[1]);\n> +\t\t\t\tpoll_pipe[0] = poll_pipe[1] = -1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\tpfd[0].fd = poll_pipe[0];\n> +\tpfd[0].events = POLLIN;\n> \n> -\tCALLOC_ARRAY(pfd, socklist->nr);\n> -\n> -\tfor (size_t i = 0; i < socklist->nr; i++) {\n> -\t\tpfd[i].fd = socklist->list[i];\n> +\tfor (size_t i = 1; i < nfds; i++) {\n> +\t\tpfd[i].fd = socklist->list[i - 1];\n> \t\tpfd[i].events = POLLIN;\n> \t}\n> \n> \tsignal(SIGCHLD, child_handler);\n> \n> \tfor (;;) {\n> +\t\tint nevents, scratch;\n> +\n> \t\tcheck_dead_children();\n> \n> -\t\tif (poll(pfd, socklist->nr, -1) < 0) {\n> +\t\tif ((nevents = poll(pfd, nfds, -1)) <= 0) {\n> \t\t\tif (errno != EINTR) {\n> \t\t\t\tlogerror(\"Poll failed, resuming: %s\",\n> \t\t\t\t strerror(errno));\n\t\t\t\tsleep(1);\n\t\t\t}\n\t\t\tcontinue;\n\nWe need to drain the signal pipe on EINTR. I think it would be best to \ndo that just before calling check_dead_childern() above. We can avoid \ncalling check_dead_children() if we don't read anything.\n\n\t\t}\n\n> @@ -1143,7 +1169,17 @@ static int service_loop(struct socketlist *socklist)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tfor (size_t i = 0; i < socklist->nr; i++) {\n> +\t\tif ((pfd[0].revents & POLLIN) && pfd[0].fd != -1) {\n> +\t\t\tif (nevents == 1 && read(pfd[0].fd, &scratch, 1) > 0)\n\nThere can be more than one byte to read from the signal_pipe - we should \ndrain it until we see EAGAIN. As I said above I think it would be better \nto do that just before calling check_dead_children(). Here we can just \ndecrement nevents if poll tells us the signal_pipe is readable.\n\nThanks\n\nPhillip\n\n> +\t\t\t\tcontinue;\n> +\t\t\telse if (!read(pfd[0].fd, &scratch, 1)) {\n> +\t\t\t\tclose(pfd[0].fd);\n> +\t\t\t\tpfd[0].fd = -1;\n> +\t\t\t}\n> +\t\t\tnevents--;\n> +\t\t}\n> +\n> +\t\tfor (size_t i = 1; nevents && i < nfds; i++) {\n> \t\t\tif (pfd[i].revents & POLLIN) {\n> \t\t\t\tunion {\n> \t\t\t\t\tstruct sockaddr sa;\n> @@ -1165,6 +1201,7 @@ static int service_loop(struct socketlist *socklist)\n> \t\t\t\t\t}\n> \t\t\t\t}\n> \t\t\t\thandle(incoming, &ss.sa, sslen);\n> +\t\t\t\tnevents--;\n> \t\t\t}\n> \t\t}\n> \t}\n"},{"id":"520838","messageId":"zy6xtfqnncs3kuipgvdb7jiu7ynodbf7mld4r2ojy3jwkkthm6@jtnzecikxda2","threadId":"63688","inReplyTo":"59087d2d-6034-44d4-9fa0-c51d4bd60683@gmail.com","subject":"Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-28T06:19:20Z","receivedAt":"2025-06-28T06:19:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Jun 27, 2025 at 09:38:36AM -0800, Phillip Wood wrote:\n> \n> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:\n> > \n> > I had a prototype (only the bare minimum) that I thought was more\n> > efficient and that would instead remove completely the need for a\n> > signal handler which I would post (only for RFC) later.\n> \n> I'm not sure injecting an fd into each child process is a good direction.\n\nI don't either, but frankly don't think it is as much of an issue, if we\nconsider that all the children are known internal processes we also own.\n\nWas hoping that by producing a working (albeit ugly in purpose so changes\nto the current code would be minimized) prototype, we could have a\ndiscussion of the potential issues/benefits that would be a little more\nverbose.\n\nIndeed I posted this an the other series both as RFC, and as replies of\neach other hoping to give them both a chance to be evaluated as alternatives\ntogether.\n\n> > diff --git a/daemon.c b/daemon.c\n> > index d1be61fd57..d3b9421575 100644\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> > \t\tadd_child(&cld, addr, addrlen);\n> > }\n> > -static void child_handler(int signo UNUSED)\n> > +int poll_pipe[2] = { -1, -1 };\n> Maybe call this signal_pipe? I'm not sure what poll_pipe means.\n> \n> > +\n> > +static void child_handler(int signo)\n> > {\n> > \t/*\n> > -\t * Otherwise empty handler because systemcalls will get interrupted\n> > -\t * upon signal receipt\n> > \t * SysV needs the handler to be rearmed\n> > \t */\n> > \tsignal(SIGCHLD, child_handler);\n> \n> I think from Chris' email that it is conventional to do this at the end of\n> the handler.\n\nIt really depends on which flags the signal was expected to use. For maximum\nportability it would seem better to have it at the beginning to minimize the\ninherent race condition from when SA_RESETHAND is on effect (which was more\nof a SysV signal() tradition).\n\nWill move it to the end, regardless, as it won't be needed once the call is\nsetup using sigaction().\n\n> As I said above we could add an additional commit that moves\n> this into the event loop to fix the infinite recursion on AIX.\n\nNot sure I understant what you mean by this. I think Chris made clear that\nthe AIX issue comes from the handler being expected to do the wait() calls,\nso the only way to \"solve\" it would be to move the `check_dead_children()`\nlogic back to the handler (which will require a lot more changes), using\nsigaction() seems like a simpler solution.\n\n> > +\n> > +\tif (poll_pipe[1] >= 0)\n> > +\t\twrite(poll_pipe[1], &signo, 1);\n> \n> write() might fail so we should save errno around it. Conventionally one\n> would re-try on EINTR as well though in this case the most likely reason for\n> that is another child exiting which means the pipe would be written to\n> anyway.\n\nEINTR shouldn't be possible here from SIGCHLD, because that signal is\nblocked here, I wrote it this way because it was only meant to be used as\na fallback to the EINTR being triggered in poll() and so it wouldn't matter\nif it was lost (for example because of a SIGPIPE).\n\nOnce we move to sigaction, SIGPIPE could be added to the mask for this\nhandler, but that code can't be implemented on the current base.\n\n> > }\n> > static int set_reuse_addr(int sockfd)\n> > @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> > static int service_loop(struct socketlist *socklist)\n> > {\n> > \tstruct pollfd *pfd;\n> > +\tunsigned long nfds = 1 + socklist->nr;\n> > +\n> > +\tALLOC_ARRAY(pfd, nfds);\n> > +\tif (!pipe(poll_pipe)) {\n> \n> If we cannot create a pipe here then things have gone pretty badly wrong and\n> I think it is unlikely we're going to be able to accept incoming connections\n> so it would be best to die().\n\nTrue, but since this is \"optional\", there is no harm on letting this continue\neven in failure.\n\n> > +\t\t\tint flags;\n> > +\n> > +\t\t\tflags = fcntl(poll_pipe[i], F_GETFD, 0);\n> > +\t\t\tif (flags >= 0)\n> > +\t\t\t\tfcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);\n> I think we should probably close the pipes if we do not set FD_CLOEXEC.\n\nWhy?, worst case is that we leak a pipe to the children, that wouldn't know\nof it and woule be able to work either way.\n\nCarlo\n"},{"id":"520925","messageId":"xmqqbjq55kf0.fsf@gitster.g","threadId":"63688","inReplyTo":"59087d2d-6034-44d4-9fa0-c51d4bd60683@gmail.com","subject":"Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-30T15:16:51Z","receivedAt":"2025-06-30T15:16:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> An obvious disadvantage (at least of this implementation), is that it\n>> actually doubles the number of events that need to be handled for each\n>> children process on most cases (ex: when `poll()` gets interrupted)\n>> I suspect that if fixing that last race condition is so important\n>> with\n>> the current foundation, it might be better to reintroduce some sort of\n>> timeout to poll(), so that they will be cleared periodically.\n>> I had a prototype (only the bare minimum) that I thought was more\n>> efficient and that would instead remove completely the need for a\n>> signal handler which I would post (only for RFC) later.\n>\n> I'm not sure injecting an fd into each child process is a good direction.\n\nYeah, I thought that the \"self pipe trick\" (in the title) refers to\nthe technique to have a single pipe for the daemon to talk to\nitself, so that it can write(2) into the pipe in non-blocking way\nupon signal and expect its select/poll to be able to notice, where\nit can reap the completed children, so having a pipe per child was a\nsurprise to me.\n"},{"id":"521048","messageId":"de0d02a8-37fd-4a24-9042-972398d72adb@gmail.com","threadId":"63688","inReplyTo":"zy6xtfqnncs3kuipgvdb7jiu7ynodbf7mld4r2ojy3jwkkthm6@jtnzecikxda2","subject":"Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-01T13:38:44Z","receivedAt":"2025-07-01T13:38:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 28/06/2025 07:19, Carlo Marcelo Arenas Belón wrote:\n> On Fri, Jun 27, 2025 at 09:38:36AM -0800, Phillip Wood wrote:\n>>\n>> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:\n>>>\n>>> diff --git a/daemon.c b/daemon.c\n>>> index d1be61fd57..d3b9421575 100644\n>>> --- a/daemon.c\n>>> +++ b/daemon.c\n>>> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n>>> \t\tadd_child(&cld, addr, addrlen);\n>>> }\n>>> -static void child_handler(int signo UNUSED)\n>>> +int poll_pipe[2] = { -1, -1 };\n>> Maybe call this signal_pipe? I'm not sure what poll_pipe means.\n>>\n>>> +\n>>> +static void child_handler(int signo)\n>>> {\n>>> \t/*\n>>> -\t * Otherwise empty handler because systemcalls will get interrupted\n>>> -\t * upon signal receipt\n>>> \t * SysV needs the handler to be rearmed\n>>> \t */\n>>> \tsignal(SIGCHLD, child_handler);\n>>\n>> I think from Chris' email that it is conventional to do this at the end of\n>> the handler.\n> \n> It really depends on which flags the signal was expected to use. For maximum\n> portability it would seem better to have it at the beginning to minimize the\n> inherent race condition from when SA_RESETHAND is on effect (which was more\n> of a SysV signal() tradition).\n> \n> Will move it to the end, regardless, as it won't be needed once the call is\n> setup using sigaction().\n> \n>> As I said above we could add an additional commit that moves\n>> this into the event loop to fix the infinite recursion on AIX.\n> \n> Not sure I understant what you mean by this.\n\nDelete the call to signal() from child_handler() and move it to the \nevent loop so it is called just before poll()\n\n> I think Chris made clear that\n> the AIX issue comes from the handler being expected to do the wait() calls,\n\nThe issue is calling signal() without calling wait() first. By removing \nsignal() from the signal handler there can be no recursion.\n\n>>> +\n>>> +\tif (poll_pipe[1] >= 0)\n>>> +\t\twrite(poll_pipe[1], &signo, 1);\n>>\n>> write() might fail so we should save errno around it. Conventionally one\n>> would re-try on EINTR as well though in this case the most likely reason for\n>> that is another child exiting which means the pipe would be written to\n>> anyway.\n> \n> EINTR shouldn't be possible here from SIGCHLD, because that signal is\n> blocked here, \n\nHmm I'm not sure that is guaranteed by POSIX though it is true with BSD \nor SysV semantics I think. From a maintainability point of view we could \nsee EINTR here if we added a handler for another signal in the future so \nI think we should make sure it is handled correctly.\n\nI wrote it this way because it was only meant to be used as\n> a fallback to the EINTR being triggered in poll() and so it wouldn't matter\n> if it was lost (for example because of a SIGPIPE).\n\nI'm not convinced about making it a fallback - to me that just means \nwe'll never notice if it is broken.\n\n> Once we move to sigaction, SIGPIPE could be added to the mask for this\n> handler, but that code can't be implemented on the current base.\n\nI'm not sure I understand. The signal mask supplied to sigaction just \nblocks those signals while the handler is running, they're still \ndelivered afterwards so we'd still receive SIGPIPE if the read end was \nclosed. We could ignore SIGPIPE so that we get an error from write() \ninstead but I'm not sure it is worth worrying about as if the read end \nof the pipe has been closed we have a bug.\n\n>>> }\n>>> static int set_reuse_addr(int sockfd)\n>>> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n>>> static int service_loop(struct socketlist *socklist)\n>>> {\n>>> \tstruct pollfd *pfd;\n>>> +\tunsigned long nfds = 1 + socklist->nr;\n>>> +\n>>> +\tALLOC_ARRAY(pfd, nfds);\n>>> +\tif (!pipe(poll_pipe)) {\n>>\n>> If we cannot create a pipe here then things have gone pretty badly wrong and\n>> I think it is unlikely we're going to be able to accept incoming connections\n>> so it would be best to die().\n> \n> True, but since this is \"optional\", there is no harm on letting this continue\n> even in failure.\n\nExcept that we'd never know if this code was broken. We don't expect it \nto fail so why make it optional?\n\nThanks\n\nPhillip\n\n"},{"id":"521477","messageId":"xmqqpleb3az6.fsf@gitster.g","threadId":"63688","inReplyTo":"087d437f-3163-4c63-b6f5-e5d726016359@gmail.com","subject":"Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T22:14:05Z","receivedAt":"2025-07-07T22:14:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> The counter argument to this is that it is masking bugs that happen on\n> platforms without SA_RESTART.\n\nAs long as we can make the thing work correctly without SA_RESTART\neven on platforms that do support SA_RESTART, I tend to agree with\nthat position.\n\nThanks.\n"},{"id":"521673","messageId":"b1027221-3e17-40d2-b293-4b1625fa095d@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/4] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-09T14:12:43Z","receivedAt":"2025-07-09T14:12:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> This series addresses and ambiguity that is at least visible in OpenBSD,\n> where zombie proceses would only be cleared after a new connection is\n> received.\n> \n> The underlying problem is that when this code was originally introduced,\n> SA_RESTART was not widely implemented, and the signal() call usually\n> implemented SysV like semantics, at least until it started being\n> reimplemented by calling sigaction() internally.\n\nI'm all in favor of using sigaction() but I think the SA_RESTART parts \nof this series are an unnecessary complication that has the potential to \nhide bugs as we support platforms without SA_RESTART. Your EINTR patches \nhave all been about efficiency rather than correctness so I think my \ninitial assessment that the existing code handles EINTR correctly was \nprobably true. As I said in my comments on patch 3 I think we can \nhappily drop patches 1 and 4.\n\nThanks\n\nPhillip\n\n\n> Changes since v2\n> \n> * Add a new patch 2 that modifies windows' sigaction so there is no more\n> need for a fallback\n> * Hopefully no more silly mistakes and a variable that finally makes sense\n> \n> Changes since v1\n> \n> * Almost all references to siginterrupt has been removed and a better named\n> variable used instead\n> * Changes had been abstracted to minimize ifdefs and their introduction\n> staged more naturally\n> \n> Carlo Marcelo Arenas Belón (4):\n> compat/posix.h: track SA_RESTART fallback\n> compat/mingw: allow sigaction(SIGCHLD)\n> daemon: use sigaction() to install child_handler()\n> daemon: explicitly allow EINTR during poll()\n> \n> Makefile | 5 +++++\n> compat/mingw-posix.h | 2 +-\n> compat/mingw.c | 4 +++-\n> compat/posix.h | 8 ++++++++\n> config.mak.uname | 7 ++++---\n> configure.ac | 16 ++++++++++++++++\n> daemon.c | 33 ++++++++++++++++++++++++++++-----\n> meson.build | 4 ++++\n> 8 files changed, 69 insertions(+), 10 deletions(-)\n> \n> \n> base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v3\n> Pull-Request: https://github.com/git/git/pull/2002\n> \n> Range-diff vs v2:\n> \n> 1: e82b7425bbc ! 1: ae1ca6bb2b2 compat/posix.h: track SA_RESTART fallback\n> @@ Makefile: include shared.mak\n> # when attempting to read from an fopen'ed directory (or even to fopen\n> # it at all).\n> #\n> -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> -+# prefer to use ANSI C signal() over POSIX sigaction()\n> ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n> +#\n> # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR\n> # when a signal is received (as opposed to restarting).\n> @@ Makefile: ifdef FREAD_READS_DIRECTORIES\n> \tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n> \tCOMPAT_OBJS += compat/fopen.o\n> endif\n> -+ifdef USE_NON_POSIX_SIGNAL\n> -+\tCOMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL\n> ++ifdef NO_RESTARTABLE_SIGNALS\n> ++\tCOMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS\n> +endif\n> ifdef OPEN_RETURNS_EINTR\n> \tCOMPAT_CFLAGS += -DOPEN_RETURNS_EINTR\n> @@ compat/posix.h: char *gitdirname(char *);\n> + * not on some systems (e.g. NonStop, QNX).\n> + */\n> +#ifndef SA_RESTART\n> -+# define SA_RESTART 0\t/* disabled for sigaction() */\n> ++# define SA_RESTART 0 /* disabled for sigaction() */\n> +#endif\n> +\n> typedef uintmax_t timestamp_t;\n> @@ config.mak.uname: ifeq ($(uname_S),Windows)\n> \tNO_STRTOUMAX = YesPlease\n> \tNO_MKDTEMP = YesPlease\n> \tNO_INTTYPES_H = YesPlease\n> -+\tUSE_NON_POSIX_SIGNAL = YesPlease\n> ++\tNO_RESTARTABLE_SIGNALS = YesPlease\n> \tCSPRNG_METHOD = rtlgenrandom\n> \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n> \t# so we don't need this:\n> @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)\n> \tNO_MMAP = YesPlease\n> \tNO_POLL = YesPlease\n> \tNO_INTPTR_T = UnfortunatelyYes\n> -+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n> ++\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n> \tCSPRNG_METHOD = openssl\n> \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n> \tSHELL_PATH = /usr/coreutils/bin/bash\n> @@ config.mak.uname: ifeq ($(uname_S),MINGW)\n> \tNEEDS_LIBICONV = YesPlease\n> \tNO_STRTOUMAX = YesPlease\n> \tNO_MKDTEMP = YesPlease\n> -+\tUSE_NON_POSIX_SIGNAL = YesPlease\n> ++\tNO_RESTARTABLE_SIGNALS = YesPlease\n> \tNO_SVN_TESTS = YesPlease\n> \n> \t# The builtin FSMonitor requires Named Pipes and Threads on Windows.\n> @@ config.mak.uname: ifeq ($(uname_S),QNX)\n> \tNO_PTHREADS = YesPlease\n> \tNO_STRCASESTR = YesPlease\n> \tNO_STRLCPY = YesPlease\n> -+\tUSE_NON_POSIX_SIGNAL = UnfortunatelyYes\n> ++\tNO_RESTARTABLE_SIGNALS = UnfortunatelyYes\n> endif\n> \n> ## configure.ac ##\n> @@ configure.ac: fi\n> GIT_CONF_SUBST([ICONV_OMITS_BOM])\n> fi\n> \n> -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or\n> -+# prefer using ANSI C signal() over POSIX sigaction()\n> ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART\n> +\n> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [\n> +\tAC_COMPILE_IFELSE(\n> +\t\t[AC_LANG_PROGRAM([#include ], [[\n> -+\t\t#ifdef SA_RESTART\n> -+\t\t#endif\n> -+\t\tsiginterrupt(SIGCHLD, 1)\n> -+\t\t]])],[ac_cv_siginterrupt=yes],[\n> ++\t\t\t#ifdef SA_RESTART\n> ++\t\t\trestartable signals supported\n> ++\t\t\t#endif\n> ++\t\t]])],[\n> +\t\t\tac_cv_siginterrupt=no\n> -+\t\t\tUSE_NON_POSIX_SIGNAL=UnfortunatelyYes\n> -+\t\t]\n> ++\t\t\tNO_RESTARTABLE_SIGNALS=UnfortunatelyYes\n> ++\t\t], [ac_cv_siginterrupt=yes]\n> +\t)\n> +])\n> -+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])\n> ++GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])\n> +\n> ## Checks for typedefs, structures, and compiler characteristics.\n> AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])\n> @@ meson.build: else\n> endif\n> \n> +if compiler.get_define('SA_RESTART', prefix: '#include ') == ''\n> -+ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'\n> ++ libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'\n> +endif\n> +\n> if not compiler.has_header('sys/select.h')\n> -: ----------- > 2: 3f63479119f compat/mingw: allow sigaction(SIGCHLD)\n> 2: 05d945aa1e5 ! 3: c66bda461f4 daemon: use sigaction() to install child_handler()\n> @@ Commit message\n> In a future change, the flags used for processing SIGCHLD will need to\n> be updated, which is only possible by using sigaction().\n> \n> - Factor out the call to set the signal handler and use sigaction instead\n> - of signal for the systems that allow that, which has the added benefit\n> - of using BSD semantics reliably and therefore not needing the rearming\n> - call.\n> + Replace signal() with an equivalent invocation of sigaction(), which\n> + has the added benefit of using BSD semantics reliably and therefore\n> + not needing the rearming call in the signal handler.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> \n> ## daemon.c ##\n> @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> - \t\tadd_child(&cld, addr, addrlen);\n> - }\n> -\n> --static void child_handler(int signo UNUSED)\n> -+static void child_handler(int signo MAYBE_UNUSED)\n> + static void child_handler(int signo UNUSED)\n> {\n> \t/*\n> -\t * Otherwise empty handler because systemcalls will get interrupted\n> @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr\n> +\t * upon signal receipt.\n> \t */\n> -\tsignal(SIGCHLD, child_handler);\n> -+#ifdef USE_NON_POSIX_SIGNAL\n> -+\t/*\n> -+\t * SysV needs the handler to be rearmed, but this is known\n> -+\t * to trigger infinite recursion crashes at least in AIX.\n> -+\t */\n> -+\tsignal(signo, child_handler);\n> -+#endif\n> }\n> \n> static int set_reuse_addr(int sockfd)\n> @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> - \t}\n> - }\n> \n> -+#ifndef USE_NON_POSIX_SIGNAL\n> -+\n> -+static void set_signal_handler(struct sigaction *psa)\n> -+{\n> -+\tsigemptyset(&psa->sa_mask);\n> -+\tpsa->sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> -+\tpsa->sa_handler = child_handler;\n> -+\tsigaction(SIGCHLD, psa, NULL);\n> -+}\n> -+\n> -+#else\n> -+\n> -+static void set_signal_handler(struct sigaction *psa UNUSED)\n> -+{\n> -+\tsignal(SIGCHLD, child_handler);\n> -+}\n> -+\n> static int service_loop(struct socketlist *socklist)\n> {\n> +\tstruct sigaction sa;\n> @@ daemon.c: static int service_loop(struct socketlist *socklist)\n> \t}\n> \n> -\tsignal(SIGCHLD, child_handler);\n> -+\tset_signal_handler(&sa);\n> ++\tsigemptyset(&sa.sa_mask);\n> ++\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n> ++\tsa.sa_handler = child_handler;\n> ++\tsigaction(SIGCHLD, &sa, NULL);\n> \n> \tfor (;;) {\n> \t\tcheck_dead_children();\n> 3: b737e0389df ! 4: 851d663be0b daemon: explicitly allow EINTR during poll()\n> @@ Commit message\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> \n> ## daemon.c ##\n> -@@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n> - \tsigaction(SIGCHLD, psa, NULL);\n> +@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n> + \t}\n> }\n> \n> ++#ifndef NO_RESTARTABLE_SIGNALS\n> ++\n> +static void set_sa_restart(struct sigaction *psa, int enable)\n> +{\n> +\tif (enable)\n> @@ daemon.c: static void set_signal_handler(struct sigaction *psa)\n> +\tsigaction(SIGCHLD, psa, NULL);\n> +}\n> +\n> - #else\n> -\n> - static void set_signal_handler(struct sigaction *psa UNUSED)\n> -@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)\n> - \tsignal(SIGCHLD, child_handler);\n> - }\n> -\n> ++#else\n> ++\n> +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)\n> +{\n> +}\n> \n\n"},{"id":"521674","messageId":"0931e1f2-6254-474f-be91-664cec9745f5@gmail.com","threadId":"63688","inReplyTo":"p6xegxqqq4wzi6gnokypy3k5auxk3d2wxmj4pj45ugfomace3q@y5q3e2al42oj","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-09T14:13:06Z","receivedAt":"2025-07-09T14:13:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote:\n> On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote:\n>> On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:\n>>> On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:\n>>>> On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n>>>>> On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n>>>>>> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n>>>>>>> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n>>>>>>>\n>>>>>>> A future change will start using sigaction to setup a SIGCHLD signal\n>>>>>>> handler.\n>>>>>>>\n>>>>>>> The current code uses signal() which returns SIG_ERR (but doesn't\n>>>>>>> seem to set errno) so instruct sigaction() to do the same.\n>>>>>>\n>>>>>> Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n>>>>>> match?\n>>>>>\n>>>>> By \"match\", I mean that in both cases we will get an error return value\n>>>>> and errno won't be set to EINVAL (which is what POSIX requires)\n>>>>>\n>>>>> In our codebase since we ignore the return code anyway, it wouldn't make\n>>>>> a difference, either way.\n>>>>>\n>>>>> signal() returns a pointer, and sigaction() returns and int,\n>>>>\n>>>> Oh right, I'd forgotten they have different return types. I think we should\n>>>> probably be setting errno = EINVAL before returning -1 to match what this\n>>>> function does with other signals it does not support - just because our\n>>>> current callers ignore the return value doesn't mean that future callers\n>>>> will and they might want check errno if they see the function fail.\n>>>\n>>> I agree, and indeed had to triple check and change my implementation after I\n>>> confirmed that signal(SIGCHLD) does not change errno on Windows (not our\n>>> version, neither of the windows libc or mingw, even if it is documented[1] to\n>>> do so.\n>>>\n>>> It might be because the signal number itself is bogus (there is none for\n>>> SIGCHLD in their headers, and git uses their own numbers in compat), but\n>>> either way, I would rather be consistent with signal() at least originally.\n>>\n>> I'm not sure I understand - don't we want the sigaction() wrapper to behave\n>> like sigaction() would?\n> \n> for at least the first iteration, I would rather have sigaction() behave\n> like signal(), so that the change doesn't introduce any regressions.\n\nWhat regressions are you worried about? We're talking about changing a \nsingle call from signal() to sigaction(). I'd have thought we're far \nmore likely to introduce regressions if we change the behavior of the \nwindows implementation of sigaction() to behave like signal() as that \nintroduces more variation between different platforms.\n\n> eventually, sigaction() should behave like any other sigaction(), but to\n> do so, I suspect the windows emulation might need to change their SIGCHLD\n> to match.\n> \n> just confirmed with MSVC that if I use 20 instead of 17, errno gets updated\n> just like the documentation says it should.\n\nOh not so setting errno as you want to do would not actually match \nsignal() on Windows in that case?\n\nThanks\n\nPhillip\n\n> Carlo\n> \n> PS. Maybe we should get dscho involved?\n>>>\n>>> [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal\n\n"},{"id":"521705","messageId":"3hrbpiapamvfiuilebjcbcruppz3vukf6mndg62j6gvko2jfs4@ll24s25shcgv","threadId":"63688","inReplyTo":"0931e1f2-6254-474f-be91-664cec9745f5@gmail.com","subject":"Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-07-09T16:36:04Z","receivedAt":"2025-07-09T16:36:07Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Jul 09, 2025 at 03:13:06PM -0800, Phillip Wood wrote:\n> On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote:\n> > On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com wrote:\n> > > On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:\n> > > > On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:\n> > > > > On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:\n> > > > > > On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:\n> > > > > > > On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> > > > > > > > From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n> > > > > > > > \n> > > > > > > > A future change will start using sigaction to setup a SIGCHLD signal\n> > > > > > > > handler.\n> > > > > > > > \n> > > > > > > > The current code uses signal() which returns SIG_ERR (but doesn't\n> > > > > > > > seem to set errno) so instruct sigaction() to do the same.\n> > > > > > > \n> > > > > > > Why are we returning -1 below instead of SIG_ERR if we want the behavior to\n> > > > > > > match?\n> > > > > > \n> > > > > > By \"match\", I mean that in both cases we will get an error return value\n> > > > > > and errno won't be set to EINVAL (which is what POSIX requires)\n> > > > > > \n> > > > > > In our codebase since we ignore the return code anyway, it wouldn't make\n> > > > > > a difference, either way.\n> > > > > > \n> > > > > > signal() returns a pointer, and sigaction() returns and int,\n> > > > > \n> > > > > Oh right, I'd forgotten they have different return types. I think we should\n> > > > > probably be setting errno = EINVAL before returning -1 to match what this\n> > > > > function does with other signals it does not support - just because our\n> > > > > current callers ignore the return value doesn't mean that future callers\n> > > > > will and they might want check errno if they see the function fail.\n> > > > \n> > > > I agree, and indeed had to triple check and change my implementation after I\n> > > > confirmed that signal(SIGCHLD) does not change errno on Windows (not our\n> > > > version, neither of the windows libc or mingw, even if it is documented[1] to\n> > > > do so.\n> > > > \n> > > > It might be because the signal number itself is bogus (there is none for\n> > > > SIGCHLD in their headers, and git uses their own numbers in compat), but\n> > > > either way, I would rather be consistent with signal() at least originally.\n> > > \n> > > I'm not sure I understand - don't we want the sigaction() wrapper to behave\n> > > like sigaction() would?\n> > \n> > for at least the first iteration, I would rather have sigaction() behave\n> > like signal(), so that the change doesn't introduce any regressions.\n> \n> What regressions are you worried about?\n\nAny code that might be surprised by a non 0 errno, even if I agree it unlikely\nto be an issue.\n\nFWIW, the Windows compat layer is not very strict on setting errno, and since\na call to the CRT (at least with the current codepaths) doesn't either\n\n> We're talking about changing a\n> single call from signal() to sigaction(). I'd have thought we're far more\n> likely to introduce regressions if we change the behavior of the windows\n> implementation of sigaction() to behave like signal() as that introduces\n> more variation between different platforms.\n\nDon't get me wrong, I also want sigaction() to behave the same regardless of\nplatform, but I would rather do that in an independent change.\n\nIndeed that is why I was asking for the possibility to change the SIGCHLD\ndefinition, so that signal() starts also behaving as documented and we can\nfix sigaction(SIGCHLD) to match.\n\n> > eventually, sigaction() should behave like any other sigaction(), but to\n> > do so, I suspect the windows emulation might need to change their SIGCHLD\n> > to match.\n> > \n> > just confirmed with MSVC that if I use 20 instead of 17, errno gets updated\n> > just like the documentation says it should.\n> \n> Oh not so setting errno as you want to do would not actually match signal()\n> on Windows in that case?\n\nI explained my thinking before already. I am not a Windows expert and indeed I\nam surprised that signal() in the CRT behaves this way, but I suspect it might\nbe because of the same reasons why raise() triggers debugger breaks that are\nexplicitally called for and avoided in the mingw compat code.\n\nEitherway my preference is for this to be handled indpendently and preferably\nnot as a prerequisite of this change.\n\n> > Carlo\n> > \n> > PS. Maybe we should get dscho involved?\n> > > > \n> > > > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal\n"},{"id":"521708","messageId":"fqqx2jvtwlsqghgjxrp5e4q2ti5mjbwg52ttcbaabmtrlacrpw@t4pbsdiebeft","threadId":"63688","inReplyTo":"b1027221-3e17-40d2-b293-4b1625fa095d@gmail.com","subject":"Re: [PATCH v3 0/4] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-07-09T17:04:55Z","receivedAt":"2025-07-09T17:04:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Jul 09, 2025 at 03:12:43PM -0800, Phillip Wood wrote:\n> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:\n> > This series addresses and ambiguity that is at least visible in OpenBSD,\n> > where zombie proceses would only be cleared after a new connection is\n> > received.\n> > \n> > The underlying problem is that when this code was originally introduced,\n> > SA_RESTART was not widely implemented, and the signal() call usually\n> > implemented SysV like semantics, at least until it started being\n> > reimplemented by calling sigaction() internally.\n> \n> I'm all in favor of using sigaction() but I think the SA_RESTART parts of\n> this series are an unnecessary complication that has the potential to hide\n> bugs as we support platforms without SA_RESTART.\n\nTrue, but those platforms (except for Windows, which is otherwise not that\nrelevant as it doesn't fail system calls with EINTR anyway) don't have that\nmany users and are therefore less likely to uncover any possible issues with\ntheir use cases.\n\nI know patch 4 looks silly, by enabling SA_RESTART just to disable it around\npoll(), but it addresses the root cause of the problem stated originally,\nwhich is that we are very likely to have SA_RESTART enabled on SIGCHLD and\nrelying on the system to excempt poll() from it.\n\nCarlo\n"},{"id":"521771","messageId":"pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v3.git.git.1750927988.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-10T19:45:41Z","receivedAt":"2025-07-10T19:45:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series addresses and ambiguity that is at least visible in OpenBSD,\nwhere zombie proceses would only be cleared after a new connection is\nreceived.\n\nThe underlying problem is that when this code was originally introduced,\nSA_RESTART was not widely implemented, and the signal() call usually\nimplemented SysV like semantics, at least until it started being\nreimplemented by calling sigaction() internally.\n\nChanges since v3\n\n * Remove patches 1 and 4 as suggested by Phillip, and setup the signal\n without SA_RESTART instead.\n\nChanges since v2\n\n * Add a new patch 2 that modifies windows' sigaction so there is no more\n need for a fallback\n * Hopefully no more silly mistakes and a variable that finally makes sense\n\nChanges since v1\n\n * Almost all references to siginterrupt has been removed and a better named\n variable used instead\n * Changes had been abstracted to minimize ifdefs and their introduction\n staged more naturally\n\nCarlo Marcelo Arenas Belón (2):\n compat/mingw: allow sigaction(SIGCHLD)\n daemon: use sigaction() to install child_handler()\n\n compat/mingw-posix.h | 1 +\n compat/mingw.c | 4 +++-\n daemon.c | 12 +++++++-----\n 3 files changed, 11 insertions(+), 6 deletions(-)\n\n\nbase-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v4\nPull-Request: https://github.com/git/git/pull/2002\n\nRange-diff vs v3:\n\n 1: ae1ca6bb2b2 < -: ----------- compat/posix.h: track SA_RESTART fallback\n 2: 3f63479119f ! 1: f21e8ff5c9d compat/mingw: allow sigaction(SIGCHLD)\n @@ Commit message\n A future change will start using sigaction to setup a SIGCHLD signal\n handler.\n \n - The current code uses signal() which returns SIG_ERR (but doesn't\n + The current code uses signal(), which returns SIG_ERR (but doesn't\n seem to set errno) so instruct sigaction() to do the same.\n \n + A new SA flag will be needed, so copy the one from Cygwinr; note that\n + the sigacgtion() implementation that is provided won't use it, so\n + its value is otherwise irrelevant.\n +\n Signed-off-by: Carlo Marcelo Arenas Belón \n \n ## compat/mingw-posix.h ##\n @@ compat/mingw-posix.h: struct sigaction {\n - \tsig_handler_t sa_handler;\n \tunsigned sa_flags;\n };\n + #define SA_RESTART 0\n +#define SA_NOCLDSTOP 1\n \n struct itimerval {\n 3: c66bda461f4 ! 2: 81c41b43e60 daemon: use sigaction() to install child_handler()\n @@ Metadata\n ## Commit message ##\n daemon: use sigaction() to install child_handler()\n \n - In a future change, the flags used for processing SIGCHLD will need to\n - be updated, which is only possible by using sigaction().\n + Replace signal() with an equivalent invocation of sigaction(), but\n + make sure to NOT set SA_RESTART so the original code that expects\n + to be interrupted when children complete still works as designed.\n \n - Replace signal() with an equivalent invocation of sigaction(), which\n - has the added benefit of using BSD semantics reliably and therefore\n - not needing the rearming call in the signal handler.\n + This change has the added benefit of using BSD signal semantics reliably\n + and therefore not needing the rearming call in the signal handler.\n \n Signed-off-by: Carlo Marcelo Arenas Belón \n \n @@ daemon.c: static int service_loop(struct socketlist *socklist)\n \n -\tsignal(SIGCHLD, child_handler);\n +\tsigemptyset(&sa.sa_mask);\n -+\tsa.sa_flags = SA_NOCLDSTOP | SA_RESTART;\n ++\tsa.sa_flags = SA_NOCLDSTOP;\n +\tsa.sa_handler = child_handler;\n +\tsigaction(SIGCHLD, &sa, NULL);\n \n 4: 851d663be0b < -: ----------- daemon: explicitly allow EINTR during poll()\n\n-- \ngitgitgadget\n"},{"id":"521772","messageId":"f21e8ff5c9df0989ce09b3d9a50c0dc81af18837.1752176743.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-10T19:45:42Z","receivedAt":"2025-07-10T19:45:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nA future change will start using sigaction to setup a SIGCHLD signal\nhandler.\n\nThe current code uses signal(), which returns SIG_ERR (but doesn't\nseem to set errno) so instruct sigaction() to do the same.\n\nA new SA flag will be needed, so copy the one from Cygwinr; note that\nthe sigacgtion() implementation that is provided won't use it, so\nits value is otherwise irrelevant.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n compat/mingw-posix.h | 1 +\n compat/mingw.c | 4 +++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex 88e0cf92924b..631a20868489 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -96,6 +96,7 @@ struct sigaction {\n \tunsigned sa_flags;\n };\n #define SA_RESTART 0\n+#define SA_NOCLDSTOP 1\n \n struct itimerval {\n \tstruct timeval it_value, it_interval;\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 8a9972a1ca19..5d69ae32f4b9 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n \n int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n {\n-\tif (sig != SIGALRM)\n+\tif (sig == SIGCHLD)\n+\t\treturn -1;\n+\telse if (sig != SIGALRM)\n \t\treturn errno = EINVAL,\n \t\t\terror(\"sigaction only implemented for SIGALRM\");\n \tif (out)\n-- \ngitgitgadget\n\n"},{"id":"521773","messageId":"81c41b43e60ca504be4c5267b335f4585f28941d.1752176743.git.gitgitgadget@gmail.com","threadId":"63688","inReplyTo":"pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] daemon: use sigaction() to install child_handler()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-10T19:45:43Z","receivedAt":"2025-07-10T19:45:49Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nReplace signal() with an equivalent invocation of sigaction(), but\nmake sure to NOT set SA_RESTART so the original code that expects\nto be interrupted when children complete still works as designed.\n\nThis change has the added benefit of using BSD signal semantics reliably\nand therefore not needing the rearming call in the signal handler.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n daemon.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d1be61fd5789..28fc19479038 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n static void child_handler(int signo UNUSED)\n {\n \t/*\n-\t * Otherwise empty handler because systemcalls will get interrupted\n-\t * upon signal receipt\n-\t * SysV needs the handler to be rearmed\n+\t * Otherwise empty handler because systemcalls should get interrupted\n+\t * upon signal receipt.\n \t */\n-\tsignal(SIGCHLD, child_handler);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s\n \n static int service_loop(struct socketlist *socklist)\n {\n+\tstruct sigaction sa;\n \tstruct pollfd *pfd;\n \n \tCALLOC_ARRAY(pfd, socklist->nr);\n@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_NOCLDSTOP;\n+\tsa.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sa, NULL);\n \n \tfor (;;) {\n \t\tcheck_dead_children();\n-- \ngitgitgadget\n"},{"id":"521778","messageId":"xmqqfrf368lz.fsf@gitster.g","threadId":"63688","inReplyTo":"pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-10T21:26:00Z","receivedAt":"2025-07-10T21:26:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue with the following typo-fixes.\n\nI'd appreciate an explicit Ack, even if this version is acceptable\nfor all those who have helped polish this topic.\n\nI understand that the self-pipe to wake ourselves up is left outside\nthis topic on purpose, which I agree with.\n\nThanks, Carlo, for putting this together from weeks' long\ndiscussions, and thanks Phillip for pushing for simpler and smaller\nset of changes.\n\nWill queue.\n\n\n1: 30773a76ce ! 1: ef03aa432a compat/mingw: allow sigaction(SIGCHLD)\n @@ Commit message\n The current code uses signal(), which returns SIG_ERR (but doesn't\n seem to set errno) so instruct sigaction() to do the same.\n \n - A new SA flag will be needed, so copy the one from Cygwinr; note that\n - the sigacgtion() implementation that is provided won't use it, so\n + A new SA flag will be needed, so copy the one from Cygwin; note that\n + the sigaction() implementation that is provided won't use it, so\n its value is otherwise irrelevant.\n \n Signed-off-by: Carlo Marcelo Arenas Belón \n2: 7a33d7a646 = 2: d83e1eef3b daemon: use sigaction() to install child_handler()\n"},{"id":"521780","messageId":"CAPig+cScQN7O-cs+-9X+RpjQqJUstD10LgNYZqSHFwyAdRL+Cg@mail.gmail.com","threadId":"63688","inReplyTo":"f21e8ff5c9df0989ce09b3d9a50c0dc81af18837.1752176743.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] compat/mingw: allow sigaction(SIGCHLD)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-10T21:38:13Z","receivedAt":"2025-07-10T21:38:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 10, 2025 at 3:45 PM Carlo Marcelo Arenas Belón via\nGitGitGadget wrote:\n> A future change will start using sigaction to setup a SIGCHLD signal\n> handler.\n>\n> The current code uses signal(), which returns SIG_ERR (but doesn't\n> seem to set errno) so instruct sigaction() to do the same.\n>\n> A new SA flag will be needed, so copy the one from Cygwinr; note that\n> the sigacgtion() implementation that is provided won't use it, so\n> its value is otherwise irrelevant.\n\ns/Cygwinr/Cygwin/\ns/sigacgtion/sigaction/\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n"},{"id":"521797","messageId":"CAPUEsphPrP+EGgwX2od-ymcqrkehePX0kNavJRv08ahfesnb+w@mail.gmail.com","threadId":"63688","inReplyTo":"xmqqfrf368lz.fsf@gitster.g","subject":"Re: [PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2025-07-10T23:18:58Z","receivedAt":"2025-07-10T23:19:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Thanks for the typo fixes\n\nCarlo\n"},{"id":"521816","messageId":"313e3b1a-a095-41ec-adb9-fc500589b979@gmail.com","threadId":"63688","inReplyTo":"xmqqfrf368lz.fsf@gitster.g","subject":"Re: [PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-11T13:14:42Z","receivedAt":"2025-07-11T13:14:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/07/2025 22:26, Junio C Hamano wrote:\n> Thanks, will queue with the following typo-fixes.\n> \n> I'd appreciate an explicit Ack, even if this version is acceptable\n> for all those who have helped polish this topic.\n\nPatch 2 looks good, using sigaction() rather than signal() should reduce \nthe variation in behavior across our unix platforms. I'd be much happier \nif we set errno on SIGCHLD in patch 1 - the argument in [1] that a \nnon-zero errno might break something because signal() did not set it \ndoes not make much sense to me. At the moment it does not matter because \nthere are no callers that check the return value let alone errno but if \na future caller does start checking for errors there going to be \nsurprised by errno not getting set.\n\nThanks\n\nPhillip\n\n[1] <3hrbpiapamvfiuilebjcbcruppz3vukf6mndg62j6gvko2jfs4@ll24s25shcgv>\n\n> I understand that the self-pipe to wake ourselves up is left outside\n> this topic on purpose, which I agree with.\n> \n> Thanks, Carlo, for putting this together from weeks' long\n> discussions, and thanks Phillip for pushing for simpler and smaller\n> set of changes.\n> \n> Will queue.\n> \n> \n> 1: 30773a76ce ! 1: ef03aa432a compat/mingw: allow sigaction(SIGCHLD)\n> @@ Commit message\n> The current code uses signal(), which returns SIG_ERR (but doesn't\n> seem to set errno) so instruct sigaction() to do the same.\n> \n> - A new SA flag will be needed, so copy the one from Cygwinr; note that\n> - the sigacgtion() implementation that is provided won't use it, so\n> + A new SA flag will be needed, so copy the one from Cygwin; note that\n> + the sigaction() implementation that is provided won't use it, so\n> its value is otherwise irrelevant.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> 2: 7a33d7a646 = 2: d83e1eef3b daemon: use sigaction() to install child_handler()\n\n"},{"id":"521924","messageId":"xmqqecuil9sm.fsf@gitster.g","threadId":"63688","inReplyTo":"313e3b1a-a095-41ec-adb9-fc500589b979@gmail.com","subject":"Re: [PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-14T21:52:41Z","receivedAt":"2025-07-14T21:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> much happier if we set errno on SIGCHLD in patch 1 - the argument in\n> [1] that a non-zero errno might break something because signal() did\n> not set it does not make much sense to me.\n\nNot to me either.\n\n> At the moment it does not\n> matter because there are no callers that check the return value let\n> alone errno but if a future caller does start checking for errors\n> there going to be surprised by errno not getting set.\n\nTrue, again.\n\nLet's queue this round and then patch the errno issue up on top\nafter the dust settles. \"might break something\" may then happen,\nat which time it is easier to see where that breakage came from,\nand we can go from there.\n\nThanks, all.\n\n"},{"id":"521954","messageId":"0f884a77-f8a8-4c0c-9a0c-dcd8d514c0ed@gmail.com","threadId":"63688","inReplyTo":"xmqqecuil9sm.fsf@gitster.g","subject":"Re: [PATCH v4 0/2] daemon: explicitly allow EINTR during poll()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-15T09:29:47Z","receivedAt":"2025-07-15T09:29:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/07/2025 22:52, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> Let's queue this round and then patch the errno issue up on top\n> after the dust settles. \"might break something\" may then happen,\n> at which time it is easier to see where that breakage came from,\n> and we can go from there.\n\nThat sounds good to me\n\nThanks\n\nPhillip\n"}]}