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 then just assert in the C code that both flags aren't set at the same time? > >> > @@ -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. > >> Some extra churn due to type hints here. Distracting. Suggest not to > >> mix adding type hints to existing code with feature work. > > > > If you would be open for a compromise, I could leave options > > unannotated, but keep the typed parameter list. > > Keeping just the function annotation is much less distracting. I can't > reject that with a "separate patches for separate things" argument. > > I'd still prefer not to, because: > > * If we do add systematic type hints in the near future, then delaying > this one until then shouldn't hurt your productivity. > > * If we don't, this lone one won't help your productivity much, but > it'll look out of place. > > I really don't want us to add type hints as we go, because such > open-ended "while we touch it anyway" conversions take forever and a > day. Maximizes the confusion integral over time. I think it's a first time that I'm asked not to document things, but I'll remove them. Kevin
