[Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Paolo Bonzini]

I noticed that the QMP input visitor does not detect extra members inside
structs.  The outermost arguments struct is handled by the QMP type
checker, but the nested ones go undetected.  That could be a problem
for complex commands such as "transaction".

This patch adds such detection to the QMP input visitor.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Is this acceptable or just wrong?

	Other small problems I noticed in running the testsuite:
        why is qemu-ga$(EXESUF) in tests/Makefile and test-qmp-commands
        not run by "make check"?

 qapi/qmp-input-visitor.c |   19 ++++++++++++++++--
 qerror.h                 |    3 +++
 test-qmp-input-visitor.c |   19 +++++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index e6b6152..416ab90 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -23,7 +23,8 @@
 typedef struct StackObject
 {
     const QObject *obj;
-    const  QListEntry *entry;
+    const QListEntry *entry;
+    int   left;
 } StackObject;
 
 struct QmpInputVisitor
@@ -31,6 +32,7 @@ struct QmpInputVisitor
     Visitor visitor;
     QObject *obj;
     StackObject stack[QIV_STACK_SIZE];
+    int left;
     int nb_stack;
 };
 
@@ -52,8 +54,13 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 
     if (qobj) {
         if (name && qobject_type(qobj) == QTYPE_QDICT) {
-            return qdict_get(qobject_to_qdict(qobj), name);
+            qobj = qdict_get(qobject_to_qdict(qobj), name);
+            if (qobj) {
+                assert(qiv->left > 0);
+                qiv->left--;
+            }
         } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
+            assert(qiv->left == -1);
             return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
         }
     }
