| Message ID | 1457194595-16189-9-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > > --- > 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...]
On 03/08/2016 09:23 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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. >> > > I think you also > > * fixed a missing allow_optional=True in check_union() More on that below. > > * 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?) Could separate those two cleanups if it helps. >> @@ -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. allow_optional only matters if allow_dict is True. We have places where we allow a dict but do not allow optionals (namely, simple unions); but where we are creating an anonymous type, we want to allow optionals at the same time we allow a dict. I think this is just a case where the commit message needs to be beefed up. >> +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. They touch the same paragraph, but I can separate them if it would make this patch feel cleaner.
Eric Blake <eblake@redhat.com> writes: > On 03/08/2016 09:23 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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. >>> > >> >> I think you also >> >> * fixed a missing allow_optional=True in check_union() > > More on that below. > >> >> * 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?) > > Could separate those two cleanups if it helps. I'd like the fix recorded in git-log. For that, a mention in the commit message would suffice, but I think I'd rather separate them. >>> @@ -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. > > allow_optional only matters if allow_dict is True. We have places where > we allow a dict but do not allow optionals (namely, simple unions); but > where we are creating an anonymous type, we want to allow optionals at > the same time we allow a dict. I think this is just a case where the > commit message needs to be beefed up. Perhaps not even that. I'm spoiled by your meticulous patch versioning, and when once in a blue moon I spot something you didn't announce there, I take note. >>> +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. > > They touch the same paragraph, but I can separate them if it would make > this patch feel cleaner.
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']) 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 +member of the base struct. 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' } } diff --git a/tests/qapi-schema/flat-union-bad-base.err b/tests/qapi-schema/flat-union-bad-base.err index 79b8a71..bee24a2 100644 --- a/tests/qapi-schema/flat-union-bad-base.err +++ b/tests/qapi-schema/flat-union-bad-base.err @@ -1 +1 @@ -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) diff --git a/tests/qapi-schema/flat-union-bad-base.json b/tests/qapi-schema/flat-union-bad-base.json index e2e622b..74dd421 100644 --- a/tests/qapi-schema/flat-union-bad-base.json +++ b/tests/qapi-schema/flat-union-bad-base.json @@ -1,5 +1,4 @@ -# 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' } } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index e722748..f571e1b 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -75,14 +75,10 @@ 'base': 'UserDefZero', 'data': { 'string': 'str', 'enum1': 'EnumOne' } } -{ 'struct': 'UserDefUnionBase2', - 'base': 'UserDefZero', - 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } } - # this variant of UserDefFlatUnion defaults to a union that uses members with # allocated types to test corner cases in the cleanup/dealloc visitor { 'union': 'UserDefFlatUnion2', - 'base': 'UserDefUnionBase2', + 'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' }, 'discriminator': 'enum1', 'data': { 'value1' : 'UserDefC', # intentional forward reference 'value2' : 'UserDefB' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index f531961..a67d375 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -8,6 +8,10 @@ object :obj-EVENT_D-arg member b: str optional=False member c: str optional=True member enum3: EnumOne optional=True +object :obj-UserDefFlatUnion2-base + member integer: int optional=True + member string: str optional=False + member enum1: QEnumTwo optional=False object :obj-__org.qemu_x-command-arg member a: __org.qemu_x-EnumList optional=False member b: __org.qemu_x-StructList optional=False @@ -121,7 +125,7 @@ object UserDefFlatUnion case value2: UserDefB case value3: UserDefB object UserDefFlatUnion2 - base UserDefUnionBase2 + base :obj-UserDefFlatUnion2-base tag enum1 case value1: UserDefC case value2: UserDefB @@ -166,10 +170,6 @@ object UserDefUnionBase base UserDefZero member string: str optional=False member enum1: EnumOne optional=False -object UserDefUnionBase2 - base UserDefZero - member string: str optional=False - member enum1: QEnumTwo optional=False object UserDefZero member integer: int optional=False object WrapAlternate
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 <eblake@redhat.com> --- v4: rebase to use implicit type, improve commit message [no v3] 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(-)