threads / patch / 36798

v2, 2 partsreplace signal() with sigaction()

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

## tl;dr

6 messages between May 30, 2014 and May 31, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jeremiah Mahler· May 30, 2014, 20:58 UTC · lore

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· May 30, 2014, 20:58 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to compat/mingw.c +5 −4
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· May 30, 2014, 20:58 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to connect.c +4 −1
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· May 30, 2014, 21:48 UTC · re: Jeremiah Mahler · lore

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

Jeremiah Mahler <jmmahler@gmail.com> writes:
Show 5 quoted lines
> 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· May 31, 2014, 10:39 UTC · re: Jeremiah Mahler · lore

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

On 31/05/14 08:58, Jeremiah Mahler wrote:
Show 30 quoted lines
> 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) {
>
Jeremiah Mahler· May 31, 2014, 14:45 UTC · re: Chris Packham · lore

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

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:
> > 
...
Show 8 quoted lines
> > -	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

← back to recent threads