Eric Blake <ebl...@redhat.com> 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.
>
> Generated code is unchanged, as long as no client uses the
> new feature.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> 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, 128 insertions(+), 21 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
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f5e7697..48263c4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -522,10 +522,14 @@ def check_type(expr_info, source, value,
> allow_array=False,
>
> def check_command(expr, expr_info):
> name = expr['command']
> + box = expr.get('box', False)
>
> + args_meta = ['struct']
> + if box:
> + args_meta += ['union', 'alternate']
> check_type(expr_info, "'data' for command '%s'" % name,
> - expr.get('data'), allow_dict=True, allow_optional=True,
> - allow_metas=['struct'])
> + expr.get('data'), allow_dict=not box, allow_optional=True,
> + allow_metas=args_meta)
> returns_meta = ['union', 'struct']
> if name in returns_whitelist:
> returns_meta += ['built-in', 'alternate', 'enum']
Leaves enforcing "if 'box':true then 'data' is mandatory" to
QAPISchemaCommand.check(). Okay. Just a reminder that we we still have
this ugly split of the semantic checking to clean up.
> @@ -537,11 +541,15 @@ def check_command(expr, expr_info):
> def check_event(expr, expr_info):
> global events
> name = expr['event']
> + box = expr.get('box', False)
> + meta = ['struct']
>
> + if box:
> + meta += ['union', 'alternate']
Let's not separate the initialization of meta with a blank line here.
Separating it from box, like you did in check_command() is fine. Can
touch up on commit.
> events.append(name)
> check_type(expr_info, "'data' for event '%s'" % name,
> - expr.get('data'), allow_dict=True, allow_optional=True,
> - allow_metas=['struct'])
> + expr.get('data'), allow_dict=not box, allow_optional=True,
> + allow_metas=meta)
>
>
> def check_union(expr, expr_info):
> @@ -694,6 +702,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
> raise QAPIExprError(info,
> "'%s' of %s '%s' should only use false value"
> % (key, meta, name))
> + if key == 'box' and value is not True:
> + raise QAPIExprError(info,
> + "'%s' of %s '%s' should only use true value"
> + % (key, meta, name))
> for key in required:
> if key not in expr:
> raise QAPIExprError(info,
> @@ -725,10 +737,10 @@ def check_exprs(exprs):
> add_struct(expr, info)
> elif 'command' in expr:
> check_keys(expr_elem, 'command', [],
> - ['data', 'returns', 'gen', 'success-response'])
> + ['data', 'returns', 'gen', 'success-response', 'box'])
> add_name(expr['command'], info, 'command')
> elif 'event' in expr:
> - check_keys(expr_elem, 'event', [], ['data'])
> + check_keys(expr_elem, 'event', [], ['data', 'box'])
> add_name(expr['event'], info, 'event')
> else:
> raise QAPIExprError(expr_elem['info'],
> @@ -1162,6 +1174,9 @@ class QAPISchemaAlternateType(QAPISchemaType):
> def visit(self, visitor):
> visitor.visit_alternate_type(self.name, self.info, self.variants)
>
> + def is_empty(self):
> + return False
> +
You want to .is_empty() the boxed type, which can either be
QAPISchemaObjectType (already has this method) or
QAPISchemaAlternateType (doesn't, so you add it).
Duck typing at work :)
>
> class QAPISchemaCommand(QAPISchemaEntity):
> def __init__(self, name, info, arg_type, ret_type, gen, success_response,
> @@ -1180,9 +1195,18 @@ class QAPISchemaCommand(QAPISchemaEntity):
> def check(self, schema):
> if self._arg_type_name:
> self.arg_type = schema.lookup_type(self._arg_type_name)
> - assert isinstance(self.arg_type, QAPISchemaObjectType)
> - assert not self.arg_type.variants # not implemented
> - assert not self.box # not implemented
> + assert (isinstance(self.arg_type, QAPISchemaObjectType) or
> + isinstance(self.arg_type, QAPISchemaAlternateType))
> + self.arg_type.check(schema)
qapi.py recurses .check() only when necessary, because undisciplined
recursion can easily become cyclic.
I think you need self.arg_type.check() here so you can
self.arg_type.is_empty() below. Correct?
> + if self.box:
> + if self.arg_type.is_empty():
> + raise QAPIExprError(self.info,
> + "Cannot use 'box' with empty type")
> + else:
> + assert not self.arg_type.variants
Lost: assert isinstance(self.arg_type, QAPISchemaObjectType), or the
equivalent assert not isinstance(self.arg_type, QAPISchemaAlternateType).
> + elif self.box:
> + raise QAPIExprError(self.info,
> + "Use of 'box' requires 'data'")
> if self._ret_type_name:
> self.ret_type = schema.lookup_type(self._ret_type_name)
> assert isinstance(self.ret_type, QAPISchemaType)
> @@ -1204,9 +1228,18 @@ class QAPISchemaEvent(QAPISchemaEntity):
> def check(self, schema):
> if self._arg_type_name:
> self.arg_type = schema.lookup_type(self._arg_type_name)
> - assert isinstance(self.arg_type, QAPISchemaObjectType)
> - assert not self.arg_type.variants # not implemented
> - assert not self.box # not implemented
> + assert (isinstance(self.arg_type, QAPISchemaObjectType) or
> + isinstance(self.arg_type, QAPISchemaAlternateType))
> + self.arg_type.check(schema)
> + if self.box:
> + if self.arg_type.is_empty():
> + raise QAPIExprError(self.info,
> + "Cannot use 'box' with empty type")
> + else:
> + assert not self.arg_type.variants
Likewise.
> + elif self.box:
> + raise QAPIExprError(self.info,
> + "Use of 'box' requires 'data'")
>
> def visit(self, visitor):
> visitor.visit_event(self.name, self.info, self.arg_type, self.box)
> @@ -1646,12 +1679,14 @@ extern const char *const %(c_name)s_lookup[];
>
>
> def gen_params(arg_type, box, extra):
> - if not arg_type:
> + if not arg_type or arg_type.is_empty():
> + assert not box
> return extra
> ret = ''
> sep = ''
> if box:
> - assert False # not implemented
> + ret += '%s arg' % arg_type.c_param_type()
> + sep = ', '
> else:
> assert not arg_type.variants
> for memb in arg_type.members:
Having two representations of "no arguments" (null arg_type and empty
arg_type) is ugly. Not this patch's fault.
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 598c4c7..8d25701 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -30,7 +30,8 @@ def gen_call(name, arg_type, box, ret_type):
>
> argstr = ''
> if box:
> - assert False # not implemented
> + assert arg_type and not arg_type.is_empty()
> + argstr = '&arg, '
> elif arg_type:
> assert not arg_type.variants
> for memb in arg_type.members:
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 024be4d..2cab588 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -79,7 +79,10 @@ def gen_event_send(name, arg_type, box):
> QObject *obj;
> Visitor *v;
> ''')
> - ret += gen_param_var(arg_type)
> + if not box:
> + ret += gen_param_var(arg_type)
> + else:
> + assert not box
>
> ret += mcgen('''
>
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 8ffeb04..5af1a46 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -59,6 +59,14 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp)
> return arg;
> }
>
> +void qmp_boxed_struct(UserDefZero *arg, Error **errp)
> +{
> +}
> +
> +void qmp_boxed_union(UserDefNativeListUnion *arg, Error **errp)
> +{
> +}
> +
> __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
> __org_qemu_x_StructList *b,
> __org_qemu_x_Union2 *c,
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 48b0b31..74171b7 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -410,7 +410,7 @@ following example objects:
> === Commands ===
>
> Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
> - '*returns': TYPE-NAME,
> + '*returns': TYPE-NAME, '*box': true,
> '*gen': false, '*success-response': false }
>
> Commands are defined by using a dictionary containing several members,
> @@ -461,6 +461,20 @@ which would validate this Client JSON Protocol
> transaction:
> => { "execute": "my-second-command" }
> <= { "return": [ { "value": "one" }, { } ] }
>
> +The generator emits a prototype for the user's function implementing
> +the command. Normally, 'data' is a dictionary for an anonymous type,
> +or names a struct type (possibly empty, but not a union), and its
> +members are passed as separate arguments to this function. If the
> +command definition include a key 'box' with the boolean value true,
> +then 'data' is instead the name of any non-empty complex type
> +(struct, union, or alternate), and a pointer to that QAPI type is
> +passed as a single argument.
> +
> +The generator also emits a marshalling function that extracts
> +arguments for the user's function out of an input QDict, calls the
> +user's function, and if it succeeded, builds an output QObject from
> +its return value.
> +
> In rare cases, QAPI cannot express a type-safe representation of a
> corresponding Client JSON Protocol command. You then have to suppress
> generation of a marshalling function by including a key 'gen' with
> @@ -484,7 +498,8 @@ use of this member.
>
> === Events ===
>
> -Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT }
> +Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
> + '*box': true }
>
> Events are defined with the keyword 'event'. It is not allowed to
> name an event 'MAX', since the generator also produces a C enumeration
> @@ -505,6 +520,14 @@ Resulting in this JSON object:
> "data": { "b": "test string" },
> "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>
> +The generator emits a function to send the event. Normally, 'data' is
> +a dictionary for an anonymous type, or names a struct type (possibly
> +empty, but not a union), and its members are passed as separate
> +arguments to this function. If the event definition include a key
> +'box' with the boolean value true, then 'data' is instead the name of
> +any non-empty complex type (struct, union, or alternate), and a
> +pointer to that QAPI type is passed as a single argument.
> +
>
> == Client JSON Protocol introspection ==
>
[Tests snipped, they look good...]
Having read PATCH 07+08 another time, I got one more stylistic remark:
I'd name the flag @boxed, not @box. It's not a box, it's a flag that
tells us that whatever it applies to is boxed.
