Re: [PATCH v6 00/25] monitor: add asynchronous command type

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <address@hidden> wrote:
>>
>> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben:
>> > > What my patch does is moving everything into a coroutine. This is wrong
>> > > because not everything can be run in a coroutine, so it needs to be made
>> > > optional (like you did with your async flag).
>> >
>> > "everything" is a bit too much ;) You proposal is to replace
>> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This
>> > is nice because indeed, it allows to reenter the mainloop with a
>> > simple yield in QMP commands. It is also simpler than my "async"
>> > proposal, because some of the state is part of the coroutine, and
>> > because it doesn't allow QMP commands concurrency (beside existing
>> > OOB).
>> >
>> > Iow, coroutine (for async) + oob (for concurrency) make my proposal
>> > kinda obsolete. I can only regret that a simple callback-based
>> > solution looked simpler to me than one that mixes both threads &
>> > coroutines, but I don't mind if everybody likes it better :) I can
>> > definitely see the point for block commands, which rely on coroutines
>> > anyway, and qemu is already that complex in general.
>>
>> Callbacks are indeed simple enough for implementing the infrastructure,
>> but for the users they only look simple as long as they do trivial
>> things. :-)
>>
>> Anyway, now that you have seen my POC hack, do you agree that this
>> should help solving the screendump problem, too?
>
> Yes, and I will work on it as soon as you have a working patch series
> or branch :)
>
>>
>> > > The problem isn't with completely coroutine-unaware code, though: That
>> > > one would just work, even if not taking advantage from the coroutine. A
>> > > potential problem exists with code that behaves differently when run in
>> > > a coroutine or outside of coroutine context (generally by checking
>> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such
>> > > functions.
>> > >
>> > > Running some command handlers outside of coroutine context wouldn't be
>> > > hard to add to my patch (basically just a BH), but I haven't looked into
>> > > the QAPI side of making it an option.
>> >
>> > Yes, I think we should have a 'coroutine': true, for commands that
>> > should be run with a coroutine.
>> >
>> > Or perhaps replace existing allow-oob with 'dispatch':
>> > - 'bh' (default)
>> > - 'coroutine'
>> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at
>> > this point)
>>
>> If it's "at this point", then making it two separate bools would make
>> more sense. But I seem to remember that OOB handlers are fundamentally
>> not supposed to block, so coroutine support would be pointless for them
>> and an enum could work.
>
> I think so too
>
>>
>> I'll defer to Markus on this one.
>
> Yup, Markus should take a look at your proposal and give some
> guidance. And hopefully, it won't take >2y.

Is it "[PATCH 0/4] qmp: Optionally run handlers in coroutines"?

[...]