Re: RFS: triggerhappy

[Niels Thykier <niels@thykier.net>]

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

On 2010-12-18 15:10, Stefan Tomanek wrote:
> Dear mentors,
> 
> 
> [...]
> 
> Looking for a way of globally assigning hotkey handlers to the special keys
> found on my notebooks, I developed triggerhappy. I also use it to control
> headless systems, e.g. a ARM based MPD jukebox which is controlled by a USB
> keypad or bluetooth wiimote. Having this software in Debian would provide an
> easy way to bind commands to input events without the need for running an
> X11 session or logging in.
> 
> [...]
> I would be glad if someone uploaded this package for me.
> 
> Kind regards
>  Stefan Tomanek
> 
> 

Hey

I just did a quick review of your package; packaging-wise your package
looks good.

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.
  Though I am not convinced this will work; even if it is possible (I
vaguely recall seeing an extension in Linux to allow something like
this), I suspect the program relies on undefined behaviour here:

		char buffer[CMSG_SPACE(sizeof(dev_fd))];
		m.msg_control = buffer;

The issue is that buffer goes out of scope before m is used (in sendmsg).


In the if in uinput.c:27 it looks like you are missing a uinput_fd = -1;

In trigger.c:205 there is no check for if fork failed and this leads to
a "silent nothing happened". There are also got a number of unchecked
malloc/strdup, which could lead to "out of memory" segfaults, but at
least that is "some kind of feedback".

cppcheck also believes that count_triggers (trigger.c) and print_ignores
(ignore.c) are never used. I did not verify that.

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");


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]')
evtable_KEY.h:154: warning: initialized field overwritten
evtable_KEY.h:154: warning: (near initialization for 'KEY_NAME[152]')
evtable_KEY.h:379: warning: initialized field overwritten
evtable_KEY.h:379: warning: (near initialization for 'KEY_NAME[113]')
In file included from eventtable.h:14,
                 from eventnames.c:5:
evtable_SW.h:5: warning: initialized field overwritten
evtable_SW.h:5: warning: (near initialization for 'SW_NAME[3]')

They are were triggered with -Wextra; it may be nothing, but I figured I
would mention it.

~Niels

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

iQIcBAEBCAAGBQJND1SBAAoJEAVLu599gGRCmvcP/ivZpTqSIr210vAC2RY+Sb9+
dEjtT/QUGkxmBqK85yJ34uDsTciguKQ+JrQO0NouvaHiFjGMGFp/JI8oD0wKEf+n
ZCLMTWLp2C1Y0F6g6F6AdLpAijmD2kHVKeSbriFB9b6thjOC3VZBi/IycGO4oIiZ
Fg4TcF/4AcMov++YMJ9HN/ruXWdFrlHcjidYVW1ihnmyTFUW4HFtoYOFoj+kEYQX
0EUQ2cfJDArVkOLvWyxF62bAXryYEg0SvUkQH6Sqaybn9vdoQf3uv8XuGdNPhcPz
AvGq4U6SgHI655T5jHUn9JUO93IaYobm+ftoFgoo4zPd8+ufLsle4KgVyXTwBItR
BbTem2UgMqALKiL2uPhw8AtUZ2ObRIv3OTgWo0fJO1C0EzfeOVcEV73eKTVup2kZ
+aqTeHHal6eynV2+dswvHmpMB4WoK+IIquS5I6upPA/JEOWRtq0hVpa3rNR0p+y5
IE6aVToP9Usop1QzbwRj27iHHEZ+q+xAVv4uK8KdWy+AxH+fjiYrxl35N83BW//V
yNphRegV11NqjqgRx/z+WoE+8jT5r52JCgJts251z1ttiSo28DljXrvuO/UYHrn3
HFJI1Mh8yv0KfVTp8bCDkWJUIyeIKttVDp/qRGzXXOgSR9uM95xlP7zmWziQjTXX
CfSgd9/evkTcod/2yq9q
=Fjok
-----END PGP SIGNATURE-----