Hi
On Wed, Mar 21, 2018 at 9:33 PM, Eric Blake <ebl...@redhat.com> 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
