| Message ID | 20200109183545.27452-2-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qmp: Optionally run handlers in coroutines | expand |
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.
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
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)
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(-)