[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands |
Date: |
Mon, 17 Feb 2020 08:08:20 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Kevin Wolf <address@hidden> 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 <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
> ---
> docs/devel/qapi-code-gen.txt | 10 ++++++++++
> 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 | 7 +++++--
> 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, 47 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..9b65cd3ab3 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,15 @@ 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.
> +
> 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 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..769c54ebfe 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -89,10 +89,13 @@ 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:
> + raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> + "are incompatible")
Only because we didn't implement coroutine context for exec-oob, for
want of a use case. I think we should note that somehow, to remind our
future selves that the two aren't actually fundamentally incompatible.
Could put the reminder into the error message, like "'coroutine': true
isn't implement with 'allow-oob': true". A comment would do as well.
A similar reminder in qapi-code-gen.txt wouldn't hurt.
>
>
> def check_if(expr, info, source):
> @@ -344,7 +347,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 0bfc5256fb..87d76b59d3 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 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)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index c6827ce8c2..d111389c03 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
You remembered to cover the error case. Appreciated!
> 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
Preferably with the "not implemented" status clarified:
Reviewed-by: Markus Armbruster <address@hidden>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands,
Markus Armbruster <=