@@ -64,10 +70,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **errp)
 {
     qiv->stack[qiv->nb_stack].obj = obj;
+    qiv->stack[qiv->nb_stack].left = qiv->left;
     if (qobject_type(obj) == QTYPE_QLIST) {
         qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
     }
     qiv->nb_stack++;
+    qiv->left = -1;
 
     if (qiv->nb_stack >= QIV_STACK_SIZE) {
         error_set(errp, QERR_BUFFER_OVERRUN);
@@ -77,7 +85,12 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
 
 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
+    if (qiv->left != -1 && qiv->left != 0) {
+        error_set(errp, QERR_EXTRA_MEMBER);
+        return;
+    }
     qiv->nb_stack--;
+    qiv->left = qiv->stack[qiv->nb_stack].left;
     if (qiv->nb_stack < 0) {
         error_set(errp, QERR_BUFFER_OVERRUN);
         return;
@@ -97,6 +110,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
     }
 
     qmp_input_push(qiv, qobj, errp);
+    qiv->left = qdict_size(qobject_to_qdict(qobj));
     if (error_is_set(errp)) {
         return;
     }
diff --git a/qerror.h b/qerror.h
index e26c635..520cdab 100644
--- a/qerror.h
+++ b/qerror.h
@@ -127,6 +127,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DUPLICATE_ID \
     "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
 
+#define QERR_EXTRA_MEMBER \
+    "{ 'class': 'ExtraInputObjectMember', 'data': {} }"
+
 #define QERR_FD_NOT_FOUND \
     "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
 
diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
index 1996e49..1783759 100644
--- a/test-qmp-input-visitor.c
+++ b/test-qmp-input-visitor.c
@@ -161,6 +161,23 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
     visit_end_struct(v, errp);
 }
 
+static void test_visitor_in_struct_extra(TestInputVisitorData *data,
+                                         const void *unused)
+{
+    TestStruct *p = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra' : [ 123, 456, 'def' ] }");
+
+    visit_type_TestStruct(v, &p, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    if (p) {
+        g_free(p->string);
+        g_free(p);
+    }
+}
+
 static void test_visitor_in_struct(TestInputVisitorData *data,
                                    const void *unused)
 {
@@ -278,6 +295,8 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_struct);
     input_visitor_test_add("/visitor/input/struct-nested",
                             &in_visitor_data, test_visitor_in_struct_nested);
+    input_visitor_test_add("/visitor/input/struct-extra",
+                            &in_visitor_data, test_visitor_in_struct_extra);
     input_visitor_test_add("/visitor/input/list",
                             &in_visitor_data, test_visitor_in_list);
     input_visitor_test_add("/visitor/input/union",
-- 
1.7.7.6

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/19/2012 02:29 PM, Paolo Bonzini wrote:
> I noticed that the QMP input visitor does not detect extra members inside
> structs.  The outermost arguments struct is handled by the QMP type
> checker, but the nested ones go undetected.  That could be a problem
> for complex commands such as "transaction".
>
> This patch adds such detection to the QMP input visitor.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
> 	Is this acceptable or just wrong?

This is a feature.  The idea is that with QMP, old clients just ignore extra 
members in a structure.  I've never felt that comfortable with this as a 
semantic but this is how QMP was designed.

If you don't allow this semantic, then it's impossible to ever add a field to an 
existing type as that would break backwards compatibility.

Regards,

Anthony Liguori

>
> 	Other small problems I noticed in running the testsuite:
>          why is qemu-ga$(EXESUF) in tests/Makefile and test-qmp-commands
>          not run by "make check"?
>
>   qapi/qmp-input-visitor.c |   19 ++++++++++++++++--
>   qerror.h                 |    3 +++
>   test-qmp-input-visitor.c |   19 +++++++++++++++++++
>   3 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e6b6152..416ab90 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -23,7 +23,8 @@
>   typedef struct StackObject
>   {
>       const QObject *obj;
> -    const  QListEntry *entry;
> +    const QListEntry *entry;
> +    int   left;
>   } StackObject;
>
>   struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
>       Visitor visitor;
>       QObject *obj;
>       StackObject stack[QIV_STACK_SIZE];
> +    int left;
>       int nb_stack;
>   };
>
> @@ -52,8 +54,13 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>
>       if (qobj) {
>           if (name&&  qobject_type(qobj) == QTYPE_QDICT) {
> -            return qdict_get(qobject_to_qdict(qobj), name);
> +            qobj = qdict_get(qobject_to_qdict(qobj), name);
> +            if (qobj) {
> +                assert(qiv->left>  0);
> +                qiv->left--;
> +            }
>           } else if (qiv->nb_stack>  0&&  qobject_type(qobj) == QTYPE_QLIST) {
> +            assert(qiv->left == -1);
>               return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
>           }
>       }
> @@ -64,10 +70,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>   static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **errp)
>   {
>       qiv->stack[qiv->nb_stack].obj = obj;
> +    qiv->stack[qiv->nb_stack].left = qiv->left;
>       if (qobject_type(obj) == QTYPE_QLIST) {
>           qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
>       }
>       qiv->nb_stack++;
> +    qiv->left = -1;
>
>       if (qiv->nb_stack>= QIV_STACK_SIZE) {
>           error_set(errp, QERR_BUFFER_OVERRUN);
> @@ -77,7 +85,12 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
>
>   static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>   {
> +    if (qiv->left != -1&&  qiv->left != 0) {
> +        error_set(errp, QERR_EXTRA_MEMBER);
> +        return;
> +    }
>       qiv->nb_stack--;
> +    qiv->left = qiv->stack[qiv->nb_stack].left;
>       if (qiv->nb_stack<  0) {
>           error_set(errp, QERR_BUFFER_OVERRUN);
>           return;
> @@ -97,6 +110,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
>       }
>
>       qmp_input_push(qiv, qobj, errp);
> +    qiv->left = qdict_size(qobject_to_qdict(qobj));
>       if (error_is_set(errp)) {
>           return;
>       }
> diff --git a/qerror.h b/qerror.h
> index e26c635..520cdab 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -127,6 +127,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_DUPLICATE_ID \
>       "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
>
> +#define QERR_EXTRA_MEMBER \
> +    "{ 'class': 'ExtraInputObjectMember', 'data': {} }"
> +
>   #define QERR_FD_NOT_FOUND \
>       "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
>
> diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
> index 1996e49..1783759 100644
> --- a/test-qmp-input-visitor.c
> +++ b/test-qmp-input-visitor.c
> @@ -161,6 +161,23 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
>       visit_end_struct(v, errp);
>   }
>
> +static void test_visitor_in_struct_extra(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra' : [ 123, 456, 'def' ] }");
> +
> +    visit_type_TestStruct(v,&p, NULL,&errp);
> +    g_assert(error_is_set(&errp));
> +    if (p) {
> +        g_free(p->string);
> +        g_free(p);
> +    }
> +}
> +
>   static void test_visitor_in_struct(TestInputVisitorData *data,
>                                      const void *unused)
>   {
> @@ -278,6 +295,8 @@ int main(int argc, char **argv)
>                               &in_visitor_data, test_visitor_in_struct);
>       input_visitor_test_add("/visitor/input/struct-nested",
>                               &in_visitor_data, test_visitor_in_struct_nested);
> +    input_visitor_test_add("/visitor/input/struct-extra",
> +&in_visitor_data, test_visitor_in_struct_extra);
>       input_visitor_test_add("/visitor/input/list",
>                               &in_visitor_data, test_visitor_in_list);
>       input_visitor_test_add("/visitor/input/union",

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Paolo Bonzini]

Il 19/03/2012 20:34, Anthony Liguori ha scritto:
>>
>>     Is this acceptable or just wrong?
> 
> This is a feature.  The idea is that with QMP, old clients just ignore
> extra members in a structure.  I've never felt that comfortable with
> this as a semantic but this is how QMP was designed.

For old clients that could be fine.  But what about old servers? :)

Perhaps we need an argument to the QMPInputVisitor constructor to
control this.

> If you don't allow this semantic, then it's impossible to ever add a
> field to an existing type as that would break backwards compatibility.

Paolo

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/19/2012 02:51 PM, Paolo Bonzini wrote:
> Il 19/03/2012 20:34, Anthony Liguori ha scritto:
>>>
>>>      Is this acceptable or just wrong?
>>
>> This is a feature.  The idea is that with QMP, old clients just ignore
>> extra members in a structure.  I've never felt that comfortable with
>> this as a semantic but this is how QMP was designed.
>
> For old clients that could be fine.  But what about old servers? :)

Same applies to old server.  If a new client tries to use a new field, if the 
old server refuses it, then the new client breaks.

There's no way in QMP to detect whether a server supports a new field.  This is 
why I proposed instituting a policy of never adding/removing fields to 
structures and why I had advocating use a C version of libqapi in terms of 
enforcing compatibility rules.

I'm not sure if the "server ignores unknown" fields thing is even reasonable to 
rely upon so maybe we should just draw a line in the sane and make the change 
you're suggesting...

Regards,

Anthony Liguori

>
> Perhaps we need an argument to the QMPInputVisitor constructor to
> control this.
>
>> If you don't allow this semantic, then it's impossible to ever add a
>> field to an existing type as that would break backwards compatibility.
>
> Paolo

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]

On 03/19/2012 01:56 PM, Anthony Liguori wrote:
>> For old clients that could be fine.  But what about old servers? :)
> 
> Same applies to old server.  If a new client tries to use a new field,
> if the old server refuses it, then the new client breaks.

I recently asked this question, and I was told that it is a feature that
unknown fields attempted by a new client are rejected by an old server:
https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html

> 
> There's no way in QMP to detect whether a server supports a new field. 
> This is why I proposed instituting a policy of never adding/removing
> fields to structures and why I had advocating use a C version of libqapi
> in terms of enforcing compatibility rules.
> 
> I'm not sure if the "server ignores unknown" fields thing is even
> reasonable to rely upon so maybe we should just draw a line in the sane
> and make the change you're suggesting...

For ideal back-compat, I think you want:

On input to the server, we can add new fields, but such new fields must
be optional (old clients that omit the fields get the default value,
rather than a new server rejecting the command due to a missing field).
 The question arises when you have a new client talking to an old
server; here, I think it's better to _always_ have the server reject
things it doesn't recognize, so that clients can use this rejection as a
feature probe, and then you _do_ have reliable ways of querying whether
a feature was added, by whether the new argument associated with that
feature is present.

On output from the server, we can add new fields (such as more details
about an error message), and old clients should ignore extra fields.
Meanwhile, these fields should be documented as optional, so a new
client can be prepared to deal with an older server that didn't send the
field.

So yes, it really does sound like you want different behavior depending
on whether it is the client or the server that is originating the new
fields.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/19/2012 03:22 PM, Eric Blake wrote:
> On 03/19/2012 01:56 PM, Anthony Liguori wrote:
>>> For old clients that could be fine.  But what about old servers? :)
>>
>> Same applies to old server.  If a new client tries to use a new field,
>> if the old server refuses it, then the new client breaks.
>
> I recently asked this question, and I was told that it is a feature that
> unknown fields attempted by a new client are rejected by an old server:
> https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html

Unfortunately that's not entirely correct.

The QMP server has a mechanism to validate that the input parameters that are 
passed conform to the spec in qmp-commands.hx, but this spec can only handle 
simple types.  Anywhere where we bypass this check (like we do for transaction), 
the checking is up to the callee which doesn't check.



>
>>
>> There's no way in QMP to detect whether a server supports a new field.
>> This is why I proposed instituting a policy of never adding/removing
>> fields to structures and why I had advocating use a C version of libqapi
>> in terms of enforcing compatibility rules.
>>
>> I'm not sure if the "server ignores unknown" fields thing is even
>> reasonable to rely upon so maybe we should just draw a line in the sane
>> and make the change you're suggesting...
>
> For ideal back-compat, I think you want:
>
> On input to the server, we can add new fields, but such new fields must
> be optional (old clients that omit the fields get the default value,
> rather than a new server rejecting the command due to a missing field).
>   The question arises when you have a new client talking to an old
> server; here, I think it's better to _always_ have the server reject
> things it doesn't recognize, so that clients can use this rejection as a
> feature probe, and then you _do_ have reliable ways of querying whether
> a feature was added, by whether the new argument associated with that
> feature is present.

The problem is that this requires transactional semantics such that if the 
command fails, there are no side effects.  I don't think we're in a position to 
guarantee that.

I'd greatly prefer to simply not add new options to existing commands.  It's 
simple, maps well to how we do things in C, and is easy to enforce.

Regards,

Anthony Liguori

>
> On output from the server, we can add new fields (such as more details
> about an error message), and old clients should ignore extra fields.
> Meanwhile, these fields should be documented as optional, so a new
> client can be prepared to deal with an older server that didn't send the
> field.
>
> So yes, it really does sound like you want different behavior depending
> on whether it is the client or the server that is originating the new
> fields.
>

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Luiz Capitulino]

