[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands |
Date: |
Thu, 16 Jan 2020 16:02:14 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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
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
- [PATCH v3 0/4] qmp: Optionally run handlers in coroutines, Kevin Wolf, 2020/01/15
- [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/16
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands,
Kevin Wolf <=
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/17
[PATCH v3 2/4] vl: Initialise main loop earlier, Kevin Wolf, 2020/01/15
[PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Kevin Wolf, 2020/01/15