[pacman-dev] [PATCH v2] pacman: Correct signal handler comment and refactor

[Silvan Jegen]

Hi

On Mon, Jun 23, 2014 at 9:39 AM, Allan McRae <allan at 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 at 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);
>>
>
>
>