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

[Peter Hutterer]

On Fri, Aug 30, 2013 at 10:30:27AM +0200, Benjamin Tissoires wrote:
> 
> 
> 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()).

good catch. I felt something wasn't right with this patch but just couldn't
put myu finger on it. I've amended as you requested, with a note in the
commit message. thanks for the review.

I don't think performance is a issue here, though I haven't measured the
impact.

Cheers,
   Peter

> 
> 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