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

[Kevin Wolf <kwolf@redhat.com>]

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.

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.

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?

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

Kevin