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

[Peter Hutterer]

On Wed, Jan 29, 2014 at 10:27:46AM +0100, Hans de Goede wrote:
> 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.

The comment was mostly for readability (I was reviewing the patches in
order). all the option handling is handled in device_added(), the fd is the
only one that didn't get set. and with this patch there is only one caller.
it makes more sense once 12/12 is applied, so leave it as-is.

> >> +    }
> >> +}
> >> +
> >> +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.

Patch for that is on the list now
http://patchwork.freedesktop.org/patch/19092/

> >> +    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.

I've become a big fan of using temporary variables to avoid
half-initialization of a struct. The above code makes sure that input.* is
unset until we've determined every subvalue correctly so if something fails
the dangers of a struct being badly initialized is lower. It also makes it
more obvious if a specific field in the struct doesn't get set.

having said that, this is personal taste, since you already have the patch
ready I won't make you change it :)

Cheers,
   Peter