[PATCH v3 0/9] replace signal() with sigaction()

[PATCH v3 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

This is version 3 of the patch set to convert signal(2) to sigaction(2)
(previous discussion [1]).

[1]: http://marc.info/?l=git&m=140148352416926&w=2

Changes in this revision include:

  - Using NULL pointers instead of 0 as per the
    Documentation/CodingGuidlines pointed out by Chris Packham.

    sigaction(SIGCHLD, &sa, NULL);

  - Conversion of all remaining files which used signal().

  - sigchain.c required the most changes.  Both the old signal handler
    was used and the return value from signal() was being checked.
    signal() would return the previous error handler which would be
    SIG_ERR if an error occurred.  sigaction() just returns -1 in this
    case.

Jeremiah Mahler (9):
  compat/mingw.c: expand MinGW support for sigaction
  connect.c: replace signal() with sigaction()
  progress.c: replace signal() with sigaction()
  write_or_die.c: replace signal() with sigaction()
  daemon.c: replace signal() with sigaction()
  builtin/log.c: replace signal() with sigaction()
  builtin/merge-index.c: replace signal() with sigaction()
  builtin/verify-tag.c: replace signal() with sigaction()
  sigchain.c: replace signal() with sigaction()

 builtin/log.c         |  6 +++++-
 builtin/merge-index.c |  5 ++++-
 builtin/verify-tag.c  |  5 ++++-
 compat/mingw.c        |  9 +++++----
 connect.c             |  5 ++++-
 daemon.c              | 16 +++++++++++++---
 progress.c            |  6 +++++-
 sigchain.c            | 14 +++++++++++---
 write_or_die.c        |  6 +++++-
 9 files changed, 56 insertions(+), 16 deletions(-)

-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 1/9] 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.8.g7bf6e1f.dirty

Re: [PATCH v3 1/9] compat/mingw.c: expand MinGW support for sigaction

[Johannes Sixt]

Am 6/1/2014 20:10, schrieb 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");

A fix for this missing implementation is needed before patch 9/9 can be
applied.

>  
> -	timer_fn = in->sa_handler;
> +	if (sig == SIGALRM)
> +		timer_fn = in->sa_handler;
> +	else
> +		signal(sig, in->sa_handler);
> +
>  	return 0;
>  }
>  
> 

-- Hannes

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

[Jeremiah Mahler]

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.

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..1dc51b2 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, NULL);
 
 	protocol = parse_connect_url(url, &hostandport, &path);
 	if (flags & CONNECT_DIAG_URL) {
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 3/9] progress.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in progress.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 progress.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/progress.c b/progress.c
index 261314e..ff676bc 100644
--- a/progress.c
+++ b/progress.c
@@ -66,8 +66,12 @@ static void set_progress_signal(void)
 static void clear_progress_signal(void)
 {
 	struct itimerval v = {{0,},};
+	struct sigaction sa;
+
 	setitimer(ITIMER_REAL, &v, NULL);
-	signal(SIGALRM, SIG_IGN);
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_IGN;
+	sigaction(SIGALRM, &sa, NULL);
 	progress_update = 0;
 }
 
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 4/9] write_or_die.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in write_or_die.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 write_or_die.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..f5fdec8 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -2,8 +2,12 @@
 
 static void check_pipe(int err)
 {
+	struct sigaction sa;
+
 	if (err == EPIPE) {
-		signal(SIGPIPE, SIG_DFL);
+		memset(&sa, 0, sizeof(sa));
+		sa.sa_handler = SIG_DFL;
+		sigaction(SIGPIPE, &sa, NULL);
 		raise(SIGPIPE);
 		/* Should never happen, but just in case... */
 		exit(141);
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 5/9] daemon.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in daemon.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 daemon.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/daemon.c b/daemon.c
index eba1255..615426e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -322,6 +322,7 @@ static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
 	int enabled = service->enabled;
+	struct sigaction sa;
 
 	loginfo("Request %s for '%s'", service->name, dir);
 
@@ -376,7 +377,9 @@ static int run_service(char *dir, struct daemon_service *service)
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.
 	 */
-	signal(SIGTERM, SIG_IGN);
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_IGN;
+	sigaction(SIGTERM, &sa, NULL);
 
 	return service->fn();
 }
