Re: [Qemu-devel] [PATCH v8 01/23] docs: update QMP documents for OOB commands

[Peter Xu <peterx@redhat.com>]

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 <peterx@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 <eblake@redhat.com>

(I agree on all the rest of the review comments too)

Thanks!

-- 
Peter Xu