| Message ID | 1430751937-17523-23-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > For a few QMP commands, we are forced to pass an arbitrary type > without tracking it properly in QAPI. Among the existing clients, > this unnamed type was spelled 'dict', 'visitor', and '**'; this > patch standardizes on '**', matching the documentation changes > earlier in the series. > > Meanwhile, for the 'gen' key, we have been ignoring the value, > although the schema consistently used "'no'" ('success-response' > was hard-coded to checking for 'no'). But now that we can support > a literal "false" in the schema, we might as well use that rather > than ignoring the value or special-casing a random string. Note > that these are one-way switches (use of 'gen':true is not the same > as omitting 'gen'). Also, the use of '**' requires 'gen':false, > but the use of 'gen':false does not mandate the use of '**'. > > There is no difference to the generated code. Add some tests on > what we'd like to guarantee, although it will take later patches > to clean up test results and actually enforce the use of a bool > parameter. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/qapi-schema.json b/qapi-schema.json index ac9594d..7f4cf86 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1561,8 +1561,8 @@ ## { 'command': 'qom-get', 'data': { 'path': 'str', 'property': 'str' }, - 'returns': 'visitor', - 'gen': 'no' } + 'returns': '**', + 'gen': false } ## # @qom-set: @@ -1579,8 +1579,8 @@ # Since: 1.2 ## { 'command': 'qom-set', - 'data': { 'path': 'str', 'property': 'str', 'value': 'visitor' }, - 'gen': 'no' } + 'data': { 'path': 'str', 'property': 'str', 'value': '**' }, + 'gen': false } ## # @set_password: @@ -1943,7 +1943,7 @@ ## { 'command': 'netdev_add', 'data': {'type': 'str', 'id': 'str', '*props': '**'}, - 'gen': 'no' } + 'gen': false } ## # @netdev_del: @@ -1976,8 +1976,8 @@ # Since: 2.0 ## { 'command': 'object-add', - 'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'}, - 'gen': 'no' } + 'data': {'qom-type': 'str', 'id': 'str', '*props': '**'}, + 'gen': false } ## # @object-del: diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 5c4cd40..fecc442 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -195,7 +195,7 @@ # Since: 0.15.0 ## { 'command': 'guest-shutdown', 'data': { '*mode': 'str' }, - 'success-response': 'no' } + 'success-response': false } ## # @guest-file-open: @@ -470,7 +470,7 @@ # # Since: 1.1 ## -{ 'command': 'guest-suspend-disk', 'success-response': 'no' } +{ 'command': 'guest-suspend-disk', 'success-response': false } ## # @guest-suspend-ram @@ -502,7 +502,7 @@ # # Since: 1.1 ## -{ 'command': 'guest-suspend-ram', 'success-response': 'no' } +{ 'command': 'guest-suspend-ram', 'success-response': false } ## # @guest-suspend-hybrid @@ -529,7 +529,7 @@ # # Since: 1.1 ## -{ 'command': 'guest-suspend-hybrid', 'success-response': 'no' } +{ 'command': 'guest-suspend-hybrid', 'success-response': false } ## # @GuestIpAddressType: diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 053ba85..cb78682 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -2,7 +2,7 @@ # QAPI command marshaller generator # # Copyright IBM, Corp. 2011 -# Copyright (C) 2014 Red Hat, Inc. +# Copyright (C) 2014-2015 Red Hat, Inc. # # Authors: # Anthony Liguori <aliguori@us.ibm.com> @@ -293,17 +293,12 @@ out: return ret -def option_value_matches(opt, val, cmd): - if opt in cmd and cmd[opt] == val: - return True - return False - def gen_registry(commands): registry="" push_indent() for cmd in commands: options = 'QCO_NO_OPTIONS' - if option_value_matches('success-response', 'no', cmd): + if not cmd.get('success-response', True): options = 'QCO_NO_SUCCESS_RESP' registry += mcgen(''' diff --git a/tests/Makefile b/tests/Makefile index c27940c..64e6785 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -216,6 +216,7 @@ 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 event-max.json \ + type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \ missing-colon.json missing-comma-list.json \ missing-comma-object.json non-objects.json \ qapi-schema-test.json quoted-structural-chars.json \ diff --git a/tests/qapi-schema/type-bypass-bad-gen.err b/tests/qapi-schema/type-bypass-bad-gen.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/type-bypass-bad-gen.exit b/tests/qapi-schema/type-bypass-bad-gen.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/type-bypass-bad-gen.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/type-bypass-bad-gen.json b/tests/qapi-schema/type-bypass-bad-gen.json new file mode 100644 index 0000000..bb70bee --- /dev/null +++ b/tests/qapi-schema/type-bypass-bad-gen.json @@ -0,0 +1,2 @@ +# FIXME: 'gen' should only appear with value false +{ 'command': 'foo', 'gen': 'whatever' } diff --git a/tests/qapi-schema/type-bypass-bad-gen.out b/tests/qapi-schema/type-bypass-bad-gen.out new file mode 100644 index 0000000..e678f2c --- /dev/null +++ b/tests/qapi-schema/type-bypass-bad-gen.out @@ -0,0 +1,3 @@ +[OrderedDict([('command', 'foo'), ('gen', 'whatever')])] +[] +[] diff --git a/tests/qapi-schema/type-bypass-no-gen.err b/tests/qapi-schema/type-bypass-no-gen.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/type-bypass-no-gen.exit b/tests/qapi-schema/type-bypass-no-gen.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/type-bypass-no-gen.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/type-bypass-no-gen.json b/tests/qapi-schema/type-bypass-no-gen.json new file mode 100644 index 0000000..af87c19 --- /dev/null +++ b/tests/qapi-schema/type-bypass-no-gen.json @@ -0,0 +1,2 @@ +# FIXME: type bypass should only work with 'gen':false +{ 'command': 'unsafe', 'data': { 'arg': '**' }, 'returns': '**' } diff --git a/tests/qapi-schema/type-bypass-no-gen.out b/tests/qapi-schema/type-bypass-no-gen.out new file mode 100644 index 0000000..8b2a9ac --- /dev/null +++ b/tests/qapi-schema/type-bypass-no-gen.out @@ -0,0 +1,3 @@ +[OrderedDict([('command', 'unsafe'), ('data', OrderedDict([('arg', '**')])), ('returns', '**')])] +[] +[] diff --git a/tests/qapi-schema/type-bypass.err b/tests/qapi-schema/type-bypass.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/type-bypass.exit b/tests/qapi-schema/type-bypass.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/type-bypass.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/type-bypass.json b/tests/qapi-schema/type-bypass.json new file mode 100644 index 0000000..48b2137 --- /dev/null +++ b/tests/qapi-schema/type-bypass.json @@ -0,0 +1,2 @@ +# 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 new file mode 100644 index 0000000..eaf20f8 --- /dev/null +++ b/tests/qapi-schema/type-bypass.out @@ -0,0 +1,3 @@ +[OrderedDict([('command', 'unsafe'), ('data', OrderedDict([('arg', '**')])), ('returns', '**'), ('gen', False)])] +[] +[]
For a few QMP commands, we are forced to pass an arbitrary type without tracking it properly in QAPI. Among the existing clients, this unnamed type was spelled 'dict', 'visitor', and '**'; this patch standardizes on '**', matching the documentation changes earlier in the series. Meanwhile, for the 'gen' key, we have been ignoring the value, although the schema consistently used "'no'" ('success-response' was hard-coded to checking for 'no'). But now that we can support a literal "false" in the schema, we might as well use that rather than ignoring the value or special-casing a random string. Note that these are one-way switches (use of 'gen':true is not the same as omitting 'gen'). Also, the use of '**' requires 'gen':false, but the use of 'gen':false does not mandate the use of '**'. There is no difference to the generated code. Add some tests on what we'd like to guarantee, although it will take later patches to clean up test results and actually enforce the use of a bool parameter. Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: Drop R-b, due to additional change to scripts to avoid regression in success-response handling --- qapi-schema.json | 14 +++++++------- qga/qapi-schema.json | 8 ++++---- scripts/qapi-commands.py | 9 ++------- tests/Makefile | 1 + tests/qapi-schema/type-bypass-bad-gen.err | 0 tests/qapi-schema/type-bypass-bad-gen.exit | 1 + tests/qapi-schema/type-bypass-bad-gen.json | 2 ++ tests/qapi-schema/type-bypass-bad-gen.out | 3 +++ tests/qapi-schema/type-bypass-no-gen.err | 0 tests/qapi-schema/type-bypass-no-gen.exit | 1 + tests/qapi-schema/type-bypass-no-gen.json | 2 ++ tests/qapi-schema/type-bypass-no-gen.out | 3 +++ 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 | 3 +++ 16 files changed, 32 insertions(+), 18 deletions(-) create mode 100644 tests/qapi-schema/type-bypass-bad-gen.err create mode 100644 tests/qapi-schema/type-bypass-bad-gen.exit create mode 100644 tests/qapi-schema/type-bypass-bad-gen.json create mode 100644 tests/qapi-schema/type-bypass-bad-gen.out create mode 100644 tests/qapi-schema/type-bypass-no-gen.err create mode 100644 tests/qapi-schema/type-bypass-no-gen.exit create mode 100644 tests/qapi-schema/type-bypass-no-gen.json create mode 100644 tests/qapi-schema/type-bypass-no-gen.out create mode 100644 tests/qapi-schema/type-bypass.err create mode 100644 tests/qapi-schema/type-bypass.exit create mode 100644 tests/qapi-schema/type-bypass.json create mode 100644 tests/qapi-schema/type-bypass.out