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

[Eric Blake]

On 03/22/2018 12:00 AM, Peter Xu wrote:

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

Seems reasonable - conceptually, the error reply comes no sooner than 
pre-patch (the client wouldn't see a parse error message until after all 
earlier successful parsed messages completed), making a parse error a 
sort of synchronization barrier (if the client gets one, then all 
outstanding message prior to the parse error have finished sending 
replies to the client, and nothing after the parse error has been 
processed at the time the error message was sent).

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

Yes, these are the sorts of bug fixes and test suite coverage that we 
want to include during freeze, for better robustness.

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

If a client disconnects before receiving a reply, I'm fine with either 
approach (the command gets discarded from the queue and never run - 
which matches if the client disconnect raced with the server actually 
getting the command, or the command ran but the response is dropped - 
which matches pre-patch behavior), while agreeing that sending the 
response to the next client is probably not a good idea (clients are 
supposed to ignore unknown id's, but we can't guarantee an 'id' 
collision will not confuse the client).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org