Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

[Marc-André Lureau]

Hi

On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf <address@hidden> wrote:
>
> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
> > Kevin Wolf <address@hidden> writes:
> >
> > > This moves the QMP dispatcher to a coroutine and runs all QMP command
> > > handlers that declare 'coroutine': true in coroutine context so they
> > > can avoid blocking the main loop while doing I/O or waiting for other
> > > events.
> > >
> > > For commands that are not declared safe to run in a coroutine, the
> > > dispatcher drops out of coroutine context by calling the QMP command
> > > handler from a bottom half.
> > >
> > > Signed-off-by: Kevin Wolf <address@hidden>
> >
> > Uh, what about @cur_mon?
> >
> > @cur_mon points to the current monitor while a command executes.
> > Initial value is null.  It is set in three places (not counting unit
> > tests), and all three save, set, do something that may use @cur_mon,
> > restore:
> >
> > * monitor_qmp_dispatch(), for use within qmp_dispatch()
> > * monitor_read(), for use within handle_hmp_command()
> > * qmp_human_monitor_command(), also for use within handle_hmp_command()
> >
> > Therefore, @cur_mon is null unless we're running within qmp_dispatch()
> > or handle_hmp_command().
>
> Can we make it NULL for coroutine-enabled handlers?

fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp
handlers that use monitor_get_fd() usually depend on cur_mon.

Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist.

>
> > Example of use: error_report() & friends print "to current monitor if we
> > have one, else to stderr."  Makes sharing code between HMP and CLI
> > easier.  Uses @cur_mon under the hood.
>
> error_report() eventually prints to stderr both for cur_mon == NULL and
> for QMP monitors. Is there an important difference between both cases?
>
> There is error_vprintf_unless_qmp(), which behaves differently for both
> cases. However, it is only used in VNC code, so that code would have to
> keep coroutines disabled.
>
> Is cur_mon used much in other functions called by QMP handlers?
>
> > @cur_mon is thread-local.
> >
> > I'm afraid we have to save, clear and restore @cur_mon around a yield.
>
> That sounds painful enough that I'd rather avoid it.
>
> Kevin
>


-- 
Marc-André Lureau