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

[Peter Hutterer]

On Tue, May 25, 2010 at 05:35:04AM -0700, Dan Nicholson wrote:
> > +
> > +    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.

comment added.

> > +    }
> > +
> > +    /* 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.

amended.

> > +        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.

done
> 
> > +
> > +    /* 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.

not quite, this one checks all tags1 against all tags2. So this part will
detect if (tag1[x] == tag2[y]), whereas the first loop will only detect
(tag1[x] == tag2[x]). I'd like to leave both checks in, the first because
it'll be the more common case if we ever bust this up and we're not strapped
for time here anyway.

> > +}
> > +
> > +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?

whoops, should be memcmp.
and funnily enough, when I just fired up the man page for memcmp, I typed
memcpy again...

so yeah, this check should be g_assert(memcmp(...) == 0);

> > +
> > +    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?

Mainly because I want to make sure that pointers don't get misguided. I've
had bugs in some protocol copy code where the value would get written in two
places but because it was written in a valid way, the tests didn't detect
it. This shouldn't be possible here with the tests, but doing this here
doesn't hurt much either.

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

thanks. I'll send out the corrected patch in a bit.
 
Cheers,
  Peter