Re: [Qemu-devel] [PATCH v7 06/15] qapi: Plumb in 'box' to qapi generator lower levels

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> A future patch will add support for passing a qapi union
> type as the 'data' of a command.  But to do that, the user
> function for implementing the command, as called by the
> generated marshal command, must take the corresponding C
> struct as a single boxed pointer, rather than a breakdown
> into one parameter per member.

Passing arguments as a single C struct may also be convenient in other
cases, e.g. when the callers already have them in a struct, or when the
number of parameters is large.

>                                 This patch adds the
> internal plubming of a 'box' flag associated with each

plumbing

> command and event.  For this patch, no behavior changes,
> other than the testsuite outputting the value of the new
> flag (always False for now).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: rebase to master
> v6: rebase to earlier changes
> ---
>  scripts/qapi.py                         | 46 
> ++++++++++++++++++++-------------
>  scripts/qapi-commands.py                | 20 +++++++-------
>  scripts/qapi-event.py                   | 37 +++++++++++++++-----------
>  scripts/qapi-introspect.py              |  4 +--
>  tests/qapi-schema/event-case.out        |  1 +
>  tests/qapi-schema/ident-with-escape.out |  2 +-
>  tests/qapi-schema/indented-expr.out     |  4 +--
>  tests/qapi-schema/qapi-schema-test.out  | 19 +++++++++-----
>  tests/qapi-schema/test-qapi.py          |  8 +++---
>  9 files changed, 84 insertions(+), 57 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 90ea30c..108363d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -823,10 +823,10 @@ class QAPISchemaVisitor(object):
>          pass
>
>      def visit_command(self, name, info, arg_type, ret_type,
> -                      gen, success_response):
> +                      gen, success_response, box):
>          pass
>
> -    def visit_event(self, name, info, arg_type):
> +    def visit_event(self, name, info, arg_type, box):
>          pass
>
>
> @@ -1158,7 +1158,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>
>  class QAPISchemaCommand(QAPISchemaEntity):
> -    def __init__(self, name, info, arg_type, ret_type, gen, 
> success_response):
> +    def __init__(self, name, info, arg_type, ret_type, gen, success_response,
> +                 box):
>          QAPISchemaEntity.__init__(self, name, info)
>          assert not arg_type or isinstance(arg_type, str)
>          assert not ret_type or isinstance(ret_type, str)
> @@ -1168,12 +1169,14 @@ class QAPISchemaCommand(QAPISchemaEntity):
>          self.ret_type = None
>          self.gen = gen
>          self.success_response = success_response
> +        self.box = box
>
>      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
> +            if not self.box:
> +                assert not self.arg_type.variants

This looks premature.  As long as box=True isn't implemented, I'd assert
not self.box here, with a "not implemented" comment.

>          if self._ret_type_name:
>              self.ret_type = schema.lookup_type(self._ret_type_name)
>              assert isinstance(self.ret_type, QAPISchemaType)
> @@ -1181,24 +1184,26 @@ class QAPISchemaCommand(QAPISchemaEntity):
>      def visit(self, visitor):
>          visitor.visit_command(self.name, self.info,
>                                self.arg_type, self.ret_type,
> -                              self.gen, self.success_response)
> +                              self.gen, self.success_response, self.box)
>
>
>  class QAPISchemaEvent(QAPISchemaEntity):
> -    def __init__(self, name, info, arg_type):
> +    def __init__(self, name, info, arg_type, box):
>          QAPISchemaEntity.__init__(self, name, info)
>          assert not arg_type or isinstance(arg_type, str)
>          self._arg_type_name = arg_type
>          self.arg_type = None
> +        self.box = box
>
>      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
> +            if not self.box:
> +                assert not self.arg_type.variants

Likewise.

>
>      def visit(self, visitor):
> -        visitor.visit_event(self.name, self.info, self.arg_type)
> +        visitor.visit_event(self.name, self.info, self.arg_type, self.box)
>
>
>  class QAPISchema(object):
> @@ -1374,6 +1379,7 @@ class QAPISchema(object):
>          rets = expr.get('returns')
>          gen = expr.get('gen', True)
>          success_response = expr.get('success-response', True)
> +        box = expr.get('box', False)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
>                  name, info, 'arg', self._make_members(data, info))
> @@ -1381,15 +1387,16 @@ class QAPISchema(object):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
>          self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
> -                                           success_response))
> +                                           success_response, box))
>
>      def _def_event(self, expr, info):
>          name = expr['event']
>          data = expr.get('data')
> +        box = expr.get('box', False)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
>                  name, info, 'arg', self._make_members(data, info))
> -        self._def_entity(QAPISchemaEvent(name, info, data))
> +        self._def_entity(QAPISchemaEvent(name, info, data, box))
>
>      def _def_exprs(self):
>          for expr_elem in self.exprs:
> @@ -1632,18 +1639,21 @@ extern const char *const %(c_name)s_lookup[];
>      return ret
>
>
> -def gen_params(arg_type, extra):
> +def gen_params(arg_type, box, extra):
>      if not arg_type:
>          return extra
> -    assert not arg_type.variants
>      ret = ''
>      sep = ''
> -    for memb in arg_type.members:
> -        ret += sep
> -        sep = ', '
> -        if memb.optional:
> -            ret += 'bool has_%s, ' % c_name(memb.name)
> -        ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name))
> +    if box:
> +        assert False     # not implemented
> +    else:
> +        for memb in arg_type.members:
> +            ret += sep
> +            sep = ', '
> +            if memb.optional:
> +                ret += 'bool has_%s, ' % c_name(memb.name)
> +            ret += '%s %s' % (memb.type.c_param_type(),
> +                              c_name(memb.name))

