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?
> 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.
>> >> > @@ -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*
>> >> 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.
I'm asking you not to mix documenting existing code with adding a
new feature to it in the same patch.
Hopefully, that won't lead to the documentation getting dropped for
good. That would be sad.
