| Message ID | 20200218154036.28562-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qmp: Optionally run handlers in coroutines | expand |
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. > > The documentation of the new flag pretends that this flag is already > used as intended, which it isn't yet after this patch. We'll implement > this in another patch in this series. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > --- > docs/devel/qapi-code-gen.txt | 11 +++++++++++ > include/qapi/qmp/dispatch.h | 1 + > tests/test-qmp-cmds.c | 4 ++++ > scripts/qapi/commands.py | 10 +++++++--- > scripts/qapi/doc.py | 2 +- > scripts/qapi/expr.py | 10 ++++++++-- > scripts/qapi/introspect.py | 2 +- > scripts/qapi/schema.py | 9 ++++++--- > tests/qapi-schema/test-qapi.py | 7 ++++--- > tests/Makefile.include | 1 + > tests/qapi-schema/oob-coroutine.err | 2 ++ > tests/qapi-schema/oob-coroutine.json | 2 ++ > tests/qapi-schema/oob-coroutine.out | 0 > tests/qapi-schema/qapi-schema-test.json | 1 + > tests/qapi-schema/qapi-schema-test.out | 2 ++ > 15 files changed, 51 insertions(+), 13 deletions(-) > create mode 100644 tests/qapi-schema/oob-coroutine.err > create mode 100644 tests/qapi-schema/oob-coroutine.json > create mode 100644 tests/qapi-schema/oob-coroutine.out > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index 59d6973e1e..df01bd852b 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,16 @@ 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. If it is true, > +the command handler is called from coroutine context and may yield while > +waiting for an external event (such as I/O completion) in order to avoid > +blocking the guest and other background operations. > + > +It is an error to specify both 'coroutine': true and 'allow-oob': true > +for a command. (We don't currently have a use case for both together and > +without a use case, it's not entirely clear what the semantics should be.) The parenthesis is new since v4. I like its contents. I'm not fond of putting complete sentences in parenthesis. I'll drop them, if you don't mind. > + > The optional 'if' member specifies a conditional. See "Configuring > the schema" below for more on this. > [...] > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index d7a289eded..95cc006cae 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -89,10 +89,16 @@ 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) > + if 'allow-oob' in expr and 'coroutine' in expr: > + # This is not necessarily a fundamental incompatibility, but we don't > + # have a use case and the desired semantics isn't obvious. The simplest > + # solution is to forbid it until we get a use case for it. Appreciated. I'll word-wrap, if you don't mind. > + raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' " > + "are incompatible") > > > def check_if(expr, info, source): > @@ -344,7 +350,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': [...] R-by stands.
Am 03.03.2020 um 09:10 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. > > > > The documentation of the new flag pretends that this flag is already > > used as intended, which it isn't yet after this patch. We'll implement > > this in another patch in this series. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > --- > > docs/devel/qapi-code-gen.txt | 11 +++++++++++ > > include/qapi/qmp/dispatch.h | 1 + > > tests/test-qmp-cmds.c | 4 ++++ > > scripts/qapi/commands.py | 10 +++++++--- > > scripts/qapi/doc.py | 2 +- > > scripts/qapi/expr.py | 10 ++++++++-- > > scripts/qapi/introspect.py | 2 +- > > scripts/qapi/schema.py | 9 ++++++--- > > tests/qapi-schema/test-qapi.py | 7 ++++--- > > tests/Makefile.include | 1 + > > tests/qapi-schema/oob-coroutine.err | 2 ++ > > tests/qapi-schema/oob-coroutine.json | 2 ++ > > tests/qapi-schema/oob-coroutine.out | 0 > > tests/qapi-schema/qapi-schema-test.json | 1 + > > tests/qapi-schema/qapi-schema-test.out | 2 ++ > > 15 files changed, 51 insertions(+), 13 deletions(-) > > create mode 100644 tests/qapi-schema/oob-coroutine.err > > create mode 100644 tests/qapi-schema/oob-coroutine.json > > create mode 100644 tests/qapi-schema/oob-coroutine.out > > > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > > index 59d6973e1e..df01bd852b 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,16 @@ 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. If it is true, > > +the command handler is called from coroutine context and may yield while > > +waiting for an external event (such as I/O completion) in order to avoid > > +blocking the guest and other background operations. > > + > > +It is an error to specify both 'coroutine': true and 'allow-oob': true > > +for a command. (We don't currently have a use case for both together and > > +without a use case, it's not entirely clear what the semantics should be.) > > The parenthesis is new since v4. I like its contents. I'm not fond of > putting complete sentences in parenthesis. I'll drop them, if you don't > mind. You asked this already in the discussion for v4. Yes, I still agree. > > + > > The optional 'if' member specifies a conditional. See "Configuring > > the schema" below for more on this. > > > [...] > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index d7a289eded..95cc006cae 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -89,10 +89,16 @@ 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) > > + if 'allow-oob' in expr and 'coroutine' in expr: > > + # This is not necessarily a fundamental incompatibility, but we don't > > + # have a use case and the desired semantics isn't obvious. The simplest > > + # solution is to forbid it until we get a use case for it. > > Appreciated. I'll word-wrap, if you don't mind. I still don't agree with comment line width being smaller than code line width, and think it's in disagreement with CODING_STYLE, but if you can't help it, adjust the formatting however you like. > > + raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' " > > + "are incompatible") > > > > > > def check_if(expr, info, source): > > @@ -344,7 +350,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': > [...] > > R-by stands. Kevin
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 59d6973e1e..df01bd852b 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,16 @@ 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. If it is true, +the command handler is called from coroutine context and may yield while +waiting for an external event (such as I/O completion) in order to avoid +blocking the guest and other background operations. + +It is an error to specify both 'coroutine': true and 'allow-oob': true +for a command. (We don't currently have a use case for both together and +without a use case, it's not entirely clear what the semantics should be.) + 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 79507d9e54..6359cc28c7 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -35,6 +35,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 afa55b055c..f2f2f8948d 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -194,7 +194,8 @@ out: return ret -def gen_register_command(name, success_response, allow_oob, allow_preconfig): +def gen_register_command(name, success_response, allow_oob, allow_preconfig, + coroutine): options = [] if not success_response: @@ -203,6 +204,8 @@ 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'] @@ -285,7 +288,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 @@ -303,7 +306,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 1787a53d91..69f971e427 100644 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -264,7 +264,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..95cc006cae 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -89,10 +89,16 @@ 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) + if 'allow-oob' in expr and 'coroutine' in expr: + # This is not necessarily a fundamental incompatibility, but we don't + # have a use case and the desired semantics isn't obvious. The simplest + # solution is to forbid it until we get a use case for it. + raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' " + "are incompatible") def check_if(expr, info, source): @@ -344,7 +350,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 5100110fa2..8fbfa42e8e 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): @@ -690,7 +690,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) @@ -703,6 +703,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) @@ -745,7 +746,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) @@ -1043,6 +1044,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): @@ -1054,6 +1056,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/test-qapi.py b/tests/qapi-schema/test-qapi.py index 41232c11a3..dcb922755d 100755 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -69,12 +69,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) diff --git a/tests/Makefile.include b/tests/Makefile.include index 2f1cafed72..893540f5a3 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -286,6 +286,7 @@ qapi-schema += missing-type.json qapi-schema += nested-struct-data.json qapi-schema += nested-struct-data-invalid-dict.json qapi-schema += non-objects.json +qapi-schema += oob-coroutine.json qapi-schema += oob-test.json qapi-schema += allow-preconfig-test.json qapi-schema += pragma-doc-required-crap.json diff --git a/tests/qapi-schema/oob-coroutine.err b/tests/qapi-schema/oob-coroutine.err new file mode 100644 index 0000000000..c01a4992bd --- /dev/null +++ b/tests/qapi-schema/oob-coroutine.err @@ -0,0 +1,2 @@ +oob-coroutine.json: In command 'oob-command-1': +oob-coroutine.json:2: flags 'allow-oob' and 'coroutine' are incompatible diff --git a/tests/qapi-schema/oob-coroutine.json b/tests/qapi-schema/oob-coroutine.json new file mode 100644 index 0000000000..0f67663bcd --- /dev/null +++ b/tests/qapi-schema/oob-coroutine.json @@ -0,0 +1,2 @@ +# Check that incompatible flags allow-oob and coroutine are rejected +{ 'command': 'oob-command-1', 'allow-oob': true, 'coroutine': true } diff --git a/tests/qapi-schema/oob-coroutine.out b/tests/qapi-schema/oob-coroutine.out new file mode 100644 index 0000000000..e69de29bb2 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/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 9bd3c4a490..fdc349991a 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -203,6 +203,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