Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
Eric Blake ebl...@redhat.com writes:
On 07/21/2015 06:43 AM, Eric Blake wrote:
On 07/01/2015 02:22 PM, Markus Armbruster wrote:
A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.
Existing test case data-int.json covers simple type 'int'. Add test
cases for type names referring to union and alternate types.
We could probably relax things to allow a union (which is always a
dictionary on the wire), but I agree that allowing an alternate type is
not appropriate (the goal here is that we require a dictionary). But
it's also easier to be conservative now and relax later.
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,4 @@
+# we do not allow alternate arguments
+# TODO should we support this?
I see no reason to allow a non-dictionary in QMP, so this TODO could be
dropped.
Or, to be clear, we document that arguments is always a dictionary, for:
{ execute:command, arguments:{} }. Allowing an alternate would
break that, so it is a different level of change to allow an alternate
(change the QMP protocol) than what it would be to allow a union (the
QMP protocol is unchanged). Not that we can't do it if we ever have a
reason, it's just that I don't see it being worth a TODO statement.
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
This, on the other hand, seems valid from the wire format (it will
always be a dictionary). I guess the problem is that we generate a C
function signature based by calling out each member of the dictionary -
but how do you do that for a union? So I see what you are doing:
marking that this test currently passes the parser, but then causes
problems for generating C code, so we should either reject it up front,
or fix the generator. The FIXME documents what you will do later in the
series (reject it up front) and the TODO documents what we can do down
the road (fix the generator to allow it).
See also 32/47 - events have the same problem.
I'll amend the next patch to fix them, too.
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
On 07/27/2015 01:50 AM, Markus Armbruster wrote: This, on the other hand, seems valid from the wire format (it will always be a dictionary). I guess the problem is that we generate a C function signature based by calling out each member of the dictionary - but how do you do that for a union? Exactly: the problem is neither conceptual nor the wire API, it's the C API we generate. So I see what you are doing: marking that this test currently passes the parser, but then causes problems for generating C code, so we should either reject it up front, or fix the generator. The FIXME documents what you will do later in the series (reject it up front) Yes, in PATCH 15. and the TODO documents what we can do down the road (fix the generator to allow it). I figure we'd change the C API not to explode the data type into multiple parameters. We can consider that when we have a use for it. See also 32/47 - events have the same problem. I'm afraid I don't see the connection to PATCH 32. Patch 32 was where I figured out that we have the same problem where the C code we generate for an event will break if an event tries to use a union type as its data dictionary; and therefore, this patch would be an ideal time to add a test for that, and patch 15 would be an ideal time to tweak events to not allow unions (as the simple fix, where someday down the road we may relax things to allow unions in both commands and events). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
Eric Blake ebl...@redhat.com writes:
On 07/21/2015 06:43 AM, Eric Blake wrote:
On 07/01/2015 02:22 PM, Markus Armbruster wrote:
A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.
Existing test case data-int.json covers simple type 'int'. Add test
cases for type names referring to union and alternate types.
We could probably relax things to allow a union (which is always a
dictionary on the wire), but I agree that allowing an alternate type is
not appropriate (the goal here is that we require a dictionary). But
it's also easier to be conservative now and relax later.
Yes.
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,4 @@
+# we do not allow alternate arguments
+# TODO should we support this?
I see no reason to allow a non-dictionary in QMP, so this TODO could be
dropped.
Or, to be clear, we document that arguments is always a dictionary, for:
{ execute:command, arguments:{} }. Allowing an alternate would
break that, so it is a different level of change to allow an alternate
(change the QMP protocol) than what it would be to allow a union (the
QMP protocol is unchanged). Not that we can't do it if we ever have a
reason, it's just that I don't see it being worth a TODO statement.
I'll drop it.
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
This, on the other hand, seems valid from the wire format (it will
always be a dictionary). I guess the problem is that we generate a C
function signature based by calling out each member of the dictionary -
but how do you do that for a union?
Exactly: the problem is neither conceptual nor the wire API, it's the C
API we generate.
So I see what you are doing:
marking that this test currently passes the parser, but then causes
problems for generating C code, so we should either reject it up front,
or fix the generator. The FIXME documents what you will do later in the
series (reject it up front)
Yes, in PATCH 15.
and the TODO documents what we can do down
the road (fix the generator to allow it).
I figure we'd change the C API not to explode the data type into
multiple parameters. We can consider that when we have a use for it.
See also 32/47 - events have the same problem.
I'm afraid I don't see the connection to PATCH 32.
Reviewed-by: Eric Blake ebl...@redhat.com
Thanks!
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
On 07/21/2015 06:43 AM, Eric Blake wrote:
On 07/01/2015 02:22 PM, Markus Armbruster wrote:
A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.
Existing test case data-int.json covers simple type 'int'. Add test
cases for type names referring to union and alternate types.
We could probably relax things to allow a union (which is always a
dictionary on the wire), but I agree that allowing an alternate type is
not appropriate (the goal here is that we require a dictionary). But
it's also easier to be conservative now and relax later.
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,4 @@
+# we do not allow alternate arguments
+# TODO should we support this?
I see no reason to allow a non-dictionary in QMP, so this TODO could be
dropped.
Or, to be clear, we document that arguments is always a dictionary, for:
{ execute:command, arguments:{} }. Allowing an alternate would
break that, so it is a different level of change to allow an alternate
(change the QMP protocol) than what it would be to allow a union (the
QMP protocol is unchanged). Not that we can't do it if we ever have a
reason, it's just that I don't see it being worth a TODO statement.
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
This, on the other hand, seems valid from the wire format (it will
always be a dictionary). I guess the problem is that we generate a C
function signature based by calling out each member of the dictionary -
but how do you do that for a union? So I see what you are doing:
marking that this test currently passes the parser, but then causes
problems for generating C code, so we should either reject it up front,
or fix the generator. The FIXME documents what you will do later in the
series (reject it up front) and the TODO documents what we can do down
the road (fix the generator to allow it).
See also 32/47 - events have the same problem.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
On 07/01/2015 02:22 PM, Markus Armbruster wrote:
A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.
Existing test case data-int.json covers simple type 'int'. Add test
cases for type names referring to union and alternate types.
We could probably relax things to allow a union (which is always a
dictionary on the wire), but I agree that allowing an alternate type is
not appropriate (the goal here is that we require a dictionary). But
it's also easier to be conservative now and relax later.
The latter is caught (good), but the former is not (bug).
Signed-off-by: Markus Armbruster arm...@redhat.com
---
tests/Makefile| 3 ++-
tests/qapi-schema/args-alternate.err | 1 +
tests/qapi-schema/args-alternate.exit | 1 +
tests/qapi-schema/args-alternate.json | 4
tests/qapi-schema/args-alternate.out | 0
tests/qapi-schema/args-union.err | 0
tests/qapi-schema/args-union.exit | 1 +
tests/qapi-schema/args-union.json | 4
tests/qapi-schema/args-union.out | 4
9 files changed, 17 insertions(+), 1 deletion(-)
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,4 @@
+# we do not allow alternate arguments
+# TODO should we support this?
I see no reason to allow a non-dictionary in QMP, so this TODO could be
dropped.
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
This, on the other hand, seems valid from the wire format (it will
always be a dictionary). I guess the problem is that we generate a C
function signature based by calling out each member of the dictionary -
but how do you do that for a union? So I see what you are doing:
marking that this test currently passes the parser, but then causes
problems for generating C code, so we should either reject it up front,
or fix the generator. The FIXME documents what you will do later in the
series (reject it up front) and the TODO documents what we can do down
the road (fix the generator to allow it).
Reviewed-by: Eric Blake ebl...@redhat.com
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH RFC v2 14/47] qapi-tests: New tests for union, alternate command arguments
A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.
Existing test case data-int.json covers simple type 'int'. Add test
cases for type names referring to union and alternate types.
The latter is caught (good), but the former is not (bug).
Signed-off-by: Markus Armbruster arm...@redhat.com
---
tests/Makefile| 3 ++-
tests/qapi-schema/args-alternate.err | 1 +
tests/qapi-schema/args-alternate.exit | 1 +
tests/qapi-schema/args-alternate.json | 4
tests/qapi-schema/args-alternate.out | 0
tests/qapi-schema/args-union.err | 0
tests/qapi-schema/args-union.exit | 1 +
tests/qapi-schema/args-union.json | 4
tests/qapi-schema/args-union.out | 4
9 files changed, 17 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/args-alternate.err
create mode 100644 tests/qapi-schema/args-alternate.exit
create mode 100644 tests/qapi-schema/args-alternate.json
create mode 100644 tests/qapi-schema/args-alternate.out
create mode 100644 tests/qapi-schema/args-union.err
create mode 100644 tests/qapi-schema/args-union.exit
create mode 100644 tests/qapi-schema/args-union.json
create mode 100644 tests/qapi-schema/args-union.out
diff --git a/tests/Makefile b/tests/Makefile
index eff5e11..5a38748 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -225,7 +225,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
type-bypass.json type-bypass-no-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 returns-array-bad.json returns-int.json \
+ data-member-array-bad.json args-union.json args-alternate.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-alternate.err
b/tests/qapi-schema/args-alternate.err
new file mode 100644
index 000..fc90a9e
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-alternate.json:4: 'data' for command 'oops' cannot use
alternate type 'Alt'
diff --git a/tests/qapi-schema/args-alternate.exit
b/tests/qapi-schema/args-alternate.exit
new file mode 100644
index 000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-alternate.json
b/tests/qapi-schema/args-alternate.json
new file mode 100644
index 000..d2f9199
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,4 @@
+# we do not allow alternate arguments
+# TODO should we support this?
+{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Alt' }
diff --git a/tests/qapi-schema/args-alternate.out
b/tests/qapi-schema/args-alternate.out
new file mode 100644
index 000..e69de29
diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err
new file mode 100644
index 000..e69de29
diff --git a/tests/qapi-schema/args-union.exit
b/tests/qapi-schema/args-union.exit
new file mode 100644
index 000..573541a
--- /dev/null
+++ b/tests/qapi-schema/args-union.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/args-union.json
b/tests/qapi-schema/args-union.json
new file mode 100644
index 000..db97ef6
--- /dev/null
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
diff --git a/tests/qapi-schema/args-union.out b/tests/qapi-schema/args-union.out
new file mode 100644
index 000..907080c
--- /dev/null
+++ b/tests/qapi-schema/args-union.out
@@ -0,0 +1,4 @@
+[OrderedDict([('union', 'Uni'), ('data', OrderedDict([('case1', 'int'),
('case2', 'str')]))]),
+ OrderedDict([('command', 'oops'), ('data', 'Uni')])]
+[{'enum_name': 'UniKind', 'enum_values': None}]
+[]
--
1.9.3
