Re: [Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Rather than requiring all flat unions to explicitly create
> a separate base struct, we can allow the qapi schema to specify
> the common members via an inline dictionary. This is similar to
> how commands can specify an inline anonymous type for its 'data'.
> We already have several struct types that only exist to serve as
> a single flat union's base; the next commit will clean them up.
>
> Now that anonymous bases are legal, we need to rework the
> flat-union-bad-base negative test (as previously written, it
> forms what is now valid QAPI; tweak it to now provide coverage
> of a new error message path), and add a positive test in
> qapi-schema-test to use an anonymous base (making the integer
> argument optional, for even more coverage).
>
> Note that this patch only allows anonymous bases for flat unions;
> simple unions are enough syntactic sugar that we do not want to
> burden them further.  Meanwhile, while it would be easy to also
> allow an anonymous base for structs, that would be quite
> redundant, as the members can be put right into the struct
> instead.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v4: rebase to use implicit type, improve commit message
> [no v3]

I think you also

* fixed a missing allow_optional=True in check_union()

* fixed a missing "non-optional" in qapi-code-gen.txt (mention in commit
  message?  separate patch?)

* renamed readonly to read-only in an example in qapi-code-gen.txt to be
  closer to the code (separate patch?)

> v2: rebase onto s/fields/members/ change
> v1: rebase and rework to use gen_visit_fields_call(), test new error
> Previously posted as part of qapi cleanup subset F:
> v6: avoid redundant error check in gen_visit_union(), rebase to
> earlier gen_err_check() improvements
> ---
>  scripts/qapi.py                            | 12 ++++++++++--
>  scripts/qapi-types.py                      | 10 ++++++----
>  docs/qapi-code-gen.txt                     | 30 
> +++++++++++++++---------------
>  tests/qapi-schema/flat-union-bad-base.err  |  2 +-
>  tests/qapi-schema/flat-union-bad-base.json |  5 ++---
>  tests/qapi-schema/qapi-schema-test.json    |  6 +-----
>  tests/qapi-schema/qapi-schema-test.out     | 10 +++++-----
>  7 files changed, 40 insertions(+), 35 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 0574b8b..227f47d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -327,6 +327,8 @@ class QAPISchemaParser(object):
>
>
>  def find_base_members(base):
> +    if isinstance(base, dict):
> +        return base
>      base_struct_define = find_struct(base)
>      if not base_struct_define:
>          return None
> @@ -560,9 +562,10 @@ def check_union(expr, expr_info):
>
>      # Else, it's a flat union.
>      else:
> -        # The object must have a string member 'base'.
> +        # The object must have a string or dictionary 'base'.
>          check_type(expr_info, "'base' for union '%s'" % name,
> -                   base, allow_metas=['struct'])
> +                   base, allow_dict=True, allow_optional=True,
> +                   allow_metas=['struct'])

This is where you added allow_optional=True.

