[PATCH] dix: add helper functions to duplicate and free InputAttributes.
Dan Nicholson
dbn.lists at gmail.com
Tue May 25 05:35:04 PDT 2010
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>
More information about the xorg-devel
mailing list