From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAoa6-0006D5-Sq for qemu-devel@nongnu.org; Thu, 22 Mar 2012 16:26:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SAoa4-0002Cc-46 for qemu-devel@nongnu.org; Thu, 22 Mar 2012 16:25:58 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:58576) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SAoa3-0002CY-US for qemu-devel@nongnu.org; Thu, 22 Mar 2012 16:25:56 -0400 Received: by yenr5 with SMTP id r5so2600497yen.4 for ; Thu, 22 Mar 2012 13:25:54 -0700 (PDT) Message-ID: <4F6B8ACE.60504@codemonkey.ws> Date: Thu, 22 Mar 2012 15:25:50 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1332417072-20329-1-git-send-email-pbonzini@redhat.com> <1332417072-20329-9-git-send-email-pbonzini@redhat.com> In-Reply-To: <1332417072-20329-9-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: lcapitulino@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com 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 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 > + * Paolo Bonzini > + * > + * 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 > +#include > + > +#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 >