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

[Markus Armbruster <armbru@redhat.com>]

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>
> ---
>  docs/devel/qapi-code-gen.txt           | 105 ++++++++++++++++++++++++-
>  docs/sphinx/qapidoc.py                 |   2 +-
>  scripts/qapi/expr.py                   |  36 ++++++++-
>  scripts/qapi/schema.py                 |  30 +++++--
>  scripts/qapi/types.py                  |   4 +-
>  scripts/qapi/visit.py                  |  34 +++++++-
>  tests/qapi-schema/test-qapi.py         |   7 +-
>  tests/qapi-schema/double-type.err      |   2 +-
>  tests/qapi-schema/unknown-expr-key.err |   2 +-
>  9 files changed, 203 insertions(+), 19 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 6906a06ad2..247c4b8ef4 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -231,7 +231,8 @@ Syntax:
>                 'data': MEMBERS,
>                 '*base': STRING,
>                 '*if': COND,
> -               '*features': FEATURES }
> +               '*features': FEATURES,
> +               '*aliases': ALIASES }
>      MEMBERS = { MEMBER, ... }
>      MEMBER = STRING : TYPE-REF
>             | STRING : { 'type': TYPE-REF,
> @@ -279,6 +280,9 @@ the schema" below for more on this.
>  The optional 'features' member specifies features.  See "Features"
>  below for more on this.
>  
> +The optional 'aliases' member specifies aliases.  See "Aliases" below
> +for more on this.
> +
>  
>  === Union types ===
>  
> @@ -286,13 +290,15 @@ Syntax:
>      UNION = { 'union': STRING,
>                'data': BRANCHES,
>                '*if': COND,
> -              '*features': FEATURES }
> +              '*features': FEATURES,
> +              '*aliases': ALIASES }
>            | { 'union': STRING,
>                'data': BRANCHES,
>                'base': ( MEMBERS | STRING ),
>                'discriminator': STRING,
>                '*if': COND,
> -              '*features': FEATURES }
> +              '*features': FEATURES,
> +              '*aliases': ALIASES }
>      BRANCHES = { BRANCH, ... }
>      BRANCH = STRING : TYPE-REF
>             | STRING : { 'type': TYPE-REF, '*if': COND }
> @@ -402,6 +408,9 @@ the schema" below for more on this.
>  The optional 'features' member specifies features.  See "Features"
>  below for more on this.
>  
> +The optional 'aliases' member specifies aliases.  See "Aliases" below
> +for more on this.
> +
>  
>  === Alternate types ===
>  
> @@ -837,6 +846,96 @@ shows a conditional entity only when the condition is satisfied in
>  this particular build.
>  
>  
> +=== Aliases ===
> +
> +Object types, including structs and unions, can contain alias
> +definitions.
> +
> +Aliases define alternative member names that may be used in the
> +external representation to provide a value for a member in the same
> +object or in a nested object.

I get what you mean by "external representation", but let's use "wire"
for consistency with existing text.  It's defined in section
"Introduction", and used throughout the text.

Aside: we'll eventually have to revamp the document to cater for "client
protocols" other than JSON, such as the one we get with keyval_parse()
and qobject_input_visitor_new_keyval().

Moreover, aliases apply just to the input direction of the wire.

What about "may be used in wire input"?

> +
> +Syntax:
> +    ALIASES = [ ALIAS, ... ]
> +    ALIAS = { '*name': STRING,
> +              'source': [ STRING, ... ] }
> +
> +If 'name' is present, then the single member referred to by 'source'
> +is made accessible with the name given in 'name' in the type where the

"given by 'name'"?

> +alias definition is specified.
> +
> +If 'name' is not present, then all members in the object referred to
> +by 'source' are made accessible in the type where the alias definition
> +is specified with the same name as they have in 'source'.
> +
> +'source' is a non-empty list of member names representing the path to
> +an object member. The first name is resolved in the same object.  Each
> +subsequent member is resolved in the object named by the preceding
> +member.
> +
> +Example: Alternative name for a member in the same object (the member
> +"path" may be given through its alias "filename" in the external
> +representation):
> +
> +{ 'struct': 'File',
> +  'data': { 'path': 'str' },
> +  'aliases': [ { 'name': 'filename', 'source': ['path'] } ] }

