[PATCH libevdev] Document that the fd should be drained before libevdev_set_fd

[Peter Hutterer]

On Wed, Dec 16, 2015 at 11:58:32AM +0100, David Herrmann wrote:
> Hi
> 
> On Wed, Dec 16, 2015 at 1:39 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > This is the caller's responsibility, for two reasons:
> > * we don't know if O_NONBLOCK is set, so draining the fd isn't a simple matter
> >   of read() until EAGAIN. A select() + read() could work around this of
> >   course.
> > * for stateless information, keys and relative data, it is not a problem when
> >   there are events waiting on the fd already, they are processed correctly,
> >   albeit with a delay.
> >
> > So punt this decision to the caller, they openend the fd, they know if they
> > care about delayed events, they can drain the fd before handing it to us.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  libevdev/libevdev.h | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> > index eae8e7b..27e36d8 100644
> > --- a/libevdev/libevdev.h
> > +++ b/libevdev/libevdev.h
> > @@ -78,7 +78,8 @@ extern "C" {
> >   *
> >   * libevdev does not handle the file descriptors directly, it merely uses
> >   * them. The caller is responsible for opening the file descriptors, setting
> > - * them to O_NONBLOCK and handling permissions.
> > + * them to O_NONBLOCK and handling permissions. A caller should drain any
> > + * events pending on the file descriptor before passing it to libevdev.
> >   *
> >   * Where does libevdev sit?
> >   * ========================
> > @@ -974,6 +975,15 @@ int libevdev_grab(struct libevdev *dev, enum libevdev_grab_mode grab);
> >   * you need to change the fd after closing and re-opening the same device, use
> >   * libevdev_change_fd().
> >   *
> > + * A caller should ensure that any events currently pending on the fd are
> > + * drained before the file descriptor is passed to libevdev for
> > + * initialization. Due to how the kernel's ioctl handling works, the initial
> > + * device state will reflect the current device state *after* applying all
> > + * events currently pending on the fd. Thus, if the fd is not drained, the
> > + * state visible to the caller will be inconsistent with the events
> > + * immediately available on the device. This does not affect state-less
> > + * events like EV_REL.
> > + *
> 
> Care to elaborate on this? Even if the caller drains the FD, there
> might be events coming in _before_ you can pass it to libevdev. I can
> see that the draining works as a barrier, but I cannot see how your
> comment addresses that. TBH, I'm not entirely sure what your comment
> is trying to say.

the specific use-case that triggered this was the X case where we open the
fd once on server start, then keep it open until shutdown/VT switch. if a
device is disabled and re-enabled the fd isn't opened again, it is kept
open. the driver doesn't necessarily know about that though, it just gets
an fd and initializes the libevdev device. So events that happen between
disabling and enabling the device are available on fd immediately when they
shouldn't be.

libevdev doesn't know this is happening and there's an argument that it
shouldn't know. hence the documentation update only, the xorg drivers (and
libinput, in this case) need to be updated to drain anything still sitting
there.

> I can imagine a situation where an ABS event, followed by a
> slot-change is pending. If you now open the FD and initialize
> libevdev, then it will have the SLOT event already parsed, as such the
> pending ABS event is wrongly attributed. Is this what this is
> referring to? Because if it is, you cannot solve it by draining the
> fd. The race is still there. The solution would rather be to force the
> kernel to flush the input-queue if you query device state (which is
> already partly done by the kernel).

it's done partially, but some events get through. simple use-case is
   xinput disable <mouse device>
   <move the mouse a bit>
   xinput enable <mouse device>

you'll see the cursor jump based on the movement while the device was
disabled. quick testing yesterday showed that this doesn't apply to key
events (though MSC_SCAN still gets through).

either way, it's a race condition, so we can't get rid of it completely.
but there's a definitive line of "this event happened before we even got the
fd" - those events are usually wrong and should be drained by the caller.

Cheers,
   Peter