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. > > > Your patch looks quite good to me, but make check hangs. Have you > > looked at it? > > I'm not sure if I was looking at some qemu-iotests cases or make check, > but yes, I did see a hang. My case was a QMP command that just doesn't > work correctly inside a coroutine without modifications, so requiring > 'coroutine': true would fix it. ok, then I suggest you do a "minimal" patch series that works. thanks! -- Marc-André Lureau
