From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44723) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adKQ5-0002NI-AN for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:23:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adKPy-000788-V5 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:23:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46414) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adKPy-000784-Mx for qemu-devel@nongnu.org; Tue, 08 Mar 2016 11:23:30 -0500 From: Markus Armbruster References: <1457194595-16189-1-git-send-email-eblake@redhat.com> <1457194595-16189-9-git-send-email-eblake@redhat.com> Date: Tue, 08 Mar 2016 17:23:27 +0100 In-Reply-To: <1457194595-16189-9-git-send-email-eblake@redhat.com> (Eric Blake's message of "Sat, 5 Mar 2016 09:16:33 -0700") Message-ID: <87si015474.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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 > > --- > 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...]