[Qemu-devel] [PATCH 00/10] QAPI minor fixes and strict mode

[Qemu-devel] [PATCH 00/10] QAPI minor fixes and strict mode

[Paolo Bonzini]

This patch includes a few minor changes to QAPI found during a
more careful code review (patches 1-5) and the implementation of
a strict mode for the QObject input visitor (patches 6-10).

While QMP in general is designed so that it is possible to ignore
unknown arguments, in the case of the QMP server it is better to
reject them to detect bad clients.  In fact, we're already doing
this at the top level in the argument checker.  Strict modes
checks for unvisited keys and raises an error if it finds one,
so that this checking extends to complex structures.

Please review.

Paolo Bonzini (10):
  qapi: add a test case for type errors
  qapi: fail hard on stack imbalance
  qapi: fix memory leak on error
  qapi: shortcut visits on errors
  qapi: allow freeing partially-allocated objects
  qapi: simplify qmp_input_next_list
  qapi: place outermost object on qiv stack
  qapi: add strict mode to input visitor
  qmp: add and use q type specifier
  qmp: parse commands in strict mode

 monitor.c                |    3 +
 qapi/qmp-input-visitor.c |  113 ++++++++++++++--------
 qapi/qmp-input-visitor.h |    2 +
 qmp-commands.hx          |    4 +-
 scripts/qapi-commands.py |    2 +-
 scripts/qapi-visit.py    |   16 +++
 test-qmp-input-strict.c  |  234 ++++++++++++++++++++++++++++++++++++++++++++++
 test-qmp-input-visitor.c |   19 ++++
 tests/Makefile           |    5 +-
 9 files changed, 354 insertions(+), 44 deletions(-)
 create mode 100644 test-qmp-input-strict.c

-- 
1.7.9.1

[Qemu-devel] [PATCH 01/10] qapi: add a test case for type errors

[Paolo Bonzini]

There is no test case for parse errors, add one.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 test-qmp-input-visitor.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
index 1996e49..c30fdc4 100644
--- a/test-qmp-input-visitor.c
+++ b/test-qmp-input-visitor.c
@@ -258,6 +258,23 @@ static void input_visitor_test_add(const char *testpath,
                visitor_input_teardown);
 }
 
+static void test_visitor_in_errors(TestInputVisitorData *data,
+                                   const void *unused)
+{
+    TestStruct *p = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
+
+    visit_type_TestStruct(v, &p, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    g_assert(p->string == NULL);
+
+    g_free(p->string);
+    g_free(p);
+}
+
 int main(int argc, char **argv)
 {
     TestInputVisitorData in_visitor_data;
@@ -282,6 +299,8 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_list);
     input_visitor_test_add("/visitor/input/union",
                             &in_visitor_data, test_visitor_in_union);
+    input_visitor_test_add("/visitor/input/errors",
+                            &in_visitor_data, test_visitor_in_errors);
 
     g_test_run();
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 01/10] qapi: add a test case for type errors

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> There is no test case for parse errors, add one.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   test-qmp-input-visitor.c |   19 +++++++++++++++++++
>   1 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
> index 1996e49..c30fdc4 100644
> --- a/test-qmp-input-visitor.c
> +++ b/test-qmp-input-visitor.c
> @@ -258,6 +258,23 @@ static void input_visitor_test_add(const char *testpath,
>                  visitor_input_teardown);
>   }
>
> +static void test_visitor_in_errors(TestInputVisitorData *data,
> +                                   const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
> +
> +    visit_type_TestStruct(v,&p, NULL,&errp);
> +    g_assert(error_is_set(&errp));
> +    g_assert(p->string == NULL);
> +
> +    g_free(p->string);
> +    g_free(p);
> +}
> +
>   int main(int argc, char **argv)
>   {
>       TestInputVisitorData in_visitor_data;
> @@ -282,6 +299,8 @@ int main(int argc, char **argv)
>                               &in_visitor_data, test_visitor_in_list);
>       input_visitor_test_add("/visitor/input/union",
>                               &in_visitor_data, test_visitor_in_union);
> +    input_visitor_test_add("/visitor/input/errors",
> +&in_visitor_data, test_visitor_in_errors);
>
>       g_test_run();
>

[Qemu-devel] [PATCH 02/10] qapi: fail hard on stack imbalance

[Paolo Bonzini]

QmpOutputVisitor will segfault if an imbalanced end function is
called.  So we can abort in QmpInputVisitor too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index e6b6152..b4013cc 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -77,11 +77,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
 
 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
+    assert(qiv->nb_stack > 0);
     qiv->nb_stack--;
-    if (qiv->nb_stack < 0) {
-        error_set(errp, QERR_BUFFER_OVERRUN);
-        return;
-    }
 }
 
 static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 02/10] qapi: fail hard on stack imbalance

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> QmpOutputVisitor will segfault if an imbalanced end function is
> called.  So we can abort in QmpInputVisitor too.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   qapi/qmp-input-visitor.c |    5 +----
>   1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e6b6152..b4013cc 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -77,11 +77,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
>
>   static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>   {
> +    assert(qiv->nb_stack>  0);
>       qiv->nb_stack--;
> -    if (qiv->nb_stack<  0) {
> -        error_set(errp, QERR_BUFFER_OVERRUN);
> -        return;
> -    }
>   }
>
>   static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,

Re: [Qemu-devel] [PATCH 02/10] qapi: fail hard on stack imbalance

[Luiz Capitulino]

On Thu, 22 Mar 2012 12:51:04 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> QmpOutputVisitor will segfault if an imbalanced end function is
> called.  So we can abort in QmpInputVisitor too.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/qmp-input-visitor.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e6b6152..b4013cc 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -77,11 +77,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
>  
>  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>  {
> +    assert(qiv->nb_stack > 0);
>      qiv->nb_stack--;
> -    if (qiv->nb_stack < 0) {
> -        error_set(errp, QERR_BUFFER_OVERRUN);
> -        return;
> -    }
>  }

Just to confirm: this can't be triggered by malicious clients, right?

The original series submitted by Michael had this, but I asked him to
change because I thought clients could trigger it. But by reading the code
now it seems to me that the end_struct() function is only generated by types
we know about.

>  
>  static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,

Re: [Qemu-devel] [PATCH 02/10] qapi: fail hard on stack imbalance

[Michael Roth]

On Mon, Mar 26, 2012 at 11:15:46AM -0300, Luiz Capitulino wrote:
> On Thu, 22 Mar 2012 12:51:04 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > QmpOutputVisitor will segfault if an imbalanced end function is
> > called.  So we can abort in QmpInputVisitor too.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  qapi/qmp-input-visitor.c |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index e6b6152..b4013cc 100644
> > --- a/qapi/qmp-input-visitor.c
> > +++ b/qapi/qmp-input-visitor.c
> > @@ -77,11 +77,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **err
> >  
> >  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
> >  {
> > +    assert(qiv->nb_stack > 0);
> >      qiv->nb_stack--;
> > -    if (qiv->nb_stack < 0) {
> > -        error_set(errp, QERR_BUFFER_OVERRUN);
> > -        return;
> > -    }
> >  }
> 
> Just to confirm: this can't be triggered by malicious clients, right?
> 
> The original series submitted by Michael had this, but I asked him to
> change because I thought clients could trigger it. But by reading the code
> now it seems to me that the end_struct() function is only generated by types
> we know about.
> 

Yah, looked into this as well. I can't see anything outside of a
bug triggering this: even if they did manage to get past the parser and
have some wonky qobject fed to us, the generated visitor code should
still always be balanced, and end_list/end_struct are the only way to
induce a pop so nb_stack should never go below 0. There may be other areas
where we need to watch out for this kind of stuff though.

> >  
> >  static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
> 
> 

[Qemu-devel] [PATCH 03/10] qapi: fix memory leak on error

[Paolo Bonzini]

QmpInputVisitor would leak the malloced struct if the stack was
overflowed.  This can be easily fixed using error_propagate.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index b4013cc..ef9288f 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -86,6 +86,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
 {
     QmpInputVisitor *qiv = to_qiv(v);
     const QObject *qobj = qmp_input_get_object(qiv, name);
+    Error *err = NULL;
 
     if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -93,8 +94,9 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
         return;
     }
 
