Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in flat union

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10853 bytes --]

On 06/16/2016 08:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Recent commits added support for an anonymous type as the base
>> of a flat union; with a bit more work, we can also allow an
>> anonymous struct as a branch of a flat union.  This probably
>> most useful when a branch adds no additional members beyond the
>> common elements of the base (that is, the branch struct is '{}'),
>> but can be used for any struct in the same way we allow for an
>> anonymous struct for a command.
>>
>> The generator has to do a bit of special-casing for the fact that
>> we do not emit a '_empty' struct nor a 'visit_type__empty_members()'
>> corresponding to the special ':empty' type; but when the branch
>> is truly empty, there's nothing to do.
> 
> Well, it could emit them, if it makes things easier.
> 
>> The testsuite gets an update to use the new feature, and to ensure
>> that we can still detect invalid collisions of QMP names.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> @@ -1061,6 +1063,9 @@ class QAPISchemaMember(object):
>>                  return '(parameter of %s)' % owner[:-4]
>>              elif owner.endswith('-base'):
>>                  return '(base of %s)' % owner[:-5]
>> +            elif owner.endswith('-branch'):
>> +                return ('(member of %s branch %s)'
>> +                        % tuple(owner[:-7].split(':')))
> 
> I think we should point to the spot that puts in the colon, and back.
> 
> Do we really need the "of %s" part?
> 

If you think the message reads okay without it, then we can avoid...

>>              else:
>>                  assert owner.endswith('-wrapper')
>>                  # Unreachable and not implemented
>> @@ -1335,7 +1340,11 @@ class QAPISchema(object):
>>                                                self._make_members(data, info),
>>                                                None))
>>
>> -    def _make_variant(self, case, typ):
>> +    def _make_variant(self, case, typ, info, owner):
>> +        if isinstance(typ, dict):
>> +            typ = self._make_implicit_object_type(
>> +                "%s:%s" % (owner, case), info, 'branch',
>> +                self._make_members(typ, info)) or 'q_empty'
> 
> This is the spot.
> 

>> @@ -1485,7 +1494,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>>          type_name = prefix
>>      return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>>
>> -c_name_trans = string.maketrans('.-', '__')
>> +c_name_trans = string.maketrans('.-:', '___')
> 
> Because you use the colon as separator.  Hmm.
> 

...the need to transliterate : into _.  More below[1]


>> +++ b/scripts/qapi-types.py
>> @@ -61,7 +61,8 @@ def gen_object(name, base, members, variants):
>    def gen_object(name, base, members, variants):
>        if name in objects_seen:
>            return ''
>        objects_seen.add(name)
> 
>>      ret = ''
>>      if variants:
>>          for v in variants.variants:
>> -            if isinstance(v.type, QAPISchemaObjectType):
>> +            if (isinstance(v.type, QAPISchemaObjectType)
>> +                    and not (v.type.is_implicit() and v.type.is_empty())):
>>                  ret += gen_object(v.type.name, v.type.base,
>>                                    v.type.local_members, v.type.variants)
>>
> 
> This is the recursion that ensures an object type's variant member types
> are emitted before the object type.
> 
> We can't simply .type == schema.the_empty_object_type like
> qapi-introspect.py does, because we don't have schema handy here.  Hmm.
> 
> Do we really need this change?  Note that gen_object() does nothing for
> name in objects_seen, and we do this in visit_begin():
> 
>         # gen_object() is recursive, ensure it doesn't visit the empty type
>         objects_seen.add(schema.the_empty_object_type.name)

Cool! Remember, these patches have been through a lot of rebase churn -
I did indeed originally have to add this check to work around the empty
type (back when it was based on v5 of the "unboxed visits" series), but
that was before incorporating your suggestions in commit 7ce106a9 of the
nicer recursion prevention (v6 of the "unboxed visits", which is what
got pulled).  You are indeed correct that even without this hunk, things
still work just fine - I just never noticed that across all the rebasing.

> 
>> @@ -123,11 +124,14 @@ def gen_variants(variants):
>>                  c_name=c_name(variants.tag_member.name))
>>
>>      for var in variants.variants:
> 
> Here, we emit the C union member for a variant:
> 
>> +        typ = var.type.c_unboxed_type()
>> +        if (isinstance(var.type, QAPISchemaObjectType) and
>> +                var.type.is_empty() and var.type.is_implicit()):
>> +            typ = 'char'
>>          ret += mcgen('''
>>          %(c_type)s %(c_name)s;
>>  ''',
>> -                     c_type=var.type.c_unboxed_type(),
>> -                     c_name=c_name(var.name))
>> +                     c_type=typ, c_name=c_name(var.name))
> 
> Your change replaces the C type when var.type is the empty type.
> Without that, we'd get 'q_empty', I guess.
> 
> Do we need the union member?  Hmm, we may want to avoid empty unions, to
> avoid pedantic warnings.
> 
> Should we make the_empty_object_type.c_unboxed_type() return a suitable
> C type instead of 'q_empty'?

