[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/6] qapi: Add support for aliases
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 5/6] qapi: Add support for aliases |
Date: |
Wed, 17 Feb 2021 17:17:25 +0100 |
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
- [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases, (continued)
[PATCH v2 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor, Kevin Wolf, 2021/02/11
[PATCH v2 5/6] qapi: Add support for aliases, Kevin Wolf, 2021/02/11
[PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor, Kevin Wolf, 2021/02/11
Re: [PATCH v2 0/6] qapi: Add support for aliases, Markus Armbruster, 2021/02/24