From e58d695e6c3a5cfa0aa2fc91b87ade017ef28b05 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:10 -0600 Subject: qapi: Guarantee NULL obj on input visitor callback error Our existing input visitors were not very consistent on errors in a function taking 'TYPE **obj'. These are start_struct(), start_alternate(), type_str(), and type_any(). next_list() is similar, but can't fail (see commit 08f9541). While all of them set '*obj' to allocated storage on success, it was not obvious whether '*obj' was guaranteed safe on failure, or whether it was left uninitialized. But a future patch wants to guarantee that visit_type_FOO() does not leak a partially-constructed obj back to the caller; it is easier to implement this if we can reliably state that input visitors assign '*obj' regardless of success or failure, and that on failure *obj is NULL. Add assertions to enforce consistency in the final setting of err vs. *obj. The opts-visitor start_struct() doesn't set an error, but it also was doing a weird check for 0 size; all callers pass in non-zero size if obj is non-NULL. The testsuite has at least one spot where we no longer need to pre-initialize a variable prior to a visit; valgrind confirms that the test is still fine with the cleanup. A later patch will document the design constraint implemented here. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-3-git-send-email-eblake@redhat.com> [visit_start_alternate()'s assertion tightened, commit message tweaked] Signed-off-by: Markus Armbruster --- tests/test-qmp-input-strict.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index d71727e..d5f80ec 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -263,7 +263,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, static void test_validate_fail_alternate(TestInputVisitorData *data, const void *unused) { - UserDefAlternate *tmp = NULL; + UserDefAlternate *tmp; Visitor *v; Error *err = NULL; -- cgit v1.1 From fc471c18d5d2ec713d5a019f9530398675494bc8 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:13 -0600 Subject: qapi: Consolidate QMP input visitor creation Rather than having two separate ways to create a QMP input visitor, where the safer approach has the more verbose name, it is better to consolidate things into a single function where the caller must explicitly choose whether to be strict or to ignore excess input. This patch is the strictly mechanical conversion; the next patch will then audit which uses can be made stricter. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-qmp-commands.c | 2 +- tests/test-qmp-input-strict.c | 2 +- tests/test-qmp-input-visitor.c | 2 +- tests/test-visitor-serialization.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 14a9ebb..a8d37c4 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -222,7 +222,7 @@ static void test_dealloc_partial(void) ud2_dict = qdict_new(); qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); - qiv = qmp_input_visitor_new(QOBJECT(ud2_dict)); + qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), false); visit_type_UserDefTwo(qmp_input_get_visitor(qiv), NULL, &ud2, &err); qmp_input_visitor_cleanup(qiv); QDECREF(ud2_dict); diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index d5f80ec..2b053a2 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -55,7 +55,7 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new_strict(data->obj); + data->qiv = qmp_input_visitor_new(data->obj, true); g_assert(data->qiv); v = qmp_input_get_visitor(data->qiv); diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 80527eb..c039806 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new(data->obj); + data->qiv = qmp_input_visitor_new(data->obj, false); g_assert(data->qiv); v = qmp_input_get_visitor(data->qiv); diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index 9adbc30..2caac2b 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -1038,7 +1038,7 @@ static void qmp_deserialize(void **native_out, void *datap, obj = qobject_from_json(qstring_get_str(output_json)); QDECREF(output_json); - d->qiv = qmp_input_visitor_new(obj); + d->qiv = qmp_input_visitor_new(obj, false); qobject_decref(obj_orig); qobject_decref(obj); visit(qmp_input_get_visitor(d->qiv), native_out, errp); -- cgit v1.1 From 240f64b6dc3346d044d7beb7cc3a53668ce47384 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:14 -0600 Subject: qapi: Use strict QMP input visitor in more places The following uses of a QMP input visitor should be strict (that is, excess keys in QDict input should be flagged if not converted to QAPI): - Testsuite code unrelated to explicitly testing non-strict mode (test-qmp-commands, test-visitor-serialization); since we want more code to be strict by default, having more tests of strict mode doesn't hurt - Code used for cloning QAPI objects (replay-input.c, qemu-sockets.c); we are reparsing a QObject just barely produced by the qmp output visitor and which therefore should not have any garbage, so while it is extra work to be strict, it validates that our clone is correct [note that a later patch series will simplify these two uses by creating an actual clone visitor that is much more efficient than a generate/reparse cycle] - qmp_object_add(), which calls into user_creatable_add_type(). Since command line parsing for '-object' uses the same user_creatable_add_type() through the OptsVisitor, and that is always strict, we want to ensure that any nested dictionaries would be treated the same in QMP and from the command line (I don't actually know if such nested dictionaries exist). Note that on this code change, strictness only matters for nested dictionaries (if even possible), since we already flag excess input at the top level during an earlier object_property_set() on an unknown key, whether from QemuOpts: $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio -object secret,id=sec0,data=letmein,format=raw,foo=bar qemu-system-x86_64: -object secret,id=sec0,data=letmein,format=raw,foo=bar: Property '.foo' not found or from QMP: $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio {"QMP": {"version": {"qemu": {"micro": 93, "minor": 5, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities"} {"return": {}} {"execute":"object-add","arguments":{"qom-type":"secret","id":"sec0","props":{"format":"raw","data":"letmein","foo":"bar"}}} {"error": {"class": "GenericError", "desc": "Property '.foo' not found"}} The only remaining uses of non-strict input visits are: - QMP 'qom-set' (which eventually executes object_property_set_qobject()) - mark it as something to revisit in the future (I didn't want to spend any more time on this patch auditing if we have any QOM dictionary properties that might be impacted, and couldn't easily prove whether this code path is shared with anything else). - test-qmp-input-visitor: explicit tests of non-strict mode. If we later get rid of users that don't need strictness, then this test should be merged with test-qmp-input-strict Signed-off-by: Eric Blake Message-Id: <1461879932-9020-7-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-qmp-commands.c | 2 +- tests/test-visitor-serialization.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index a8d37c4..597fb44 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -222,7 +222,7 @@ static void test_dealloc_partial(void) ud2_dict = qdict_new(); qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); - qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), false); + qiv = qmp_input_visitor_new(QOBJECT(ud2_dict), true); visit_type_UserDefTwo(qmp_input_get_visitor(qiv), NULL, &ud2, &err); qmp_input_visitor_cleanup(qiv); QDECREF(ud2_dict); diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index 2caac2b..7b14b5a 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -1038,7 +1038,7 @@ static void qmp_deserialize(void **native_out, void *datap, obj = qobject_from_json(qstring_get_str(output_json)); QDECREF(output_json); - d->qiv = qmp_input_visitor_new(obj, false); + d->qiv = qmp_input_visitor_new(obj, true); qobject_decref(obj_orig); qobject_decref(obj); visit(qmp_input_get_visitor(d->qiv), native_out, errp); -- cgit v1.1 From 7d7a337ec32c77f11ba04da412752b35e18d3c5c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:21 -0600 Subject: tests: Add check-qnull Add a new test, for checking reference counting of qnull(). As part of the new file, move a previous reference counting change added in commit a861564 to a more logical place. Note that while most of the check-q*.c leave visitor stuff to the test-qmp-*-visitor.c, in this case we actually want the visitor tests in our new file because we are validating the reference count of qnull_, which is an internal detail that test-qmp-*-visitor should not be peeking into (or put another way, qnull() is the only special case where we don't have independent allocation of a QObject, so none of the other visitor tests require the layering violation present in this test). Signed-off-by: Eric Blake Message-Id: <1461879932-9020-14-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/.gitignore | 1 + tests/Makefile | 6 +++- tests/check-qnull.c | 66 +++++++++++++++++++++++++++++++++++++++++ tests/test-qmp-output-visitor.c | 2 -- 4 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 tests/check-qnull.c (limited to 'tests') diff --git a/tests/.gitignore b/tests/.gitignore index 9eed229..a06a8ba 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -3,6 +3,7 @@ check-qfloat check-qint check-qjson check-qlist +check-qnull check-qstring check-qom-interface check-qom-proplist diff --git a/tests/Makefile b/tests/Makefile index 9194f18..9dddde6 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -16,6 +16,8 @@ check-unit-y += tests/check-qstring$(EXESUF) gcov-files-check-qstring-y = qobject/qstring.c check-unit-y += tests/check-qlist$(EXESUF) gcov-files-check-qlist-y = qobject/qlist.c +check-unit-y += tests/check-qnull$(EXESUF) +gcov-files-check-qnull-y = qobject/qnull.c check-unit-y += tests/check-qjson$(EXESUF) gcov-files-check-qjson-y = qobject/qjson.c check-unit-y += tests/test-qmp-output-visitor$(EXESUF) @@ -382,7 +384,8 @@ GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \ tests/test-qmp-introspect.h test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ - tests/check-qlist.o tests/check-qfloat.o tests/check-qjson.o \ + tests/check-qlist.o tests/check-qfloat.o tests/check-qnull.o \ + tests/check-qjson.o \ tests/test-coroutine.o tests/test-string-output-visitor.o \ tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ @@ -410,6 +413,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y) tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y) tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y) tests/check-qfloat$(EXESUF): tests/check-qfloat.o $(test-util-obj-y) +tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y) tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y) tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y) tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y) diff --git a/tests/check-qnull.c b/tests/check-qnull.c new file mode 100644 index 0000000..4a1c3d8 --- /dev/null +++ b/tests/check-qnull.c @@ -0,0 +1,66 @@ +/* + * QNull unit-tests. + * + * Copyright (C) 2016 Red Hat Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ +#include "qemu/osdep.h" +#include + +#include "qapi/qmp/qobject.h" +#include "qemu-common.h" +#include "qapi/qmp-output-visitor.h" + +/* + * Public Interface test-cases + * + * (with some violations to access 'private' data) + */ + +static void qnull_ref_test(void) +{ + QObject *obj; + + g_assert(qnull_.refcnt == 1); + obj = qnull(); + g_assert(obj); + g_assert(obj == &qnull_); + g_assert(qnull_.refcnt == 2); + g_assert(qobject_type(obj) == QTYPE_QNULL); + qobject_decref(obj); + g_assert(qnull_.refcnt == 1); +} + +static void qnull_visit_test(void) +{ + QObject *obj; + QmpOutputVisitor *qov; + + /* + * Most tests of interactions between QObject and visitors are in + * test-qmp-*-visitor; but these tests live here because they + * depend on layering violations to check qnull_ refcnt. + */ + + g_assert(qnull_.refcnt == 1); + qov = qmp_output_visitor_new(); + /* FIXME: Empty visits are ugly, we should have a visit_type_null(). */ + obj = qmp_output_get_qobject(qov); + g_assert(obj == &qnull_); + qobject_decref(obj); + + qmp_output_visitor_cleanup(qov); + g_assert(qnull_.refcnt == 1); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/public/qnull_ref", qnull_ref_test); + g_test_add_func("/public/qnull_visit", qnull_visit_test); + + return g_test_run(); +} diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index c709267..fddb5a6 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -484,8 +484,6 @@ static void test_visitor_out_empty(TestOutputVisitorData *data, arg = qmp_output_get_qobject(data->qov); g_assert(qobject_type(arg) == QTYPE_QNULL); - /* Check that qnull reference counting is sane */ - g_assert(arg->refcnt == 2); qobject_decref(arg); } -- cgit v1.1 From 3df016f185521f8dfa5bd89168722887156405c7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:23 -0600 Subject: qmp: Support explicit null during visits Implement the new type_null() callback for the qmp input and output visitors. While we don't yet have a use for this in QAPI input (the generator will need some tweaks first), some potential usages have already been discussed on the list. Meanwhile, the output visitor could already output explicit null via type_any, but this gives us finer control. At any rate, it's easy to test that we can round-trip an explicit null through manual use of visit_type_null() wrapped by a virtual visit_start_struct() walk, even if we can't do the visit in a QAPI type. Repurpose the test_visitor_out_empty test, particularly since a future patch will tighten semantics to forbid use of qmp_output_get_qobject() without at least one intervening visit_type_*. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-16-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/check-qnull.c | 13 +++++++++++-- tests/test-qmp-input-visitor.c | 29 +++++++++++++++++++++++++++++ tests/test-qmp-output-visitor.c | 20 +++++++++++++++----- 3 files changed, 55 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/check-qnull.c b/tests/check-qnull.c index 4a1c3d8..fd9c68f 100644 --- a/tests/check-qnull.c +++ b/tests/check-qnull.c @@ -11,7 +11,9 @@ #include "qapi/qmp/qobject.h" #include "qemu-common.h" +#include "qapi/qmp-input-visitor.h" #include "qapi/qmp-output-visitor.h" +#include "qapi/error.h" /* * Public Interface test-cases @@ -37,6 +39,7 @@ static void qnull_visit_test(void) { QObject *obj; QmpOutputVisitor *qov; + QmpInputVisitor *qiv; /* * Most tests of interactions between QObject and visitors are in @@ -45,13 +48,19 @@ static void qnull_visit_test(void) */ g_assert(qnull_.refcnt == 1); + obj = qnull(); + qiv = qmp_input_visitor_new(obj, true); + qobject_decref(obj); + visit_type_null(qmp_input_get_visitor(qiv), NULL, &error_abort); + qmp_input_visitor_cleanup(qiv); + qov = qmp_output_visitor_new(); - /* FIXME: Empty visits are ugly, we should have a visit_type_null(). */ + visit_type_null(qmp_output_get_visitor(qov), NULL, &error_abort); obj = qmp_output_get_qobject(qov); g_assert(obj == &qnull_); qobject_decref(obj); - qmp_output_visitor_cleanup(qov); + g_assert(qnull_.refcnt == 1); } diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index c039806..10e00ec 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -279,6 +279,33 @@ static void test_visitor_in_any(TestInputVisitorData *data, qobject_decref(res); } +static void test_visitor_in_null(TestInputVisitorData *data, + const void *unused) +{ + Visitor *v; + Error *err = NULL; + char *tmp; + + /* + * FIXME: Since QAPI doesn't know the 'null' type yet, we can't + * test visit_type_null() by reading into a QAPI struct then + * checking that it was populated correctly. The best we can do + * for now is ensure that we consumed null from the input, proven + * by the fact that we can't re-read the key; and that we detect + * when input is not null. + */ + + v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }"); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + visit_type_null(v, "a", &error_abort); + visit_type_str(v, "a", &tmp, &err); + g_assert(!tmp); + error_free_or_abort(&err); + visit_type_null(v, "b", &err); + error_free_or_abort(&err); + visit_end_struct(v, &error_abort); +} + static void test_visitor_in_union_flat(TestInputVisitorData *data, const void *unused) { @@ -829,6 +856,8 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_list); input_visitor_test_add("/visitor/input/any", &in_visitor_data, test_visitor_in_any); + input_visitor_test_add("/visitor/input/null", + &in_visitor_data, test_visitor_in_null); input_visitor_test_add("/visitor/input/union-flat", &in_visitor_data, test_visitor_in_union_flat); input_visitor_test_add("/visitor/input/alternate", diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index fddb5a6..8acc229 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -477,13 +477,23 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qobject_decref(arg); } -static void test_visitor_out_empty(TestOutputVisitorData *data, - const void *unused) +static void test_visitor_out_null(TestOutputVisitorData *data, + const void *unused) { QObject *arg; + QDict *qdict; + QObject *nil; + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); + visit_type_null(data->ov, "a", &error_abort); + visit_end_struct(data->ov, &error_abort); arg = qmp_output_get_qobject(data->qov); - g_assert(qobject_type(arg) == QTYPE_QNULL); + g_assert(qobject_type(arg) == QTYPE_QDICT); + qdict = qobject_to_qdict(arg); + g_assert_cmpint(qdict_size(qdict), ==, 1); + nil = qdict_get(qdict, "a"); + g_assert(nil); + g_assert(qobject_type(nil) == QTYPE_QNULL); qobject_decref(arg); } @@ -837,8 +847,8 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_union_flat); output_visitor_test_add("/visitor/output/alternate", &out_visitor_data, test_visitor_out_alternate); - output_visitor_test_add("/visitor/output/empty", - &out_visitor_data, test_visitor_out_empty); + output_visitor_test_add("/visitor/output/null", + &out_visitor_data, test_visitor_out_null); output_visitor_test_add("/visitor/output/native_list/int", &out_visitor_data, test_visitor_out_native_list_int); -- cgit v1.1 From f2ff429bfa713da1366333417fecfa939a709cf1 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 9 May 2016 22:20:06 -0600 Subject: qmp: Don't reuse qmp visitor after grabbing output The testsuite was the only client that attempted to reuse a QmpOutputVisitor for a second visit after encountering an error and/or calling qmp_output_get_qobject() on a first visit. The next patch is about to tighten the semantics to be one-shot usage of the visitor, like all other visitors (which will enable further simplifications down the road). Signed-off-by: Eric Blake Message-Id: <1462854006-24658-1-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-qmp-output-visitor.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'tests') diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 8acc229..5543511 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -43,6 +43,12 @@ static void visitor_output_teardown(TestOutputVisitorData *data, data->ov = NULL; } +static void visitor_reset(TestOutputVisitorData *data) +{ + visitor_output_teardown(data, NULL); + visitor_output_setup(data, NULL); +} + static void test_visitor_out_int(TestOutputVisitorData *data, const void *unused) { @@ -139,6 +145,7 @@ static void test_visitor_out_enum(TestOutputVisitorData *data, g_assert_cmpstr(qstring_get_str(qobject_to_qstring(obj)), ==, EnumOne_lookup[i]); qobject_decref(obj); + visitor_reset(data); } } @@ -153,6 +160,7 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data, visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err); g_assert(err); error_free(err); + visitor_reset(data); } } @@ -262,6 +270,7 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, visit_type_UserDefOne(data->ov, "unused", &pu, &err); g_assert(err); error_free(err); + visitor_reset(data); } } @@ -366,6 +375,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data, qobject_decref(obj); qobject_decref(qobj); + visitor_reset(data); qdict = qdict_new(); qdict_put(qdict, "integer", qint_from_int(-42)); qdict_put(qdict, "boolean", qbool_from_bool(true)); @@ -442,6 +452,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qapi_free_UserDefAlternate(tmp); qobject_decref(arg); + visitor_reset(data); tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QSTRING; tmp->u.s = g_strdup("hello"); @@ -455,6 +466,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qapi_free_UserDefAlternate(tmp); qobject_decref(arg); + visitor_reset(data); tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QDICT; tmp->u.udfu.integer = 1; -- cgit v1.1 From 15c2f669e3fb2bc97f7b42d1871f595c0ac24af8 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:27 -0600 Subject: qapi: Split visit_end_struct() into pieces As mentioned in previous patches, we want to call visit_end_struct() functions unconditionally, so that visitors can release resources tied up since the matching visit_start_struct() without also having to worry about error priority if more than one error occurs. Even though error_propagate() can be safely used to ignore a second error during cleanup caused by a first error, it is simpler if the cleanup cannot set an error. So, split out the error checking portion (basically, input visitors checking for unvisited keys) into a new function visit_check_struct(), which can be safely skipped if any earlier errors are encountered, and leave the cleanup portion (which never fails, but must be called unconditionally if visit_start_struct() succeeded) in visit_end_struct(). Generated code in qapi-visit.c has diffs resembling: |@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v, | goto out_obj; | } | visit_type_ACPIOSTInfo_members(v, obj, &err); |- error_propagate(errp, err); |- err = NULL; |+ if (err) { |+ goto out_obj; |+ } |+ visit_check_struct(v, &err); | out_obj: |- visit_end_struct(v, &err); |+ visit_end_struct(v); | out: and in qapi-event.c: @@ -47,7 +47,10 @@ void qapi_event_send_acpi_device_ost(ACP | goto out; | } | visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, ¶m, &err); |- visit_end_struct(v, err ? NULL : &err); |+ if (!err) { |+ visit_check_struct(v, &err); |+ } |+ visit_end_struct(v); | if (err) { | goto out; Signed-off-by: Eric Blake Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com> [Conflict with a doc fixup resolved] Signed-off-by: Markus Armbruster --- tests/test-qmp-input-visitor.c | 3 ++- tests/test-qmp-output-visitor.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 10e00ec..6617276 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -303,7 +303,8 @@ static void test_visitor_in_null(TestInputVisitorData *data, error_free_or_abort(&err); visit_type_null(v, "b", &err); error_free_or_abort(&err); - visit_end_struct(v, &error_abort); + visit_check_struct(v, &error_abort); + visit_end_struct(v); } static void test_visitor_in_union_flat(TestInputVisitorData *data, diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 5543511..1f80e69 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -498,7 +498,8 @@ static void test_visitor_out_null(TestOutputVisitorData *data, visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); visit_type_null(data->ov, "a", &error_abort); - visit_end_struct(data->ov, &error_abort); + visit_check_struct(data->ov, &error_abort); + visit_end_struct(data->ov); arg = qmp_output_get_qobject(data->qov); g_assert(qobject_type(arg) == QTYPE_QDICT); qdict = qobject_to_qdict(arg); -- cgit v1.1 From 7337468385f0ad258657b2ce76ac0a7306f9f186 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 28 Apr 2016 15:45:29 -0600 Subject: tests/string-input-visitor: Add negative integer tests Add two negative tests, one for int and one for int16List. The latter exposes a bug: nonsensical input results in an empty list instead of an error. Signed-off-by: Markus Armbruster Message-Id: <1461325048-14122-1-git-send-email-armbru@redhat.com> Signed-off-by: Eric Blake Message-Id: <1461879932-9020-22-git-send-email-eblake@redhat.com> --- tests/test-string-input-visitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'tests') diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index 9e6906a..8114908 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -63,6 +63,13 @@ static void test_visitor_in_int(TestInputVisitorData *data, visit_type_int(v, NULL, &res, &err); g_assert(!err); g_assert_cmpint(res, ==, value); + + visitor_input_teardown(data, unused); + + v = visitor_input_test_init(data, "not an int"); + + visit_type_int(v, NULL, &res, &err); + error_free_or_abort(&err); } static void test_visitor_in_intList(TestInputVisitorData *data, @@ -70,6 +77,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data, { int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20}; int16List *res = NULL, *tmp; + Error *err = NULL; Visitor *v; int i = 0; @@ -90,6 +98,13 @@ static void test_visitor_in_intList(TestInputVisitorData *data, g_free(tmp); tmp = res; } + + visitor_input_teardown(data, unused); + + v = visitor_input_test_init(data, "not an int list"); + + visit_type_int16List(v, NULL, &res, &err); + /* FIXME fix the visitor, then error_free_or_abort(&err) here */ } static void test_visitor_in_bool(TestInputVisitorData *data, -- cgit v1.1 From 74f24cb6306d065045d0e2215a7d10533fa59c57 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:30 -0600 Subject: qapi: Fix string input visitor handling of invalid list As shown in the previous commit, the string input visitor was treating bogus input as an empty list rather than an error. Fix parse_str() to set errp, then the callers to exit early if an error was reported. Meanwhile, fix the testsuite to use the generated qapi_free_int16List() instead of rolling our own, and to validate the fixed behavior, while at the same time documenting one more change that we'd like to make in a later patch (a failed visit_start_list should guarantee a NULL pointer, regardless of what things were on input). Signed-off-by: Eric Blake Message-Id: <1461879932-9020-23-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-string-input-visitor.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index 8114908..f99824d 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -92,19 +92,17 @@ static void test_visitor_in_intList(TestInputVisitorData *data, } g_assert(!tmp); - tmp = res; - while (tmp) { - res = res->next; - g_free(tmp); - tmp = res; - } + qapi_free_int16List(res); visitor_input_teardown(data, unused); v = visitor_input_test_init(data, "not an int list"); + /* FIXME: res should be NULL on failure, regardless of starting value */ + res = NULL; visit_type_int16List(v, NULL, &res, &err); - /* FIXME fix the visitor, then error_free_or_abort(&err) here */ + error_free_or_abort(&err); + g_assert(!res); } static void test_visitor_in_bool(TestInputVisitorData *data, -- cgit v1.1 From d9f62dde1303286b24ac8ce88be27e2b9b9c5f46 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:31 -0600 Subject: qapi: Simplify semantics of visit_next_list() The semantics of the list visit are somewhat baroque, with the following pseudocode when FooList is used: start() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList **' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. Thankfully, we only have two uses of list visits in the entire code base: one in spapr_drc (which completely avoids visit_next_list(), feeding in integers from a different source than uint8List), and one in qapi-visit.py. That is, all other list visitors are generated in qapi-visit.c, and share the same paradigm based on a qapi FooList type, so we can refactor how lists are laid out with minimal churn among clients. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = *head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). As a minor drawback, we now allocate in two functions instead of one, and have to pass the size to both functions (unless we were to tweak the input visitors to cache the size to start_list for reuse during next_list, but that defeats the goal of less visitor state). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Several pre-requisite cleanup patches made the reshuffling of the various visitors easier; particularly the qmp input visitor. Signed-off-by: Eric Blake Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-string-input-visitor.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'tests') diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index f99824d..5a56920 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -98,8 +98,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data, v = visitor_input_test_init(data, "not an int list"); - /* FIXME: res should be NULL on failure, regardless of starting value */ - res = NULL; visit_type_int16List(v, NULL, &res, &err); error_free_or_abort(&err); g_assert(!res); -- cgit v1.1 From 68ab47e4b4ecc1c4649362b8cc1e49794d1a6537 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 28 Apr 2016 15:45:32 -0600 Subject: qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/test-qmp-commands.c | 13 ++++++------- tests/test-qmp-input-strict.c | 17 +++++++---------- tests/test-qmp-input-visitor.c | 10 ++-------- 3 files changed, 15 insertions(+), 25 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 597fb44..5c3edd7 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -228,14 +228,13 @@ static void test_dealloc_partial(void) QDECREF(ud2_dict); } - /* verify partial success */ - assert(ud2 != NULL); - assert(ud2->string0 != NULL); - assert(strcmp(ud2->string0, text) == 0); - assert(ud2->dict1 == NULL); - - /* confirm & release construction error */ + /* verify that visit_type_XXX() cleans up properly on error */ error_free_or_abort(&err); + assert(!ud2); + + /* Manually create a partial object, leaving ud2->dict1 at NULL */ + ud2 = g_new0(UserDefTwo, 1); + ud2->string0 = g_strdup(text); /* tear down partial object */ qapi_free_UserDefTwo(ud2); diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index 2b053a2..4602529 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -182,10 +182,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data, visit_type_TestStruct(v, NULL, &p, &err); error_free_or_abort(&err); - if (p) { - g_free(p->string); - } - g_free(p); + g_assert(!p); } static void test_validate_fail_struct_nested(TestInputVisitorData *data, @@ -199,7 +196,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data, visit_type_UserDefTwo(v, NULL, &udp, &err); error_free_or_abort(&err); - qapi_free_UserDefTwo(udp); + g_assert(!udp); } static void test_validate_fail_list(TestInputVisitorData *data, @@ -213,7 +210,7 @@ static void test_validate_fail_list(TestInputVisitorData *data, visit_type_UserDefOneList(v, NULL, &head, &err); error_free_or_abort(&err); - qapi_free_UserDefOneList(head); + g_assert(!head); } static void test_validate_fail_union_native_list(TestInputVisitorData *data, @@ -228,7 +225,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data, visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefNativeListUnion(tmp); + g_assert(!tmp); } static void test_validate_fail_union_flat(TestInputVisitorData *data, @@ -242,7 +239,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, visit_type_UserDefFlatUnion(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefFlatUnion(tmp); + g_assert(!tmp); } static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, @@ -257,7 +254,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefFlatUnion2(tmp); + g_assert(!tmp); } static void test_validate_fail_alternate(TestInputVisitorData *data, @@ -271,7 +268,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data, visit_type_UserDefAlternate(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefAlternate(tmp); + g_assert(!tmp); } static void do_test_validate_qmp_introspect(TestInputVisitorData *data, diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 6617276..cee07ce 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -773,18 +773,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data, visit_type_TestStruct(v, NULL, &p, &err); error_free_or_abort(&err); - /* FIXME - a failed parse should not leave a partially-allocated p - * for us to clean up; this could cause callers to leak memory. */ - g_assert(p->string == NULL); - - g_free(p->string); - g_free(p); + g_assert(!p); v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]"); visit_type_strList(v, NULL, &q, &err); error_free_or_abort(&err); - assert(q); - qapi_free_strList(q); + assert(!q); } static void test_visitor_in_wrong_type(TestInputVisitorData *data, -- cgit v1.1