git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/5] replace signal() with sigaction()
@ 2014-05-28  6:14 Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 1/5] progress.c: " Jeremiah Mahler
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

This patch set replaces calls to signal() with sigaction() in all files
except sigchain.c.  sigchain.c is a bit more complicated than the others
and will be done in a separate patch.

Jeremiah Mahler (5):
  progress.c: replace signal() with sigaction()
  daemon.c run_service(): replace signal() with sigaction()
  daemon.c child_handler(): replace signal() with sigaction()
  daemon.c service_loop(): replace signal() with sigaction()
  connect.c: replace signal() with sigaction()

 connect.c  |  5 ++++-
 daemon.c   | 15 ++++++++++++---
 progress.c |  6 +++++-
 3 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.0.0.rc4.6.g127602c

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
@ 2014-05-28  6:14 ` Jeremiah Mahler
  2014-05-28  8:07   ` Chris Packham
  2014-05-28  6:14 ` [PATCH 2/5] daemon.c run_service(): " Jeremiah Mahler
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

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..24df263 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;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = SIG_IGN;
 	setitimer(ITIMER_REAL, &v, NULL);
-	signal(SIGALRM, SIG_IGN);
+	sigaction(SIGALRM, &sa, 0);
 	progress_update = 0;
 }
 
-- 
2.0.0.rc4.6.g127602c

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] daemon.c run_service(): replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 1/5] progress.c: " Jeremiah Mahler
@ 2014-05-28  6:14 ` Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 3/5] daemon.c child_handler(): " Jeremiah Mahler
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

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

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

diff --git a/daemon.c b/daemon.c
index eba1255..78c4a1c 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, 0);
 
 	return service->fn();
 }
-- 
2.0.0.rc4.6.g127602c

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] daemon.c child_handler(): replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 1/5] progress.c: " Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 2/5] daemon.c run_service(): " Jeremiah Mahler
@ 2014-05-28  6:14 ` Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 4/5] daemon.c service_loop(): " Jeremiah Mahler
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

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

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

diff --git a/daemon.c b/daemon.c
index 78c4a1c..3c48dc0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -791,12 +791,15 @@ 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, 0);
 }
 
 static int set_reuse_addr(int sockfd)
-- 
2.0.0.rc4.6.g127602c

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] daemon.c service_loop(): replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
                   ` (2 preceding siblings ...)
  2014-05-28  6:14 ` [PATCH 3/5] daemon.c child_handler(): " Jeremiah Mahler
@ 2014-05-28  6:14 ` Jeremiah Mahler
  2014-05-28  6:14 ` [PATCH 5/5] connect.c: " Jeremiah Mahler
  2014-05-28  7:40 ` [PATCH 0/5] " Johannes Sixt
  5 siblings, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

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

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

diff --git a/daemon.c b/daemon.c
index 3c48dc0..8c28b3d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1002,6 +1002,7 @@ static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
 	int i;
+	struct sigaction sa;
 
 	pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
 
@@ -1010,7 +1011,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, 0);
 
 	for (;;) {
 		int i;
-- 
2.0.0.rc4.6.g127602c

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] connect.c: replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
                   ` (3 preceding siblings ...)
  2014-05-28  6:14 ` [PATCH 4/5] daemon.c service_loop(): " Jeremiah Mahler
@ 2014-05-28  6:14 ` Jeremiah Mahler
  2014-05-28  7:40 ` [PATCH 0/5] " Johannes Sixt
  5 siblings, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28  6:14 UTC (permalink / raw
  To: git; +Cc: Jeremiah Mahler

From signal(2)

  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.  See Portability below.

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.rc4.6.g127602c

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] replace signal() with sigaction()
  2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
                   ` (4 preceding siblings ...)
  2014-05-28  6:14 ` [PATCH 5/5] connect.c: " Jeremiah Mahler
