Re: [PATCH] Input: evdev - add event-mask API

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Tue, Apr 22, 2014 at 6:29 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
>> +/* requires the buffer lock to be held */
>> +static bool __evdev_is_masked(struct evdev_client *client,
>> +                           unsigned int type,
>> +                           unsigned int code)
>> +{
>> +     unsigned long *mask;
>> +     size_t cnt;
>> +
>> +     /* EV_SYN and unknown codes are never masked */
>> +     if (!type || type >= EV_CNT)
>
> why not use type == EV_SYN?

You mean instead of "!type"? Yeah, probably easier to read.

>> +             return false;
>> +
>> +     /* first test whether the type is masked */
>> +     mask = client->evmasks[0];
>
> if mask is NULL, you already know it's not mask, you can return early.

Nope. If evmasks[0] is NULL, then no type-mask has been set. That
doesn't mean the given code-mask is NULL.

For instance:
  evmasks[0] == NULL
  evmasks[EV_KEY][KEY_A] == 0

In that case, the user wants all events but KEY_A (even though
evmasks[0] is NULL).

>>  #define EVIOCGRAB            _IOW('E', 0x90, int)                    /* Grab/Release device */
>>  #define EVIOCREVOKE          _IOW('E', 0x91, int)                    /* Revoke device access */
>> +#define EVIOCGMASK           _IOR('E', 0x92, struct input_mask)      /* Get event-masks */
>> +#define EVIOCSMASK           _IOW('E', 0x93, struct input_mask)      /* Set event-masks */
>
> This is missing from all other ioctls but while you're adding a new one
> anyway: please add documentation on what the ioctl does, the input and
> return value/output expected, side-effects etc. right now, understanding the
> evdev ioctls requires either reading the kernel code or existing user-space
> code, with the usual risk of getting it wrong.

Meh.. I thought no-one will notice.. Clearly, I was wrong. I will add
proper docs in v2.

Thanks a lot!
David