From 748053c97b11039f657525fd7d57a39806d8083e Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:25 -0700
Subject: [PATCH 01/12] qapi: Use generated TestStruct machinery in tests
Commit d88f5fd and friends first introduced the various test-qmp-*
tests in 2011, with duplicated hand-rolled TestStruct machinery,
to make sure the qapi visitor interface was tested. Later, commit
4f193e3 in 2013 added a .json file for further testing use by the
files, but without consolidating any of the existing hand-rolled
visitors. And with four copies, subtle differences have crept in,
between the tests themselves (mainly whitespace differences, but
also a question of whether to use NULL or "TestStruct" when
calling visit_start_struct()) and from what the generator produces
(the hand-rolled versions did not cater to partially-allocated
objects, because they did not have a deallocation usage).
Of course, just because the visitor interface is tested does not
mean it is a sane interface; and future patches will be changing
some of the visitor contracts. Rather than having to duplicate
the cleanup work in each copy of the TestStruct visitor, and keep
each hand-rolled copy in sync with what the generator supplies, we
might as well just test what the generator should give us in the
first place.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-2-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/qapi-schema/qapi-schema-test.json | 6 ++-
tests/qapi-schema/qapi-schema-test.out | 5 +++
tests/test-qmp-input-strict.c | 35 ---------------
tests/test-qmp-input-visitor.c | 34 ---------------
tests/test-qmp-output-visitor.c | 58 -------------------------
tests/test-visitor-serialization.c | 34 ---------------
6 files changed, 10 insertions(+), 162 deletions(-)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 48e104ba13..44638da948 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -3,6 +3,9 @@
# This file is a stress test of supported qapi constructs that must
# parse and compile correctly.
+{ 'struct': 'TestStruct',
+ 'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } }
+
# for testing enums
{ 'struct': 'NestedEnumsOne',
'data': { 'enum1': 'EnumOne', # Intentional forward reference
@@ -46,7 +49,8 @@
# dummy struct to force generation of array types not otherwise mentioned
{ 'struct': 'ForceArrays',
- 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
+ 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
+ 'unused3':['TestStruct'] } }
# for testing unions
# Among other things, test that a name collision between branches does
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index a7e9aabec0..e20a8239ad 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -92,6 +92,7 @@ object EventStructOne
object ForceArrays
member unused1: UserDefOneList optional=False
member unused2: UserDefTwoList optional=False
+ member unused3: TestStructList optional=False
enum MyEnum []
object NestedEnumsOne
member enum1: EnumOne optional=False
@@ -100,6 +101,10 @@ object NestedEnumsOne
member enum4: EnumOne optional=True
enum QEnumTwo ['value1', 'value2']
prefix QENUM_TWO
+object TestStruct
+ member integer: int optional=False
+ member boolean: bool optional=False
+ member string: str optional=False
object UserDefA
member boolean: bool optional=False
member a_b: int optional=True
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 53a769388c..b44184fed1 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -89,41 +89,6 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data,
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)
-{
- Error *err = NULL;
-
- visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
- &err);
- if (err) {
- goto out;
- }
-
- visit_type_int(v, &(*obj)->integer, "integer", &err);
- if (err) {
- goto out_end;
- }
- visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
- if (err) {
- goto out_end;
- }
- visit_type_str(v, &(*obj)->string, "string", &err);
-
-out_end:
- error_propagate(errp, err);
- err = NULL;
- visit_end_struct(v, &err);
-out:
- error_propagate(errp, err);
-}
static void test_validate_struct(TestInputVisitorData *data,
const void *unused)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index de65982d47..3f6bc4d2af 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -185,40 +185,6 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
data->qiv = NULL;
}
-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)
-{
- Error *err = NULL;
-
- visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
- &err);
- if (err) {
- goto out;
- }
- visit_type_int(v, &(*obj)->integer, "integer", &err);
- if (err) {
- goto out_end;
- }
- visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
- if (err) {
- goto out_end;
- }
- visit_type_str(v, &(*obj)->string, "string", &err);
-
-out_end:
- error_propagate(errp, err);
- err = NULL;
- visit_end_struct(v, &err);
-out:
- error_propagate(errp, err);
-}
static void test_visitor_in_struct(TestInputVisitorData *data,
const void *unused)
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 09d0dd81f0..baf58dc716 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -166,41 +166,6 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
}
}
-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)
-{
- Error *err = NULL;
-
- visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
- &err);
- if (err) {
- goto out;
- }
-
- visit_type_int(v, &(*obj)->integer, "integer", &err);
- if (err) {
- goto out_end;
- }
- visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
- if (err) {
- goto out_end;
- }
- visit_type_str(v, &(*obj)->string, "string", &err);
-
-out_end:
- error_propagate(errp, err);
- err = NULL;
- visit_end_struct(v, &err);
-out:
- error_propagate(errp, err);
-}
static void test_visitor_out_struct(TestOutputVisitorData *data,
const void *unused)
@@ -314,29 +279,6 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
}
}
-typedef struct TestStructList
-{
- union {
- TestStruct *value;
- uint64_t padding;
- };
- struct TestStructList *next;
-} TestStructList;
-
-static void visit_type_TestStructList(Visitor *v, TestStructList **obj,
- const char *name, Error **errp)
-{
- GenericList *i, **head = (GenericList **)obj;
-
- visit_start_list(v, name, errp);
-
- for (*head = i = visit_next_list(v, head, errp); i; i = visit_next_list(v, &i, errp)) {
- TestStructList *native_i = (TestStructList *)i;
- visit_type_TestStruct(v, &native_i->value, NULL, errp);
- }
-
- visit_end_list(v, errp);
-}
static void test_visitor_out_list(TestOutputVisitorData *data,
const void *unused)
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 634563bae4..c024e5eaf4 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -186,40 +186,6 @@ static void visit_primitive_list(Visitor *v, void **native, Error **errp)
}
}
-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)
-{
- Error *err = NULL;
-
- visit_start_struct(v, (void **)obj, NULL, name, sizeof(TestStruct), &err);
- if (err) {
- goto out;
- }
-
- visit_type_int(v, &(*obj)->integer, "integer", &err);
- if (err) {
- goto out_end;
- }
- visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
- if (err) {
- goto out_end;
- }
- visit_type_str(v, &(*obj)->string, "string", &err);
-
-out_end:
- error_propagate(errp, err);
- err = NULL;
- visit_end_struct(v, &err);
-out:
- error_propagate(errp, err);
-}
static TestStruct *struct_create(void)
{
From bd20588d19e9ff0e94b2d4ca3b5d6b3b3d6a1274 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:26 -0700
Subject: [PATCH 02/12] qapi: Strengthen test of TestStructList
Make each list element different, to ensure that order is
preserved, and use the generated free function instead of
hand-rolling our own to ensure (under valgrind) that the
list is properly cleaned.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-3-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-output-visitor.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index baf58dc716..9364843218 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -283,7 +283,7 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
static void test_visitor_out_list(TestOutputVisitorData *data,
const void *unused)
{
- char *value_str = (char *) "list value";
+ const char *value_str = "list value";
TestStructList *p, *head = NULL;
const int max_items = 10;
bool value_bool = true;
@@ -294,12 +294,13 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
QList *qlist;
int i;
+ /* Build the list in reverse order... */
for (i = 0; i < max_items; i++) {
p = g_malloc0(sizeof(*p));
p->value = g_malloc0(sizeof(*p->value));
- p->value->integer = value_int;
+ p->value->integer = value_int + (max_items - i - 1);
p->value->boolean = value_bool;
- p->value->string = value_str;
+ p->value->string = g_strdup(value_str);
p->next = head;
head = p;
@@ -315,6 +316,7 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
qlist = qobject_to_qlist(obj);
g_assert(!qlist_empty(qlist));
+ /* ...and ensure that the visitor sees it in order */
i = 0;
QLIST_FOREACH_ENTRY(qlist, entry) {
QDict *qdict;
@@ -322,7 +324,7 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
g_assert(qobject_type(entry->value) == QTYPE_QDICT);
qdict = qobject_to_qdict(entry->value);
g_assert_cmpint(qdict_size(qdict), ==, 3);
- g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, value_int);
+ g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, value_int + i);
g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, value_bool);
g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, value_str);
i++;
@@ -330,13 +332,7 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
g_assert_cmpint(i, ==, max_items);
QDECREF(qlist);
-
- for (p = head; p;) {
- TestStructList *tmp = p->next;
- g_free(p->value);
- g_free(p);
- p = tmp;
- }
+ qapi_free_TestStructList(head);
}
static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
From cc9f60d4a2a4bf2578a9309a18f1c4602c9f5ce7 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:27 -0700
Subject: [PATCH 03/12] qobject: Protect against use-after-free in
qobject_decref()
Adding an assertion to qobject_decref() will ensure that a
programming error causing use-after-free will result in
immediate failure (provided no other thread has started
using the memory) instead of silently attempting to wrap
refcnt around and leaving the problem to potentially bite
later at a harder point to diagnose.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-4-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/qmp/qobject.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index c856f553b7..4b96ed5837 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -90,6 +90,7 @@ static inline void qobject_incref(QObject *obj)
*/
static inline void qobject_decref(QObject *obj)
{
+ assert(!obj || obj->refcnt);
if (obj && --obj->refcnt == 0) {
assert(obj->type != NULL);
assert(obj->type->destroy != NULL);
From 0920a17199d23b3def3a60fa1fbbdeadcdda452d Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:28 -0700
Subject: [PATCH 04/12] qapi: Share test_init code in test-qmp-input*
Rather than duplicate the body of two functions just to
decide between qobject_from_jsonv() and qobject_from_json(),
exploit the fact that qobject_from_jsonv() intentionally
takes 'va_list *' instead of the more common 'va_list', and
that qobject_from_json() just calls qobject_from_jsonv(,NULL).
For each file, our two existing init functions then become
thin wrappers around a new internal function, and future
updates to initialization don't have to be duplicated.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-5-git-send-email-eblake@redhat.com>
[Two old comment typos fixed]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-input-strict.c | 48 ++++++++++++++++-----------------
tests/test-qmp-input-visitor.c | 49 ++++++++++++++++------------------
2 files changed, 46 insertions(+), 51 deletions(-)
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index b44184fed1..4837b31388 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -40,9 +40,27 @@ static void validate_teardown(TestInputVisitorData *data,
}
}
-/* 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()) */
+/* The various test_init functions are provided instead of a test setup
+ function so that the JSON string used by the tests are kept in the test
+ functions (and not in main()). */
+static Visitor *validate_test_init_internal(TestInputVisitorData *data,
+ const char *json_string,
+ va_list *ap)
+{
+ Visitor *v;
+
+ data->obj = qobject_from_jsonv(json_string, ap);
+ g_assert(data->obj);
+
+ data->qiv = qmp_input_visitor_new_strict(data->obj);
+ g_assert(data->qiv);
+
+ v = qmp_input_get_visitor(data->qiv);
+ g_assert(v);
+
+ return v;
+}
+
static GCC_FMT_ATTR(2, 3)
Visitor *validate_test_init(TestInputVisitorData *data,
const char *json_string, ...)
@@ -51,17 +69,8 @@ Visitor *validate_test_init(TestInputVisitorData *data,
va_list ap;
va_start(ap, json_string);
- data->obj = qobject_from_jsonv(json_string, &ap);
+ v = validate_test_init_internal(data, 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;
}
@@ -75,18 +84,7 @@ Visitor *validate_test_init(TestInputVisitorData *data,
static Visitor *validate_test_init_raw(TestInputVisitorData *data,
const char *json_string)
{
- Visitor *v;
-
- data->obj = qobject_from_json(json_string);
- 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;
+ return validate_test_init_internal(data, json_string, NULL);
}
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 3f6bc4d2af..8856f3169f 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -36,9 +36,27 @@ static void visitor_input_teardown(TestInputVisitorData *data,
}
}
-/* 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()) */
+/* The various test_init functions are provided instead of a test setup
+ function so that the JSON string used by the tests are kept in the test
+ functions (and not in main()). */
+static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
+ const char *json_string,
+ va_list *ap)
+{
+ Visitor *v;
+
+ data->obj = qobject_from_jsonv(json_string, ap);
+ g_assert(data->obj);
+
+ data->qiv = qmp_input_visitor_new(data->obj);
+ g_assert(data->qiv);
+
+ v = qmp_input_get_visitor(data->qiv);
+ g_assert(v);
+
+ return v;
+}
+
static GCC_FMT_ATTR(2, 3)
Visitor *visitor_input_test_init(TestInputVisitorData *data,
const char *json_string, ...)
@@ -47,17 +65,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
va_list ap;
va_start(ap, json_string);
- data->obj = qobject_from_jsonv(json_string, &ap);
+ v = visitor_input_test_init_internal(data, json_string, &ap);
va_end(ap);
-
- g_assert(data->obj != NULL);
-
- data->qiv = qmp_input_visitor_new(data->obj);
- g_assert(data->qiv != NULL);
-
- v = qmp_input_get_visitor(data->qiv);
- g_assert(v != NULL);
-
return v;
}
@@ -71,19 +80,7 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
const char *json_string)
{
- Visitor *v;
-
- data->obj = qobject_from_json(json_string);
-
- g_assert(data->obj != NULL);
-
- data->qiv = qmp_input_visitor_new(data->obj);
- g_assert(data->qiv != NULL);
-
- v = qmp_input_get_visitor(data->qiv);
- g_assert(v != NULL);
-
- return v;
+ return visitor_input_test_init_internal(data, json_string, NULL);
}
static void test_visitor_in_int(TestInputVisitorData *data,
From b18f1141d0afa00de11a8e079f4f5305c9e36893 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:29 -0700
Subject: [PATCH 05/12] qapi: Plug leaks in test-qmp-*
Make valgrind happy with the current state of the tests, so that
it is easier to see if future patches introduce new memory problems
without being drowned in noise. Many of the leaks were due to
calling a second init without tearing down the data from an earlier
visit. But since teardown is already idempotent, and we already
register teardown as part of input_visitor_test_add(), it is nicer
to just make init() safe to call multiple times than it is to have
to make all tests call teardown.
Another common leak was forgetting to clean up an error object,
after testing that an error was raised.
Another leak was in test_visitor_in_struct_nested(), failing to
clean the base member of UserDefTwo. Cleaning that up left
check_and_free_str() as dead code (since using the qapi_free_*
takes care of recursion, and we don't want double frees).
A final leak was in test_visitor_out_any(), which was reassigning
the qobj local variable to a subset of the overall structure
needing freeing; it did not result in a use-after-free, but
was not cleaning up all the qdict.
test-qmp-event and test-qmp-commands were already clean.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-6-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-input-strict.c | 9 ++++++++
tests/test-qmp-input-visitor.c | 41 ++++++---------------------------
tests/test-qmp-output-visitor.c | 3 ++-
3 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 4837b31388..821efe025c 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -49,6 +49,8 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data,
{
Visitor *v;
+ validate_teardown(data, NULL);
+
data->obj = qobject_from_jsonv(json_string, ap);
g_assert(data->obj);
@@ -191,6 +193,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data,
visit_type_TestStruct(v, &p, NULL, &err);
g_assert(err);
+ error_free(err);
if (p) {
g_free(p->string);
}
@@ -208,6 +211,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data,
visit_type_UserDefTwo(v, &udp, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefTwo(udp);
}
@@ -222,6 +226,7 @@ static void test_validate_fail_list(TestInputVisitorData *data,
visit_type_UserDefOneList(v, &head, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefOneList(head);
}
@@ -237,6 +242,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefNativeListUnion(tmp);
}
@@ -251,6 +257,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,
visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefFlatUnion(tmp);
}
@@ -266,6 +273,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefFlatUnion2(tmp);
}
@@ -280,6 +288,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data,
visit_type_UserDefAlternate(v, &tmp, NULL, &err);
g_assert(err);
+ error_free(err);
qapi_free_UserDefAlternate(tmp);
}
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 8856f3169f..8d49a9724d 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -45,6 +45,8 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
{
Visitor *v;
+ visitor_input_teardown(data, NULL);
+
data->obj = qobject_from_jsonv(json_string, ap);
g_assert(data->obj);
@@ -174,12 +176,7 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
visit_type_EnumOne(v, &res, NULL, &err);
g_assert(!err);
g_assert_cmpint(i, ==, res);
-
- visitor_input_teardown(data, NULL);
}
-
- data->obj = NULL;
- data->qiv = NULL;
}
@@ -202,12 +199,6 @@ static void test_visitor_in_struct(TestInputVisitorData *data,
g_free(p);
}
-static void check_and_free_str(char *str, const char *cmp)
-{
- g_assert_cmpstr(str, ==, cmp);
- g_free(str);
-}
-
static void test_visitor_in_struct_nested(TestInputVisitorData *data,
const void *unused)
{
@@ -223,17 +214,14 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
visit_type_UserDefTwo(v, &udp, NULL, &err);
g_assert(!err);
- check_and_free_str(udp->string0, "string0");
- check_and_free_str(udp->dict1->string1, "string1");
+ g_assert_cmpstr(udp->string0, ==, "string0");
+ g_assert_cmpstr(udp->dict1->string1, ==, "string1");
g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42);
- check_and_free_str(udp->dict1->dict2->userdef->string, "string");
- check_and_free_str(udp->dict1->dict2->string, "string2");
+ g_assert_cmpstr(udp->dict1->dict2->userdef->string, ==, "string");
+ g_assert_cmpstr(udp->dict1->dict2->string, ==, "string2");
g_assert(udp->dict1->has_dict3 == false);
- g_free(udp->dict1->dict2->userdef);
- g_free(udp->dict1->dict2);
- g_free(udp->dict1);
- g_free(udp);
+ qapi_free_UserDefTwo(udp);
}
static void test_visitor_in_list(TestInputVisitorData *data,
@@ -343,14 +331,12 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
g_assert_cmpint(tmp->u.i, ==, 42);
qapi_free_UserDefAlternate(tmp);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "'string'");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "false");
visit_type_UserDefAlternate(v, &tmp, NULL, &err);
@@ -358,7 +344,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
error_free(err);
err = NULL;
qapi_free_UserDefAlternate(tmp);
- visitor_input_teardown(data, NULL);
}
static void test_visitor_in_alternate_number(TestInputVisitorData *data,
@@ -381,7 +366,6 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free(err);
err = NULL;
qapi_free_AltStrBool(asb);
- visitor_input_teardown(data, NULL);
/* FIXME: Order of alternate should not affect semantics; asn should
* parse the same as ans */
@@ -393,35 +377,30 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free(err);
err = NULL;
qapi_free_AltStrNum(asn);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
g_assert_cmpfloat(ans->u.n, ==, 42);
qapi_free_AltNumStr(ans);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltStrInt(v, &asi, NULL, &error_abort);
g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
g_assert_cmpint(asi->u.i, ==, 42);
qapi_free_AltStrInt(asi);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
g_assert_cmpint(ain->u.i, ==, 42);
qapi_free_AltIntNum(ain);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
- visitor_input_teardown(data, NULL);
/* Parsing a double */
@@ -431,21 +410,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free(err);
err = NULL;
qapi_free_AltStrBool(asb);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrNum(v, &asn, NULL, &error_abort);
g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
g_assert_cmpfloat(asn->u.n, ==, 42.5);
qapi_free_AltStrNum(asn);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
g_assert_cmpfloat(ans->u.n, ==, 42.5);
qapi_free_AltNumStr(ans);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrInt(v, &asi, NULL, &err);
@@ -453,21 +429,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free(err);
err = NULL;
qapi_free_AltStrInt(asi);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
g_assert_cmpfloat(ain->u.n, ==, 42.5);
qapi_free_AltIntNum(ain);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
g_assert_cmpfloat(ani->u.n, ==, 42.5);
qapi_free_AltNumInt(ani);
- visitor_input_teardown(data, NULL);
}
static void test_native_list_integer_helper(TestInputVisitorData *data,
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 9364843218..8606111d45 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
qobj = QOBJECT(qdict);
visit_type_any(data->ov, &qobj, NULL, &err);
g_assert(!err);
+ qobject_decref(qobj);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
qdict = qobject_to_qdict(obj);
@@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
g_assert(qstring);
g_assert_cmpstr(qstring_get_str(qstring), ==, "foo");
qobject_decref(obj);
- qobject_decref(qobj);
}
static void test_visitor_out_union_flat(TestOutputVisitorData *data,
@@ -463,6 +463,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
qapi_free_UserDefAlternate(tmp);
+ qobject_decref(arg);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
From 3f66f764ee25f10d3e1144ebc057a949421b7728 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:30 -0700
Subject: [PATCH 06/12] qapi: Simplify non-error testing in test-qmp-*
By using &error_abort, we can avoid a local err variable in
situations where we expect success. It also has the nice
effect that if the test breaks, the error message from
error_abort tends to be nicer than that of g_assert().
This patch has an additional bonus of fixing several call sites that
were passing &err to two different functions without checking it in
between. In general that is unsafe practice; because if the first
function sets an error, the second function could abort() if it tries to
set a different error. We got away with it because we were asserting
that err was NULL through the entire chain, but switching to
&error_abort avoids the questionable practice up front.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-7-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-input-strict.c | 31 ++++------------
tests/test-qmp-input-visitor.c | 59 ++++++++----------------------
tests/test-qmp-output-visitor.c | 56 +++++++---------------------
tests/test-visitor-serialization.c | 42 +++++++++------------
4 files changed, 53 insertions(+), 135 deletions(-)
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 821efe025c..d91539c647 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -94,13 +94,11 @@ static void test_validate_struct(TestInputVisitorData *data,
const void *unused)
{
TestStruct *p = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
- visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(!err);
+ visit_type_TestStruct(v, &p, NULL, &error_abort);
g_free(p->string);
g_free(p);
}
@@ -109,7 +107,6 @@ static void test_validate_struct_nested(TestInputVisitorData *data,
const void *unused)
{
UserDefTwo *udp = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init(data, "{ 'string0': 'string0', "
@@ -117,8 +114,7 @@ static void test_validate_struct_nested(TestInputVisitorData *data,
"'dict2': { 'userdef': { 'integer': 42, "
"'string': 'string' }, 'string': 'string2'}}}");
- visit_type_UserDefTwo(v, &udp, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefTwo(v, &udp, NULL, &error_abort);
qapi_free_UserDefTwo(udp);
}
@@ -126,13 +122,11 @@ static void test_validate_list(TestInputVisitorData *data,
const void *unused)
{
UserDefOneList *head = NULL;
- Error *err = 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, &err);
- g_assert(!err);
+ visit_type_UserDefOneList(v, &head, NULL, &error_abort);
qapi_free_UserDefOneList(head);
}
@@ -141,12 +135,10 @@ static void test_validate_union_native_list(TestInputVisitorData *data,
{
UserDefNativeListUnion *tmp = NULL;
Visitor *v;
- Error *err = NULL;
v = validate_test_init(data, "{ 'type': 'integer', 'data' : [ 1, 2 ] }");
- visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefNativeListUnion(v, &tmp, NULL, &error_abort);
qapi_free_UserDefNativeListUnion(tmp);
}
@@ -155,7 +147,6 @@ static void test_validate_union_flat(TestInputVisitorData *data,
{
UserDefFlatUnion *tmp = NULL;
Visitor *v;
- Error *err = NULL;
v = validate_test_init(data,
"{ 'enum1': 'value1', "
@@ -163,8 +154,7 @@ static void test_validate_union_flat(TestInputVisitorData *data,
"'string': 'str', "
"'boolean': true }");
- visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefFlatUnion(v, &tmp, NULL, &error_abort);
qapi_free_UserDefFlatUnion(tmp);
}
@@ -173,12 +163,10 @@ static void test_validate_alternate(TestInputVisitorData *data,
{
UserDefAlternate *tmp = NULL;
Visitor *v;
- Error *err = NULL;
v = validate_test_init(data, "42");
- visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
qapi_free_UserDefAlternate(tmp);
}
@@ -296,16 +284,11 @@ static void do_test_validate_qmp_introspect(TestInputVisitorData *data,
const char *schema_json)
{
SchemaInfoList *schema = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init_raw(data, schema_json);
- visit_type_SchemaInfoList(v, &schema, NULL, &err);
- if (err) {
- fprintf(stderr, "%s", error_get_pretty(err));
- }
- g_assert(!err);
+ visit_type_SchemaInfoList(v, &schema, NULL, &error_abort);
g_assert(schema);
qapi_free_SchemaInfoList(schema);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 8d49a9724d..630a69fefd 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -89,13 +89,11 @@ static void test_visitor_in_int(TestInputVisitorData *data,
const void *unused)
{
int64_t res = 0, value = -42;
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "%" PRId64, value);
- visit_type_int(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_int(v, &res, NULL, &error_abort);
g_assert_cmpint(res, ==, value);
}
@@ -120,14 +118,12 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data,
static void test_visitor_in_bool(TestInputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
bool res = false;
Visitor *v;
v = visitor_input_test_init(data, "true");
- visit_type_bool(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_bool(v, &res, NULL, &error_abort);
g_assert_cmpint(res, ==, true);
}
@@ -135,13 +131,11 @@ static void test_visitor_in_number(TestInputVisitorData *data,
const void *unused)
{
double res = 0, value = 3.14;
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "%f", value);
- visit_type_number(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_number(v, &res, NULL, &error_abort);
g_assert_cmpfloat(res, ==, value);
}
@@ -149,13 +143,11 @@ static void test_visitor_in_string(TestInputVisitorData *data,
const void *unused)
{
char *res = NULL, *value = (char *) "Q E M U";
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "%s", value);
- visit_type_str(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_str(v, &res, NULL, &error_abort);
g_assert_cmpstr(res, ==, value);
g_free(res);
@@ -164,7 +156,6 @@ static void test_visitor_in_string(TestInputVisitorData *data,
static void test_visitor_in_enum(TestInputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
Visitor *v;
EnumOne i;
@@ -173,8 +164,7 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
v = visitor_input_test_init(data, "%s", EnumOne_lookup[i]);
- visit_type_EnumOne(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_EnumOne(v, &res, NULL, &error_abort);
g_assert_cmpint(i, ==, res);
}
}
@@ -184,13 +174,11 @@ static void test_visitor_in_struct(TestInputVisitorData *data,
const void *unused)
{
TestStruct *p = NULL;
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
- visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(!err);
+ visit_type_TestStruct(v, &p, NULL, &error_abort);
g_assert_cmpint(p->integer, ==, -42);
g_assert(p->boolean == true);
g_assert_cmpstr(p->string, ==, "foo");
@@ -203,7 +191,6 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
const void *unused)
{
UserDefTwo *udp = NULL;
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "{ 'string0': 'string0', "
@@ -211,8 +198,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
"'dict2': { 'userdef': { 'integer': 42, "
"'string': 'string' }, 'string': 'string2'}}}");
- visit_type_UserDefTwo(v, &udp, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefTwo(v, &udp, NULL, &error_abort);
g_assert_cmpstr(udp->string0, ==, "string0");
g_assert_cmpstr(udp->dict1->string1, ==, "string1");
@@ -228,14 +214,12 @@ static void test_visitor_in_list(TestInputVisitorData *data,
const void *unused)
{
UserDefOneList *item, *head = NULL;
- Error *err = NULL;
Visitor *v;
int i;
v = visitor_input_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 } ]");
- visit_type_UserDefOneList(v, &head, NULL, &err);
- g_assert(!err);
+ visit_type_UserDefOneList(v, &head, NULL, &error_abort);
g_assert(head != NULL);
for (i = 0, item = head; item; item = item->next, i++) {
@@ -253,7 +237,6 @@ static void test_visitor_in_any(TestInputVisitorData *data,
const void *unused)
{
QObject *res = NULL;
- Error *err = NULL;
Visitor *v;
QInt *qint;
QBool *qbool;
@@ -262,16 +245,14 @@ static void test_visitor_in_any(TestInputVisitorData *data,
QObject *qobj;
v = visitor_input_test_init(data, "-42");
- visit_type_any(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_any(v, &res, NULL, &error_abort);
qint = qobject_to_qint(res);
g_assert(qint);
g_assert_cmpint(qint_get_int(qint), ==, -42);
qobject_decref(res);
v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
- visit_type_any(v, &res, NULL, &err);
- g_assert(!err);
+ visit_type_any(v, &res, NULL, &error_abort);
qdict = qobject_to_qdict(res);
g_assert(qdict && qdict_size(qdict) == 3);
qobj = qdict_get(qdict, "integer");
@@ -296,7 +277,6 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
const void *unused)
{
Visitor *v;
- Error *err = NULL;
UserDefFlatUnion *tmp;
UserDefUnionBase *base;
@@ -306,8 +286,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
"'string': 'str', "
"'boolean': true }");
- visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefFlatUnion(v, &tmp, NULL, &error_abort);
g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
g_assert_cmpstr(tmp->string, ==, "str");
g_assert_cmpint(tmp->integer, ==, 41);
@@ -448,7 +427,6 @@ static void test_native_list_integer_helper(TestInputVisitorData *data,
UserDefNativeListUnionKind kind)
{
UserDefNativeListUnion *cvalue = NULL;
- Error *err = NULL;
Visitor *v;
GString *gstr_list = g_string_new("");
GString *gstr_union = g_string_new("");
@@ -465,8 +443,7 @@ static void test_native_list_integer_helper(TestInputVisitorData *data,
gstr_list->str);
v = visitor_input_test_init_raw(data, gstr_union->str);
- visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &error_abort);
g_assert(cvalue != NULL);
g_assert_cmpint(cvalue->type, ==, kind);
@@ -611,7 +588,6 @@ static void test_visitor_in_native_list_bool(TestInputVisitorData *data,
{
UserDefNativeListUnion *cvalue = NULL;
boolList *elem = NULL;
- Error *err = NULL;
Visitor *v;
GString *gstr_list = g_string_new("");
GString *gstr_union = g_string_new("");
@@ -628,8 +604,7 @@ static void test_visitor_in_native_list_bool(TestInputVisitorData *data,
gstr_list->str);
v = visitor_input_test_init_raw(data, gstr_union->str);
- visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &error_abort);
g_assert(cvalue != NULL);
g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
@@ -647,7 +622,6 @@ static void test_visitor_in_native_list_string(TestInputVisitorData *data,
{
UserDefNativeListUnion *cvalue = NULL;
strList *elem = NULL;
- Error *err = NULL;
Visitor *v;
GString *gstr_list = g_string_new("");
GString *gstr_union = g_string_new("");
@@ -663,8 +637,7 @@ static void test_visitor_in_native_list_string(TestInputVisitorData *data,
gstr_list->str);
v = visitor_input_test_init_raw(data, gstr_union->str);
- visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &error_abort);
g_assert(cvalue != NULL);
g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
@@ -686,7 +659,6 @@ static void test_visitor_in_native_list_number(TestInputVisitorData *data,
{
UserDefNativeListUnion *cvalue = NULL;
numberList *elem = NULL;
- Error *err = NULL;
Visitor *v;
GString *gstr_list = g_string_new("");
GString *gstr_union = g_string_new("");
@@ -702,8 +674,7 @@ static void test_visitor_in_native_list_number(TestInputVisitorData *data,
gstr_list->str);
v = visitor_input_test_init_raw(data, gstr_union->str);
- visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &error_abort);
g_assert(cvalue != NULL);
g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 8606111d45..0164984ec6 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -45,11 +45,9 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
const void *unused)
{
int64_t value = -42;
- Error *err = NULL;
QObject *obj;
- visit_type_int(data->ov, &value, NULL, &err);
- g_assert(!err);
+ visit_type_int(data->ov, &value, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -62,12 +60,10 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
static void test_visitor_out_bool(TestOutputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
bool value = true;
QObject *obj;
- visit_type_bool(data->ov, &value, NULL, &err);
- g_assert(!err);
+ visit_type_bool(data->ov, &value, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -81,11 +77,9 @@ static void test_visitor_out_number(TestOutputVisitorData *data,
const void *unused)
{
double value = 3.14;
- Error *err = NULL;
QObject *obj;
- visit_type_number(data->ov, &value, NULL, &err);
- g_assert(!err);
+ visit_type_number(data->ov, &value, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -99,11 +93,9 @@ static void test_visitor_out_string(TestOutputVisitorData *data,
const void *unused)
{
char *string = (char *) "Q E M U";
- Error *err = NULL;
QObject *obj;
- visit_type_str(data->ov, &string, NULL, &err);
- g_assert(!err);
+ visit_type_str(data->ov, &string, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -117,12 +109,10 @@ static void test_visitor_out_no_string(TestOutputVisitorData *data,
const void *unused)
{
char *string = NULL;
- Error *err = NULL;
QObject *obj;
/* A null string should return "" */
- visit_type_str(data->ov, &string, NULL, &err);
- g_assert(!err);
+ visit_type_str(data->ov, &string, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -135,13 +125,11 @@ static void test_visitor_out_no_string(TestOutputVisitorData *data,
static void test_visitor_out_enum(TestOutputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
QObject *obj;
EnumOne i;
for (i = 0; i < ENUM_ONE_MAX; i++) {
- visit_type_EnumOne(data->ov, &i, "unused", &err);
- g_assert(!err);
+ visit_type_EnumOne(data->ov, &i, "unused", &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -174,12 +162,10 @@ static void test_visitor_out_struct(TestOutputVisitorData *data,
.boolean = false,
.string = (char *) "foo"};
TestStruct *p = &test_struct;
- Error *err = NULL;
QObject *obj;
QDict *qdict;
- visit_type_TestStruct(data->ov, &p, NULL, &err);
- g_assert(!err);
+ visit_type_TestStruct(data->ov, &p, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -198,7 +184,6 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
const void *unused)
{
int64_t value = 42;
- Error *err = NULL;
UserDefTwo *ud2;
QObject *obj;
QDict *qdict, *dict1, *dict2, *dict3, *userdef;
@@ -225,8 +210,7 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
ud2->dict1->dict3->userdef->integer = value;
ud2->dict1->dict3->string = g_strdup(strings[3]);
- visit_type_UserDefTwo(data->ov, &ud2, "unused", &err);
- g_assert(!err);
+ visit_type_UserDefTwo(data->ov, &ud2, "unused", &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -288,7 +272,6 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
const int max_items = 10;
bool value_bool = true;
int value_int = 10;
- Error *err = NULL;
QListEntry *entry;
QObject *obj;
QList *qlist;
@@ -306,8 +289,7 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
head = p;
}
- visit_type_TestStructList(data->ov, &head, NULL, &err);
- g_assert(!err);
+ visit_type_TestStructList(data->ov, &head, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -367,7 +349,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
const void *unused)
{
QObject *qobj;
- Error *err = NULL;
QInt *qint;
QBool *qbool;
QString *qstring;
@@ -375,8 +356,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
QObject *obj;
qobj = QOBJECT(qint_from_int(-42));
- visit_type_any(data->ov, &qobj, NULL, &err);
- g_assert(!err);
+ visit_type_any(data->ov, &qobj, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
g_assert(qobject_type(obj) == QTYPE_QINT);
@@ -389,8 +369,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
qdict_put(qdict, "boolean", qbool_from_bool(true));
qdict_put(qdict, "string", qstring_from_str("foo"));
qobj = QOBJECT(qdict);
- visit_type_any(data->ov, &qobj, NULL, &err);
- g_assert(!err);
+ visit_type_any(data->ov, &qobj, NULL, &error_abort);
qobject_decref(qobj);
obj = qmp_output_get_qobject(data->qov);
g_assert(obj != NULL);
@@ -420,8 +399,6 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
QObject *arg;
QDict *qdict;
- Error *err = NULL;
-
UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
tmp->enum1 = ENUM_ONE_VALUE1;
tmp->string = g_strdup("str");
@@ -429,8 +406,7 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
tmp->integer = 41;
tmp->u.value1->boolean = true;
- visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &error_abort);
arg = qmp_output_get_qobject(data->qov);
g_assert(qobject_type(arg) == QTYPE_QDICT);
@@ -449,14 +425,12 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
const void *unused)
{
QObject *arg;
- Error *err = NULL;
UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
tmp->type = USER_DEF_ALTERNATE_KIND_I;
tmp->u.i = 42;
- visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
arg = qmp_output_get_qobject(data->qov);
g_assert(qobject_type(arg) == QTYPE_QINT);
@@ -697,14 +671,12 @@ static void test_native_list(TestOutputVisitorData *data,
UserDefNativeListUnionKind kind)
{
UserDefNativeListUnion *cvalue = g_new0(UserDefNativeListUnion, 1);
- Error *err = NULL;
QObject *obj;
cvalue->type = kind;
init_native_list(cvalue);
- visit_type_UserDefNativeListUnion(data->ov, &cvalue, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefNativeListUnion(data->ov, &cvalue, NULL, &error_abort);
obj = qmp_output_get_qobject(data->qov);
check_native_list(obj, cvalue->type);
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index c024e5eaf4..9f67f9e003 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -302,14 +302,13 @@ static void test_primitives(gconstpointer opaque)
const SerializeOps *ops = args->ops;
PrimitiveType *pt = args->test_data;
PrimitiveType *pt_copy = g_malloc0(sizeof(*pt_copy));
- Error *err = NULL;
void *serialize_data;
pt_copy->type = pt->type;
- ops->serialize(pt, &serialize_data, visit_primitive_type, &err);
- ops->deserialize((void **)&pt_copy, serialize_data, visit_primitive_type, &err);
+ ops->serialize(pt, &serialize_data, visit_primitive_type, &error_abort);
+ ops->deserialize((void **)&pt_copy, serialize_data, visit_primitive_type,
+ &error_abort);
- g_assert(err == NULL);
g_assert(pt_copy != NULL);
if (pt->type == PTYPE_STRING) {
g_assert_cmpstr(pt->value.string, ==, pt_copy->value.string);
@@ -345,7 +344,6 @@ static void test_primitive_lists(gconstpointer opaque)
PrimitiveList pl = { .value = { NULL } };
PrimitiveList pl_copy = { .value = { NULL } };
PrimitiveList *pl_copy_ptr = &pl_copy;
- Error *err = NULL;
void *serialize_data;
void *cur_head = NULL;
int i;
@@ -492,10 +490,11 @@ static void test_primitive_lists(gconstpointer opaque)
}
}
- ops->serialize((void **)&pl, &serialize_data, visit_primitive_list, &err);
- ops->deserialize((void **)&pl_copy_ptr, serialize_data, visit_primitive_list, &err);
+ ops->serialize((void **)&pl, &serialize_data, visit_primitive_list,
+ &error_abort);
+ ops->deserialize((void **)&pl_copy_ptr, serialize_data,
+ visit_primitive_list, &error_abort);
- g_assert(err == NULL);
i = 0;
/* compare our deserialized list of primitives to the original */
@@ -652,10 +651,8 @@ static void test_primitive_lists(gconstpointer opaque)
g_assert_cmpint(i, ==, 33);
ops->cleanup(serialize_data);
- dealloc_helper(&pl, visit_primitive_list, &err);
- g_assert(!err);
- dealloc_helper(&pl_copy, visit_primitive_list, &err);
- g_assert(!err);
+ dealloc_helper(&pl, visit_primitive_list, &error_abort);
+ dealloc_helper(&pl_copy, visit_primitive_list, &error_abort);
g_free(args);
}
@@ -665,13 +662,12 @@ static void test_struct(gconstpointer opaque)
const SerializeOps *ops = args->ops;
TestStruct *ts = struct_create();
TestStruct *ts_copy = NULL;
- Error *err = NULL;
void *serialize_data;
- ops->serialize(ts, &serialize_data, visit_struct, &err);
- ops->deserialize((void **)&ts_copy, serialize_data, visit_struct, &err);
+ ops->serialize(ts, &serialize_data, visit_struct, &error_abort);
+ ops->deserialize((void **)&ts_copy, serialize_data, visit_struct,
+ &error_abort);
- g_assert(err == NULL);
struct_compare(ts, ts_copy);
struct_cleanup(ts);
@@ -687,14 +683,12 @@ static void test_nested_struct(gconstpointer opaque)
const SerializeOps *ops = args->ops;
UserDefTwo *udnp = nested_struct_create();
UserDefTwo *udnp_copy = NULL;
- Error *err = NULL;
void *serialize_data;
- ops->serialize(udnp, &serialize_data, visit_nested_struct, &err);
+ ops->serialize(udnp, &serialize_data, visit_nested_struct, &error_abort);
ops->deserialize((void **)&udnp_copy, serialize_data, visit_nested_struct,
- &err);
+ &error_abort);
- g_assert(err == NULL);
nested_struct_compare(udnp, udnp_copy);
nested_struct_cleanup(udnp);
@@ -709,7 +703,6 @@ static void test_nested_struct_list(gconstpointer opaque)
TestArgs *args = (TestArgs *) opaque;
const SerializeOps *ops = args->ops;
UserDefTwoList *listp = NULL, *tmp, *tmp_copy, *listp_copy = NULL;
- Error *err = NULL;
void *serialize_data;
int i = 0;
@@ -720,11 +713,10 @@ static void test_nested_struct_list(gconstpointer opaque)
listp = tmp;
}
- ops->serialize(listp, &serialize_data, visit_nested_struct_list, &err);
+ ops->serialize(listp, &serialize_data, visit_nested_struct_list,
+ &error_abort);
ops->deserialize((void **)&listp_copy, serialize_data,
- visit_nested_struct_list, &err);
-
- g_assert(err == NULL);
+ visit_nested_struct_list, &error_abort);
tmp = listp;
tmp_copy = listp_copy;
From a12a5a1a0132527afe87c079e4aae4aad372bd94 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:31 -0700
Subject: [PATCH 07/12] qapi: Simplify error cleanup in test-qmp-*
We have several tests that perform multiple sub-actions that are
expected to fail. Asserting that an error occurred, then clearing
it up to prepare for the next action, turned into enough
boilerplate that it was sometimes forgotten (for example, a number
of tests added to test-qmp-input-visitor.c in d88f5fd leaked err).
Worse, if an error is not reset to NULL, we risk invalidating
later use of that error (passing a non-NULL err into a function
is generally a bad idea). Encapsulate the boilerplate into a
single helper function error_free_or_abort(), and consistently
use it.
The new function is added into error.c for use everywhere,
although it is anticipated that testsuites will be the main
client.
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 9 +++++++++
tests/test-qmp-commands.c | 3 +--
tests/test-qmp-input-strict.c | 21 +++++++--------------
tests/test-qmp-input-visitor.c | 26 +++++++-------------------
util/error.c | 7 +++++++
5 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index c69dddbbf2..4d42cdc5fd 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -30,6 +30,10 @@
* Handle an error without reporting it (just for completeness):
* error_free(err);
*
+ * Assert that an expected error occurred, but clean it up without
+ * reporting it (primarily useful in testsuites):
+ * error_free_or_abort(&err);
+ *
* Pass an existing error to the caller:
* error_propagate(errp, err);
* where Error **errp is a parameter, by convention the last one.
@@ -189,6 +193,11 @@ Error *error_copy(const Error *err);
*/
void error_free(Error *err);
+/*
+ * Convenience function to assert that *@errp is set, then silently free it.
+ */
+void error_free_or_abort(Error **errp);
+
/*
* Convenience function to error_report() and free @err.
*/
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index f23d8eaf2a..888fb5ffec 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -225,8 +225,7 @@ static void test_dealloc_partial(void)
assert(ud2->dict1 == NULL);
/* confirm & release construction error */
- assert(err != NULL);
- error_free(err);
+ error_free_or_abort(&err);
/* 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 d91539c647..f1c2e3ba67 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -180,8 +180,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data,
v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
if (p) {
g_free(p->string);
}
@@ -198,8 +197,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data,
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_UserDefTwo(v, &udp, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefTwo(udp);
}
@@ -213,8 +211,7 @@ static void test_validate_fail_list(TestInputVisitorData *data,
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, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefOneList(head);
}
@@ -229,8 +226,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data,
"{ 'type': 'integer', 'data' : [ 'string' ] }");
visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefNativeListUnion(tmp);
}
@@ -244,8 +240,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,
v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefFlatUnion(tmp);
}
@@ -260,8 +255,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
v = validate_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }");
visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefFlatUnion2(tmp);
}
@@ -275,8 +269,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data,
v = validate_test_init(data, "3.14");
visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
qapi_free_UserDefAlternate(tmp);
}
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 630a69fefd..ef5284daa2 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -111,8 +111,7 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data,
v = visitor_input_test_init(data, "%f", DBL_MAX);
visit_type_int(v, &res, NULL, &err);
- g_assert(err);
- error_free(err);
+ error_free_or_abort(&err);
}
static void test_visitor_in_bool(TestInputVisitorData *data,
@@ -319,9 +318,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "false");
visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_UserDefAlternate(tmp);
}
@@ -341,9 +338,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_AltStrBool(v, &asb, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
/* FIXME: Order of alternate should not affect semantics; asn should
@@ -352,9 +347,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
visit_type_AltStrNum(v, &asn, NULL, &err);
/* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
/* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrNum(asn);
v = visitor_input_test_init(data, "42");
@@ -385,9 +378,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrBool(v, &asb, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
v = visitor_input_test_init(data, "42.5");
@@ -404,9 +395,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrInt(v, &asi, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
qapi_free_AltStrInt(asi);
v = visitor_input_test_init(data, "42.5");
@@ -713,12 +702,11 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(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);
- error_free(err);
g_free(p->string);
g_free(p);
}
diff --git a/util/error.c b/util/error.c
index 8b86490ba1..80c89a2079 100644
--- a/util/error.c
+++ b/util/error.c
@@ -220,6 +220,13 @@ void error_free(Error *err)
}
}
+void error_free_or_abort(Error **errp)
+{
+ assert(errp && *errp);
+ error_free(*errp);
+ *errp = NULL;
+}
+
void error_propagate(Error **dst_errp, Error *local_err)
{
if (!local_err) {
From 12fafd7cedad51854c468ea0496a6542b3222b29 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:32 -0700
Subject: [PATCH 08/12] qapi: More tests of alternate output
The testsuite was only covering that we could output the 'int'
branch of an alternate (no additional allocation/cleanup required).
Add a test of the 'str' branch, to make sure that things still
work even when a branch involves allocation.
Update to modern style of g_new0() over g_malloc0() while
touching it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-9-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-output-visitor.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 0164984ec6..0d0c85989a 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -425,8 +425,9 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
const void *unused)
{
QObject *arg;
+ UserDefAlternate *tmp;
- UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
+ tmp = g_new0(UserDefAlternate, 1);
tmp->type = USER_DEF_ALTERNATE_KIND_I;
tmp->u.i = 42;
@@ -438,6 +439,19 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
qapi_free_UserDefAlternate(tmp);
qobject_decref(arg);
+
+ tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = USER_DEF_ALTERNATE_KIND_S;
+ tmp->u.s = g_strdup("hello");
+
+ visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
+ arg = qmp_output_get_qobject(data->qov);
+
+ g_assert(qobject_type(arg) == QTYPE_QSTRING);
+ g_assert_cmpstr(qstring_get_str(qobject_to_qstring(arg)), ==, "hello");
+
+ qapi_free_UserDefAlternate(tmp);
+ qobject_decref(arg);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
From dd5ee2c2d3e3a17647ddd9bfa97935b8cb5dfa40 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:33 -0700
Subject: [PATCH 09/12] qapi: Test failure in middle of array parse
Our generated list visitors have the same problem as has been
mentioned elsewhere (see commit 2f52e20): they allocate data
even on failure. An upcoming patch will correct things to
provide saner guarantees, but first we need to expose the
behavior in the testsuite to ensure we aren't introducing any
memory usage bugs.
There are more test cases throughout the test-qmp-input-* tests
that already deal with partial allocation; a later commit will
clean up all visit_type_FOO(), without marking all of the tests
with FIXME at this time.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-10-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi-visit.py | 4 ++++
tests/test-qmp-input-visitor.c | 10 +++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f40c3c792f..3ef5c16a66 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -138,6 +138,10 @@ def gen_visit_struct(name, base, members):
def gen_visit_list(name, element_type):
+ # FIXME: if *obj is NULL on entry, and the first visit_next_list()
+ # assigns to *obj, while a later one fails, we should clean up *obj
+ # rather than leaving it non-NULL. As currently written, the caller must
+ # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList.
return mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index ef5284daa2..aa02eec765 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -698,8 +698,10 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
TestStruct *p = NULL;
Error *err = NULL;
Visitor *v;
+ strList *q = NULL;
- v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
+ v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', "
+ "'string': -42 }");
visit_type_TestStruct(v, &p, NULL, &err);
error_free_or_abort(&err);
@@ -709,6 +711,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
g_free(p->string);
g_free(p);
+
+ v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]");
+ visit_type_strList(v, &q, NULL, &err);
+ error_free_or_abort(&err);
+ assert(q);
+ qapi_free_strList(q);
}
int main(int argc, char **argv)
From 2533377c7b0c686d1510ed6499cedf938607e805 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:34 -0700
Subject: [PATCH 10/12] qapi: More tests of input arrays
Our testsuite had no coverage of empty arrays, nor of what
happens when the input does not match the expected type.
Useful to have, especially if we start changing the visitor
contracts.
I did not think it worth duplicating these additions to
test-qmp-input-strict; since all strict mode does is add
the ability to reject JSON input that has more keys than
what the visitor expects, yet the additions in this patch
error out earlier than that point regardless of whether
strict mode was requested.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-11-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qmp-input-visitor.c | 52 ++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index aa02eec765..d48ebdd62f 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -230,6 +230,12 @@ static void test_visitor_in_list(TestInputVisitorData *data,
}
qapi_free_UserDefOneList(head);
+ head = NULL;
+
+ /* An empty list is valid */
+ v = visitor_input_test_init(data, "[]");
+ visit_type_UserDefOneList(v, &head, NULL, &error_abort);
+ g_assert(!head);
}
static void test_visitor_in_any(TestInputVisitorData *data,
@@ -719,6 +725,50 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
qapi_free_strList(q);
}
+static void test_visitor_in_wrong_type(TestInputVisitorData *data,
+ const void *unused)
+{
+ TestStruct *p = NULL;
+ Visitor *v;
+ strList *q = NULL;
+ int64_t i;
+ Error *err = NULL;
+
+ /* Make sure arrays and structs cannot be confused */
+
+ v = visitor_input_test_init(data, "[]");
+ visit_type_TestStruct(v, &p, NULL, &err);
+ error_free_or_abort(&err);
+ g_assert(!p);
+
+ v = visitor_input_test_init(data, "{}");
+ visit_type_strList(v, &q, NULL, &err);
+ error_free_or_abort(&err);
+ assert(!q);
+
+ /* Make sure primitives and struct cannot be confused */
+
+ v = visitor_input_test_init(data, "1");
+ visit_type_TestStruct(v, &p, NULL, &err);
+ error_free_or_abort(&err);
+ g_assert(!p);
+
+ v = visitor_input_test_init(data, "{}");
+ visit_type_int(v, &i, NULL, &err);
+ error_free_or_abort(&err);
+
+ /* Make sure primitives and arrays cannot be confused */
+
+ v = visitor_input_test_init(data, "1");
+ visit_type_strList(v, &q, NULL, &err);
+ error_free_or_abort(&err);
+ assert(!q);
+
+ v = visitor_input_test_init(data, "[]");
+ visit_type_int(v, &i, NULL, &err);
+ error_free_or_abort(&err);
+}
+
int main(int argc, char **argv)
{
TestInputVisitorData in_visitor_data;
@@ -751,6 +801,8 @@ int main(int argc, char **argv)
&in_visitor_data, test_visitor_in_alternate);
input_visitor_test_add("/visitor/input/errors",
&in_visitor_data, test_visitor_in_errors);
+ input_visitor_test_add("/visitor/input/wrong-type",
+ &in_visitor_data, test_visitor_in_wrong_type);
input_visitor_test_add("/visitor/input/alternate-number",
&in_visitor_data, test_visitor_in_alternate_number);
input_visitor_test_add("/visitor/input/native_list/int",
From ce5fcb472d512455a8d13fae4c04ecf8eb00573b Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:35 -0700
Subject: [PATCH 11/12] qapi: Provide nicer array names in introspection
For the sake of humans reading introspection output, it is nice
to have the name of implicit array types be recognizable as
arrays of the underlying type. However, while this patch allows
humans to skip from a command with return type "[123]" straight
to the definition of type "123" without having to first inspect
type "[123]", document that this shortcut should not be taken by
client apps.
This makes the resulting introspection string slightly larger by
default (just over 200 bytes), but it's in the noise (less than
0.3% of the overall 70k size of 'query-qmp-capabilities').
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-12-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
docs/qapi-code-gen.txt | 7 +++++--
scripts/qapi-introspect.py | 8 +++++---
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 4d8c2fcf02..ba29bc634a 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -662,11 +662,14 @@ Example: the SchemaInfo for BlockRef from section Alternate types
The SchemaInfo for an array type has meta-type "array", and variant
member "element-type", which names the array's element type. Array
-types are implicitly defined.
+types are implicitly defined. For convenience, the array's name may
+resemble the element type; however, clients should examine member
+"element-type" instead of making assumptions based on parsing member
+"name".
Example: the SchemaInfo for ['str']
- { "name": "strList", "meta-type": "array",
+ { "name": "[str]", "meta-type": "array",
"element-type": "str" }
The SchemaInfo for an enumeration type has meta-type "enum" and
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index c0dad6679c..64f2cd0631 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -107,10 +107,12 @@ def _use_type(self, typ):
# characters.
if isinstance(typ, QAPISchemaBuiltinType):
return typ.name
+ if isinstance(typ, QAPISchemaArrayType):
+ return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
def _gen_json(self, name, mtype, obj):
- if mtype != 'command' and mtype != 'event' and mtype != 'builtin':
+ if mtype not in ('command', 'event', 'builtin', 'array'):
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
@@ -136,8 +138,8 @@ def visit_enum_type(self, name, info, values, prefix):
self._gen_json(name, 'enum', {'values': values})
def visit_array_type(self, name, info, element_type):
- self._gen_json(name, 'array',
- {'element-type': self._use_type(element_type)})
+ element = self._use_type(element_type)
+ self._gen_json('[' + element + ']', 'array', {'element-type': element})
def visit_object_type_flat(self, name, info, members, variants):
obj = {'members': [self._gen_member(m) for m in members]}
From f5455044201747fd72531f5e8c1b1e9c56573d9c Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Nov 2015 23:35:36 -0700
Subject: [PATCH 12/12] qapi-introspect: Document lack of sorting
qapi-code-gen.txt already claims that types, commands, and
events share a common namespace; set this in stone by further
documenting that our introspection output will never have
collisions with the same name tied to more than one meta-type.
Our largest QMP enum currently has 125 values, our largest
object type has 27 members, and the mean for each is less than
10. These sizes are small enough that the per-element overhead
of O(log n) binary searching probably outweighs the speed
possible with direct O(n) linear searching (a better algorithm
with more overhead will only beat a leaner naive algorithm only
as you scale to larger input sizes).
Arguably, the overall SchemaInfo array could be sorted by name;
there, we currently have 531 entities, large enough for a binary
search to be faster than linear. However, remember that we have
mutually-recursive types, which means there is no topological
ordering that will allow clients to learn all information about
that type in a single linear pass; thus clients will want to do
random access over the data, and they will probably read the
introspection output into a hashtable for O(1) lookup rather
than O(log n) binary searching, at which point, pre-sorting our
introspection output doesn't help the client.
It doesn't help that sorting can be subjective if you introduce
locales into the mix (I'm not experienced enough with Python
to know for sure, but at least it looks like it defaults to
sorting in the C locale even when run under a different locale).
And while our current introspection output is deterministic
(because we visit entities in a sorted order), we may want
to change that order in the future (such as using OrderedDict
to stick to .json declaration order).
For these reasons, we simply document that clients should not
rely on any particular order of items in introspection output.
And since it is now a documented part of the contract, we have
the freedom to later rearrange output if needed, without
worrying about breaking well-written clients.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1446791754-23823-13-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
docs/qapi-code-gen.txt | 19 +++++++++++++++----
qapi/introspect.json | 17 ++++++++++++-----
2 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index ba29bc634a..f9fa6f3d96 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -516,6 +516,10 @@ query-qmp-schema. QGA currently doesn't support introspection.
query-qmp-schema returns a JSON array of SchemaInfo objects. These
objects together describe the wire ABI, as defined in the QAPI schema.
+There is no specified order to the SchemaInfo objects returned; a
+client must search for a particular name throughout the entire array
+to learn more about that name, but is at least guaranteed that there
+will be no collisions between type, command, and event names.
However, the SchemaInfo can't reflect all the rules and restrictions
that apply to QMP. It's interface introspection (figuring out what's
@@ -596,7 +600,9 @@ any. Each element is a JSON object with members "name" (the member's
name), "type" (the name of its type), and optionally "default". The
member is optional if "default" is present. Currently, "default" can
only have value null. Other values are reserved for future
-extensions.
+extensions. The "members" array is in no particular order; clients
+must search the entire object when learning whether a particular
+member is supported.
Example: the SchemaInfo for MyType from section Struct types
@@ -610,7 +616,9 @@ Example: the SchemaInfo for MyType from section Struct types
"variants" is a JSON array describing the object's variant members.
Each element is a JSON object with members "case" (the value of type
tag this element applies to) and "type" (the name of an object type
-that provides the variant members for this type tag value).
+that provides the variant members for this type tag value). The
+"variants" array is in no particular order, and is not guaranteed to
+list cases in the same order as the corresponding "tag" enum type.
Example: the SchemaInfo for flat union BlockdevOptions from section
Union types
@@ -651,7 +659,8 @@ Union types
The SchemaInfo for an alternate type has meta-type "alternate", and
variant member "members". "members" is a JSON array. Each element is
a JSON object with member "type", which names a type. Values of the
-alternate type conform to exactly one of its member types.
+alternate type conform to exactly one of its member types. There is
+no guarantee on the order in which "members" will be listed.
Example: the SchemaInfo for BlockRef from section Alternate types
@@ -673,7 +682,9 @@ Example: the SchemaInfo for ['str']
"element-type": "str" }
The SchemaInfo for an enumeration type has meta-type "enum" and
-variant member "values".
+variant member "values". The values are listed in no particular
+order; clients must search the entire enum when learning whether a
+particular value is supported.
Example: the SchemaInfo for MyEnum from section Enumeration types
diff --git a/qapi/introspect.json b/qapi/introspect.json
index cc50dc6bcb..e7c4c3e998 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -25,6 +25,10 @@
# Returns: array of @SchemaInfo, where each element describes an
# entity in the ABI: command, event, type, ...
#
+# The order of the various SchemaInfo is unspecified; however, all
+# names are guaranteed to be unique (no name will be duplicated with
+# different meta-types).
+#
# Note: the QAPI schema is also used to help define *internal*
# interfaces, by defining QAPI types. These are not part of the QMP
# wire ABI, and therefore not returned by this command.
@@ -78,7 +82,8 @@
# Commands and events have the name defined in the QAPI schema.
# Unlike command and event names, type names are not part of
# the wire ABI. Consequently, type names are meaningless
-# strings here.
+# strings here, although they are still guaranteed unique
+# regardless of @meta-type.
#
# All references to other SchemaInfo are by name.
#
@@ -130,7 +135,7 @@
#
# Additional SchemaInfo members for meta-type 'enum'.
#
-# @values: the enumeration type's values.
+# @values: the enumeration type's values, in no particular order.
#
# Values of this type are JSON string on the wire.
#
@@ -158,14 +163,16 @@
#
# Additional SchemaInfo members for meta-type 'object'.
#
-# @members: the object type's (non-variant) members.
+# @members: the object type's (non-variant) members, in no particular order.
#
# @tag: #optional the name of the member serving as type tag.
# An element of @members with this name must exist.
#
# @variants: #optional variant members, i.e. additional members that
# depend on the type tag's value. Present exactly when
-# @tag is present.
+# @tag is present. The variants are in no particular order,
+# and may even differ from the order of the values of the
+# enum type of the @tag.
#
# Values of this type are JSON object on the wire.
#
@@ -219,7 +226,7 @@
#
# Additional SchemaInfo members for meta-type 'alternate'.
#
-# @members: the alternate type's members.
+# @members: the alternate type's members, in no particular order.
# The members' wire encoding is distinct, see
# docs/qapi-code-gen.txt section Alternate types.
#