On Fri, Mar 09, 2018 at 11:13:23AM -0600, Eric Blake wrote:
> On 03/09/2018 02:59 AM, Peter Xu wrote:
> > Update both the developer and spec for the new QMP OOB (Out-Of-Band)
> > command.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > docs/devel/qapi-code-gen.txt | 65
> > ++++++++++++++++++++++++++++++++++++++++----
> > docs/interop/qmp-spec.txt | 30 +++++++++++++++++---
> > 2 files changed, 86 insertions(+), 9 deletions(-)
>
> If all goes well, I'm planning on taking this series through my QAPI tree,
> with a pull request either late today or early Monday in order to make the
> soft freeze deadline. We'll see what my review turns up, but hopefully at
> this point it's either clean or minor enough tweaks that I can polish it
> without a v9.
That'll be great. Thanks Eric!
[...]
> > @@ -102,10 +113,16 @@ The format for command execution is:
> > required. Each command documents what contents will be considered
> > valid when handling the json-argument
> > - The "id" member is a transaction identification associated with the
> > - command execution, it is optional and will be part of the response if
> > + command execution. It is required if OOB is enabled, and optional
> > + if not. The same "id" field will be part of the response if
>
> Ambiguous on whether this is the per-command 'run-oob', or whether this is
> the generic QMP capability requested during qmp_capabilities. I think the
> intent is that if you enabled the QMP capability up front, ALL commands must
> have an "id", even if they are run in-band without 'run-oob', because the
> 'id' in the response is what will distinguish whether a later OOB request
> overtook a pending in-band reply.
Yes, maybe I should use "OOB capability" to be explicit - exactly as
you explained.
>
> > provided. The "id" member can be any json-value, although most
> > clients merely use a json-number incremented for each successive
> > command
> > +- The "control" member is optional, and currently only used for
> > + "out-of-band" execution ("oob" as shortcut). The handling or
>
> A bit late to be introducing "oob" as shortcut, given that you've already
> used the abbreviation oob earlier in the document.
>
> > + response of an "oob" command can overtake prior in-band commands.
> > + To enable "oob" handling of a particular command, just provide a
> > + control field with: { "control": { "run-oob": true } }
> > 2.4 Commands Responses
> > ----------------------
> > @@ -113,6 +130,11 @@ The format for command execution is:
> > There are two possible responses which the Server will issue as the result
> > of a command execution: success or error.
> > +As long as the commands were issued with a proper "id" field, then the
> > +same "id" field will be attached in the corresponding response message
> > +so that requests and responses can match. Clients should drop all the
> > +responses that are with unknown "id" field.
>
> s/are with/have an/
>
> I've got quite a few wording tweaks, but the general concept is good. If you
> need to respin for other reasons, feel free to make those improvements, but
> I also don't mind making tweaks myself as part of queuing for a pull
> request, so:
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
(I agree on all the rest of the review comments too)
Thanks!
--
Peter Xu