Sounds like something worthwhile to try, and simpler to handle if it works.

> 
>>
>>      ret += mcgen('''
>>      } u;
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 07ae6d1..46f8b39 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -79,13 +79,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>>          for var in variants.variants:
>>              ret += mcgen('''
>>      case %(case)s:
> 
> Here, we emit the visit of the "active" C union member:
> 
>> -        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
>> -        break;
>>  ''',
>>                           case=c_enum_const(variants.tag_member.type.name,
>>                                             var.name,
>> -                                           variants.tag_member.type.prefix),
>> -                         c_type=var.type.c_name(), c_name=c_name(var.name))
>> +                                           variants.tag_member.type.prefix))
>> +            if (not isinstance(var.type, QAPISchemaObjectType) or
>> +                    not var.type.is_empty()):
>> +                ret += mcgen('''
>> +        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
>> +''',
>> +                             c_type=var.type.c_name(), c_name=c_name(var.name))
>> +            ret += mcgen('''
>> +        break;
>> +''')
> 
> Your change suppresses the visit_type_FOO_members() call when var.type
> is the empty type.  Without that, we'd get visit_type_q_empty_members(),
> which doesn't exist.
> 
> Why not make it exist?

If .c_unboxed_type() returns 'char' instead of the name of an empty
type, then we are not only avoiding 'q_empty branch;' where we need to
avoid a missing visit_type_q_empty_members(), but we would also be
changing CpuInfo's 'CpuInfoOther other' to 'char other', and would want
to avoid generating visit_type_CpuInfoOther_members().  Then again, one
of the end goals is getting rid of the need for an explicit empty type
in the .json files (CpuInfoOther disappears, to be replaced by an
anonymous branch).

I guess it's a tradeoff for whether special casing the empty type makes
things easier or harder for the rest of the code.  But commit 7ce106a9
documented some reasons for why making visit_type_q_empty_members()
exist is rather hard to achieve - as a built-in type, we would have to
solve problems on how to avoid it having multiple declarations between
the main QMP use and the QGA use.


>> +++ b/tests/qapi-schema/flat-union-inline.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 'TestUnion' should be a type name
>> +tests/qapi-schema/flat-union-inline.json:6: 'kind' (member of TestUnion branch value2) collides with 'kind' (member of Base)
>> diff --git a/tests/qapi-schema/flat-union-inline.json b/tests/qapi-schema/flat-union-inline.json
>> index 62c7cda..a049ec8 100644
>> --- a/tests/qapi-schema/flat-union-inline.json
>> +++ b/tests/qapi-schema/flat-union-inline.json
>> @@ -1,5 +1,4 @@
>> -# we require branches to be a struct name
>> -# TODO: should we allow anonymous inline branch types?
>> +# we allow anonymous union branches, but they must not have clashing names
>>  { 'enum': 'TestEnum',
>>    'data': [ 'value1', 'value2' ] }
>>  { 'struct': 'Base',
>> @@ -8,4 +7,4 @@
>>    'base': 'Base',
>>    'discriminator': 'enum1',
>>    'data': { 'value1': { 'string': 'str' },
>> -            'value2': { 'integer': 'int' } } }
>> +            'value2': { 'kind': 'int' } } }

[1] Here's an example of the error message that would be changed if we
simplify the '%s:%s' string notation mentioned above.  Let's try the
shorter version:

'kind' (member of branch value2) collides with 'kind' (member of Base)

Okay, that might work.  But the other thing we have to worry about is
collision prevention.  When a branch type is anonymous, we synthesize a
visit_type_FOO_members() call - and so we must ensure FOO is unique.  If
two different flat unions both have a branch named 'bar', the current
patch proposal gives:

visit_type_q_obj_Foo1_bar_branch_members()
visit_type_q_obj_Foo2_bar_branch_members()

where the type owning the branch is the only thing different; but
changing the generated name to avoid the colon would produce collisions:

visit_type_q_obj_bar_branch_members() // from Foo1
visit_type_q_obj_bar_branch_members() // from Foo2

So we really DO want both the type name and the branch name included in
the generated name; what's more, since both type names AND branch names
can include '-' (and even '_'), we need a third character to split on if
we are going to have _pretty_owner() do a proper split back into legible
error messages.


>> @@ -81,7 +81,8 @@
>>    'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' },
>>    'discriminator': 'enum1',
>>    'data': { 'value1' : 'UserDefC', # intentional forward reference
>> -            'value2' : 'UserDefB' } }
> 
> You're deleting the equally intentional case of a backward reference :)

Okay, I'll add the new code as a new branch, rather than deleting an
existing one.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]