@@ -788,12 +791,16 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 
 static void child_handler(int signo)
 {
+	struct sigaction sa;
+
 	/*
 	 * Otherwise empty handler because systemcalls will get interrupted
 	 * upon signal receipt
 	 * SysV needs the handler to be rearmed
 	 */
-	signal(SIGCHLD, child_handler);
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sa, NULL);
 }
 
 static int set_reuse_addr(int sockfd)
@@ -996,6 +1003,7 @@ static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
 	int i;
+	struct sigaction sa;
 
 	pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
 
@@ -1004,7 +1012,9 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sa, NULL);
 
 	for (;;) {
 		int i;
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 6/9] builtin/log.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in builtin/log.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..f1deea1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -315,7 +315,11 @@ static void setup_early_output(struct rev_info *rev)
 static void finish_early_output(struct rev_info *rev)
 {
 	int n = estimate_commit_count(rev, rev->commits);
-	signal(SIGALRM, SIG_IGN);
+	struct sigaction sa;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_IGN;
+	sigaction(SIGALRM, &sa, NULL);
 	show_early_header(rev, "done", n);
 }
 
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 7/9] builtin/merge-index.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in builtin/merge-index.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/merge-index.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index b416d92..25db374 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -68,11 +68,14 @@ static void merge_all(void)
 int cmd_merge_index(int argc, const char **argv, const char *prefix)
 {
 	int i, force_file = 0;
+	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, NULL);
 
 	if (argc < 3)
 		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] <filename>*)");
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 8/9] builtin/verify-tag.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in builtin/verify-tag.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/verify-tag.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9cdf332..d5ccbad 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -73,6 +73,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_END()
 	};
+	struct sigaction sa;
 
 	git_config(git_verify_tag_config, NULL);
 
@@ -83,7 +84,9 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 
 	/* sometimes the program was terminated because this signal
 	 * was received in the process of writing the gpg input: */
-	signal(SIGPIPE, SIG_IGN);
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_IGN;
+	sigaction(SIGPIPE, &sa, NULL);
 	while (i < argc)
 		if (verify_tag(argv[i++], verbose))
 			had_error = 1;
-- 
2.0.0.8.g7bf6e1f.dirty

[PATCH v3 9/9] sigchain.c: replace signal() with sigaction()

[Jeremiah Mahler]

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.

