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

[Eric Blake]

On 01/11/2018 10:28 PM, Peter Xu wrote:
> 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?

Yes, that also matches singular vs. plural between the verb and object.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

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 
> > ---
> >  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

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

[Eric Blake]

On 12/19/2017 02:45 AM, Peter Xu wrote:
> There was no QMP capabilities defined.  Define the first "oob" as

s/was/were/

> capability to allow out-of-band messages.
> 
> Also, touch up qmp-test.c to test the new bits.
> 
> Signed-off-by: Peter Xu 
> ---
>  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.

> +++ 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).

>  
> +##
> +# @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.

> +#
> +# @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...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Stefan Hajnoczi]

On Tue, Dec 19, 2017 at 04:45:41PM +0800, Peter Xu wrote:
> There was no QMP capabilities defined.  Define the first "oob" as
> capability to allow out-of-band messages.
> 
> Also, touch up qmp-test.c to test the new bits.
> 
> Signed-off-by: Peter Xu 
> ---
>  monitor.c| 10 --
>  qapi-schema.json | 13 +
>  tests/qmp-test.c | 10 +-
>  3 files changed, 30 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

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

[Peter Xu]

On Thu, Dec 21, 2017 at 05:56:37PM +0800, Fam Zheng wrote:
> On Tue, 12/19 16:45, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index 4b2bee773f..81fb0a42b4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
> >  
> >  static QObject *get_qmp_greeting(void)
> >  {
> > +QList *cap_list = qlist_new();
> >  QObject *ver = NULL;
> > +QMPCapability cap;
> >  
> >  qmp_marshal_query_version(NULL, , NULL);
> >  
> > -return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': 
> > []}}",
> > -  ver);
> > +for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > +qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
> 
> Did we want to not include "oob" if current monitor doesn't have use_io_thr?

Yes.  As you have already noticed, it's in next patch. :)

Thanks,

-- 
Peter Xu

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

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index 4b2bee773f..81fb0a42b4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
>  
>  static QObject *get_qmp_greeting(void)
>  {
> +QList *cap_list = qlist_new();
>  QObject *ver = NULL;
> +QMPCapability cap;
>  
>  qmp_marshal_query_version(NULL, , NULL);
>  
> -return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
> -  ver);
> +for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> +qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));

Did we want to not include "oob" if current monitor doesn't have use_io_thr?

Fam

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

[Peter Xu]

There was no QMP capabilities defined.  Define the first "oob" as
capability to allow out-of-band messages.

Also, touch up qmp-test.c to test the new bits.

Signed-off-by: Peter Xu 
---
 monitor.c| 10 --
 qapi-schema.json | 13 +
 tests/qmp-test.c | 10 +-
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4b2bee773f..81fb0a42b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+QList *cap_list = qlist_new();
 QObject *ver = NULL;
+QMPCapability cap;
 
 qmp_marshal_query_version(NULL, , NULL);
 
-return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
-  ver);
+for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
+}
+
+return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
+  ver, cap_list);
 }
 
 static void monitor_qmp_event(void *opaque, int event)
diff --git a/qapi-schema.json b/qapi-schema.json
index 18457954a8..03201578b4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -118,6 +118,19 @@
 ##
 { 'command': 'qmp_capabilities' }
 
+##
+# @QMPCapability:
+#
+# QMP supported capabilities to be broadcasted to the clients.
+#
+# @oob:   QMP ability to support Out-Of-Band requests.
+#
+# Since: 2.12
+#
+##
+{ 'enum': 'QMPCapability',
+  'data': [ 'oob' ] }
+
 ##
 # @VersionTriple:
 #
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index c5a5c10b41..292c5f135a 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -17,6 +17,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/util.h"
 #include "qapi/visitor.h"
+#include "qapi/qmp/qstring.h"
 
 const char common_args[] = "-nodefaults -machine none";
 
@@ -75,6 +76,8 @@ static void test_qmp_protocol(void)
 {
 QDict *resp, *q, *ret;
 QList *capabilities;
+const QListEntry *entry;
+QString *qstr;
 
 global_qtest = qtest_init_without_qmp_handshake(common_args);
 
@@ -84,7 +87,12 @@ static void test_qmp_protocol(void)
 g_assert(q);
 test_version(qdict_get(q, "version"));
 capabilities = qdict_get_qlist(q, "capabilities");
-g_assert(capabilities && qlist_empty(capabilities));
+g_assert(capabilities);
+entry = qlist_first(capabilities);
+g_assert(entry);
+qstr = qobject_to_qstring(entry->value);
+g_assert(qstr);
+g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");
 QDECREF(resp);
 
 /* Test valid command before handshake */
-- 
2.14.3