Re: [PATCH 1/4] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf <kwolf@redhat.com>]

Am 10.01.2020 um 20:00 hat Eric Blake geschrieben:
> On 1/9/20 12:35 PM, Kevin Wolf wrote:
> > This patch adds a new 'coroutine' flag to QMP command definitions that
> > tells the QMP dispatcher than the command handler is safe to be run in a
> 
> s/than/that/

Thanks. If this remains the only change, I hope Markus can fix it while
applying the patch.

> > coroutine.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@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..55f596dbaa 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 }
> 
> Not user-visible (it's the testsuite), but why not follow our naming
> convention of 'coroutine-cmd'?

I just copied and modified the simplest example from a few lines above:

    { 'command': 'user_def_cmd', 'data': {} }

The command names in the test schema seem to follow no particular style,
there are even some CamelCaseCommands. But if I have to respin, I can
change it.

> > +++ 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
> 
> Maybe "The optional 'coroutine' member tells..." for symmetry with the next
> paragraph.

Starting with 'Member ...' seems to be what is done for most other
options. I phrased it this way specifically for consistency.

> > +++ 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]
> 
> Aha - now that we require python 3, you're going to exploit it ;)

Of course. :-)

I was hoping that I could get the type checker to tell me if I forgot to
change one of the callers, but that doesn't really work until we add
type annotations to the callers as well. I'm going to send a separate
series to do a little more about type checking.

> > +++ 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),
> 
> I'm assuming the new flag is internal only, and doesn't affect behavior seen
> by the user, and thus nothing to change in the introspection output.

Yes. The result would hopefully be visible to the user (the guest
doesn't hang any more where it used to hang), but that's just a bug fix
and nothing that would require a change in the way a client uses QMP.

Kevin