[PATCH 1/4] Fix a couple of compiler warnings
Dan Nicholson
dbn.lists at gmail.com
Fri May 14 06:08:00 PDT 2010
On Wed, May 12, 2010 at 09:38:05PM +0200, Julien Cristau wrote:
> On Wed, May 12, 2010 at 10:53:18 -0700, Dan Nicholson wrote:
>
> > Yeah, clearly libudev has it's own copies of the strings that it will
> > keep until we unref the udev_device. So, here's a patch that makes the
> > InputAttributes members const. Like Keith, I couldn't figure out how to
> > handle char **tags without an explicit cast.
> >
> The attached seems to build without warnings on master.
>
> Cheers,
> Julien
> From 4a2cce5d324a30847b9591ba7cc9a9c450ec8ae6 Mon Sep 17 00:00:00 2001
> From: Julien Cristau <jcristau at debian.org>
> Date: Wed, 12 May 2010 21:18:54 +0200
> Subject: [PATCH 1/2] Make InputAttributes strings const
>
> ---
> config/hal.c | 20 ++++++++++----------
> config/udev.c | 10 ++++++----
> include/input.h | 8 ++++----
> 3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/config/hal.c b/config/hal.c
> index 6a22323..9caf139 100644
> --- a/config/hal.c
> +++ b/config/hal.c
> @@ -129,6 +129,8 @@ static void
> device_added(LibHalContext *hal_ctx, const char *udi)
> {
> char *path = NULL, *driver = NULL, *name = NULL, *config_info = NULL;
> + char **tags = NULL;
> + char *vendor = NULL;
> InputOption *options = NULL, *tmpo = NULL;
> InputAttributes attrs = {0};
> DeviceIntPtr dev = NULL;
> @@ -155,16 +157,16 @@ device_added(LibHalContext *hal_ctx, const char *udi)
> LogMessage(X_WARNING,"config/hal: no driver or path specified for %s\n", udi);
> goto unwind;
> }
> - attrs.device = xstrdup(path);
> + attrs.device = path;
>
> name = get_prop_string(hal_ctx, udi, "info.product");
> if (!name)
> name = xstrdup("(unnamed)");
> else
> - attrs.product = xstrdup(name);
> + attrs.product = name;
>
> - attrs.vendor = get_prop_string(hal_ctx, udi, "info.vendor");
> - attrs.tags = xstrtokenize(get_prop_string(hal_ctx, udi, "input.tags"), ",");
> + attrs.vendor = vendor = get_prop_string(hal_ctx, udi, "info.vendor");
> + attrs.tags = tags = xstrtokenize(get_prop_string(hal_ctx, udi, "input.tags"), ",");
>
> if (libhal_device_query_capability(hal_ctx, udi, "input.keys", NULL))
> attrs.flags |= ATTR_KEYBOARD;
> @@ -389,16 +391,14 @@ unwind:
> free(tmpo);
> }
>
> - free(attrs.product);
> - free(attrs.vendor);
> - free(attrs.device);
> - if (attrs.tags) {
> - char **tag = attrs.tags;
> + free(vendor);
> + if (tags) {
> + char **tag = tags;
> while (*tag) {
> free(*tag);
> tag++;
> }
> - free(attrs.tags);
> + free(tags);
> }
>
> if (xkb_opts.layout)
> diff --git a/config/udev.c b/config/udev.c
> index 5e8d8da..941bfbe 100644
> --- a/config/udev.c
> +++ b/config/udev.c
> @@ -46,6 +46,7 @@ device_added(struct udev_device *udev_device)
> char *config_info = NULL;
> const char *syspath;
> const char *key, *value, *tmp;
> + char **tags = NULL;
> InputOption *options = NULL, *tmpo;
> InputAttributes attrs = {};
> DeviceIntPtr dev = NULL;
> @@ -87,7 +88,8 @@ device_added(struct udev_device *udev_device)
> add_option(&options, "path", path);
> add_option(&options, "device", path);
> attrs.device = path;
> - attrs.tags = xstrtokenize(udev_device_get_property_value(udev_device, "ID_INPUT.tags"), ",");
> + tags = xstrtokenize(udev_device_get_property_value(udev_device, "ID_INPUT.tags"), ",");
> + attrs.tags = tags;
>
> config_info = Xprintf("udev:%s", syspath);
> if (!config_info)
> @@ -154,13 +156,13 @@ device_added(struct udev_device *udev_device)
> free(tmpo);
> }
>
> - if (attrs.tags) {
> - char **tag = attrs.tags;
> + if (tags) {
> + char **tag = tags;
> while (*tag) {
> free(*tag);
> tag++;
> }
> - free(attrs.tags);
> + free(tags);
> }
>
> return;
> diff --git a/include/input.h b/include/input.h
> index 63f981e..aadcdf1 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -212,10 +212,10 @@ typedef struct _InputOption {
> } InputOption;
>
> typedef struct _InputAttributes {
> - char *product;
> - char *vendor;
> - char *device;
> - char **tags; /* null-terminated */
> + const char *product;
> + const char *vendor;
> + const char *device;
> + char * const *tags; /* null-terminated */
I guess I would prefer this as "const char **", a pointer to a pointer
to an immutable string, instead of "char * const *", a pointer to an
immutable pointer to a string. Better would be "const char * const *",
but maybe that's getting pathological.
Did you choose this because it required no explicit casts?
> uint32_t flags;
> } InputAttributes;
>
> --
> 1.7.1
>
> From d074587905b1d86431efaf85977f5e04850b6f57 Mon Sep 17 00:00:00 2001
> From: Julien Cristau <jcristau at debian.org>
> Date: Wed, 12 May 2010 21:29:55 +0200
> Subject: [PATCH 2/2] config/hal: don't leak the input.tags property
>
> ---
> config/hal.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/config/hal.c b/config/hal.c
> index 9caf139..9396cef 100644
> --- a/config/hal.c
> +++ b/config/hal.c
> @@ -130,7 +130,7 @@ device_added(LibHalContext *hal_ctx, const char *udi)
> {
> char *path = NULL, *driver = NULL, *name = NULL, *config_info = NULL;
> char **tags = NULL;
> - char *vendor = NULL;
> + char *vendor = NULL, *hal_tags;
> InputOption *options = NULL, *tmpo = NULL;
> InputAttributes attrs = {0};
> DeviceIntPtr dev = NULL;
> @@ -166,7 +166,9 @@ device_added(LibHalContext *hal_ctx, const char *udi)
> attrs.product = name;
>
> attrs.vendor = vendor = get_prop_string(hal_ctx, udi, "info.vendor");
> - attrs.tags = tags = xstrtokenize(get_prop_string(hal_ctx, udi, "input.tags"), ",");
> + hal_tags = get_prop_string(hal_ctx, udi, "input.tags");
> + attrs.tags = tags = xstrtokenize(hal_tags, ",");
> + free(hal_tags);
>
> if (libhal_device_query_capability(hal_ctx, udi, "input.keys", NULL))
> attrs.flags |= ATTR_KEYBOARD;
> --
> 1.7.1
Yes, this looks necessary.
Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>
--
Dan
More information about the xorg-devel
mailing list