[PATCH libevdev 3/4] memcpy instead of invidual bittoggle

[Benjamin Tissoires]

On Fri, Aug 30, 2013 at 3:06 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> The ioctls return the number of bytes copied into the destination, so just
> copy them into the device state instead of individually flipping bits.
>
> For easier review: rc is the return value of the EVIOCG* ioctl, which is
> the number of bytes copied.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---

Reviewed-by: Benjamin TIssoires <benjamin.tissoires at gmail.com>

Hi Peter,

well, it took me some time to review this, because I felt that something
weird could happen. I think I found a use case where thinks are broken
(but it's a weird one, and is not fixed either by the patch):

Let's imagine first that we introduce in the kernel a new range of keys,
big enough that NLONGS(KEY_CNT) is incremented.
Of course, in order to avoid recompilation if we run the same lib
against different kernels, we define those new KEY_* in libevdev. That
means that NLONGS(KEY_CNT) in libevdev is bigger than NLONGS(KEY_CNT) in
the kernel.

Now, let's look at the current code of sync_key_state():

---

static int
sync_key_state(struct libevdev *dev)
{
	int rc;
	int i;
	unsigned long keystate[NLONGS(KEY_CNT)];

// keystate may contain here garbage

	rc = ioctl(dev->fd, EVIOCGKEY(sizeof(keystate)), keystate);
	if (rc < 0)
		goto out;

// rc should be "sizeof(keystate) - sizeof(unsigned long)" if I am not
// wrong

	for (i = 0; i < KEY_CNT; i++) {
		int old, new;
		old = bit_is_set(dev->key_values, i);
		new = bit_is_set(keystate, i);

// "new" for bits in range [NLONGS(KEY_CNT) - 2 .. NLONGS(KEY_CNT) -1]
// is random

		if (old ^ new) {
			struct input_event *ev = queue_push(dev);
			init_event(dev, ev, EV_KEY, i, new ? 1 : 0);

// we are sending random events

		}
		set_bit_state(dev->key_values, i, new);
	}

	rc = 0;
out:
	return rc ? -errno : 0;
}

---

Actually, what is bothering me, is that we don't take into account the
return value "rc" before doing the for loop.

Would it cost too much to do:

  unsigned long keystate[NLONGS(KEY_CNT)] = {0};

in term of performance? Of course, if that fixes this problem. (and same
goes for each other of the sync_*_state()).

Cheers,
Benjamin

>  libevdev/libevdev.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index 73405bc..9b37f70 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -320,9 +320,10 @@ sync_key_state(struct libevdev *dev)
>                         struct input_event *ev = queue_push(dev);
>                         init_event(dev, ev, EV_KEY, i, new ? 1 : 0);
>                 }
> -               set_bit_state(dev->key_values, i, new);
>         }
>
> +       memcpy(dev->key_values, keystate, rc);
> +
>         rc = 0;
>  out:
>         return rc ? -errno : 0;
> @@ -347,9 +348,10 @@ sync_sw_state(struct libevdev *dev)
>                         struct input_event *ev = queue_push(dev);
>                         init_event(dev, ev, EV_SW, i, new ? 1 : 0);
>                 }
> -               set_bit_state(dev->sw_values, i, new);
>         }
>
> +       memcpy(dev->sw_values, swstate, rc);
> +
>         rc = 0;
>  out:
>         return rc ? -errno : 0;
> @@ -374,9 +376,10 @@ sync_led_state(struct libevdev *dev)
>                         struct input_event *ev = queue_push(dev);
>                         init_event(dev, ev, EV_LED, i, new ? 1 : 0);
>                 }
> -               set_bit_state(dev->led_values, i, new);
>         }
>
> +       memcpy(dev->led_values, ledstate, rc);
> +
>         rc = 0;
>  out:
>         return rc ? -errno : 0;
> --
> 1.8.2.1
>
> _______________________________________________
> Input-tools mailing list
> Input-tools at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/input-tools