On Mon, 19 Mar 2012 15:30:26 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/19/2012 03:22 PM, Eric Blake wrote:
> > On 03/19/2012 01:56 PM, Anthony Liguori wrote:
> >>> For old clients that could be fine.  But what about old servers? :)
> >>
> >> Same applies to old server.  If a new client tries to use a new field,
> >> if the old server refuses it, then the new client breaks.
> >
> > I recently asked this question, and I was told that it is a feature that
> > unknown fields attempted by a new client are rejected by an old server:
> > https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html
> 
> Unfortunately that's not entirely correct.
> 
> The QMP server has a mechanism to validate that the input parameters that are 
> passed conform to the spec in qmp-commands.hx, but this spec can only handle 
> simple types.  Anywhere where we bypass this check (like we do for transaction), 
> the checking is up to the callee which doesn't check.

Right.

> >> There's no way in QMP to detect whether a server supports a new field.
> >> This is why I proposed instituting a policy of never adding/removing
> >> fields to structures and why I had advocating use a C version of libqapi
> >> in terms of enforcing compatibility rules.
> >>
> >> I'm not sure if the "server ignores unknown" fields thing is even
> >> reasonable to rely upon so maybe we should just draw a line in the sane
> >> and make the change you're suggesting...
> >
> > For ideal back-compat, I think you want:
> >
> > On input to the server, we can add new fields, but such new fields must
> > be optional (old clients that omit the fields get the default value,
> > rather than a new server rejecting the command due to a missing field).
> >   The question arises when you have a new client talking to an old
> > server; here, I think it's better to _always_ have the server reject
> > things it doesn't recognize, so that clients can use this rejection as a
> > feature probe, and then you _do_ have reliable ways of querying whether
> > a feature was added, by whether the new argument associated with that
> > feature is present.
> 
> The problem is that this requires transactional semantics such that if the 
> command fails, there are no side effects.  I don't think we're in a position to 
> guarantee that.

IMHO failing with side effects is a bug. Although no command that does that comes
to my mind right now.

> I'd greatly prefer to simply not add new options to existing commands.  It's 
> simple, maps well to how we do things in C, and is easy to enforce.

We extensively discussed this some months ago. I honestly don't want to repeat
that discussion. So, if there's consensus that adding new commands is better
than extending existing ones, this is fine with me.

Another solution would be to extend query-commands with arguments info, which
is something that I think we will do soon or later...

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Michael Roth]

On Mon, Mar 19, 2012 at 05:43:14PM -0300, Luiz Capitulino wrote:
> On Mon, 19 Mar 2012 15:30:26 -0500
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> 
> > On 03/19/2012 03:22 PM, Eric Blake wrote:
> > > On 03/19/2012 01:56 PM, Anthony Liguori wrote:
> > >>> For old clients that could be fine.  But what about old servers? :)
> > >>
> > >> Same applies to old server.  If a new client tries to use a new field,
> > >> if the old server refuses it, then the new client breaks.
> > >
> > > I recently asked this question, and I was told that it is a feature that
> > > unknown fields attempted by a new client are rejected by an old server:
> > > https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html
> > 
> > Unfortunately that's not entirely correct.
> > 
> > The QMP server has a mechanism to validate that the input parameters that are 
> > passed conform to the spec in qmp-commands.hx, but this spec can only handle 
> > simple types.  Anywhere where we bypass this check (like we do for transaction), 
> > the checking is up to the callee which doesn't check.
> 
> Right.
> 
> > >> There's no way in QMP to detect whether a server supports a new field.
> > >> This is why I proposed instituting a policy of never adding/removing
> > >> fields to structures and why I had advocating use a C version of libqapi
> > >> in terms of enforcing compatibility rules.
> > >>
> > >> I'm not sure if the "server ignores unknown" fields thing is even
> > >> reasonable to rely upon so maybe we should just draw a line in the sane
> > >> and make the change you're suggesting...
> > >
> > > For ideal back-compat, I think you want:
> > >
> > > On input to the server, we can add new fields, but such new fields must
> > > be optional (old clients that omit the fields get the default value,
> > > rather than a new server rejecting the command due to a missing field).
> > >   The question arises when you have a new client talking to an old
> > > server; here, I think it's better to _always_ have the server reject
> > > things it doesn't recognize, so that clients can use this rejection as a
> > > feature probe, and then you _do_ have reliable ways of querying whether
> > > a feature was added, by whether the new argument associated with that
> > > feature is present.
> > 
> > The problem is that this requires transactional semantics such that if the 
> > command fails, there are no side effects.  I don't think we're in a position to 
> > guarantee that.
> 
> IMHO failing with side effects is a bug. Although no command that does that comes
> to my mind right now.
> 
> > I'd greatly prefer to simply not add new options to existing commands.  It's 
> > simple, maps well to how we do things in C, and is easy to enforce.
> 
> We extensively discussed this some months ago. I honestly don't want to repeat
> that discussion. So, if there's consensus that adding new commands is better
> than extending existing ones, this is fine with me.
> 
> Another solution would be to extend query-commands with arguments info, which
> is something that I think we will do soon or later...

It's doable, we could even just give them QAPI schema, which is valid
json and serves as our QMP command documentation anyway. I think that's a
complete solution, and makes thing easy on the server/qemu-side. But for a
client it may be somewhat of a pain.

