Don't read events unless required

With the previous approach, every libevdev_next_event() invocation triggered a
read() on the device fd. This is not efficient, the kernel provides whole
event frames at a time so we're guaranteed to have more events waiting unless
the current event is a SYN_REPORT.

Assuming a fast-enough client and e.g. a touchpad device with multiple axes
per frame, we'd thus trigger several unnecessary read() calls per event frame.

Drop this behavior, instead only trigger the read when our internal queue is
empty and we need more events.

Fallout:
- we don't have any warning about a too-slow sync, i.e. if a SYN_DROPPED
  arrives while we're syncing, we don't get a warning in the log anymore.
  the test for this was removed.
- the tests that required the specific behavior were rewritten accordingly
- a revoke on a kernel device doesn't return ENODEV until already-received
  events have been processed

The above shouldn't be an issue for existing real-world clients.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
index 761ba5a..c5798b4 100644 (file)
--- a/libevdev/libevdev.c
+++ b/libevdev/libevdev.c
@@ -1068,8 +1068,7 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
           read in any more.
         */
        do {
-               if (!(flags & LIBEVDEV_READ_FLAG_BLOCKING) ||
-                   queue_num_elements(dev) == 0) {
+               if (queue_num_elements(dev) == 0) {
                        rc = read_more_events(dev);
                        if (rc < 0 && rc != -EAGAIN)
                                goto out;
@@ -1101,15 +1100,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
        if (flags & LIBEVDEV_READ_FLAG_SYNC && dev->queue_nsync > 0) {
                dev->queue_nsync--;
                rc = LIBEVDEV_READ_STATUS_SYNC;
-               if (dev->queue_nsync == 0) {
-                       struct input_event next;
+               if (dev->queue_nsync == 0)
                        dev->sync_state = SYNC_NONE;
-
-                       if (queue_peek(dev, 0, &next) == 0 &&
-                           next.type == EV_SYN && next.code == SYN_DROPPED)
-                               log_info(dev, "SYN_DROPPED received after finished "
-                                        "sync - you're not keeping up\n");
-               }
        }
 
 out:

diff --git a/test/test-kernel.c b/test/test-kernel.c
index 1314d89..e17c693 100644 (file)
--- a/test/test-kernel.c
+++ b/test/test-kernel.c
@@ -61,15 +61,17 @@ START_TEST(test_revoke)
        uinput_device_event(uidev, EV_REL, REL_X, 1);
        uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
 
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev1);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
+       for (int i = 0; i < 2; i++) {
+               rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev1);
+               ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
 
-       rc = libevdev_next_event(dev2, LIBEVDEV_READ_FLAG_NORMAL, &ev2);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
+               rc = libevdev_next_event(dev2, LIBEVDEV_READ_FLAG_NORMAL, &ev2);
+               ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
 
-       ck_assert_int_eq(ev1.type, ev2.type);
-       ck_assert_int_eq(ev1.code, ev2.code);
-       ck_assert_int_eq(ev1.value, ev2.value);
+               ck_assert_int_eq(ev1.type, ev2.type);
+               ck_assert_int_eq(ev1.code, ev2.code);
+               ck_assert_int_eq(ev1.value, ev2.value);
+       }
 
        /* revoke first device, expect it closed, second device still open */
        dev_fd = libevdev_get_fd(dev);
@@ -81,6 +83,9 @@ START_TEST(test_revoke)
        }
        ck_assert_msg(rc == 0, "Failed to revoke device: %s", strerror(errno));
 
+       uinput_device_event(uidev, EV_REL, REL_X, 1);
+       uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
+
        rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev1);
        ck_assert_int_eq(rc, -ENODEV);
 

diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
index 165490d..5763d8a 100644 (file)
--- a/test/test-libevdev-events.c
+++ b/test/test-libevdev-events.c
@@ -147,11 +147,10 @@ START_TEST(test_syn_dropped_event)
 
        /* This is a bit complicated:
           we can't get SYN_DROPPED through uinput, so we push two events down
-          uinput, and fetch one off libevdev (reading in the other one on the
-          way). Then write a SYN_DROPPED on a pipe, switch the fd and read
-          one event off the wire (but returning the second event from
-          before). Switch back, so that when we do read off the SYN_DROPPED
-          we have the fd back on the device and the ioctls work.
+          uinput, and process those. Then write a SYN_DROPPED on a pipe,
+          switch the fd and read one event off the wire. Switch back, so
+          that when we do read off the SYN_DROPPED we have the fd back on
+          the device and the ioctls work.
         */
        uinput_device_event(uidev, EV_KEY, BTN_LEFT, 1);
        uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
@@ -159,81 +158,12 @@ START_TEST(test_syn_dropped_event)
        ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
        ck_assert_int_eq(ev.type, EV_KEY);
        ck_assert_int_eq(ev.code, BTN_LEFT);
-       rc = pipe2(pipefd, O_NONBLOCK);
-       ck_assert_int_eq(rc, 0);
 
-       libevdev_change_fd(dev, pipefd[0]);
-       ev.type = EV_SYN;
-       ev.code = SYN_DROPPED;
-       ev.value = 0;
-       rc = write(pipefd[1], &ev, sizeof(ev));
-       ck_assert_int_eq(rc, sizeof(ev));
        rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
