Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@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 <eblake@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.