Re: [Qemu-devel] [PATCH v2 16/19] qapi: Allow anonymous base for flat union

[Eric Blake]

On 03/03/2016 06:04 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Rather than requiring all flat unions to explicitly create
>> a separate base struct, we can allow the qapi schema to specify
>> the common members via an inline dictionary. This is similar to
>> how commands can specify an inline anonymous type for its 'data',
> 
> Suggest to end the sentence here, and then...
> 
>> and matches the fact that our code base already has several
>> flat unions that had to create a separate base type that is used
>> nowhere but in the union.
> 
> "We already have several struct types that only exist to serve as a
> single flat union's base.  The next commits will clean them up."
> 
> Replace "them" by "some" if you don't clean them all up.
> 
> It's a nice step towards having a variant record type in the schema
> language similar to what we have in introspection.
> 

>> @@ -63,7 +62,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
>> *obj, Error **errp)
>>                  c_name=c_name(name))
>>
>>      if base:
>> -        ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
>> +        ret += gen_visit_members_call(base, 'qapi_%s_base(obj)' % 
>> c_name(name),
> 
> I started at this for several minutes until I could guess what's going
> on here.
> 
> The old code works fine when the type isn't implicit.
> 
> When it is, it fails the assertion in base.c_name(), even though
> gen_visit_members_call() is not going to use its value.
> 
> You hack around it by passing 'qapi_NAME_base(obj)' instead.
> 
> If NAME isn't implicit, the function exists, and does the same as the
> expression it replaces.
> 
> If NAME is implicit, the function doesn't exist, but
> gen_visit_members_call() doesn't care, because it doesn't use the
> argument then.
> 
> Ugh!  More evidence that we better not munge the two cases together into
> one function.

Even with my v4 work towards exposing implicit types as a concrete
struct, I'm still not creating qapi_NAME_base(obj) for objects with an
implicit type.  But '(_obj_FOO_base *)FOO' works well for a base with a
concrete implicit base type.


>> @@ -354,7 +355,7 @@ code generator can ensure that branches exist for all 
>> values of the
>>  enum (although the order of the keys need not match the declaration of
>>  the enum).  In the resulting generated C data types, a flat union is
>>  represented as a struct with the base member fields included directly,
>> -and then a union of structures for each branch of the struct.
>> +and then a union of pointers to structures for each branch of the struct.
> 
> Uh, that became wrong in commit 544a373 already, didn't it?
> 
> Is that a bug in PATCH 3 then?

Yes, and fixed up accordingly in my v3 respin.  (I think it was some
rebase conflicts that I resolved incorrectly at some point).


>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -75,14 +75,10 @@
>>    'base': 'UserDefZero',
>>    'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>>
>> -{ 'struct': 'UserDefUnionBase2',
>> -  'base': 'UserDefZero',
>> -  'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
>> -
>>  # this variant of UserDefFlatUnion defaults to a union that uses fields with
>>  # allocated types to test corner cases in the cleanup/dealloc visitor
>>  { 'union': 'UserDefFlatUnion2',
>> -  'base': 'UserDefUnionBase2',
>> +  'base': { 'string': 'str', 'enum1': 'QEnumTwo' },
> 
> You lost member 'integer' from the base's base.  Harmless (I think), but
> visible when you compare generated output.

Easy enough to keep.

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

signature.asc
Description: OpenPGP digital signature