Re: RFS: triggerhappy

[Niels Thykier <niels@thykier.net>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 2010-12-20 17:08, Stefan Tomanek wrote:
> Dies schrieb Niels Thykier (niels@thykier.net):
> 
>> Though the upstream code might need a little attention (or some
>> clarification); e.g. cppcheck finds a resource leak:
>>
>> Checking cmdsocket.c...
>> [cmdsocket.c:126]: (error) Resource leak: fd
>>
>> Looking at the code I am not sure what the intention is. My best guess
>> is that it is trying to transmit the "leaked" file descriptor.
> 
> Yes, it does that. This allows the daemon itself to run as user "nobody",
> while th-cmd (launched by udev) can open a new device and pass the fd
> to thd.
> 

Okay, perhaps you could document this in the code that this is
intentional (e.g. transmitting the fd and the leak is a false-positive)?

>> [...]
>> cppcheck also believes that count_triggers (trigger.c) and print_ignores
>> (ignore.c) are never used. I did not verify that.
> 
> Indeed, I used those functions for debugging purposes only, but I guess
> leaving them there does no harm.
> 

There is no harm in leaving them; worst case the program is a bit larger
than needed (which could be solved with some #ifdef...#endif magic).
Anyhow just double checking it was intentional here. :)

>> In thd.c:331 the error message implies that the user does not exist, but
>> getpwnam can fail with other reasons; it might be a good idea to
>> use a more "neutral" error like:
>>   int err = errno;
>>   fprintf(stderr, "Could not look up the user %s\n", user);
>>   errno = err;
>>   perror("getpwnam");
> 
> OK, I "neutralized" the error message, however using perror(errno) from getpwnam
> is not really interesting, since all I get is "Sucess", even when specifying a completely
> bogus name.
> 

Mmm, reading getpwnam(3) I see that it may leave errno untouched (or set
it to 0; not quite sure) on a failure, but it is system/error dependent?

I guess you can do something like:
  errno = 0;
  var = getpwnam(...);
  if(!var){
      if(!errno){
         fprintf(stderr, "No such user %s\n", ...);
      } else {
        /*possible no such user or "other" issue*/
      }
  }

Or simply suppress the perror if errno is 0. Anyhow - not a strict
requirement.

>> Adding warnings to gcc I saw the following:
>> cc -g -O2 -Wall -Wextra -g -O2 -Wall -Wextra  -c -o eventnames.o
>> eventnames.c
>> In file included from eventtable.h:10,
>>                  from eventnames.c:5:
>> evtable_KEY.h:123: warning: initialized field overwritten
>> evtable_KEY.h:123: warning: (near initialization for 'KEY_NAME[122]')
> [...]
>> They are were triggered with -Wextra; it may be nothing, but I figured I
>> would mention it.
> 
> Yes, I was aware of those. evtable_{KEY,EV,SW} are autogenerated from <linux/input.h>,
> generating a mapping table from event names to the integer values, but since some events
> have multiple names, these messages appear.
> 

Cool, just double checking this as well since I was not sure :)

> Thanks for your review, I'll push the changed advocated by you to my git repository
> today (https://github.com/wertarbyte/triggerhappy).
> 
> 

You are welcome.

~Niels

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCAAGBQJND4U/AAoJEAVLu599gGRCjjkQAIzvwcIwjAOJqkNaH9yvsU5B
BmJdBxiVXBjgsYpH2fa0RNVlRCjwZCuA87O8OoZlbkPdcLxy8g5V8fSfR0wwK0gl
o4MqZxrBjbOdAeuFoQSyfsQ1wSAPJ5fXXN3dzOrVDl+FoQLkGfFrjn9n0bjRpkqX
+6muhjq5cBJBiWkpwgsRrfR1/kujTMt8PdS8RRmpBT7LXcnpreJEMk9zaG4UOT5b
Z1+YaNcW/rK6XDTCx5LLYWj6K/RZbND5H0e5xyqWN2MyJyepWh+fgg1kxAJKmcEJ
T4ixcB5qk4lpcllwjAgBhYdLmM5Mlgy8e2FTlf0UOVEdaUehiNIK3GCTJmKw3kWo
P8YlPZ2ZiwoHi/xNsTGlvYoLEWHbMLgU5KBz1pDX1PFsspmLhncR2fnspdPqmF+U
lvyOmlCyD9Wa8EMNA9Y/3sO9STlIgvipXtoEbbpvqAaLfBJph09IpRn46ZKJNY7F
Ef8t6UZcSkgwX70R4vb/piSns3KgoncGAuQoKbgZeSZfcJwJudV7ZZp3tRKD25fG
xVTH6ywOGwIesRyhmdn03nk25RwCPESjNqPzwpnxhwnHBWzYxa3eTrZ6W/qrgCCc
xHEsrMSxl9fy6EJLN0KZGk6OZOiPppPqk5Jypq7I4dAQenrXQ3jRpPXLSyDLC83m
DgUfTW3Ra96YNheQi6uK
=mWKg
-----END PGP SIGNATURE-----