>          if not base:
>              raise QAPIExprError(expr_info,
>                                  "Flat union '%s' must have a base"
> @@ -1049,6 +1052,8 @@ class QAPISchemaMember(object):
>              owner = owner[5:]
>              if owner.endswith('-arg'):
>                  return '(parameter of %s)' % owner[:-4]
> +            elif owner.endswith('-base'):
> +                return '(base of %s)' % owner[:-5]
>              else:
>                  assert owner.endswith('-wrapper')
>                  # Unreachable and not implemented
> @@ -1336,6 +1341,9 @@ class QAPISchema(object):
>          base = expr.get('base')
>          tag_name = expr.get('discriminator')
>          tag_member = None
> +        if isinstance(base, dict):
> +            base = (self._make_implicit_object_type(
> +                    name, info, 'base', self._make_members(base, info)))
>          if tag_name:
>              variants = [self._make_variant(key, value)
>                          for (key, value) in data.iteritems()]
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b8499ac..c003721 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -72,12 +72,14 @@ struct %(c_name)s {
>                   c_name=c_name(name))
>
>      if base:
> -        ret += mcgen('''
> +        if not base.is_implicit():
> +            ret += mcgen('''
>      /* Members inherited from %(c_name)s: */
>  ''',
> -                     c_name=base.c_name())
> +                         c_name=base.c_name())
>          ret += gen_struct_members(base.members)
> -        ret += mcgen('''
> +        if not base.is_implicit():
> +            ret += mcgen('''
>      /* Own members: */
>  ''')
>      ret += gen_struct_members(members)
> @@ -223,7 +225,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>      def visit_object_type(self, name, info, base, members, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
>          self.decl += gen_object(name, base, members, variants)
> -        if base:
> +        if base and not base.is_implicit():
>              self.decl += gen_upcast(name, base)
>          if name[0] != ':':
>              # implicit types won't be directly allocated/freed
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index e0b2ef1..a92d4da 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -284,7 +284,7 @@ better than open-coding the member to be type 'str'.
>  === Union types ===
>
>  Usage: { 'union': STRING, 'data': DICT }
> -or:    { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME,
> +or:    { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME-OR-DICT,
>           'discriminator': ENUM-MEMBER-OF-BASE }
>
>  Union types are used to let the user choose between several different
> @@ -320,24 +320,25 @@ an implicit C enum 'NameKind' is created, corresponding 
> to the union
>  the union can be named 'max', as this would collide with the implicit
>  enum.  The value for each branch can be of any type.
>
> -A flat union definition specifies a struct as its base, and
> -avoids nesting on the wire.  All branches of the union must be
> -complex types, and the top-level members of the union dictionary on
> -the wire will be combination of members from both the base type and the
> -appropriate branch type (when merging two dictionaries, there must be
> -no keys in common).  The 'discriminator' member must be the name of an
> -enum-typed member of the base struct.
> +A flat union definition avoids nesting on the wire, and specifies a
> +set of common members that occur in all variants of the union.  The
> +'base' key must specifiy either a type name (the type must be a
> +struct, not a union), or a dictionary representing an anonymous type.
> +All branches of the union must be complex types, and the top-level
> +members of the union dictionary on the wire will be combination of
> +members from both the base type and the appropriate branch type (when
> +merging two dictionaries, there must be no keys in common).  The
> +'discriminator' member must be the name of a non-optional enum-typed

This is where you supplied the missing "non-optional".

> +member of the base struct.
>

And below, you rename readonly to read-only.

>  The following example enhances the above simple union example by
> -adding a common member 'readonly', renaming the discriminator to
> +adding a common member 'read-only', renaming the discriminator to
>  something more applicable, and reducing the number of {} required on
>  the wire:
>
>   { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
> - { 'struct': 'BlockdevCommonOptions',
> -   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>   { 'union': 'BlockdevOptions',
> -   'base': 'BlockdevCommonOptions',
> +   'base': { 'driver': 'BlockdevDriver', 'read-only': 'bool' },
>     'discriminator': 'driver',
>     'data': { 'file': 'FileOptions',
>               'qcow2': 'Qcow2Options' } }
> @@ -346,7 +347,7 @@ Resulting in these JSON objects:
>
>   { "driver": "file", "readonly": true,
>     "filename": "/some/place/my-image" }
> - { "driver": "qcow2", "readonly": false,
> + { "driver": "qcow2", "read-only": false,
>     "backing-file": "/some/place/my-image", "lazy-refcounts": true }
>
>  Notice that in a flat union, the discriminator name is controlled by
> @@ -366,10 +367,9 @@ union has a struct with a single member named 'data'.  
> That is,
>  is identical on the wire to:
>
>   { 'enum': 'Enum', 'data': ['one', 'two'] }
> - { 'struct': 'Base', 'data': { 'type': 'Enum' } }
>   { 'struct': 'Branch1', 'data': { 'data': 'str' } }
>   { 'struct': 'Branch2', 'data': { 'data': 'int' } }
> - { 'union': 'Flat', 'base': 'Base', 'discriminator': 'type',
> + { 'union': 'Flat': 'base': { 'type': 'Enum' }, 'discriminator': 'type',
>     'data': { 'one': 'Branch1', 'two': 'Branch2' } }
>
>
[Tests look good...]