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

[Kevin Wolf]

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?

> > 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'll defer to Markus on this one.

> 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.

Kevin