[pacman-dev] [PATCH] pacman: Correct signal handler behavior
One of the comments for this function is out of sync with the code. If a SIGHUP is received the code calls the release function only if there is no transaction in flight (contrary to the comments). This change brings the code in line with the comment (and moves the comment for readability). Additionally, it changes the exit message to reflect the fact that the signal received was either a SIGHUP or a SIGTERM. Signed-off-by: Silvan Jegen <s.jegen@gmail.com> --- src/pacman/pacman.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..599a141 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,20 +301,20 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { - if(signum == SIGINT) { - msg = "\nInterrupt signal received\n"; - } else { - msg = "\nHangup signal received\n"; - } - xwrite(err, msg, strlen(msg)); + } + /* SIGINT: no committing transaction, release it now and then exit pacman + * SIGHUP, SIGTERM: release no matter what */ + if(signum == SIGINT) { + msg = "\nInterrupt signal received\n"; if(alpm_trans_interrupt(config->handle) == 0) { /* a transaction is being interrupted, don't exit pacman yet. */ return; } + } else { + msg = "\nHangup/Termination signal received\n"; } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */ + + xwrite(err, msg, strlen(msg)); alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1); -- 2.0.0
On 06/20/14 at 10:00pm, Silvan Jegen wrote:
One of the comments for this function is out of sync with the code. If a SIGHUP is received the code calls the release function only if there is no transaction in flight (contrary to the comments).
This change brings the code in line with the comment (and moves the comment for readability). Additionally, it changes the exit message to reflect the fact that the signal received was either a SIGHUP or a SIGTERM.
Signed-off-by: Silvan Jegen <s.jegen@gmail.com> --- src/pacman/pacman.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
I think the comment is just a typo. According to its commit message, SIGHUP and SIGINT should be treated the same. SIGHUP seems like a poor reason to kill an active transaction rather than more gracefully interrupt it. apg
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..599a141 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,20 +301,20 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { - if(signum == SIGINT) { - msg = "\nInterrupt signal received\n"; - } else { - msg = "\nHangup signal received\n"; - } - xwrite(err, msg, strlen(msg)); + } + /* SIGINT: no committing transaction, release it now and then exit pacman + * SIGHUP, SIGTERM: release no matter what */ + if(signum == SIGINT) { + msg = "\nInterrupt signal received\n"; if(alpm_trans_interrupt(config->handle) == 0) { /* a transaction is being interrupted, don't exit pacman yet. */ return; } + } else { + msg = "\nHangup/Termination signal received\n"; } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */ + + xwrite(err, msg, strlen(msg)); alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1); -- 2.0.0
On Fri, Jun 20, 2014 at 05:12:10PM -0400, Andrew Gregory wrote:
On 06/20/14 at 10:00pm, Silvan Jegen wrote:
One of the comments for this function is out of sync with the code. If a SIGHUP is received the code calls the release function only if there is no transaction in flight (contrary to the comments).
This change brings the code in line with the comment (and moves the comment for readability). Additionally, it changes the exit message to reflect the fact that the signal received was either a SIGHUP or a SIGTERM.
Signed-off-by: Silvan Jegen <s.jegen@gmail.com> --- src/pacman/pacman.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
I think the comment is just a typo. According to its commit message, SIGHUP and SIGINT should be treated the same. SIGHUP seems like a poor reason to kill an active transaction rather than more gracefully interrupt it.
I tend to agree. Should I send a patch to change the comment accordingly (and maybe move it before the "else if(signum == SIGINT || signum == SIGHUP) {" in the original code)?
apg
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..599a141 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,20 +301,20 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { - if(signum == SIGINT) { - msg = "\nInterrupt signal received\n"; - } else { - msg = "\nHangup signal received\n"; - } - xwrite(err, msg, strlen(msg)); + } + /* SIGINT: no committing transaction, release it now and then exit pacman + * SIGHUP, SIGTERM: release no matter what */ + if(signum == SIGINT) { + msg = "\nInterrupt signal received\n"; if(alpm_trans_interrupt(config->handle) == 0) { /* a transaction is being interrupted, don't exit pacman yet. */ return; } + } else { + msg = "\nHangup/Termination signal received\n"; } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */ + + xwrite(err, msg, strlen(msg)); alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1); -- 2.0.0
One of the comments for this function is out of sync with the code. Since the code exhibits the more sane behavior of treating SIGINT and SIGHUB the same way (by not exiting pacman when there is a commit in flight) we adjust the comment. Given this code flow, the if/else statements can be simplified somewhat as well. Signed-off-by: Silvan Jegen <s.jegen@gmail.com> --- src/pacman/pacman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..c0903ee 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,7 +301,9 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { + /* SIGINT/SIGHUP: no committing transaction, release it now and then exit pacman + * SIGTERM: release no matter what */ + } else if(signum != SIGTERM) { if(signum == SIGINT) { msg = "\nInterrupt signal received\n"; } else { @@ -313,8 +315,6 @@ static void handler(int signum) return; } } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */ alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1); -- 2.0.0
On 21/06/14 18:18, Silvan Jegen wrote:
One of the comments for this function is out of sync with the code. Since the code exhibits the more sane behavior of treating SIGINT and SIGHUB the same way (by not exiting pacman when there is a commit in flight) we adjust the comment.
Given this code flow, the if/else statements can be simplified somewhat as well.
Signed-off-by: Silvan Jegen <s.jegen@gmail.com>
There is nothing I see correct about this patch.
--- src/pacman/pacman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..c0903ee 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,7 +301,9 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { + /* SIGINT/SIGHUP: no committing transaction, release it now and then exit pacman
This comment makes little sense here. It was better where it was. See below.
+ * SIGTERM: release no matter what */
This should stay where it was. See below.
+ } else if(signum != SIGTERM) {
So SIGQUIT is handled like SIGINT and SIGHUP?
if(signum == SIGINT) { msg = "\nInterrupt signal received\n"; } else { @@ -313,8 +315,6 @@ static void handler(int signum) return; } } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */
Comments are correct. We get here with SIGINT or SIGHUP/SIGTERM only when the transaction was not committing.
alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1);
Hi On Mon, Jun 23, 2014 at 9:39 AM, Allan McRae <allan@archlinux.org> wrote:
On 21/06/14 18:18, Silvan Jegen wrote:
One of the comments for this function is out of sync with the code. Since the code exhibits the more sane behavior of treating SIGINT and SIGHUB the same way (by not exiting pacman when there is a commit in flight) we adjust the comment.
Given this code flow, the if/else statements can be simplified somewhat as well.
Signed-off-by: Silvan Jegen <s.jegen@gmail.com>
There is nothing I see correct about this patch.
--- src/pacman/pacman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..c0903ee 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,7 +301,9 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { + /* SIGINT/SIGHUP: no committing transaction, release it now and then exit pacman
This comment makes little sense here. It was better where it was. See below.
+ * SIGTERM: release no matter what */
This should stay where it was. See below.
+ } else if(signum != SIGTERM) {
So SIGQUIT is handled like SIGINT and SIGHUP?
SIGQUIT is not handled by this function. See the handler setup at pacman.c: 1021-1037 .
if(signum == SIGINT) { msg = "\nInterrupt signal received\n"; } else { @@ -313,8 +315,6 @@ static void handler(int signum) return; } } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */
Comments are correct. We get here with SIGINT or SIGHUP/SIGTERM only when the transaction was not committing.
SIGTERM releases *without* checking the return value of alpm_trans_interrupt (which checks for a committing transaction I assume). With SIGINT or SIGHUP alpm_trans_interrupt's return value *will* be checked. So SIGINT and SIGHUP are being handled the same way, contrary to the comment. That's why I suggest to adjust the comment and simplify the else/if somewhat. If you prefer, I can send a patch to correct the comment without moving it or changing the if/else logic (though I think nested "if"s checking for the same condition twice are ugly and unnecessary).
alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1);
On 23/06/14 19:23, Silvan Jegen wrote:
Hi
On Mon, Jun 23, 2014 at 9:39 AM, Allan McRae <allan@archlinux.org> wrote:
On 21/06/14 18:18, Silvan Jegen wrote:
One of the comments for this function is out of sync with the code. Since the code exhibits the more sane behavior of treating SIGINT and SIGHUB the same way (by not exiting pacman when there is a commit in flight) we adjust the comment.
Given this code flow, the if/else statements can be simplified somewhat as well.
Signed-off-by: Silvan Jegen <s.jegen@gmail.com>
There is nothing I see correct about this patch.
--- src/pacman/pacman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..c0903ee 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,7 +301,9 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { + /* SIGINT/SIGHUP: no committing transaction, release it now and then exit pacman
This comment makes little sense here. It was better where it was. See below.
+ * SIGTERM: release no matter what */
This should stay where it was. See below.
+ } else if(signum != SIGTERM) {
So SIGQUIT is handled like SIGINT and SIGHUP?
SIGQUIT is not handled by this function. See the handler setup at pacman.c: 1021-1037 .
My memory is bad...
if(signum == SIGINT) { msg = "\nInterrupt signal received\n"; } else { @@ -313,8 +315,6 @@ static void handler(int signum) return; } } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */
Comments are correct. We get here with SIGINT or SIGHUP/SIGTERM only when the transaction was not committing.
SIGTERM releases *without* checking the return value of alpm_trans_interrupt (which checks for a committing transaction I assume). With SIGINT or SIGHUP alpm_trans_interrupt's return value *will* be checked. So SIGINT and SIGHUP are being handled the same way, contrary to the comment. That's why I suggest to adjust the comment and simplify the else/if somewhat.
Yes. Move SIGHUP please. I missed the comments were not moved verbatim.
If you prefer, I can send a patch to correct the comment without moving it or changing the if/else logic (though I think nested "if"s checking for the same condition twice are ugly and unnecessary).
I'll accept the refactor now you have pointed out the lack of other signal handling. Thanks, Allan
One of the comments for this function is out of sync with the code. Since the code exhibits the more sane behavior of treating SIGINT and SIGHUB the same way (by not exiting pacman when there is a commit in flight) we adjust the comment. Given this code flow, the if/else statements can be simplified somewhat as well. Signed-off-by: Silvan Jegen <s.jegen@gmail.com> --- src/pacman/pacman.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ef86d39..89852a3 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -301,7 +301,7 @@ static void handler(int signum) "Please submit a full bug report with --debug if appropriate.\n"; xwrite(err, msg, strlen(msg)); exit(signum); - } else if(signum == SIGINT || signum == SIGHUP) { + } else if(signum != SIGTERM) { if(signum == SIGINT) { msg = "\nInterrupt signal received\n"; } else { @@ -313,8 +313,8 @@ static void handler(int signum) return; } } - /* SIGINT: no committing transaction, release it now and then exit pacman - * SIGHUP, SIGTERM: release no matter what */ + /* SIGINT/SIGHUP: no committing transaction, release it now and then exit pacman + * SIGTERM: release no matter what */ alpm_trans_release(config->handle); /* output a newline to be sure we clear any line we may be on */ xwrite(out, "\n", 1); -- 2.0.0
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Silvan Jegen