Re: [Qemu-devel] your mail

[Peter Xu]

On Thu, Jun 28, 2018 at 01:00:44PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 28, 2018 at 10:31:42AM +0200, Markus Armbruster wrote:
> > Peter Xu <address@hidden> writes:
> > 
> > > On Tue, Jun 26, 2018 at 07:21:49PM +0200, Markus Armbruster wrote:
> > >> I fooled around a bit, and I think there are a few lose ends.
> > [...]
> > >> Talking to a QMP monitor that supports OOB:
> > >> 
> > >>     $ socat UNIX:test-qmp 
> > >> READLINE,history=$HOME/.qmp_history,prompt='QMP> '
> > >>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, 
> > >> "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
> > >>     QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
> > >>     {"error": {"class": "GenericError", "desc": "Parameter 'oob' is 
> > >> unexpected"}}
> > >>     QMP> { "execute": "qmp_capabilities", "arguments": { "enable": 
> > >> ["oob"] } }
> > >>     {"return": {}}
> > >>     QMP> { "execute": "query-qmp-schema" }
> > >>     {"error": {"class": "GenericError", "desc": "Out-Of-Band capability 
> > >> requires that every command contains an 'id' field"}}
> > >> 
> > >> Why does every command require 'id'?
> > 
> > Why am I asking?  Requiring 'id' is rather onerous when you play with
> > QMP by hand.
> > 
> > > The COMMAND_DROPPED event is one reason, as you mentioned in the other
> > > thread. Meanwhile as long as we have OOB command it means that we can
> > > have more than one commands running in parallel, then it's a must that
> > > we can mark that out in the response to mark out which command we are
> > > replying to.
> > 
> > Without OOB, the client can match response to command, because it'll
> > receive responses in command issue order.
> > 
> > Example 1: after sending
> > 
> >     { "execute": "cmd1", ... }
> >     { "execute": "cmd2", ... }
> >     { "execute": "cmd3", ... }
> > 
> > the client will receive three responses, first one is cmd1's, second one
> > is cmd2's, third one is cmd3's.
> > 
> > With OOB, we have to independent command streams, in-band and
> > out-of-band.  Each separate stream's responses arrive in-order, but the
> > two streams may be interleaved.
> > 
> > Example 2: after sending
> > 
> >     { "execute": "cmd1", ... }
> >     { "execute": "cmd2", ... }
> >     { "execute": "cmd3", ... }
> >     { "execute": "oob1", "control": { "run-oob": true }, ... }
> >     { "execute": "oob2", "control": { "run-oob": true }, ... }
> >     { "execute": "oob3", "control": { "run-oob": true }, ... }
> > 
> > the client will receive six responses: cmd1's before cmd2's before
> > cmd3's, and oob1's before oob2's before oob3's.
> 
> I thought the intention was that oob1, oob2, and oob3 are defined
> to return in any order. It just happens that we only hve a single
> oob processing stream right now, but we may have more in future.

Exactly.

> 
> > If the client sends "id" with each command, it can match responses to
> > commands by id.
> > 
> > But that's not the only way to match.  Imagine the client had a perfect
> > oracle to tell it whether a response is in-band or out-of-band.  Then
> > matching can work pretty much as in example 1: the first in-band
> > response is cmd1's, the second one is cmd2's, and the third one is
> > cmd3's; the first out-of-band response is oob1's, the second one is
> > oob2's and the third one is oob3's.
> 
> Not if oob1/2/3 can retunr in any order in future, which I think we
> should be allowing fore.
> 
> IMHO 'id' should be mandatory for oob usage.
> 
> > Possible solutions other than requiring "id":
> > 
> > 1. Make out-of-band responses recognizable
> > 
> >    Just like we copy "id" to the response, we could copy "run-oob", or
> >    maybe "control".
> > 
> >    Solves the "match response to command" problem, doesn't solve the
> >    "match COMMAND_DROPPED event to command" problem.
> > 
> >    Permit me a digression.  "control": { "run-oob": true } is awfully
> >    verbose.  Back when we hashed out the OOB design, I proposed to use
> >    "exec-oob" instead of "execute" to run a command OOB.  I missed when
> >    that morphed into flag "run-oob" wrapped in object "control".  Was it
> >    in response to reviewer feedback?  Can you give me a pointer?

It's me who proposed this, not from any reviewer's feedback.  It just
happened that no one was against it.

> > 
> >    The counterpart to "exec-oob" would be "return-oob" and "error-oob".
> >    Hmm.
> 
> I do prefer the less verbose proposal too.

But frankly speaking I still prefer current way.  QMP is majorly for
clients (computer programs) rather than users to use it.  Comparing to
verbosity, I would care more about coherency for how we define the
interface.  Say, OOB execution is still one way to execute a command,
so naturally it should still be using the same way that we execute a
QMP command, we just need some extra fields to tell the server that
"this message is more urgent than the other ones".

If we are discussing HMP interfaces, I'll have the same preference
with both of you to consider more about whether it's user-friendly or
not.  But now we are talking about QMP, then I'll prefer "control".

In the future, it's also possible to add some more sub-fields into the
"control" field.  When that happens, do we want to introduce another
standalone wording for that?  I would assume the answer is no.

> 
> > 2. Do nothing; it's the client's problem
> > 
> >    If the client needs to match responses and events to commands, it
> >    should use the feature that was expressly designed for helping with
> >    that: "id".
> > 
> >    Note that QMP's initial design assumed asynchronous commands,
> >    i.e. respones that may arrive in any order.  Nevertheless, it did not
> >    require "id".  Same reasoning: if the client needs to match, it can
> >    use "id".
> 
> IMHO we should just mandate 'id' when using "exec-oob", as it is
> conceptually broken to use OOB without a way to match responses.
> We don't want clients relying on all OOB replies coming in sequence
> now, and then breaking when we allow multiple OOB processing threads.
> That would require us to add a eec-oob-no-really-i-mean-it command
> to allow overlapping OOB responses later.

Agreed, again.

Thanks,

-- 
Peter Xu