[PATCH 2/4] input: make InputOption opaque, provide interface functions.

[Peter Hutterer]

On Fri, Aug 12, 2011 at 06:06:55AM -0700, Dan Nicholson wrote:
> On Wed, Aug 10, 2011 at 8:20 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > InputOptions is not switched to use struct list for a future patch to unify
> > it with the XF86OptionRec.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---

[...]

> > --- a/config/udev.c
> > +++ b/config/udev.c
> > @@ -60,7 +60,7 @@ device_added(struct udev_device *udev_device)
> >     const char *syspath;
> >     const char *tags_prop;
> >     const char *key, *value, *tmp;
> > -    InputOption *options = NULL, *tmpo;
> > +    InputOption *input_options;
> >     InputAttributes attrs = {};
> >     DeviceIntPtr dev = NULL;
> >     struct udev_list_entry *set, *entry;
> > @@ -94,8 +94,9 @@ device_added(struct udev_device *udev_device)
> >         return;
> >     }
> >
> > -    if (!add_option(&options, "_source", "server/udev"))
> > -        goto unwind;
> > +    input_options = input_option_new(NULL, "_source", "server/udev");
> > +    if (!input_options)
> > +        return;
> >
> >     name = udev_device_get_property_value(udev_device, "ID_MODEL");
> >     LOG_SYSATTR(path, "ID_MODEL", name);
> > @@ -144,10 +145,9 @@ device_added(struct udev_device *udev_device)
> >         name = "(unnamed)";
> >     else
> >         attrs.product = strdup(name);
> > -    add_option(&options, "name", name);
> > -
> > -    add_option(&options, "path", path);
> > -    add_option(&options, "device", path);
> > +    input_options = input_option_new(input_options, "name", name);
> > +    input_options = input_option_new(input_options, "path", path);
> > +    input_options = input_option_new(input_options, "device", path);
> 
> Is there any reason to catch the return value if it's not going to be
> tested at all? From what I can tell, the pointer to input_options
> won't be changed unless the list is being initialized. The only case
> that seems reasonable to test for would be NULL return, but that would
> be a bigger overhaul to unwind from each of these failure cases.

this code is evolutionary, I had a different interface before where it was
necessary to catch the list. Nonetheless, I think it makes sense for
consistency and as a template to use the interface in case we need to switch
the backend again where it's not guaranteed that the parameter passed in
stays the same.

[...]
> > +
> > +/*
> > + * Create a new InputOption with the key/value pair provided.
> > + * If a list is provided, the new options is added to the list and the list
> > + * is returned.
> > + *
> > + * If a new option is added to a list that already contains that option, the
> > + * previous option is overwritten.
> > + *
> > + * @param list The list to add to.
> > + * @param key Option key, will be copied.
> > + * @param value Option value, will be copied.
> > + *
> > + * @return If list is not NULL, the list with the new option added. If list
> > + * is NULL, a new option list with one element. On failure, NULL is
> > + * returned.
> > + */
> > +InputOption*
> > +input_option_new(InputOption* list, const char *key, const char *value)
> > +{
> > +    InputOption *opt = NULL;
> > +
> > +    if (!key)
> > +        return NULL;
> > +
> > +    if (list)
> > +    {
> > +        nt_list_for_each_entry(opt, list, next)
> > +        {
> > +            if (strcmp(input_option_get_key(opt), key) == 0)
> > +            {
> > +                input_option_set_value(opt, value);
> > +                return list;
> > +            }
> > +        }
> > +    }
> > +
> > +    opt = calloc(1, sizeof(InputOption));
> > +    if (!opt)
> > +        goto error;
> > +    nt_list_init(opt, next);
> > +
> > +    input_option_set_key(opt, key);
> > +    input_option_set_value(opt, value);
> > +
> > +    if (list)
> > +    {
> > +        nt_list_append(opt, list, InputOption, next);
> > +        return list;
> > +    } else
> > +        return opt;
> > +
> > +error:
> > +    input_option_free(opt);
> > +    return NULL;
> > +}
> 
> The only case where you arrive at error is where opt failed
> allocation. Seems like you could just return NULL immediately in that
> case and dispense with the goto that tries to undo work we know
> failed. Not a big deal, though.

amended, thank you. this is a leftover from my struct list experiments..

[...]

> > @@ -916,13 +916,11 @@ NewInputDeviceRequest (InputOption *options, InputAttributes *attrs,
> >         }
> >     }
> >
> > -    for (option = options; option; option = option->next) {
> > -        /* Steal option key/value strings from the provided list.
> > -         * We need those strings, the InputOption list doesn't. */
> > +    nt_list_for_each_entry(option, options, next) {
> > +        /* Copy option key/value strings from the provided list */
> >         pInfo->options = xf86addNewOption(pInfo->options,
> > -                                               option->key, option->value);
> > -        option->key = NULL;
> > -        option->value = NULL;
> > +                                          strdup(input_option_get_key(option)),
> > +                                          strdup(input_option_get_value(option)));
> 
> This part was always really odd to me, but does changing it to copying
> introduce a leak? My guess is that no, because eventually you free the
> whole thing back in config/udev. Right?

correct. The intent here is that instead of magically reassigning pointers,
let's properly duplicate them and free them where they were allocated to
begin with. It's a bit more of a hit but easier to track down and
understand.

Others amended as suggested, thanks.

Cheers,
  Peter