-    qmp_input_push(qiv, qobj, errp);
-    if (error_is_set(errp)) {
+    qmp_input_push(qiv, qobj, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 03/10] qapi: fix memory leak on error

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> QmpInputVisitor would leak the malloced struct if the stack was
> overflowed.  This can be easily fixed using error_propagate.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   qapi/qmp-input-visitor.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index b4013cc..ef9288f 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -86,6 +86,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
>       const QObject *qobj = qmp_input_get_object(qiv, name);
> +    Error *err = NULL;
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -93,8 +94,9 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
>           return;
>       }
>
> -    qmp_input_push(qiv, qobj, errp);
> -    if (error_is_set(errp)) {
> +    qmp_input_push(qiv, qobj,&err);
> +    if (err) {
> +        error_propagate(errp, err);
>           return;
>       }
>

[Qemu-devel] [PATCH 04/10] qapi: shortcut visits on errors

[Paolo Bonzini]

We can exit very soon if we enter a visitor with a preexisting error.
This simplifies some cases because we will not have to deal with
obj being non-NULL while *obj is NULL.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi-visit.py |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 54117d4..b242315 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -61,6 +61,9 @@ def generate_visit_struct(name, members):
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
 {
+    if (error_is_set(errp)) {
+        return;
+    }
     visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
 ''',
                 name=name)
@@ -81,6 +84,9 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name,
 {
     GenericList *i, **head = (GenericList **)obj;
 
+    if (error_is_set(errp)) {
+        return;
+    }
     visit_start_list(m, name, errp);
 
     for (*head = i = visit_next_list(m, head, errp); i; i = visit_next_list(m, &i, errp)) {
@@ -112,6 +118,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 {
     Error *err = NULL;
 
+    if (error_is_set(errp)) {
+        return;
+    }
     visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
     visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
     if (err) {
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 04/10] qapi: shortcut visits on errors

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> We can exit very soon if we enter a visitor with a preexisting error.
> This simplifies some cases because we will not have to deal with
> obj being non-NULL while *obj is NULL.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   scripts/qapi-visit.py |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 54117d4..b242315 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -61,6 +61,9 @@ def generate_visit_struct(name, members):
>
>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
>   {
> +    if (error_is_set(errp)) {
> +        return;
> +    }
>       visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
>   ''',
>                   name=name)
> @@ -81,6 +84,9 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name,
>   {
>       GenericList *i, **head = (GenericList **)obj;
>
> +    if (error_is_set(errp)) {
> +        return;
> +    }
>       visit_start_list(m, name, errp);
>
>       for (*head = i = visit_next_list(m, head, errp); i; i = visit_next_list(m,&i, errp)) {
> @@ -112,6 +118,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>   {
>       Error *err = NULL;
>
> +    if (error_is_set(errp)) {
> +        return;
> +    }
>       visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s),&err);
>       visit_type_%(name)sKind(m,&(*obj)->kind, "type",&err);
>       if (err) {

[Qemu-devel] [PATCH 05/10] qapi: allow freeing partially-allocated objects

[Paolo Bonzini]

Objects going through the dealloc visitor can be only partially allocated.
Detect the situation and avoid a segfault.  This also helps with the
input visitor, when there are errors.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi-visit.py |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b242315..a85fb76 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -65,6 +65,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
         return;
     }
     visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
+    if (obj && !*obj) {
+        goto end;
+    }
 ''',
                 name=name)
     push_indent()
@@ -72,6 +75,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     pop_indent()
 
     ret += mcgen('''
+end:
     visit_end_struct(m, errp);
 }
 ''')
@@ -122,6 +126,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
         return;
     }
     visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+    if (obj && !*obj) {
+        goto end;
+    }
     visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
     if (err) {
         error_propagate(errp, err);
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 05/10] qapi: allow freeing partially-allocated objects

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> Objects going through the dealloc visitor can be only partially allocated.
> Detect the situation and avoid a segfault.  This also helps with the
> input visitor, when there are errors.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   scripts/qapi-visit.py |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index b242315..a85fb76 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -65,6 +65,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>           return;
>       }
>       visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
> +    if (obj&&  !*obj) {
> +        goto end;
> +    }
>   ''',
>                   name=name)
>       push_indent()
> @@ -72,6 +75,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>       pop_indent()
>
>       ret += mcgen('''
> +end:
>       visit_end_struct(m, errp);
>   }
>   ''')
> @@ -122,6 +126,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>           return;
>       }
>       visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s),&err);
> +    if (obj&&  !*obj) {
> +        goto end;
> +    }
>       visit_type_%(name)sKind(m,&(*obj)->kind, "type",&err);
>       if (err) {
>           error_propagate(errp, err);

[Qemu-devel] [PATCH 06/10] qapi: simplify qmp_input_next_list

[Paolo Bonzini]

Tweak a bit the code so that there is cleaner separation between
operation on the list and operation on the output.  The next patch
requires this change.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index ef9288f..dfc859a 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -64,9 +64,7 @@ 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;
-    if (qobject_type(obj) == QTYPE_QLIST) {
-        qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
-    }
+    qiv->stack[qiv->nb_stack].entry = NULL;
     qiv->nb_stack++;
 
     if (qiv->nb_stack >= QIV_STACK_SIZE) {
@@ -134,16 +132,17 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     StackObject *so = &qiv->stack[qiv->nb_stack - 1];
 
     if (so->entry == NULL) {
+        so->entry = qlist_first(qobject_to_qlist(so->obj));
+    } else {
+        so->entry = qlist_next(so->entry);
+    }
+
+    if (so->entry == NULL) {
         return NULL;
     }
 
     entry = g_malloc0(sizeof(*entry));
     if (*list) {
-        so->entry = qlist_next(so->entry);
-        if (so->entry == NULL) {
-            g_free(entry);
-            return NULL;
-        }
         (*list)->next = entry;
     }
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 06/10] qapi: simplify qmp_input_next_list

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> Tweak a bit the code so that there is cleaner separation between
> operation on the list and operation on the output.  The next patch
> requires this change.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

I've been staring a this patch for the past 5 minutes and I can't figure out 
what's going on here.

Maybe the code was too obscure to begin with.  Could you enhance the commit 
message a bit with what's going on here?

Regards,

Anthony Liguori

> ---
>   qapi/qmp-input-visitor.c |   15 +++++++--------
>   1 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index ef9288f..dfc859a 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -64,9 +64,7 @@ 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;
> -    if (qobject_type(obj) == QTYPE_QLIST) {
> -        qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
> -    }
> +    qiv->stack[qiv->nb_stack].entry = NULL;
>       qiv->nb_stack++;
>
>       if (qiv->nb_stack>= QIV_STACK_SIZE) {
> @@ -134,16 +132,17 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
>       StackObject *so =&qiv->stack[qiv->nb_stack - 1];
>
>       if (so->entry == NULL) {
> +        so->entry = qlist_first(qobject_to_qlist(so->obj));
> +    } else {
> +        so->entry = qlist_next(so->entry);
> +    }
> +
> +    if (so->entry == NULL) {
>           return NULL;
>       }
>
>       entry = g_malloc0(sizeof(*entry));
>       if (*list) {
> -        so->entry = qlist_next(so->entry);
> -        if (so->entry == NULL) {
> -            g_free(entry);
> -            return NULL;
> -        }
>           (*list)->next = entry;
>       }
>

Re: [Qemu-devel] [PATCH 06/10] qapi: simplify qmp_input_next_list

[Paolo Bonzini]

Il 22/03/2012 21:22, Anthony Liguori ha scritto:
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> 
> I've been staring a this patch for the past 5 minutes and I can't figure
> out what's going on here.
> 
> Maybe the code was too obscure to begin with.  Could you enhance the
> commit message a bit with what's going on here?

There's three possible questions about what's going on:

1) What's going on before the patch with so->entry

2) What's going on after the patch with so->entry

3) What's going on with *list.  The patch doesn't change this, it just
unties it with so->entry, but it's the most puzzling part so the
question is a good one. :)


First of all, so->entry here is advanced and also used to figure out
whether we have a next element.  It is then accessed in qmp_input_get_obj.

The caller must:

* call start_list

* call next_list for each element *including the first*

* on the first call to next_list, the result is the head of the list
(works for both input and output visitor).

Before: so->entry is initialized to qlist_first on start_list.  The
first call will have *list == NULL, so it skips the qlist_next.  The
caller assigns the result and makes *list not NULL, so that the next
iteration will advance so->entry and modify (*list)->next.

After: so->entry is initialized to NULL on start_list.  The first call
sees so->entry == NULL and does qlist_first; subsequent calls do
qlist_next.  *list is handled same as above: assignment done by the
caller on the first call, done by next_list on the next ones.


Thanks for making me write all this down, because it looks like there is
still room for further simplifying the handling of *list (for example,
do not have the caller write the list head).  I'll submit a v2.
Depending on the amount of simplification I'll document the result in
either a code comment or the commit message.

Luckily this code is very well tested.  It broke all the time for me. :P

Paolo

Re: [Qemu-devel] [PATCH 06/10] qapi: simplify qmp_input_next_list

[Anthony Liguori]

