[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