From 9d3524b39e1fe5f3bb7a990ad7841e469e954a3b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 16 Feb 2016 16:39:25 -0700 Subject: qapi-visit: Honor prefix of discriminator enum When we added support for a user-specified prefix for an enum type (commit 351d36e), we forgot to teach the qapi-visit code to honor that prefix in the case of using a prefixed enum as the discriminator for a flat union. While there is still some on-list debate on whether we want to keep prefixes, we should at least make it work as long as it is still part of the code base. Reported-by: Daniel P. Berrange Signed-off-by: Eric Blake Message-Id: <1455665965-27638-1-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/qapi-schema/qapi-schema-test.json | 9 ++++++--- tests/qapi-schema/qapi-schema-test.out | 7 +++++-- 2 files changed, 11 insertions(+), 5 deletions(-) (limited to 'tests') diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 4b89527..353a34e 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -73,14 +73,17 @@ 'base': 'UserDefZero', 'data': { 'string': 'str', 'enum1': 'EnumOne' } } +{ 'struct': 'UserDefUnionBase2', + 'base': 'UserDefZero', + 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } } + # this variant of UserDefFlatUnion defaults to a union that uses fields with # allocated types to test corner cases in the cleanup/dealloc visitor { 'union': 'UserDefFlatUnion2', - 'base': 'UserDefUnionBase', + 'base': 'UserDefUnionBase2', 'discriminator': 'enum1', 'data': { 'value1' : 'UserDefC', # intentional forward reference - 'value2' : 'UserDefB', - 'value3' : 'UserDefA' } } + 'value2' : 'UserDefB' } } { 'alternate': 'UserDefAlternate', 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 2c546b7..241aadb 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -121,11 +121,10 @@ object UserDefFlatUnion case value2: UserDefB case value3: UserDefB object UserDefFlatUnion2 - base UserDefUnionBase + base UserDefUnionBase2 tag enum1 case value1: UserDefC case value2: UserDefB - case value3: UserDefA object UserDefNativeListUnion member type: UserDefNativeListUnionKind optional=False case integer: :obj-intList-wrapper @@ -167,6 +166,10 @@ object UserDefUnionBase base UserDefZero member string: str optional=False member enum1: EnumOne optional=False +object UserDefUnionBase2 + base UserDefZero + member string: str optional=False + member enum1: QEnumTwo optional=False object UserDefZero member integer: int optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct -- cgit v1.1 From 02a57ae32b08e8981b59979b80e682c9a153e94d Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:16 -0700 Subject: qapi: Forbid empty unions and useless alternates Empty unions serve no purpose, and while we compile with gcc which permits them, strict C99 forbids them. We happen to inject a dummy 'void *data' member into the C unions that represent QAPI unions and alternates, but we want to get rid of that member (it pollutes the namespace for no good reason), which would leave us with an empty union if the user didn't provide any branches. While empty structs make sense in QAPI, empty unions don't add any expressiveness to the QMP language. So prohibit them at parse time. Update the documentation and testsuite to match. Note that the documentation already mentioned that alternates should have "two or more JSON data types"; so this also fixes the code to enforce that. However, we have existing uses of a union type with only one branch, so the 2-or-more strictness is intentionally limited to alternates. Signed-off-by: Eric Blake Message-Id: <1455778109-6278-3-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/qapi-schema/alternate-empty.err | 1 + tests/qapi-schema/alternate-empty.exit | 2 +- tests/qapi-schema/alternate-empty.json | 2 +- tests/qapi-schema/alternate-empty.out | 5 ----- tests/qapi-schema/flat-union-empty.err | 1 + tests/qapi-schema/flat-union-empty.exit | 2 +- tests/qapi-schema/flat-union-empty.json | 2 +- tests/qapi-schema/flat-union-empty.out | 9 --------- tests/qapi-schema/union-empty.err | 1 + tests/qapi-schema/union-empty.exit | 2 +- tests/qapi-schema/union-empty.json | 2 +- tests/qapi-schema/union-empty.out | 6 ------ 12 files changed, 9 insertions(+), 26 deletions(-) (limited to 'tests') diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err index e69de29..bb06c5b 100644 --- a/tests/qapi-schema/alternate-empty.err +++ b/tests/qapi-schema/alternate-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data' diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/alternate-empty.exit +++ b/tests/qapi-schema/alternate-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json index db3820f..fff15ba 100644 --- a/tests/qapi-schema/alternate-empty.json +++ b/tests/qapi-schema/alternate-empty.json @@ -1,2 +1,2 @@ -# FIXME - alternates should list at least two types to be useful +# alternates must list at least two types to be useful { 'alternate': 'Alt', 'data': { 'i': 'int' } } diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out index f78f174..e69de29 100644 --- a/tests/qapi-schema/alternate-empty.out +++ b/tests/qapi-schema/alternate-empty.out @@ -1,5 +0,0 @@ -object :empty -alternate Alt - case i: int -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err index e69de29..15754f5 100644 --- a/tests/qapi-schema/flat-union-empty.err +++ b/tests/qapi-schema/flat-union-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-empty.json:4: Union 'Union' cannot have empty 'data' diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/flat-union-empty.exit +++ b/tests/qapi-schema/flat-union-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json index 67dd297..77f1d9a 100644 --- a/tests/qapi-schema/flat-union-empty.json +++ b/tests/qapi-schema/flat-union-empty.json @@ -1,4 +1,4 @@ -# FIXME - flat unions should not be empty +# flat unions cannot be empty { 'enum': 'Empty', 'data': [ ] } { 'struct': 'Base', 'data': { 'type': 'Empty' } } { 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } } diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out index eade2d5..e69de29 100644 --- a/tests/qapi-schema/flat-union-empty.out +++ b/tests/qapi-schema/flat-union-empty.out @@ -1,9 +0,0 @@ -object :empty -object Base - member type: Empty optional=False -enum Empty [] -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE -object Union - base Base - tag type diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err index e69de29..12c2022 100644 --- a/tests/qapi-schema/union-empty.err +++ b/tests/qapi-schema/union-empty.err @@ -0,0 +1 @@ +tests/qapi-schema/union-empty.json:2: Union 'Union' cannot have empty 'data' diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/union-empty.exit +++ b/tests/qapi-schema/union-empty.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json index 1785007..1f0b13c 100644 --- a/tests/qapi-schema/union-empty.json +++ b/tests/qapi-schema/union-empty.json @@ -1,2 +1,2 @@ -# FIXME - unions should not be empty +# unions cannot be empty { 'union': 'Union', 'data': { } } diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out index bdf17e5..e69de29 100644 --- a/tests/qapi-schema/union-empty.out +++ b/tests/qapi-schema/union-empty.out @@ -1,6 +0,0 @@ -object :empty -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] - prefix QTYPE -object Union - member type: UnionKind optional=False -enum UnionKind [] -- cgit v1.1 From 46534309e667fd860720f983c2c9aefe0354340d Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:17 -0700 Subject: qapi: Forbid 'any' inside an alternate The whole point of an alternate is to allow some type-safety while still accepting more than one JSON type. Meanwhile, the 'any' type exists to bypass type-safety altogether. The two are incompatible: you can't accept every type, and still tell which branch of the alternate to use for the parse; fix this to give a sane error instead of a Python stack trace. Note that other types that can't be alternate members are caught earlier, by check_type(). Signed-off-by: Eric Blake Message-Id: <1455778109-6278-4-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster --- tests/Makefile | 1 + tests/qapi-schema/alternate-any.err | 1 + tests/qapi-schema/alternate-any.exit | 1 + tests/qapi-schema/alternate-any.json | 4 ++++ tests/qapi-schema/alternate-any.out | 0 5 files changed, 7 insertions(+) create mode 100644 tests/qapi-schema/alternate-any.err create mode 100644 tests/qapi-schema/alternate-any.exit create mode 100644 tests/qapi-schema/alternate-any.json create mode 100644 tests/qapi-schema/alternate-any.out (limited to 'tests') diff --git a/tests/Makefile b/tests/Makefile index c1c605f..7c66d16 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) check-qtest-generic-y += tests/qom-test$(EXESUF) +qapi-schema += alternate-any.json qapi-schema += alternate-array.json qapi-schema += alternate-base.json qapi-schema += alternate-clash.json diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err new file mode 100644 index 0000000..aaa0154 --- /dev/null +++ b/tests/qapi-schema/alternate-any.err @@ -0,0 +1 @@ +tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any' diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/alternate-any.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json new file mode 100644 index 0000000..e47a73a --- /dev/null +++ b/tests/qapi-schema/alternate-any.json @@ -0,0 +1,4 @@ +# we do not allow the 'any' type as an alternate branch +{ 'alternate': 'Alt', + 'data': { 'one': 'any', + 'two': 'int' } } diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out new file mode 100644 index 0000000..e69de29 -- cgit v1.1 From 68d078395d8233ca8455a95f05e5c23e367777c2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:18 -0700 Subject: qapi: Add tests of complex objects within alternate Upcoming patches will adjust how we visit an object branch of an alternate; but we were completely lacking testsuite coverage. Rectify this, so that the future patches will be able to highlight the changes and still prove that we avoided regressions. In particular, the use of a flat union UserDefFlatUnion rather than a simple struct UserDefA as the branch will give us coverage of an object with variants. And visiting an alternate as both the top level and as a nested member gives confidence in correct memory allocation handling, especially if the test is run under valgrind. Signed-off-by: Eric Blake Message-Id: <1455778109-6278-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- tests/qapi-schema/qapi-schema-test.json | 4 +++- tests/qapi-schema/qapi-schema-test.out | 4 +++- tests/test-qmp-input-visitor.c | 37 ++++++++++++++++++++++++++++++++- tests/test-qmp-output-visitor.c | 26 ++++++++++++++++++++++- 4 files changed, 67 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 353a34e..632964a 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -85,8 +85,10 @@ 'data': { 'value1' : 'UserDefC', # intentional forward reference 'value2' : 'UserDefB' } } +{ 'struct': 'WrapAlternate', + 'data': { 'alt': 'UserDefAlternate' } } { 'alternate': 'UserDefAlternate', - 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } + 'data': { 'udfu': 'UserDefFlatUnion', 's': 'str', 'i': 'int' } } { 'struct': 'UserDefC', 'data': { 'string1': 'str', 'string2': 'str' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 241aadb..f5e2a73 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -105,7 +105,7 @@ object UserDefA member boolean: bool optional=False member a_b: int optional=True alternate UserDefAlternate - case uda: UserDefA + case udfu: UserDefFlatUnion case s: str case i: int object UserDefB @@ -172,6 +172,8 @@ object UserDefUnionBase2 member enum1: QEnumTwo optional=False object UserDefZero member integer: int optional=False +object WrapAlternate + member alt: UserDefAlternate optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct alternate __org.qemu_x-Alt case __org.qemu_x-branch: str diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index c72cdad..ef836d5 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -1,7 +1,7 @@ /* * QMP Input Visitor unit-tests. * - * Copyright (C) 2011, 2015 Red Hat Inc. + * Copyright (C) 2011-2016 Red Hat Inc. * * Authors: * Luiz Capitulino @@ -309,6 +309,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, Visitor *v; Error *err = NULL; UserDefAlternate *tmp; + WrapAlternate *wrap; v = visitor_input_test_init(data, "42"); visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); @@ -322,10 +323,44 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpstr(tmp->u.s, ==, "string"); qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "{'integer':1, 'string':'str', " + "'enum1':'value1', 'boolean':true}"); + visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); + g_assert_cmpint(tmp->type, ==, QTYPE_QDICT); + g_assert_cmpint(tmp->u.udfu->integer, ==, 1); + g_assert_cmpstr(tmp->u.udfu->string, ==, "str"); + g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true); + g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false); + qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "false"); visit_type_UserDefAlternate(v, NULL, &tmp, &err); error_free_or_abort(&err); qapi_free_UserDefAlternate(tmp); + + v = visitor_input_test_init(data, "{ 'alt': 42 }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QINT); + g_assert_cmpint(wrap->alt->u.i, ==, 42); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': 'string' }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING); + g_assert_cmpstr(wrap->alt->u.s, ==, "string"); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': {'integer':1, 'string':'str', " + "'enum1':'value1', 'boolean':true} }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT); + g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1); + g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str"); + g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true); + g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false); + qapi_free_WrapAlternate(wrap); } static void test_visitor_in_alternate_number(TestInputVisitorData *data, diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 965f298..2b0f7e9 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -1,7 +1,7 @@ /* * QMP Output Visitor unit-tests. * - * Copyright (C) 2011, 2015 Red Hat Inc. + * Copyright (C) 2011-2016 Red Hat Inc. * * Authors: * Luiz Capitulino @@ -427,6 +427,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, { QObject *arg; UserDefAlternate *tmp; + QDict *qdict; tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QINT; @@ -453,6 +454,29 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qapi_free_UserDefAlternate(tmp); qobject_decref(arg); + + tmp = g_new0(UserDefAlternate, 1); + tmp->type = QTYPE_QDICT; + tmp->u.udfu = g_new0(UserDefFlatUnion, 1); + tmp->u.udfu->integer = 1; + tmp->u.udfu->string = g_strdup("str"); + tmp->u.udfu->enum1 = ENUM_ONE_VALUE1; + tmp->u.udfu->u.value1 = g_new0(UserDefA, 1); + tmp->u.udfu->u.value1->boolean = true; + + visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort); + arg = qmp_output_get_qobject(data->qov); + + g_assert_cmpint(qobject_type(arg), ==, QTYPE_QDICT); + qdict = qobject_to_qdict(arg); + g_assert_cmpint(qdict_size(qdict), ==, 4); + g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 1); + g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str"); + g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1"); + g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true); + + qapi_free_UserDefAlternate(tmp); + qobject_decref(arg); } static void test_visitor_out_empty(TestOutputVisitorData *data, -- cgit v1.1 From becceedc4d9bc1435099c90a0514945a89844d3a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:26 -0700 Subject: qapi: Don't box struct branch of alternate There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ break; |+ } |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster --- tests/test-qmp-input-visitor.c | 20 ++++++++++---------- tests/test-qmp-output-visitor.c | 11 +++++------ 2 files changed, 15 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index ef836d5..47cf6aa 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -327,11 +327,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, "'enum1':'value1', 'boolean':true}"); visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); g_assert_cmpint(tmp->type, ==, QTYPE_QDICT); - g_assert_cmpint(tmp->u.udfu->integer, ==, 1); - g_assert_cmpstr(tmp->u.udfu->string, ==, "str"); - g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true); - g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false); + g_assert_cmpint(tmp->u.udfu.integer, ==, 1); + g_assert_cmpstr(tmp->u.udfu.string, ==, "str"); + g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true); + g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false); qapi_free_UserDefAlternate(tmp); v = visitor_input_test_init(data, "false"); @@ -355,11 +355,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, "'enum1':'value1', 'boolean':true} }"); visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT); - g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1); - g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str"); - g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true); - g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false); + g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1); + g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str"); + g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true); + g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false); qapi_free_WrapAlternate(wrap); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 2b0f7e9..fe2f1a1 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -457,12 +457,11 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QDICT; - tmp->u.udfu = g_new0(UserDefFlatUnion, 1); - tmp->u.udfu->integer = 1; - tmp->u.udfu->string = g_strdup("str"); - tmp->u.udfu->enum1 = ENUM_ONE_VALUE1; - tmp->u.udfu->u.value1 = g_new0(UserDefA, 1); - tmp->u.udfu->u.value1->boolean = true; + tmp->u.udfu.integer = 1; + tmp->u.udfu.string = g_strdup("str"); + tmp->u.udfu.enum1 = ENUM_ONE_VALUE1; + tmp->u.udfu.u.value1 = g_new0(UserDefA, 1); + tmp->u.udfu.u.value1->boolean = true; visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov); -- cgit v1.1 From 544a3731591f5d53e15f22de00ce5ac758d490b3 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:27 -0700 Subject: qapi: Don't box branches of flat unions There's no reason to do two malloc's for a flat union; let's just inline the branch struct directly into the C union branch of the flat union. Surprisingly, fewer clients were actually using explicit references to the branch types in comparison to the number of flat unions thus modified. This lets us reduce the hack in qapi-types:gen_variants() added in the previous patch; we no longer need to distinguish between alternates and flat unions. The change to unboxed structs means that u.data (added in commit cee2dedb) is now coincident with random fields of each branch of the flat union, whereas beforehand it was only coincident with pointers (since all branches of a flat union have to be objects). Note that this was already the case for simple unions - but there we got lucky. Remember, visit_start_union() blindly returns true for all visitors except for the dealloc visitor, where it returns the value !!obj->u.data, and that this result then controls whether to proceed with the visit to the variant. Pre-patch, this meant that flat unions were testing whether the boxed pointer was still NULL, and thereby skipping visit_end_implicit_struct() and avoiding a NULL dereference if the pointer had not been allocated. The same was true for simple unions where the current branch had pointer type, except there we bypassed visit_type_FOO(). But for simple unions where the current branch had scalar type, the contents of that scalar meant that the decision to call visit_type_FOO() was data-dependent - the reason we got lucky there is that visit_type_FOO() for all scalar types in the dealloc visitor is a no-op (only the pointer variants had anything to free), so it did not matter whether the dealloc visit was skipped. But with this patch, we would risk leaking memory if we could skip a call to visit_type_FOO_fields() based solely on a data-dependent decision. But notice: in the dealloc visitor, visit_type_FOO() already handles a NULL obj - it was only the visit_type_implicit_FOO() that was failing to check for NULL. And now that we have refactored things to have the branch be part of the parent struct, we no longer have a separate pointer that can be NULL in the first place. So we can just delete the call to visit_start_union() altogether, and blindly visit the branch type; there is no change in behavior except to the dealloc visitor, where we now unconditionally visit the branch, but where that visit is now always safe (for a flat union, we can no longer dereference NULL, and for a simple union, visit_type_FOO() was already safely handling NULL on pointer types). Unfortunately, simple unions are not as easy to switch to unboxed layout; because we are special-casing the hidden implicit type with a single 'data' member, we really DO need to keep calling another layer of visit_start_struct(), with a second malloc; although there are some cleanups planned for simple unions in later patches. visit_start_union() and gen_visit_implicit_struct() are now unused. Drop them. Note that after this patch, the only remaining use of visit_start_implicit_struct() is for alternate types; the next patch will do further cleanup based on that fact. Signed-off-by: Eric Blake Message-Id: <1455778109-6278-14-git-send-email-eblake@redhat.com> [Dead code deletion squashed in, commit message updated accordingly] Signed-off-by: Markus Armbruster --- tests/test-qmp-input-visitor.c | 10 +++++----- tests/test-qmp-output-visitor.c | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 47cf6aa..b05da5b 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -295,7 +295,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data, g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1); g_assert_cmpstr(tmp->string, ==, "str"); g_assert_cmpint(tmp->integer, ==, 41); - g_assert_cmpint(tmp->u.value1->boolean, ==, true); + g_assert_cmpint(tmp->u.value1.boolean, ==, true); base = qapi_UserDefFlatUnion_base(tmp); g_assert(&base->enum1 == &tmp->enum1); @@ -330,8 +330,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpint(tmp->u.udfu.integer, ==, 1); g_assert_cmpstr(tmp->u.udfu.string, ==, "str"); g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true); - g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false); + g_assert_cmpint(tmp->u.udfu.u.value1.boolean, ==, true); + g_assert_cmpint(tmp->u.udfu.u.value1.has_a_b, ==, false); qapi_free_UserDefAlternate(tmp); v = visitor_input_test_init(data, "false"); @@ -358,8 +358,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1); g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str"); g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true); - g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false); + g_assert_cmpint(wrap->alt->u.udfu.u.value1.boolean, ==, true); + g_assert_cmpint(wrap->alt->u.udfu.u.value1.has_a_b, ==, false); qapi_free_WrapAlternate(wrap); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index fe2f1a1..a7f8b45 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -403,9 +403,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data, UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion)); tmp->enum1 = ENUM_ONE_VALUE1; tmp->string = g_strdup("str"); - tmp->u.value1 = g_malloc0(sizeof(UserDefA)); tmp->integer = 41; - tmp->u.value1->boolean = true; + tmp->u.value1.boolean = true; visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov); @@ -460,8 +459,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, tmp->u.udfu.integer = 1; tmp->u.udfu.string = g_strdup("str"); tmp->u.udfu.enum1 = ENUM_ONE_VALUE1; - tmp->u.udfu.u.value1 = g_new0(UserDefA, 1); - tmp->u.udfu.u.value1->boolean = true; + tmp->u.udfu.u.value1.boolean = true; visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov); -- cgit v1.1