[PATCH v2 3/7] input: un-constify InputAttributes

[Hans de Goede]

Hi,

On 02/04/2014 02:34 AM, Peter Hutterer wrote:
> Introduced in fecc7eb1cf66db64728ee2d68cd9443df7e70879 and reverts most of
> that but it's helpfully mixed with other stuff.
> 
> InputAttributes are not const, they're strdup'd everywhere but the test code
> and freed properly. Revert the const char changes and fix the test up instead.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> I thought I did run the tests after this patch but apparently I didn't.
> FreeInputAttributes caused an invalid free because one struct in the tests
> was on the stack.
> 
> Changes to v1:
> - use xnfstrdup, not strdup
> - now that we call FreeInputAttributes(orig) in the test, that struct must
>   be allocated on the heap, since FIA will free it.
> 
> The latter looks like a big change (last hunk) but it's really just a struct
> vs pointer replacement.
> Hans, your rev-by still good for this?

Just re-reviewed it, and yep this is still:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans


> 
>  config/udev.c    | 14 +++++++-------
>  dix/inpututils.c | 16 ++++++++--------
>  include/input.h  | 12 ++++++------
>  test/input.c     | 56 ++++++++++++++++++++++++++++++--------------------------
>  4 files changed, 51 insertions(+), 47 deletions(-)
> 
> diff --git a/config/udev.c b/config/udev.c
> index 436b8f0..68ed348 100644
> --- a/config/udev.c
> +++ b/config/udev.c
> @@ -242,16 +242,16 @@ device_added(struct udev_device *udev_device)
>      free(config_info);
>      input_option_free_list(&input_options);
>  
> -    free((void *) attrs.usb_id);
> -    free((void *) attrs.pnp_id);
> -    free((void *) attrs.product);
> -    free((void *) attrs.device);
> -    free((void *) attrs.vendor);
> +    free(attrs.usb_id);
> +    free(attrs.pnp_id);
> +    free(attrs.product);
> +    free(attrs.device);
> +    free(attrs.vendor);
>      if (attrs.tags) {
> -        const char **tag = attrs.tags;
> +        char **tag = attrs.tags;
>  
>          while (*tag) {
> -            free((void *) *tag);
> +            free(*tag);
>              tag++;
>          }
>          free(attrs.tags);
> diff --git a/dix/inpututils.c b/dix/inpututils.c
> index 3e1d75f..a10a7c7 100644
> --- a/dix/inpututils.c
> +++ b/dix/inpututils.c
> @@ -351,7 +351,7 @@ DuplicateInputAttributes(InputAttributes * attrs)
>  {
>      InputAttributes *new_attr;
>      int ntags = 0;
> -    const char **tags, **new_tags;
> +    char **tags, **new_tags;
>  
>      if (!attrs)
>          return NULL;
> @@ -403,20 +403,20 @@ DuplicateInputAttributes(InputAttributes * attrs)
>  void
>  FreeInputAttributes(InputAttributes * attrs)
>  {
> -    const char **tags;
> +    char **tags;
>  
>      if (!attrs)
>          return;
>  
> -    free((void *) attrs->product);
> -    free((void *) attrs->vendor);
> -    free((void *) attrs->device);
> -    free((void *) attrs->pnp_id);
> -    free((void *) attrs->usb_id);
> +    free(attrs->product);
> +    free(attrs->vendor);
> +    free(attrs->device);
> +    free(attrs->pnp_id);
> +    free(attrs->usb_id);
>  
>      if ((tags = attrs->tags))
>          while (*tags)
> -            free((void *) *tags++);
> +            free(*tags++);
>  
>      free(attrs->tags);
>      free(attrs);
> diff --git a/include/input.h b/include/input.h
> index 455963f..6f047ee 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -221,12 +221,12 @@ typedef struct _InputOption InputOption;
>  typedef struct _XI2Mask XI2Mask;
>  
>  typedef struct _InputAttributes {
> -    const char *product;
> -    const char *vendor;
> -    const char *device;
> -    const char *pnp_id;
> -    const char *usb_id;
> -    const char **tags;                /* null-terminated */
> +    char *product;
> +    char *vendor;
> +    char *device;
> +    char *pnp_id;
> +    char *usb_id;
> +    char **tags;                /* null-terminated */
>      uint32_t flags;
>  } InputAttributes;
>  
> diff --git a/test/input.c b/test/input.c
> index aaa7a69..5813e6d 100644
> --- a/test/input.c
> +++ b/test/input.c
> @@ -1101,7 +1101,7 @@ xi_unregister_handlers(void)
>  static void
>  cmp_attr_fields(InputAttributes * attr1, InputAttributes * attr2)
>  {
> -    const char **tags1, **tags2;
> +    char **tags1, **tags2;
>  
>      assert(attr1 && attr2);
>      assert(attr1 != attr2);
> @@ -1180,50 +1180,54 @@ cmp_attr_fields(InputAttributes * attr1, InputAttributes * attr2)
>  static void
>  dix_input_attributes(void)
>  {
> -    InputAttributes orig = { 0 };
> +    InputAttributes *orig;
>      InputAttributes *new;
> -    const char *tags[4] = { "tag1", "tag2", "tag2", NULL };
>  
>      new = DuplicateInputAttributes(NULL);
>      assert(!new);
>  
> -    new = DuplicateInputAttributes(&orig);
> -    assert(memcmp(&orig, new, sizeof(InputAttributes)) == 0);
> +    orig = calloc(1, sizeof(InputAttributes));
> +    assert(orig);
>  
> -    orig.product = "product name";
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    new = DuplicateInputAttributes(orig);
> +    assert(memcmp(orig, new, sizeof(InputAttributes)) == 0);
> +
> +    orig->product = xnfstrdup("product name");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.vendor = "vendor name";
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->vendor = xnfstrdup("vendor name");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.device = "device path";
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->device = xnfstrdup("device path");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.pnp_id = "PnPID";
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->pnp_id = xnfstrdup("PnPID");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.usb_id = "USBID";
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->usb_id = xnfstrdup("USBID");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.flags = 0xF0;
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->flags = 0xF0;
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
>  
> -    orig.tags = tags;
> -    new = DuplicateInputAttributes(&orig);
> -    cmp_attr_fields(&orig, new);
> +    orig->tags = xstrtokenize("tag1 tag2 tag3", " ");
> +    new = DuplicateInputAttributes(orig);
> +    cmp_attr_fields(orig, new);
>      FreeInputAttributes(new);
> +
> +    FreeInputAttributes(orig);
>  }
>  
>  static void
>