On 03/22/2012 04:24 PM, Paolo Bonzini wrote:
> Il 22/03/2012 21:22, Anthony Liguori ha scritto:
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>
>> I've been staring a this patch for the past 5 minutes and I can't figure
>> out what's going on here.
>>
>> Maybe the code was too obscure to begin with.  Could you enhance the
>> commit message a bit with what's going on here?
>
> There's three possible questions about what's going on:
>
> 1) What's going on before the patch with so->entry
>
> 2) What's going on after the patch with so->entry
>
> 3) What's going on with *list.  The patch doesn't change this, it just
> unties it with so->entry, but it's the most puzzling part so the
> question is a good one. :)
>
>
> First of all, so->entry here is advanced and also used to figure out
> whether we have a next element.  It is then accessed in qmp_input_get_obj.
>
> The caller must:
>
> * call start_list
>
> * call next_list for each element *including the first*
>
> * on the first call to next_list, the result is the head of the list
> (works for both input and output visitor).
>
> Before: so->entry is initialized to qlist_first on start_list.  The
> first call will have *list == NULL, so it skips the qlist_next.  The
> caller assigns the result and makes *list not NULL, so that the next
> iteration will advance so->entry and modify (*list)->next.
>
> After: so->entry is initialized to NULL on start_list.  The first call
> sees so->entry == NULL and does qlist_first; subsequent calls do
> qlist_next.  *list is handled same as above: assignment done by the
> caller on the first call, done by next_list on the next ones.
>
>
> Thanks for making me write all this down,

No problem :-)  I was really looking for the commit message breakdown.  I 
convinced myself that what you were doing was right but it took me sufficiently 
long that I thought it warranted a better commit message.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH v2 06/10] qapi: untangle next_list

[Paolo Bonzini]

Right now, the semantics of next_list are complicated.  The caller must:

* call start_list

* call next_list for each element *including the first*

* on the first call to next_list, the second argument should point to
NULL and the result is the head of the list.  On subsequent calls,
the second argument should point to the last node (last result of
next_list) and next_list itself tacks the element at the tail of the
list.

This works for both input and output visitor, but having the visitor
write memory when it is only reading the list is ugly.  Plus, relying
on *list to detect the first call is tricky and undocumented.

