[PATCH xserver] inputthread: Re-add fd to the inputThreadInfo->fds pollfd set when re-added

[Hans de Goede]

Hi,

On 10/14/2016 06:38 AM, Peter Hutterer wrote:
> On Wed, Oct 12, 2016 at 04:55:08PM +0200, Hans de Goede wrote:
>> If the following call sequence happens:
>> 1) InputThreadUnregisterDev(fd)
>> 2) close(fd)
>> 3) fd = open(...) /* returns same fd as just closed */
>> 4) InputThreadRegisterDev(fd, ...)
>>
>> Without InputThreadDoWork(); running in the mean time, then we would
>> keep the closed fd in the inputThreadInfo->fds pollfd set, rather then
>> removing it and adding the new one, causing some devices to not work
>> after a vt-switch when using xf86-input-evdev.
>>
>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97880
>> Reported-and-tested-by: Mihail Konev <k.mvc at ya.ru>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  os/inputthread.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/os/inputthread.c b/os/inputthread.c
>> index ab1559f..c20c21c 100644
>> --- a/os/inputthread.c
>> +++ b/os/inputthread.c
>> @@ -49,6 +49,7 @@ Bool InputThreadEnable = TRUE;
>>
>>  typedef enum _InputDeviceState {
>>      device_state_added,
>> +    device_state_re_added,
>>      device_state_running,
>>      device_state_removed
>>  } InputDeviceState;
>> @@ -206,8 +207,8 @@ InputThreadRegisterDev(int fd,
>>      if (dev) {
>>          dev->readInputProc = readInputProc;
>>          dev->readInputArgs = readInputArgs;
>> -        /* Override possible unhandled state == device_state_removed */
>> -        dev->state = device_state_running;
>> +        if (dev->state == device_state_removed)
>> +            dev->state = device_state_re_added;
>
> I'm wondering, especially with the other patch in mind:
> https://patchwork.freedesktop.org/patch/113763/
>
> how about we just treat InputThreadDevice in state device_state_removed as
> "invisible" to InputThreadRegisterDev, i.e. we don't re-use the struct but
> allocate a new one. This way we have the old fd automatically removed and
> the new one added as expected, skipping the need for the
> device_state_re_added handling.
>
> This would be only a one-line change a bit above this hunk
> - if (old->fd == fd) {
> + if (old->fd == fd && dev->state != device_state_removed) {
>
> The only drawback is that we rely on xorg_list_append() and that the new
> entry is later than the previous one so we have the same remove/add order as
> in your device_state_re_added handling below. That needs a comment
> but other than that we should get the same result?

I agree that as long as we can somehow guarantee that the ospoll_remove
happens before the ospoll_add that we can then simply fix this by not
re-using the struct.

Regards,

Hans




>
> Cheers,
>    Peter
>
>
>>      } else {
>>          dev = calloc(1, sizeof(InputThreadDevice));
>>          if (dev == NULL) {
>> @@ -344,6 +345,16 @@ InputThreadDoWork(void *arg)
>>                      ospoll_listen(inputThreadInfo->fds, dev->fd, X_NOTIFY_READ);
>>                      dev->state = device_state_running;
>>                      break;
>> +                case device_state_re_added:
>> +                    /* Device might use a new fd with the same number */
>> +                    ospoll_remove(inputThreadInfo->fds, dev->fd);
>> +                    ospoll_add(inputThreadInfo->fds, dev->fd,
>> +                               ospoll_trigger_level,
>> +                               InputReady,
>> +                               dev);
>> +                    ospoll_listen(inputThreadInfo->fds, dev->fd, X_NOTIFY_READ);
>> +                    dev->state = device_state_running;
>> +                    break;
>>                  case device_state_running:
>>                      break;
>>                  case device_state_removed:
>> --
>> 2.9.3
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>