New field -> new command is seems easier on the client-side in theory, but once
you go beyond a handful of variations and get to a point where several
include the desired option, you basically need to start encoding the
option "capability" in the command name, or using versioned commands, so
long term it may not scale well. It may also be difficult to ensure commands
with a certain name don't lose/gain fields downstream.

So IMO, returning arguments actually seems easier for both clients and the
server, and is more resilient to downstream changes. It does require a more
formal specification of the protocol though. Basically: "an option/field
may not be supported in older/future versions of QEMU, so it is up to
the client to check in advance by referencing the QAPI schema for their
qemu version or programatically via get_schema(<command>)"

> 

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/19/2012 05:29 PM, Michael Roth wrote:
> On Mon, Mar 19, 2012 at 05:43:14PM -0300, Luiz Capitulino wrote:
>> On Mon, 19 Mar 2012 15:30:26 -0500
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>
>>> On 03/19/2012 03:22 PM, Eric Blake wrote:
>>>> On 03/19/2012 01:56 PM, Anthony Liguori wrote:
>>>>>> For old clients that could be fine.  But what about old servers? :)
>>>>>
>>>>> Same applies to old server.  If a new client tries to use a new field,
>>>>> if the old server refuses it, then the new client breaks.
>>>>
>>>> I recently asked this question, and I was told that it is a feature that
>>>> unknown fields attempted by a new client are rejected by an old server:
>>>> https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html
>>>
>>> Unfortunately that's not entirely correct.
>>>
>>> The QMP server has a mechanism to validate that the input parameters that are
>>> passed conform to the spec in qmp-commands.hx, but this spec can only handle
>>> simple types.  Anywhere where we bypass this check (like we do for transaction),
>>> the checking is up to the callee which doesn't check.
>>
>> Right.
>>
>>>>> There's no way in QMP to detect whether a server supports a new field.
>>>>> This is why I proposed instituting a policy of never adding/removing
>>>>> fields to structures and why I had advocating use a C version of libqapi
>>>>> in terms of enforcing compatibility rules.
>>>>>
>>>>> I'm not sure if the "server ignores unknown" fields thing is even
>>>>> reasonable to rely upon so maybe we should just draw a line in the sane
>>>>> and make the change you're suggesting...
>>>>
>>>> For ideal back-compat, I think you want:
>>>>
>>>> On input to the server, we can add new fields, but such new fields must
>>>> be optional (old clients that omit the fields get the default value,
>>>> rather than a new server rejecting the command due to a missing field).
>>>>    The question arises when you have a new client talking to an old
>>>> server; here, I think it's better to _always_ have the server reject
>>>> things it doesn't recognize, so that clients can use this rejection as a
>>>> feature probe, and then you _do_ have reliable ways of querying whether
>>>> a feature was added, by whether the new argument associated with that
>>>> feature is present.
>>>
>>> The problem is that this requires transactional semantics such that if the
>>> command fails, there are no side effects.  I don't think we're in a position to
>>> guarantee that.
>>
>> IMHO failing with side effects is a bug. Although no command that does that comes
>> to my mind right now.
>>
>>> I'd greatly prefer to simply not add new options to existing commands.  It's
>>> simple, maps well to how we do things in C, and is easy to enforce.
>>
>> We extensively discussed this some months ago. I honestly don't want to repeat
>> that discussion. So, if there's consensus that adding new commands is better
>> than extending existing ones, this is fine with me.
>>
>> Another solution would be to extend query-commands with arguments info, which
>> is something that I think we will do soon or later...
>
> It's doable, we could even just give them QAPI schema, which is valid
> json and serves as our QMP command documentation anyway. I think that's a
> complete solution, and makes thing easy on the server/qemu-side. But for a
> client it may be somewhat of a pain.

Right, you would need to be able to fully interpret the schema which means 
recursing into each of the types to discover if a field was added anywhere. 
It's down right cruel to expect clients to do this IMHO.

> New field ->  new command is seems easier on the client-side in theory, but once
> you go beyond a handful of variations and get to a point where several
> include the desired option, you basically need to start encoding the
> option "capability" in the command name, or using versioned commands, so
> long term it may not scale well. It may also be difficult to ensure commands
> with a certain name don't lose/gain fields downstream.

And this is why we should take our time designing commands attempting to think 
through the future use cases.

> So IMO, returning arguments actually seems easier for both clients and the
> server, and is more resilient to downstream changes. It does require a more
> formal specification of the protocol though. Basically: "an option/field
> may not be supported in older/future versions of QEMU, so it is up to
> the client to check in advance by referencing the QAPI schema for their
> qemu version or programatically via get_schema(<command>)"

The complexity of writing a client using get_schema() is close to staggering :-/

Regards,

Anthony Liguori

>>
>

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Michael Roth]