What about

   Example: Alternative name for a member in the same object

   { 'struct': 'File',
     'data': { 'path': 'str' },
     'aliases': [ { 'name': 'filename', 'source': ['path'] } ] }

   Member "path" may instead be given through its alias "filename" in
   input.

> +
> +Example: Alias for a member in a nested object:

Let's drop the second colon.

> +
> +{ 'struct': 'A',
> +  'data': { 'zahl': 'int' } }
> +{ 'struct': 'B',
> +  'data': { 'drei': 'A' } }
> +{ 'struct': 'C',
> +  'data': { 'zwei': 'B' } }
> +{ 'struct': 'D',
> +  'data': { 'eins': 'C' },
> +  'aliases': [ { 'name': 'number',
> +                 'source': ['eins', 'zwei', 'drei', 'zahl' ] },
> +               { 'name': 'the_B',
> +                 'source': ['eins','zwei'] } ] }
> +
> +With this definition, each of the following inputs mean the same:

"inputs for 'D'"

> +
> +* { 'eins': { 'zwei': { 'drei': { 'zahl': 42 } } } }
> +
> +* { 'the_B': { 'drei': { 'zahl': 42 } } }
> +
> +* { 'number': 42 }
> +
> +Example: Flattening a union with a wildcard alias that maps all
> +members of 'data' to the top level:

"Flattening a simple union"

Let's drop the second colon.

> +
> +{ 'union': 'SocketAddress',
> +  'data': {
> +    'inet': 'InetSocketAddress',
> +    'unix': 'UnixSocketAddress' },
> +  'aliases': [ { 'source': ['data'] } ] }
> +
> +Aliases are transitive: 'source' may refer to another alias name.  In
> +this case, the alias is effectively an altenative name for the source

Typo in "alternative".

> +of the other alias.
> +
> +Example: Giving "the_answer" on the top level provides a value for
> +"zahl" in the nested object:
> +
> +{ 'struct': 'A',
> +  'data': { 'zahl': 'int' },
> +  'aliases': [ { 'name': 'number', 'source': ['zahl'] } ] }
> +{ 'struct': 'B',
> +  'data': { 'nested': 'A' },
> +  'aliases': [ { 'name': 'the_answer',
> +                 'source': ['nested', 'number'] } ] }

Do we need both this one and the "Example: Alias for a member in a
nested object" above?

> +
> +In order to accommodate unions where variants differ in structure, it
> +is allowed to use a path that doesn't necessarily match an existing
> +member in every variant or even at all; in this case, the alias
> +remains unused.  Note that the QAPI generator does not check whether
> +there is at least one branch for which an alias could match.  If a
> +source member is misspelt, the alias just won't work.

In my review of PATCH 1, I wondered whether this patch would be
sufficiently explicit on "does not check".  It is.