@ 2014-05-28  7:40 ` Johannes Sixt
  2014-05-28  8:11   ` Chris Packham
  2014-05-28 16:11   ` Jeremiah Mahler
  5 siblings, 2 replies; 16+ messages in thread
From: Johannes Sixt @ 2014-05-28  7:40 UTC (permalink / raw
  To: Jeremiah Mahler, git

Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
> From signal(2)
> 
>   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.  See Portability below.
> 
> This patch set replaces calls to signal() with sigaction() in all files
> except sigchain.c.  sigchain.c is a bit more complicated than the others
> and will be done in a separate patch.

In compat/mingw.c we have:

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;
	return 0;
}

Notice "only implemented for SIGALRM". Are adding the missing signals
somewhere (here or in a later patch)?

> Jeremiah Mahler (5):
>   progress.c: replace signal() with sigaction()
>   daemon.c run_service(): replace signal() with sigaction()
>   daemon.c child_handler(): replace signal() with sigaction()
>   daemon.c service_loop(): replace signal() with sigaction()
>   connect.c: replace signal() with sigaction()
> 
>  connect.c  |  5 ++++-
>  daemon.c   | 15 ++++++++++++---
>  progress.c |  6 +++++-
>  3 files changed, 21 insertions(+), 5 deletions(-)

Isn't this a bit too much of code churn, given that there were no bug
reports where signal handling is identified as the culprit despite
the warning you cited above?

-- Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  6:14 ` [PATCH 1/5] progress.c: " Jeremiah Mahler
@ 2014-05-28  8:07   ` Chris Packham
  2014-05-28  8:19     ` David Kastrup
  2014-05-28 15:45     ` Jeremiah Mahler
  0 siblings, 2 replies; 16+ messages in thread
From: Chris Packham @ 2014-05-28  8:07 UTC (permalink / raw
  To: Jeremiah Mahler, git

On 28/05/14 18:14, Jeremiah Mahler wrote:
> From signal(2)
> 
>   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.  See Portability below.

Minor nit. The last sentence applies to the man page you're quoting and
doesn't really make sense when viewed in the context of this commit
message. Same applies to other patches in this series.

> 
> 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..24df263 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;
> +
> +	memset(&sa, 0, sizeof(sa));
> +	sa.sa_handler = SIG_IGN;

A C99 initialiser here would save the call to memset. Unfortunately
Documentation/CodingGuidelines is fairly clear on not using C99
initialisers, given the fact we're now at git 2.0 maybe it's time to
revisit this policy?

>  	setitimer(ITIMER_REAL, &v, NULL);
> -	signal(SIGALRM, SIG_IGN);
> +	sigaction(SIGALRM, &sa, 0);
>  	progress_update = 0;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] replace signal() with sigaction()
  2014-05-28  7:40 ` [PATCH 0/5] " Johannes Sixt
@ 2014-05-28  8:11   ` Chris Packham
  2014-05-28  8:23     ` Erik Faye-Lund
  2014-05-28 16:11   ` Jeremiah Mahler
  1 sibling, 1 reply; 16+ messages in thread
From: Chris Packham @ 2014-05-28  8:11 UTC (permalink / raw
  To: Johannes Sixt, Jeremiah Mahler, git

On 28/05/14 19:40, Johannes Sixt wrote:
> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
>> From signal(2)
>>
>>   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.  See Portability below.
>>
>> This patch set replaces calls to signal() with sigaction() in all files
>> except sigchain.c.  sigchain.c is a bit more complicated than the others
>> and will be done in a separate patch.
> 
> In compat/mingw.c we have:
> 
> 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;
> 	return 0;
> }
> 
> Notice "only implemented for SIGALRM". Are adding the missing signals
> somewhere (here or in a later patch)?
> 

* note: not a windows/mingw programmer *

Will the ones setting SIG_IGN be OK? Presumably we won't get these
signals on windows anyway so we're already getting what we want.

>> Jeremiah Mahler (5):
>>   progress.c: replace signal() with sigaction()
>>   daemon.c run_service(): replace signal() with sigaction()
>>   daemon.c child_handler(): replace signal() with sigaction()
>>   daemon.c service_loop(): replace signal() with sigaction()
>>   connect.c: replace signal() with sigaction()
>>
>>  connect.c  |  5 ++++-
>>  daemon.c   | 15 ++++++++++++---
>>  progress.c |  6 +++++-
>>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> Isn't this a bit too much of code churn, given that there were no bug
> reports where signal handling is identified as the culprit despite
> the warning you cited above?
> 
> -- Hannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  8:07   ` Chris Packham
@ 2014-05-28  8:19     ` David Kastrup
  2014-05-28  8:48       ` Erik Faye-Lund
  2014-05-28 17:35       ` Junio C Hamano
  2014-05-28 15:45     ` Jeremiah Mahler
  1 sibling, 2 replies; 16+ messages in thread
From: David Kastrup @ 2014-05-28  8:19 UTC (permalink / raw
  To: Chris Packham; +Cc: Jeremiah Mahler, git

Chris Packham <judge.packham@gmail.com> writes:

> On 28/05/14 18:14, Jeremiah Mahler wrote:
>> From signal(2)
>> 
>>   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.  See Portability below.
>
> Minor nit. The last sentence applies to the man page you're quoting and
> doesn't really make sense when viewed in the context of this commit
> message. Same applies to other patches in this series.
>
>> 
>> 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..24df263 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;
>> +
>> +	memset(&sa, 0, sizeof(sa));
>> +	sa.sa_handler = SIG_IGN;
>
> A C99 initialiser here would save the call to memset. Unfortunately
> Documentation/CodingGuidelines is fairly clear on not using C99
> initialisers, given the fact we're now at git 2.0 maybe it's time to
> revisit this policy?

If I look at the initialization of v in the context immediately above
the new code, it would appear that somebody already revisited this
policy.

-- 
David Kastrup

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] replace signal() with sigaction()
  2014-05-28  8:11   ` Chris Packham
@ 2014-05-28  8:23     ` Erik Faye-Lund
  0 siblings, 0 replies; 16+ messages in thread
From: Erik Faye-Lund @ 2014-05-28  8:23 UTC (permalink / raw
  To: Chris Packham; +Cc: Johannes Sixt, Jeremiah Mahler, GIT Mailing-list

On Wed, May 28, 2014 at 10:11 AM, Chris Packham <judge.packham@gmail.com> wrote:
> On 28/05/14 19:40, Johannes Sixt wrote:
>> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
>>> From signal(2)
>>>
>>>   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.  See Portability below.
>>>
>>> This patch set replaces calls to signal() with sigaction() in all files
>>> except sigchain.c.  sigchain.c is a bit more complicated than the others
>>> and will be done in a separate patch.
>>
>> In compat/mingw.c we have:
>>
>> 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;
>>       return 0;
>> }
>>
>> Notice "only implemented for SIGALRM". Are adding the missing signals
>> somewhere (here or in a later patch)?
>>
>
> * note: not a windows/mingw programmer *
>
> Will the ones setting SIG_IGN be OK? Presumably we won't get these
> signals on windows anyway so we're already getting what we want.

