[PATCH] dix: add helper functions to duplicate and free InputAttributes.

[Dan Nicholson]

On Tue, May 25, 2010 at 12:12 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> No special memory handling is used to give drivers the maximum flexibility
> with the data. Drivers should be able to call realloc on the product string
> if needed and perform similar operations.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  dix/inpututils.c |   79 +++++++++++++++++++++++++++++++++++++++++
>  include/input.h  |    2 +
>  test/input.c     |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 183 insertions(+), 0 deletions(-)
>
> diff --git a/dix/inpututils.c b/dix/inpututils.c
> index 8e75372..df2ace0 100644
> --- a/dix/inpututils.c
> +++ b/dix/inpututils.c
> @@ -331,3 +331,82 @@ int generate_modkeymap(ClientPtr client, DeviceIntPtr dev,
>
>     return Success;
>  }
> +
> +/**
> + * Duplicate the InputAttributes in the most obvious way.
> + * No special memory handling is used to give drivers the maximum
> + * flexibility with the data. Drivers should be able to call realloc on the
> + * product string if needed and perform similar operations.
> + */
> +InputAttributes*
> +DuplicateInputAttributes(InputAttributes *attrs)
> +{
> +    InputAttributes *new_attr;
> +    int ntags = 0;
> +    char **tags, **new_tags;
> +
> +    if (!attrs)
> +        return NULL;
> +
> +    if (!(new_attr = calloc(1, sizeof(InputAttributes))))
> +        goto unwind;

I was going to say, "why calloc?" here, but it does make it easier to
just do FreeInputAttributes in the unwind.

> +
> +    if (attrs->product && !(new_attr->product = strdup(attrs->product)))
> +        goto unwind;
> +    if (attrs->vendor && !(new_attr->vendor = strdup(attrs->vendor)))
> +        goto unwind;
> +    if (attrs->device && !(new_attr->device = strdup(attrs->device)))
> +        goto unwind;
> +
> +    new_attr->flags = attrs->flags;
> +
> +    if ((tags = attrs->tags))
> +    {
> +        while(*tags++)
> +            ntags++;
> +
> +        new_attr->tags = calloc(ntags + 1, sizeof(char*));
> +        if (!new_attr->tags)
> +            goto unwind;
> +
> +        tags = attrs->tags;
> +        new_tags = new_attr->tags;
> +
> +        while(*tags)
> +        {
> +            *new_tags = strdup(*tags);
> +            if (!*new_tags)
> +                goto unwind;
> +
> +            tags++;
> +            new_tags++;
> +        }
> +    }

Geez, maybe NULL terminated arrays wasn't the hottest type to use. :)

> +
> +    return new_attr;
> +
> +unwind:
> +    FreeInputAttributes(new_attr);
> +    return NULL;
> +}
> +
> +void
> +FreeInputAttributes(InputAttributes *attrs)
> +{
> +    char **tags;
> +
> +    if (!attrs)
> +        return;
> +
> +    free(attrs->product);
> +    free(attrs->vendor);
> +    free(attrs->device);
> +
> +    if ((tags = attrs->tags))
> +        while(*tags)
> +            free(*tags++);
> +
> +    free(attrs->tags);
> +    free(attrs);
> +}
> +
> diff --git a/include/input.h b/include/input.h
> index 0f7c215..5426c44 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -521,6 +521,8 @@ extern int AllocXTestDevice(ClientPtr client,
>  extern BOOL IsXTestDevice(DeviceIntPtr dev, DeviceIntPtr master);
>  extern DeviceIntPtr GetXTestDevice(DeviceIntPtr master);
>  extern void SendDevicePresenceEvent(int deviceid, int type);
> +extern _X_EXPORT InputAttributes *DuplicateInputAttributes(InputAttributes *attrs);
> +extern _X_EXPORT void FreeInputAttributes(InputAttributes *attrs);
>
>  /* misc event helpers */
>  extern Mask GetEventFilter(DeviceIntPtr dev, xEvent *event);
> diff --git a/test/input.c b/test/input.c
> index 63d1a18..8a54af9 100644
> --- a/test/input.c
> +++ b/test/input.c
> @@ -771,11 +771,112 @@ static void xi_unregister_handlers(void)
>
>  }
>
> +static void cmp_attr_fields(InputAttributes *attr1,
> +                            InputAttributes *attr2)
> +{
> +    char **tags1, **tags2;
> +
> +    g_assert(attr1 && attr2);
> +    g_assert(attr1 != attr2);
> +    g_assert(attr1->flags == attr2->flags);
> +
> +    if (attr1->product != NULL)
> +    {
> +        g_assert(attr1->product != attr2->product);
> +        g_assert(strcmp(attr1->product, attr2->product) == 0);
> +    } else
> +        g_assert(attr2->product == NULL);
> +
> +    if (attr1->vendor != NULL)
> +    {
> +        g_assert(attr1->vendor != attr2->vendor);
> +        g_assert(strcmp(attr1->vendor, attr2->vendor) == 0);
> +    } else
> +        g_assert(attr2->vendor == NULL);
> +
> +    if (attr1->device != NULL)
> +    {
> +        g_assert(attr1->device != attr2->device);
> +        g_assert(strcmp(attr1->device, attr2->device) == 0);
> +    } else
> +        g_assert(attr2->device == NULL);
> +
> +    tags1 = attr1->tags;
> +    tags2 = attr2->tags;
> +    if (!tags1)
> +    {
> +        g_assert(!tags2);
> +        return;

This last part might be clearer with a goto instead of an embedded
return, but probably not a big deal.

> +    }
> +
> +    /* check for identical content, but duplicated */
> +    while (*tags1)
> +    {

Probably should have g_assert(*tags2) first to ensure there's actually
an element there and the array didn't end.

> +        g_assert(*tags1 != *tags2);
> +        g_assert(strcmp(*tags1, *tags2) == 0);
> +        tags1++;
> +        tags2++;
> +    }
> +
> +    g_assert(!*tags2);

Would be nice to have a comment explaining that this catches tags2
array longer than tags1 array.

> +
> +    /* check for not sharing memory */
> +    tags1 = attr1->tags;
> +    while (*tags1)
> +    {
> +        tags2 = attr2->tags;
> +        while (*tags2)
> +            g_assert(*tags1 != *tags2++);
> +
> +        tags1++;
> +    }

I think you just did this last chunk (g_assert(*tags1 != *tags2)) in
the previous loop more clearly ("but not duplicated"). Can be droppped
I think.

> +}
> +
> +static void dix_input_attributes(void)
> +{
> +    InputAttributes orig = {0};
> +    InputAttributes *new;
> +    char *tags[4] = {"tag1", "tag2", "tag2", NULL};
> +
> +    new = DuplicateInputAttributes(NULL);
> +    g_assert(!new);
> +
> +    new = DuplicateInputAttributes(&orig);
> +    g_assert(memcpy(&orig, new, sizeof(InputAttributes)));

Why the dup and then memcpy? Just ensuring that the correct storage
size has been allocated?

> +
> +    orig.product = "product name";
> +    new = DuplicateInputAttributes(&orig);
> +    cmp_attr_fields(&orig, new);
> +    FreeInputAttributes(new);
> +
> +    orig.vendor = "vendor name";
> +    new = DuplicateInputAttributes(&orig);
> +    cmp_attr_fields(&orig, new);
> +    FreeInputAttributes(new);
> +
> +    orig.device = "device path";
> +    new = DuplicateInputAttributes(&orig);
> +    cmp_attr_fields(&orig, new);
> +    FreeInputAttributes(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);
> +    FreeInputAttributes(new);

Any reason not to set all the members at once then do the dup, cmp, free spin?

> +}
> +
> +
>  int main(int argc, char** argv)
>  {
>     g_test_init(&argc, &argv,NULL);
>     g_test_bug_base("https://bugzilla.freedesktop.org/show_bug.cgi?id=");
>
> +    g_test_add_func("/dix/input/attributes", dix_input_attributes);
>     g_test_add_func("/dix/input/init-valuators", dix_init_valuators);
>     g_test_add_func("/dix/input/event-core-conversion", dix_event_to_core_conversion);
>     g_test_add_func("/dix/input/check-grab-values", dix_check_grab_values);
> @@ -784,5 +885,6 @@ int main(int argc, char** argv)
>     g_test_add_func("/include/byte_padding_macros", include_byte_padding_macros);
>     g_test_add_func("/Xi/xiproperty/register-unregister", xi_unregister_handlers);
>
> +
>     return g_test_run();
>  }
> --
> 1.7.0.1

Feel free to clean up the test at your discretion, but the API looks good to me.

Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>