Re: [PATCH v7 09/13] qmp: Move dispatcher to a coroutine

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2020 um 17:30 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> 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 <kwolf@redhat.com>
>> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> > ---
[...]
>> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> > index 5677ba92ca..754f7b854c 100644
>> > --- a/qapi/qmp-dispatch.c
>> > +++ b/qapi/qmp-dispatch.c
[...]
>> > @@ -153,12 +181,35 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>> >          qobject_ref(args);
>> >      }
>> >  
>> > +    assert(!(oob && qemu_in_coroutine()));
>> >      assert(monitor_cur() == NULL);
>> > -    monitor_set_cur(qemu_coroutine_self(), cur_mon);
>> > -
>> > -    cmd->fn(args, &ret, &err);
>> > -
>> > -    monitor_set_cur(qemu_coroutine_self(), NULL);
>> > +    if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
>> > +        monitor_set_cur(qemu_coroutine_self(), cur_mon);
>> > +        cmd->fn(args, &ret, &err);
>> > +        monitor_set_cur(qemu_coroutine_self(), NULL);
>> > +    } else {
>> > +        /*
>> > +         * Not being in coroutine context implies that we're handling
>> > +         * an OOB command, which must not have QCO_COROUTINE.
>> > +         *
>> > +         * This implies that we are in coroutine context, but the
>> > +         * command doesn't have QCO_COROUTINE. We must drop out of
>> > +         * coroutine context for this one.
>> > +         */
>> 
>> I had to read this several times to get it.  The first sentence leads me
>> into coroutine context, and then the next sentence tells me the
>> opposite, throwing me into confusion.
>> 
>> Perhaps something like this:
>> 
>>            /*
>>             * Actual context doesn't match the one the command needs.
>>             * Case 1: we are in coroutine context, but command does not
>>             * have QCO_COROUTINE.  We need to drop out of coroutine
>>             * context for executing it.
>>             * Case 2: we are outside coroutine context, but command has
>>             * QCO_COROUTINE.  Can't actually happen, because we get here
>>             * outside coroutine context only when executing a command
>>             * out of band, and OOB commands never have QCO_COROUTINE.
>>             */
>
> Works for me. Can you squash this in while applying?

Sure!