On Mon, Mar 19, 2012 at 05:38:54PM -0500, Anthony Liguori wrote:
> On 03/19/2012 05:29 PM, Michael Roth wrote:
> >On Mon, Mar 19, 2012 at 05:43:14PM -0300, Luiz Capitulino wrote:
> >>On Mon, 19 Mar 2012 15:30:26 -0500
> >>Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >>
> >>>On 03/19/2012 03:22 PM, Eric Blake wrote:
> >>>>On 03/19/2012 01:56 PM, Anthony Liguori wrote:
> >>>>>>For old clients that could be fine.  But what about old servers? :)
> >>>>>
> >>>>>Same applies to old server.  If a new client tries to use a new field,
> >>>>>if the old server refuses it, then the new client breaks.
> >>>>
> >>>>I recently asked this question, and I was told that it is a feature that
> >>>>unknown fields attempted by a new client are rejected by an old server:
> >>>>https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg00815.html
> >>>
> >>>Unfortunately that's not entirely correct.
> >>>
> >>>The QMP server has a mechanism to validate that the input parameters that are
> >>>passed conform to the spec in qmp-commands.hx, but this spec can only handle
> >>>simple types.  Anywhere where we bypass this check (like we do for transaction),
> >>>the checking is up to the callee which doesn't check.
> >>
> >>Right.
> >>
> >>>>>There's no way in QMP to detect whether a server supports a new field.
> >>>>>This is why I proposed instituting a policy of never adding/removing
> >>>>>fields to structures and why I had advocating use a C version of libqapi
> >>>>>in terms of enforcing compatibility rules.
> >>>>>
> >>>>>I'm not sure if the "server ignores unknown" fields thing is even
> >>>>>reasonable to rely upon so maybe we should just draw a line in the sane
> >>>>>and make the change you're suggesting...
> >>>>
> >>>>For ideal back-compat, I think you want:
> >>>>
> >>>>On input to the server, we can add new fields, but such new fields must
> >>>>be optional (old clients that omit the fields get the default value,
> >>>>rather than a new server rejecting the command due to a missing field).
> >>>>   The question arises when you have a new client talking to an old
> >>>>server; here, I think it's better to _always_ have the server reject
> >>>>things it doesn't recognize, so that clients can use this rejection as a
> >>>>feature probe, and then you _do_ have reliable ways of querying whether
> >>>>a feature was added, by whether the new argument associated with that
> >>>>feature is present.
> >>>
> >>>The problem is that this requires transactional semantics such that if the
> >>>command fails, there are no side effects.  I don't think we're in a position to
> >>>guarantee that.
> >>
> >>IMHO failing with side effects is a bug. Although no command that does that comes
> >>to my mind right now.
> >>
> >>>I'd greatly prefer to simply not add new options to existing commands.  It's
> >>>simple, maps well to how we do things in C, and is easy to enforce.
> >>
> >>We extensively discussed this some months ago. I honestly don't want to repeat
> >>that discussion. So, if there's consensus that adding new commands is better
> >>than extending existing ones, this is fine with me.
> >>
> >>Another solution would be to extend query-commands with arguments info, which
> >>is something that I think we will do soon or later...
> >
> >It's doable, we could even just give them QAPI schema, which is valid
> >json and serves as our QMP command documentation anyway. I think that's a
> >complete solution, and makes thing easy on the server/qemu-side. But for a
> >client it may be somewhat of a pain.
> 
> Right, you would need to be able to fully interpret the schema which
> means recursing into each of the types to discover if a field was
> added anywhere. It's down right cruel to expect clients to do this
> IMHO.
> 
> >New field ->  new command is seems easier on the client-side in theory, but once
> >you go beyond a handful of variations and get to a point where several
> >include the desired option, you basically need to start encoding the
> >option "capability" in the command name, or using versioned commands, so
> >long term it may not scale well. It may also be difficult to ensure commands
> >with a certain name don't lose/gain fields downstream.
> 
> And this is why we should take our time designing commands
> attempting to think through the future use cases.

Well, true, I guess it's worked out for long enough that we shouldn't assume
things would get that bad any time soon.

> 
> >So IMO, returning arguments actually seems easier for both clients and the
> >server, and is more resilient to downstream changes. It does require a more
> >formal specification of the protocol though. Basically: "an option/field
> >may not be supported in older/future versions of QEMU, so it is up to
> >the client to check in advance by referencing the QAPI schema for their
> >qemu version or programatically via get_schema(<command>)"
> 
> The complexity of writing a client using get_schema() is close to staggering :-/

I'm not sure, I mean, take libvirt, you need to marshall up the
arguments anyway and put them into a QMP payload, so in that case the
client developer is aware of the schema that they're coding against,
and also understand the QAPI schema specification, and if the schema
is nested then so is the client version of that "schema".

So, conceptually at least, it seems like it wouldn't be that big a jump
to maintain an internal representation of their schema to
programatically check against the specification they were coding
against, it's just the part where they then need to bake it into the
client implementation that's a bit heavy-handed.

Thinking about it more the, this does seem to be completely at odds with
any future prospects of a libqmp, so that's a pretty big trade-off...

