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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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

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

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