* [Qemu-devel] [PATCH v7 01/14] qapi: Use generated TestStruct machinery in tests
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-22 15:58 ` Markus Armbruster
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 02/14] qapi: Strengthen test of TestStructList Eric Blake
` (12 subsequent siblings)
13 siblings, 1 reply; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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.
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>
---
v7: rebase on top of subset B v9; defer unrelated trailing whitespace
cleanups to later in series
v6: new patch
---
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 22e15eb..efa7acc 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 feaf20d..34fb317 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -93,6 +93,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
@@ -101,6 +102,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 53a7693..b44184f 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 617eada..c311fc6 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 09d0dd8..baf58dc 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 634563b..c024e5e 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)
{
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v7 01/14] qapi: Use generated TestStruct machinery in tests
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 01/14] qapi: Use generated TestStruct machinery in tests Eric Blake
@ 2015-10-22 15:58 ` Markus Armbruster
0 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2015-10-22 15:58 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> 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.
The only one between the hand-rolled copies I can see apart from
whitespace is tests/test-visitor-serialization.c passing NULL rather
than "TestStruct" to visit_start_struct().
Compared to the generated version, the hand-rolled copies neglect to
skip visiting members when !*obj. If I remember correctly, we do that
so we can use the visitor to deallocate partly allocated stuff (worth a
comment, by the way), and that's not necessary in these tests.
> 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>
Patch looks good.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v7 02/14] qapi: Strengthen test of TestStructList
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 01/14] qapi: Use generated TestStruct machinery in tests Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 03/14] qapi: Provide nicer array names in introspection Eric Blake
` (11 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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>
---
v7: new patch
---
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 baf58dc..9364843 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,
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 03/14] qapi: Provide nicer array names in introspection
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 01/14] qapi: Use generated TestStruct machinery in tests Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 02/14] qapi: Strengthen test of TestStructList Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-22 16:12 ` Markus Armbruster
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 04/14] qapi-introspect: Guarantee particular sorting Eric Blake
` (10 subsequent siblings)
13 siblings, 1 reply; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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, but slightly smaller when -u is in use (as '[FOO]' is
nicer than 'FOOList' for expressing 'array of FOO').
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: no change
v6: no change
---
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 df2d198..15a1c70 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 c0dad66..64f2cd0 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -107,10 +107,12 @@ const char %(c_name)s[] = %(c_string)s;
# 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 @@ const char %(c_name)s[] = %(c_string)s;
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]}
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v7 03/14] qapi: Provide nicer array names in introspection
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 03/14] qapi: Provide nicer array names in introspection Eric Blake
@ 2015-10-22 16:12 ` Markus Armbruster
0 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2015-10-22 16:12 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> 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.
Personally, I still don't see much value in special-casing array names.
On the other hand, the patch is really simple.
> This makes the resulting introspection string slightly larger by
> default, but slightly smaller when -u is in use (as '[FOO]' is
> nicer than 'FOOList' for expressing 'array of FOO').
Size with -u is unimportant.
If we don't want to pay the prize in size without -u, we can make it
conditional on -u. But with -u, [FOO] isn't much of an improvement over
FooList.
Anyway, if you want the feature, I'll take the patch.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v7 04/14] qapi-introspect: Guarantee particular sorting
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (2 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 03/14] qapi: Provide nicer array names in introspection Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions Eric Blake
` (9 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Sorting the values of an enum makes it easier to look up whether
a particular value is present, via binary rather than linear search
(probably most visible with QKeyCode, which has grown over
several releases). Additionally, QMP clients need not know which
C value is associated with an enum name, so sorting is an
effective way to hide that non-ABI aspect of qapi.
While we are at it, it is also easy to sort the members and
variants of objects, to allow for a similar binary search (although
less compelling, as any struct with that many menbers should
arguably be broken into hierarchical substructs), and equally
valid since JSON objects have no specific order in which keys must
appear. There is no trivial or obvious way to sort the types of
an alternate, so that is left unchanged.
However, the overall SchemaInfo array remains unsorted. It might
make sense to sort with 'meta-type' as a primary key and 'name'
as a secondary key, but it is not obvious that this will provide
benefits to end-user clients (we allow mutually recursive types,
so there is no posible topological sorting where a single pass
over the array could resolve all types, and while binary searches
could be made possible by sorting, it would be even more efficient
for clients to read the array into a hashtable for O(1) rather
than O(log n) random-access lookups, at which point pre-sorting is
wasted effort).
Document these conventions, so that clients will know what can
and cannot be relied on.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
TODO: should the documentation mention that the sorting is done
in the C locale? Is there anything required to ensure that python
sorts sanely (ie. that the choice of locale while building
doesn't cause inadvertent sorting differences such as turning on
case-insensitivity)?
v7: tweak commit wording
v6: no change from v5
---
docs/qapi-code-gen.txt | 21 +++++++++++++++++----
qapi/introspect.json | 22 +++++++++++++++++-----
scripts/qapi-introspect.py | 9 ++++++---
3 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 15a1c70..e970440 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -516,6 +516,13 @@ 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 and meta-type throughout the
+entire array to learn more about that name. For now, the name for
+each SchemaInfo is unique thanks to qapi naming conventions; however
+this may be changed in the future (such as allowing a command and an
+event with the same name), so it is important that the client check
+for the desired meta-type.
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 +603,8 @@ 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 sorted by "name", so that clients
+can use a binary search to see if a particular member is supported.
Example: the SchemaInfo for MyType from section Struct types
@@ -610,7 +618,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 sorted by "case", so it appears in the same
+order as the enum type matching "tag".
Example: the SchemaInfo for flat union BlockdevOptions from section
Union types
@@ -651,7 +661,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 +684,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 sorted order,
+so clients can use a binary search to see if a particular value
+is present.
Example: the SchemaInfo for MyEnum from section Enumeration types
diff --git a/qapi/introspect.json b/qapi/introspect.json
index cc50dc6..71632af 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -25,6 +25,11 @@
# Returns: array of @SchemaInfo, where each element describes an
# entity in the ABI: command, event, type, ...
#
+# The order of the various SchemaInfo is unspecified. For now, the
+# name of each SchemaInfo is unique regardless of meta-type, but to be
+# safe, clients should check that a given name has the expected
+# meta-type.
+#
# 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 +83,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 all names are currently unique
+# regardless of @meta-type, clients should not rely on this.
#
# All references to other SchemaInfo are by name.
#
@@ -130,7 +136,9 @@
#
# Additional SchemaInfo members for meta-type 'enum'.
#
-# @values: the enumeration type's values.
+# @values: the enumeration type's values. The values are sorted, so
+# clients can use a binary search to see if a particular value
+# is present.
#
# Values of this type are JSON string on the wire.
#
@@ -158,14 +166,18 @@
#
# Additional SchemaInfo members for meta-type 'object'.
#
-# @members: the object type's (non-variant) members.
+# @members: the object type's (non-variant) members. The members are
+# sorted by name, so clients can use a binary search to see
+# if a given member is present.
#
# @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 sorted by case, which
+# means they appear in the same order as the values of the
+# enum type of the @tag.
#
# Values of this type are JSON object on the wire.
#
@@ -219,7 +231,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.
#
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 64f2cd0..6a5a843 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -10,6 +10,7 @@
# See the COPYING file in the top-level directory.
from qapi import *
+from operator import attrgetter
# Caveman's json.dumps() replacement (we're stuck at Python 2.4)
@@ -126,7 +127,8 @@ const char %(c_name)s[] = %(c_string)s;
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
- 'variants': [self._gen_variant(v) for v in variants]}
+ 'variants': [self._gen_variant(v) for v in
+ sorted(variants, key=attrgetter('name'))]}
def _gen_variant(self, variant):
return {'case': variant.name, 'type': self._use_type(variant.type)}
@@ -135,14 +137,15 @@ const char %(c_name)s[] = %(c_string)s;
self._gen_json(name, 'builtin', {'json-type': json_type})
def visit_enum_type(self, name, info, values, prefix):
- self._gen_json(name, 'enum', {'values': values})
+ self._gen_json(name, 'enum', {'values': sorted(values)})
def visit_array_type(self, name, info, 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]}
+ obj = {'members': [self._gen_member(m) for m in
+ sorted(members, key=attrgetter('name'))]}
if variants:
obj.update(self._gen_variants(variants.tag_member.name,
variants.variants))
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (3 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 04/14] qapi-introspect: Guarantee particular sorting Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-23 23:33 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 06/14] qapi: Update tests related to QMP/branch collisions Eric Blake
` (8 subsequent siblings)
13 siblings, 1 reply; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that we have separate namespaces for QMP vs. tag values,
we can simplify how the QAPISchema*.check() methods check for
collisions. Each QAPISchemaObjectTypeMember check() call is
given a single set of names it must not collide with; this set
is either the QMP names (when this member is used by an
ObjectType) or the case names (when this member is used by an
ObjectTypeVariants). We no longer need an all_members
parameter, as it can be computed by seen.values(). When used
by a union, QAPISchemaObjectTypeVariant must also perform a
QMP collision check for each member of its corresponding type.
The new ObjectType.check_qmp() is an idempotent subset of
check(), and can be called multiple times over different seen
sets (useful, since the members of one type can be applied
into more than one other location via inheritance or flat
union variants).
The code needs a temporary hack of passing a 'union' flag
through Variants.check(), since we do not inline the branches
of an alternate type into a parent QMP object. A later patch
will rework how alternates are laid out, by adding a new
subclass, and that will allow us to drop the extra parameter.
There are no changes to generated code, and we can now add a
positive test to qapi-schema-test that proves that alternates
can now use names that would previously trigger assertions
(see commit 7b2a5c2f for an example of the failure that was
still possible for alternates before this commit).
Future patches will add more positive tests, improve error
message quality on actual collisions, and move collision
checks out of ad hoc parse code into the check() methods.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: new patch, although it is a much cleaner implementation of
what was attempted by subset B v8 15/18
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03042.html
---
scripts/qapi.py | 55 ++++++++++++++++++++-------------
tests/qapi-schema/qapi-schema-test.json | 2 ++
tests/qapi-schema/qapi-schema-test.out | 5 +++
3 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index aab2b46..098ba5d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -974,28 +974,28 @@ class QAPISchemaObjectType(QAPISchemaType):
self.variants = variants
self.members = None
+ # Finish construction, and validate that all members are usable
def check(self, schema):
assert self.members is not False # not running in cycles
if self.members:
return
self.members = False # mark as being checked
+ seen = OrderedDict()
if self._base_name:
self.base = schema.lookup_type(self._base_name)
- assert isinstance(self.base, QAPISchemaObjectType)
- assert not self.base.variants # not implemented
- self.base.check(schema)
- members = list(self.base.members)
- else:
- members = []
- seen = {}
- for m in members:
- assert c_name(m.name) not in seen
- seen[m.name] = m
+ self.base.check_qmp(schema, seen)
for m in self.local_members:
- m.check(schema, members, seen)
+ m.check(schema, seen)
if self.variants:
- self.variants.check(schema, members, seen)
- self.members = members
+ self.variants.check(schema, seen)
+ self.members = seen.values()
+
+ # Check that this type does not introduce QMP collisions into seen
+ def check_qmp(self, schema, seen):
+ self.check(schema)
+ assert not self.variants # not implemented
+ for m in self.members:
+ m.check(schema, seen)
def is_implicit(self):
# See QAPISchema._make_implicit_object_type()
@@ -1029,11 +1029,13 @@ class QAPISchemaObjectTypeMember(object):
self.type = None
self.optional = optional
- def check(self, schema, all_members, seen):
+ def check(self, schema, seen):
+ # seen is a map of names we must not collide with (either QMP
+ # names, when called by ObjectType, or case names, when called
+ # by Variant). This method is safe to call over multiple 'seen'.
assert self.name not in seen
self.type = schema.lookup_type(self._type_name)
assert self.type
- all_members.append(self)
seen[self.name] = self
@@ -1052,24 +1054,33 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
- def check(self, schema, members, seen):
+ # TODO drop union once alternates can be distinguished by tag_member
+ def check(self, schema, seen, union=True):
if self.tag_name:
self.tag_member = seen[self.tag_name]
+ assert self.tag_member
else:
- self.tag_member.check(schema, members, seen)
+ self.tag_member.check(schema, seen)
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ cases = OrderedDict()
for v in self.variants:
- vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ # Reset seen array for each variant, since QMP names from one
+ # branch do not affect another branch, nor add to all_members
+ v.check(schema, self.tag_member.type, dict(seen), cases, union)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
- def check(self, schema, tag_type, seen):
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ # TODO drop union once alternates can be distinguished by tag_type
+ def check(self, schema, tag_type, seen, cases, union):
+ # cases is case names we must not collide with
+ QAPISchemaObjectTypeMember.check(self, schema, cases)
assert self.name in tag_type.values
+ if union:
+ # seen is QMP names our members must not collide with
+ self.type.check_qmp(schema, seen)
# This function exists to support ugly simple union special cases
# TODO get rid of them, and drop the function
@@ -1090,7 +1101,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, [], {})
+ self.variants.check(schema, {}, False)
def json_type(self):
return 'value'
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index efa7acc..926bd7e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -120,6 +120,8 @@
{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
'myList': 'has_a' } }
+{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
+ 'myKind': 'has_a' } }
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 34fb317..1c39a2a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -61,6 +61,11 @@ alternate AltIntNum
case i: int
case n: number
enum AltIntNumKind ['i', 'n']
+alternate AltName
+ case type: int
+ case u: bool
+ case myKind: has_a
+enum AltNameKind ['type', 'u', 'myKind']
alternate AltNumInt
case n: number
case i: int
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions Eric Blake
@ 2015-10-23 23:33 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-23 23:33 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]
On 10/16/2015 10:35 PM, Eric Blake wrote:
> Now that we have separate namespaces for QMP vs. tag values,
> we can simplify how the QAPISchema*.check() methods check for
> collisions. Each QAPISchemaObjectTypeMember check() call is
> given a single set of names it must not collide with; this set
> is either the QMP names (when this member is used by an
> ObjectType) or the case names (when this member is used by an
> ObjectTypeVariants). We no longer need an all_members
> parameter, as it can be computed by seen.values(). When used
> by a union, QAPISchemaObjectTypeVariant must also perform a
> QMP collision check for each member of its corresponding type.
>
> The new ObjectType.check_qmp() is an idempotent subset of
> check(), and can be called multiple times over different seen
> sets (useful, since the members of one type can be applied
> into more than one other location via inheritance or flat
> union variants).
>
> The code needs a temporary hack of passing a 'union' flag
> through Variants.check(), since we do not inline the branches
> of an alternate type into a parent QMP object. A later patch
> will rework how alternates are laid out, by adding a new
> subclass, and that will allow us to drop the extra parameter.
>
> There are no changes to generated code, and we can now add a
> positive test to qapi-schema-test that proves that alternates
> can now use names that would previously trigger assertions
> (see commit 7b2a5c2f for an example of the failure that was
> still possible for alternates before this commit).
>
> Future patches will add more positive tests, improve error
> message quality on actual collisions, and move collision
> checks out of ad hoc parse code into the check() methods.
This paragraph may be a bit out of date based on rebasing that I've
done, but one patch for sure that needs to come later is deferring
subset B v10 24/25 to after this point. The patch itself should be in
good state for reviewing, even if the commit message needs massaging.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: new patch, although it is a much cleaner implementation of
> what was attempted by subset B v8 15/18
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03042.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v7 06/14] qapi: Update tests related to QMP/branch collisions
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (4 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 07/14] qapi: Simplify visiting of alternate types Eric Blake
` (7 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that branches are in a separate C namespace, we can remove
the restrictions in the parser that claim a branch name would
collide with QMP, delete the negative tests that are no longer
problematic, and add positive tests to qapi-schema-test to
ensure things compile correctly.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: new patch
---
scripts/qapi.py | 15 +--------------
tests/Makefile | 3 ---
tests/qapi-schema/flat-union-clash-branch.err | 1 -
tests/qapi-schema/flat-union-clash-branch.exit | 1 -
tests/qapi-schema/flat-union-clash-branch.json | 16 ----------------
tests/qapi-schema/flat-union-clash-branch.out | 0
tests/qapi-schema/flat-union-clash-type.err | 1 -
tests/qapi-schema/flat-union-clash-type.exit | 1 -
tests/qapi-schema/flat-union-clash-type.json | 14 --------------
tests/qapi-schema/flat-union-clash-type.out | 0
tests/qapi-schema/qapi-schema-test.json | 12 +++++++++---
tests/qapi-schema/qapi-schema-test.out | 12 +++++++++++-
tests/qapi-schema/union-clash-type.err | 1 -
tests/qapi-schema/union-clash-type.exit | 1 -
tests/qapi-schema/union-clash-type.json | 7 -------
tests/qapi-schema/union-clash-type.out | 0
16 files changed, 21 insertions(+), 64 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.err
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.json
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.out
delete mode 100644 tests/qapi-schema/flat-union-clash-type.err
delete mode 100644 tests/qapi-schema/flat-union-clash-type.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-type.json
delete mode 100644 tests/qapi-schema/flat-union-clash-type.out
delete mode 100644 tests/qapi-schema/union-clash-type.err
delete mode 100644 tests/qapi-schema/union-clash-type.exit
delete mode 100644 tests/qapi-schema/union-clash-type.json
delete mode 100644 tests/qapi-schema/union-clash-type.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 098ba5d..7aa451e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -546,7 +546,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
+ values = {'MAX': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -592,14 +592,6 @@ def check_union(expr, expr_info):
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
- # TODO: As long as branch names can collide with QMP names, we
- # must prevent branches starting with 'has_'. However, we do not
- # need to reject 'u', because that is reserved for when we start
- # sticking branch names in a C union named 'u'.
- if key.startswith('has-') or key.startswith('has_'):
- raise QAPIExprError(expr_info,
- "Branch of union '%s' uses reserved name '%s'"
- % (name, key))
# Each value must name a known type; furthermore, in flat unions,
# branches must be a struct with no overlapping member names
@@ -620,11 +612,6 @@ def check_union(expr, expr_info):
"Discriminator value '%s' is not found in "
"enum '%s'" %
(key, enum_define["enum_name"]))
- if discriminator in enum_define['enum_values']:
- raise QAPIExprError(expr_info,
- "Discriminator name '%s' collides with "
- "enum value in '%s'" %
- (discriminator, enum_define["enum_name"]))
# Otherwise, check for conflicts in the generated enum
else:
diff --git a/tests/Makefile b/tests/Makefile
index b3516ad..8c1843a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -276,9 +276,7 @@ qapi-schema += flat-union-bad-base.json
qapi-schema += flat-union-bad-discriminator.json
qapi-schema += flat-union-base-any.json
qapi-schema += flat-union-base-union.json
-qapi-schema += flat-union-clash-branch.json
qapi-schema += flat-union-clash-member.json
-qapi-schema += flat-union-clash-type.json
qapi-schema += flat-union-empty.json
qapi-schema += flat-union-inline.json
qapi-schema += flat-union-int-branch.json
@@ -336,7 +334,6 @@ qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
-qapi-schema += union-clash-type.json
qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
qapi-schema += union-max.json
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
deleted file mode 100644
index e6b6294..0000000
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-clash-branch.json:13: Branch of union 'TestUnion' uses reserved name 'has-a'
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
deleted file mode 100644
index 8efbcfd..0000000
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ /dev/null
@@ -1,16 +0,0 @@
-# Flat union branch name collision
-# This is rejected because the C struct would have duplicate 'has_a'
-# (one as the implicit flag for the optional base member, the other from
-# the C member for the branch name).
-# TODO: we should munge generated branch names to not collide with the
-# non-variant struct members.
-{ 'enum': 'TestEnum',
- 'data': [ 'has-a' ] }
-{ 'struct': 'Base',
- 'data': { 'enum1': 'TestEnum', '*a': 'str' } }
-{ 'struct': 'Branch1',
- 'data': { 'string': 'str' } }
-{ 'union': 'TestUnion',
- 'base': 'Base',
- 'discriminator': 'enum1',
- 'data': { 'has-a': 'Branch1' } }
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
deleted file mode 100644
index b44dd40..0000000
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/flat-union-clash-type.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json
deleted file mode 100644
index 8f710f0..0000000
--- a/tests/qapi-schema/flat-union-clash-type.json
+++ /dev/null
@@ -1,14 +0,0 @@
-# Flat union branch 'type'
-# Reject this, because we would have a clash in generated C, between the
-# outer tag 'type' and the branch name 'type' within the union.
-# TODO: We could munge the generated C branch name to let it compile.
-{ 'enum': 'TestEnum',
- 'data': [ 'type' ] }
-{ 'struct': 'Base',
- 'data': { 'type': 'TestEnum' } }
-{ 'struct': 'Branch1',
- 'data': { 'string': 'str' } }
-{ 'union': 'TestUnion',
- 'base': 'Base',
- 'discriminator': 'type',
- 'data': { 'type': 'Branch1' } }
diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 926bd7e..26a5b76 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -116,10 +116,16 @@
# Even though 'u' and 'has_*' are forbidden as struct member names, they
# should still be valid as a type or union branch name. And although
# '*Kind' and '*List' are forbidden as type names, they should not be
-# forbidden as a member or branch name.
-{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
+# forbidden as a member or branch name. Flat union branches do not
+# collide with base members.
+{ 'enum': 'EnumName', 'data': [ 'value1', 'has_a', 'u', 'type' ] }
+{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'],
+ 'value1': 'EnumName' } }
{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
- 'myList': 'has_a' } }
+ 'myList': 'has_a', 'has_a': 'has_a' } }
+{ 'union': 'UnionName', 'base': 'has_a', 'discriminator': 'value1',
+ 'data': { 'value1': 'UserDefZero', 'has_a': 'UserDefZero',
+ 'u': 'UserDefZero', 'type': 'UserDefZero' } }
{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
'myKind': 'has_a' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 1c39a2a..719bdf1 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -90,6 +90,7 @@ event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
event EVENT_D :obj-EVENT_D-arg
+enum EnumName ['value1', 'has_a', 'u', 'type']
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
@@ -111,6 +112,13 @@ object TestStruct
member integer: int optional=False
member boolean: bool optional=False
member string: str optional=False
+object UnionName
+ base has_a
+ tag value1
+ case value1: UserDefZero
+ case has_a: UserDefZero
+ case u: UserDefZero
+ case type: UserDefZero
object UserDefA
member boolean: bool optional=False
member a_b: int optional=True
@@ -211,11 +219,13 @@ command guest-sync :obj-guest-sync-arg -> any
object has_a
member MyKind: int optional=False
member MyList: intList optional=False
+ member value1: EnumName optional=False
object u
case u: :obj-uint8-wrapper
case myKind: :obj-has_a-wrapper
case myList: :obj-has_a-wrapper
-enum uKind ['u', 'myKind', 'myList']
+ case has_a: :obj-has_a-wrapper
+enum uKind ['u', 'myKind', 'myList', 'has_a']
command user_def_cmd None -> None
gen=True success_response=True
command user_def_cmd1 :obj-user_def_cmd1-arg -> None
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
deleted file mode 100644
index c14bbdd..0000000
--- a/tests/qapi-schema/union-clash-type.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-clash-type.json:6: Union 'TestUnion' member 'type' clashes with '(automatic tag)'
diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-clash-type.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
deleted file mode 100644
index 641b2d5..0000000
--- a/tests/qapi-schema/union-clash-type.json
+++ /dev/null
@@ -1,7 +0,0 @@
-# Union branch 'type'
-# Reject this, because we would have a clash in generated C, between the
-# simple union's implicit tag member 'type' and the branch name 'type'
-# within the union.
-# TODO: If desired, we could munge the branch name to allow compilation.
-{ 'union': 'TestUnion',
- 'data': { 'kind': 'int', 'type': 'str' } }
diff --git a/tests/qapi-schema/union-clash-type.out b/tests/qapi-schema/union-clash-type.out
deleted file mode 100644
index e69de29..0000000
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 07/14] qapi: Simplify visiting of alternate types
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (5 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 06/14] qapi: Update tests related to QMP/branch collisions Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 08/14] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
` (6 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Previously, working with alternates required two enums, and
some indirection: for type Foo, we created Foo_qtypes[] which
maps each qtype to a member of FooKind_lookup[], then use
FooKind_lookup[] like we do for other union types.
This has a subtle bug: since the values of FooKind_lookup
start at zero, all entries of Foo_qtypes that were not
explicitly initialized map to the same branch of the union as
the first member of the alternate, rather than triggering a
failure in visit_get_next_type(). Fortunately, the bug
seldom bites; the very next thing the input visitor does is
try to parse the incoming JSON with the wrong parser, which
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).
However, it IS observable in one case: the behavior of an
alternate that contains a 'number' member but no 'int' member
differs according to whether the 'number' was first in the
qapi definition, and when the input being parsed is an integer;
this is because the 'number' parser accepts QTYPE_QINT in
addition to the expected QTYPE_QFLOAT. A later patch will worry
about fixing alternates to parse all inputs that a non-alternate
'number' would accept, for now it is still marked FIXME.
This patch fixes the validation bug by deleting the indirection,
and modifying get_next_type() to directly return a qtype code.
There is no longer a need to generate an implicit FooKind array
associated with the alternate type (since the QMP wire format
never uses the stringized counterparts of the C union member
names); that also means we no longer have a collision with an
alternate branch named 'max'. Next, the generated visitor is
fixed to properly detect unexpected qtypes in the switch
statement. This is done via the use of a new
QAPISchemaAlternateTypeTag subclass and the use of a new
member.c_type() method when producing qapi-types. The new
subtype also allows us to clean up a TODO left in the previous
commit.
Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. If that gets too confusing, we
could reintroduce FooKind, but initialize it differently than
most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type. But without a current client, I
didn't see the point of doing it now.
There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: rebase onto earlier changes, rework how subtype makes things work
v6: rebase onto tag_member subclass, testsuite, gen_err_check(),
and info improvements
---
docs/qapi-code-gen.txt | 3 ---
include/qapi/visitor-impl.h | 3 ++-
include/qapi/visitor.h | 8 +++++-
qapi/qapi-visit-core.c | 4 +--
qapi/qmp-input-visitor.c | 4 +--
scripts/qapi-types.py | 36 +--------------------------
scripts/qapi-visit.py | 12 +++++----
scripts/qapi.py | 43 ++++++++++++++++++++++-----------
tests/qapi-schema/alternate-empty.out | 1 -
tests/qapi-schema/qapi-schema-test.json | 2 +-
tests/qapi-schema/qapi-schema-test.out | 10 +-------
tests/test-qmp-input-visitor.c | 33 +++++++++++++------------
tests/test-qmp-output-visitor.c | 21 ++++++++++++----
13 files changed, 86 insertions(+), 94 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index e970440..c18484f 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -383,9 +383,6 @@ where each branch of the union names a QAPI type. For example:
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
-Just like for a simple union, an implicit C enum 'NameKind' is created
-to enumerate the branches for the alternate 'Name'.
-
Unlike a union, the discriminator string is never passed on the wire
for the Client JSON Protocol. Instead, the value's JSON type serves
as an implicit discriminator, which in turn means that an alternate
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8c0ba57..6d95b36 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,7 +32,8 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
- void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
+ /* May be NULL; most useful for input visitors. */
+ void (*get_next_type)(Visitor *v, qtype_code *type,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index cfc19a6..b765993 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,7 +41,13 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+
+/**
+ * Determine the qtype of the item @name in the current object visit.
+ * For input visitors, set *@type to the correct qtype of a qapi
+ * alternate type; for other visitors, leave *@type unchanged.
+ */
+void visit_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 59ed506..3f24daa 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+void visit_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, obj, qtypes, name, errp);
+ v->get_next_type(v, type, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 5dd9ed5..803ffad 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
+static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
return;
}
- *kind = qobjects[qobject_type(qobj)];
+ *type = qobject_type(qobj);
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 235fe07..cdc6426 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -47,7 +47,7 @@ def gen_struct_field(member):
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=member.type.c_type(), c_name=c_name(member.name))
+ c_type=member.c_type(), c_name=c_name(member.name))
return ret
@@ -95,38 +95,6 @@ struct %(c_name)s {
return ret
-def gen_alternate_qtypes_decl(name):
- return mcgen('''
-
-extern const int %(c_name)s_qtypes[];
-''',
- c_name=c_name(name))
-
-
-def gen_alternate_qtypes(name, variants):
- ret = mcgen('''
-
-const int %(c_name)s_qtypes[QTYPE_MAX] = {
-''',
- c_name=c_name(name))
-
- for var in variants.variants:
- qtype = var.type.alternate_qtype()
- assert qtype
-
- ret += mcgen('''
- [%(qtype)s] = %(enum_const)s,
-''',
- qtype=qtype,
- enum_const=c_enum_const(variants.tag_member.type.name,
- var.name))
-
- ret += mcgen('''
-};
-''')
- return ret
-
-
def gen_union(name, base, variants):
ret = mcgen('''
@@ -264,9 +232,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
- self._fwdefn += gen_alternate_qtypes(name, variants)
self.decl += gen_union(name, None, variants)
- self.decl += gen_alternate_qtypes_decl(name)
self._gen_type_cleanup(name)
# If you link code generated from multiple schemata, you want only one
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2afe811..17a82af 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -182,7 +182,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(v, &(*obj)->type, name, &err);
if (err) {
goto out_obj;
}
@@ -196,14 +196,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
break;
''',
- case=c_enum_const(variants.tag_member.type.name,
- var.name),
+ case=var.type.alternate_qtype(),
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
default:
- abort();
+ error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+ "%(name)s");
}
out_obj:
error_propagate(errp, err);
@@ -212,7 +212,8 @@ out_obj:
out:
error_propagate(errp, err);
}
-''')
+''',
+ name=name)
return ret
@@ -417,6 +418,7 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
#include "%(prefix)sqapi-types.h"
''',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7aa451e..733d308 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -626,15 +626,15 @@ def check_union(expr, expr_info):
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
- values = {'MAX': '(automatic)'}
+ values = {}
types_seen = {}
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
- # Check for conflicts in the generated enum
- c_key = camel_to_upper(key)
+ # Check for conflicts in the branch names
+ c_key = c_name(key)
if c_key in values:
raise QAPIExprError(expr_info,
"Alternate '%s' member '%s' clashes with '%s'"
@@ -1025,13 +1025,17 @@ class QAPISchemaObjectTypeMember(object):
assert self.type
seen[self.name] = self
+ def c_type(self):
+ return self.type.c_type()
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
# Flat unions pass tag_name but not tag_member.
# Simple unions and alternates pass tag_member but not tag_name.
# After check(), tag_member is always set, and tag_name remains
- # a reliable witness of being used by a flat union.
+ # a reliable witness of being used by a flat union, and
+ # tag_member.type being None is a reliable witness of an alternate.
assert bool(tag_member) != bool(tag_name)
assert (isinstance(tag_name, str) or
isinstance(tag_member, QAPISchemaObjectTypeMember))
@@ -1041,32 +1045,31 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
- # TODO drop union once alternates can be distinguished by tag_member
- def check(self, schema, seen, union=True):
+ def check(self, schema, seen):
if self.tag_name:
self.tag_member = seen[self.tag_name]
assert self.tag_member
else:
self.tag_member.check(schema, seen)
- assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ if not isinstance(self.tag_member, QAPISchemaAlternateTypeTag):
+ assert isinstance(self.tag_member.type, QAPISchemaEnumType)
cases = OrderedDict()
for v in self.variants:
# Reset seen array for each variant, since QMP names from one
# branch do not affect another branch, nor add to all_members
- v.check(schema, self.tag_member.type, dict(seen), cases, union)
+ v.check(schema, self.tag_member.type, dict(seen), cases)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
- # TODO drop union once alternates can be distinguished by tag_type
- def check(self, schema, tag_type, seen, cases, union):
+ def check(self, schema, tag_type, seen, cases):
# cases is case names we must not collide with
QAPISchemaObjectTypeMember.check(self, schema, cases)
- assert self.name in tag_type.values
- if union:
+ if tag_type:
# seen is QMP names our members must not collide with
+ assert self.name in tag_type.values
self.type.check_qmp(schema, seen)
# This function exists to support ugly simple union special cases
@@ -1088,7 +1091,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, {}, False)
+ self.variants.check(schema, {})
def json_type(self):
return 'value'
@@ -1097,6 +1100,18 @@ class QAPISchemaAlternateType(QAPISchemaType):
visitor.visit_alternate_type(self.name, self.info, self.variants)
+class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember):
+ def __init__(self):
+ QAPISchemaObjectTypeMember.__init__(self, 'type', '', False)
+
+ def check(self, schema, seen):
+ assert len(seen) == 0
+ seen[self.name] = self
+
+ def c_type(self):
+ return 'qtype_code'
+
+
class QAPISchemaCommand(QAPISchemaEntity):
def __init__(self, name, info, arg_type, ret_type, gen, success_response):
QAPISchemaEntity.__init__(self, name, info)
@@ -1290,7 +1305,7 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
- tag_member = self._make_implicit_tag(name, info, variants)
+ tag_member = QAPISchemaAlternateTypeTag()
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index 0f153b6..9b010d8 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -1,4 +1,3 @@
object :empty
alternate Alt
case i: int
-enum AltKind ['i']
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 26a5b76..8af6da7 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -127,7 +127,7 @@
'data': { 'value1': 'UserDefZero', 'has_a': 'UserDefZero',
'u': 'UserDefZero', 'type': 'UserDefZero' } }
{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
- 'myKind': 'has_a' } }
+ 'myKind': 'has_a', 'max': 'str' } }
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 719bdf1..3bc51dd 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -60,32 +60,26 @@ object :obj-user_def_cmd2-arg
alternate AltIntNum
case i: int
case n: number
-enum AltIntNumKind ['i', 'n']
alternate AltName
case type: int
case u: bool
case myKind: has_a
-enum AltNameKind ['type', 'u', 'myKind']
+ case max: str
alternate AltNumInt
case n: number
case i: int
-enum AltNumIntKind ['n', 'i']
alternate AltNumStr
case n: number
case s: str
-enum AltNumStrKind ['n', 's']
alternate AltStrBool
case s: str
case b: bool
-enum AltStrBoolKind ['s', 'b']
alternate AltStrInt
case s: str
case i: int
-enum AltStrIntKind ['s', 'i']
alternate AltStrNum
case s: str
case n: number
-enum AltStrNumKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
@@ -126,7 +120,6 @@ alternate UserDefAlternate
case uda: UserDefA
case s: str
case i: int
-enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
member a-b: bool optional=True
@@ -194,7 +187,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str
case b: __org.qemu_x-Base
-enum __org.qemu_x-AltKind ['__org.qemu_x-branch', 'b']
object __org.qemu_x-Base
member __org.qemu_x-member1: __org.qemu_x-Enum optional=False
enum __org.qemu_x-Enum ['__org.qemu_x-value']
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c311fc6..7869a43 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -338,14 +338,14 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QINT);
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_cmpint(tmp->type, ==, QTYPE_QSTRING);
g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
visitor_input_teardown(data, NULL);
@@ -381,11 +381,10 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltStrBool(asb);
visitor_input_teardown(data, NULL);
- /* FIXME: Order of alternate should not affect semantics; asn should
- * parse the same as ans */
+ /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
visit_type_AltStrNum(v, &asn, NULL, &err);
- /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
+ /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
/* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
g_assert(err);
error_free(err);
@@ -393,30 +392,34 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltStrNum(asn);
visitor_input_teardown(data, NULL);
+ /* FIXME: integer should parse as number */
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);
+ visit_type_AltNumStr(v, &ans, NULL, &err);
+ /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
+ /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
+ g_assert(err);
+ error_free(err);
+ err = NULL;
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->type, ==, QTYPE_QINT);
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->type, ==, QTYPE_QINT);
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->type, ==, QTYPE_QINT);
g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
visitor_input_teardown(data, NULL);
@@ -433,14 +436,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
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_cmpint(asn->type, ==, QTYPE_QFLOAT);
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_cmpint(ans->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ans->u.n, ==, 42.5);
qapi_free_AltNumStr(ans);
visitor_input_teardown(data, NULL);
@@ -455,14 +458,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
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_cmpint(ain->type, ==, QTYPE_QFLOAT);
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_cmpint(ani->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ani->u.n, ==, 42.5);
qapi_free_AltNumInt(ani);
visitor_input_teardown(data, NULL);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 9364843..695db32 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -449,20 +449,31 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
const void *unused)
{
QObject *arg;
- Error *err = NULL;
+ UserDefAlternate *tmp;
- UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
- tmp->type = USER_DEF_ALTERNATE_KIND_I;
+ tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = QTYPE_QINT;
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);
g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
qapi_free_UserDefAlternate(tmp);
+
+ tmp = g_malloc0(sizeof(UserDefAlternate));
+ tmp->type = QTYPE_QSTRING;
+ 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);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 08/14] qapi: Fix alternates that accept 'number' but not 'int'
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (6 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 07/14] qapi: Simplify visiting of alternate types Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 09/14] qapi: Remove dead visitor code Eric Blake
` (5 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT. However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' but not 'int' would reject integral values.
With this patch, we now have the following desirable table:
alternate has case selected for
'int' 'number' QTYPE_QINT QTYPE_QFLOAT
no no error error
no yes 'number' 'number'
yes no 'int' error
yes yes 'int' 'number'
While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: rebase to named .u union
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h | 2 +-
include/qapi/visitor.h | 3 ++-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 4 ++++
scripts/qapi-visit.py | 9 +++++++--
tests/test-qmp-input-visitor.c | 20 ++++++--------------
6 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 6d95b36..1d09b7b 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -33,7 +33,7 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
/* May be NULL; most useful for input visitors. */
- void (*get_next_type)(Visitor *v, qtype_code *type,
+ void (*get_next_type)(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b765993..baea594 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -46,8 +46,9 @@ void visit_optional(Visitor *v, bool *present, const char *name,
* Determine the qtype of the item @name in the current object visit.
* For input visitors, set *@type to the correct qtype of a qapi
* alternate type; for other visitors, leave *@type unchanged.
+ * If @promote_int, treat integers as QTYPE_FLOAT.
*/
-void visit_get_next_type(Visitor *v, qtype_code *type,
+void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 3f24daa..884fe94 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, qtype_code *type,
+void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, type, name, errp);
+ v->get_next_type(v, type, promote_int, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 803ffad..5310db5 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -209,6 +209,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
}
static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
+ bool promote_int,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -219,6 +220,9 @@ static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
return;
}
*type = qobject_type(qobj);
+ if (promote_int && *type == QTYPE_QINT) {
+ *type = QTYPE_QFLOAT;
+ }
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 17a82af..ed694a4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error
def gen_visit_alternate(name, variants):
+ promote_int = 'true'
+ for var in variants.variants:
+ if var.type.alternate_qtype() == 'QTYPE_QINT':
+ promote_int = 'false'
+
ret = mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
@@ -182,13 +187,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(v, &(*obj)->type, name, &err);
+ visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err);
if (err) {
goto out_obj;
}
switch ((*obj)->type) {
''',
- c_name=c_name(name))
+ c_name=c_name(name), promote_int=promote_int)
for var in variants.variants:
ret += mcgen('''
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 7869a43..085ff3f 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -381,25 +381,17 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltStrBool(asb);
visitor_input_teardown(data, NULL);
- /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
- visit_type_AltStrNum(v, &asn, NULL, &err);
- /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
- /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltStrNum(v, &asn, NULL, &error_abort);
+ g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(asn->u.n, ==, 42);
qapi_free_AltStrNum(asn);
visitor_input_teardown(data, NULL);
- /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
- visit_type_AltNumStr(v, &ans, NULL, &err);
- /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
- /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+ g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(ans->u.n, ==, 42);
qapi_free_AltNumStr(ans);
visitor_input_teardown(data, NULL);
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 09/14] qapi: Remove dead visitor code
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (7 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 08/14] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 10/14] qapi: Plug leaks in test-qmp-* Eric Blake
` (4 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Commit cbc95538 removed unused start_handle() and end_handle(),
but forgot got remove their declarations.
Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but except for type_uint64 and type_size,
none of them have ever been supplied (the generic implementation
based on using type_int then bounds-checking works just fine).
In the interest of simplicity, it's easier to make the visitor
callback interface not have to worry about the other sizes.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: no change
v6: no change
---
include/qapi/visitor-impl.h | 19 +++----
include/qapi/visitor.h | 3 -
qapi/qapi-visit-core.c | 131 +++++++++++++++++---------------------------
3 files changed, 58 insertions(+), 95 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 1d09b7b..370935a 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -1,7 +1,7 @@
/*
* Core Definitions for QAPI Visitor implementations
*
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012, 2015 Red Hat, Inc.
*
* Author: Paolo Bonizni <pbonzini@redhat.com>
*
@@ -48,18 +48,15 @@ struct Visitor
void (*optional)(Visitor *v, bool *present, const char *name,
Error **errp);
- void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
- void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
- void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
- void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
- void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
- void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
- void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
- void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
- /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
- void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
bool (*start_union)(Visitor *v, bool data_present, Error **errp);
void (*end_union)(Visitor *v, bool data_present, Error **errp);
+
+ /* Only required to visit uint64 differently than (*type_int)(). */
+ void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
+ /* Only required to visit sizes differently than (*type_uint64)(). */
+ void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
};
void input_type_enum(Visitor *v, int *obj, const char * const strings[],
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index baea594..67ddd83 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,9 +27,6 @@ typedef struct GenericList
struct GenericList *next;
} GenericList;
-void visit_start_handle(Visitor *v, void **obj, const char *kind,
- const char *name, Error **errp);
-void visit_end_handle(Visitor *v, Error **errp);
void visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
void visit_end_struct(Visitor *v, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 884fe94..cbf7780 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -104,57 +104,48 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_uint8) {
- v->type_uint8(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT8_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint8_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT8_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint8_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
- if (v->type_uint16) {
- v->type_uint16(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT16_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint16_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT16_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint16_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
- if (v->type_uint32) {
- v->type_uint32(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT32_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint32_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT32_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint32_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
@@ -171,77 +162,55 @@ void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int8) {
- v->type_int8(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT8_MIN || value > INT8_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int8_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT8_MIN || value > INT8_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int8_t");
+ return;
}
+ *obj = value;
}
void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int16) {
- v->type_int16(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT16_MIN || value > INT16_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int16_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT16_MIN || value > INT16_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int16_t");
+ return;
}
+ *obj = value;
}
void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int32) {
- v->type_int32(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT32_MIN || value > INT32_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int32_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT32_MIN || value > INT32_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int32_t");
+ return;
}
+ *obj = value;
}
void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
{
- if (v->type_int64) {
- v->type_int64(v, obj, name, errp);
- } else {
- v->type_int(v, obj, name, errp);
- }
+ v->type_int(v, obj, name, errp);
}
void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
{
- int64_t value;
-
if (v->type_size) {
v->type_size(v, obj, name, errp);
- } else if (v->type_uint64) {
- v->type_uint64(v, obj, name, errp);
} else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- *obj = value;
+ visit_type_uint64(v, obj, name, errp);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 10/14] qapi: Plug leaks in test-qmp-*
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (8 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 09/14] qapi: Remove dead visitor code Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 11/14] qapi: Simplify error testing " Eric Blake
` (3 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: no change
v6: make init repeatable rather than adding teardown everywhere,
fix additional leak with UserDefTwo base, plug additional files
---
tests/test-qmp-input-strict.c | 10 ++++++++++
tests/test-qmp-input-visitor.c | 41 +++++++----------------------------------
tests/test-qmp-output-visitor.c | 4 +++-
3 files changed, 20 insertions(+), 35 deletions(-)
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index b44184f..910e2f9 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data,
{
Visitor *v;
+ validate_teardown(data, NULL);
+
data->obj = qobject_from_json(json_string);
g_assert(data->obj != NULL);
@@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVisitorData *data,
visit_type_TestStruct(v, &p, NULL, &err);
g_assert(err);
+ error_free(err);
+ /* FIXME: visitor should not allocate p when returning error */
if (p) {
g_free(p->string);
}
@@ -210,6 +214,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);
}
@@ -224,6 +229,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);
}
@@ -239,6 +245,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);
}
@@ -253,6 +260,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);
}
@@ -268,6 +276,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);
}
@@ -282,6 +291,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 085ff3f..1b8fbea 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
Visitor *v;
va_list ap;
+ visitor_input_teardown(data, NULL);
+
va_start(ap, json_string);
data->obj = qobject_from_jsonv(json_string, &ap);
va_end(ap);
@@ -177,12 +179,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;
}
@@ -205,12 +202,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)
{
@@ -226,17 +217,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,
@@ -341,14 +329,12 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->type, ==, QTYPE_QINT);
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, ==, QTYPE_QSTRING);
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);
@@ -356,7 +342,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,
@@ -379,42 +364,36 @@ 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");
visit_type_AltStrNum(v, &asn, NULL, &error_abort);
g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(asn->u.n, ==, 42);
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, ==, QTYPE_QFLOAT);
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, ==, QTYPE_QINT);
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, ==, QTYPE_QINT);
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, ==, QTYPE_QINT);
g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
- visitor_input_teardown(data, NULL);
/* Parsing a double */
@@ -424,21 +403,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, ==, QTYPE_QFLOAT);
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, ==, QTYPE_QFLOAT);
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);
@@ -446,21 +422,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, ==, QTYPE_QFLOAT);
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, ==, QTYPE_QFLOAT);
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 695db32..42458d7 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,
@@ -462,6 +462,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);
tmp = g_malloc0(sizeof(UserDefAlternate));
tmp->type = QTYPE_QSTRING;
@@ -474,6 +475,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
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,
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 11/14] qapi: Simplify error testing in test-qmp-*
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (9 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 10/14] qapi: Plug leaks in test-qmp-* Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 12/14] qapi: Test failure in middle of array parse Eric Blake
` (2 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
By using &error_abort, we can avoid a local err variable in
situations where we expect success.
By moving err into data, we can let test teardown take care
of cleaning up any collected error (and allowing for fewer
lines of code between repeated tests where init runs teardown
on our behalf).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: pick up whitespace changes dropped from earlier commit
v6: new patch
---
tests/test-qmp-input-strict.c | 77 +++++++++-------------------
tests/test-qmp-input-visitor.c | 101 ++++++++++++-------------------------
tests/test-qmp-output-visitor.c | 52 +++++--------------
tests/test-visitor-serialization.c | 42 +++++++--------
4 files changed, 86 insertions(+), 186 deletions(-)
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 910e2f9..f8da75c 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -26,6 +26,7 @@
typedef struct TestInputVisitorData {
QObject *obj;
QmpInputVisitor *qiv;
+ Error *err;
} TestInputVisitorData;
static void validate_teardown(TestInputVisitorData *data,
@@ -34,6 +35,9 @@ static void validate_teardown(TestInputVisitorData *data,
qobject_decref(data->obj);
data->obj = NULL;
+ error_free(data->err);
+ data->err = NULL;
+
if (data->qiv) {
qmp_input_visitor_cleanup(data->qiv);
data->qiv = NULL;
@@ -96,13 +100,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);
}
@@ -111,7 +113,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', "
@@ -119,8 +120,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);
}
@@ -128,13 +128,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);
}
@@ -143,12 +141,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);
}
@@ -157,7 +153,6 @@ static void test_validate_union_flat(TestInputVisitorData *data,
{
UserDefFlatUnion *tmp = NULL;
Visitor *v;
- Error *err = NULL;
v = validate_test_init(data,
"{ 'enum1': 'value1', "
@@ -165,8 +160,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);
}
@@ -175,12 +169,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);
}
@@ -188,14 +180,12 @@ static void test_validate_fail_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', 'extra': 42 }");
- visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_TestStruct(v, &p, NULL, &data->err);
+ g_assert(data->err);
/* FIXME: visitor should not allocate p when returning error */
if (p) {
g_free(p->string);
@@ -207,14 +197,12 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data,
const void *unused)
{
UserDefTwo *udp = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
- visit_type_UserDefTwo(v, &udp, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_UserDefTwo(v, &udp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefTwo(udp);
}
@@ -222,14 +210,12 @@ static void test_validate_fail_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, 'extra': 'ggg' } ]");
- visit_type_UserDefOneList(v, &head, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_UserDefOneList(v, &head, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefOneList(head);
}
@@ -237,15 +223,13 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data,
const void *unused)
{
UserDefNativeListUnion *tmp = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init(data,
"{ 'type': 'integer', 'data' : [ 'string' ] }");
- visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_UserDefNativeListUnion(v, &tmp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefNativeListUnion(tmp);
}
@@ -253,14 +237,12 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,
const void *unused)
{
UserDefFlatUnion *tmp = NULL;
- Error *err = NULL;
Visitor *v;
v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
- visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_UserDefFlatUnion(v, &tmp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefFlatUnion(tmp);
}
@@ -268,15 +250,13 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
const void *unused)
{
UserDefFlatUnion2 *tmp = NULL;
- Error *err = NULL;
Visitor *v;
/* test situation where discriminator field ('enum1' here) is missing */
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);
+ visit_type_UserDefFlatUnion2(v, &tmp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefFlatUnion2(tmp);
}
@@ -285,13 +265,11 @@ static void test_validate_fail_alternate(TestInputVisitorData *data,
{
UserDefAlternate *tmp = NULL;
Visitor *v;
- Error *err = NULL;
v = validate_test_init(data, "3.14");
- visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
+ visit_type_UserDefAlternate(v, &tmp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefAlternate(tmp);
}
@@ -299,16 +277,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 1b8fbea..9bffd81 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -22,6 +22,7 @@
typedef struct TestInputVisitorData {
QObject *obj;
QmpInputVisitor *qiv;
+ Error *err;
} TestInputVisitorData;
static void visitor_input_teardown(TestInputVisitorData *data,
@@ -30,6 +31,9 @@ static void visitor_input_teardown(TestInputVisitorData *data,
qobject_decref(data->obj);
data->obj = NULL;
+ error_free(data->err);
+ data->err = NULL;
+
if (data->qiv) {
qmp_input_visitor_cleanup(data->qiv);
data->qiv = NULL;
@@ -92,13 +96,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);
}
@@ -106,7 +108,6 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data,
const void *unused)
{
int64_t res = 0;
- Error *err = NULL;
Visitor *v;
/* this will overflow a Qint/int64, so should be deserialized into
@@ -115,22 +116,19 @@ 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);
+ visit_type_int(v, &res, NULL, &data->err);
+ g_assert(data->err);
}
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);
}
@@ -138,13 +136,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);
}
@@ -152,13 +148,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);
@@ -167,7 +161,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;
@@ -176,8 +169,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);
}
}
@@ -187,13 +179,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");
@@ -206,7 +196,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', "
@@ -214,8 +203,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");
@@ -231,14 +219,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++) {
@@ -256,7 +242,6 @@ static void test_visitor_in_any(TestInputVisitorData *data,
const void *unused)
{
QObject *res = NULL;
- Error *err = NULL;
Visitor *v;
QInt *qint;
QBool *qbool;
@@ -265,16 +250,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");
@@ -299,7 +282,6 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
const void *unused)
{
Visitor *v;
- Error *err = NULL;
UserDefFlatUnion *tmp;
v = visitor_input_test_init(data,
@@ -308,8 +290,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);
@@ -321,7 +302,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
const void *unused)
{
Visitor *v;
- Error *err = NULL;
UserDefAlternate *tmp;
v = visitor_input_test_init(data, "42");
@@ -337,10 +317,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
qapi_free_UserDefAlternate(tmp);
v = visitor_input_test_init(data, "false");
- visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_UserDefAlternate(v, &tmp, NULL, &data->err);
+ g_assert(data->err);
qapi_free_UserDefAlternate(tmp);
}
@@ -348,7 +326,6 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
const void *unused)
{
Visitor *v;
- Error *err = NULL;
AltStrBool *asb;
AltStrNum *asn;
AltNumStr *ans;
@@ -359,10 +336,8 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
/* Parsing an int */
v = visitor_input_test_init(data, "42");
- visit_type_AltStrBool(v, &asb, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltStrBool(v, &asb, NULL, &data->err);
+ g_assert(data->err);
qapi_free_AltStrBool(asb);
v = visitor_input_test_init(data, "42");
@@ -398,10 +373,8 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
/* Parsing a double */
v = visitor_input_test_init(data, "42.5");
- visit_type_AltStrBool(v, &asb, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltStrBool(v, &asb, NULL, &data->err);
+ g_assert(data->err);
qapi_free_AltStrBool(asb);
v = visitor_input_test_init(data, "42.5");
@@ -417,10 +390,8 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltNumStr(ans);
v = visitor_input_test_init(data, "42.5");
- visit_type_AltStrInt(v, &asi, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltStrInt(v, &asi, NULL, &data->err);
+ g_assert(data->err);
qapi_free_AltStrInt(asi);
v = visitor_input_test_init(data, "42.5");
@@ -441,7 +412,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("");
@@ -458,8 +428,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);
@@ -604,7 +573,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("");
@@ -621,8 +589,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);
@@ -640,7 +607,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("");
@@ -656,8 +622,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);
@@ -679,7 +644,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("");
@@ -695,8 +659,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);
@@ -729,18 +692,16 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
const void *unused)
{
TestStruct *p = NULL;
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
- visit_type_TestStruct(v, &p, NULL, &err);
- g_assert(err);
+ visit_type_TestStruct(v, &p, NULL, &data->err);
+ g_assert(data->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/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 42458d7..9d3bfca 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);
@@ -709,14 +685,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 c024e5e..9f67f9e 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;
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 12/14] qapi: Test failure in middle of array parse
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (10 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 11/14] qapi: Simplify error testing " Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 13/14] qapi: More tests of input arrays Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 14/14] qapi: Simplify visits of optional fields Eric Blake
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: no change
v6: rebase onto earlier gen_err_check() and testsuite improvements
---
scripts/qapi-visit.py | 4 ++++
tests/test-qmp-input-visitor.c | 10 ++++++++++
2 files changed, 14 insertions(+)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ed694a4..100eed9 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -131,6 +131,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
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 9bffd81..f036b65 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -693,6 +693,7 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
{
TestStruct *p = NULL;
Visitor *v;
+ strList *q = NULL;
v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");
@@ -704,6 +705,15 @@ 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' ]");
+ /* FIXME - a failed parse should not leave a partially-allocated
+ * array for us to clean up; this could cause callers to leak
+ * memory. */
+ visit_type_strList(v, &q, NULL, &data->err);
+ assert(q);
+ assert(data->err);
+ qapi_free_strList(q);
}
int main(int argc, char **argv)
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 13/14] qapi: More tests of input arrays
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (11 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 12/14] qapi: Test failure in middle of array parse Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 14/14] qapi: Simplify visits of optional fields Eric Blake
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: no change
v6: new patch
---
tests/test-qmp-input-visitor.c | 51 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index f036b65..ce5bc65 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -236,6 +236,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,
@@ -716,6 +722,49 @@ 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;
+
+ /* Make sure arrays and structs cannot be confused */
+
+ v = visitor_input_test_init(data, "[]");
+ visit_type_TestStruct(v, &p, NULL, &data->err);
+ g_assert(data->err);
+ g_assert(!p);
+
+ v = visitor_input_test_init(data, "{}");
+ visit_type_strList(v, &q, NULL, &data->err);
+ assert(data->err);
+ assert(!q);
+
+ /* Make sure primitives and struct cannot be confused */
+
+ v = visitor_input_test_init(data, "1");
+ visit_type_TestStruct(v, &p, NULL, &data->err);
+ g_assert(data->err);
+ g_assert(!p);
+
+ v = visitor_input_test_init(data, "{}");
+ visit_type_int(v, &i, NULL, &data->err);
+ assert(data->err);
+
+ /* Make sure primitives and arrays cannot be confused */
+
+ v = visitor_input_test_init(data, "1");
+ visit_type_strList(v, &q, NULL, &data->err);
+ assert(data->err);
+ assert(!q);
+
+ v = visitor_input_test_init(data, "[]");
+ visit_type_int(v, &i, NULL, &data->err);
+ assert(data->err);
+}
+
int main(int argc, char **argv)
{
TestInputVisitorData in_visitor_data;
@@ -748,6 +797,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",
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v7 14/14] qapi: Simplify visits of optional fields
2015-10-17 4:35 [Qemu-devel] [PATCH v7 00/14] alternate layout (post-introspection cleanups, subset C) Eric Blake
` (12 preceding siblings ...)
2015-10-17 4:35 ` [Qemu-devel] [PATCH v7 13/14] qapi: More tests of input arrays Eric Blake
@ 2015-10-17 4:35 ` Eric Blake
13 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-10-17 4:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
None of the visitor callbacks would set an error when testing
if an optional field was present; make this part of the interface
contract by eliminating the errp argument. Then, for less code,
reflect the determined boolean value back to the caller instead
of making the caller read the boolean after the fact.
The resulting generated code has a nice diff:
|- visit_optional(v, &has_fdset_id, "fdset-id", &err);
|- if (err) {
|- goto out;
|- }
|- if (has_fdset_id) {
|+ if (visit_optional(v, &has_fdset_id, "fdset-id")) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: rebase to no member.c_name()
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h | 5 ++---
include/qapi/visitor.h | 10 ++++++++--
qapi/opts-visitor.c | 2 +-
qapi/qapi-visit-core.c | 6 +++---
qapi/qmp-input-visitor.c | 3 +--
qapi/string-input-visitor.c | 3 +--
scripts/qapi.py | 7 +------
7 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 370935a..fd2e905 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -44,9 +44,8 @@ struct Visitor
void (*type_any)(Visitor *v, QObject **obj, const char *name,
Error **errp);
- /* May be NULL */
- void (*optional)(Visitor *v, bool *present, const char *name,
- Error **errp);
+ /* May be NULL; most useful for input visitors. */
+ void (*optional)(Visitor *v, bool *present, const char *name);
bool (*start_union)(Visitor *v, bool data_present, Error **errp);
void (*end_union)(Visitor *v, bool data_present, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 67ddd83..e52ad39 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -36,8 +36,14 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
void visit_start_list(Visitor *v, const char *name, Error **errp);
GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
-void visit_optional(Visitor *v, bool *present, const char *name,
- Error **errp);
+
+/**
+ * Check if an optional member @name of an object needs visiting.
+ * For input visitors, set *@present according to whether the
+ * corresponding visit_type_*() needs calling; for other visitors,
+ * leave *@present unchanged. Return *@present for convenience.
+ */
+bool visit_optional(Visitor *v, bool *present, const char *name);
/**
* Determine the qtype of the item @name in the current object visit.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index cd10392..ef5fb8b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -488,7 +488,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
static void
-opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
+opts_optional(Visitor *v, bool *present, const char *name)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index cbf7780..2594147 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -73,12 +73,12 @@ void visit_end_union(Visitor *v, bool data_present, Error **errp)
}
}
-void visit_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+bool visit_optional(Visitor *v, bool *present, const char *name)
{
if (v->optional) {
- v->optional(v, present, name, errp);
+ v->optional(v, present, name);
}
+ return *present;
}
void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 5310db5..f714dfc 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -300,8 +300,7 @@ static void qmp_input_type_any(Visitor *v, QObject **obj, const char *name,
*obj = qobj;
}
-static void qmp_input_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void qmp_input_optional(Visitor *v, bool *present, const char *name)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, true);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index bbd6a54..dee780a 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -299,8 +299,7 @@ static void parse_type_number(Visitor *v, double *obj, const char *name,
*obj = val;
}
-static void parse_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void parse_optional(Visitor *v, bool *present, const char *name)
{
StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 733d308..470f2a0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1613,15 +1613,10 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
for memb in members:
if memb.optional:
ret += mcgen('''
- visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
+ if (visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s")) {
''',
prefix=prefix, c_name=c_name(memb.name),
name=memb.name, errp=errparg)
- ret += gen_err_check(skiperr=skiperr)
- ret += mcgen('''
- if (%(prefix)shas_%(c_name)s) {
-''',
- prefix=prefix, c_name=c_name(memb.name))
push_indent()
# Ugly: sometimes we need to cast away const
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread