| Message ID | 1435782155-31412-45-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/01/2015 02:22 PM, Markus Armbruster wrote: > 'gen': false needs to stay for now, because netdev_add is still using > it. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > docs/qapi-code-gen.txt | 15 ++++----------- > scripts/qapi.py | 20 ++++---------------- > tests/Makefile | 4 ++-- > tests/qapi-schema/args-returns-any.err | 1 + > ...type-bypass-no-gen.exit => args-returns-any.exit} | 0 > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 2367c66..ca578dd 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -457,13 +454,9 @@ which would validate this Client JSON Protocol transaction: > > In rare cases, QAPI cannot express a type-safe representation of a > corresponding Client JSON Protocol command. In these cases, if the > -command expression includes the key 'gen' with boolean value false, > -then the 'data' or 'returns' member that intends to bypass generated > -type-safety and do its own manual validation should use an inline > -dictionary definition, with a value of '**' rather than a valid type > -name for the keys that the generated code will not validate. Please > -try to avoid adding new commands that rely on this, and instead use > -type-safe unions. For an example of bypass usage: > +command expression includes the key 'gen' with boolean value false. Incomplete sentence. "if the command expression includes... false" needs "then something...". > +Please try to avoid adding new commands that rely on this, and instead > +use type-safe unions. For an example of bypass usage: And given my ideas, we may be able to make netdev_add typesafe and eliminate 'gen' altogether in the future :) > +++ b/tests/Makefile > @@ -222,11 +222,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ > bad-type-dict.json double-data.json unknown-expr-key.json \ > redefined-type.json redefined-command.json redefined-builtin.json \ > redefined-event.json command-int.json bad-data.json event-max.json \ > - type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \ > + type-bypass-bad-gen.json \ If I understand correctly (git didn't quite guess right), type-bypass-no-gen.json disappears completely (we no longer have working '**'), and type-bypass.json... > data-array-empty.json data-array-unknown.json data-int.json \ > data-unknown.json data-member-unknown.json data-member-array.json \ > data-member-array-bad.json args-union.json args-alternate.json \ > - returns-array-bad.json returns-int.json \ > + args-returns-any.json returns-array-bad.json returns-int.json \ ...gets renamed args-returns-any.json (because type bypass is now expressed via 'any'. > +# built-in type 'any' in arguments and returns > +# works except for 'data': 'any', because that has to be a struct > +# note: command name 'qom-get' chosen to avoid "cannot use built-in" error > +{ 'command': 'qom-get', 'data': { 'arg': 'any' }, 'returns': 'any' } > +{ 'command': 'foo', 'returns': { 'arg': 'any' } } > +{ 'command': 'bar', 'data': 'any' } This shows what the parser accepts and rejects, but doesn't prove it compiles to C; and the previous patch that touched test-qmp-input-visitor.c was testing that the low-level interactions worked but did not map it all the way back to qapi. I wonder if you should also enhance qapi-schema-test to use 'any', since that tends to be the one test that most fully exercises interactions with generated code based on a valid qapi schema. On the right track, but I want to make sure the botched documentation is correct before giving R-b.
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> 'gen': false needs to stay for now, because netdev_add is still using >> it. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> docs/qapi-code-gen.txt | 15 ++++----------- >> scripts/qapi.py | 20 ++++---------------- >> tests/Makefile | 4 ++-- >> tests/qapi-schema/args-returns-any.err | 1 + >> ...type-bypass-no-gen.exit => args-returns-any.exit} | 0 > >> >> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt >> index 2367c66..ca578dd 100644 >> --- a/docs/qapi-code-gen.txt >> +++ b/docs/qapi-code-gen.txt > >> @@ -457,13 +454,9 @@ which would validate this Client JSON Protocol transaction: >> >> In rare cases, QAPI cannot express a type-safe representation of a >> corresponding Client JSON Protocol command. In these cases, if the >> -command expression includes the key 'gen' with boolean value false, >> -then the 'data' or 'returns' member that intends to bypass generated >> -type-safety and do its own manual validation should use an inline >> -dictionary definition, with a value of '**' rather than a valid type >> -name for the keys that the generated code will not validate. Please >> -try to avoid adding new commands that rely on this, and instead use >> -type-safe unions. For an example of bypass usage: >> +command expression includes the key 'gen' with boolean value false. > > Incomplete sentence. "if the command expression includes... false" > needs "then something...". Oops. I'll rephrase. >> +Please try to avoid adding new commands that rely on this, and instead >> +use type-safe unions. For an example of bypass usage: > > And given my ideas, we may be able to make netdev_add typesafe and > eliminate 'gen' altogether in the future :) The harder problem is qapifying device_add without use of 'gen': false. It's structurally just like netdev_add, "only" with has many, many more variants (the device models), which are collected only at run time. Changing things to define device model properties in the schema seems unappealing. We could try to collect them at build time. Not trivial. We could collect them at run time, but then the schema becomes dynamic. Not trivial, either. The easiest solution is probably something like Python's **kwds parameter: collect any remaining arguments in a dictionary, and pass that as argument kwds. Sadly, QAPI introspection remains useless for device models then. >> +++ b/tests/Makefile >> @@ -222,11 +222,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ >> bad-type-dict.json double-data.json unknown-expr-key.json \ >> redefined-type.json redefined-command.json redefined-builtin.json \ >> redefined-event.json command-int.json bad-data.json event-max.json \ >> - type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \ >> + type-bypass-bad-gen.json \ > > If I understand correctly (git didn't quite guess right), > type-bypass-no-gen.json disappears completely (we no longer have working > '**'), and type-bypass.json... > >> data-array-empty.json data-array-unknown.json data-int.json \ >> data-unknown.json data-member-unknown.json data-member-array.json \ >> data-member-array-bad.json args-union.json args-alternate.json \ >> - returns-array-bad.json returns-int.json \ >> + args-returns-any.json returns-array-bad.json returns-int.json \ > > ...gets renamed args-returns-any.json (because type bypass is now > expressed via 'any'. I'd view args-returns-any.json as new. But viewing it as a replacement for type-bypass.json is okay. >> +# built-in type 'any' in arguments and returns >> +# works except for 'data': 'any', because that has to be a struct >> +# note: command name 'qom-get' chosen to avoid "cannot use built-in" error >> +{ 'command': 'qom-get', 'data': { 'arg': 'any' }, 'returns': 'any' } >> +{ 'command': 'foo', 'returns': { 'arg': 'any' } } >> +{ 'command': 'bar', 'data': 'any' } > > This shows what the parser accepts and rejects, but doesn't prove it > compiles to C; and the previous patch that touched > test-qmp-input-visitor.c was testing that the low-level interactions > worked but did not map it all the way back to qapi. I wonder if you > should also enhance qapi-schema-test to use 'any', since that tends to > be the one test that most fully exercises interactions with generated > code based on a valid qapi schema. I guess I'll have to split args-returns-any.json: move the positive test cases to qapi-schema-test.json, move the negative test case to its own test. > On the right track, but I want to make sure the botched documentation is > correct before giving R-b. Thanks!
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 2367c66..ca578dd 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -111,10 +111,7 @@ and field names within a type, should be all lower case with words separated by a hyphen. However, some existing older commands and complex types use underscore; when extending such expressions, consistency is preferred over blindly avoiding underscore. Event -names should be ALL_CAPS with words separated by underscore. The -special string '**' appears for some commands that manually perform -their own type checking rather than relying on the type-safe code -produced by the qapi code generators. +names should be ALL_CAPS with words separated by underscore. Any name (command, event, type, field, or enum value) beginning with "x-" is marked experimental, and may be withdrawn or changed @@ -457,13 +454,9 @@ which would validate this Client JSON Protocol transaction: In rare cases, QAPI cannot express a type-safe representation of a corresponding Client JSON Protocol command. In these cases, if the -command expression includes the key 'gen' with boolean value false, -then the 'data' or 'returns' member that intends to bypass generated -type-safety and do its own manual validation should use an inline -dictionary definition, with a value of '**' rather than a valid type -name for the keys that the generated code will not validate. Please -try to avoid adding new commands that rely on this, and instead use -type-safe unions. For an example of bypass usage: +command expression includes the key 'gen' with boolean value false. +Please try to avoid adding new commands that rely on this, and instead +use type-safe unions. For an example of bypass usage: { 'command': 'netdev_add', 'data': {'type': 'str', 'id': 'str'}, diff --git a/scripts/qapi.py b/scripts/qapi.py index df63130..6ddb33e 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -423,16 +423,13 @@ def is_enum(name): def check_type(expr_info, source, value, allow_array = False, allow_dict = False, allow_optional = False, - allow_star = False, allow_metas = []): + allow_metas = []): global all_names orig_value = value if value is None: return - if allow_star and value == '**': - return - # Check if array type for value is okay if isinstance(value, list): if not allow_array: @@ -447,10 +444,6 @@ def check_type(expr_info, source, value, allow_array = False, # Check if type name for value is okay if isinstance(value, str): - if value == '**': - raise QAPIExprError(expr_info, - "%s uses '**' but did not request 'gen':false" - % source) if not value in all_names: raise QAPIExprError(expr_info, "%s uses unknown type '%s'" @@ -474,7 +467,7 @@ def check_type(expr_info, source, value, allow_array = False, # Todo: allow dictionaries to represent default values of # an optional argument. check_type(expr_info, "Member '%s' of %s" % (key, source), arg, - allow_array=True, allow_star=allow_star, + allow_array=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) @@ -494,18 +487,16 @@ def check_member_clash(expr_info, base_name, data, source = ""): def check_command(expr, expr_info): name = expr['command'] - allow_star = expr.has_key('gen') check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), allow_dict=True, allow_optional=True, - allow_metas=['struct'], allow_star=allow_star) + allow_metas=['struct']) returns_meta = ['union', 'struct'] if name in returns_whitelist: returns_meta += ['built-in', 'alternate', 'enum'] check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, allow_dict=True, - allow_optional=True, allow_metas=returns_meta, - allow_star=allow_star) + allow_optional=True, allow_metas=returns_meta) def check_event(expr, expr_info): global events @@ -1051,7 +1042,6 @@ class QAPISchema(object): ('bool', 'boolean', 'bool', 'false'), ('any', 'value', 'QObject' + pointer_suffix , 'NULL')]: self._def_builtin_type(*t) - self.entity_dict['**'] = self.lookup_type('any') # TODO drop this alias def _make_implicit_enum_type(self, name, values): name = name + 'Kind' @@ -1192,8 +1182,6 @@ class QAPISchema(object): def visit(self, visitor): visitor.visit_begin() for name in sorted(self.entity_dict.keys()): - if self.entity_dict[name].name != name: - continue # ignore alias TODO drop alias and remove self.entity_dict[name].visit(visitor) visitor.visit_end() diff --git a/tests/Makefile b/tests/Makefile index 5a38748..60b82e2 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -222,11 +222,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ bad-type-dict.json double-data.json unknown-expr-key.json \ redefined-type.json redefined-command.json redefined-builtin.json \ redefined-event.json command-int.json bad-data.json event-max.json \ - type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \ + type-bypass-bad-gen.json \ data-array-empty.json data-array-unknown.json data-int.json \ data-unknown.json data-member-unknown.json data-member-array.json \ data-member-array-bad.json args-union.json args-alternate.json \ - returns-array-bad.json returns-int.json \ + args-returns-any.json returns-array-bad.json returns-int.json \ returns-unknown.json returns-alternate.json returns-whitelist.json \ missing-colon.json missing-comma-list.json missing-comma-object.json \ nested-struct-data.json nested-struct-returns.json non-objects.json \ diff --git a/tests/qapi-schema/args-returns-any.err b/tests/qapi-schema/args-returns-any.err new file mode 100644 index 0000000..f7ce727 --- /dev/null +++ b/tests/qapi-schema/args-returns-any.err @@ -0,0 +1 @@ +tests/qapi-schema/args-returns-any.json:6: 'data' for command 'bar' cannot use built-in type 'any' diff --git a/tests/qapi-schema/type-bypass-no-gen.exit b/tests/qapi-schema/args-returns-any.exit similarity index 100% rename from tests/qapi-schema/type-bypass-no-gen.exit rename to tests/qapi-schema/args-returns-any.exit diff --git a/tests/qapi-schema/args-returns-any.json b/tests/qapi-schema/args-returns-any.json new file mode 100644 index 0000000..a74fc6f --- /dev/null +++ b/tests/qapi-schema/args-returns-any.json @@ -0,0 +1,6 @@ +# built-in type 'any' in arguments and returns +# works except for 'data': 'any', because that has to be a struct +# note: command name 'qom-get' chosen to avoid "cannot use built-in" error +{ 'command': 'qom-get', 'data': { 'arg': 'any' }, 'returns': 'any' } +{ 'command': 'foo', 'returns': { 'arg': 'any' } } +{ 'command': 'bar', 'data': 'any' } diff --git a/tests/qapi-schema/type-bypass-no-gen.out b/tests/qapi-schema/args-returns-any.out similarity index 100% rename from tests/qapi-schema/type-bypass-no-gen.out rename to tests/qapi-schema/args-returns-any.out diff --git a/tests/qapi-schema/flat-union-base-star.err b/tests/qapi-schema/flat-union-base-star.err index b7748f0..43e5aa4 100644 --- a/tests/qapi-schema/flat-union-base-star.err +++ b/tests/qapi-schema/flat-union-base-star.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-base-star.json:8: Base '**' is not a valid struct +tests/qapi-schema/flat-union-base-star.json:8: Base 'any' is not a valid struct diff --git a/tests/qapi-schema/flat-union-base-star.json b/tests/qapi-schema/flat-union-base-star.json index 5099439..fe66b71 100644 --- a/tests/qapi-schema/flat-union-base-star.json +++ b/tests/qapi-schema/flat-union-base-star.json @@ -6,7 +6,7 @@ { 'struct': 'TestTypeB', 'data': { 'integer': 'int' } } { 'union': 'TestUnion', - 'base': '**', + 'base': 'any', 'discriminator': 'enum1', 'data': { 'value1': 'TestTypeA', 'value2': 'TestTypeB' } } diff --git a/tests/qapi-schema/type-bypass-no-gen.err b/tests/qapi-schema/type-bypass-no-gen.err deleted file mode 100644 index 20cef0a..0000000 --- a/tests/qapi-schema/type-bypass-no-gen.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/type-bypass-no-gen.json:2: Member 'arg' of 'data' for command 'unsafe' uses '**' but did not request 'gen':false diff --git a/tests/qapi-schema/type-bypass-no-gen.json b/tests/qapi-schema/type-bypass-no-gen.json deleted file mode 100644 index 4feae37..0000000 --- a/tests/qapi-schema/type-bypass-no-gen.json +++ /dev/null @@ -1,2 +0,0 @@ -# type bypass only works with 'gen':false -{ 'command': 'unsafe', 'data': { 'arg': '**' }, 'returns': '**' } diff --git a/tests/qapi-schema/type-bypass.err b/tests/qapi-schema/type-bypass.err deleted file mode 100644 index e69de29..0000000 diff --git a/tests/qapi-schema/type-bypass.exit b/tests/qapi-schema/type-bypass.exit deleted file mode 100644 index 573541a..0000000 --- a/tests/qapi-schema/type-bypass.exit +++ /dev/null @@ -1 +0,0 @@ -0 diff --git a/tests/qapi-schema/type-bypass.json b/tests/qapi-schema/type-bypass.json deleted file mode 100644 index 48b2137..0000000 --- a/tests/qapi-schema/type-bypass.json +++ /dev/null @@ -1,2 +0,0 @@ -# Use of 'gen':false allows bypassing type system -{ 'command': 'unsafe', 'data': { 'arg': '**' }, 'returns': '**', 'gen': false } diff --git a/tests/qapi-schema/type-bypass.out b/tests/qapi-schema/type-bypass.out deleted file mode 100644 index 2dc0c09..0000000 --- a/tests/qapi-schema/type-bypass.out +++ /dev/null @@ -1,4 +0,0 @@ -object :obj-unsafe-args - member arg: any optional=False -command unsafe :obj-unsafe-args -> any - gen=False success_response=True
'gen': false needs to stay for now, because netdev_add is still using it. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/qapi-code-gen.txt | 15 ++++----------- scripts/qapi.py | 20 ++++---------------- tests/Makefile | 4 ++-- tests/qapi-schema/args-returns-any.err | 1 + ...type-bypass-no-gen.exit => args-returns-any.exit} | 0 tests/qapi-schema/args-returns-any.json | 6 ++++++ .../{type-bypass-no-gen.out => args-returns-any.out} | 0 tests/qapi-schema/flat-union-base-star.err | 2 +- tests/qapi-schema/flat-union-base-star.json | 2 +- tests/qapi-schema/type-bypass-no-gen.err | 1 - tests/qapi-schema/type-bypass-no-gen.json | 2 -- tests/qapi-schema/type-bypass.err | 0 tests/qapi-schema/type-bypass.exit | 1 - tests/qapi-schema/type-bypass.json | 2 -- tests/qapi-schema/type-bypass.out | 4 ---- 15 files changed, 19 insertions(+), 41 deletions(-) create mode 100644 tests/qapi-schema/args-returns-any.err rename tests/qapi-schema/{type-bypass-no-gen.exit => args-returns-any.exit} (100%) create mode 100644 tests/qapi-schema/args-returns-any.json rename tests/qapi-schema/{type-bypass-no-gen.out => args-returns-any.out} (100%) delete mode 100644 tests/qapi-schema/type-bypass-no-gen.err delete mode 100644 tests/qapi-schema/type-bypass-no-gen.json delete mode 100644 tests/qapi-schema/type-bypass.err delete mode 100644 tests/qapi-schema/type-bypass.exit delete mode 100644 tests/qapi-schema/type-bypass.json delete mode 100644 tests/qapi-schema/type-bypass.out