Re: [Qemu-devel] [PATCH v9 09/17] qapi: Implement boxed types for commands/events

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Turn on the ability to pass command and event arguments in
> a single boxed parameter, which must name a non-empty type
> (although the type can be a struct with all optional members).
> For structs, it makes it possible to pass a single qapi type
> instead of a breakout of all struct members (useful if the
> arguments are already in a struct or if the number of members
> is large); for other complex types, it is now possible to use
> a union or alternate as the data for a command or event.
>
> The empty type may be technically feasible if needed down the
> road, but it's easier to forbid it now and relax things to allow
> it later, than it is to allow it now and have to special case
> how the generated 'q_empty' type is handled (see commit 7ce106a9
> for reasons why nothing is generated for the empty type).  An
> alternate type is never considered empty, but now that a boxed
> type can be either an object or an alternate, we have to provide
> a trivial QAPISchemaAlternateType.is_empty().  The new call to
> arg_type.is_empty() during QAPISchemaCommand.check() requires
> that we first check the type in question; but there is no chance
> of introducing a cycle since objects do not refer back to commands.
>
> We still have a split in syntax checking between ad-hoc parsing
> up front (merely validates that 'boxed' has a sane value) and
> during .check() methods (if 'boxed' is set, then 'data' must name
> a non-empty user-defined type).
>
> Generated code is unchanged, as long as no client uses the
> new feature.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: s/box/boxed/, formatting tweak, commit message improvements,
> restore assertions rather than relying on implicit checking
> v8: forbid empty type, allow alternates, improve docs
> v7: rebase to latest
> v6: retitle, rebase, and merge v5 40/46 and 41/46 into one patch
> ---
>  scripts/qapi.py                         | 63 
> ++++++++++++++++++++++++++-------
>  scripts/qapi-commands.py                |  3 +-
>  scripts/qapi-event.py                   |  5 ++-
>  tests/test-qmp-commands.c               |  8 +++++
>  docs/qapi-code-gen.txt                  | 27 ++++++++++++--
>  tests/Makefile.include                  |  5 +++
>  tests/qapi-schema/args-bad-box.err      |  1 +
>  tests/qapi-schema/args-bad-box.exit     |  1 +
>  tests/qapi-schema/args-bad-box.json     |  2 ++
>  tests/qapi-schema/args-bad-box.out      |  0
>  tests/qapi-schema/args-box-anon.err     |  1 +
>  tests/qapi-schema/args-box-anon.exit    |  1 +
>  tests/qapi-schema/args-box-anon.json    |  2 ++
>  tests/qapi-schema/args-box-anon.out     |  0
>  tests/qapi-schema/args-box-empty.err    |  1 +
>  tests/qapi-schema/args-box-empty.exit   |  1 +
>  tests/qapi-schema/args-box-empty.json   |  3 ++
>  tests/qapi-schema/args-box-empty.out    |  0
>  tests/qapi-schema/args-box-string.err   |  1 +
>  tests/qapi-schema/args-box-string.exit  |  1 +
>  tests/qapi-schema/args-box-string.json  |  2 ++
>  tests/qapi-schema/args-box-string.out   |  0
>  tests/qapi-schema/args-union.err        |  2 +-
>  tests/qapi-schema/args-union.json       |  3 +-
>  tests/qapi-schema/event-box-empty.err   |  1 +
>  tests/qapi-schema/event-box-empty.exit  |  1 +
>  tests/qapi-schema/event-box-empty.json  |  2 ++
>  tests/qapi-schema/event-box-empty.out   |  0
>  tests/qapi-schema/qapi-schema-test.json |  4 +++
>  tests/qapi-schema/qapi-schema-test.out  |  8 +++++
>  30 files changed, 129 insertions(+), 20 deletions(-)
>  create mode 100644 tests/qapi-schema/args-bad-box.err
>  create mode 100644 tests/qapi-schema/args-bad-box.exit
>  create mode 100644 tests/qapi-schema/args-bad-box.json
>  create mode 100644 tests/qapi-schema/args-bad-box.out
>  create mode 100644 tests/qapi-schema/args-box-anon.err
>  create mode 100644 tests/qapi-schema/args-box-anon.exit
>  create mode 100644 tests/qapi-schema/args-box-anon.json
>  create mode 100644 tests/qapi-schema/args-box-anon.out
>  create mode 100644 tests/qapi-schema/args-box-empty.err
>  create mode 100644 tests/qapi-schema/args-box-empty.exit
>  create mode 100644 tests/qapi-schema/args-box-empty.json
>  create mode 100644 tests/qapi-schema/args-box-empty.out
>  create mode 100644 tests/qapi-schema/args-box-string.err
>  create mode 100644 tests/qapi-schema/args-box-string.exit
>  create mode 100644 tests/qapi-schema/args-box-string.json
>  create mode 100644 tests/qapi-schema/args-box-string.out
>  create mode 100644 tests/qapi-schema/event-box-empty.err
>  create mode 100644 tests/qapi-schema/event-box-empty.exit
>  create mode 100644 tests/qapi-schema/event-box-empty.json
>  create mode 100644 tests/qapi-schema/event-box-empty.out

I'm inclined to change box to boxed in the test file names on commit.
What do you think?

[...]
> diff --git a/tests/qapi-schema/args-union.json 
> b/tests/qapi-schema/args-union.json
> index 7bdcbb7..c0ce091 100644
> --- a/tests/qapi-schema/args-union.json
> +++ b/tests/qapi-schema/args-union.json
> @@ -1,4 +1,3 @@
> -# we do not allow union arguments
> -# TODO should we support this?
> +# use of union arguments requires 'box':true

You missed a box here.  Can touch up on commit.

>  { 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
>  { 'command': 'oops', 'data': 'Uni' }
[...]