Am 17.01.2020 um 11:43 hat Markus Armbruster geschrieben: > 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()?
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: <20200116202558.31473-1-arm...@redhat.com> 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
