Re: [Qemu-devel] [PATCH v11 11/28] qapi: Check for qapi collisions of flat union branches

[Eric Blake]

On 11/11/2015 06:42 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:

> Your patch actually does two things:
> 
> 1. Extend the "object type cannot contain itself" check to cover its
>    variant types in addition to base type.  This is the two-liner change
>    to QAPISchemaObjectTypeVariants.check().
> 
> 2. Extend the "object type cannot contain members with clashing names"
>    to cover variant members.
> 
> They're related: you need the former's side effect to compute .members
> to be able to do the latter.
> 
> Doing it in two separate patches *might* make explaining it in the
> commit message easier.  See below.

Interesting observation.  I can split if you still think it is worth it;
otherwise, I think your wording makes both changes obvious:


> Let me have a stab at the commit message of the *unsplit* patch, use as
> you see fit:
> 
> qapi: Check for collisions involving variant members
> 
> Right now, our ad hoc parser ensures that we cannot have a flat union
> that introduces any members that would clash with non-variant members
> inherited from the union's base type (see flat-union-clash-member.json).
> We want QAPISchemaObjectType.check() to make the same check, so we can
> later reduce some of the ad hoc checks.
> 
> We already have a map 'seen' of all non-variant members.  We still need
> to check for collisions between each variant type's members and the
> non-variant ones.
> 
> To know the variant type's members, we need to call
> variant.type.check().  This also detects when a type contains itself in
> a variant, exactly like the existing base.check() detects when a type
> contains itself as a base.

If I split, this paragraph would go in the first patch (adding the
.check() for cycle detection).

> 
> Slight complication: an alternate's variant can have arbitrary type, but
> only an object type's check() may be called outside QAPISchema.check().
> We could either skip the call for variants of alternates, or skip it for
> non-object types.  Do the latter, because it's easier.

I actually have some churn on this; later in 22/28, I'm stuck making
Variants.check() do something for unions only, and had to reintroduce an
'if seen:' conditional, at which point I moved the v.type.check() back
to unions only.  I guess your review of that patch may determine whether
I minimize churn back here, or add a 'for now' phrase to the commit
message, or just not worry about it.

> 
> Then we can call each variant member's check_clash() with the
> appropriate 'seen' map.  Since members of different variants can't
> clash, We have to clone a fresh seen for each variant.  Wrap this in the
> new helper method QAPISchemaObjectTypeVariants.check_clash().
> 
> Note that cloning 'seen' inside Variants.check_clash()
> resembles the one we just removed from Variants.check(); the
> difference here is that we are now checking for clashes
> among the qapi members of the variant type, rather than for
> a single clash with the variant tag name itself.
> 
> Note that by construction collisions can't actually happen for simple
> unions: each variant's type is a wrapper with a single member 'data',
> which will never collide with the only non-variant member 'type'.
> 
> For alternates, there's nothing for a variant object type's members to
> clash with, and therefore no need to call variants.check_clash().
> 
> No change to generated code.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature