[v3,1/4] qapi: Add a 'coroutine' flag for commands
diff mbox series

Message ID 20200115122326.26393-2-kwolf@redhat.com
State New
Headers show
Series
  • qmp: Optionally run handlers in coroutines
Related show

Commit Message

Kevin Wolf Jan. 15, 2020, 12:23 p.m. UTC
This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  1 +
 docs/devel/qapi-code-gen.txt            |  4 ++++
 include/qapi/qmp/dispatch.h             |  1 +
 tests/test-qmp-cmds.c                   |  4 ++++
 scripts/qapi/commands.py                | 17 +++++++++++------
 scripts/qapi/doc.py                     |  2 +-
 scripts/qapi/expr.py                    |  4 ++--
 scripts/qapi/introspect.py              |  2 +-
 scripts/qapi/schema.py                  |  9 ++++++---
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 tests/qapi-schema/test-qapi.py          |  7 ++++---
 11 files changed, 37 insertions(+), 16 deletions(-)

Comments

Markus Armbruster Jan. 15, 2020, 2:59 p.m. UTC | #1
Kevin Wolf <kwolf@redhat.com> writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that
> tells the QMP dispatcher that the command handler is safe to be run in a
> coroutine.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  1 +
>  docs/devel/qapi-code-gen.txt            |  4 ++++
>  include/qapi/qmp/dispatch.h             |  1 +
>  tests/test-qmp-cmds.c                   |  4 ++++
>  scripts/qapi/commands.py                | 17 +++++++++++------
>  scripts/qapi/doc.py                     |  2 +-
>  scripts/qapi/expr.py                    |  4 ++--
>  scripts/qapi/introspect.py              |  2 +-
>  scripts/qapi/schema.py                  |  9 ++++++---
>  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>  tests/qapi-schema/test-qapi.py          |  7 ++++---
>  11 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 9abf175fe0..1a850fe171 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -147,6 +147,7 @@
>    'returns': 'UserDefTwo' }
>  
>  { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
> +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
>  
>  # Returning a non-dictionary requires a name from the whitelist
>  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 45c93a43cc..753f6711d3 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -457,6 +457,7 @@ Syntax:
>                  '*gen': false,
>                  '*allow-oob': true,
>                  '*allow-preconfig': true,
> +                '*coroutine': true,
>                  '*if': COND,
>                  '*features': FEATURES }
>  
> @@ -581,6 +582,9 @@ before the machine is built.  It defaults to false.  For example:
>  QMP is available before the machine is built only when QEMU was
>  started with --preconfig.
>  
> +Member 'coroutine' tells the QMP dispatcher whether the command handler
> +is safe to be run in a coroutine. It defaults to false.

Two spaces after sentence-ending period, for consistency with the rest
of the file.

As discussed in review of prior versions, coroutine-safety is an
implementation detail that should not be exposed to management
applications.  Therefore, we want a flag, not a feature.

As far as I can tell, the new flag has no effect until PATCH 3 puts it
to use.  That's okay.

The doc update tells us when we may say 'coroutine': true, namely when
the handler function is coroutine-safe.  It doesn't quite tell us what
difference it makes, or rather will make after PATCH 3.  I think it
should.

In review of a prior version, Marc-André wondered whether keeping
allow-oob and coroutine separate makes sense.  Recall qapi-code-gen.txt:

    An OOB-capable command handler must satisfy the following conditions:

    - It terminates quickly.
    - It does not invoke system calls that may block.
    - It does not access guest RAM that may block when userfaultfd is
      enabled for postcopy live migration.
    - It takes only "fast" locks, i.e. all critical sections protected by
      any lock it takes also satisfy the conditions for OOB command
      handler code.

    The restrictions on locking limit access to shared state.  Such access
    requires synchronization, but OOB commands can't take the BQL or any
    other "slow" lock.

Kevin, does this rule out coroutine use?

> +
>  The optional 'if' member specifies a conditional.  See "Configuring
>  the schema" below for more on this.
>  
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 9aa426a398..d6ce9efc8e 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -24,6 +24,7 @@ typedef enum QmpCommandOptions
>      QCO_NO_SUCCESS_RESP       =  (1U << 0),
>      QCO_ALLOW_OOB             =  (1U << 1),
>      QCO_ALLOW_PRECONFIG       =  (1U << 2),
> +    QCO_COROUTINE             =  (1U << 3),
>  } QmpCommandOptions;
>  
>  typedef struct QmpCommand
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index 27b0afe55a..e2f71e42a1 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -34,6 +34,10 @@ void qmp_cmd_success_response(Error **errp)
>  {
>  }
>  
> +void qmp_coroutine_cmd(Error **errp)
> +{
> +}
> +
>  Empty2 *qmp_user_def_cmd0(Error **errp)
>  {
>      return g_new0(Empty2, 1);
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index ab98e504f3..97e51401f1 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory.
>  
>  from qapi.common import *
>  from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
> +from typing import List
>  
>  
>  def gen_command_decl(name, arg_type, boxed, ret_type):
> @@ -194,8 +195,9 @@ out:
>      return ret
>  
>  
> -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> -    options = []
> +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
> +                         allow_preconfig: bool, coroutine: bool) -> str:
> +    options = [] # type: List[str]

Not sure such isolated type hints make sense.  I'd welcome patches to
explore systematic use of type hints.  Suggest to start with just one
module, so we can gauge effort and benefit before we jump in whole hog.

>  
>      if not success_response:
>          options += ['QCO_NO_SUCCESS_RESP']
> @@ -203,18 +205,20 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig):
>          options += ['QCO_ALLOW_OOB']
>      if allow_preconfig:
>          options += ['QCO_ALLOW_PRECONFIG']
> +    if coroutine:
> +        options += ['QCO_COROUTINE']
>  
>      if not options:
>          options = ['QCO_NO_OPTIONS']
>  
> -    options = " | ".join(options)
> +    options_str = " | ".join(options)
>  
>      ret = mcgen('''
>      qmp_register_command(cmds, "%(name)s",
>                           qmp_marshal_%(c_name)s, %(opts)s);
>  ''',
>                  name=name, c_name=c_name(name),
> -                opts=options)
> +                opts=options_str)
>      return ret
>  
>  

Some extra churn due to type hints here.  Distracting.  Suggest not to
mix adding type hints to existing code with feature work.

> @@ -278,7 +282,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>                        success_response, boxed, allow_oob, allow_preconfig,
> -                      features):
> +                      coroutine, features):
>          if not gen:
>              return
>          # FIXME: If T is a user-defined type, the user is responsible
> @@ -296,7 +300,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>              self._genh.add(gen_marshal_decl(name))
>              self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>              self._regy.add(gen_register_command(name, success_response,
> -                                                allow_oob, allow_preconfig))
> +                                                allow_oob, allow_preconfig,
> +                                                coroutine))
>  
>  
>  def gen_commands(schema, output_dir, prefix):
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 6f1c17f71f..8b6978c81e 100644
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -265,7 +265,7 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
>  
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>                        success_response, boxed, allow_oob, allow_preconfig,
> -                      features):
> +                      coroutine, features):
>          doc = self.cur_doc
>          self._gen.add(texi_msg('Command', doc, ifcond,
>                                 texi_arguments(doc,
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d7a289eded..9dbfa3edf0 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -89,7 +89,7 @@ def check_flags(expr, info):
>          if key in expr and expr[key] is not False:
>              raise QAPISemError(
>                  info, "flag '%s' may only use false value" % key)
> -    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
> +    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
>          if key in expr and expr[key] is not True:
>              raise QAPISemError(
>                  info, "flag '%s' may only use true value" % key)
> @@ -344,7 +344,7 @@ def check_exprs(exprs):
>                         ['command'],
>                         ['data', 'returns', 'boxed', 'if', 'features',
>                          'gen', 'success-response', 'allow-oob',
> -                        'allow-preconfig'])
> +                        'allow-preconfig', 'coroutine'])
>              normalize_members(expr.get('data'))
>              check_command(expr, info)
>          elif meta == 'event':
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index b3a463dd8b..8a296a69d6 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -212,7 +212,7 @@ const QLitObject %(c_name)s = %(c_string)s;
>  
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>                        success_response, boxed, allow_oob, allow_preconfig,
> -                      features):
> +                      coroutine, features):
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
>          obj = {'arg-type': self._use_type(arg_type),
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cf0045f34e..753bf233a6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -128,7 +128,7 @@ class QAPISchemaVisitor(object):
>  
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>                        success_response, boxed, allow_oob, allow_preconfig,
> -                      features):
> +                      coroutine, features):
>          pass
>  
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
> @@ -678,7 +678,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>  
>      def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
>                   gen, success_response, boxed, allow_oob, allow_preconfig,
> -                 features):
> +                 coroutine, features):
>          QAPISchemaEntity.__init__(self, name, info, doc, ifcond, features)
>          assert not arg_type or isinstance(arg_type, str)
>          assert not ret_type or isinstance(ret_type, str)
> @@ -691,6 +691,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>          self.boxed = boxed
>          self.allow_oob = allow_oob
>          self.allow_preconfig = allow_preconfig
> +        self.coroutine = coroutine
>  
>      def check(self, schema):
>          QAPISchemaEntity.check(self, schema)
> @@ -732,7 +733,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>                                self.arg_type, self.ret_type,
>                                self.gen, self.success_response,
>                                self.boxed, self.allow_oob,
> -                              self.allow_preconfig,
> +                              self.allow_preconfig, self.coroutine,
>                                self.features)
>  
>  
> @@ -1014,6 +1015,7 @@ class QAPISchema(object):
>          boxed = expr.get('boxed', False)
>          allow_oob = expr.get('allow-oob', False)
>          allow_preconfig = expr.get('allow-preconfig', False)
> +        coroutine = expr.get('coroutine', False)
>          ifcond = expr.get('if')
>          features = expr.get('features', [])
>          if isinstance(data, OrderedDict):
> @@ -1025,6 +1027,7 @@ class QAPISchema(object):
>          self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
>                                             gen, success_response,
>                                             boxed, allow_oob, allow_preconfig,
> +                                           coroutine,
>                                             self._make_features(features, info)))
>  
>      def _def_event(self, expr, info, doc):
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 3660e75a48..51bfe8bfc7 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -217,6 +217,8 @@ command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
>      gen=True success_response=True boxed=False oob=False preconfig=False
>  command cmd-success-response None -> None
>      gen=True success_response=False boxed=False oob=False preconfig=False
> +command coroutine-cmd None -> None
> +    gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True
>  object q_obj_guest-get-time-arg
>      member a: int optional=False
>      member b: int optional=True
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index bad14edb47..7a8e65188d 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -70,12 +70,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>  
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>                        success_response, boxed, allow_oob, allow_preconfig,
> -                      features):
> +                      coroutine, features):
>          print('command %s %s -> %s'
>                % (name, arg_type and arg_type.name,
>                   ret_type and ret_type.name))
> -        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
> -              % (gen, success_response, boxed, allow_oob, allow_preconfig))
> +        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
> +              % (gen, success_response, boxed, allow_oob, allow_preconfig,
> +                 " coroutine=True" if coroutine else ""))
>          self._print_if(ifcond)
>          self._print_features(features)

Looks sane to me.
Kevin Wolf Jan. 15, 2020, 3:58 p.m. UTC | #2
Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This patch adds a new 'coroutine' flag to QMP command definitions that
> > tells the QMP dispatcher that the command handler is safe to be run in a
> > coroutine.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/qapi-schema/qapi-schema-test.json |  1 +
> >  docs/devel/qapi-code-gen.txt            |  4 ++++
> >  include/qapi/qmp/dispatch.h             |  1 +
> >  tests/test-qmp-cmds.c                   |  4 ++++
> >  scripts/qapi/commands.py                | 17 +++++++++++------
> >  scripts/qapi/doc.py                     |  2 +-
> >  scripts/qapi/expr.py                    |  4 ++--
> >  scripts/qapi/introspect.py              |  2 +-
> >  scripts/qapi/schema.py                  |  9 ++++++---
> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
> >  11 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> > index 9abf175fe0..1a850fe171 100644
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -147,6 +147,7 @@
> >    'returns': 'UserDefTwo' }
> >  
> >  { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
> > +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
> >  
> >  # Returning a non-dictionary requires a name from the whitelist
> >  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 45c93a43cc..753f6711d3 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -457,6 +457,7 @@ Syntax:
> >                  '*gen': false,
> >                  '*allow-oob': true,
> >                  '*allow-preconfig': true,
> > +                '*coroutine': true,
> >                  '*if': COND,
> >                  '*features': FEATURES }
> >  
> > @@ -581,6 +582,9 @@ before the machine is built.  It defaults to false.  For example:
> >  QMP is available before the machine is built only when QEMU was
> >  started with --preconfig.
> >  
> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
> > +is safe to be run in a coroutine. It defaults to false.
> 
> Two spaces after sentence-ending period, for consistency with the rest
> of the file.

Ok.

> As discussed in review of prior versions, coroutine-safety is an
> implementation detail that should not be exposed to management
> applications.  Therefore, we want a flag, not a feature.
> 
> As far as I can tell, the new flag has no effect until PATCH 3 puts it
> to use.  That's okay.
> 
> The doc update tells us when we may say 'coroutine': true, namely when
> the handler function is coroutine-safe.  It doesn't quite tell us what
> difference it makes, or rather will make after PATCH 3.  I think it
> should.

Fair requirement. Can I describe it as if patch 3 were already in? That
is, the documentation says that the handler _will_ be run in a coroutine
rather than _may_ run it in a coroutine?

> In review of a prior version, Marc-André wondered whether keeping
> allow-oob and coroutine separate makes sense.  Recall qapi-code-gen.txt:
> 
>     An OOB-capable command handler must satisfy the following conditions:
> 
>     - It terminates quickly.
>     - It does not invoke system calls that may block.
>     - It does not access guest RAM that may block when userfaultfd is
>       enabled for postcopy live migration.
>     - It takes only "fast" locks, i.e. all critical sections protected by
>       any lock it takes also satisfy the conditions for OOB command
>       handler code.
> 
>     The restrictions on locking limit access to shared state.  Such access
>     requires synchronization, but OOB commands can't take the BQL or any
>     other "slow" lock.
> 
> Kevin, does this rule out coroutine use?

Not strictly, though I also can't think of a case where you would want
to use a coroutine with these requirements.

If I understand correctly, OOB-capable commands can be run either OOB
with 'exec-oob' or like normal commands with 'execute'. If an OOB
handler is marked as coroutine-safe, 'execute' will run it in a
coroutine (and the restriction above don't apply) and 'exec-oob' will
run it outside of coroutine context.

I have no idea if we will eventually get a case where the command wants
to behave different between the two modes and actually has use for a
coroutine. I hope not.

But using two bools rather than a single enum keeps the code simple and
leaves us all options open if it turns out that we do have a use case.

> > +
> >  The optional 'if' member specifies a conditional.  See "Configuring
> >  the schema" below for more on this.
> >  
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 9aa426a398..d6ce9efc8e 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -24,6 +24,7 @@ typedef enum QmpCommandOptions
> >      QCO_NO_SUCCESS_RESP       =  (1U << 0),
> >      QCO_ALLOW_OOB             =  (1U << 1),
> >      QCO_ALLOW_PRECONFIG       =  (1U << 2),
> > +    QCO_COROUTINE             =  (1U << 3),
> >  } QmpCommandOptions;
> >  
> >  typedef struct QmpCommand
> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> > index 27b0afe55a..e2f71e42a1 100644
> > --- a/tests/test-qmp-cmds.c
> > +++ b/tests/test-qmp-cmds.c
> > @@ -34,6 +34,10 @@ void qmp_cmd_success_response(Error **errp)
> >  {
> >  }
> >  
> > +void qmp_coroutine_cmd(Error **errp)
> > +{
> > +}
> > +
> >  Empty2 *qmp_user_def_cmd0(Error **errp)
> >  {
> >      return g_new0(Empty2, 1);
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index ab98e504f3..97e51401f1 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory.
> >  
> >  from qapi.common import *
> >  from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
> > +from typing import List
> >  
> >  
> >  def gen_command_decl(name, arg_type, boxed, ret_type):
> > @@ -194,8 +195,9 @@ out:
> >      return ret
> >  
> >  
> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> > -    options = []
> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
> > +                         allow_preconfig: bool, coroutine: bool) -> str:
> > +    options = [] # type: List[str]
> 
> Not sure such isolated type hints make sense.  I'd welcome patches to
> explore systematic use of type hints.  Suggest to start with just one
> module, so we can gauge effort and benefit before we jump in whole hog.

Any documentation is better than no documentation, imho.

If you insist, I'll remove the type hints, but finding the origin of
values passed as parameters to find out what type they are is a very
common activity for me when touching the QAPI code. Doing that every
time from scratch is not a good use of my time.

> >  
> >      if not success_response:
> >          options += ['QCO_NO_SUCCESS_RESP']
> > @@ -203,18 +205,20 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> >          options += ['QCO_ALLOW_OOB']
> >      if allow_preconfig:
> >          options += ['QCO_ALLOW_PRECONFIG']
> > +    if coroutine:
> > +        options += ['QCO_COROUTINE']
> >  
> >      if not options:
> >          options = ['QCO_NO_OPTIONS']
> >  
> > -    options = " | ".join(options)
> > +    options_str = " | ".join(options)
> >  
> >      ret = mcgen('''
> >      qmp_register_command(cmds, "%(name)s",
> >                           qmp_marshal_%(c_name)s, %(opts)s);
> >  ''',
> >                  name=name, c_name=c_name(name),
> > -                opts=options)
> > +                opts=options_str)
> >      return ret
> >  
> >  
> 
> Some extra churn due to type hints here.  Distracting.  Suggest not to
> mix adding type hints to existing code with feature work.

If you would be open for a compromise, I could leave options
unannotated, but keep the typed parameter list.

Kevin
Markus Armbruster Jan. 16, 2020, 1 p.m. UTC | #3
Kevin Wolf <kwolf@redhat.com> writes:

> Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This patch adds a new 'coroutine' flag to QMP command definitions that
>> > tells the QMP dispatcher that the command handler is safe to be run in a
>> > coroutine.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/qapi-schema/qapi-schema-test.json |  1 +
>> >  docs/devel/qapi-code-gen.txt            |  4 ++++
>> >  include/qapi/qmp/dispatch.h             |  1 +
>> >  tests/test-qmp-cmds.c                   |  4 ++++
>> >  scripts/qapi/commands.py                | 17 +++++++++++------
>> >  scripts/qapi/doc.py                     |  2 +-
>> >  scripts/qapi/expr.py                    |  4 ++--
>> >  scripts/qapi/introspect.py              |  2 +-
>> >  scripts/qapi/schema.py                  |  9 ++++++---
>> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
>> >  11 files changed, 37 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>> > index 9abf175fe0..1a850fe171 100644
>> > --- a/tests/qapi-schema/qapi-schema-test.json
>> > +++ b/tests/qapi-schema/qapi-schema-test.json
>> > @@ -147,6 +147,7 @@
>> >    'returns': 'UserDefTwo' }
>> >  
>> >  { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
>> > +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
>> >  
>> >  # Returning a non-dictionary requires a name from the whitelist
>> >  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> > index 45c93a43cc..753f6711d3 100644
>> > --- a/docs/devel/qapi-code-gen.txt
>> > +++ b/docs/devel/qapi-code-gen.txt
>> > @@ -457,6 +457,7 @@ Syntax:
>> >                  '*gen': false,
>> >                  '*allow-oob': true,
>> >                  '*allow-preconfig': true,
>> > +                '*coroutine': true,
>> >                  '*if': COND,
>> >                  '*features': FEATURES }
>> >  
>> > @@ -581,6 +582,9 @@ before the machine is built.  It defaults to false.  For example:
>> >  QMP is available before the machine is built only when QEMU was
>> >  started with --preconfig.
>> >  
>> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
>> > +is safe to be run in a coroutine. It defaults to false.
>> 
>> Two spaces after sentence-ending period, for consistency with the rest
>> of the file.
>
> Ok.
>
>> As discussed in review of prior versions, coroutine-safety is an
>> implementation detail that should not be exposed to management
>> applications.  Therefore, we want a flag, not a feature.
>> 
>> As far as I can tell, the new flag has no effect until PATCH 3 puts it
>> to use.  That's okay.
>> 
>> The doc update tells us when we may say 'coroutine': true, namely when
>> the handler function is coroutine-safe.  It doesn't quite tell us what
>> difference it makes, or rather will make after PATCH 3.  I think it
>> should.
>
> Fair requirement. Can I describe it as if patch 3 were already in? That
> is, the documentation says that the handler _will_ be run in a coroutine
> rather than _may_ run it in a coroutine?

Your choice.  If you choose to pretend PATCH 3 was in, have your commit
message point that out.

>> In review of a prior version, Marc-André wondered whether keeping
>> allow-oob and coroutine separate makes sense.  Recall qapi-code-gen.txt:
>> 
>>     An OOB-capable command handler must satisfy the following conditions:
>> 
>>     - It terminates quickly.
>>     - It does not invoke system calls that may block.
>>     - It does not access guest RAM that may block when userfaultfd is
>>       enabled for postcopy live migration.
>>     - It takes only "fast" locks, i.e. all critical sections protected by
>>       any lock it takes also satisfy the conditions for OOB command
>>       handler code.
>> 
>>     The restrictions on locking limit access to shared state.  Such access
>>     requires synchronization, but OOB commands can't take the BQL or any
>>     other "slow" lock.
>> 
>> Kevin, does this rule out coroutine use?
>
> Not strictly, though I also can't think of a case where you would want
> to use a coroutine with these requirements.
>
> If I understand correctly, OOB-capable commands can be run either OOB
> with 'exec-oob' or like normal commands with 'execute'.

Correct.

>                                                         If an OOB
> handler is marked as coroutine-safe, 'execute' will run it in a
> coroutine (and the restriction above don't apply) and 'exec-oob' will
> run it outside of coroutine context.

Let me convince myself you're right.

Cases before this series:

(exec) execute, allow-oob does not matter

    Run in main loop bottom half monitor_qmp_bh_dispatcher(), outside
    coroutine context, scheduled by handle_qmp_command()

(err1) exec-oob, QMP_CAPABILITY_OOB off, allow-oob does not matter

  Error

(err2) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: false

  Error

(exec-oob) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: true

  Run in iothread / handle_qmp_command(), outside coroutine context

Peeking ahead to PATCH 3...  it split cases (exec):

(exec-co): execute, allow-oob does not matter, coroutine: true

  Run in main loop coroutine qmp_dispatcher_co(), in coroutine context,
  woken up by handle_qmp_command()

(exec): execute, allow-oob does not matter, coroutine: false

  Run in main loop bottom half do_qmp_dispatch_bh(), outside coroutine
  context, scheduled by qmp_dispatcher_co()

It appears not to touch case exec-oob.  Thus, coroutine: true has no
effect when the command is executed with exec-oob.

Looks like you're right :)

> I have no idea if we will eventually get a case where the command wants
> to behave different between the two modes and actually has use for a
> coroutine. I hope not.
>
> But using two bools rather than a single enum keeps the code simple and
> leaves us all options open if it turns out that we do have a use case.

I can buy the argument "the two are conceptually orthogonal, although we
don't haven't found a use for one of the four cases".

Let's review the four combinations of the two flags once more:

* allow-oob: false, coroutine: false

  Handler runs in main loop, outside coroutine context.  Okay.

* allow-oob: false, coroutine: true

  Handler runs in main loop, in coroutine context.  Okay.

* allow-oob: true, coroutine: false

  Handler may run in main loop or in iothread, outside coroutine
  context.  Okay.

* allow-oob: true, coroutine: true

  Handler may run (in main loop, in coroutine context) or (in iothread,
  outside coroutine context).  This "in coroutine context only with
  execute, not with exec-oob" behavior is a bit surprising.

  We could document it, noting that it may change to always run in
  coroutine context.  Or we simply reject this case as "not
  implemented".  Since we have no uses, I'm leaning towards reject.  One
  fewer case to test then.

>> > +
>> >  The optional 'if' member specifies a conditional.  See "Configuring
>> >  the schema" below for more on this.
>> >  
>> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
>> > index 9aa426a398..d6ce9efc8e 100644
>> > --- a/include/qapi/qmp/dispatch.h
>> > +++ b/include/qapi/qmp/dispatch.h
>> > @@ -24,6 +24,7 @@ typedef enum QmpCommandOptions
>> >      QCO_NO_SUCCESS_RESP       =  (1U << 0),
>> >      QCO_ALLOW_OOB             =  (1U << 1),
>> >      QCO_ALLOW_PRECONFIG       =  (1U << 2),
>> > +    QCO_COROUTINE             =  (1U << 3),
>> >  } QmpCommandOptions;
>> >  
>> >  typedef struct QmpCommand
>> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
>> > index 27b0afe55a..e2f71e42a1 100644
>> > --- a/tests/test-qmp-cmds.c
>> > +++ b/tests/test-qmp-cmds.c
>> > @@ -34,6 +34,10 @@ void qmp_cmd_success_response(Error **errp)
>> >  {
>> >  }
>> >  
>> > +void qmp_coroutine_cmd(Error **errp)
>> > +{
>> > +}
>> > +
>> >  Empty2 *qmp_user_def_cmd0(Error **errp)
>> >  {
>> >      return g_new0(Empty2, 1);
>> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> > index ab98e504f3..97e51401f1 100644
>> > --- a/scripts/qapi/commands.py
>> > +++ b/scripts/qapi/commands.py
>> > @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory.
>> >  
>> >  from qapi.common import *
>> >  from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
>> > +from typing import List
>> >  
>> >  
>> >  def gen_command_decl(name, arg_type, boxed, ret_type):
>> > @@ -194,8 +195,9 @@ out:
>> >      return ret
>> >  
>> >  
>> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
>> > -    options = []
>> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
>> > +                         allow_preconfig: bool, coroutine: bool) -> str:
>> > +    options = [] # type: List[str]

One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
526 instead:

          options: List[str] = []

I think we should.

>> 
>> Not sure such isolated type hints make sense.  I'd welcome patches to
>> explore systematic use of type hints.  Suggest to start with just one
>> module, so we can gauge effort and benefit before we jump in whole hog.
>
> Any documentation is better than no documentation, imho.

Inconsistent or grossly incomplete documentation can be worse than no
documentation.

> If you insist, I'll remove the type hints, but finding the origin of
> values passed as parameters to find out what type they are is a very
> common activity for me when touching the QAPI code. Doing that every
> time from scratch is not a good use of my time.

Understand, and I therefore support the idea of exploring use of type
hints.  I'd rather not mix it up with other work, though.

>> >  
>> >      if not success_response:
>> >          options += ['QCO_NO_SUCCESS_RESP']
>> > @@ -203,18 +205,20 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig):
>> >          options += ['QCO_ALLOW_OOB']
>> >      if allow_preconfig:
>> >          options += ['QCO_ALLOW_PRECONFIG']
>> > +    if coroutine:
>> > +        options += ['QCO_COROUTINE']
>> >  
>> >      if not options:
>> >          options = ['QCO_NO_OPTIONS']
>> >  
>> > -    options = " | ".join(options)
>> > +    options_str = " | ".join(options)
>> >  
>> >      ret = mcgen('''
>> >      qmp_register_command(cmds, "%(name)s",
>> >                           qmp_marshal_%(c_name)s, %(opts)s);
>> >  ''',
>> >                  name=name, c_name=c_name(name),
>> > -                opts=options)
>> > +                opts=options_str)
>> >      return ret
>> >  
>> >  
>> 
>> Some extra churn due to type hints here.  Distracting.  Suggest not to
>> mix adding type hints to existing code with feature work.
>
> If you would be open for a compromise, I could leave options
> unannotated, but keep the typed parameter list.

Keeping just the function annotation is much less distracting.  I can't
reject that with a "separate patches for separate things" argument.

I'd still prefer not to, because:

* If we do add systematic type hints in the near future, then delaying
  this one until then shouldn't hurt your productivity.

* If we don't, this lone one won't help your productivity much, but
  it'll look out of place.

I really don't want us to add type hints as we go, because such
open-ended "while we touch it anyway" conversions take forever and a
day.  Maximizes the confusion integral over time.
Kevin Wolf Jan. 16, 2020, 3:02 p.m. UTC | #4
Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > I have no idea if we will eventually get a case where the command wants
> > to behave different between the two modes and actually has use for a
> > coroutine. I hope not.
> >
> > But using two bools rather than a single enum keeps the code simple and
> > leaves us all options open if it turns out that we do have a use case.
> 
> I can buy the argument "the two are conceptually orthogonal, although we
> don't haven't found a use for one of the four cases".
> 
> Let's review the four combinations of the two flags once more:
> 
> * allow-oob: false, coroutine: false
> 
>   Handler runs in main loop, outside coroutine context.  Okay.
> 
> * allow-oob: false, coroutine: true
> 
>   Handler runs in main loop, in coroutine context.  Okay.
> 
> * allow-oob: true, coroutine: false
> 
>   Handler may run in main loop or in iothread, outside coroutine
>   context.  Okay.
> 
> * allow-oob: true, coroutine: true
> 
>   Handler may run (in main loop, in coroutine context) or (in iothread,
>   outside coroutine context).  This "in coroutine context only with
>   execute, not with exec-oob" behavior is a bit surprising.
> 
>   We could document it, noting that it may change to always run in
>   coroutine context.  Or we simply reject this case as "not
>   implemented".  Since we have no uses, I'm leaning towards reject.  One
>   fewer case to test then.

What would be the right mode of rejecting it?

I assume we should catch it somewhere in the QAPI generator (where?) and
then just assert in the C code that both flags aren't set at the same
time?

> >> > @@ -194,8 +195,9 @@ out:
> >> >      return ret
> >> >  
> >> >  
> >> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> >> > -    options = []
> >> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
> >> > +                         allow_preconfig: bool, coroutine: bool) -> str:
> >> > +    options = [] # type: List[str]
> 
> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
> 526 instead:
> 
>           options: List[str] = []
> 
> I think we should.

This requires Python 3.6, unfortunately. The minimum requirement for
building QEMU is 3.5.

> >> Some extra churn due to type hints here.  Distracting.  Suggest not to
> >> mix adding type hints to existing code with feature work.
> >
> > If you would be open for a compromise, I could leave options
> > unannotated, but keep the typed parameter list.
> 
> Keeping just the function annotation is much less distracting.  I can't
> reject that with a "separate patches for separate things" argument.
> 
> I'd still prefer not to, because:
> 
> * If we do add systematic type hints in the near future, then delaying
>   this one until then shouldn't hurt your productivity.
> 
> * If we don't, this lone one won't help your productivity much, but
>   it'll look out of place.
> 
> I really don't want us to add type hints as we go, because such
> open-ended "while we touch it anyway" conversions take forever and a
> day.  Maximizes the confusion integral over time.

I think it's a first time that I'm asked not to document things, but
I'll remove them.

Kevin
Markus Armbruster Jan. 17, 2020, 7:50 a.m. UTC | #5
Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>> 
>>> > This patch adds a new 'coroutine' flag to QMP command definitions that
>>> > tells the QMP dispatcher that the command handler is safe to be run in a
>>> > coroutine.
>>> >
>>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> > ---
>>> >  tests/qapi-schema/qapi-schema-test.json |  1 +
>>> >  docs/devel/qapi-code-gen.txt            |  4 ++++
>>> >  include/qapi/qmp/dispatch.h             |  1 +
>>> >  tests/test-qmp-cmds.c                   |  4 ++++
>>> >  scripts/qapi/commands.py                | 17 +++++++++++------
>>> >  scripts/qapi/doc.py                     |  2 +-
>>> >  scripts/qapi/expr.py                    |  4 ++--
>>> >  scripts/qapi/introspect.py              |  2 +-
>>> >  scripts/qapi/schema.py                  |  9 ++++++---
>>> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>>> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
>>> >  11 files changed, 37 insertions(+), 16 deletions(-)
>>> >
>>> > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>>> > index 9abf175fe0..1a850fe171 100644
>>> > --- a/tests/qapi-schema/qapi-schema-test.json
>>> > +++ b/tests/qapi-schema/qapi-schema-test.json
>>> > @@ -147,6 +147,7 @@
>>> >    'returns': 'UserDefTwo' }
>>> >  
>>> >  { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
>>> > +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
>>> >  
>>> >  # Returning a non-dictionary requires a name from the whitelist
>>> >  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
>>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>>> > index 45c93a43cc..753f6711d3 100644
>>> > --- a/docs/devel/qapi-code-gen.txt
>>> > +++ b/docs/devel/qapi-code-gen.txt
>>> > @@ -457,6 +457,7 @@ Syntax:
>>> >                  '*gen': false,
>>> >                  '*allow-oob': true,
>>> >                  '*allow-preconfig': true,
>>> > +                '*coroutine': true,
>>> >                  '*if': COND,
>>> >                  '*features': FEATURES }
>>> >  
>>> > @@ -581,6 +582,9 @@ before the machine is built.  It defaults to false.  For example:
>>> >  QMP is available before the machine is built only when QEMU was
>>> >  started with --preconfig.
>>> >  
>>> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
>>> > +is safe to be run in a coroutine. It defaults to false.
>>> 
>>> Two spaces after sentence-ending period, for consistency with the rest
>>> of the file.
>>
>> Ok.
>>
>>> As discussed in review of prior versions, coroutine-safety is an
>>> implementation detail that should not be exposed to management
>>> applications.  Therefore, we want a flag, not a feature.
>>> 
>>> As far as I can tell, the new flag has no effect until PATCH 3 puts it
>>> to use.  That's okay.
>>> 
>>> The doc update tells us when we may say 'coroutine': true, namely when
>>> the handler function is coroutine-safe.  It doesn't quite tell us what
>>> difference it makes, or rather will make after PATCH 3.  I think it
>>> should.
>>
>> Fair requirement. Can I describe it as if patch 3 were already in? That
>> is, the documentation says that the handler _will_ be run in a coroutine
>> rather than _may_ run it in a coroutine?
>
> Your choice.  If you choose to pretend PATCH 3 was in, have your commit
> message point that out.
>
>>> In review of a prior version, Marc-André wondered whether keeping
>>> allow-oob and coroutine separate makes sense.  Recall qapi-code-gen.txt:
>>> 
>>>     An OOB-capable command handler must satisfy the following conditions:
>>> 
>>>     - It terminates quickly.
>>>     - It does not invoke system calls that may block.
>>>     - It does not access guest RAM that may block when userfaultfd is
>>>       enabled for postcopy live migration.
>>>     - It takes only "fast" locks, i.e. all critical sections protected by
>>>       any lock it takes also satisfy the conditions for OOB command
>>>       handler code.
>>> 
>>>     The restrictions on locking limit access to shared state.  Such access
>>>     requires synchronization, but OOB commands can't take the BQL or any
>>>     other "slow" lock.
>>> 
>>> Kevin, does this rule out coroutine use?
>>
>> Not strictly, though I also can't think of a case where you would want
>> to use a coroutine with these requirements.
>>
>> If I understand correctly, OOB-capable commands can be run either OOB
>> with 'exec-oob' or like normal commands with 'execute'.
>
> Correct.
>
>>                                                         If an OOB
>> handler is marked as coroutine-safe, 'execute' will run it in a
>> coroutine (and the restriction above don't apply) and 'exec-oob' will
>> run it outside of coroutine context.
>
> Let me convince myself you're right.
>
> Cases before this series:
>
> (exec) execute, allow-oob does not matter
>
>     Run in main loop bottom half monitor_qmp_bh_dispatcher(), outside
>     coroutine context, scheduled by handle_qmp_command()
>
> (err1) exec-oob, QMP_CAPABILITY_OOB off, allow-oob does not matter
>
>   Error
>
> (err2) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: false
>
>   Error
>
> (exec-oob) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: true
>
>   Run in iothread / handle_qmp_command(), outside coroutine context
>
> Peeking ahead to PATCH 3...  it split cases (exec):
>
> (exec-co): execute, allow-oob does not matter, coroutine: true
>
>   Run in main loop coroutine qmp_dispatcher_co(), in coroutine context,
>   woken up by handle_qmp_command()
>
> (exec): execute, allow-oob does not matter, coroutine: false
>
>   Run in main loop bottom half do_qmp_dispatch_bh(), outside coroutine
>   context, scheduled by qmp_dispatcher_co()
>
> It appears not to touch case exec-oob.  Thus, coroutine: true has no
> effect when the command is executed with exec-oob.

Looking at PATCH 3 again, I got temporarily confused again.  Let me
spell things out even more, to improve my chances at staying not
confused...

To effect the split of (exec), you rewrite bottom half
monitor_qmp_bh_dispatcher() as coroutine monitor_qmp_dispatcher_co(),
then have do_qmp_dispatch() either call the the handler directly, or
schedule it to run in a bottom half.  Cases:

(exec-co): handle_qmp_command() sends the command to coroutine
monitor_qmp_dispatcher_co(), which calls monitor_qmp_dispatch(), which
runs the handler, in coroutine context, in the main loop.

(exec): Likewise, except monitor_qmp_dispatch() schedules the handler to
run in a bottom half, outside coroutine context, in the main loop.

(exec-oob): handle_qmp_command() runs monitor_qmp_dispatch(), which runs
the handler, outside coroutine context, in the iothread.

> Looks like you're right :)

[...]
Markus Armbruster Jan. 17, 2020, 7:57 a.m. UTC | #6
Kevin Wolf <kwolf@redhat.com> writes:

> Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > I have no idea if we will eventually get a case where the command wants
>> > to behave different between the two modes and actually has use for a
>> > coroutine. I hope not.
>> >
>> > But using two bools rather than a single enum keeps the code simple and
>> > leaves us all options open if it turns out that we do have a use case.
>> 
>> I can buy the argument "the two are conceptually orthogonal, although we
>> don't haven't found a use for one of the four cases".
>> 
>> Let's review the four combinations of the two flags once more:
>> 
>> * allow-oob: false, coroutine: false
>> 
>>   Handler runs in main loop, outside coroutine context.  Okay.
>> 
>> * allow-oob: false, coroutine: true
>> 
>>   Handler runs in main loop, in coroutine context.  Okay.
>> 
>> * allow-oob: true, coroutine: false
>> 
>>   Handler may run in main loop or in iothread, outside coroutine
>>   context.  Okay.
>> 
>> * allow-oob: true, coroutine: true
>> 
>>   Handler may run (in main loop, in coroutine context) or (in iothread,
>>   outside coroutine context).  This "in coroutine context only with
>>   execute, not with exec-oob" behavior is a bit surprising.
>> 
>>   We could document it, noting that it may change to always run in
>>   coroutine context.  Or we simply reject this case as "not
>>   implemented".  Since we have no uses, I'm leaning towards reject.  One
>>   fewer case to test then.
>
> What would be the right mode of rejecting it?
>
> I assume we should catch it somewhere in the QAPI generator (where?) and

check_flags() in expr.py?

> then just assert in the C code that both flags aren't set at the same
> time?

I think you already do, in do_qmp_dispatch():

    assert(!(oob && qemu_in_coroutine()));

Not sure that's the best spot.  Let's see when I review PATCH 3.

>> >> > @@ -194,8 +195,9 @@ out:
>> >> >      return ret
>> >> >  
>> >> >  
>> >> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
>> >> > -    options = []
>> >> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
>> >> > +                         allow_preconfig: bool, coroutine: bool) -> str:
>> >> > +    options = [] # type: List[str]
>> 
>> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
>> 526 instead:
>> 
>>           options: List[str] = []
>> 
>> I think we should.
>
> This requires Python 3.6, unfortunately. The minimum requirement for
> building QEMU is 3.5.

*Sigh*

>> >> Some extra churn due to type hints here.  Distracting.  Suggest not to
>> >> mix adding type hints to existing code with feature work.
>> >
>> > If you would be open for a compromise, I could leave options
>> > unannotated, but keep the typed parameter list.
>> 
>> Keeping just the function annotation is much less distracting.  I can't
>> reject that with a "separate patches for separate things" argument.
>> 
>> I'd still prefer not to, because:
>> 
>> * If we do add systematic type hints in the near future, then delaying
>>   this one until then shouldn't hurt your productivity.
>> 
>> * If we don't, this lone one won't help your productivity much, but
>>   it'll look out of place.
>> 
>> I really don't want us to add type hints as we go, because such
>> open-ended "while we touch it anyway" conversions take forever and a
>> day.  Maximizes the confusion integral over time.
>
> I think it's a first time that I'm asked not to document things, but
> I'll remove them.

I'm asking you not to mix documenting existing code with adding a
new feature to it in the same patch.

Hopefully, that won't lead to the documentation getting dropped for
good.  That would be sad.
Kevin Wolf Jan. 17, 2020, 9:40 a.m. UTC | #7
Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> > I have no idea if we will eventually get a case where the command wants
> >> > to behave different between the two modes and actually has use for a
> >> > coroutine. I hope not.
> >> >
> >> > But using two bools rather than a single enum keeps the code simple and
> >> > leaves us all options open if it turns out that we do have a use case.
> >> 
> >> I can buy the argument "the two are conceptually orthogonal, although we
> >> don't haven't found a use for one of the four cases".
> >> 
> >> Let's review the four combinations of the two flags once more:
> >> 
> >> * allow-oob: false, coroutine: false
> >> 
> >>   Handler runs in main loop, outside coroutine context.  Okay.
> >> 
> >> * allow-oob: false, coroutine: true
> >> 
> >>   Handler runs in main loop, in coroutine context.  Okay.
> >> 
> >> * allow-oob: true, coroutine: false
> >> 
> >>   Handler may run in main loop or in iothread, outside coroutine
> >>   context.  Okay.
> >> 
> >> * allow-oob: true, coroutine: true
> >> 
> >>   Handler may run (in main loop, in coroutine context) or (in iothread,
> >>   outside coroutine context).  This "in coroutine context only with
> >>   execute, not with exec-oob" behavior is a bit surprising.
> >> 
> >>   We could document it, noting that it may change to always run in
> >>   coroutine context.  Or we simply reject this case as "not
> >>   implemented".  Since we have no uses, I'm leaning towards reject.  One
> >>   fewer case to test then.
> >
> > What would be the right mode of rejecting it?
> >
> > I assume we should catch it somewhere in the QAPI generator (where?) and
> 
> check_flags() in expr.py?

Looks like the right place, thanks.

> > then just assert in the C code that both flags aren't set at the same
> > time?
> 
> I think you already do, in do_qmp_dispatch():
> 
>     assert(!(oob && qemu_in_coroutine()));
> 
> Not sure that's the best spot.  Let's see when I review PATCH 3.

This asserts that exec-oob handlers aren't executed in coroutine
context. It doesn't assert that the handler doesn't have QCO_COROUTINE
and QCO_ALLOW_OOB set at the same time.

> >> >> > @@ -194,8 +195,9 @@ out:
> >> >> >      return ret
> >> >> >  
> >> >> >  
> >> >> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> >> >> > -    options = []
> >> >> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
> >> >> > +                         allow_preconfig: bool, coroutine: bool) -> str:
> >> >> > +    options = [] # type: List[str]
> >> 
> >> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
> >> 526 instead:
> >> 
> >>           options: List[str] = []
> >> 
> >> I think we should.
> >
> > This requires Python 3.6, unfortunately. The minimum requirement for
> > building QEMU is 3.5.
> 
> *Sigh*

One of the reasons why I would have preferred 3.6 as the minimum, but
our policy says that Debian oldstabe is still relevant for another two
years. *shrug*

Kevin
Markus Armbruster Jan. 17, 2020, 10:43 a.m. UTC | #8
Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> > I have no idea if we will eventually get a case where the command wants
>> >> > to behave different between the two modes and actually has use for a
>> >> > coroutine. I hope not.
>> >> >
>> >> > But using two bools rather than a single enum keeps the code simple and
>> >> > leaves us all options open if it turns out that we do have a use case.
>> >> 
>> >> I can buy the argument "the two are conceptually orthogonal, although we
>> >> don't haven't found a use for one of the four cases".
>> >> 
>> >> Let's review the four combinations of the two flags once more:
>> >> 
>> >> * allow-oob: false, coroutine: false
>> >> 
>> >>   Handler runs in main loop, outside coroutine context.  Okay.
>> >> 
>> >> * allow-oob: false, coroutine: true
>> >> 
>> >>   Handler runs in main loop, in coroutine context.  Okay.
>> >> 
>> >> * allow-oob: true, coroutine: false
>> >> 
>> >>   Handler may run in main loop or in iothread, outside coroutine
>> >>   context.  Okay.
>> >> 
>> >> * allow-oob: true, coroutine: true
>> >> 
>> >>   Handler may run (in main loop, in coroutine context) or (in iothread,
>> >>   outside coroutine context).  This "in coroutine context only with
>> >>   execute, not with exec-oob" behavior is a bit surprising.
>> >> 
>> >>   We could document it, noting that it may change to always run in
>> >>   coroutine context.  Or we simply reject this case as "not
>> >>   implemented".  Since we have no uses, I'm leaning towards reject.  One
>> >>   fewer case to test then.
>> >
>> > What would be the right mode of rejecting it?
>> >
>> > I assume we should catch it somewhere in the QAPI generator (where?) and
>> 
>> check_flags() in expr.py?
>
> Looks like the right place, thanks.
>
>> > then just assert in the C code that both flags aren't set at the same
>> > time?
>> 
>> I think you already do, in do_qmp_dispatch():
>> 
>>     assert(!(oob && qemu_in_coroutine()));
>> 
>> Not sure that's the best spot.  Let's see when I review PATCH 3.
>
> This asserts that exec-oob handlers aren't executed in coroutine
> context. It doesn't assert that the handler doesn't have QCO_COROUTINE
> and QCO_ALLOW_OOB set at the same time.

Asserting this explicitly can't hurt.  qmp_register_command()?

>> >> >> > @@ -194,8 +195,9 @@ out:
>> >> >> >      return ret
>> >> >> >  
>> >> >> >  
>> >> >> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
>> >> >> > -    options = []
>> >> >> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
>> >> >> > +                         allow_preconfig: bool, coroutine: bool) -> str:
>> >> >> > +    options = [] # type: List[str]
>> >> 
>> >> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
>> >> 526 instead:
>> >> 
>> >>           options: List[str] = []
>> >> 
>> >> I think we should.
>> >
>> > This requires Python 3.6, unfortunately. The minimum requirement for
>> > building QEMU is 3.5.
>> 
>> *Sigh*
>
> One of the reasons why I would have preferred 3.6 as the minimum, but
> our policy says that Debian oldstabe is still relevant for another two
> years. *shrug*

3.5 EOL is scheduled for 2020-09-13.
https://devguide.python.org/#status-of-python-branches

Whether Debian can support it beyond that date seems doubtful.

For another reason to want 3.6, see
[PATCH] qapi: Fix code generation with Python 3.5
Message-Id: <20200116202558.31473-1-armbru@redhat.com>
Kevin Wolf Jan. 17, 2020, 11:08 a.m. UTC | #9
Am 17.01.2020 um 11:43 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> > I have no idea if we will eventually get a case where the command wants
> >> >> > to behave different between the two modes and actually has use for a
> >> >> > coroutine. I hope not.
> >> >> >
> >> >> > But using two bools rather than a single enum keeps the code simple and
> >> >> > leaves us all options open if it turns out that we do have a use case.
> >> >> 
> >> >> I can buy the argument "the two are conceptually orthogonal, although we
> >> >> don't haven't found a use for one of the four cases".
> >> >> 
> >> >> Let's review the four combinations of the two flags once more:
> >> >> 
> >> >> * allow-oob: false, coroutine: false
> >> >> 
> >> >>   Handler runs in main loop, outside coroutine context.  Okay.
> >> >> 
> >> >> * allow-oob: false, coroutine: true
> >> >> 
> >> >>   Handler runs in main loop, in coroutine context.  Okay.
> >> >> 
> >> >> * allow-oob: true, coroutine: false
> >> >> 
> >> >>   Handler may run in main loop or in iothread, outside coroutine
> >> >>   context.  Okay.
> >> >> 
> >> >> * allow-oob: true, coroutine: true
> >> >> 
> >> >>   Handler may run (in main loop, in coroutine context) or (in iothread,
> >> >>   outside coroutine context).  This "in coroutine context only with
> >> >>   execute, not with exec-oob" behavior is a bit surprising.
> >> >> 
> >> >>   We could document it, noting that it may change to always run in
> >> >>   coroutine context.  Or we simply reject this case as "not
> >> >>   implemented".  Since we have no uses, I'm leaning towards reject.  One
> >> >>   fewer case to test then.
> >> >
> >> > What would be the right mode of rejecting it?
> >> >
> >> > I assume we should catch it somewhere in the QAPI generator (where?) and
> >> 
> >> check_flags() in expr.py?
> >
> > Looks like the right place, thanks.
> >
> >> > then just assert in the C code that both flags aren't set at the same
> >> > time?
> >> 
> >> I think you already do, in do_qmp_dispatch():
> >> 
> >>     assert(!(oob && qemu_in_coroutine()));
> >> 
> >> Not sure that's the best spot.  Let's see when I review PATCH 3.
> >
> > This asserts that exec-oob handlers aren't executed in coroutine
> > context. It doesn't assert that the handler doesn't have QCO_COROUTINE
> > and QCO_ALLOW_OOB set at the same time.
> 
> Asserting this explicitly can't hurt.  qmp_register_command()?

Yes, that's where I put it.

