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