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

[Marc-André Lureau]

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.

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?


-- 
Marc-André Lureau