[PATCH 2/4] input: make InputOption opaque, provide interface functions.
Dan Nicholson
dbn.lists at gmail.com
Fri Aug 12 06:06:55 PDT 2011
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>
> ---
> config/config-backends.h | 2 +-
> config/config.c | 22 ------
> config/udev.c | 35 ++++------
> dix/inpututils.c | 145 ++++++++++++++++++++++++++++++++++++++++
> hw/xfree86/common/xf86Xinput.c | 28 ++++----
> include/input.h | 17 +++--
> include/inputstr.h | 7 ++
> test/input.c | 58 ++++++++++++++++
> 8 files changed, 251 insertions(+), 63 deletions(-)
>
> diff --git a/config/config-backends.h b/config/config-backends.h
> index 0d36d72..35ab8a0 100644
> --- a/config/config-backends.h
> +++ b/config/config-backends.h
> @@ -27,10 +27,10 @@
> #include <dix-config.h>
> #endif
> #include "input.h"
> +#include "list.h"
>
> void remove_devices(const char *backend, const char *config_info);
> BOOL device_is_duplicate(const char *config_info);
> -InputOption* add_option(InputOption **options, const char *key, const char *value);
>
> #ifdef CONFIG_UDEV
> int config_udev_init(void);
> diff --git a/config/config.c b/config/config.c
> index af8f4f9..9c28785 100644
> --- a/config/config.c
> +++ b/config/config.c
> @@ -122,25 +122,3 @@ device_is_duplicate(const char *config_info)
> return FALSE;
> }
>
> -/**
> - * Allocate a new option and append to the list.
> - *
> - * @return A pointer to the newly allocated InputOption struct.
> - */
> -InputOption*
> -add_option(InputOption **options, const char *key, const char *value)
> -{
> - if (!value || *value == '\0')
> - return NULL;
> -
> - for (; *options; options = &(*options)->next)
> - ;
> - *options = calloc(sizeof(**options), 1);
> - if (!*options) /* Yeesh. */
> - return NULL;
> - (*options)->key = strdup(key);
> - (*options)->value = strdup(value);
> - (*options)->next = NULL;
> -
> - return *options;
> -}
> diff --git a/config/udev.c b/config/udev.c
> index c17c4fd..2ab9260 100644
> --- 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.
> if (path)
> attrs.device = strdup(path);
>
> @@ -177,15 +177,15 @@ device_added(struct udev_device *udev_device)
> LOG_PROPERTY(path, key, value);
> tmp = key + sizeof(UDEV_XKB_PROP_KEY) - 1;
> if (!strcasecmp(tmp, "rules"))
> - add_option(&options, "xkb_rules", value);
> + input_options = input_option_new(input_options, "xkb_rules", value);
> else if (!strcasecmp(tmp, "layout"))
> - add_option(&options, "xkb_layout", value);
> + input_options = input_option_new(input_options, "xkb_layout", value);
> else if (!strcasecmp(tmp, "variant"))
> - add_option(&options, "xkb_variant", value);
> + input_options = input_option_new(input_options, "xkb_variant", value);
> else if (!strcasecmp(tmp, "model"))
> - add_option(&options, "xkb_model", value);
> + input_options = input_option_new(input_options, "xkb_model", value);
> else if (!strcasecmp(tmp, "options"))
> - add_option(&options, "xkb_options", value);
> + input_options = input_option_new(input_options, "xkb_options", value);
> } else if (!strcmp(key, "ID_VENDOR")) {
> LOG_PROPERTY(path, key, value);
> attrs.vendor = strdup(value);
> @@ -210,22 +210,17 @@ device_added(struct udev_device *udev_device)
> }
> }
>
> - add_option(&options, "config_info", config_info);
> + input_options = input_option_new(input_options, "config_info", config_info);
>
> LogMessage(X_INFO, "config/udev: Adding input device %s (%s)\n",
> name, path);
> - rc = NewInputDeviceRequest(options, &attrs, &dev);
> + rc = NewInputDeviceRequest(input_options, &attrs, &dev);
> if (rc != Success)
> goto unwind;
>
> unwind:
> free(config_info);
> - while ((tmpo = options)) {
> - options = tmpo->next;
> - free(tmpo->key); /* NULL if dev != NULL */
> - free(tmpo->value); /* NULL if dev != NULL */
> - free(tmpo);
> - }
> + input_option_free_list(&input_options);
>
> free(attrs.usb_id);
> free(attrs.pnp_id);
> diff --git a/dix/inpututils.c b/dix/inpututils.c
> index 9632076..ed600c4 100644
> --- a/dix/inpututils.c
> +++ b/dix/inpututils.c
> @@ -598,3 +598,148 @@ void init_device_event(DeviceEvent *event, DeviceIntPtr dev, Time ms)
> event->deviceid = dev->id;
> event->sourceid = dev->id;
> }
> +
> +/**
> + * Delete the element with the key from the list, freeing all memory
> + * associated with the element..
> + */
> +static void
> +input_option_free(InputOption *o)
> +{
> + free(o->key);
> + free(o->value);
> + free(o);
> +}
> +
> +/*
> + * 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.
> +
> +InputOption*
> +input_option_free_element(InputOption *list, const char *key)
> +{
> + InputOption *element;
> +
> + nt_list_for_each_entry(element, list, next) {
> + if (strcmp(input_option_get_key(element), key) == 0) {
> + nt_list_del(element, list, InputOption, next);
> + input_option_free(element);
> + break;
> + }
> + }
> + return list;
> +}
> +
> +/**
> + * Free the list pointed at by opt.
> + */
> +void
> +input_option_free_list(InputOption **opt)
> +{
> + InputOption *element, *tmp;
> +
> + nt_list_for_each_entry_safe(element, tmp, *opt, next) {
> + nt_list_del(element, *opt, InputOption, next);
> + input_option_free(element);
> + }
> + *opt = NULL;
> +}
> +
> +
> +/**
> + * Find the InputOption with the given option name.
> + *
> + * @return The InputOption or NULL if not present.
> + */
> +InputOption*
> +input_option_find(InputOption *list, const char *key)
> +{
> + InputOption *element;
> +
> + nt_list_for_each_entry(element, list, next) {
> + if (strcmp(input_option_get_key(element), key) == 0)
> + return element;
> + }
> +
> + return NULL;
> +}
> +
> +const char*
> +input_option_get_key(const InputOption *opt)
> +{
> + return opt->key;
> +}
> +
> +const char*
> +input_option_get_value(const InputOption *opt)
> +{
> + return opt->value;
> +}
> +
> +void
> +input_option_set_key(InputOption *opt, const char *key)
> +{
> + free(opt->key);
> + if (key)
> + opt->key = strdup(key);
> +}
> +
> +void
> +input_option_set_value(InputOption *opt, const char *value)
> +{
> + free(opt->value);
> + if (value)
> + opt->value = strdup(value);
> +}
> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index cecf4ff..444efec 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -879,35 +879,35 @@ NewInputDeviceRequest (InputOption *options, InputAttributes *attrs,
> if (!pInfo)
> return BadAlloc;
>
> - for (option = options; option; option = option->next) {
> - if (strcasecmp(option->key, "driver") == 0) {
> + nt_list_for_each_entry(option, options, next) {
> + if (strcasecmp(input_option_get_key(option), "driver") == 0) {
> if (pInfo->driver) {
> rval = BadRequest;
> goto unwind;
> }
> - pInfo->driver = xstrdup(option->value);
> + pInfo->driver = xstrdup(input_option_get_value(option));
> if (!pInfo->driver) {
> rval = BadAlloc;
> goto unwind;
> }
> }
>
> - if (strcasecmp(option->key, "name") == 0 ||
> - strcasecmp(option->key, "identifier") == 0) {
> + if (strcasecmp(input_option_get_key(option), "name") == 0 ||
> + strcasecmp(input_option_get_key(option), "identifier") == 0) {
> if (pInfo->name) {
> rval = BadRequest;
> goto unwind;
> }
> - pInfo->name = xstrdup(option->value);
> + pInfo->name = xstrdup(input_option_get_value(option));
> if (!pInfo->name) {
> rval = BadAlloc;
> goto unwind;
> }
> }
>
> - if (strcmp(option->key, "_source") == 0 &&
> - (strcmp(option->value, "server/hal") == 0 ||
> - strcmp(option->value, "server/udev") == 0)) {
> + if (strcmp(input_option_get_key(option), "_source") == 0 &&
> + (strcmp(input_option_get_value(option), "server/hal") == 0 ||
> + strcmp(input_option_get_value(option), "server/udev") == 0)) {
> is_auto = 1;
> if (!xf86Info.autoAddDevices) {
> rval = BadMatch;
> @@ -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?
> }
>
> /* Apply InputClass settings */
> diff --git a/include/input.h b/include/input.h
> index 5377a0c..0258f4f 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -56,6 +56,7 @@ SOFTWARE.
> #include "window.h" /* for WindowPtr */
> #include "xkbrules.h"
> #include "events.h"
> +#include "list.h"
>
> #define DEVICE_INIT 0
> #define DEVICE_ON 1
> @@ -202,11 +203,7 @@ typedef struct {
> extern _X_EXPORT KeybdCtrl defaultKeyboardControl;
> extern _X_EXPORT PtrCtrl defaultPointerControl;
>
> -typedef struct _InputOption {
> - char *key;
> - char *value;
> - struct _InputOption *next;
> -} InputOption;
> +typedef struct _InputOption InputOption;
>
> typedef struct _InputAttributes {
> char *product;
> @@ -595,4 +592,14 @@ extern _X_EXPORT void valuator_mask_copy(ValuatorMask *dest,
> const ValuatorMask *src);
> extern _X_EXPORT int valuator_mask_get(const ValuatorMask *mask, int valnum);
>
> +/* InputOption handling interface */
> +extern _X_EXPORT InputOption* input_option_new(InputOption *list, const char *key, const char *value);
> +extern _X_EXPORT void input_option_free_list(InputOption **opt);
> +extern _X_EXPORT InputOption* input_option_free_element(InputOption *opt, const char *key);
> +extern _X_EXPORT InputOption* input_option_find(InputOption *list, const char *key);
> +extern _X_EXPORT const char* input_option_get_key(const InputOption *opt);
> +extern _X_EXPORT const char* input_option_get_value(const InputOption *opt);
> +extern _X_EXPORT void input_option_set_key(InputOption *opt, const char* key);
> +extern _X_EXPORT void input_option_set_value(InputOption *opt, const char* value);
> +
> #endif /* INPUT_H */
> diff --git a/include/inputstr.h b/include/inputstr.h
> index 838f9f0..480e956 100644
> --- a/include/inputstr.h
> +++ b/include/inputstr.h
> @@ -602,4 +602,11 @@ static inline WindowPtr DeepestSpriteWin(SpritePtr sprite)
> return sprite->spriteTrace[sprite->spriteTraceGood - 1];
> }
>
> +struct _InputOption {
> + char *key;
> + char *value;
> + struct _InputOption *next;
> +};
> +
> +
> #endif /* INPUTSTRUCT_H */
> diff --git a/test/input.c b/test/input.c
> index c2b0eb0..30321ed 100644
> --- a/test/input.c
> +++ b/test/input.c
> @@ -1308,6 +1308,63 @@ static void dix_get_master(void)
> }
>
>
> +static void input_option_test(void)
> +{
> + InputOption *list = NULL;
> + InputOption *opt;
> + const char *val;
> +
> + printf("Testing input_option list interface\n");
> +
> + list = input_option_new(list, "key", "value");
> + assert(list);
> + opt = input_option_find(list, "key");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "value") == 0);
> +
> + list = input_option_new(list, "2", "v2");
> + opt = input_option_find(list, "key");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "value") == 0);
> +
> + opt = input_option_find(list, "2");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "v2") == 0);
> +
> + list = input_option_new(list, "3", "v3");
> +
> + /* search, delete */
> + opt = input_option_find(list, "key");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "value") == 0);
> + list = input_option_free_element(list, "key");
> + opt = input_option_find(list, "key");
> + assert(opt == NULL);
> +
> + opt = input_option_find(list, "2");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "v2") == 0);
> + list = input_option_free_element(list, "2");
> + opt = input_option_find(list, "2");
> + assert(opt == NULL);
> +
> + opt = input_option_find(list, "3");
> + val = input_option_get_value(opt);
> + assert(strcmp(val, "v3") == 0);
> + list = input_option_free_element(list, "3");
> + opt = input_option_find(list, "3");
> + assert(opt == NULL);
I don't see any exercising of set_key/set_value here. Might make sense
to add one case where you change the key and value and check that you
got the right thing back.
> +
> + /* recreate */
> + list = input_option_new(list, "3", "v3");
> + list = input_option_new(list, "3", "v3");
> + list = input_option_new(list, "3", "v3");
> + input_option_free_list(&list);
> +
> + assert(list == NULL);
> +}
> +
> +
> int main(int argc, char** argv)
> {
> dix_input_valuator_masks();
> @@ -1324,6 +1381,7 @@ int main(int argc, char** argv)
> xi_unregister_handlers();
> dix_valuator_alloc();
> dix_get_master();
> + input_option_test();
>
> return 0;
> }
The one other major thing I saw is that you didn't change config/hal
to the new interface. I don't use the hal interface anymore, but you
might as well get the build failures handled now before Alan tries to
build this thing.
With those comments addressed,
Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>
More information about the xorg-devel
mailing list