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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

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

Sets up and arms a bear trap, I'm afraid.

We can rely on code review to ensure a handler we want to
coroutine-enable does not access @cur_mon.  But how can we ensure no
uses creep back?  Not even on error paths, where testing tends to be
ineffective?

@cur_mon goes back to Jan's commit 376253ece48 (March 2009).  Meant to
be merely a stop gap:

    For the case that monitor outputs so far happen without clearly
    identifiable context, the global variable cur_mon is introduced that
    shall once provide a pointer either to the current active monitor (while
    processing commands) or to the default one. On the mid or long term,
    those use case will be obsoleted so that this variable can be removed
    again.

It's been eleven years, and we haven't really gotten closer to getting
them "obsoleted".  I can't stop you if you want to try.  Myself, I'd
rather give up and eliminate the Monitor * parameter passing.

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

This goes back to

commit 4ad417baa43424b6b988c52b83989fd95670c113
Author: Cole Robinson <address@hidden>
Date:   Fri Mar 21 19:42:24 2014 -0400

    error: Print error_report() to stderr if using qmp

    monitor_printf will drop the requested output if cur_mon is qmp (for
    good reason). However these messages are often helpful for debugging
    issues with via libvirt.

    If we know the message won't hit the monitor, send it to stderr.

    Cc: Luiz Capitulino <address@hidden>
    Cc: Markus Armbruster <address@hidden>
    Signed-off-by: Cole Robinson <address@hidden>
    Reviewed-by: Paolo Bonzini <address@hidden>
    Signed-off-by: Luiz Capitulino <address@hidden>

Use of error_report() in QMP context is wrong more often than not.
Still, spewing the message to stderr is less bad than throwing it away.

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

It has seen other users over the years.

error_printf_unless_qmp() is a convenient way to print hints for humans.
In HMP context, you reach the human by printing to @cur_mon.  Outside
monitor context, you reach the human by printing to stdout or stderr.
In QMP context, we're talking to a machine.

> Is cur_mon used much in other functions called by QMP handlers?

Marc-André pointed to the file-descriptor passing stuff.

To be sure that's all, we'd have to review all uses of @cur_mon.

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

I've seen coroutine implementations for C that provide coroutine-local
variables of sorts by managing a thread-local opaque pointer.