Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Peter Xu]

On Thu, Jan 11, 2018 at 05:07:11PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > There was no QMP capabilities defined.  Define the first "oob" as
> 
> s/was/were/

Fixed.

Just to confirm: is "There was no QMP capability" also correct?

> 
> > capability to allow out-of-band messages.
> > 
> > Also, touch up qmp-test.c to test the new bits.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  monitor.c        | 10 ++++++++--
> >  qapi-schema.json | 13 +++++++++++++
> >  tests/qmp-test.c | 10 +++++++++-
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> I'm assuming later patches will document this?
> 
> I'm somewhat a fan of documentation alongside or before implementation,
> as getting the general overview right and then checking that the code
> matches is a bit nicer than random coding then documenting what we ended
> up with.  But I don't know if reordering patches in your series is
> necessary, as long as the end product is properly documented.

Yes, I put the whole document at the end of the series.  I can put it
as the first patch too.

> 
> > +++ b/qapi-schema.json
> > @@ -118,6 +118,19 @@
> >  ##
> >  { 'command': 'qmp_capabilities' }
> 
> The client can't request a particular feature alongside the command?  Or
> is that in later patches?  With just this patch, the enum QMPCapability
> is not introspected, because it is not referenced by any command
> (although introspection is a bit moot, since the client will learn what
> the host advertises from the initial handshake before the client can
> even request introspection).

Yes, client can't if without further patches.

> 
> >  
> > +##
> > +# @QMPCapability:
> > +#
> > +# QMP supported capabilities to be broadcasted to the clients.
> 
> 'broadcast' is one of those weird verbs that doesn't change spelling
> when constructing its past tense (there is no 'broadcasted').  However,
> I think this description is a bit nicer (and avoids the problematic word
> altogether):
> 
> Enumeration of capabilities to be advertised during initial client
> connection, used for agreeing on particular QMP extension behaviors.

I'll take your advise.

> 
> > +#
> > +# @oob:   QMP ability to support Out-Of-Band requests.
> 
> Rather terse (it doesn't say what Out-Of-Band requests are); even a
> pointer to the QMP spec (where OOB is more fully documented) might be
> nice (of course, that means we need a patch to docs/interop/qmp-spec.txt
> somewhere in the series, especially since this patch renders 2.2.1 in
> that document obsolete...)

Sorry for the inconvenience.  Please refer to the last doc patch for
details.

I thought the doc patch would explain itself but obviously I should be
more careful on the ordering next time to ease reviewers.  I'll move
the doc patch to front when repost, and I'll note here to refer to
qmp-spec.txt for more information.

Thanks,

-- 
Peter Xu