[PATCH 2/4] input: make InputOption opaque, provide interface functions.
Peter Hutterer
peter.hutterer at who-t.net
Sun Aug 14 17:30:37 PDT 2011
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
More information about the xorg-devel
mailing list