Commit ac4338f8 authored by Eric Blake's avatar Eric Blake Committed by Markus Armbruster
Browse files

qapi: Allow anonymous base for flat union 



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.
In particular, this patch's change to the BlockdevOptions example
in qapi-code-gen.txt will actually be done in the real QAPI schema.

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 already 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: default avatarEric Blake <eblake@redhat.com>
Message-Id: <1458254921-17042-15-git-send-email-eblake@redhat.com>
Signed-off-by: default avatarMarkus Armbruster <armbru@redhat.com>
parent bd59adce

docs/qapi-code-gen.txt

+13 −13
Original line number Diff line number Diff line
@@ -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,13 +320,16 @@ 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 a

non-optional 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

member of the base struct.



The following example enhances the above simple union example by

adding an optional common member 'read-only', renaming the
@@ -334,10 +337,8 @@ discriminator to something more applicable than the simple union's 
default of 'type', and reducing the number of {} required on the wire:



 { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }

 { 'struct': 'BlockdevOptionsBase',

   'data': { 'driver': 'BlockdevDriver', '*read-only': 'bool' } }

 { 'union': 'BlockdevOptions',

   'base': 'BlockdevOptionsBase',

   'base': { 'driver': 'BlockdevDriver', '*read-only': 'bool' },

   'discriminator': 'driver',

   'data': { 'file': 'BlockdevOptionsFile',

             'qcow2': 'BlockdevOptionsQcow2' } }
@@ -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' } }

scripts/qapi-types.py

+6 −4
Original line number Diff line number Diff line
@@ -72,11 +72,13 @@ struct %(c_name)s { 
                 c_name=c_name(name))



    if base:

        if not base.is_implicit():

            ret += mcgen('''

    /* Members inherited from %(c_name)s: */

''',

                         c_name=base.c_name())

        ret += gen_struct_members(base.members)

        if not base.is_implicit():

            ret += mcgen('''

    /* Own members: */

''')
@@ -224,7 +226,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): 
            return

        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)

        # TODO Worth changing the visitor signature, so we could

        # directly use rather than repeat type.is_implicit()?

scripts/qapi.py

+10 −2
Original line number Diff line number Diff line
@@ -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
@@ -561,9 +563,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'])

        if not base:

            raise QAPIExprError(expr_info,

                                "Flat union '%s' must have a base"
@@ -1039,6 +1042,8 @@ class QAPISchemaMember(object): 
            owner = owner[6:]

            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
@@ -1325,6 +1330,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()]

tests/qapi-schema/flat-union-bad-base.err

+1 −1
Original line number Diff line number Diff line 
tests/qapi-schema/flat-union-bad-base.json:9: 'base' for union 'TestUnion' should be a type name
 
tests/qapi-schema/flat-union-bad-base.json:8: 'string' (member of TestTypeA) collides with 'string' (base of TestUnion)

tests/qapi-schema/flat-union-bad-base.json

+2 −3
Original line number Diff line number Diff line 
# we require the base to be an existing struct

# TODO: should we allow an anonymous inline base type?

# we allow anonymous base, but enforce no duplicate keys

{ 'enum': 'TestEnum',

  'data': [ 'value1', 'value2' ] }

{ 'struct': 'TestTypeA',
@@ -7,7 +6,7 @@ 
{ 'struct': 'TestTypeB',

  'data': { 'integer': 'int' } }

{ 'union': 'TestUnion',

  'base': { 'enum1': 'TestEnum', 'kind': 'str' },

  'base': { 'enum1': 'TestEnum', 'string': 'str' },

  'discriminator': 'enum1',

  'data': { 'value1': 'TestTypeA',

            'value2': 'TestTypeB' } }