Am 13.01.2020 um 16:58 hat Markus Armbruster geschrieben: > Marc-André Lureau <marcandre.lur...@gmail.com> writes: > > > Hi > > > > On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kw...@redhat.com> 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"?
Yes, that's the one. Kevin