Replaced signal() with sigaction() in sigchain.c

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 sigchain.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sigchain.c b/sigchain.c
index 1118b99..deab262 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -19,11 +19,16 @@ static void check_signum(int sig)
 int sigchain_push(int sig, sigchain_fun f)
 {
 	struct sigchain_signal *s = signals + sig;
+	struct sigaction sa, sa_old;
+	int result;
 	check_signum(sig);
 
 	ALLOC_GROW(s->old, s->n + 1, s->alloc);
-	s->old[s->n] = signal(sig, f);
-	if (s->old[s->n] == SIG_ERR)
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = f;
+	result = sigaction(sig, &sa, &sa_old);
+	s->old[s->n] = sa_old.sa_handler;
+	if (result == -1)
 		return -1;
 	s->n++;
 	return 0;
@@ -32,11 +37,14 @@ int sigchain_push(int sig, sigchain_fun f)
 int sigchain_pop(int sig)
 {
 	struct sigchain_signal *s = signals + sig;
+	struct sigaction sa, sa_old;
 	check_signum(sig);
 	if (s->n < 1)
 		return 0;
 
-	if (signal(sig, s->old[s->n - 1]) == SIG_ERR)
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = s->old[s->n - 1];
+	if (sigaction(sig, &sa, &sa_old) == -1)
 		return -1;
 	s->n--;
 	return 0;
-- 
2.0.0.8.g7bf6e1f.dirty

Re: [PATCH v3 0/9] replace signal() with sigaction()

[Johannes Sixt]

Am 6/1/2014 20:10, schrieb Jeremiah Mahler:
> This is version 3 of the patch set to convert signal(2) to sigaction(2)
> (previous discussion [1]).
> 
> [1]: http://marc.info/?l=git&m=140148352416926&w=2
> 
> Changes in this revision include:
> 
>   - Using NULL pointers instead of 0 as per the
>     Documentation/CodingGuidlines pointed out by Chris Packham.
> 
>     sigaction(SIGCHLD, &sa, NULL);
> 
>   - Conversion of all remaining files which used signal().
> 
>   - sigchain.c required the most changes.  Both the old signal handler
>     was used and the return value from signal() was being checked.
>     signal() would return the previous error handler which would be
>     SIG_ERR if an error occurred.  sigaction() just returns -1 in this
>     case.
> 
> Jeremiah Mahler (9):
>   compat/mingw.c: expand MinGW support for sigaction
>   connect.c: replace signal() with sigaction()
>   progress.c: replace signal() with sigaction()
>   write_or_die.c: replace signal() with sigaction()
>   daemon.c: replace signal() with sigaction()
>   builtin/log.c: replace signal() with sigaction()
>   builtin/merge-index.c: replace signal() with sigaction()
>   builtin/verify-tag.c: replace signal() with sigaction()
>   sigchain.c: replace signal() with sigaction()

The series without patch 9/9 works on Windows so far.

Without patch patch 9/9 and a more complete implementation of sigaction in
compat/mingw.c the series misses its goal. But even if you complete it, it
is IMHO only code churn without practical merits.

-- Hannes

> 
>  builtin/log.c         |  6 +++++-
>  builtin/merge-index.c |  5 ++++-
>  builtin/verify-tag.c  |  5 ++++-
>  compat/mingw.c        |  9 +++++----
>  connect.c             |  5 ++++-
>  daemon.c              | 16 +++++++++++++---
>  progress.c            |  6 +++++-
>  sigchain.c            | 14 +++++++++++---
>  write_or_die.c        |  6 +++++-
>  9 files changed, 56 insertions(+), 16 deletions(-)

Re: [PATCH v3 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

Hannes,

On Mon, Jun 02, 2014 at 01:28:25PM +0200, Johannes Sixt wrote:
> Am 6/1/2014 20:10, schrieb Jeremiah Mahler:
> > This is version 3 of the patch set to convert signal(2) to sigaction(2)
> > (previous discussion [1]).
> > 
...
> >   sigchain.c: replace signal() with sigaction()
> 
> The series without patch 9/9 works on Windows so far.
> 
> Without patch patch 9/9 and a more complete implementation of sigaction in
> compat/mingw.c the series misses its goal. But even if you complete it, it
> is IMHO only code churn without practical merits.
> 
> -- Hannes
> 

You are right, I missed the case where the old signal was used, as is
done in sigchain.c.  Sorry about that.

Thanks again for looking at my patch.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Re: [PATCH v3 0/9] replace signal() with sigaction()

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>> Jeremiah Mahler (9):
>>   compat/mingw.c: expand MinGW support for sigaction
>>   connect.c: replace signal() with sigaction()
>>   progress.c: replace signal() with sigaction()
>>   write_or_die.c: replace signal() with sigaction()
>>   daemon.c: replace signal() with sigaction()
>>   builtin/log.c: replace signal() with sigaction()
>>   builtin/merge-index.c: replace signal() with sigaction()
>>   builtin/verify-tag.c: replace signal() with sigaction()
>>   sigchain.c: replace signal() with sigaction()
>
> The series without patch 9/9 works on Windows so far.
>
> Without patch patch 9/9 and a more complete implementation of sigaction in
> compat/mingw.c the series misses its goal. But even if you complete it, it
> is IMHO only code churn without practical merits.

Hmm, you sound a bit harsher than you usually do---although I
sort of share with you the doubt on the practical merits.

Re: [PATCH v3 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

On Mon, Jun 02, 2014 at 12:05:29PM -0700, Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> >> Jeremiah Mahler (9):
> >>   compat/mingw.c: expand MinGW support for sigaction
> >>   connect.c: replace signal() with sigaction()
> >>   progress.c: replace signal() with sigaction()
> >>   write_or_die.c: replace signal() with sigaction()
> >>   daemon.c: replace signal() with sigaction()
> >>   builtin/log.c: replace signal() with sigaction()
> >>   builtin/merge-index.c: replace signal() with sigaction()
> >>   builtin/verify-tag.c: replace signal() with sigaction()
> >>   sigchain.c: replace signal() with sigaction()
> >
> > The series without patch 9/9 works on Windows so far.
> >
> > Without patch patch 9/9 and a more complete implementation of sigaction in
> > compat/mingw.c the series misses its goal. But even if you complete it, it
> > is IMHO only code churn without practical merits.
> 
> Hmm, you sound a bit harsher than you usually do---although I
> sort of share with you the doubt on the practical merits.
> 

Alright, I'm dropping it.  Too much work for no real gain other than
some piece of mind.

Thanks Johannes and Junio for your feedback.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler