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

[Kevin Wolf]

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?

> 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