[PATCH 11/12] config-udev: Refactor input device addition for delayed input device probing

[Hans de Goede]

Hi,

On 01/29/2014 01:42 AM, Peter Hutterer wrote:
> On Wed, Jan 15, 2014 at 03:32:25PM +0100, Hans de Goede wrote:
>> With systemd-logind we need to delay input device probing when switched away
>> (iow not on the active vt). This is a preparation patch for this.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  config/udev.c | 150 +++++++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 85 insertions(+), 65 deletions(-)
>>
>> diff --git a/config/udev.c b/config/udev.c
>> index 9579bf2..58bc3d9 100644
>> --- a/config/udev.c
>> +++ b/config/udev.c
>> @@ -52,6 +52,12 @@
>>                     "returned \"%s\"\n",                                 \
>>                     (attr), (path), (val) ? (val) : "(null)")
>>  
>> +struct input_device_info {
>> +    InputOption *options;
>> +    InputAttributes attrs;
>> +    dev_t devnum;
>> +};
>> +
>>  static struct udev_monitor *udev_monitor;
>>  
>>  #ifdef CONFIG_UDEV_KMS
>> @@ -70,6 +76,48 @@ static const char *itoa(int i)
>>  }
>>  
>>  static void
>> +add_input_device(struct input_device_info *input, int fd)
>> +{
>> +    DeviceIntPtr dev = NULL;
>> +    int rc;
>> +
>> +    if (fd != -1)
>> +        input->options = input_option_new(input->options, "fd", itoa(fd));
> 
> move this into the caller.

Why? Both callers need this, and the clean-up of fd when the driver
probe failed is also handled ...

> 
>> +
>> +    LogMessage(X_INFO, "config/udev: Adding input device %s (%s:%d)\n",
>> +               input->attrs.product, input->attrs.device, fd);
>> +
>> +    rc = NewInputDeviceRequest(input->options, &input->attrs, &dev);
>> +    if (rc != Success) {
>> +        if (fd != -1) {
>> +            systemd_logind_put_fd(input->devnum);
>> +            close(fd);
>> +        }

Here, so in the same function. But if you insist I can move it into the
caller.

>> +    }
>> +}
>> +
>> +static void
>> +free_input_device(struct input_device_info *input)
>> +{
>> +    input_option_free_list(&input->options);
>> +
>> +    free((void *) input->attrs.usb_id);
>> +    free((void *) input->attrs.pnp_id);
>> +    free((void *) input->attrs.product);
>> +    free((void *) input->attrs.device);
>> +    free((void *) input->attrs.vendor);
> 
> whoah, when did this stuff become const? I might need to undo this first,
> looks like fecc7eb1cf66db64728ee2d68cd9443df7e70879 was a bit eager there.

Yeah all the compiler warnings this triggers are annoying. If you plan to
revert this, please CC me on the revert patch then I'll add that to my
tree and rebase the rest on top of it.

> 
>> +    if (input->attrs.tags) {
>> +        const char **tag = input->attrs.tags;
>> +
>> +        while (*tag) {
>> +            free((void *) *tag);
>> +            tag++;
>> +        }
>> +        free(input->attrs.tags);
>> +    }
>> +}
>> +
>> +static void
>>  device_added(struct udev_device *udev_device)
>>  {
>>      const char *path, *name = NULL;
>> @@ -77,12 +125,10 @@ device_added(struct udev_device *udev_device)
>>      const char *syspath;
>>      const char *tags_prop;
>>      const char *key, *value, *tmp;
>> -    InputOption *input_options;
>> -    InputAttributes attrs = { };
>> -    DeviceIntPtr dev = NULL;
>> +    struct input_device_info input = { NULL, { } };
> 
> this patch would be a lot simpler if you just leave the input_options, attrs
> in place and before the call to add_input_device() merely do a
> 
> input.options = input_options;
> input.attrs  = attrs;
> input.devnum = devnum;
> add_input_device(&attrs, fd);

The patch would be simpler, agreed, but the code would also be uglier IMHO, so
I believe the slightly more invasive patch is better in the long run.

Regards,

Hans