I understand you're rewriting the code this way so you can slot in the
implementation later.  The alternative is to simply assert not box for
now, like this:

  -def gen_params(arg_type, extra):
  +def gen_params(arg_type, box, extra):
       if not arg_type:
           return extra
  -    assert not arg_type.variants
  +    assert not arg_type.variants and not box    # not implemented
       ret = ''

Restricts this patch to propagating the box value to the places that
need it, and leaves the restructuring of the code those places need for
box to the patch that actually implements box.  Might come out neater.
Your choice.  If you choose to keep the restructuring here, please
prepare the reader for it in the commit message.

>      if extra:
>          ret += sep + extra
>      return ret
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 34b6a3a..79d4eea 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -16,20 +16,22 @@ from qapi import *
>  import re
>
>
> -def gen_command_decl(name, arg_type, ret_type):
> +def gen_command_decl(name, arg_type, box, ret_type):
>      return mcgen('''
>  %(c_type)s qmp_%(c_name)s(%(params)s);
>  ''',
>                   c_type=(ret_type and ret_type.c_type()) or 'void',
>                   c_name=c_name(name),
> -                 params=gen_params(arg_type, 'Error **errp'))
> +                 params=gen_params(arg_type, box, 'Error **errp'))
>
>
> -def gen_call(name, arg_type, ret_type):
> +def gen_call(name, arg_type, box, ret_type):
>      ret = ''
>
>      argstr = ''
> -    if arg_type:
> +    if box:
> +        assert False    # not implemented
> +    elif arg_type:
>          assert not arg_type.variants
>          for memb in arg_type.members:
>              if memb.optional:

More restructuring, only less invasive.

> @@ -92,7 +94,7 @@ def gen_marshal_decl(name):
>                   proto=gen_marshal_proto(name))
>
>
> -def gen_marshal(name, arg_type, ret_type):
> +def gen_marshal(name, arg_type, box, ret_type):
>      ret = mcgen('''
>
>  %(proto)s
> @@ -134,7 +136,7 @@ def gen_marshal(name, arg_type, ret_type):
>      (void)args;
>  ''')
>
> -    ret += gen_call(name, arg_type, ret_type)
> +    ret += gen_call(name, arg_type, box, ret_type)
>
>      # 'goto out' produced above for arg_type, and by gen_call() for ret_type
>      if (arg_type and arg_type.members) or ret_type:
> @@ -210,16 +212,16 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>          self._visited_ret_types = None
>
>      def visit_command(self, name, info, arg_type, ret_type,
> -                      gen, success_response):
> +                      gen, success_response, box):
>          if not gen:
>              return
> -        self.decl += gen_command_decl(name, arg_type, ret_type)
> +        self.decl += gen_command_decl(name, arg_type, box, ret_type)
>          if ret_type and ret_type not in self._visited_ret_types:
>              self._visited_ret_types.add(ret_type)
>              self.defn += gen_marshal_output(ret_type)
>          if middle_mode:
>              self.decl += gen_marshal_decl(name)
> -        self.defn += gen_marshal(name, arg_type, ret_type)
> +        self.defn += gen_marshal(name, arg_type, box, ret_type)
>          if not middle_mode:
>              self._regy += gen_register_command(name, success_response)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 09c0a2a..fd953fe 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -14,18 +14,18 @@
>  from qapi import *
>
>
> -def gen_event_send_proto(name, arg_type):
> +def gen_event_send_proto(name, arg_type, box):
>      return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
>          'c_name': c_name(name.lower()),
> -        'param': gen_params(arg_type, 'Error **errp')}
> +        'param': gen_params(arg_type, box, 'Error **errp')}
>
>
> -def gen_event_send_decl(name, arg_type):
> +def gen_event_send_decl(name, arg_type, box):
>      return mcgen('''
>
>  %(proto)s;
>  ''',
> -                 proto=gen_event_send_proto(name, arg_type))
> +                 proto=gen_event_send_proto(name, arg_type, box))
>
>
>  # Declare and initialize an object 'qapi' using parameters from gen_params()
> @@ -52,7 +52,7 @@ def gen_param_var(typ):
>      return ret
>
>
> -def gen_event_send(name, arg_type):
> +def gen_event_send(name, arg_type, box):
>      # FIXME: Our declaration of local variables (and of 'errp' in the
>      # parameter list) can collide with exploded members of the event's
>      # data type passed in as parameters.  If this collision ever hits in
> @@ -67,14 +67,15 @@ def gen_event_send(name, arg_type):
>      Error *err = NULL;
>      QMPEventFuncEmit emit;
>  ''',
> -                proto=gen_event_send_proto(name, arg_type))
> +                proto=gen_event_send_proto(name, arg_type, box))
>
>      if arg_type and not arg_type.is_empty():
>          ret += mcgen('''
>      QObject *obj;
>      Visitor *v;
>  ''')
> -        ret += gen_param_var(arg_type)
> +        if not box:
> +            ret += gen_param_var(arg_type)
>
>      ret += mcgen('''
>

