[PATCH v2 0/2] replace signal() with sigaction()

[PATCH v2 0/2] replace signal() with sigaction()

[Jeremiah Mahler]

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

[PATCH v2 1/2] compat/mingw.c: expand MinGW support for sigaction

[Jeremiah Mahler]

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 <jmmahler@gmail.com>
---
 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

[PATCH v2 2/2] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

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 <jmmahler@gmail.com>
---
 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

Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction()

[Andreas Schwab]

Jeremiah Mahler <jmmahler@gmail.com> 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."

Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction()

[Chris Packham]

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 <jmmahler@gmail.com>
> ---
>  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) {
> 

Re: [PATCH v2 2/2] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

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