> +
> +
>  === Documentation comments ===
>  
>  A multi-line comment that starts and ends with a '##' line is a
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e03abcbb95..6c94c01148 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
>                        + self._nodes_for_if_section(ifcond))
>  
>      def visit_object_type(self, name, info, ifcond, features,
> -                          base, members, variants):
> +                          base, members, variants, aliases):
>          doc = self._cur_doc
>          if base and base.is_implicit():
>              base = None
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 2fcaaa2497..743e23ec85 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -198,6 +198,34 @@ def check_features(features, info):
>          check_if(f, info, source)
>  
>  
> +def check_aliases(aliases, info):
> +    if aliases is None:
> +        return
> +    if not isinstance(aliases, list):
> +        raise QAPISemError(info, "'aliases' must be an array")
> +    for a in aliases:
> +        if not isinstance(a, dict):
> +            raise QAPISemError(info, "'aliases' members must be objects")
> +        check_keys(a, info, "'aliases' member", ['source'], ['name'])
> +
> +        if 'name' in a:
> +            source = "alias member 'name'"
> +            check_name_is_str(a['name'], info, source)
> +            check_name_str(a['name'], info, source)
> +
> +        if not isinstance(a['source'], list):
> +            raise QAPISemError(info,
> +                "alias member 'source' must be an array")
> +        if not a['source']:
> +            raise QAPISemError(info,
> +                "alias member 'source' must not be empty")
> +
> +        source = "member of alias member 'source'"
> +        for s in a['source']:
> +            check_name_is_str(s, info, source)
> +            check_name_str(s, info, source)
> +
> +
>  def check_enum(expr, info):
>      name = expr['enum']
>      members = expr['data']
> @@ -228,6 +256,7 @@ def check_struct(expr, info):
>  
>      check_type(members, info, "'data'", allow_dict=name)
>      check_type(expr.get('base'), info, "'base'")
> +    check_aliases(expr.get('aliases'), info)
>  
>  
>  def check_union(expr, info):
> @@ -245,6 +274,8 @@ def check_union(expr, info):
>              raise QAPISemError(info, "'discriminator' requires 'base'")
>          check_name_is_str(discriminator, info, "'discriminator'")
>  
> +    check_aliases(expr.get('aliases'), info)
> +
>      for (key, value) in members.items():
>          source = "'data' member '%s'" % key
>          check_name_str(key, info, source)
> @@ -331,7 +362,7 @@ def check_exprs(exprs):
>          elif meta == 'union':
>              check_keys(expr, info, meta,
>                         ['union', 'data'],
> -                       ['base', 'discriminator', 'if', 'features'])
> +                       ['base', 'discriminator', 'if', 'features', 'aliases'])
>              normalize_members(expr.get('base'))
>              normalize_members(expr['data'])
>              check_union(expr, info)
> @@ -342,7 +373,8 @@ def check_exprs(exprs):
>              check_alternate(expr, info)
>          elif meta == 'struct':
>              check_keys(expr, info, meta,
> -                       ['struct', 'data'], ['base', 'if', 'features'])
> +                       ['struct', 'data'],
> +                       ['base', 'if', 'features', 'aliases'])
>              normalize_members(expr['data'])
>              check_struct(expr, info)
>          elif meta == 'command':
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 353e8020a2..14a2b0175b 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -118,7 +118,7 @@ class QAPISchemaVisitor:
>          pass
>  
>      def visit_object_type(self, name, info, ifcond, features,
> -                          base, members, variants):
> +                          base, members, variants, aliases):
>          pass
>  
>      def visit_object_type_flat(self, name, info, ifcond, features,
> @@ -362,9 +362,19 @@ class QAPISchemaArrayType(QAPISchemaType):
>          return "%s type ['%s']" % (self.meta, self._element_type_name)
>  
>  
> +class QAPISchemaAlias:
> +    def __init__(self, name, source):
> +        assert name is None or isinstance(name, str)
> +        assert source
> +        for member in source:
> +            assert isinstance(member, str)
> +        self.name = name
> +        self.source = source
> +
> +
>  class QAPISchemaObjectType(QAPISchemaType):
>      def __init__(self, name, info, doc, ifcond, features,
> -                 base, local_members, variants):
> +                 base, local_members, variants, aliases=None):
>          # struct has local_members, optional base, and no variants
>          # flat union has base, variants, and no local_members
>          # simple union has local_members, variants, and no base
> @@ -382,6 +392,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>          self.local_members = local_members
>          self.variants = variants
>          self.members = None
> +        self.aliases = aliases or []
>  
>      def check(self, schema):
>          # This calls another type T's .check() exactly when the C
> @@ -474,7 +485,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>          super().visit(visitor)
>          visitor.visit_object_type(
>              self.name, self.info, self.ifcond, self.features,
> -            self.base, self.local_members, self.variants)
> +            self.base, self.local_members, self.variants, self.aliases)
>          visitor.visit_object_type_flat(
>              self.name, self.info, self.ifcond, self.features,
>              self.members, self.variants)
> @@ -964,6 +975,12 @@ class QAPISchema:
>          return [QAPISchemaFeature(f['name'], info, f.get('if'))
>                  for f in features]
>  
> +    def _make_aliases(self, aliases):
> +        if aliases is None:
> +            return []
> +        return [QAPISchemaAlias(a.get('name'), a['source'])
> +                for a in aliases]
> +
>      def _make_enum_members(self, values, info):
>          return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
>                  for v in values]
> @@ -1038,11 +1055,12 @@ class QAPISchema:
>          base = expr.get('base')
>          data = expr['data']
>          ifcond = expr.get('if')
> +        aliases = self._make_aliases(expr.get('aliases'))
>          features = self._make_features(expr.get('features'), info)
>          self._def_entity(QAPISchemaObjectType(
>              name, info, doc, ifcond, features, base,
>              self._make_members(data, info),
> -            None))
> +            None, aliases))
>  
>      def _make_variant(self, case, typ, ifcond, info):
>          return QAPISchemaVariant(case, info, typ, ifcond)
> @@ -1061,6 +1079,7 @@ class QAPISchema:
>          data = expr['data']
>          base = expr.get('base')
>          ifcond = expr.get('if')
> +        aliases = self._make_aliases(expr.get('aliases'))
>          features = self._make_features(expr.get('features'), info)
>          tag_name = expr.get('discriminator')
>          tag_member = None
> @@ -1085,7 +1104,8 @@ class QAPISchema:
>              QAPISchemaObjectType(name, info, doc, ifcond, features,
>                                   base, members,
>                                   QAPISchemaVariants(
> -                                     tag_name, info, tag_member, variants)))
> +                                     tag_name, info, tag_member, variants),
> +                                 aliases))
>  
>      def _def_alternate_type(self, expr, info, doc):
>          name = expr['alternate']
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2bdd626847..c8306479f5 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -25,6 +25,7 @@ from .common import (
>  from .gen import QAPISchemaModularCVisitor, ifcontext
>  from .schema import (
>      QAPISchema,
> +    QAPISchemaAlias,
>      QAPISchemaEnumMember,
>      QAPISchemaFeature,
>      QAPISchemaObjectType,
> @@ -332,7 +333,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],
>                            members: List[QAPISchemaObjectTypeMember],
> -                          variants: Optional[QAPISchemaVariants]) -> None:
> +                          variants: Optional[QAPISchemaVariants],
> +                          aliases: List[QAPISchemaAlias]) -> None:
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
> 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 });