> 
> Regards,
> 
> Anthony Liguori
> 
> >>
> >
> 

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/19/2012 06:45 PM, Michael Roth wrote:
>>> So IMO, returning arguments actually seems easier for both clients and the
>>> server, and is more resilient to downstream changes. It does require a more
>>> formal specification of the protocol though. Basically: "an option/field
>>> may not be supported in older/future versions of QEMU, so it is up to
>>> the client to check in advance by referencing the QAPI schema for their
>>> qemu version or programatically via get_schema(<command>)"
>>
>> The complexity of writing a client using get_schema() is close to staggering :-/
>
> I'm not sure, I mean, take libvirt, you need to marshall up the
> arguments anyway and put them into a QMP payload, so in that case the
> client developer is aware of the schema that they're coding against,
> and also understand the QAPI schema specification, and if the schema
> is nested then so is the client version of that "schema".
>
> So, conceptually at least, it seems like it wouldn't be that big a jump
> to maintain an internal representation of their schema to
> programatically check against the specification they were coding
> against, it's just the part where they then need to bake it into the
> client implementation that's a bit heavy-handed.

So let's work through a few examples.   Today, you have to maintain a list of 
commands returned from query-commands and check for set membership:

if 'query-netdev2' in commands:
    qmp.query_netdev2(foo)
else:
    qmp.query_netdev()

Pretty simple.  If we have a schema representation, we'll need to be able to 
check for arguments.  That means we need a way to check if a command has an 
argument.  We could do this as nested lists:

if (commands.has_key('query-netdev') and
     commands['query-netdev']['args'].has_key('interface')):
    qmp.query_netdev(foo)
else:
    qmp.query_netdev()

This is only really possible in a dynamically typed language, which sucks, but 
let's ignore that for now.  This is a bit more complicated but unfortunately, 
it's not enough.

Because we can add members to structures and structures can be arguments.  So we 
really need a way to say:

1) Do we have the query-netdev command
2) Does it take a parameter called interface of type NetDevArgument
3) Does the NetDevArgument have a member called 'foo'.

But this actually can be arbitrarily deep in terms of complex.  Maybe you make a 
simple query language so you can execute schema queries in a single line.

So you have an interface definition that is the schema that describes an 
unstructured wire protocol and a query language to determine when and how the 
schema changes.  If that doesn't ring a bell to you, congratulations, you've 
invented SOAP.

The interface description in SOAP is WSDL and XPath is the query language. 
Writing a SOAP client is a train wreck.  It's so bad that even in dynamic 
languages, people almost always use code generators.

A good RPC makes the client simple.  If that means we need to take care when 
introducing new interfaces, then it's a small price to pay for avoiding the 
nightmares of an over engineered RPC like SOAP.

Regards,

Anthony Liguori

> Thinking about it more the, this does seem to be completely at odds with
> any future prospects of a libqmp, so that's a pretty big trade-off...
>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>>
>>>
>>

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Luiz Capitulino]

On Mon, 19 Mar 2012 19:49:46 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/19/2012 06:45 PM, Michael Roth wrote:
> >>> So IMO, returning arguments actually seems easier for both clients and the
> >>> server, and is more resilient to downstream changes. It does require a more
> >>> formal specification of the protocol though. Basically: "an option/field
> >>> may not be supported in older/future versions of QEMU, so it is up to
> >>> the client to check in advance by referencing the QAPI schema for their
> >>> qemu version or programatically via get_schema(<command>)"
> >>
> >> The complexity of writing a client using get_schema() is close to staggering :-/
> >
> > I'm not sure, I mean, take libvirt, you need to marshall up the
> > arguments anyway and put them into a QMP payload, so in that case the
> > client developer is aware of the schema that they're coding against,
> > and also understand the QAPI schema specification, and if the schema
> > is nested then so is the client version of that "schema".
> >
> > So, conceptually at least, it seems like it wouldn't be that big a jump
> > to maintain an internal representation of their schema to
> > programatically check against the specification they were coding
> > against, it's just the part where they then need to bake it into the
> > client implementation that's a bit heavy-handed.
> 
> So let's work through a few examples.   Today, you have to maintain a list of 
> commands returned from query-commands and check for set membership:
> 
> if 'query-netdev2' in commands:
>     qmp.query_netdev2(foo)
> else:
>     qmp.query_netdev()
> 
> Pretty simple.  If we have a schema representation, we'll need to be able to 
> check for arguments.

Aren't we going to have the schema representation anyway? Or if we go for the
way above we are not going to have it?

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori]

On 03/20/2012 07:15 AM, Luiz Capitulino wrote:
>> So let's work through a few examples.   Today, you have to maintain a list of
>> commands returned from query-commands and check for set membership:
>>
>> if 'query-netdev2' in commands:
>>      qmp.query_netdev2(foo)
>> else:
>>      qmp.query_netdev()
>>
>> Pretty simple.  If we have a schema representation, we'll need to be able to
>> check for arguments.
>
> Aren't we going to have the schema representation anyway? Or if we go for the
> way above we are not going to have it?

We have it, and we should expose it, but we should not IMHO require deep schema 
introspection in order for a client to make use of new functionality.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Paolo Bonzini]

Il 19/03/2012 20:56, Anthony Liguori ha scritto:
> I'm not sure if the "server ignores unknown" fields thing is even
> reasonable to rely upon so maybe we should just draw a line in the sane
> and make the change you're suggesting...

