[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability |
Date: |
Fri, 12 Jan 2018 12:28:09 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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