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

[Kevin Wolf]

Am 17.01.2020 um 11:43 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <address@hidden> writes:
> >> 
> >> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <address@hidden> writes:
> >> >> > I have no idea if we will eventually get a case where the command 
> >> >> > wants
> >> >> > to behave different between the two modes and actually has use for a
> >> >> > coroutine. I hope not.
> >> >> >
> >> >> > But using two bools rather than a single enum keeps the code simple 
> >> >> > and
> >> >> > leaves us all options open if it turns out that we do have a use case.
> >> >> 
> >> >> I can buy the argument "the two are conceptually orthogonal, although we
> >> >> don't haven't found a use for one of the four cases".
> >> >> 
> >> >> Let's review the four combinations of the two flags once more:
> >> >> 
> >> >> * allow-oob: false, coroutine: false
> >> >> 
> >> >>   Handler runs in main loop, outside coroutine context.  Okay.
> >> >> 
> >> >> * allow-oob: false, coroutine: true
> >> >> 
> >> >>   Handler runs in main loop, in coroutine context.  Okay.
> >> >> 
> >> >> * allow-oob: true, coroutine: false
> >> >> 
> >> >>   Handler may run in main loop or in iothread, outside coroutine
> >> >>   context.  Okay.
> >> >> 
> >> >> * allow-oob: true, coroutine: true
> >> >> 
> >> >>   Handler may run (in main loop, in coroutine context) or (in iothread,
> >> >>   outside coroutine context).  This "in coroutine context only with
> >> >>   execute, not with exec-oob" behavior is a bit surprising.
> >> >> 
> >> >>   We could document it, noting that it may change to always run in
> >> >>   coroutine context.  Or we simply reject this case as "not
> >> >>   implemented".  Since we have no uses, I'm leaning towards reject.  One
> >> >>   fewer case to test then.
> >> >
> >> > What would be the right mode of rejecting it?
> >> >
> >> > I assume we should catch it somewhere in the QAPI generator (where?) and
> >> 
> >> check_flags() in expr.py?
> >
> > Looks like the right place, thanks.
> >
> >> > then just assert in the C code that both flags aren't set at the same
> >> > time?
> >> 
> >> I think you already do, in do_qmp_dispatch():
> >> 
> >>     assert(!(oob && qemu_in_coroutine()));
> >> 
> >> Not sure that's the best spot.  Let's see when I review PATCH 3.
> >
> > This asserts that exec-oob handlers aren't executed in coroutine
> > context. It doesn't assert that the handler doesn't have QCO_COROUTINE
> > and QCO_ALLOW_OOB set at the same time.
> 
> Asserting this explicitly can't hurt.  qmp_register_command()?

Yes, that's where I put it.

> >> >> >> > @@ -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]
> >> >> 
> >> >> One more: this is a PEP 484 type hint.  With Python 3, we can use PEP
> >> >> 526 instead:
> >> >> 
> >> >>           options: List[str] = []
> >> >> 
> >> >> I think we should.
> >> >
> >> > This requires Python 3.6, unfortunately. The minimum requirement for
> >> > building QEMU is 3.5.
> >> 
> >> *Sigh*
> >
> > One of the reasons why I would have preferred 3.6 as the minimum, but
> > our policy says that Debian oldstabe is still relevant for another two
> > years. *shrug*
> 
> 3.5 EOL is scheduled for 2020-09-13.
> https://devguide.python.org/#status-of-python-branches
> 
> Whether Debian can support it beyond that date seems doubtful.

You may doubt the quality of their support, but I think it's even more
doubtful that they'll do a version upgrade in oldstable.

> For another reason to want 3.6, see
> [PATCH] qapi: Fix code generation with Python 3.5
> Message-Id: <address@hidden>

The release notes for 3.6 call this an implementation detail that you
shouldn't rely on. However, 3.7 guarantees the order, so I guess we can
effectively rely on it starting from 3.6.

Kevin