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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> -- Dan _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