We can initialize so->entry in next_list instead of start_list, leaving
it NULL in start_list.  This way next_list sees clearly whether it is
on the first call---as a bonus, it discriminates the cases based on
internal state of the visitor rather than external state.  We can
also pull the assignment of the list head from generated code up to
next_list.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/qapi-code-gen.txt   |    4 ++--
 qapi/qmp-input-visitor.c |   22 +++++++++++++---------
 scripts/qapi-visit.py    |    4 ++--
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 5831e37..ad11767 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -194,11 +194,11 @@ Example:
 
     void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp)
     {
-        GenericList *i;
+        GenericList *i, **prev = (GenericList **)obj;
 
         visit_start_list(m, name, errp);
 
-        for (i = visit_next_list(m, (GenericList **)obj, errp); i; i = visit_next_list(m, &i, errp)) {
+        for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
             UserDefOneList *native_i = (UserDefOneList *)i;
             visit_type_UserDefOne(m, &native_i->value, NULL, errp);
         }
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index ef9288f..413e333 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -64,9 +64,7 @@ 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;
-    if (qobject_type(obj) == QTYPE_QLIST) {
-        qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
-    }
+    qiv->stack[qiv->nb_stack].entry = NULL;
     qiv->nb_stack++;
 
     if (qiv->nb_stack >= QIV_STACK_SIZE) {
@@ -132,18 +130,24 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     QmpInputVisitor *qiv = to_qiv(v);
     GenericList *entry;
     StackObject *so = &qiv->stack[qiv->nb_stack - 1];
+    bool first;
+
+    if (so->entry == NULL) {
+        so->entry = qlist_first(qobject_to_qlist(so->obj));
+        first = true;
+    } else {
+        so->entry = qlist_next(so->entry);
+        first = false;
+    }
 
     if (so->entry == NULL) {
         return NULL;
     }
 
     entry = g_malloc0(sizeof(*entry));
-    if (*list) {
-        so->entry = qlist_next(so->entry);
-        if (so->entry == NULL) {
-            g_free(entry);
-            return NULL;
-        }
+    if (first) {
+        *list = entry;
+    } else {
         (*list)->next = entry;
     }
 
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a85fb76..1ff81f4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -86,14 +86,14 @@ def generate_visit_list(name, members):
 
 void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
 {
-    GenericList *i, **head = (GenericList **)obj;
+    GenericList *i, **prev = (GenericList **)obj;
 
     if (error_is_set(errp)) {
         return;
     }
     visit_start_list(m, name, errp);
 
-    for (*head = i = visit_next_list(m, head, errp); i; i = visit_next_list(m, &i, errp)) {
+    for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
         %(name)sList *native_i = (%(name)sList *)i;
         visit_type_%(name)s(m, &native_i->value, NULL, errp);
     }
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH v2 06/10] qapi: untangle next_list

[Michael Roth]

On Thu, Mar 22, 2012 at 10:38:40PM +0100, Paolo Bonzini wrote:
> Right now, the semantics of next_list are complicated.  The caller must:
> 
> * call start_list
> 
> * call next_list for each element *including the first*
> 
> * on the first call to next_list, the second argument should point to
> NULL and the result is the head of the list.  On subsequent calls,
> the second argument should point to the last node (last result of
> next_list) and next_list itself tacks the element at the tail of the
> list.
> 
> This works for both input and output visitor, but having the visitor
> write memory when it is only reading the list is ugly.  Plus, relying
> on *list to detect the first call is tricky and undocumented.
> 
> We can initialize so->entry in next_list instead of start_list, leaving
> it NULL in start_list.  This way next_list sees clearly whether it is
> on the first call---as a bonus, it discriminates the cases based on
> internal state of the visitor rather than external state.  We can
> also pull the assignment of the list head from generated code up to
> next_list.

Nice cleanup. We'd originally moved assignment of the list head out of
next_list because we had no way of knowing whether it was the first call
at that point, so it was either always assign the current entry or
never, so we did the latter, but that complicated the calling code.

Now that we can make that distinction I think doing this change makes
sense.

Patch in general looks good as well.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  docs/qapi-code-gen.txt   |    4 ++--
>  qapi/qmp-input-visitor.c |   22 +++++++++++++---------
>  scripts/qapi-visit.py    |    4 ++--
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 5831e37..ad11767 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -194,11 +194,11 @@ Example:
> 
>      void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp)
>      {
> -        GenericList *i;
> +        GenericList *i, **prev = (GenericList **)obj;
> 
>          visit_start_list(m, name, errp);
> 
> -        for (i = visit_next_list(m, (GenericList **)obj, errp); i; i = visit_next_list(m, &i, errp)) {
> +        for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
>              UserDefOneList *native_i = (UserDefOneList *)i;
>              visit_type_UserDefOne(m, &native_i->value, NULL, errp);
>          }
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index ef9288f..413e333 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -64,9 +64,7 @@ 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;
> -    if (qobject_type(obj) == QTYPE_QLIST) {
> -        qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
> -    }
> +    qiv->stack[qiv->nb_stack].entry = NULL;
>      qiv->nb_stack++;
> 
>      if (qiv->nb_stack >= QIV_STACK_SIZE) {
> @@ -132,18 +130,24 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
>      QmpInputVisitor *qiv = to_qiv(v);
>      GenericList *entry;
>      StackObject *so = &qiv->stack[qiv->nb_stack - 1];
> +    bool first;
> +
> +    if (so->entry == NULL) {
> +        so->entry = qlist_first(qobject_to_qlist(so->obj));
> +        first = true;
> +    } else {
> +        so->entry = qlist_next(so->entry);
> +        first = false;
> +    }
> 
>      if (so->entry == NULL) {
>          return NULL;
>      }
> 
>      entry = g_malloc0(sizeof(*entry));
> -    if (*list) {
> -        so->entry = qlist_next(so->entry);
> -        if (so->entry == NULL) {
> -            g_free(entry);
> -            return NULL;
> -        }
> +    if (first) {
> +        *list = entry;
> +    } else {
>          (*list)->next = entry;
>      }
> 
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index a85fb76..1ff81f4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -86,14 +86,14 @@ def generate_visit_list(name, members):
> 
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>  {
> -    GenericList *i, **head = (GenericList **)obj;
> +    GenericList *i, **prev = (GenericList **)obj;
> 
>      if (error_is_set(errp)) {
>          return;
>      }
>      visit_start_list(m, name, errp);
> 
> -    for (*head = i = visit_next_list(m, head, errp); i; i = visit_next_list(m, &i, errp)) {
> +    for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
>          %(name)sList *native_i = (%(name)sList *)i;
>          visit_type_%(name)s(m, &native_i->value, NULL, errp);
>      }
> -- 
> 1.7.9.1
> 

[Qemu-devel] [PATCH 07/10] qapi: place outermost object on qiv stack

[Paolo Bonzini]

This is a slight change in the implementation of QMPInputVisitor
that helps when adding strict mode.

Const QObjects cannot be inc/decref-ed, and that's why QMPInputVisitor
relies heavily on weak references to inner objects.  I'm not removing
the weak references now, but since refcount+const is a lost battle in C
(C++ has "mutable") I think removing const is fine in this case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |   41 +++++++++++++++++------------------------
 1 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index dfc859a..97a0186 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -22,14 +22,13 @@
 
 typedef struct StackObject
 {
-    const QObject *obj;
-    const  QListEntry *entry;
+    QObject *obj;
+    const QListEntry *entry;
 } StackObject;
 
 struct QmpInputVisitor
 {
     Visitor visitor;
-    QObject *obj;
     StackObject stack[QIV_STACK_SIZE];
     int nb_stack;
 };
@@ -39,21 +38,15 @@ static QmpInputVisitor *to_qiv(Visitor *v)
     return container_of(v, QmpInputVisitor, visitor);
 }
 
-static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
-                                           const char *name)
+static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
+                                     const char *name)
 {
-    const QObject *qobj;
-
-    if (qiv->nb_stack == 0) {
-        qobj = qiv->obj;
-    } else {
-        qobj = qiv->stack[qiv->nb_stack - 1].obj;
-    }
+    QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
 
     if (qobj) {
         if (name && qobject_type(qobj) == QTYPE_QDICT) {
             return qdict_get(qobject_to_qdict(qobj), name);
-        } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
+        } else if (qiv->stack[qiv->nb_stack - 1].entry) {
             return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
         }
     }
@@ -61,7 +54,7 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
     return qobj;
 }
 
-static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **errp)
+static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
 {
     qiv->stack[qiv->nb_stack].obj = obj;
     qiv->stack[qiv->nb_stack].entry = NULL;
@@ -83,7 +76,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
                                    const char *name, size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
     Error *err = NULL;
 
     if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
@@ -113,7 +106,7 @@ static void qmp_input_end_struct(Visitor *v, Error **errp)
 static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -160,7 +153,7 @@ static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
                                Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -175,7 +168,7 @@ static void qmp_input_type_bool(Visitor *v, bool *obj, const char *name,
                                 Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj || qobject_type(qobj) != QTYPE_QBOOL) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -190,7 +183,7 @@ static void qmp_input_type_str(Visitor *v, char **obj, const char *name,
                                Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj || qobject_type(qobj) != QTYPE_QSTRING) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -205,7 +198,7 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
                                   Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -220,7 +213,7 @@ static void qmp_input_start_optional(Visitor *v, bool *present,
                                      const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    const QObject *qobj = qmp_input_get_object(qiv, name);
+    QObject *qobj = qmp_input_get_object(qiv, name);
 
     if (!qobj) {
         *present = false;
@@ -237,7 +230,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
 
 void qmp_input_visitor_cleanup(QmpInputVisitor *v)
 {
-    qobject_decref(v->obj);
+    qobject_decref(v->stack[0].obj);
     g_free(v);
 }
 
@@ -259,8 +252,8 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.type_number = qmp_input_type_number;
     v->visitor.start_optional = qmp_input_start_optional;
 
-    v->obj = obj;
-    qobject_incref(v->obj);
+    qmp_input_push(v, obj, NULL);
+    qobject_incref(obj);
 
     return v;
 }
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 07/10] qapi: place outermost object on qiv stack

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> This is a slight change in the implementation of QMPInputVisitor
> that helps when adding strict mode.
>
> Const QObjects cannot be inc/decref-ed, and that's why QMPInputVisitor
> relies heavily on weak references to inner objects.  I'm not removing
> the weak references now, but since refcount+const is a lost battle in C
> (C++ has "mutable") I think removing const is fine in this case.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

But I seem to recall that this was this way because some QMP function returns a 
const QObject... oh well, if things still compile with this patch, I'm happy 
with it.

Regards,

Anthony Liguori

> ---
>   qapi/qmp-input-visitor.c |   41 +++++++++++++++++------------------------
>   1 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index dfc859a..97a0186 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -22,14 +22,13 @@
>
>   typedef struct StackObject
>   {
> -    const QObject *obj;
> -    const  QListEntry *entry;
> +    QObject *obj;
> +    const QListEntry *entry;
>   } StackObject;
>
>   struct QmpInputVisitor
>   {
>       Visitor visitor;
> -    QObject *obj;
>       StackObject stack[QIV_STACK_SIZE];
>       int nb_stack;
>   };
> @@ -39,21 +38,15 @@ static QmpInputVisitor *to_qiv(Visitor *v)
>       return container_of(v, QmpInputVisitor, visitor);
>   }
>
> -static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
> -                                           const char *name)
> +static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
> +                                     const char *name)
>   {
> -    const QObject *qobj;
> -
> -    if (qiv->nb_stack == 0) {
> -        qobj = qiv->obj;
> -    } else {
> -        qobj = qiv->stack[qiv->nb_stack - 1].obj;
> -    }
> +    QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
>
>       if (qobj) {
>           if (name&&  qobject_type(qobj) == QTYPE_QDICT) {
>               return qdict_get(qobject_to_qdict(qobj), name);
> -        } else if (qiv->nb_stack>  0&&  qobject_type(qobj) == QTYPE_QLIST) {
> +        } else if (qiv->stack[qiv->nb_stack - 1].entry) {
>               return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
>           }
>       }
> @@ -61,7 +54,7 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>       return qobj;
>   }
>
> -static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error **errp)
> +static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>   {
>       qiv->stack[qiv->nb_stack].obj = obj;
>       qiv->stack[qiv->nb_stack].entry = NULL;
> @@ -83,7 +76,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
>                                      const char *name, size_t size, Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>       Error *err = NULL;
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
> @@ -113,7 +106,7 @@ static void qmp_input_end_struct(Visitor *v, Error **errp)
>   static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -160,7 +153,7 @@ static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
>                                  Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -175,7 +168,7 @@ static void qmp_input_type_bool(Visitor *v, bool *obj, const char *name,
>                                   Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QBOOL) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -190,7 +183,7 @@ static void qmp_input_type_str(Visitor *v, char **obj, const char *name,
>                                  Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QSTRING) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -205,7 +198,7 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
>                                     Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
>           error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -220,7 +213,7 @@ static void qmp_input_start_optional(Visitor *v, bool *present,
>                                        const char *name, Error **errp)
>   {
>       QmpInputVisitor *qiv = to_qiv(v);
> -    const QObject *qobj = qmp_input_get_object(qiv, name);
> +    QObject *qobj = qmp_input_get_object(qiv, name);
>
>       if (!qobj) {
>           *present = false;
> @@ -237,7 +230,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
>
>   void qmp_input_visitor_cleanup(QmpInputVisitor *v)
>   {
> -    qobject_decref(v->obj);
> +    qobject_decref(v->stack[0].obj);
>       g_free(v);
>   }
>
> @@ -259,8 +252,8 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>       v->visitor.type_number = qmp_input_type_number;
>       v->visitor.start_optional = qmp_input_start_optional;
>
> -    v->obj = obj;
> -    qobject_incref(v->obj);
> +    qmp_input_push(v, obj, NULL);
> +    qobject_incref(obj);
>
>       return v;
>   }

[Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor

[Paolo Bonzini]

While QMP in general is designed so that it is possible to ignore
unknown arguments, in the case of the QMP server it is better to
reject them to detect bad clients.  In fact, we're already doing
this at the top level in the argument checker.  To extend this to
complex structures, add a mode to the input visitor where it checks
for unvisited keys and raises an error if it finds one.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |   48 +++++++++-
 qapi/qmp-input-visitor.h |    2 +
 test-qmp-input-strict.c  |  234 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile           |    5 +-
 4 files changed, 285 insertions(+), 4 deletions(-)
 create mode 100644 test-qmp-input-strict.c

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 97a0186..eb09874 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -24,6 +24,7 @@ typedef struct StackObject
 {
     QObject *obj;
     const QListEntry *entry;
+    GHashTable *h;
 } StackObject;
 
 struct QmpInputVisitor
@@ -31,6 +32,7 @@ struct QmpInputVisitor
     Visitor visitor;
     StackObject stack[QIV_STACK_SIZE];
     int nb_stack;
+    bool strict;
 };
 
 static QmpInputVisitor *to_qiv(Visitor *v)
@@ -45,6 +47,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
 
     if (qobj) {
         if (name && qobject_type(qobj) == QTYPE_QDICT) {
+            if (qiv->stack[qiv->nb_stack - 1].h) {
+                g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
+            }
             return qdict_get(qobject_to_qdict(qobj), name);
         } else if (qiv->stack[qiv->nb_stack - 1].entry) {
             return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
@@ -54,20 +59,47 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
     return qobj;
 }
 
+static void qdict_add_key(const char *key, QObject *obj, void *opaque)
+{
+    GHashTable *h = opaque;
+    g_hash_table_insert(h, (gpointer) key, NULL);
+}
+
 static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
 {
-    qiv->stack[qiv->nb_stack].obj = obj;
-    qiv->stack[qiv->nb_stack].entry = NULL;
-    qiv->nb_stack++;
+    GHashTable *h;
 
     if (qiv->nb_stack >= QIV_STACK_SIZE) {
         error_set(errp, QERR_BUFFER_OVERRUN);
         return;
     }
+
+    qiv->stack[qiv->nb_stack].obj = obj;
+    qiv->stack[qiv->nb_stack].entry = NULL;
+    qiv->stack[qiv->nb_stack].h = NULL;
+
+    if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
+        h = g_hash_table_new(g_str_hash, g_str_equal);
+        qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
+        qiv->stack[qiv->nb_stack].h = h;
+    }
+
+    qiv->nb_stack++;
 }
 
 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
+    GHashTableIter iter;
+    gpointer key;
+
+    if (qiv->strict && qiv->stack[qiv->nb_stack - 1].h) {
+        g_hash_table_iter_init(&iter, qiv->stack[qiv->nb_stack - 1].h);
+        if (g_hash_table_iter_next(&iter, &key, NULL)) {
+            error_set(errp, QERR_QMP_EXTRA_MEMBER, (char *) key);
+        }
+        g_hash_table_unref(qiv->stack[qiv->nb_stack - 1].h);
+    }
+
     assert(qiv->nb_stack > 0);
     qiv->nb_stack--;
 }
@@ -257,3 +289,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
 
     return v;
 }
