qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [PATCH 5/6] qapi: Add support for aliases
Date: Wed, 10 Feb 2021 15:29:10 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)

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>
> ---
[...]
>  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..21fded529b 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -198,6 +198,32 @@ 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' entries must be objects")
> +        check_keys(a, info, "alias", ['source'], ['alias'])
> +
> +        if 'alias' in a:
> +            source = "alias member 'alias'"
> +            check_name_is_str(a['alias'], info, source)
> +            check_name_str(a['alias'], info, source)
> +
> +        if not isinstance(a['source'], list):
> +            raise QAPISemError(info, "'source' must be an array")
> +        if not a['source']:
> +            raise QAPISemError(info, "'source' must not be empty")
> +
> +        source = "element 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 +254,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 +272,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 +360,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 +371,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 720449feee..5daa137163 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -117,7 +117,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,
> @@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType):
>          return "%s type ['%s']" % (self.meta, self._element_type_name)
>  
>  
> +class QAPISchemaAlias:
> +    def __init__(self, alias, source):
> +        assert source
> +        self.alias = alias
> +        self.source = source

The existing QAPISchemaFOO.__init__() assert the arguments are sane.  I
expect John's type hinting work to replace these assertions.  Adding
type hints is much easier when you have assertions to replace.  Let's
add them here, too.

> +
> +
>  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
> @@ -351,6 +358,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
> @@ -443,7 +451,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)
> @@ -934,6 +942,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('alias'), 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]
> @@ -1008,11 +1022,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)
> @@ -1031,6 +1046,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
> @@ -1055,7 +1071,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 2b4916cdaa..e870bebb7e 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 339f152152..a35921ef2c 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,25 @@ 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.alias:
> +            alias = '"%s"' % a.alias
> +        else:
> +            alias = "NULL"
> +
> +        source_list = ", ".join('"%s"' % x for x in a.source)
> +        source = "(const char * []) { %s, NULL }" % source_list
> +
> +        ret += mcgen('''
> +    visit_define_alias(v, %(alias)s, %(source)s);
> +''',
> +                     alias=alias, source=source)
> +

I prefer putting C code into mcgen() as much as practical.  Like this:

           if a.alias:
               alias = '"%s"' % a.alias
           else:
               alias = "NULL"
           source = ", ".join('"%s"' % x for x in a.source)
           ret += mcgen('''
       visit_define_alias(v, %(alias)s, (const char * []){ %(source), NULL });
   ''',
                        alias=alias, source=source)

>      if base:
>          ret += mcgen('''
>      if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
> @@ -133,6 +154,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 +387,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))
> +                                                    members, variants, 
> aliases))

Long line.  Recommend hanging indent:

               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..adf8bda89a 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.alias:
> +                print('    alias %s -> %s' % (a.alias, '/'.join(a.source)))
> +            else:
> +                print('    alias * -> %s/*' % '/'.join(a.source))

For better or worse, we use '.' as a separator elsewhere.

>          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'.

Straightforward, easy to review.  Appreciated!




reply via email to

[Prev in Thread] Current Thread [Next in Thread]