Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Thu, Mar 22, 2018 at 12:32:36AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 21, 2018 at 9:33 PM, Eric Blake <address@hidden> wrote:
> > On 03/21/2018 03:09 PM, Dr. David Alan Gilbert wrote:
> >
> >>>>
> >>>> So the parsing job and the dispatching job is isolated now.  It gives us
> >>>> a chance in following up patches to totally move the parser outside.
> >>>>
> >>>> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> >>>> used for all the monitors.
> >>>>
> >
> >>>> +
> >>>> +    /*
> >>>> +     * If OOB is not enabled on current monitor, we'll emulate the old
> >>>> +     * behavior that we won't process current monitor any more until
> >>>> +     * it is responded.  This helps make sure that as long as OOB is
> >>>> +     * not enabled, the server will never drop any command.
> >>>> +     */
> >>>> +    if (!qmp_oob_enabled(mon)) {
> >>>> +        monitor_suspend(mon);
> >>>> +        req_obj->need_resume = true;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * Put the request to the end of queue so that requests will be
> >>>> +     * handled in time order.  Ownership for req_obj, req, id,
> >>>
> >>>
> >>> I think the order is not respected if subsequent messages have errors
> >>> (in either json parsing, dispatch_check_obj, oob_check). So if I
> >>> enable oob, and queue a few command, then send a bad command/message,
> >>> I won't be able to tell for which command.
> >>
> >>
> >> Doesn't OOB insist on having an ID field with the command?
> >
> >
> > OOB insists on an id field - but there is the situation that SOME errors
> > occur even before the id field has been encountered (for example, if you
> > send non-JSON, the parser gets all confused - it has to emit SOME error, but
> > that error can't refer to an id because it wasn't able to parse one yet).  A
> > well-formed client will never do this, but we MUST be robust even in the
> > face of a malicious client (or even a well-intentioned client but a noisy
> > communication medium that manages to corrupt bytes). I'm not sure if the
> > testsuite adequately covers what happens in this scenario.  Peter?
> 
> I think a solution would be to queue the error reply (the reply only)
> instead of replying directly.

IMHO this should be fine.

Note that to be compatible with existing QMP we'll suspend the monitor
if OOB is not enabled in the session on receiving the first command.
It means even if another faulty command is sent after the good one,
it'll not be read by QMP parser until the first one is fully complete.

Since I'm working on some test patches after all, I'll try to add a
test case for this to see whether Eric would like them too.

> 
> Another problem I see with the current solution is that pending
> commands are not discarded when a new client connects. So I suppose a
> new client can receive replies for commands it didn't make, with id
> namespace that may conflict with its own. breaking ordering etc. A
> possible solution is to mark the pending request to not send the reply
> somehow?

Yeah, to be simpler - maybe we can even drop the commands that have
not yet been dispatched?

I'll treat a command as "complete" only until the client receives a
response, otherwise if a client disconnects before receiving a reply
then I think it's fine the behavior is undefined - IMHO it's fine
either the QMP server executes the command or not (and no matter what,
we drop the responses).  Would that work?

Thanks,

-- 
Peter Xu