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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>