[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!