More restructuring in this and the following hunks.

> @@ -92,6 +93,12 @@ def gen_event_send(name, arg_type):
>          ret += mcgen('''
>      v = qmp_output_visitor_new(&obj);
>
> +''')
> +
> +        if box:
> +            assert False     # not implemented
> +        else:
> +            ret += mcgen('''
>      visit_start_struct(v, "%(name)s", NULL, 0, &err);
>      if (err) {
>          goto out;

The need for part would probably be obvious when done together with the
implementation.  It isn't now.

> @@ -101,14 +108,14 @@ def gen_event_send(name, arg_type):
>          visit_check_struct(v, &err);
>      }
>      visit_end_struct(v, NULL);
> -    if (err) {
> -        goto out;
> -    }
> +''',
> +                     name=name, c_name=arg_type.c_name())
> +        ret += gen_err_check()

You're replacing the open-coded emission of the error check by
gen_err_check().  I feel that helper became pointless in commit
12f254f.  Eliminate it?

> +        ret += mcgen('''
>
>      visit_complete(v, &obj);
>      qdict_put_obj(qmp, "data", obj);
> -''',
> -                     name=name, c_name=arg_type.c_name())
> +''')
>
>      ret += mcgen('''
>      emit(%(c_enum)s, qmp, &err);
> @@ -145,9 +152,9 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>          self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>          self._event_names = None
>
> -    def visit_event(self, name, info, arg_type):
> -        self.decl += gen_event_send_decl(name, arg_type)
> -        self.defn += gen_event_send(name, arg_type)
> +    def visit_event(self, name, info, arg_type, box):
> +        self.decl += gen_event_send_decl(name, arg_type, box)
> +        self.defn += gen_event_send(name, arg_type, box)
>          self._event_names.append(name)
>
>
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 474eafd..7646c89 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -154,14 +154,14 @@ const char %(c_name)s[] = %(c_string)s;
>                                      for m in variants.variants]})
>
>      def visit_command(self, name, info, arg_type, ret_type,
> -                      gen, success_response):
> +                      gen, success_response, box):
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
>          self._gen_json(name, 'command',
>                         {'arg-type': self._use_type(arg_type),
>                          'ret-type': self._use_type(ret_type)})
>
> -    def visit_event(self, name, info, arg_type):
> +    def visit_event(self, name, info, arg_type, box):
>          arg_type = arg_type or self._schema.the_empty_object_type
>          self._gen_json(name, 'event', {'arg-type': self._use_type(arg_type)})
>
[Boring tests/qapi-schema/*.out diffs snipped...]
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 65ca19a..fb899a1 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -36,13 +36,15 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          self._print_variants(variants)
>
>      def visit_command(self, name, info, arg_type, ret_type,
> -                      gen, success_response):
> +                      gen, success_response, box):
>          print 'command %s %s -> %s' % \
>              (name, arg_type and arg_type.name, ret_type and ret_type.name)
> -        print '   gen=%s success_response=%s' % (gen, success_response)
> +        print '   gen=%s success_response=%s box=%s' % (gen, 
> success_response,
> +                                                        box)
>
> -    def visit_event(self, name, info, arg_type):
> +    def visit_event(self, name, info, arg_type, box):
>          print 'event %s %s' % (name, arg_type and arg_type.name)
> +        print '   box=%s' % box
>
>      @staticmethod
>      def _print_variants(variants):

No test cases for the new box syntax.  I figure you'll have some in the
patches that implement box.