# [PATCH v2 0/2] replace signal() with sigaction() 6 messages from 2014-05-30 to 2014-05-31. Participants: Jeremiah Mahler, Andreas Schwab, Chris Packham. Thread: https://gitlist.dev/t/36798 ## Jeremiah Mahler, 2014-05-30 20:58 Subject: [PATCH v2 0/2] replace signal() with sigaction() Message-ID: URL: https://gitlist.dev/e/cover.1401482787.git.jmmahler%40gmail.com ``` This is the second revision to the patch set to replace signal(2) with sigaction(2) [1]. As Johannes pointed out [2], replacing signal with sigaction would break MinGW compatibility. The first patch in this series addresses this problem by expanding the faux sigaction function in compat/mingw.c to support signals other than just SIGALRM. Details are in the patch description. The second patch is a proof of concept. It converts signal to sigaction in a case where signal SIGCHILD was used. Previously this would have failed with MinGW since the faux sigaction function only supported SIGALRM. Now it works as expected. I have tested these changes under Linux and under Windows 7 using Msysgit. [1]: http://marc.info/?l=git&m=140125769223552&w=2 [2]: http://marc.info/?l=git&m=140126288325213&w=2 Jeremiah Mahler (2): compat/mingw.c: expand MinGW support for sigaction connect.c: replace signal() with sigaction() compat/mingw.c | 9 +++++---- connect.c | 5 ++++- 2 files changed, 9 insertions(+), 5 deletions(-) -- 2.0.0.2.g1d11d5d ``` ## Jeremiah Mahler, 2014-05-30 20:58 Subject: [PATCH v2 1/2] compat/mingw.c: expand MinGW support for sigaction Message-ID: URL: https://gitlist.dev/e/d5fc7d72b2d51d1b90fb7f238eff120cb4c6d0e7.1401482787.git.jmmahler%40gmail.com In-Reply-To: ``` Due to portability issues across UNIX versions sigaction(2) should be used instead of signal(2). From the signal(2) man page: The behavior of signal() varies across UNIX versions, and has also var‐ ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. Unfortunately MinGW under Windows has limited support for signal and no support for sigaction. And this prevents sigaction from being used across the entire Git project. In compat/mingw.c there is a faux sigaction function but it only supports SIGALARM. Hence the need for continuing to use signal() in other cases. This patch expands the faux sigaction function so that it calls signal in cases other than SIGALRM. Now sigaction can be used across the entire Git project and MinGW will still work with signal as it did before. Signed-off-by: Jeremiah Mahler --- compat/mingw.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index e9892f8..e504cef 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1651,14 +1651,15 @@ int setitimer(int type, struct itimerval *in, struct itimerval *out) int sigaction(int sig, struct sigaction *in, struct sigaction *out) { - if (sig != SIGALRM) - return errno = EINVAL, - error("sigaction only implemented for SIGALRM"); if (out != NULL) return errno = EINVAL, error("sigaction: param 3 != NULL not implemented"); - timer_fn = in->sa_handler; + if (sig == SIGALRM) + timer_fn = in->sa_handler; + else + signal(sig, in->sa_handler); + return 0; } -- 2.0.0.2.g1d11d5d ``` ## Jeremiah Mahler, 2014-05-30 20:58 Subject: [PATCH v2 2/2] connect.c: replace signal() with sigaction() Message-ID: <1d11d5da7ebc3a6e0d4ab64802b601526c19113c.1401482787.git.jmmahler@gmail.com> URL: https://gitlist.dev/e/1d11d5da7ebc3a6e0d4ab64802b601526c19113c.1401482787.git.jmmahler%40gmail.com In-Reply-To: ``` From signal(2) man page: The behavior of signal() varies across UNIX versions, and has also var‐ ied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. Replaced signal() with sigaction() in connect.c Signed-off-by: Jeremiah Mahler --- connect.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/connect.c b/connect.c index a983d06..b2a33c9 100644 --- a/connect.c +++ b/connect.c @@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char *url, enum protocol protocol; const char **arg; struct strbuf cmd = STRBUF_INIT; + struct sigaction sa; /* Without this we cannot rely on waitpid() to tell * what happened to our children. */ - signal(SIGCHLD, SIG_DFL); + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sigaction(SIGCHLD, &sa, 0); protocol = parse_connect_url(url, &hostandport, &path); if (flags & CONNECT_DIAG_URL) { -- 2.0.0.2.g1d11d5d ``` ## Andreas Schwab, 2014-05-30 21:48 Subject: Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction() Message-ID: <87sinr5457.fsf@igel.home> URL: https://gitlist.dev/e/87sinr5457.fsf%40igel.home In-Reply-To: <1d11d5da7ebc3a6e0d4ab64802b601526c19113c.1401482787.git.jmmahler@gmail.com> ``` Jeremiah Mahler writes: > From signal(2) man page: > > The behavior of signal() varies across UNIX versions, and has also var‐ > ied historically across different versions of Linux. Avoid its use: > use sigaction(2) instead. I don't think this matters for SIG_DFL or SIG_IGN. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Chris Packham, 2014-05-31 10:39 Subject: Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction() Message-ID: <5389B16B.6070004@gmail.com> URL: https://gitlist.dev/e/5389B16B.6070004%40gmail.com In-Reply-To: <1d11d5da7ebc3a6e0d4ab64802b601526c19113c.1401482787.git.jmmahler@gmail.com> ``` On 31/05/14 08:58, Jeremiah Mahler wrote: > From signal(2) man page: > > The behavior of signal() varies across UNIX versions, and has also var‐ > ied historically across different versions of Linux. Avoid its use: > use sigaction(2) instead. > > Replaced signal() with sigaction() in connect.c > > Signed-off-by: Jeremiah Mahler > --- > connect.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/connect.c b/connect.c > index a983d06..b2a33c9 100644 > --- a/connect.c > +++ b/connect.c > @@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char *url, > enum protocol protocol; > const char **arg; > struct strbuf cmd = STRBUF_INIT; > + struct sigaction sa; > > /* Without this we cannot rely on waitpid() to tell > * what happened to our children. > */ > - signal(SIGCHLD, SIG_DFL); > + memset(&sa, 0, sizeof(sa)); > + sa.sa_handler = SIG_DFL; > + sigaction(SIGCHLD, &sa, 0); I think this got lost in the wash with v1 but Documentation/CodingGuidelines says to use NULL here instead of 0. > > protocol = parse_connect_url(url, &hostandport, &path); > if (flags & CONNECT_DIAG_URL) { > ``` ## Jeremiah Mahler, 2014-05-31 14:45 Subject: Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction() Message-ID: <20140531144516.GA22508@hudson.localdomain> URL: https://gitlist.dev/e/20140531144516.GA22508%40hudson.localdomain In-Reply-To: <5389B16B.6070004@gmail.com> ``` Chris, On Sat, May 31, 2014 at 10:39:39PM +1200, Chris Packham wrote: > On 31/05/14 08:58, Jeremiah Mahler wrote: > > From signal(2) man page: > > ... > > - signal(SIGCHLD, SIG_DFL); > > + memset(&sa, 0, sizeof(sa)); > > + sa.sa_handler = SIG_DFL; > > + sigaction(SIGCHLD, &sa, 0); > > I think this got lost in the wash with v1 but > Documentation/CodingGuidelines says to use NULL here instead of 0. > Got it. Will be in to the next revision. sigaction(SIGCHLD, &sa, NULL); Thanks, -- Jeremiah Mahler jmmahler@gmail.com http://github.com/jmahler ```