Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands

[Markus Armbruster]

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            | 12 ++++++++++++
>  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                  | 12 ++++++++----
>  tests/qapi-schema/test-qapi.py          |  7 ++++---
>  tests/qapi-schema/meson.build           |  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, 54 insertions(+), 14 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 f3e7ced212..36daa9b5f8 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -472,6 +472,7 @@ Syntax:
>                  '*gen': false,
>                  '*allow-oob': true,
>                  '*allow-preconfig': true,
> +                '*coroutine': true,
>                  '*if': COND,
>                  '*features': FEATURES }
>  
> @@ -596,6 +597,17 @@ 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.

We need to document what exactly makes a command handler coroutine safe
/ unsafe.  We discussed this at some length in review of PATCH v4 1/4:

    Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html

I'd be willing to accept a follow-up patch, if that's more convenient
for you.

>                                     It defaults to false.  If it is true,
> +the command handler is called from coroutine context and may yield while

Is it *always* called from coroutine context, or could it be called
outside coroutine context, too?  I guess the former, thanks to PATCH 10,
and as long as we diligently mark HMP commands that such call QMP
handlers, like you do in PATCH 13.

What's the worst than can happen when we neglect to mark such an HMP
command?

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