libevdev: flushing state forcefully

[David Herrmann]

Hi

On Wed, Aug 27, 2014 at 8:38 PM, Ran Benita <ran234 at gmail.com> wrote:
> On Wed, Aug 27, 2014 at 01:38:31PM +0200, David Herrmann wrote:
>> Hi
>>
>> I'm hacking on some xkb stuff again and stumbled across some
>> inconsistencies in libevdev state handling. In particular, there is
>> currently no easy way to parse initial evdev state when opening a
>> device. Imagine the following setup:
>>
>> 1) ctrl+alt+F2 is pressed, a session switch to session #2 occurs. The
>> session is activated the first time, therefore, it opens evdev devices
>> (either directly or via TakeDevice on logind) and initializes its
>> libevdev state.
>>
>> 2) F2 button is released.
>>
>> 3) F3 button is pressed. No session switch occurs, as the libevdev has
>> not generated any event for the ctrl and alt buttons, thus, the xkb
>> state does not know of them.
>
> [So implicit here is that you sync the xkb state to be like the evdev
> state, rather than the other way around?]

Yes!

>> If the session has already been loaded and you switch to it, we
>> usually force a libevdev resync via LIBEVDEV_READ_FLAG_FORCE_SYNC.
>> Therefore, the new modifiers are updated in xkb. However, during
>> device setup, there is no way to get events for the initial state.
>>
>> Two solutions:
>>
>> 1) Iterate over all the available types+codes and generate events manually.
>> 2) Don't sync initial state during setup. To keep backwards-compat,
>>    libevdev_set_fd_no_sync() would be needed (name subject
>>    to change, obviously).
>
> If such a function is added, I'd recommend it'd be a libevdev_set_fd()
> variant which accepts a flags argument, instead of _no_sync().

Of course, this was what I meant with "name is subject to change".
Sorry, that wasn't really clear. I don't care what type of API we add,
with _no_sync() I was just referring to "any kind of API" we might
add.

>> Now, solution 1) obviously sounds easier as there is no need to
>> introduce a new API. However, there's one more issue: Imagine you
>> change the xkb-keymap of an in-flight session. We have to destroy the
>> xkb_state object and recreate it for the new keymap. However, this
>> looses all state and we need to resync it. This is impossible to
>> achieve with libevdev, as it assumes we already parsed the state.
>>
>> Again, two solutions:
>>
>> 1) Iterate over all the available types+codes and generate
>>    events for them.
>> 2) Destroy the libevdev object and recreate it via
>>    libevdev_set_fd_no_sync() and sync yourself.
>>
>> Merging both problems, I see the following proper solutions:
>>
>> 1) Let applications deal with it.
>>
>> 2) Add LIBEVDEV_FOREACH_CODE() which is a convenience
>>    for-loop that iterates all available type+code combinations
>>    of the libevdev device. This can be easily used to generate
>>    required events in the application itself.
>>
>> 3) Add libevdev_reset_state(). This flushes all internal event
>>    state of an libevdev object. A following forced SYNC will
>>    then properly resync the device.
>>    This probably also implies a libevdev_set_fd_no_sync() so
>>    we avoid the initial state sync.
>
> I'm not very familiar with libevdev, but FWIW:
>
> Solution 3 seems cleanest to me, especially since libevdev already has
> the sync_key_state() logic, so why not reuse it? Also, you don't lose
> any flexibility you'd get with a FOREACH, since you can know which
> events are "fake" (from the libevdev_next_event() retval).

Me too. Especially, I cannot see much difference in initial syncing
and syncing due to SYN_DROPPED. In both cases the events are reported
out-of-order.

> But is there a reason to add libevdev_reset_state() instead of
> recreating, using libevdev_set_fd_no_sync(), then FORCE_SYNC?

Well, re-creating state is heavy. It's not like xkb_state were you
just need to malloc(). For libevdev, we retrieve all the device
information from the kernel, calling lots of syscalls. We could split
libevdev into a device-object and a state object, but I don't think
it's worth it.

Long story short: I'd be fine with recreating the state, but I
strongly vote for the fast-path that avoids the heavy re-creation.

> [Aside: from a naive look at sync_key_state(), it looks like there might
> be a NULL deref, since queue_push() can return NULL. Don't know if that
> can actually happen though.]

The queue is guaranteed to be big enough for all events during a
re-sync (we count enabled bits and only flush those events). It would
hurt to add an assert()-like bail-out path, though.

Thanks for the comments!
David