Re: [Qemu-devel] [PATCH RFC v2 27/47] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs

[Eric Blake]

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> Fixes flat unions to visit the base's base members (the previous
> commit merely added them to the struct).  Same test case.
> 
> Patch's effect on visit_type_UserDefFlatUnion():
> 
>      static void visit_type_UserDefFlatUnion_fields(Visitor *m, 
> UserDefFlatUnion **obj, Error **errp)
>      {
>          Error *err = NULL;
> 
>     +    visit_type_int(m, &(*obj)->integer, "integer", &err);
>     +    if (err) {
>     +        goto out;
>     +    }
>          visit_type_str(m, &(*obj)->string, "string", &err);
>          if (err) {
>              goto out;
> 

> +def gen_visit_union(name, base, variants):
> +    ret = ''
>  
>      if base:
> -        assert discriminator
> -        base_fields = find_struct(base)['data'].copy()
> -        del base_fields[discriminator]
> -        ret += generate_visit_struct_fields(name, base_fields)
> +        members = [m for m in base.members if m != variants.tag_member]
> +        ret += generate_visit_struct_fields(name, members)

Ouch. This hurts.  If the same class is used as both the base class of a
flat union, and the base class of an ordinary struct, then the struct
tries to visit the base class, but no longer parses the field that the
union was using as its discriminator.

We don't have any code that demonstrates this, but probably should.  I
ran into it while working up my POC of what it would take to unbox
inherited structs (http://thread.gmane.org/gmane.comp.emulators.qemu/353204)

We want visit_FOO_fields to visit _all_ fields of the struct, no matter
who called generate_visit_struct_fields().  So what you must instead do
here is use the fact that we've already visited the discriminator...

>  
> -    if discriminator:
> -        for key in members:
> -            ret += generate_visit_implicit_struct(members[key])
> +    for var in variants.variants:
> +        if var.flat:
> +            ret += generate_visit_implicit_struct(var.type)
>  
>      ret += mcgen('''
>  
> @@ -300,41 +268,39 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s 
> **obj, const char *name, Error
>  ''',
>                       name=c_name(name))
>  
> -    if not discriminator:
> -        tag = 'kind'
> -        disc_key = "type"
> -    else:
> -        tag = discriminator
> -        disc_key = discriminator
> +    disc_key = variants.tag_member.name
> +    if not variants.tag_name:
> +        # we pointlessly use a different key for simple unions
> +        disc_key = 'type'
>      ret += mcgen('''
> -        visit_type_%(disc_type)s(m, &(*obj)->%(c_tag)s, "%(disc_key)s", 
> &err);
> +        visit_type_%(disc_type)s(m, &(*obj)->%(c_name)s, "%(disc_key)s", 
> &err);

...and omit this call if the flat union's base class already took care
of it.

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

signature.asc
Description: OpenPGP digital signature