{"thread":{"id":"36772","subject":"[PATCH 0/5] replace signal() with sigaction()","startedAt":"2014-05-28T06:14:10Z","lastAt":"2014-05-28T17:35:38Z","messageCount":16,"participants":["Jeremiah Mahler","Johannes Sixt","Chris Packham","David Kastrup","Erik Faye-Lund","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"242837","messageId":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":null,"subject":"[PATCH 0/5] replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:10Z","receivedAt":"2014-05-28T06:14:10Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nThis patch set replaces calls to signal() with sigaction() in all files\nexcept sigchain.c. sigchain.c is a bit more complicated than the others\nand will be done in a separate patch.\n\nJeremiah Mahler (5):\n progress.c: replace signal() with sigaction()\n daemon.c run_service(): replace signal() with sigaction()\n daemon.c child_handler(): replace signal() with sigaction()\n daemon.c service_loop(): replace signal() with sigaction()\n connect.c: replace signal() with sigaction()\n\n connect.c | 5 ++++-\n daemon.c | 15 ++++++++++++---\n progress.c | 6 +++++-\n 3 files changed, 21 insertions(+), 5 deletions(-)\n\n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242838","messageId":"1401257655-6043-2-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:11Z","receivedAt":"2014-05-28T06:14:11Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nReplaced signal() with sigaction() in progress.c\n\nSigned-off-by: Jeremiah Mahler \n---\n progress.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/progress.c b/progress.c\nindex 261314e..24df263 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -66,8 +66,12 @@ static void set_progress_signal(void)\n static void clear_progress_signal(void)\n {\n \tstruct itimerval v = {{0,},};\n+\tstruct sigaction sa;\n+\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = SIG_IGN;\n \tsetitimer(ITIMER_REAL, &v, NULL);\n-\tsignal(SIGALRM, SIG_IGN);\n+\tsigaction(SIGALRM, &sa, 0);\n \tprogress_update = 0;\n }\n \n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242839","messageId":"1401257655-6043-3-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH 2/5] daemon.c run_service(): replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:12Z","receivedAt":"2014-05-28T06:14:12Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nReplaced signal() with sigaction() in daemon.c run_service()\n\nSigned-off-by: Jeremiah Mahler \n---\n daemon.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex eba1255..78c4a1c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -322,6 +322,7 @@ static int run_service(char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n \tint enabled = service->enabled;\n+\tstruct sigaction sa;\n \n \tloginfo(\"Request %s for '%s'\", service->name, dir);\n \n@@ -376,7 +377,9 @@ static int run_service(char *dir, struct daemon_service *service)\n \t * We'll ignore SIGTERM from now on, we have a\n \t * good client.\n \t */\n-\tsignal(SIGTERM, SIG_IGN);\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = SIG_IGN;\n+\tsigaction(SIGTERM, &sa, 0);\n \n \treturn service->fn();\n }\n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242841","messageId":"1401257655-6043-4-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH 3/5] daemon.c child_handler(): replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:13Z","receivedAt":"2014-05-28T06:14:13Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nReplaced signal() with sigaction() in daemon.c child_handler()\n\nSigned-off-by: Jeremiah Mahler \n---\n daemon.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 78c4a1c..3c48dc0 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -791,12 +791,15 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \n static void child_handler(int signo)\n {\n+\tstruct sigaction sa;\n \t/*\n \t * Otherwise empty handler because systemcalls will get interrupted\n \t * upon signal receipt\n \t * SysV needs the handler to be rearmed\n \t */\n-\tsignal(SIGCHLD, child_handler);\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sa, 0);\n }\n \n static int set_reuse_addr(int sockfd)\n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242840","messageId":"1401257655-6043-5-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH 4/5] daemon.c service_loop(): replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:14Z","receivedAt":"2014-05-28T06:14:14Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nReplaced signal() with sigaction() in daemon.c service_loop()\n\nSigned-off-by: Jeremiah Mahler \n---\n daemon.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 3c48dc0..8c28b3d 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1002,6 +1002,7 @@ static int service_loop(struct socketlist *socklist)\n {\n \tstruct pollfd *pfd;\n \tint i;\n+\tstruct sigaction sa;\n \n \tpfd = xcalloc(socklist->nr, sizeof(struct pollfd));\n \n@@ -1010,7 +1011,9 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sa, 0);\n \n \tfor (;;) {\n \t\tint i;\n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242842","messageId":"1401257655-6043-6-git-send-email-jmmahler@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH 5/5] connect.c: replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T06:14:15Z","receivedAt":"2014-05-28T06:14:15Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"From signal(2)\n\n The behavior of signal() varies across UNIX versions, and has also var‐\n ied historically across different versions of Linux. Avoid its use:\n use sigaction(2) instead. See Portability below.\n\nReplaced signal() with sigaction() in connect.c\n\nSigned-off-by: Jeremiah Mahler \n---\n connect.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex a983d06..b2a33c9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tenum protocol protocol;\n \tconst char **arg;\n \tstruct strbuf cmd = STRBUF_INIT;\n+\tstruct sigaction sa;\n \n \t/* Without this we cannot rely on waitpid() to tell\n \t * what happened to our children.\n \t */\n-\tsignal(SIGCHLD, SIG_DFL);\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = SIG_DFL;\n+\tsigaction(SIGCHLD, &sa, 0);\n \n \tprotocol = parse_connect_url(url, &hostandport, &path);\n \tif (flags & CONNECT_DIAG_URL) {\n-- \n2.0.0.rc4.6.g127602c\n"},{"id":"242844","messageId":"53859307.8080506@viscovery.net","threadId":"36772","inReplyTo":"1401257655-6043-1-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH 0/5] replace signal() with sigaction()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-05-28T07:40:55Z","receivedAt":"2014-05-28T07:40:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/28/2014 8:14, schrieb Jeremiah Mahler:\n> From signal(2)\n> \n> The behavior of signal() varies across UNIX versions, and has also var‐\n> ied historically across different versions of Linux. Avoid its use:\n> use sigaction(2) instead. See Portability below.\n> \n> This patch set replaces calls to signal() with sigaction() in all files\n> except sigchain.c. sigchain.c is a bit more complicated than the others\n> and will be done in a separate patch.\n\nIn compat/mingw.c we have:\n\nint sigaction(int sig, struct sigaction *in, struct sigaction *out)\n{\n\tif (sig != SIGALRM)\n\t\treturn errno = EINVAL,\n\t\t\terror(\"sigaction only implemented for SIGALRM\");\n\tif (out != NULL)\n\t\treturn errno = EINVAL,\n\t\t\terror(\"sigaction: param 3 != NULL not implemented\");\n\n\ttimer_fn = in->sa_handler;\n\treturn 0;\n}\n\nNotice \"only implemented for SIGALRM\". Are adding the missing signals\nsomewhere (here or in a later patch)?\n\n> Jeremiah Mahler (5):\n> progress.c: replace signal() with sigaction()\n> daemon.c run_service(): replace signal() with sigaction()\n> daemon.c child_handler(): replace signal() with sigaction()\n> daemon.c service_loop(): replace signal() with sigaction()\n> connect.c: replace signal() with sigaction()\n> \n> connect.c | 5 ++++-\n> daemon.c | 15 ++++++++++++---\n> progress.c | 6 +++++-\n> 3 files changed, 21 insertions(+), 5 deletions(-)\n\nIsn't this a bit too much of code churn, given that there were no bug\nreports where signal handling is identified as the culprit despite\nthe warning you cited above?\n\n-- Hannes\n"},{"id":"242845","messageId":"5385994A.5040507@gmail.com","threadId":"36772","inReplyTo":"1401257655-6043-2-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2014-05-28T08:07:38Z","receivedAt":"2014-05-28T08:07:38Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 28/05/14 18:14, Jeremiah Mahler wrote:\n> From signal(2)\n> \n> The behavior of signal() varies across UNIX versions, and has also var‐\n> ied historically across different versions of Linux. Avoid its use:\n> use sigaction(2) instead. See Portability below.\n\nMinor nit. The last sentence applies to the man page you're quoting and\ndoesn't really make sense when viewed in the context of this commit\nmessage. Same applies to other patches in this series.\n\n> \n> Replaced signal() with sigaction() in progress.c\n> \n> Signed-off-by: Jeremiah Mahler \n> ---\n> progress.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/progress.c b/progress.c\n> index 261314e..24df263 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -66,8 +66,12 @@ static void set_progress_signal(void)\n> static void clear_progress_signal(void)\n> {\n> \tstruct itimerval v = {{0,},};\n> +\tstruct sigaction sa;\n> +\n> +\tmemset(&sa, 0, sizeof(sa));\n> +\tsa.sa_handler = SIG_IGN;\n\nA C99 initialiser here would save the call to memset. Unfortunately\nDocumentation/CodingGuidelines is fairly clear on not using C99\ninitialisers, given the fact we're now at git 2.0 maybe it's time to\nrevisit this policy?\n\n> \tsetitimer(ITIMER_REAL, &v, NULL);\n> -\tsignal(SIGALRM, SIG_IGN);\n> +\tsigaction(SIGALRM, &sa, 0);\n> \tprogress_update = 0;\n> }\n> \n> \n"},{"id":"242846","messageId":"53859A44.7040806@gmail.com","threadId":"36772","inReplyTo":"53859307.8080506@viscovery.net","subject":"Re: [PATCH 0/5] replace signal() with sigaction()","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2014-05-28T08:11:48Z","receivedAt":"2014-05-28T08:11:48Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 28/05/14 19:40, Johannes Sixt wrote:\n> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:\n>> From signal(2)\n>>\n>> The behavior of signal() varies across UNIX versions, and has also var‐\n>> ied historically across different versions of Linux. Avoid its use:\n>> use sigaction(2) instead. See Portability below.\n>>\n>> This patch set replaces calls to signal() with sigaction() in all files\n>> except sigchain.c. sigchain.c is a bit more complicated than the others\n>> and will be done in a separate patch.\n> \n> In compat/mingw.c we have:\n> \n> int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n> {\n> \tif (sig != SIGALRM)\n> \t\treturn errno = EINVAL,\n> \t\t\terror(\"sigaction only implemented for SIGALRM\");\n> \tif (out != NULL)\n> \t\treturn errno = EINVAL,\n> \t\t\terror(\"sigaction: param 3 != NULL not implemented\");\n> \n> \ttimer_fn = in->sa_handler;\n> \treturn 0;\n> }\n> \n> Notice \"only implemented for SIGALRM\". Are adding the missing signals\n> somewhere (here or in a later patch)?\n> \n\n* note: not a windows/mingw programmer *\n\nWill the ones setting SIG_IGN be OK? Presumably we won't get these\nsignals on windows anyway so we're already getting what we want.\n\n>> Jeremiah Mahler (5):\n>> progress.c: replace signal() with sigaction()\n>> daemon.c run_service(): replace signal() with sigaction()\n>> daemon.c child_handler(): replace signal() with sigaction()\n>> daemon.c service_loop(): replace signal() with sigaction()\n>> connect.c: replace signal() with sigaction()\n>>\n>> connect.c | 5 ++++-\n>> daemon.c | 15 ++++++++++++---\n>> progress.c | 6 +++++-\n>> 3 files changed, 21 insertions(+), 5 deletions(-)\n> \n> Isn't this a bit too much of code churn, given that there were no bug\n> reports where signal handling is identified as the culprit despite\n> the warning you cited above?\n> \n> -- Hannes\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"242847","messageId":"87sinuiad2.fsf@fencepost.gnu.org","threadId":"36772","inReplyTo":"5385994A.5040507@gmail.com","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-05-28T08:19:21Z","receivedAt":"2014-05-28T08:19:21Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Chris Packham writes:\n\n> On 28/05/14 18:14, Jeremiah Mahler wrote:\n>> From signal(2)\n>> \n>> The behavior of signal() varies across UNIX versions, and has also var‐\n>> ied historically across different versions of Linux. Avoid its use:\n>> use sigaction(2) instead. See Portability below.\n>\n> Minor nit. The last sentence applies to the man page you're quoting and\n> doesn't really make sense when viewed in the context of this commit\n> message. Same applies to other patches in this series.\n>\n>> \n>> Replaced signal() with sigaction() in progress.c\n>> \n>> Signed-off-by: Jeremiah Mahler \n>> ---\n>> progress.c | 6 +++++-\n>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/progress.c b/progress.c\n>> index 261314e..24df263 100644\n>> --- a/progress.c\n>> +++ b/progress.c\n>> @@ -66,8 +66,12 @@ static void set_progress_signal(void)\n>> static void clear_progress_signal(void)\n>> {\n>> \tstruct itimerval v = {{0,},};\n>> +\tstruct sigaction sa;\n>> +\n>> +\tmemset(&sa, 0, sizeof(sa));\n>> +\tsa.sa_handler = SIG_IGN;\n>\n> A C99 initialiser here would save the call to memset. Unfortunately\n> Documentation/CodingGuidelines is fairly clear on not using C99\n> initialisers, given the fact we're now at git 2.0 maybe it's time to\n> revisit this policy?\n\nIf I look at the initialization of v in the context immediately above\nthe new code, it would appear that somebody already revisited this\npolicy.\n\n-- \nDavid Kastrup\n"},{"id":"242848","messageId":"CABPQNSZqazRLj3iOT1zei6ym6rcv5rzGrhK=O1vCPCh6p-Ad2A@mail.gmail.com","threadId":"36772","inReplyTo":"53859A44.7040806@gmail.com","subject":"Re: [PATCH 0/5] replace signal() with sigaction()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-05-28T08:23:19Z","receivedAt":"2014-05-28T08:23:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, May 28, 2014 at 10:11 AM, Chris Packham wrote:\n> On 28/05/14 19:40, Johannes Sixt wrote:\n>> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:\n>>> From signal(2)\n>>>\n>>> The behavior of signal() varies across UNIX versions, and has also var‐\n>>> ied historically across different versions of Linux. Avoid its use:\n>>> use sigaction(2) instead. See Portability below.\n>>>\n>>> This patch set replaces calls to signal() with sigaction() in all files\n>>> except sigchain.c. sigchain.c is a bit more complicated than the others\n>>> and will be done in a separate patch.\n>>\n>> In compat/mingw.c we have:\n>>\n>> int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n>> {\n>> if (sig != SIGALRM)\n>> return errno = EINVAL,\n>> error(\"sigaction only implemented for SIGALRM\");\n>> if (out != NULL)\n>> return errno = EINVAL,\n>> error(\"sigaction: param 3 != NULL not implemented\");\n>>\n>> timer_fn = in->sa_handler;\n>> return 0;\n>> }\n>>\n>> Notice \"only implemented for SIGALRM\". Are adding the missing signals\n>> somewhere (here or in a later patch)?\n>>\n>\n> * note: not a windows/mingw programmer *\n>\n> Will the ones setting SIG_IGN be OK? Presumably we won't get these\n> signals on windows anyway so we're already getting what we want.\n\nWe'll still emit an useless error unless someone cooks up a fix.\n"},{"id":"242849","messageId":"CABPQNSbRZS6DuNOLpWKtx6150sz20GjQ6RaakiSdi7FEQW5X3Q@mail.gmail.com","threadId":"36772","inReplyTo":"87sinuiad2.fsf@fencepost.gnu.org","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-05-28T08:48:35Z","receivedAt":"2014-05-28T08:48:35Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, May 28, 2014 at 10:19 AM, David Kastrup wrote:\n> Chris Packham writes:\n>\n>> On 28/05/14 18:14, Jeremiah Mahler wrote:\n>>> From signal(2)\n>>>\n>>> The behavior of signal() varies across UNIX versions, and has also var‐\n>>> ied historically across different versions of Linux. Avoid its use:\n>>> use sigaction(2) instead. See Portability below.\n>>\n>> Minor nit. The last sentence applies to the man page you're quoting and\n>> doesn't really make sense when viewed in the context of this commit\n>> message. Same applies to other patches in this series.\n>>\n>>>\n>>> Replaced signal() with sigaction() in progress.c\n>>>\n>>> Signed-off-by: Jeremiah Mahler \n>>> ---\n>>> progress.c | 6 +++++-\n>>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/progress.c b/progress.c\n>>> index 261314e..24df263 100644\n>>> --- a/progress.c\n>>> +++ b/progress.c\n>>> @@ -66,8 +66,12 @@ static void set_progress_signal(void)\n>>> static void clear_progress_signal(void)\n>>> {\n>>> struct itimerval v = {{0,},};\n>>> + struct sigaction sa;\n>>> +\n>>> + memset(&sa, 0, sizeof(sa));\n>>> + sa.sa_handler = SIG_IGN;\n>>\n>> A C99 initialiser here would save the call to memset. Unfortunately\n>> Documentation/CodingGuidelines is fairly clear on not using C99\n>> initialisers, given the fact we're now at git 2.0 maybe it's time to\n>> revisit this policy?\n>\n> If I look at the initialization of v in the context immediately above\n> the new code, it would appear that somebody already revisited this\n> policy.\n\nHuh, the initialization of v doesn't use C99-features...?\n"},{"id":"242851","messageId":"87k396i7yu.fsf@fencepost.gnu.org","threadId":"36772","inReplyTo":"CABPQNSbRZS6DuNOLpWKtx6150sz20GjQ6RaakiSdi7FEQW5X3Q@mail.gmail.com","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-05-28T09:11:05Z","receivedAt":"2014-05-28T09:11:05Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Erik Faye-Lund writes:\n\n> On Wed, May 28, 2014 at 10:19 AM, David Kastrup wrote:\n>> Chris Packham writes:\n>>\n>>> On 28/05/14 18:14, Jeremiah Mahler wrote:\n>>>> static void clear_progress_signal(void)\n>>>> {\n>>>> struct itimerval v = {{0,},};\n>>>> + struct sigaction sa;\n>>>> +\n>>>> + memset(&sa, 0, sizeof(sa));\n>>>> + sa.sa_handler = SIG_IGN;\n>>>\n>>> A C99 initialiser here would save the call to memset. Unfortunately\n>>> Documentation/CodingGuidelines is fairly clear on not using C99\n>>> initialisers, given the fact we're now at git 2.0 maybe it's time to\n>>> revisit this policy?\n>>\n>> If I look at the initialization of v in the context immediately above\n>> the new code, it would appear that somebody already revisited this\n>> policy.\n>\n> Huh, the initialization of v doesn't use C99-features...?\n\nWell, for me anything post-K&R apparently is C99.\n\nCf\n\n\nI have to admit that gcc -c -ansi -std=c89 -pedantic does not complain,\nso that makes it quite probable that I was erring somewhat on the side\nof the ancient ones and zeros.\n\n-- \nDavid Kastrup\n"},{"id":"242865","messageId":"20140528154509.GA7076@hudson.localdomain","threadId":"36772","inReplyTo":"5385994A.5040507@gmail.com","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T15:45:09Z","receivedAt":"2014-05-28T15:45:09Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 28, 2014 at 08:07:38PM +1200, Chris Packham wrote:\n> On 28/05/14 18:14, Jeremiah Mahler wrote:\n> > From signal(2)\n> > \n> > The behavior of signal() varies across UNIX versions, and has also var‐\n> > ied historically across different versions of Linux. Avoid its use:\n> > use sigaction(2) instead. See Portability below.\n> \n> Minor nit. The last sentence applies to the man page you're quoting and\n> doesn't really make sense when viewed in the context of this commit\n> message. Same applies to other patches in this series.\n> \nYes, that is confusing.\n\n...\n> > @@ -66,8 +66,12 @@ static void set_progress_signal(void)\n> > static void clear_progress_signal(void)\n> > {\n> > \tstruct itimerval v = {{0,},};\n> > +\tstruct sigaction sa;\n> > +\n> > +\tmemset(&sa, 0, sizeof(sa));\n> > +\tsa.sa_handler = SIG_IGN;\n> \n> A C99 initialiser here would save the call to memset. Unfortunately\n> Documentation/CodingGuidelines is fairly clear on not using C99\n> initialisers, given the fact we're now at git 2.0 maybe it's time to\n> revisit this policy?\n> \n\nstruct sigaction sa = { .sa_handler = SIG_IGN };\n\nI do like that.\n\nThis brings up another issue. memset(&sa, 0, sizeof(sa)); The sigaction\nexamples I have seen always use memset. The manpage for sigaction(2)\ndoesn't mention it. Other code it Git uses memset with sigaction (see\nfast-import.c line 530).\n\nIs this struct guaranteed to be initialized to zero so I don't have to\nuse memset?\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242867","messageId":"20140528161100.GB7076@hudson.localdomain","threadId":"36772","inReplyTo":"53859307.8080506@viscovery.net","subject":"Re: [PATCH 0/5] replace signal() with sigaction()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-28T16:11:00Z","receivedAt":"2014-05-28T16:11:00Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 28, 2014 at 09:40:55AM +0200, Johannes Sixt wrote:\n> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:\n> > From signal(2)\n> > \n> > The behavior of signal() varies across UNIX versions, and has also var‐\n> > ied historically across different versions of Linux. Avoid its use:\n> > use sigaction(2) instead. See Portability below.\n> > \n> > This patch set replaces calls to signal() with sigaction() in all files\n> > except sigchain.c. sigchain.c is a bit more complicated than the others\n> > and will be done in a separate patch.\n> \n> In compat/mingw.c we have:\n> \n> int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n> {\n> \tif (sig != SIGALRM)\n> \t\treturn errno = EINVAL,\n> \t\t\terror(\"sigaction only implemented for SIGALRM\");\n> \tif (out != NULL)\n> \t\treturn errno = EINVAL,\n> \t\t\terror(\"sigaction: param 3 != NULL not implemented\");\n> \n> \ttimer_fn = in->sa_handler;\n> \treturn 0;\n> }\n> \n> Notice \"only implemented for SIGALRM\". Are adding the missing signals\n> somewhere (here or in a later patch)?\n> \nGood catch Johannes, I did not account for this.\n\nIf I am reading this correctly, sigaction() can only be used for\nSIGALRM, and no other signals. But signal() it would have worked for\nthese other signals.\n\nMy first idea is why not call signal() from within this sigaction()\ncall? The handler could be read from the struct sigaction passed in.\n\n> > Jeremiah Mahler (5):\n> > progress.c: replace signal() with sigaction()\n> > daemon.c run_service(): replace signal() with sigaction()\n> > daemon.c child_handler(): replace signal() with sigaction()\n> > daemon.c service_loop(): replace signal() with sigaction()\n> > connect.c: replace signal() with sigaction()\n> > \n> > connect.c | 5 ++++-\n> > daemon.c | 15 ++++++++++++---\n> > progress.c | 6 +++++-\n> > 3 files changed, 21 insertions(+), 5 deletions(-)\n> \n> Isn't this a bit too much of code churn, given that there were no bug\n> reports where signal handling is identified as the culprit despite\n> the warning you cited above?\n> \nWe might get bugs from continuing to use signal(). We might get bugs from\nchanging to sigaction(). I feel more comfortable using sigaction() since\nits man page is so adamant about not using signal().\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242876","messageId":"xmqqvbspdcwl.fsf@gitster.dls.corp.google.com","threadId":"36772","inReplyTo":"87sinuiad2.fsf@fencepost.gnu.org","subject":"Re: [PATCH 1/5] progress.c: replace signal() with sigaction()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-28T17:35:38Z","receivedAt":"2014-05-28T17:35:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n>>> diff --git a/progress.c b/progress.c\n>>> index 261314e..24df263 100644\n>>> --- a/progress.c\n>>> +++ b/progress.c\n>>> @@ -66,8 +66,12 @@ static void set_progress_signal(void)\n>>> static void clear_progress_signal(void)\n>>> {\n>>> \tstruct itimerval v = {{0,},};\n>>> +\tstruct sigaction sa;\n>>> +\n>>> +\tmemset(&sa, 0, sizeof(sa));\n>>> +\tsa.sa_handler = SIG_IGN;\n>>\n>> A C99 initialiser here would save the call to memset. Unfortunately\n>> Documentation/CodingGuidelines is fairly clear on not using C99\n>> initialisers, given the fact we're now at git 2.0 maybe it's time to\n>> revisit this policy?\n>\n> If I look at the initialization of v in the context immediately above\n> the new code, it would appear that somebody already revisited this\n> policy.\n\nThe existing structure initialization that says \"the first field of\nthe structure is set to 0\" implying \"everything else will also be\nset to 0\" is not what we avoid. That is straight C89.\n\nWhat we avoid is an initializer with a designator, e.g.\n\n\tstruct sigaction sa = {\n \t.sa_handler = NULL,\n .sa_flags = 0,\n\t};\n"}]}