The more old-fashioned

       visit_define_alias(v, "bar", "nested", "foo", NULL);

with variadic visit_define_alias() feels tempting.

> +''',
> +                     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;
>  }
> @@ -361,14 +386,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],
>                            members: List[QAPISchemaObjectTypeMember],
> -                          variants: Optional[QAPISchemaVariants]) -> None:
> +                          variants: Optional[QAPISchemaVariants],
> +                          aliases: List[QAPISchemaAlias]) -> None:
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.add(gen_visit_members_decl(name))
> -            self._genc.add(gen_visit_object_members(name, base,
> -                                                    members, variants))
> +            self._genc.add(gen_visit_object_members(
> +                name, base, members, variants, aliases))
>              # TODO Worth changing the visitor signature, so we could
>              # directly use rather than repeat type.is_implicit()?
>              if not name.startswith('q_'):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index e8db9d09d9..1679d1b5da 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -47,7 +47,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          self._print_if(ifcond)
>  
>      def visit_object_type(self, name, info, ifcond, features,
> -                          base, members, variants):
> +                          base, members, variants, aliases):
>          print('object %s' % name)
>          if base:
>              print('    base %s' % base.name)
> @@ -56,6 +56,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>                    % (m.name, m.type.name, m.optional))
>              self._print_if(m.ifcond, 8)
>              self._print_features(m.features, indent=8)
> +        for a in aliases:
> +            if a.name:
> +                print('    alias %s -> %s' % (a.name, '.'.join(a.source)))
> +            else:
> +                print('    alias * -> %s.*' % '.'.join(a.source))
>          self._print_variants(variants)
>          self._print_if(ifcond)
>          self._print_features(features)
> diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
> index 71fc4dbb52..5d25d7623c 100644
> --- a/tests/qapi-schema/double-type.err
> +++ b/tests/qapi-schema/double-type.err
> @@ -1,3 +1,3 @@
>  double-type.json: In struct 'bar':
>  double-type.json:2: struct has unknown key 'command'
> -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
> diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
> index c5f395bf79..7429d1ff03 100644
> --- a/tests/qapi-schema/unknown-expr-key.err
> +++ b/tests/qapi-schema/unknown-expr-key.err
> @@ -1,3 +1,3 @@
>  unknown-expr-key.json: In struct 'bar':
>  unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
> -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.