[systemd-devel] [PATCH udev v2] udev: Add program/rule to export touchscreen/tablet size as udev properties

[Carlos Garnacho]

Hey Lennart,

On vie, 2014-12-19 at 17:20 +0100, Lennart Poettering wrote:
> On Fri, 19.12.14 16:01, Carlos Garnacho (carlosg at gnome.org) wrote:
> 
> > +
> > +#include <linux/input.h>
> > +#include "udev.h"
> > +
> > +/* Resolution is defined to be in units/mm for ABS_X/Y */
> > +#define ABS_SIZE_MM(absinfo) ((absinfo.maximum - absinfo.minimum) / absinfo.resolution)
> 
> Just out of principle: can you turn this into a function please? We
> try to avoid function-like macros that evaluate passed arguments
> multiple times, because of the risk of side effects.

Aha sure.

> 
> (Also, for macros like this, please always enclose the arguments you
> use in ())

Yup, that was an oversight, originally not in a #define at all.

<snip>
> Please size these arrays with the right length. We have
> DECIMAL_STR_MAX(int) as a macro to get the right size.

Ah, cheers! didn't notice that one.

> 
> > +        _cleanup_close_ int fd = -1;
> > +
> > +        if ((fd = open(devpath, O_RDONLY)) < 0)
> > +                return;
> 
> Please split this into two lines. Also out of principle, always use
> O_CLOEXEC (see CODING_STYLE for details):
> 
> fd = open(devpath, O_RDONLY|O_CLOEXEC);
> if (fd < 0) 
>      return;

Oh, makes sense.

Another update coming...

Cheers,
  Carlos