Re: [PATCH v2 5/6] qapi: Add support for aliases

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.02.2021 um 16:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Introduce alias definitions for object types (structs and unions). This
>> > allows using the same QAPI type and visitor for many syntax variations
>> > that exist in the external representation, like between QMP and the
>> > command line. It also provides a new tool for evolving the schema while
>> > maintaining backwards compatibility during a deprecation period.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> [...]
>> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> > index 22e62df901..e370485f6e 100644
>> > --- a/scripts/qapi/visit.py
>> > +++ b/scripts/qapi/visit.py
>> > @@ -26,6 +26,7 @@ from .common import (
>> >  from .gen import QAPISchemaModularCVisitor, ifcontext
>> >  from .schema import (
>> >      QAPISchema,
>> > +    QAPISchemaAlias,
>> >      QAPISchemaEnumMember,
>> >      QAPISchemaEnumType,
>> >      QAPISchemaFeature,
>> > @@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
>> >  def gen_visit_object_members(name: str,
>> >                               base: Optional[QAPISchemaObjectType],
>> >                               members: List[QAPISchemaObjectTypeMember],
>> > -                             variants: Optional[QAPISchemaVariants]) -> str:
>> > +                             variants: Optional[QAPISchemaVariants],
>> > +                             aliases: List[QAPISchemaAlias]) -> str:
>> >      ret = mcgen('''
>> >  
>> >  bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>> > @@ -68,6 +70,24 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>> >  ''',
>> >                  c_name=c_name(name))
>> >  
>> > +    if aliases:
>> > +        ret += mcgen('''
>> > +    visit_start_alias_scope(v);
>> > +''')
>> > +
>> > +    for a in aliases:
>> > +        if a.name:
>> > +            name = '"%s"' % a.name
>> > +        else:
>> > +            name = "NULL"
>> > +
>> > +        source = ", ".join('"%s"' % x for x in a.source)
>> > +
>> > +        ret += mcgen('''
>> > +    visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
>> > +''',
>> > +                     name=name, source=source)
>> > +
>> >      if base:
>> >          ret += mcgen('''
>> >      if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
>> > @@ -133,6 +153,11 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>> >      }
>> >  ''')
>> >  
>> > +    if aliases:
>> > +        ret += mcgen('''
>> > +    visit_end_alias_scope(v);
>> > +''')
>> > +
>> >      ret += mcgen('''
>> >      return true;
>> >  }
>> 
>> The visit_type_FOO_members() are primarily helpers for the
>> visit_type_FOO().  But they also get called
>> 
>> * by visit_type_BAR_members() when
>> 
>>   - struct or union BAR has 'base': 'FOO'
>>   - union or alternate BAR a variant 'FOO'
>> 
>> * by qmp_marshal_CMD() when
>> 
>>   - CMD has 'boxed': true, 'data': 'FOO'
>> 
>> Have you considered these cases?
>> 
>> How's the test coverage?
>
> What is the difference between these cases? The visiting should work the
> same, no matter from where it was started.
>
> I did consider the struct base/union variant case and this is why I
> introduced visit_start/end_alias_scope so that aliases wouldn't leak to
> the outer level.

Good!

The qmp_marshal_CMD()'s code around visit_type_FOO_members() is fairly
close to visit_type_FOO()'s code: it avoids allocation and drops cases
that can't happen with the visitor at hand.  Can't see why aliases would
not work.

"Can't see why it would not work" is of course a sensible precondition
for testing, not an excuse for not testing.

> Now that I'm trying to think of a test case, this probably only protects
> against weird corner cases: The input object is the same anyway, so I
> guess the only way for this to make a difference is when the base struct
> defines an alias for a member that it doesn't even have (so the alias
> would remain unused when applied to the base struct independently), but
> that exists in the struct in which it is embedded.

It's best to strangle such weird corner cases in the crib, before they
can conspire with other weird corner cases to make a real mess.

> I hope adding a test case that checks that this is an error should be
> easy. Would the right place be tests/test-qobject-input-visitor.c?

Sounds good to me.

> Can you think of any other specific differences that need to be tested?

I think you nailed the negative tests.

The positive tests are limited to frontend tests now, i.e. put some
valid use into qapi-schema-test.json, then check the resulting
qapi-schema-test.out looks sane.  Covered there, I think:

* empty 'aliases' have no effect: AliasStruct0

  Aside: we could choose to outlaw instead.

* alias a struct member: AliasStruct1

* alias a member of a struct member: AliasStruct2

* alias all members of a struct member: AliasStruct3

* alias a member of a flat union variant: AliasFlatUnion

* alias all members of all simple union variants: AliasSimpleUnion

Missing, I think:

* alias a member of the base struct

* alias a member of a member of a struct member

I figure both are relatively uninteresting as front-end tests, but unit
tests (below) will need them.

Possibly:

* alias a member of a simple union variant

* alias all members of a simple union variant

Not sure how much these buy us over the other tests.  Perhaps we should
spend our limited time on killing simple unions instead (I hope aliases
will help us get there).

Aside: we could add aliases to alternates, for rewriting a non-object
variant into an object variant.  Not now, and later only if we find
sufficiently compelling uses.

Unit tests go further than frontend tests: they test the generated
code.  And the hand-written code, of course.

We should cover the various paths through
qobject_input_try_get_object().

I'd like us to cover its use in the various contexts, i.e. from all
kinds of callers of visit_type_BAR_members().  See above for a list.

I don't think we need to do the full test matrix.  The inner workings of
qobject_input_try_get_object() look independent enough from the contexts
to justify risking a short cut.  So, cover the paths, and also cover the
contexts, but don't cover the paths for each context.

What do you think?

Feel free to suggest more tests, or more shortcuts.