[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: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands |
Date: |
Fri, 17 Jan 2020 11:43:35 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>>
>> > 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
>>
>> 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: <address@hidden>
- [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, 2020/01/16
- 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 <=
- 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