[PATCH xorg-gtest 6/6] device: use inotify to wait for the device to appear

[Peter Hutterer]

On Wed, Aug 29, 2012 at 04:26:47PM -0700, Chase Douglas wrote:
> On 08/29/2012 03:25 PM, Peter Hutterer wrote:
> >On Wed, Aug 29, 2012 at 01:09:32PM -0700, Chase Douglas wrote:
> >>On 08/28/2012 11:14 PM, Peter Hutterer wrote:
> >>>uinput can be too slow, leaving us with GuessDeviceNode() failing and an
> >>>empty device string. Use inotify to tell us when a /dev/input/event device
> >>>appeared and then continue.  However, even when inotify tells us the device
> >>>is there, the EVIOCGNAME may still fail on the device, so we need to keep
> >>>the backup GuessDeviceNode() in place.
> >>>
> >>>This leaves us with a race condition - if a different device pops up while
> >>>we're waiting, then we may still not get the device name. Chance of that
> >>>happening is tiny.
> >>>
> >>>Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>>---
> >>>  src/device.cpp                     | 59 +++++++++++++++++++++++++++++++++++++-
> >>>  test/.gitignore                    |  1 +
> >>>  test/Makefile.am                   | 10 +++++--
> >>>  test/PIXART-USB-OPTICAL-MOUSE.desc | 41 ++++++++++++++++++++++++++
> >>>  test/device-test.cpp               | 33 +++++++++++++++++++++
> >>>  5 files changed, 141 insertions(+), 3 deletions(-)
> >>>  create mode 100644 test/PIXART-USB-OPTICAL-MOUSE.desc
> >>>  create mode 100644 test/device-test.cpp
> >>>
> >>>diff --git a/src/device.cpp b/src/device.cpp
> >>>index 232d3ae..ea98d17 100644
> >>>--- a/src/device.cpp
> >>>+++ b/src/device.cpp
> >>>@@ -31,6 +31,8 @@
> >>>  #include <linux/input.h>
> >>>  #include <fcntl.h>
> >>>  #include <dirent.h>
> >>>+#include <sys/inotify.h>
> >>>+#include <poll.h>
> >>>
> >>>  #include <stdexcept>
> >>>
> >>>@@ -110,6 +112,48 @@ void xorg::testing::evemu::Device::GuessDeviceNode(time_t ctime) {
> >>>    free(event_devices);
> >>>  }
> >>>
> >>>+static std::string wait_for_inotify(int fd)
> >>>+{
> >>>+  std::string devnode;
> >>>+  bool found = false;
> >>>+  struct pollfd pfd;
> >>>+
> >>>+  pfd.fd = fd;
> >>>+  pfd.events = POLLIN;
> >>>+
> >>>+  char buf[1024];
> >>>+  size_t bufidx = 0;
> >>>+
> >>>+  while (!found && poll(&pfd, 1, 2000) > 0) {
> >>>+    ssize_t r;
> >>>+
> >>>+    r = read(fd, buf + bufidx, sizeof(buf) - bufidx);
> >>>+    if (r == -1 && errno != EAGAIN) {
> >>>+      std::cerr << "inotify read failed with: " << std::string(strerror(errno)) << std::endl;
> >>>+      break;
> >>>+    }
> >>>+
> >>>+    bufidx += r;
> >>>+
> >>>+    struct inotify_event *e = reinterpret_cast<struct inotify_event*>(buf);
> >>>+
> >>>+    while (bufidx > sizeof(*e) && bufidx >= sizeof(*e) + e->len) {
> >>>+      if (strncmp(e->name, "event", 5) == 0) {
> >>>+        devnode = DEV_INPUT_DIR + std::string(e->name);
> >>>+        found = true;
> >>>+        break;
> >>>+      }
> >>>+
> >>>+      /* this packet didn't contain what we're looking for */
> >>>+      int len = sizeof(*e) + e->len;
> >>>+      memmove(buf, buf + len, bufidx - len);
> >>>+      bufidx -= len;
> >>>+    }
> >>>+  }
> >>>+
> >>>+  return devnode;
> >>>+}
> >>>+
> >>>  xorg::testing::evemu::Device::Device(const std::string& path)
> >>>      : d_(new Private) {
> >>>    static const char UINPUT_NODE[] = "/dev/uinput";
> >>>@@ -132,6 +176,14 @@ xorg::testing::evemu::Device::Device(const std::string& path)
> >>>
> >>>    fclose(fp);
> >>>
> >>>+  int ifd = inotify_init1(IN_NONBLOCK);
> >>>+  if (ifd == -1 || inotify_add_watch(ifd, DEV_INPUT_DIR, IN_CREATE) == -1) {
> >>>+    std::cerr << "Failed to create inotify watch" << std::endl;
> >>>+    if (ifd != -1)
> >>>+      close(ifd);
> >>>+    ifd = -1;
> >>>+  }
> >>>+
> >>>    d_->fd = open(UINPUT_NODE, O_WRONLY);
> >>>    if (d_->fd < 0) {
> >>>      evemu_delete(d_->device);
> >>>@@ -145,7 +197,12 @@ xorg::testing::evemu::Device::Device(const std::string& path)
> >>>      throw std::runtime_error("Failed to create evemu device");
> >>>    }
> >>>
> >>>-  GuessDeviceNode(d_->ctime);
> >>>+  if (ifd != -1) {
> >>>+    std::string devnode = wait_for_inotify(ifd);
> >>>+    if (event_is_device(devnode, evemu_get_name(d_->device), d_->ctime))
> >>>+        d_->device_node = devnode;
> >>>+    close(ifd);
> >>
> >>We could instead loop around here waiting until the correct device
> >>is seen, potentially with a timeout? I'm guessing you've already
> >>thought of that, so I'm interested to hear your thoughts.
> >
> >the problem here is the failed ioctl(EVIOCGNAME). inotify gives us the right
> >device, but because the ioctl fails we cannot be sure it really is our
> >device.
> >
> >the alternative here is to loop until the ioctl succeeds but I don't know if
> >that can hang the test, and putting a loop limit on it just means we'll need
> >GuessDeviceNode() again.
> >
> >you can try it, remove the GuessDeviceNode() call and run the new test in a
> >loop
> >
> >     while [ $? -eq 0 ]; do sudo ./device-test; done
> >
> >it'll run anywhere from a minute to several minutes before it fails.
> 
> Hmm... strange. Oh well. This shouldn't be a huge concern when
> running the tests.

actually, it is. that's why I started running the test as above. I hit the
issue in real test runs (i.e. make check) after around 30-40 tests, but
inconsistently. the while loop above was just a quicker way to reproduce,
and it ruled out any issues with the tests themselves.

> Reviewed-by: Chase Douglas <chase.douglas at canonical.com>

thanks!

Cheers,
   Peter