[PATCH libinput 1/9] evdev: Dynamically allocate slot array
Peter Hutterer
peter.hutterer at who-t.net
Wed Apr 9 23:43:38 PDT 2014
On Thu, Apr 10, 2014 at 08:17:37AM +0200, Jonas Ådahl wrote:
> On Thu, Apr 10, 2014 at 03:43:25PM +1000, Peter Hutterer wrote:
> > On Wed, Apr 09, 2014 at 09:02:08PM +0200, Jonas Ådahl wrote:
> > > Don't have a hard coded (previously 16) slot array size; instead
> > > allocate dynamically depending what slots are assigned. There is still a
> > > hard coded max though, to protect from invalid input, but its changed
> > > to 60.
> > >
> > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > ---
> > > src/evdev.c | 82 +++++++++++++++++++++++++++++++++++++++++++++----------------
> > > src/evdev.h | 13 ++++++----
> > > 2 files changed, 69 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/src/evdev.c b/src/evdev.c
> > > index 72e4086..cc0c4bf 100644
> > > --- a/src/evdev.c
> > > +++ b/src/evdev.c
> > > @@ -280,6 +280,36 @@ evdev_process_key(struct evdev_device *device, struct input_event *e, int time)
> > > }
> > > }
> > >
> > > +static int
> > > +alloc_mt_slot_states(struct evdev_device *device, int len)
> >
> > I'd prefer this to be size_t, and nslots instead of len to be more obvious
> > re:malloc.
>
> To use size_t here I had to change cast size_t to a signed integer (or
> other way around), but sure, I'll make the change.
maybe ssize_t? either way, I'm more concerned about len vs nslots, the
int/size_t doesn't matter that much.
>
> >
> > > +{
> > > + struct mt_slot *slots = device->mt.slots;
> > > + int i;
> > > +
> > > + slots = realloc(slots, sizeof(struct mt_slot) * len);
> > > + if (!slots)
> > > + return -1;
> > > +
> > > + for (i = device->mt.slots_len; i < len; ++i) {
> > > + slots[i].seat_slot = -1;
> > > + slots[i].x = 0;
> > > + slots[i].y = 0;
> > > + }
> > > + device->mt.slots = slots;
> > > + device->mt.slots_len = len;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void
> > > +set_active_slot(struct evdev_device *device, int32_t slot)
> > > +{
> > > + slot = min(slot, MAX_SLOTS);
> > > + if (slot >= device->mt.slots_len)
> > > + alloc_mt_slot_states(device, device->mt.slots_len * 2);
> > > + device->mt.slot = min(slot, device->mt.slots_len - 1);
> > > +}
> >
> > couldn't we just do alloc_mt_slots(dev, libevdev_num_slots(evdev)) during
> > configure? the kernel filters events that are outside of the announced slot
> > range, libevdev filters events outside of the slot range so this code seems
> > to churn a lot for no reason.
> >
> > libevdev 1.2 also gets rid of the MAX_SLOTS define, so we don't need this
> > limit anymore.
>
> MAX_SLOTS is defined in libinput/src/evdev.h. I guess we could allocate
> less ad-hoc; until now I've kept MAX_SLOTS check to protect against
> potentially misbehaving driver (if a driver lazilly advertises max slots
> to some very high number for example). Will libevdev protect against
> this?
no, it doesn't. OTOH we could assume that if this happens it's a kernel bug
and should be fixed there.
Cheers,
Peter
>
> >
> > > +
> > > static void
> > > evdev_process_touch(struct evdev_device *device,
> > > struct input_event *e,
> > > @@ -288,7 +318,7 @@ evdev_process_touch(struct evdev_device *device,
> > > switch (e->code) {
> > > case ABS_MT_SLOT:
> > > evdev_flush_pending_event(device, time);
> > > - device->mt.slot = e->value;
> > > + set_active_slot(device, e->value);
> > > break;
> > > case ABS_MT_TRACKING_ID:
> > > if (device->pending_event != EVDEV_NONE &&
> > > @@ -543,9 +573,11 @@ evdev_device_dispatch(void *data)
> > > static int
> > > evdev_configure_device(struct evdev_device *device)
> > > {
> > > + struct libevdev *evdev = device->evdev;
> >
> > tbh, I'd have preferred this to be a separate change. without this change,
> > the next hunks would only be 3 lines or so and much easier to review.
>
> Sure, I'll split it out.
>
> >
> > Cheers,
> > Peter
> >
> >
> > > const struct input_absinfo *absinfo;
> > > int has_abs, has_rel, has_mt;
> > > int has_button, has_keyboard, has_touch;
> > > + int active_slot;
> > > unsigned int i;
> > >
> > > has_rel = 0;
> > > @@ -555,14 +587,14 @@ evdev_configure_device(struct evdev_device *device)
> > > has_keyboard = 0;
> > > has_touch = 0;
> > >
> > > - if (libevdev_has_event_type(device->evdev, EV_ABS)) {
> > > + if (libevdev_has_event_type(evdev, EV_ABS)) {
> > >
> > > - if ((absinfo = libevdev_get_abs_info(device->evdev, ABS_X))) {
> > > + if ((absinfo = libevdev_get_abs_info(evdev, ABS_X))) {
> > > device->abs.min_x = absinfo->minimum;
> > > device->abs.max_x = absinfo->maximum;
> > > has_abs = 1;
> > > }
> > > - if ((absinfo = libevdev_get_abs_info(device->evdev, ABS_Y))) {
> > > + if ((absinfo = libevdev_get_abs_info(evdev, ABS_Y))) {
> > > device->abs.min_y = absinfo->minimum;
> > > device->abs.max_y = absinfo->maximum;
> > > has_abs = 1;
> > > @@ -570,56 +602,58 @@ evdev_configure_device(struct evdev_device *device)
> > > /* We only handle the slotted Protocol B in weston.
> > > Devices with ABS_MT_POSITION_* but not ABS_MT_SLOT
> > > require mtdev for conversion. */
> > > - if (libevdev_has_event_code(device->evdev, EV_ABS, ABS_MT_POSITION_X) &&
> > > - libevdev_has_event_code(device->evdev, EV_ABS, ABS_MT_POSITION_Y)) {
> > > - absinfo = libevdev_get_abs_info(device->evdev, ABS_MT_POSITION_X);
> > > + if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) &&
> > > + libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) {
> > > + absinfo = libevdev_get_abs_info(evdev, ABS_MT_POSITION_X);
> > > device->abs.min_x = absinfo->minimum;
> > > device->abs.max_x = absinfo->maximum;
> > > - absinfo = libevdev_get_abs_info(device->evdev, ABS_MT_POSITION_Y);
> > > + absinfo = libevdev_get_abs_info(evdev, ABS_MT_POSITION_Y);
> > > device->abs.min_y = absinfo->minimum;
> > > device->abs.max_y = absinfo->maximum;
> > > device->is_mt = 1;
> > > has_touch = 1;
> > > has_mt = 1;
> > >
> > > - if (!libevdev_has_event_code(device->evdev, EV_ABS, ABS_MT_SLOT)) {
> > > + if (!libevdev_has_event_code(evdev,
> > > + EV_ABS, ABS_MT_SLOT)) {
> > > device->mtdev = mtdev_new_open(device->fd);
> > > if (!device->mtdev)
> > > return -1;
> > > - device->mt.slot = device->mtdev->caps.slot.value;
> > > + active_slot = device->mtdev->caps.slot.value;
> > > } else {
> > > - device->mt.slot = libevdev_get_current_slot(device->evdev);
> > > + active_slot = libevdev_get_current_slot(evdev);
> > > }
> > > + set_active_slot(device, active_slot);
> > > }
> > > }
> > > - if (libevdev_has_event_code(device->evdev, EV_REL, REL_X) ||
> > > - libevdev_has_event_code(device->evdev, EV_REL, REL_Y))
> > > + if (libevdev_has_event_code(evdev, EV_REL, REL_X) ||
> > > + libevdev_has_event_code(evdev, EV_REL, REL_Y))
> > > has_rel = 1;
> > >
> > > - if (libevdev_has_event_type(device->evdev, EV_KEY)) {
> > > - if (libevdev_has_event_code(device->evdev, EV_KEY, BTN_TOOL_FINGER) &&
> > > - !libevdev_has_event_code(device->evdev, EV_KEY, BTN_TOOL_PEN) &&
> > > + if (libevdev_has_event_type(evdev, EV_KEY)) {
> > > + if (libevdev_has_event_code(evdev, EV_KEY, BTN_TOOL_FINGER) &&
> > > + !libevdev_has_event_code(evdev, EV_KEY, BTN_TOOL_PEN) &&
> > > (has_abs || has_mt)) {
> > > device->dispatch = evdev_mt_touchpad_create(device);
> > > }
> > > for (i = KEY_ESC; i < KEY_MAX; i++) {
> > > if (i >= BTN_MISC && i < KEY_OK)
> > > continue;
> > > - if (libevdev_has_event_code(device->evdev, EV_KEY, i)) {
> > > + if (libevdev_has_event_code(evdev, EV_KEY, i)) {
> > > has_keyboard = 1;
> > > break;
> > > }
> > > }
> > > - if (libevdev_has_event_code(device->evdev, EV_KEY, BTN_TOUCH))
> > > + if (libevdev_has_event_code(evdev, EV_KEY, BTN_TOUCH))
> > > has_touch = 1;
> > > for (i = BTN_MISC; i < BTN_JOYSTICK; i++) {
> > > - if (libevdev_has_event_code(device->evdev, EV_KEY, i)) {
> > > + if (libevdev_has_event_code(evdev, EV_KEY, i)) {
> > > has_button = 1;
> > > break;
> > > }
> > > }
> > > }
> > > - if (libevdev_has_event_type(device->evdev, EV_LED))
> > > + if (libevdev_has_event_type(evdev, EV_LED))
> > > has_keyboard = 1;
> > >
> > > if ((has_abs || has_rel) && has_button)
> > > @@ -670,7 +704,6 @@ evdev_device_create(struct libinput_seat *seat,
> > > device->mtdev = NULL;
> > > device->devnode = strdup(devnode);
> > > device->sysname = strdup(sysname);
> > > - device->mt.slot = -1;
> > > device->rel.dx = 0;
> > > device->rel.dy = 0;
> > > device->dispatch = NULL;
> > > @@ -680,6 +713,12 @@ evdev_device_create(struct libinput_seat *seat,
> > >
> > > libinput_seat_ref(seat);
> > >
> > > + device->mt.slot = -1;
> > > + device->mt.slots = NULL;
> > > + device->mt.slots_len = 0;
> > > + if (alloc_mt_slot_states(device, 4) == -1)
> > > + goto err;
> > > +
> > > if (evdev_configure_device(device) == -1)
> > > goto err;
> > >
> > > @@ -786,6 +825,7 @@ evdev_device_destroy(struct evdev_device *device)
> > >
> > > libinput_seat_unref(device->base.seat);
> > > libevdev_free(device->evdev);
> > > + free(device->mt.slots);
> > > free(device->devnode);
> > > free(device->sysname);
> > > free(device);
> > > diff --git a/src/evdev.h b/src/evdev.h
> > > index 0ab9572..4ec387a 100644
> > > --- a/src/evdev.h
> > > +++ b/src/evdev.h
> > > @@ -31,7 +31,7 @@
> > >
> > > #include "libinput-private.h"
> > >
> > > -#define MAX_SLOTS 16
> > > +#define MAX_SLOTS 60
> > >
> > > enum evdev_event_type {
> > > EVDEV_NONE,
> > > @@ -50,6 +50,11 @@ enum evdev_device_seat_capability {
> > > EVDEV_DEVICE_TOUCH = (1 << 2)
> > > };
> > >
> > > +struct mt_slot {
> > > + int32_t seat_slot;
> > > + int32_t x, y;
> > > +};
> > > +
> > > struct evdev_device {
> > > struct libinput_device base;
> > >
> > > @@ -74,10 +79,8 @@ struct evdev_device {
> > >
> > > struct {
> > > int slot;
> > > - struct {
> > > - int32_t seat_slot;
> > > - int32_t x, y;
> > > - } slots[MAX_SLOTS];
> > > + struct mt_slot *slots;
> > > + int slots_len;
> > > } mt;
> > > struct mtdev *mtdev;
> > >
> > > --
> > > 1.8.3.2
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > >
More information about the wayland-devel
mailing list