Kevin Wolf <kw...@redhat.com> writes: > Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kw...@redhat.com> 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()? >> >> >> > @@ -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. For another reason to want 3.6, see [PATCH] qapi: Fix code generation with Python 3.5 Message-Id: <20200116202558.31473-1-arm...@redhat.com>