> >> >> >> > @@ -194,8 +195,9 @@ out:
> >> >> >> >      return ret
> >> >> >> >  
> >> >> >> >  
> >> >> >> > -def gen_register_command(name, success_response, allow_oob, allow_preconfig):
> >> >> >> > -    options = []
> >> >> >> > +def gen_register_command(name: str, success_response: bool, allow_oob: bool,
> >> >> >> > +                         allow_preconfig: bool, coroutine: bool) -> str:
> >> >> >> > +    options = [] # type: List[str]
> >> >> 
> >> >> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
> >> >> 526 instead:
> >> >> 
> >> >>           options: List[str] = []
> >> >> 
> >> >> I think we should.
> >> >
> >> > This requires Python 3.6, unfortunately. The minimum requirement for
> >> > building QEMU is 3.5.
> >> 
> >> *Sigh*
> >
> > One of the reasons why I would have preferred 3.6 as the minimum, but
> > our policy says that Debian oldstabe is still relevant for another two
> > years. *shrug*
> 
> 3.5 EOL is scheduled for 2020-09-13.
> https://devguide.python.org/#status-of-python-branches
> 
> Whether Debian can support it beyond that date seems doubtful.

You may doubt the quality of their support, but I think it's even more
doubtful that they'll do a version upgrade in oldstable.

> For another reason to want 3.6, see
> [PATCH] qapi: Fix code generation with Python 3.5
> Message-Id: <20200116202558.31473-1-armbru@redhat.com>

The release notes for 3.6 call this an implementation detail that you
shouldn't rely on. However, 3.7 guarantees the order, so I guess we can
effectively rely on it starting from 3.6.

Kevin

Patch
diff mbox series

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 9abf175fe0..1a850fe171 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -147,6 +147,7 @@ 
   'returns': 'UserDefTwo' }
 
 { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
 
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 45c93a43cc..753f6711d3 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -457,6 +457,7 @@  Syntax:
                 '*gen': false,
                 '*allow-oob': true,
                 '*allow-preconfig': true,
+                '*coroutine': true,
                 '*if': COND,
                 '*features': FEATURES }
 
@@ -581,6 +582,9 @@  before the machine is built.  It defaults to false.  For example:
 QMP is available before the machine is built only when QEMU was
 started with --preconfig.
 
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine. It defaults to false.
+
 The optional 'if' member specifies a conditional.  See "Configuring
 the schema" below for more on this.
 
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9aa426a398..d6ce9efc8e 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -24,6 +24,7 @@  typedef enum QmpCommandOptions
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
     QCO_ALLOW_PRECONFIG       =  (1U << 2),
+    QCO_COROUTINE             =  (1U << 3),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 27b0afe55a..e2f71e42a1 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -34,6 +34,10 @@  void qmp_cmd_success_response(Error **errp)
 {
 }
 
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index ab98e504f3..97e51401f1 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -15,6 +15,7 @@  See the COPYING file in the top-level directory.
 
 from qapi.common import *
 from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
+from typing import List
 
 
 def gen_command_decl(name, arg_type, boxed, ret_type):
@@ -194,8 +195,9 @@  out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
-    options = []
+def gen_register_command(name: str, success_response: bool, allow_oob: bool,
+                         allow_preconfig: bool, coroutine: bool) -> str:
+    options = [] # type: List[str]
 
     if not success_response:
         options += ['QCO_NO_SUCCESS_RESP']
@@ -203,18 +205,20 @@  def gen_register_command(name, success_response, allow_oob, allow_preconfig):
         options += ['QCO_ALLOW_OOB']
     if allow_preconfig:
         options += ['QCO_ALLOW_PRECONFIG']
+    if coroutine:
+        options += ['QCO_COROUTINE']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
 
-    options = " | ".join(options)
+    options_str = " | ".join(options)
 
     ret = mcgen('''
     qmp_register_command(cmds, "%(name)s",
                          qmp_marshal_%(c_name)s, %(opts)s);
 ''',
                 name=name, c_name=c_name(name),
-                opts=options)
+                opts=options_str)
     return ret
 
 
@@ -278,7 +282,7 @@  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      coroutine, features):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -296,7 +300,8 @@  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
             self._genh.add(gen_marshal_decl(name))
             self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
             self._regy.add(gen_register_command(name, success_response,
-                                                allow_oob, allow_preconfig))
+                                                allow_oob, allow_preconfig,
+                                                coroutine))
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 6f1c17f71f..8b6978c81e 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -265,7 +265,7 @@  class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      coroutine, features):
         doc = self.cur_doc
         self._gen.add(texi_msg('Command', doc, ifcond,
                                texi_arguments(doc,
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index d7a289eded..9dbfa3edf0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -89,7 +89,7 @@  def check_flags(expr, info):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
                 info, "flag '%s' may only use false value" % key)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
@@ -344,7 +344,7 @@  def check_exprs(exprs):
                        ['command'],
                        ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig'])
+                        'allow-preconfig', 'coroutine'])
             normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b3a463dd8b..8a296a69d6 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -212,7 +212,7 @@  const QLitObject %(c_name)s = %(c_string)s;
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      coroutine, features):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cf0045f34e..753bf233a6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -128,7 +128,7 @@  class QAPISchemaVisitor(object):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      coroutine, features):
         pass
 
     def visit_event(self, name, info, ifcond, arg_type, boxed):
@@ -678,7 +678,7 @@  class QAPISchemaCommand(QAPISchemaEntity):
 
     def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
                  gen, success_response, boxed, allow_oob, allow_preconfig,
-                 features):
+                 coroutine, features):
         QAPISchemaEntity.__init__(self, name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -691,6 +691,7 @@  class QAPISchemaCommand(QAPISchemaEntity):
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self.coroutine = coroutine
 
     def check(self, schema):
         QAPISchemaEntity.check(self, schema)
@@ -732,7 +733,7 @@  class QAPISchemaCommand(QAPISchemaEntity):
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
                               self.boxed, self.allow_oob,
-                              self.allow_preconfig,
+                              self.allow_preconfig, self.coroutine,
                               self.features)
 
 
@@ -1014,6 +1015,7 @@  class QAPISchema(object):
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
+        coroutine = expr.get('coroutine', False)
         ifcond = expr.get('if')
         features = expr.get('features', [])
         if isinstance(data, OrderedDict):
@@ -1025,6 +1027,7 @@  class QAPISchema(object):
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
                                            gen, success_response,
                                            boxed, allow_oob, allow_preconfig,
+                                           coroutine,
                                            self._make_features(features, info)))
 
     def _def_event(self, expr, info, doc):
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3660e75a48..51bfe8bfc7 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -217,6 +217,8 @@  command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
     gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-success-response None -> None
     gen=True success_response=False boxed=False oob=False preconfig=False
+command coroutine-cmd None -> None
+    gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index bad14edb47..7a8e65188d 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -70,12 +70,13 @@  class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
                       success_response, boxed, allow_oob, allow_preconfig,
-                      features):
+                      coroutine, features):
         print('command %s %s -> %s'
               % (name, arg_type and arg_type.name,
                  ret_type and ret_type.name))
-        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
-              % (gen, success_response, boxed, allow_oob, allow_preconfig))
+        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
+              % (gen, success_response, boxed, allow_oob, allow_preconfig,
+                 " coroutine=True" if coroutine else ""))
         self._print_if(ifcond)
         self._print_features(features)