Re: [PATCH v5 1/4] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf]

Am 03.03.2020 um 09:10 hat Markus Armbruster geschrieben:
> 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>
> > Reviewed-by: Markus Armbruster <address@hidden>
> > ---
> >  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