[PATCH 1/2] xfree86: use a thread for the generation of input events

[Fernando Carrijo]

Adam Jackson <ajax at redhat.com> wrote:

[...]

> +/**
> + * Unregister a device in the threaded input facility
> + *
> + * @param fd File descriptor which identifies the input device
> + *
> + * @return 1 if success; 0 otherwise.
> + */
> +int
> +InputThreadUnregisterDev(int fd)
> +{
> +    InputThreadDevice *prev, *dev;
> +
> +
> +    /* return silently if input thread is already finished (e.g., at
> +     * DisableDevice time, evdev tries to call this function again through
> +     * xf86RemoveEnabledDevice */
> +    if (!inputThreadInfo)
> +        return 0;
> +
> +    prev = NULL;
> +    dev = inputThreadInfo->devs;
> +    while (dev != NULL) {
> +        if (dev->fd == fd)
> +            break;
> +        prev = dev;
> +        dev = dev->next;
> +    }
> +
> +    /* fd didn't match any registered device. */
> +    if (dev == NULL)
> +        return 0;
> +
> +    if (prev == NULL)
> +        inputThreadInfo->devs = dev->next;
> +    else
> +        prev->next = dev->next;
> +
> +    FD_CLR(fd, &inputThreadInfo->fds);
> +    dev->readInputProc = NULL;
> +    dev->readInputArgs = NULL;
> +    dev->fd = 0;
> +    dev = 0;
> +    free(dev);

There's a memory leak caused by free(0) right above.

I wish I could send a patch to fix it, instead of just pointing it out, but I
lost my development machine a few weeks (months?) ago and I couldn't afford a
replacement yet.

> +
> +    InputThreadFillPipe(hotplugPipeWrite);
> +    DebugF("input-thread: unregistered device: %d\n", fd);
> +
> +    return 1;
> +}
> +
> +/**
> + * The workhorse of threaded input event generation.
> + *
> + * Or if you prefer: The WaitForSomething for input devices. :)
> + *
> + * Runs in parallel with the server main thread, listening to input devices in
> + * an endless loop. Whenever new input data is made available, calls the
> + * proper device driver's routines which are ultimately responsible for the
> + * generation of input events.
> + *
> + * @see InputThreadPreInit()
> + * @see InputThreadInit()
> + */
> +
> +static void*
> +InputThreadDoWork(void *arg)
> +{
> +    fd_set readyFds;
> +    InputThreadDevice *dev;
> +
> +    FD_ZERO(&readyFds);
> +
> +    while (1)
> +    {
> +        XFD_COPYSET(&inputThreadInfo->fds, &readyFds);
> +        FD_SET(hotplugPipeRead, &readyFds);
> +
> +        DebugF("input-thread: InputThreadDoWork waiting for devices\n");
> +
> +        if (Select(MaxInputDevices, &readyFds, NULL, NULL, NULL) < 0) {

As you pointed before, the above occurrence of MaxInputDevices abuses the
semantics of select() which, according to the OpenGroup docs, expects as its
first argument the highest-numbered file descriptor of any of the three fd sets
it blocks on. I have a patch to fix it and, although I doubt it still applies to
master, you might find a warmer place for it then annarchy's disks. Yeah, that's
a quibble! :)

  http://people.freedesktop.org/~fcarrijo/patches/input-thread-after-review-take-2/0003-inputthread-Use-our-own-count-for-the-highest-fd-amo.patch

> +            if (errno == EINVAL)
> +                FatalError("input-thread: InputThreadDoWork (%s)", strerror(errno));
> +            else if (errno != EINTR)
> +                ErrorF("input-thread: InputThreadDoWork (%s)\n", strerror(errno));
> +        }
> +
> +        DebugF("input-thread: InputThreadDoWork generating events\n");
> +        /* Call the device drivers to generate input events for us */
> +        for (dev = inputThreadInfo->devs; dev != NULL; dev = dev->next)
> +            if (FD_ISSET(dev->fd, &readyFds) && dev->readInputProc)
> +                dev->readInputProc(dev->readInputArgs);
> +
> +        /* Kick main thread to process the generated input events and drain
> +         * events from hotplug pipe */
> +        InputThreadFillPipe(inputThreadInfo->writePipe);
> +        InputThreadReadPipe(hotplugPipeRead);
> +    }
> +}
> +
> +static void
> +InputThreadWakeup(pointer blockData, int err, pointer pReadmask)
> +{
> +    InputThreadReadPipe(inputThreadInfo->readPipe);
> +}

The decision of breaking the initialization code into PreInit() and Init() was
driven by the fact that the input devices are registered with the input thread
only after the communication pipes are created. If we pthread_create'd at that
point, the input thread would block indefinitely on select(), waiting for yet
non-registered devices.