We'll still emit an useless error unless someone cooks up a fix.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  8:19     ` David Kastrup
@ 2014-05-28  8:48       ` Erik Faye-Lund
  2014-05-28  9:11         ` David Kastrup
  2014-05-28 17:35       ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Erik Faye-Lund @ 2014-05-28  8:48 UTC (permalink / raw
  To: David Kastrup; +Cc: Chris Packham, Jeremiah Mahler, GIT Mailing-list

On Wed, May 28, 2014 at 10:19 AM, David Kastrup <dak@gnu.org> wrote:
> Chris Packham <judge.packham@gmail.com> writes:
>
>> On 28/05/14 18:14, Jeremiah Mahler wrote:
>>> From signal(2)
>>>
>>>   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.  See Portability below.
>>
>> Minor nit. The last sentence applies to the man page you're quoting and
>> doesn't really make sense when viewed in the context of this commit
>> message. Same applies to other patches in this series.
>>
>>>
>>> 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..24df263 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;
>>> +
>>> +    memset(&sa, 0, sizeof(sa));
>>> +    sa.sa_handler = SIG_IGN;
>>
>> A C99 initialiser here would save the call to memset. Unfortunately
>> Documentation/CodingGuidelines is fairly clear on not using C99
>> initialisers, given the fact we're now at git 2.0 maybe it's time to
>> revisit this policy?
>
> If I look at the initialization of v in the context immediately above
> the new code, it would appear that somebody already revisited this
> policy.

Huh, the initialization of v doesn't use C99-features...?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  8:48       ` Erik Faye-Lund
@ 2014-05-28  9:11         ` David Kastrup
  0 siblings, 0 replies; 16+ messages in thread
From: David Kastrup @ 2014-05-28  9:11 UTC (permalink / raw
  To: Erik Faye-Lund; +Cc: Chris Packham, Jeremiah Mahler, GIT Mailing-list

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Wed, May 28, 2014 at 10:19 AM, David Kastrup <dak@gnu.org> wrote:
>> Chris Packham <judge.packham@gmail.com> writes:
>>
>>> On 28/05/14 18:14, Jeremiah Mahler wrote:
>>>>  static void clear_progress_signal(void)
>>>>  {
>>>>      struct itimerval v = {{0,},};
>>>> +    struct sigaction sa;
>>>> +
>>>> +    memset(&sa, 0, sizeof(sa));
>>>> +    sa.sa_handler = SIG_IGN;
>>>
>>> A C99 initialiser here would save the call to memset. Unfortunately
>>> Documentation/CodingGuidelines is fairly clear on not using C99
>>> initialisers, given the fact we're now at git 2.0 maybe it's time to
>>> revisit this policy?
>>
>> If I look at the initialization of v in the context immediately above
>> the new code, it would appear that somebody already revisited this
>> policy.
>
> Huh, the initialization of v doesn't use C99-features...?

Well, for me anything post-K&R apparently is C99.

Cf
<URL:http://computer-programming-forum.com/47-c-language/859a1b6693a0ddc5.htm>

I have to admit that gcc -c -ansi -std=c89 -pedantic does not complain,
so that makes it quite probable that I was erring somewhat on the side
of the ancient ones and zeros.

-- 
David Kastrup

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  8:07   ` Chris Packham
  2014-05-28  8:19     ` David Kastrup
@ 2014-05-28 15:45     ` Jeremiah Mahler
  1 sibling, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28 15:45 UTC (permalink / raw
  To: Chris Packham; +Cc: git

On Wed, May 28, 2014 at 08:07:38PM +1200, Chris Packham wrote:
> On 28/05/14 18:14, Jeremiah Mahler wrote:
> > From signal(2)
> > 
> >   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.  See Portability below.
> 
> Minor nit. The last sentence applies to the man page you're quoting and
> doesn't really make sense when viewed in the context of this commit
> message. Same applies to other patches in this series.
> 
Yes, that is confusing.

...
> > @@ -66,8 +66,12 @@ static void set_progress_signal(void)
> >  static void clear_progress_signal(void)
> >  {
> >  	struct itimerval v = {{0,},};
> > +	struct sigaction sa;
> > +
> > +	memset(&sa, 0, sizeof(sa));
> > +	sa.sa_handler = SIG_IGN;
> 
> A C99 initialiser here would save the call to memset. Unfortunately
> Documentation/CodingGuidelines is fairly clear on not using C99
> initialisers, given the fact we're now at git 2.0 maybe it's time to
> revisit this policy?
> 

struct sigaction sa = { .sa_handler = SIG_IGN };

I do like that.

This brings up another issue.  memset(&sa, 0, sizeof(sa)); The sigaction
examples I have seen always use memset.  The manpage for sigaction(2)
doesn't mention it.  Other code it Git uses memset with sigaction (see
fast-import.c line 530).

Is this struct guaranteed to be initialized to zero so I don't have to
use memset?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] replace signal() with sigaction()
  2014-05-28  7:40 ` [PATCH 0/5] " Johannes Sixt
  2014-05-28  8:11   ` Chris Packham
@ 2014-05-28 16:11   ` Jeremiah Mahler
  1 sibling, 0 replies; 16+ messages in thread
From: Jeremiah Mahler @ 2014-05-28 16:11 UTC (permalink / raw
  To: Johannes Sixt; +Cc: git

On Wed, May 28, 2014 at 09:40:55AM +0200, Johannes Sixt wrote:
> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
> > From signal(2)
> > 
> >   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.  See Portability below.
> > 
> > This patch set replaces calls to signal() with sigaction() in all files
> > except sigchain.c.  sigchain.c is a bit more complicated than the others
> > and will be done in a separate patch.
> 
> In compat/mingw.c we have:
> 
> 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;
> 	return 0;
> }
> 
> Notice "only implemented for SIGALRM". Are adding the missing signals
> somewhere (here or in a later patch)?
> 
Good catch Johannes, I did not account for this.

If I am reading this correctly, sigaction() can only be used for
SIGALRM, and no other signals.  But signal() it would have worked for
these other signals.

My first idea is why not call signal() from within this sigaction()
call?  The handler could be read from the struct sigaction passed in.

> > Jeremiah Mahler (5):
> >   progress.c: replace signal() with sigaction()
> >   daemon.c run_service(): replace signal() with sigaction()
> >   daemon.c child_handler(): replace signal() with sigaction()
> >   daemon.c service_loop(): replace signal() with sigaction()
> >   connect.c: replace signal() with sigaction()
> > 
> >  connect.c  |  5 ++++-
> >  daemon.c   | 15 ++++++++++++---
> >  progress.c |  6 +++++-
> >  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> Isn't this a bit too much of code churn, given that there were no bug
> reports where signal handling is identified as the culprit despite
> the warning you cited above?
> 
We might get bugs from continuing to use signal().  We might get bugs from
changing to sigaction().  I feel more comfortable using sigaction() since
its man page is so adamant about not using signal().

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] progress.c: replace signal() with sigaction()
  2014-05-28  8:19     ` David Kastrup
  2014-05-28  8:48       ` Erik Faye-Lund
@ 2014-05-28 17:35       ` Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2014-05-28 17:35 UTC (permalink / raw
  To: David Kastrup; +Cc: Chris Packham, Jeremiah Mahler, git

David Kastrup <dak@gnu.org> writes:

>>> diff --git a/progress.c b/progress.c
>>> index 261314e..24df263 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;
>>> +
>>> +	memset(&sa, 0, sizeof(sa));
>>> +	sa.sa_handler = SIG_IGN;
>>
>> A C99 initialiser here would save the call to memset. Unfortunately
>> Documentation/CodingGuidelines is fairly clear on not using C99
>> initialisers, given the fact we're now at git 2.0 maybe it's time to
>> revisit this policy?
>
> If I look at the initialization of v in the context immediately above
> the new code, it would appear that somebody already revisited this
> policy.

The existing structure initialization that says "the first field of
the structure is set to 0" implying "everything else will also be
set to 0" is not what we avoid.  That is straight C89.

What we avoid is an initializer with a designator, e.g.

	struct sigaction sa = {
        	.sa_handler = NULL,
                .sa_flags = 0,
	};

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2014-05-28 17:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-28  6:14 [PATCH 0/5] replace signal() with sigaction() Jeremiah Mahler
2014-05-28  6:14 ` [PATCH 1/5] progress.c: " Jeremiah Mahler
2014-05-28  8:07   ` Chris Packham
2014-05-28  8:19     ` David Kastrup
2014-05-28  8:48       ` Erik Faye-Lund
2014-05-28  9:11         ` David Kastrup
2014-05-28 17:35       ` Junio C Hamano
2014-05-28 15:45     ` Jeremiah Mahler
2014-05-28  6:14 ` [PATCH 2/5] daemon.c run_service(): " Jeremiah Mahler
2014-05-28  6:14 ` [PATCH 3/5] daemon.c child_handler(): " Jeremiah Mahler
2014-05-28  6:14 ` [PATCH 4/5] daemon.c service_loop(): " Jeremiah Mahler
2014-05-28  6:14 ` [PATCH 5/5] connect.c: " Jeremiah Mahler
2014-05-28  7:40 ` [PATCH 0/5] " Johannes Sixt
2014-05-28  8:11   ` Chris Packham
2014-05-28  8:23     ` Erik Faye-Lund
2014-05-28 16:11   ` Jeremiah Mahler

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).