+
+QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj)
+{
+    QmpInputVisitor *v;
+
+    v = qmp_input_visitor_new(obj);
+    v->strict = true;
+
+    return v;
+}
diff --git a/qapi/qmp-input-visitor.h b/qapi/qmp-input-visitor.h
index 3f798f0..e0a48a5 100644
--- a/qapi/qmp-input-visitor.h
+++ b/qapi/qmp-input-visitor.h
@@ -20,6 +20,8 @@
 typedef struct QmpInputVisitor QmpInputVisitor;
 
 QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
+QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
+
 void qmp_input_visitor_cleanup(QmpInputVisitor *v);
 
 Visitor *qmp_input_get_visitor(QmpInputVisitor *v);
diff --git a/test-qmp-input-strict.c b/test-qmp-input-strict.c
new file mode 100644
index 0000000..f6df8cb
--- /dev/null
+++ b/test-qmp-input-strict.c
@@ -0,0 +1,234 @@
+/*
+ * QMP Input Visitor unit-tests (strict mode).
+ *
+ * Copyright (C) 2011-2012 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <stdarg.h>
+
+#include "qapi/qmp-input-visitor.h"
+#include "test-qapi-types.h"
+#include "test-qapi-visit.h"
+#include "qemu-objects.h"
+
+typedef struct TestInputVisitorData {
+    QObject *obj;
+    QmpInputVisitor *qiv;
+} TestInputVisitorData;
+
+static void validate_teardown(TestInputVisitorData *data,
+                               const void *unused)
+{
+    qobject_decref(data->obj);
+    data->obj = NULL;
+
+    if (data->qiv) {
+        qmp_input_visitor_cleanup(data->qiv);
+        data->qiv = NULL;
+    }
+}
+
+/* This is provided instead of a test setup function so that the JSON
+   string used by the tests are kept in the test functions (and not
+   int main()) */
+static GCC_FMT_ATTR(2, 3)
+Visitor *validate_test_init(TestInputVisitorData *data,
+                             const char *json_string, ...)
+{
+    Visitor *v;
+    va_list ap;
+
+    va_start(ap, json_string);
+    data->obj = qobject_from_jsonv(json_string, &ap);
+    va_end(ap);
+
+    g_assert(data->obj != NULL);
+
+    data->qiv = qmp_input_visitor_new_strict(data->obj);
+    g_assert(data->qiv != NULL);
+
+    v = qmp_input_get_visitor(data->qiv);
+    g_assert(v != NULL);
+
+    return v;
+}
+
+typedef struct TestStruct
+{
+    int64_t integer;
+    bool boolean;
+    char *string;
+} TestStruct;
+
+static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
+                                  const char *name, Error **errp)
+{
+    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
+                       errp);
+
+    visit_type_int(v, &(*obj)->integer, "integer", errp);
+    visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
+    visit_type_str(v, &(*obj)->string, "string", errp);
+
+    visit_end_struct(v, errp);
+}
+
+static void test_validate_struct(TestInputVisitorData *data,
+                                  const void *unused)
+{
+    TestStruct *p = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
+
+    visit_type_TestStruct(v, &p, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    g_free(p->string);
+    g_free(p);
+}
+
+static void test_validate_struct_nested(TestInputVisitorData *data,
+                                         const void *unused)
+{
+    UserDefNested *udp = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string' }, 'string2': 'string2'}}}");
+
+    visit_type_UserDefNested(v, &udp, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    qapi_free_UserDefNested(udp);
+}
+
+static void test_validate_list(TestInputVisitorData *data,
+                                const void *unused)
+{
+    UserDefOneList *head = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 } ]");
+
+    visit_type_UserDefOneList(v, &head, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    qapi_free_UserDefOneList(head);
+}
+
+static void test_validate_union(TestInputVisitorData *data,
+                                 const void *unused)
+{
+    UserDefUnion *tmp = NULL;
+    Visitor *v;
+    Error *errp = NULL;
+
+    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
+
+    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    qapi_free_UserDefUnion(tmp);
+}
+
+static void test_validate_fail_struct(TestInputVisitorData *data,
+                                       const void *unused)
+{
+    TestStruct *p = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
+
+    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_validate_fail_struct_nested(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    UserDefNested *udp = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
+
+    visit_type_UserDefNested(v, &udp, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    qapi_free_UserDefNested(udp);
+}
+
+static void test_validate_fail_list(TestInputVisitorData *data,
+                                     const void *unused)
+{
+    UserDefOneList *head = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
+
+    visit_type_UserDefOneList(v, &head, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    qapi_free_UserDefOneList(head);
+}
+
+static void test_validate_fail_union(TestInputVisitorData *data,
+                                      const void *unused)
+{
+    UserDefUnion *tmp = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 }, 'extra': 'yyy' }");
+
+    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    qapi_free_UserDefUnion(tmp);
+}
+
+static void validate_test_add(const char *testpath,
+                               TestInputVisitorData *data,
+                               void (*test_func)(TestInputVisitorData *data, const void *user_data))
+{
+    g_test_add(testpath, TestInputVisitorData, data, NULL, test_func,
+               validate_teardown);
+}
+
+int main(int argc, char **argv)
+{
+    TestInputVisitorData testdata;
+
+    g_test_init(&argc, &argv, NULL);
+
+    validate_test_add("/visitor/input-strict/pass/struct",
+                       &testdata, test_validate_struct);
+    validate_test_add("/visitor/input-strict/pass/struct-nested",
+                       &testdata, test_validate_struct_nested);
+    validate_test_add("/visitor/input-strict/pass/list",
+                       &testdata, test_validate_list);
+    validate_test_add("/visitor/input-strict/pass/union",
+                       &testdata, test_validate_union);
+    validate_test_add("/visitor/input-strict/fail/struct",
+                       &testdata, test_validate_fail_struct);
+    validate_test_add("/visitor/input-strict/fail/struct-nested",
+                       &testdata, test_validate_fail_struct_nested);
+    validate_test_add("/visitor/input-strict/fail/list",
+                       &testdata, test_validate_fail_list);
+    validate_test_add("/visitor/input-strict/fail/union",
+                       &testdata, test_validate_fail_union);
+
+    g_test_run();
+
+    return 0;
+}
diff --git a/tests/Makefile b/tests/Makefile
index c78ade1..31349f7 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -15,7 +15,7 @@ check-qfloat: check-qfloat.o qfloat.o $(tools-obj-y)
 check-qjson: check-qjson.o $(qobject-obj-y) $(tools-obj-y)
 test-coroutine: test-coroutine.o qemu-timer-common.o async.o $(coroutine-obj-y) $(tools-obj-y)
 
-test-qmp-input-visitor.o test-qmp-output-visitor.o \
+test-qmp-input-visitor.o test-qmp-output-visitor.o test-qmp-input-strict.o \
 test-string-input-visitor.o test-string-output-visitor.o \
 	test-qmp-commands.o qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)
 
@@ -36,6 +36,9 @@ test-string-output-visitor: test-string-output-visitor.o $(qobject-obj-y) $(qapi
 test-string-input-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
 test-string-input-visitor: test-string-input-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
 
+test-qmp-input-strict.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
+test-qmp-input-strict: test-qmp-input-strict.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
+
 test-qmp-output-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
 test-qmp-output-visitor: test-qmp-output-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> While QMP in general is designed so that it is possible to ignore
> unknown arguments, in the case of the QMP server it is better to
> reject them to detect bad clients.  In fact, we're already doing
> this at the top level in the argument checker.  To extend this to
> complex structures, add a mode to the input visitor where it checks
> for unvisited keys and raises an error if it finds one.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

We need to document this behavior in QMP/qmp-spec.txt.

Regards,

Anthony Liguori

> ---
>   qapi/qmp-input-visitor.c |   48 +++++++++-
>   qapi/qmp-input-visitor.h |    2 +
>   test-qmp-input-strict.c  |  234 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/Makefile           |    5 +-
>   4 files changed, 285 insertions(+), 4 deletions(-)
>   create mode 100644 test-qmp-input-strict.c
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 97a0186..eb09874 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -24,6 +24,7 @@ typedef struct StackObject
>   {
>       QObject *obj;
>       const QListEntry *entry;
> +    GHashTable *h;
>   } StackObject;
>
>   struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
>       Visitor visitor;
>       StackObject stack[QIV_STACK_SIZE];
>       int nb_stack;
> +    bool strict;
>   };
>
>   static QmpInputVisitor *to_qiv(Visitor *v)
> @@ -45,6 +47,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>
>       if (qobj) {
>           if (name&&  qobject_type(qobj) == QTYPE_QDICT) {
> +            if (qiv->stack[qiv->nb_stack - 1].h) {
> +                g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
> +            }
>               return qdict_get(qobject_to_qdict(qobj), name);
>           } else if (qiv->stack[qiv->nb_stack - 1].entry) {
>               return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> @@ -54,20 +59,47 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>       return qobj;
>   }
>
> +static void qdict_add_key(const char *key, QObject *obj, void *opaque)
> +{
> +    GHashTable *h = opaque;
> +    g_hash_table_insert(h, (gpointer) key, NULL);
> +}
> +
>   static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>   {
> -    qiv->stack[qiv->nb_stack].obj = obj;
> -    qiv->stack[qiv->nb_stack].entry = NULL;
> -    qiv->nb_stack++;
> +    GHashTable *h;
>
>       if (qiv->nb_stack>= QIV_STACK_SIZE) {
>           error_set(errp, QERR_BUFFER_OVERRUN);
>           return;
>       }
> +
> +    qiv->stack[qiv->nb_stack].obj = obj;
> +    qiv->stack[qiv->nb_stack].entry = NULL;
> +    qiv->stack[qiv->nb_stack].h = NULL;
> +
> +    if (qiv->strict&&  qobject_type(obj) == QTYPE_QDICT) {
> +        h = g_hash_table_new(g_str_hash, g_str_equal);
> +        qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
> +        qiv->stack[qiv->nb_stack].h = h;
> +    }
> +
> +    qiv->nb_stack++;
>   }
>
>   static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>   {
> +    GHashTableIter iter;
> +    gpointer key;
> +
> +    if (qiv->strict&&  qiv->stack[qiv->nb_stack - 1].h) {
> +        g_hash_table_iter_init(&iter, qiv->stack[qiv->nb_stack - 1].h);
> +        if (g_hash_table_iter_next(&iter,&key, NULL)) {
> +            error_set(errp, QERR_QMP_EXTRA_MEMBER, (char *) key);
> +        }
> +        g_hash_table_unref(qiv->stack[qiv->nb_stack - 1].h);
> +    }
> +
>       assert(qiv->nb_stack>  0);
>       qiv->nb_stack--;
>   }
> @@ -257,3 +289,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>       return v;
>   }
> +
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj)
> +{
> +    QmpInputVisitor *v;
> +
> +    v = qmp_input_visitor_new(obj);
> +    v->strict = true;
> +
> +    return v;
> +}
> diff --git a/qapi/qmp-input-visitor.h b/qapi/qmp-input-visitor.h
> index 3f798f0..e0a48a5 100644
> --- a/qapi/qmp-input-visitor.h
> +++ b/qapi/qmp-input-visitor.h
> @@ -20,6 +20,8 @@
>   typedef struct QmpInputVisitor QmpInputVisitor;
>
>   QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
> +
>   void qmp_input_visitor_cleanup(QmpInputVisitor *v);
>
>   Visitor *qmp_input_get_visitor(QmpInputVisitor *v);
> diff --git a/test-qmp-input-strict.c b/test-qmp-input-strict.c
> new file mode 100644
> index 0000000..f6df8cb
> --- /dev/null
> +++ b/test-qmp-input-strict.c
> @@ -0,0 +1,234 @@
> +/*
> + * QMP Input Visitor unit-tests (strict mode).
> + *
> + * Copyright (C) 2011-2012 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino<lcapitulino@redhat.com>
> + *  Paolo Bonzini<pbonzini@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include<glib.h>
> +#include<stdarg.h>
> +
> +#include "qapi/qmp-input-visitor.h"
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
> +#include "qemu-objects.h"
> +
> +typedef struct TestInputVisitorData {
> +    QObject *obj;
> +    QmpInputVisitor *qiv;
> +} TestInputVisitorData;
> +
> +static void validate_teardown(TestInputVisitorData *data,
> +                               const void *unused)
> +{
> +    qobject_decref(data->obj);
> +    data->obj = NULL;
> +
> +    if (data->qiv) {
> +        qmp_input_visitor_cleanup(data->qiv);
> +        data->qiv = NULL;
> +    }
> +}
> +
> +/* This is provided instead of a test setup function so that the JSON
> +   string used by the tests are kept in the test functions (and not
> +   int main()) */
> +static GCC_FMT_ATTR(2, 3)
> +Visitor *validate_test_init(TestInputVisitorData *data,
> +                             const char *json_string, ...)
> +{
> +    Visitor *v;
> +    va_list ap;
> +
> +    va_start(ap, json_string);
> +    data->obj = qobject_from_jsonv(json_string,&ap);
> +    va_end(ap);
> +
> +    g_assert(data->obj != NULL);
> +
> +    data->qiv = qmp_input_visitor_new_strict(data->obj);
> +    g_assert(data->qiv != NULL);
> +
> +    v = qmp_input_get_visitor(data->qiv);
> +    g_assert(v != NULL);
> +
> +    return v;
> +}
> +
> +typedef struct TestStruct
> +{
> +    int64_t integer;
> +    bool boolean;
> +    char *string;
> +} TestStruct;
> +
> +static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
> +                                  const char *name, Error **errp)
> +{
> +    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> +                       errp);
> +
> +    visit_type_int(v,&(*obj)->integer, "integer", errp);
> +    visit_type_bool(v,&(*obj)->boolean, "boolean", errp);
> +    visit_type_str(v,&(*obj)->string, "string", errp);
> +
> +    visit_end_struct(v, errp);
> +}
> +
> +static void test_validate_struct(TestInputVisitorData *data,
> +                                  const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
> +
> +    visit_type_TestStruct(v,&p, NULL,&errp);
> +    g_assert(!error_is_set(&errp));
> +    g_free(p->string);
> +    g_free(p);
> +}
> +
> +static void test_validate_struct_nested(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string' }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v,&udp, NULL,&errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_list(TestInputVisitorData *data,
> +                                const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 } ]");
> +
> +    visit_type_UserDefOneList(v,&head, NULL,&errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_union(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Visitor *v;
> +    Error *errp = NULL;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
> +
> +    visit_type_UserDefUnion(v,&tmp, NULL,&errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void test_validate_fail_struct(TestInputVisitorData *data,
> +                                       const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
> +
> +    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_validate_fail_struct_nested(TestInputVisitorData *data,
> +                                              const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v,&udp, NULL,&errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_fail_list(TestInputVisitorData *data,
> +                                     const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
> +
> +    visit_type_UserDefOneList(v,&head, NULL,&errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_fail_union(TestInputVisitorData *data,
> +                                      const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 }, 'extra': 'yyy' }");
> +
> +    visit_type_UserDefUnion(v,&tmp, NULL,&errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void validate_test_add(const char *testpath,
> +                               TestInputVisitorData *data,
> +                               void (*test_func)(TestInputVisitorData *data, const void *user_data))
> +{
> +    g_test_add(testpath, TestInputVisitorData, data, NULL, test_func,
> +               validate_teardown);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    TestInputVisitorData testdata;
> +
> +    g_test_init(&argc,&argv, NULL);
> +
> +    validate_test_add("/visitor/input-strict/pass/struct",
> +&testdata, test_validate_struct);
> +    validate_test_add("/visitor/input-strict/pass/struct-nested",
> +&testdata, test_validate_struct_nested);
> +    validate_test_add("/visitor/input-strict/pass/list",
> +&testdata, test_validate_list);
> +    validate_test_add("/visitor/input-strict/pass/union",
> +&testdata, test_validate_union);
> +    validate_test_add("/visitor/input-strict/fail/struct",
> +&testdata, test_validate_fail_struct);
> +    validate_test_add("/visitor/input-strict/fail/struct-nested",
> +&testdata, test_validate_fail_struct_nested);
> +    validate_test_add("/visitor/input-strict/fail/list",
> +&testdata, test_validate_fail_list);
> +    validate_test_add("/visitor/input-strict/fail/union",
> +&testdata, test_validate_fail_union);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> diff --git a/tests/Makefile b/tests/Makefile
> index c78ade1..31349f7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,7 +15,7 @@ check-qfloat: check-qfloat.o qfloat.o $(tools-obj-y)
>   check-qjson: check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>   test-coroutine: test-coroutine.o qemu-timer-common.o async.o $(coroutine-obj-y) $(tools-obj-y)
>
> -test-qmp-input-visitor.o test-qmp-output-visitor.o \
> +test-qmp-input-visitor.o test-qmp-output-visitor.o test-qmp-input-strict.o \
>   test-string-input-visitor.o test-string-output-visitor.o \
>   	test-qmp-commands.o qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)
>
> @@ -36,6 +36,9 @@ test-string-output-visitor: test-string-output-visitor.o $(qobject-obj-y) $(qapi
>   test-string-input-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>   test-string-input-visitor: test-string-input-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>
> +test-qmp-input-strict.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
> +test-qmp-input-strict: test-qmp-input-strict.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> +
>   test-qmp-output-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>   test-qmp-output-visitor: test-qmp-output-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>

Re: [Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor

[Laurent Desnogues]

Hello,

On Thu, Mar 22, 2012 at 12:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> While QMP in general is designed so that it is possible to ignore
> unknown arguments, in the case of the QMP server it is better to
> reject them to detect bad clients.  In fact, we're already doing
> this at the top level in the argument checker.  To extend this to
> complex structures, add a mode to the input visitor where it checks
> for unvisited keys and raises an error if it finds one.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/qmp-input-visitor.c |   48 +++++++++-
>  qapi/qmp-input-visitor.h |    2 +
>  test-qmp-input-strict.c  |  234 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile           |    5 +-
>  4 files changed, 285 insertions(+), 4 deletions(-)
>  create mode 100644 test-qmp-input-strict.c
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 97a0186..eb09874 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -24,6 +24,7 @@ typedef struct StackObject
>  {
>     QObject *obj;
>     const QListEntry *entry;
> +    GHashTable *h;
>  } StackObject;
>
>  struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
>     Visitor visitor;
>     StackObject stack[QIV_STACK_SIZE];
>     int nb_stack;
> +    bool strict;
>  };
>
>  static QmpInputVisitor *to_qiv(Visitor *v)
> @@ -45,6 +47,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>
>     if (qobj) {
>         if (name && qobject_type(qobj) == QTYPE_QDICT) {
> +            if (qiv->stack[qiv->nb_stack - 1].h) {
> +                g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
> +            }
>             return qdict_get(qobject_to_qdict(qobj), name);
>         } else if (qiv->stack[qiv->nb_stack - 1].entry) {
>             return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> @@ -54,20 +59,47 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>     return qobj;
>  }
>
> +static void qdict_add_key(const char *key, QObject *obj, void *opaque)
> +{
> +    GHashTable *h = opaque;
> +    g_hash_table_insert(h, (gpointer) key, NULL);
> +}
> +
>  static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>  {
> -    qiv->stack[qiv->nb_stack].obj = obj;
> -    qiv->stack[qiv->nb_stack].entry = NULL;
> -    qiv->nb_stack++;
> +    GHashTable *h;
>
>     if (qiv->nb_stack >= QIV_STACK_SIZE) {
>         error_set(errp, QERR_BUFFER_OVERRUN);
>         return;
>     }
> +
> +    qiv->stack[qiv->nb_stack].obj = obj;
> +    qiv->stack[qiv->nb_stack].entry = NULL;
> +    qiv->stack[qiv->nb_stack].h = NULL;
> +
> +    if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> +        h = g_hash_table_new(g_str_hash, g_str_equal);
> +        qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
> +        qiv->stack[qiv->nb_stack].h = h;
> +    }
> +
> +    qiv->nb_stack++;
>  }
>
>  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>  {
> +    GHashTableIter iter;

GHashTableIter is alas not available in the glib (2.12) that
the distros we use at work run.  Is there a workaround for
this issue?

Thanks,

Laurent

> +    gpointer key;
> +
> +    if (qiv->strict && qiv->stack[qiv->nb_stack - 1].h) {
> +        g_hash_table_iter_init(&iter, qiv->stack[qiv->nb_stack - 1].h);
> +        if (g_hash_table_iter_next(&iter, &key, NULL)) {
> +            error_set(errp, QERR_QMP_EXTRA_MEMBER, (char *) key);
> +        }
> +        g_hash_table_unref(qiv->stack[qiv->nb_stack - 1].h);
> +    }
> +
>     assert(qiv->nb_stack > 0);
>     qiv->nb_stack--;
>  }
> @@ -257,3 +289,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>     return v;
>  }
> +
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj)
> +{
> +    QmpInputVisitor *v;
> +
> +    v = qmp_input_visitor_new(obj);
> +    v->strict = true;
> +
> +    return v;
> +}
> diff --git a/qapi/qmp-input-visitor.h b/qapi/qmp-input-visitor.h
> index 3f798f0..e0a48a5 100644
> --- a/qapi/qmp-input-visitor.h
> +++ b/qapi/qmp-input-visitor.h
> @@ -20,6 +20,8 @@
>  typedef struct QmpInputVisitor QmpInputVisitor;
>
>  QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
> +
>  void qmp_input_visitor_cleanup(QmpInputVisitor *v);
>
>  Visitor *qmp_input_get_visitor(QmpInputVisitor *v);
> diff --git a/test-qmp-input-strict.c b/test-qmp-input-strict.c
> new file mode 100644
> index 0000000..f6df8cb
> --- /dev/null
> +++ b/test-qmp-input-strict.c
> @@ -0,0 +1,234 @@
> +/*
> + * QMP Input Visitor unit-tests (strict mode).
> + *
> + * Copyright (C) 2011-2012 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <lcapitulino@redhat.com>
> + *  Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <glib.h>
> +#include <stdarg.h>
> +
> +#include "qapi/qmp-input-visitor.h"
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
> +#include "qemu-objects.h"
> +
> +typedef struct TestInputVisitorData {
> +    QObject *obj;
> +    QmpInputVisitor *qiv;
> +} TestInputVisitorData;
> +
> +static void validate_teardown(TestInputVisitorData *data,
> +                               const void *unused)
> +{
> +    qobject_decref(data->obj);
> +    data->obj = NULL;
> +
> +    if (data->qiv) {
> +        qmp_input_visitor_cleanup(data->qiv);
> +        data->qiv = NULL;
> +    }
> +}
> +
> +/* This is provided instead of a test setup function so that the JSON
> +   string used by the tests are kept in the test functions (and not
> +   int main()) */
> +static GCC_FMT_ATTR(2, 3)
> +Visitor *validate_test_init(TestInputVisitorData *data,
> +                             const char *json_string, ...)
> +{
> +    Visitor *v;
> +    va_list ap;
> +
> +    va_start(ap, json_string);
> +    data->obj = qobject_from_jsonv(json_string, &ap);
> +    va_end(ap);
> +
> +    g_assert(data->obj != NULL);
> +
> +    data->qiv = qmp_input_visitor_new_strict(data->obj);
> +    g_assert(data->qiv != NULL);
> +
> +    v = qmp_input_get_visitor(data->qiv);
> +    g_assert(v != NULL);
> +
> +    return v;
> +}
> +
> +typedef struct TestStruct
> +{
> +    int64_t integer;
> +    bool boolean;
> +    char *string;
> +} TestStruct;
> +
> +static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
> +                                  const char *name, Error **errp)
> +{
> +    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> +                       errp);
> +
> +    visit_type_int(v, &(*obj)->integer, "integer", errp);
> +    visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
> +    visit_type_str(v, &(*obj)->string, "string", errp);
> +
> +    visit_end_struct(v, errp);
> +}
> +
> +static void test_validate_struct(TestInputVisitorData *data,
> +                                  const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
> +
> +    visit_type_TestStruct(v, &p, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    g_free(p->string);
> +    g_free(p);
> +}
> +
> +static void test_validate_struct_nested(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string' }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v, &udp, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_list(TestInputVisitorData *data,
> +                                const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 } ]");
> +
> +    visit_type_UserDefOneList(v, &head, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_union(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Visitor *v;
> +    Error *errp = NULL;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
> +
> +    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void test_validate_fail_struct(TestInputVisitorData *data,
> +                                       const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
> +
> +    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_validate_fail_struct_nested(TestInputVisitorData *data,
> +                                              const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v, &udp, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_fail_list(TestInputVisitorData *data,
> +                                     const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
> +
> +    visit_type_UserDefOneList(v, &head, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_fail_union(TestInputVisitorData *data,
> +                                      const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 }, 'extra': 'yyy' }");
> +
> +    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void validate_test_add(const char *testpath,
> +                               TestInputVisitorData *data,
> +                               void (*test_func)(TestInputVisitorData *data, const void *user_data))
> +{
> +    g_test_add(testpath, TestInputVisitorData, data, NULL, test_func,
> +               validate_teardown);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    TestInputVisitorData testdata;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    validate_test_add("/visitor/input-strict/pass/struct",
> +                       &testdata, test_validate_struct);
> +    validate_test_add("/visitor/input-strict/pass/struct-nested",
> +                       &testdata, test_validate_struct_nested);
> +    validate_test_add("/visitor/input-strict/pass/list",
> +                       &testdata, test_validate_list);
> +    validate_test_add("/visitor/input-strict/pass/union",
> +                       &testdata, test_validate_union);
> +    validate_test_add("/visitor/input-strict/fail/struct",
> +                       &testdata, test_validate_fail_struct);
> +    validate_test_add("/visitor/input-strict/fail/struct-nested",
> +                       &testdata, test_validate_fail_struct_nested);
> +    validate_test_add("/visitor/input-strict/fail/list",
> +                       &testdata, test_validate_fail_list);
> +    validate_test_add("/visitor/input-strict/fail/union",
> +                       &testdata, test_validate_fail_union);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> diff --git a/tests/Makefile b/tests/Makefile
> index c78ade1..31349f7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,7 +15,7 @@ check-qfloat: check-qfloat.o qfloat.o $(tools-obj-y)
>  check-qjson: check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>  test-coroutine: test-coroutine.o qemu-timer-common.o async.o $(coroutine-obj-y) $(tools-obj-y)
>
> -test-qmp-input-visitor.o test-qmp-output-visitor.o \
> +test-qmp-input-visitor.o test-qmp-output-visitor.o test-qmp-input-strict.o \
>  test-string-input-visitor.o test-string-output-visitor.o \
>        test-qmp-commands.o qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)
>
> @@ -36,6 +36,9 @@ test-string-output-visitor: test-string-output-visitor.o $(qobject-obj-y) $(qapi
>  test-string-input-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>  test-string-input-visitor: test-string-input-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>
> +test-qmp-input-strict.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
> +test-qmp-input-strict: test-qmp-input-strict.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> +
>  test-qmp-output-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>  test-qmp-output-visitor: test-qmp-output-visitor.o $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>
> --
> 1.7.9.1
>
>
>

Re: [Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor

[Paolo Bonzini]

Il 02/04/2012 12:34, Laurent Desnogues ha scritto:
>> >  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>> >  {
>> > +    GHashTableIter iter;
> GHashTableIter is alas not available in the glib (2.12) that
> the distros we use at work run.  Is there a workaround for
> this issue?

Yeah, since the hash table is going to be destroyed anyway, and will
always be empty in the common case, you can call
g_hash_table_foreach_remove, and raise an error if it returns > 0

Something like

gboolean hash_table_empty_check(gpointer key, gpointer value,
                                gpointer user_data)
{
    gpointer *p_key = user_data;
    if (!*p_key) {
        *p_key = key;
    }
    return true;
}

....

    key = NULL;
    if (g_hash_table_foreach_remove(qiv->stack[qiv->nb_stack - 1].h,
                                    hash_table_empty_check, &key) {
        error_set(errp, QERR_QMP_EXTRA_MEMBER, (char *) key);
    }
    g_hash_table_unref(qiv->stack[qiv->nb_stack - 1].h);

Can you make a patch?

Paolo

[Qemu-devel] [PATCH 09/10] qmp: add and use q type specifier

[Paolo Bonzini]

"O" is being used by the transaction and qom-set commands to mean "any
QObject", but it really means "do not validate the argument list".
Add a new specifier with the correct meaning.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c       |    3 +++
 qmp-commands.hx |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2ff1e0b..8946a10 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4157,6 +4157,9 @@ static int check_client_args_type(const QDict *client_args,
         case 'O':
             assert(flags & QMP_ACCEPT_UNKNOWNS);
             break;
+        case 'q':
+            /* Any QObject can be passed.  */
+            break;
         case '/':
         case '.':
             /*
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c626ba8..9447871 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -708,7 +708,7 @@ EQMP
     },
     {
         .name       = "transaction",
-        .args_type  = "actions:O",
+        .args_type  = "actions:q",
         .mhandler.cmd_new = qmp_marshal_input_transaction,
     },
 
@@ -2125,7 +2125,7 @@ EQMP
 
     {
         .name       = "qom-set",
-	.args_type  = "path:s,property:s,opts:O",
+	.args_type  = "path:s,property:s,value:q",
 	.mhandler.cmd_new = qmp_qom_set,
     },
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 09/10] qmp: add and use q type specifier

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> "O" is being used by the transaction and qom-set commands to mean "any
> QObject", but it really means "do not validate the argument list".
> Add a new specifier with the correct meaning.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   monitor.c       |    3 +++
>   qmp-commands.hx |    4 ++--
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 2ff1e0b..8946a10 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4157,6 +4157,9 @@ static int check_client_args_type(const QDict *client_args,
>           case 'O':
>               assert(flags&  QMP_ACCEPT_UNKNOWNS);
>               break;
> +        case 'q':
> +            /* Any QObject can be passed.  */
> +            break;
>           case '/':
>           case '.':
>               /*
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c626ba8..9447871 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -708,7 +708,7 @@ EQMP
>       },
>       {
>           .name       = "transaction",
> -        .args_type  = "actions:O",
> +        .args_type  = "actions:q",
>           .mhandler.cmd_new = qmp_marshal_input_transaction,
>       },
>
> @@ -2125,7 +2125,7 @@ EQMP
>
>       {
>           .name       = "qom-set",
> -	.args_type  = "path:s,property:s,opts:O",
> +	.args_type  = "path:s,property:s,value:q",
>   	.mhandler.cmd_new = qmp_qom_set,
>       },
>

[Qemu-devel] [PATCH 10/10] qmp: parse commands in strict mode

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi-commands.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 3aabf61..4774b61 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -140,7 +140,7 @@ v = qapi_dealloc_get_visitor(md);
 ''')
     else:
         ret += mcgen('''
-mi = qmp_input_visitor_new(%(obj)s);
+mi = qmp_input_visitor_new_strict(%(obj)s);
 v = qmp_input_get_visitor(mi);
 ''',
                      obj=obj)
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 10/10] qmp: parse commands in strict mode

[Anthony Liguori]

On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Looks good overall.  I think documentation is the only real issue.

I assume this will go through Luiz's tree?

Regards,

Anthony Liguori

> ---
>   scripts/qapi-commands.py |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 3aabf61..4774b61 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -140,7 +140,7 @@ v = qapi_dealloc_get_visitor(md);
>   ''')
>       else:
>           ret += mcgen('''
> -mi = qmp_input_visitor_new(%(obj)s);
> +mi = qmp_input_visitor_new_strict(%(obj)s);
>   v = qmp_input_get_visitor(mi);
>   ''',
>                        obj=obj)

Re: [Qemu-devel] [PATCH 10/10] qmp: parse commands in strict mode

[Luiz Capitulino]

On Thu, 22 Mar 2012 15:28:00 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/22/2012 06:51 AM, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> 
> Reviewed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Looks good overall.  I think documentation is the only real issue.
> 
> I assume this will go through Luiz's tree?

Yeah, but I'll only have time to pick them up tomorrow.

[Qemu-devel] [PATCH 11/10] qmp: document strict parsing

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 QMP/qmp-spec.txt |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 9d30a8c..1ba916c 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -209,13 +209,27 @@ incompatible way are disabled by default and will be advertised by the
 capabilities array (section '2.2 Server Greeting'). Thus, Clients can check
 that array and enable the capabilities they support.
 
-Additionally, Clients must not assume any particular:
-
-- Size of json-objects or length of json-arrays
+The QMP Server performs a type check on the arguments to a command.  It
+generates an error if a value does not have the expected type for its
+key, or if it does not understand a key that the Client included.  The
+strictness of the Server catches wrong assumptions of Clients about
+the Server's schema.  Clients can assume that, when such validation
+errors occur, they will be reported before the command generated any
+side effect.
+
+However, Clients must not assume any particular:
+
+- Length of json-arrays
+- Size of json-objects; in particular, future versions of QEMU may add
+  new keys and Clients should be able to ignore them.
 - Order of json-object members or json-array elements
 - Amount of errors generated by a command, that is, new errors can be added
   to any existing command in newer versions of the Server
 
+Of course, the Server does guarantee to send valid JSON.  But apart from
+this, a Client should be "conservative in what they send, and liberal in
+what they accept".
+
 6. Downstream extension of QMP
 ------------------------------
 
-- 
1.7.9.1