[systemd-devel] [PATCH 1/5] udev-builtin-input_id: identify trackpoints and accelerometers

[Hans de Goede]

Hi,

On 02-04-15 12:21, Lennart Poettering wrote:
> On Thu, 02.04.15 11:48, Hans de Goede (hdegoede at redhat.com) wrote:
>
>> The kernel has been setting the INPUT_PROP_POINTING_STICK property bit
>> on trackpoints for a while now, and this is useful information to have
>> in various places, so make input_id aware of this and make it set
>> ID_INPUT_POINTING_STICK on trackpoints.
>>
>> While adding support for querying properties, also add support for the
>> recently added INPUT_PROP_ACCELEROMETER property bit.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>   src/udev/udev-builtin-input_id.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/udev/udev-builtin-input_id.c b/src/udev/udev-builtin-input_id.c
>> index 46f1c53..5b1790c 100644
>> --- a/src/udev/udev-builtin-input_id.c
>> +++ b/src/udev/udev-builtin-input_id.c
>> @@ -33,6 +33,14 @@
>>   #include "udev.h"
>>   #include "util.h"
>>
>> +#ifndef INPUT_PROP_POINTING_STICK
>> +#define INPUT_PROP_POINTING_STICK       0x05
>> +#endif
>> +
>> +#ifndef INPUT_PROP_ACCELEROMETER
>> +#define INPUT_PROP_ACCELEROMETER        0x06
>> +#endif
>
> Are these definitions normally defined in input.h?

Yes.

> If so, please add them to missing.h, where we try to centralize definitions of this kind.

Ok, will fix for v2.

>>   /* we must use this kernel-compatible implementation */
>>   #define BITS_PER_LONG (sizeof(unsigned long) * 8)
>>   #define NBITS(x) ((((x)-1)/BITS_PER_LONG)+1)
>> @@ -131,6 +139,7 @@ static void test_pointers (struct udev_device *dev,
>>                              const unsigned long* bitmask_abs,
>>                              const unsigned long* bitmask_key,
>>                              const unsigned long* bitmask_rel,
>> +                           unsigned long prop,
>
> "unsigned long"? Is this really necessary? Shouldn't we just use
> uint64_t? here?

unsigned long matches what is used in the kernel, it is a bit field,
I do not know what type is preferred for bit fields in systemd / udev.

>
>>                              bool test) {
>>           int is_mouse = 0;
>>           int is_touchpad = 0;
>> @@ -182,6 +191,10 @@ static void test_pointers (struct udev_device *dev,
>>                   udev_builtin_add_property(dev, test, "ID_INPUT_MOUSE", "1");
>>           if (is_touchpad)
>>                   udev_builtin_add_property(dev, test, "ID_INPUT_TOUCHPAD", "1");
>> +        if (prop & (1 << INPUT_PROP_POINTING_STICK))
>> +                udev_builtin_add_property(dev, test, "ID_INPUT_TRACKPOINT", "1");
>> +        if (prop & (1 << INPUT_PROP_ACCELEROMETER))
>> +                udev_builtin_add_property(dev, test, "ID_INPUT_ACCELEROMETER", "1");
>>   }
>>
>>   /* key like devices */
>> @@ -232,7 +245,8 @@ static int builtin_input_id(struct udev_device *dev, int argc, char *argv[], boo
>>           unsigned long bitmask_abs[NBITS(ABS_MAX)];
>>           unsigned long bitmask_key[NBITS(KEY_MAX)];
>>           unsigned long bitmask_rel[NBITS(REL_MAX)];
>> -        const char *sysname, *devnode;
>> +        unsigned long prop = 0;
>> +        const char *sysname, *devnode, *prop_str;
>>
>>           /* walk up the parental chain until we find the real input device; the
>>            * argument is very likely a subdevice of this, like eventN */
>> @@ -248,7 +262,10 @@ static int builtin_input_id(struct udev_device *dev, int argc, char *argv[], boo
>>                   get_cap_mask(dev, pdev, "capabilities/abs", bitmask_abs, sizeof(bitmask_abs), test);
>>                   get_cap_mask(dev, pdev, "capabilities/rel", bitmask_rel, sizeof(bitmask_rel), test);
>>                   get_cap_mask(dev, pdev, "capabilities/key", bitmask_key, sizeof(bitmask_key), test);
>> -                test_pointers(dev, bitmask_ev, bitmask_abs, bitmask_key, bitmask_rel, test);
>> +                prop_str = udev_device_get_property_value(pdev, "PROP");
>> +                if (prop_str)
>> +                       prop = strtoul(prop_str, NULL, 16);
>
> Hmm, we try to avoid direct invocations of strtoul() and friends, due
> to the annoying error handling... Can't we use safe_atou64() here, or so?

If that can handle hex without a 0x prefix then yes.

Regards,

Hans