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

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

Commit Message

Kevin Wolf Jan. 9, 2020, 6:35 p.m. UTC
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
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(-)

Comments

Eric Blake Jan. 10, 2020, 7 p.m. UTC | #1
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/

> 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'?


> +++ 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.

> +++ 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 ;)


> +++ 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.
Kevin Wolf Jan. 13, 2020, 10:46 a.m. UTC | #2
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

Patch
diff mbox series

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 }
 
 # 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..771e736675 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)