Yeah, especially since right now it's half-half (worst of both worlds):
toplevel unknowns error out, while those deep in a struct are ignored.

I found a few other issues while working out a strict mode for the input
visitor, I'll submit the patches in a few days.

Paolo

Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Michael Roth]

On Mon, Mar 19, 2012 at 08:29:28PM +0100, Paolo Bonzini wrote:
> I noticed that the QMP input visitor does not detect extra members inside
> structs.  The outermost arguments struct is handled by the QMP type
> checker, but the nested ones go undetected.  That could be a problem
> for complex commands such as "transaction".
> 
> This patch adds such detection to the QMP input visitor.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Is this acceptable or just wrong?
> 
> 	Other small problems I noticed in running the testsuite:
>         why is qemu-ga$(EXESUF) in tests/Makefile and test-qmp-commands

qemu-ga looks like an inadvertant copy/paste. and yah, test-qmp-commands
should be part of make check, might've been left out initially because it
barfs out debug statements. Just sent some patches.

>         not run by "make check"?
> 
>  qapi/qmp-input-visitor.c |   19 ++++++++++++++++--
>  qerror.h                 |    3 +++
>  test-qmp-input-visitor.c |   19 +++++++++++++++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e6b6152..416ab90 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -23,7 +23,8 @@
>  typedef struct StackObject
>  {
>      const QObject *obj;
> -    const  QListEntry *entry;
> +    const QListEntry *entry;
> +    int   left;
>  } StackObject;
> 
>  struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
>      Visitor visitor;
>      QObject *obj;
>      StackObject stack[QIV_STACK_SIZE];
> +    int left;
>      int nb_stack;
>  };
> 
> @@ -52,8 +54,13 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
> 
>      if (qobj) {
>          if (name && qobject_type(qobj) == QTYPE_QDICT) {
> -            return qdict_get(qobject_to_qdict(qobj), name);
> +            qobj = qdict_get(qobject_to_qdict(qobj), name);
> +            if (qobj) {
> +                assert(qiv->left > 0);
> +                qiv->left--;
> +            }
>          } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
> +            assert(qiv->left == -1);
>              return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
>          }
>      }
> @@ -64,10 +70,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>  static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **errp)
>  {
>      qiv->stack[qiv->nb_stack].obj = obj;
> +    qiv->stack[qiv->nb_stack].left = qiv->left;
>      if (qobject_type(obj) == QTYPE_QLIST) {
>          qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
>      }
>      qiv->nb_stack++;
> +    qiv->left = -1;
> 
>      if (qiv->nb_stack >= QIV_STACK_SIZE) {
>          error_set(errp, QERR_BUFFER_OVERRUN);
> @@ -77,7 +85,12 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
> 
>  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>  {
> +    if (qiv->left != -1 && qiv->left != 0) {
> +        error_set(errp, QERR_EXTRA_MEMBER);
> +        return;
> +    }
>      qiv->nb_stack--;
> +    qiv->left = qiv->stack[qiv->nb_stack].left;
>      if (qiv->nb_stack < 0) {
>          error_set(errp, QERR_BUFFER_OVERRUN);
>          return;
> @@ -97,6 +110,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
>      }
> 
>      qmp_input_push(qiv, qobj, errp);
> +    qiv->left = qdict_size(qobject_to_qdict(qobj));
>      if (error_is_set(errp)) {
>          return;
>      }
> diff --git a/qerror.h b/qerror.h
> index e26c635..520cdab 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -127,6 +127,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_DUPLICATE_ID \
>      "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
> 
> +#define QERR_EXTRA_MEMBER \
> +    "{ 'class': 'ExtraInputObjectMember', 'data': {} }"
> +
>  #define QERR_FD_NOT_FOUND \
>      "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
> 
> diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
> index 1996e49..1783759 100644
> --- a/test-qmp-input-visitor.c
> +++ b/test-qmp-input-visitor.c
> @@ -161,6 +161,23 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
>      visit_end_struct(v, errp);
>  }
> 
> +static void test_visitor_in_struct_extra(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra' : [ 123, 456, 'def' ] }");
> +
> +    visit_type_TestStruct(v, &p, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    if (p) {
> +        g_free(p->string);
> +        g_free(p);
> +    }
> +}
> +
>  static void test_visitor_in_struct(TestInputVisitorData *data,
>                                     const void *unused)
>  {
> @@ -278,6 +295,8 @@ int main(int argc, char **argv)
>                              &in_visitor_data, test_visitor_in_struct);
>      input_visitor_test_add("/visitor/input/struct-nested",
>                              &in_visitor_data, test_visitor_in_struct_nested);
> +    input_visitor_test_add("/visitor/input/struct-extra",
> +                            &in_visitor_data, test_visitor_in_struct_extra);
>      input_visitor_test_add("/visitor/input/list",
>                              &in_visitor_data, test_visitor_in_list);
>      input_visitor_test_add("/visitor/input/union",
> -- 
> 1.7.7.6
> 
>