-
-       libevdev_change_fd(dev, uinput_device_get_fd(uidev));
-
        ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
        ck_assert_int_eq(ev.type, EV_SYN);
        ck_assert_int_eq(ev.code, SYN_REPORT);
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
-       ck_assert_int_eq(ev.type, EV_SYN);
-       ck_assert_int_eq(ev.code, SYN_DROPPED);
-
-       /* only check for the rc, nothing actually changed on the device */
-
-       libevdev_free(dev);
-       uinput_device_free(uidev);
-
-       close(pipefd[0]);
-       close(pipefd[1]);
-
-}
-END_TEST
 
-void double_syn_dropped_logfunc(enum libevdev_log_priority priority,
-                               void *data,
-                               const char *file, int line,
-                               const char *func,
-                               const char *format, va_list args)
-{
-       unsigned int *hit = data;
-       *hit = 1;
-}
-
-START_TEST(test_double_syn_dropped_event)
-{
-       struct uinput_device* uidev;
-       struct libevdev *dev;
-       int rc;
-       struct input_event ev;
-       int pipefd[2];
-       unsigned int logfunc_hit = 0;
-
-       test_create_device(&uidev, &dev,
-                          EV_SYN, SYN_REPORT,
-                          EV_SYN, SYN_DROPPED,
-                          EV_REL, REL_X,
-                          EV_REL, REL_Y,
-                          EV_KEY, BTN_LEFT,
-                          -1);
-
-       libevdev_set_log_function(double_syn_dropped_logfunc,  &logfunc_hit);
-
-       /* This is a bit complicated:
-          we can't get SYN_DROPPED through uinput, so we push two events down
-          uinput, and fetch one off libevdev (reading in the other one on the
-          way). Then write a SYN_DROPPED on a pipe, switch the fd and read
-          one event off the wire (but returning the second event from
-          before). Switch back, so that when we do read off the SYN_DROPPED
-          we have the fd back on the device and the ioctls work.
-        */
-       uinput_device_event(uidev, EV_KEY, BTN_LEFT, 1);
-       uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
-       ck_assert_int_eq(ev.type, EV_KEY);
-       ck_assert_int_eq(ev.code, BTN_LEFT);
        rc = pipe2(pipefd, O_NONBLOCK);
        ck_assert_int_eq(rc, 0);
 
@@ -243,48 +173,14 @@ START_TEST(test_double_syn_dropped_event)
        ev.value = 0;
        rc = write(pipefd[1], &ev, sizeof(ev));
        ck_assert_int_eq(rc, sizeof(ev));
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
-
-       /* sneak in a button change event while we're not looking, this way
-        * the sync queue contains 2 events: BTN_LEFT and SYN_REPORT. */
-       uinput_device_event(uidev, EV_KEY, BTN_LEFT, 0);
-       ck_assert_int_eq(read(pipefd[0], &ev, sizeof(ev)), -1);
-
-       libevdev_change_fd(dev, uinput_device_get_fd(uidev));
 
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
-       ck_assert_int_eq(ev.type, EV_SYN);
-       ck_assert_int_eq(ev.code, SYN_REPORT);
        rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
        ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
        ck_assert_int_eq(ev.type, EV_SYN);
        ck_assert_int_eq(ev.code, SYN_DROPPED);
 
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
-       ck_assert_int_eq(ev.type, EV_KEY);
-       ck_assert_int_eq(ev.code, BTN_LEFT);
-       ck_assert_int_eq(ev.value, 0);
-
-       /* now write the second SYN_DROPPED on the pipe so we pick it up
-        * before we finish syncing. */
-       libevdev_change_fd(dev, pipefd[0]);
-       ev.type = EV_SYN;
-       ev.code = SYN_DROPPED;
-       ev.value = 0;
-       rc = write(pipefd[1], &ev, sizeof(ev));
-       ck_assert_int_eq(rc, sizeof(ev));
-
-       rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
-       ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
-       ck_assert_int_eq(ev.type, EV_SYN);
-       ck_assert_int_eq(ev.code, SYN_REPORT);
-       ck_assert_int_eq(ev.value, 0);
-
-       /* back to enable the ioctls again */
        libevdev_change_fd(dev, uinput_device_get_fd(uidev));
-
-       ck_assert_int_eq(logfunc_hit, 1);
+       /* only check for the rc, nothing actually changed on the device */
 
        libevdev_free(dev);
        uinput_device_free(uidev);
@@ -292,7 +188,6 @@ START_TEST(test_double_syn_dropped_event)
        close(pipefd[0]);
        close(pipefd[1]);
 
-       libevdev_set_log_function(test_logfunc_abort_on_error, NULL);
 }
 END_TEST
 
@@ -2149,7 +2044,6 @@ TEST_SUITE_ROOT_PRIVILEGES(libevdev_events)
        tcase_add_test(tc, test_next_event_invalid_fd);
        tcase_add_test(tc, test_next_event_blocking);
        tcase_add_test(tc, test_syn_dropped_event);
-       tcase_add_test(tc, test_double_syn_dropped_event);
        tcase_add_test(tc, test_event_type_filtered);
        tcase_add_test(tc, test_event_code_filtered);
        tcase_add_test(tc, test_has_event_pending);