| Message ID | 1427227433-5030-19-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 '**'. > > Meanwhile, for both 'gen' and 'success-response' keys, we have been > ignoring the value, although the schema consistently used "'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. > > There is no difference to the generated code. As these features > were previously undocumented before this series, add some tests > and documentation 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. You don't actually add documentation in this patch. Aside: 'gen': false is required when '**' is used anywhere in the command. If it was permitted only then, it would be redundant. I think we happily accept 'gen': false without '**' so far, although we don't use it. That's okay. > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 03/26/2015 11:38 AM, Markus Armbruster wrote: > 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 '**'. >> >> Meanwhile, for both 'gen' and 'success-response' keys, we have been >> ignoring the value, although the schema consistently used "'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. >> >> There is no difference to the generated code. As these features >> were previously undocumented before this series, add some tests >> and documentation 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. > > You don't actually add documentation in this patch. Hmm, more evidence that I waffled about per-commit doc fixes, vs. lumping it all in patch 1, and I obviously failed to scrub the commit messages after changing my mind. > > Aside: 'gen': false is required when '**' is used anywhere in the > command. If it was permitted only then, it would be redundant. I think > we happily accept 'gen': false without '**' so far, although we don't > use it. That's okay. Also, even though the code accepts 'gen':false, it rejects 'gen':true ('gen' is only a one-way switch away from the default). Also something I didn't think worth worrying about. > >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > >
Eric Blake <eblake@redhat.com> writes: > On 03/26/2015 11:38 AM, Markus Armbruster wrote: >> 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 '**'. >>> >>> Meanwhile, for both 'gen' and 'success-response' keys, we have been >>> ignoring the value, although the schema consistently used "'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. >>> >>> There is no difference to the generated code. As these features >>> were previously undocumented before this series, add some tests >>> and documentation 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. >> >> You don't actually add documentation in this patch. > > Hmm, more evidence that I waffled about per-commit doc fixes, vs. > lumping it all in patch 1, and I obviously failed to scrub the commit > messages after changing my mind. Fortunately, commit messages don't need testing, so this is easy enough to fix in a respin :) >> Aside: 'gen': false is required when '**' is used anywhere in the >> command. If it was permitted only then, it would be redundant. I think >> we happily accept 'gen': false without '**' so far, although we don't >> use it. That's okay. > > Also, even though the code accepts 'gen':false, it rejects 'gen':true > ('gen' is only a one-way switch away from the default). Also something > I didn't think worth worrying about. Agree. >>> 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 95f49e3..fe5be7e 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/tests/Makefile b/tests/Makefile index 7079e2a..941e839 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -215,6 +215,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 '**'. Meanwhile, for both 'gen' and 'success-response' keys, we have been ignoring the value, although the schema consistently used "'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. There is no difference to the generated code. As these features were previously undocumented before this series, add some tests and documentation 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> --- qapi-schema.json | 14 +++++++------- qga/qapi-schema.json | 8 ++++---- 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 +++ 15 files changed, 30 insertions(+), 11 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