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

[Allan McRae]

On 23/06/14 19:23, Silvan Jegen wrote:
> 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 .
> 

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