From e65d89bf1a4484e0db0f3dc820a8b209f2fb1e8b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:23 -0700 Subject: qapi: Adjust layout of FooList types By sticking the next pointer first, we don't need a union with 64-bit padding for smaller types. On 32-bit platforms, this can reduce the size of uint8List from 16 bytes (or 12, depending on whether 64-bit ints can tolerate 4-byte alignment) down to 8. It has no effect on 64-bit platforms (where alignment still dictates a 16-byte struct); but fewer anonymous unions is still a win in my book. It requires visit_next_list() to gain a size parameter, to know what size element to allocate; comparable to the size parameter of visit_start_struct(). I debated about going one step further, to allow for fewer casts, by doing: typedef GenericList GenericList; struct GenericList { GenericList *next; }; struct FooList { GenericList base; Foo *value; }; so that you convert to 'GenericList *' by '&foolist->base', and back by 'container_of(generic, GenericList, base)' (as opposed to the existing '(GenericList *)foolist' and '(FooList *)generic'). But doing that would require hoisting the declaration of GenericList prior to inclusion of qapi-types.h, rather than its current spot in visitor.h; it also makes iteration a bit more verbose through 'foolist->base.next' instead of 'foolist->next'. Note that for lists of objects, the 'value' payload is still hidden behind a boxed pointer. Someday, it would be nice to do: struct FooList { FooList *next; Foo value; }; for one less level of malloc for each list element. This patch is a step in that direction (now that 'next' is no longer at a fixed non-zero offset within the struct, we can store more than just a pointer's-worth of data as the value payload), but the actual conversion would be a task for another series, as it will touch a lot of code. Signed-off-by: Eric Blake Message-Id: <1455778109-6278-10-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'scripts/qapi-types.py') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 7b0dca8c72..83f230a662 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -26,11 +26,8 @@ def gen_array(name, element_type): return mcgen(''' struct %(c_name)s { - union { - %(c_type)s value; - uint64_t padding; - }; %(c_name)s *next; + %(c_type)s value; }; ''', c_name=c_name(name), c_type=element_type.c_type()) -- cgit 1.4.1 From 1de5d4ca0752138034305f3d4e8fe17ef6503569 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 17 Feb 2016 23:48:24 -0700 Subject: qapi: Emit structs used as variants in topological order Right now, we emit the branches of union types as a boxed pointer, and it suffices to have a forward declaration of the type. However, a future patch will swap things to directly use the branch type, instead of hiding it behind a pointer. For this to work, the compiler needs the full definition of the type, not just a forward declaration, prior to the union that is including the branch type. This patch just adds topological sorting to hoist all types mentioned in a branch of a union to be fully declared before the union itself. The sort is always possible, because we do not allow circular union types that include themselves as a direct branch (it is, however, still possible to include a branch type that itself has a pointer to the union, for a type that can indirectly recursively nest itself - that remains safe, because that the member of the branch type will remain a pointer, and the QMP representation of such a type adds another {} for each recurring layer of the union type). Signed-off-by: Eric Blake Message-Id: <1455778109-6278-11-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) (limited to 'scripts/qapi-types.py') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 83f230a662..6ea0ae6fb7 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -2,7 +2,7 @@ # QAPI types generator # # Copyright IBM, Corp. 2011 -# Copyright (c) 2013-2015 Red Hat Inc. +# Copyright (c) 2013-2016 Red Hat Inc. # # Authors: # Anthony Liguori @@ -14,6 +14,11 @@ from qapi import * +# variants must be emitted before their container; track what has already +# been output +objects_seen = set() + + def gen_fwd_object_or_array(name): return mcgen(''' @@ -49,11 +54,23 @@ def gen_struct_fields(members): def gen_object(name, base, members, variants): - ret = mcgen(''' + if name in objects_seen: + return '' + objects_seen.add(name) + + ret = '' + if variants: + for v in variants.variants: + if (isinstance(v.type, QAPISchemaObjectType) and + not v.type.is_implicit()): + ret += gen_object(v.type.name, v.type.base, + v.type.local_members, v.type.variants) + + ret += mcgen(''' struct %(c_name)s { ''', - c_name=c_name(name)) + c_name=c_name(name)) if base: ret += mcgen(''' -- cgit 1.4.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 --- scripts/qapi-types.py | 10 +++++++++- scripts/qapi-visit.py | 33 +++++++++++++++++++++++++++------ scripts/qapi.py | 12 +++++++----- tests/test-qmp-input-visitor.c | 20 ++++++++++---------- tests/test-qmp-output-visitor.c | 11 +++++------ 5 files changed, 58 insertions(+), 28 deletions(-) (limited to 'scripts/qapi-types.py') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 6ea0ae6fb7..4dabe91c92 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): + # HACK: Determine if this is an alternate (at least one variant + # is not an object); unions have all branches as objects. + unboxed = False + for v in variants.variants: + if not isinstance(v.type, QAPISchemaObjectType): + unboxed = True + break + # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -136,7 +144,7 @@ def gen_variants(variants): ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(), + c_type=typ.c_type(is_unboxed=unboxed), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index d7d7ed5a32..f4e38d1068 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -201,11 +201,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error def gen_visit_alternate(name, variants): promote_int = 'true' + ret = '' for var in variants.variants: if var.type.alternate_qtype() == 'QTYPE_QINT': promote_int = 'false' + if isinstance(var.type, QAPISchemaObjectType): + ret += gen_visit_fields_decl(var.type) - ret = mcgen(''' + ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { @@ -221,17 +224,35 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } switch ((*obj)->type) { ''', - c_name=c_name(name), promote_int=promote_int) + c_name=c_name(name), promote_int=promote_int) for var in variants.variants: ret += mcgen(''' case %(case)s: +''', + case=var.type.alternate_qtype()) + if isinstance(var.type, QAPISchemaObjectType): + ret += mcgen(''' + visit_start_struct(v, name, NULL, 0, &err); + if (err) { + break; + } + visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err); + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); +''', + c_type=var.type.c_name(), + c_name=c_name(var.name)) + else: + ret += mcgen(''' visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); - break; ''', - case=var.type.alternate_qtype(), - c_type=var.type.c_name(), - c_name=c_name(var.name)) + c_type=var.type.c_name(), + c_name=c_name(var.name)) + ret += mcgen(''' + break; +''') ret += mcgen(''' default: diff --git a/scripts/qapi.py b/scripts/qapi.py index 17bf633e39..8497777d94 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -824,7 +824,7 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): return c_name(self.name) + pointer_suffix def c_null(self): @@ -857,7 +857,7 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): if is_param and self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -891,7 +891,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): return c_name(self.name) def member_names(self): @@ -987,9 +987,11 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): assert not self.is_implicit() - return QAPISchemaType.c_type(self) + if is_unboxed: + return c_name(self.name) + return c_name(self.name) + pointer_suffix def json_type(self): return 'object' diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index ef836d5fbe..47cf6aaff0 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 2b0f7e9c53..fe2f1a1326 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 1.4.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 --- cpus.c | 18 ++++++------------ hmp.c | 12 ++++++------ include/qapi/visitor-impl.h | 2 -- include/qapi/visitor.h | 1 - qapi/qapi-dealloc-visitor.c | 26 -------------------------- qapi/qapi-visit-core.c | 8 -------- scripts/qapi-types.py | 13 +++---------- scripts/qapi-visit.py | 36 ++---------------------------------- tests/test-qmp-input-visitor.c | 10 +++++----- tests/test-qmp-output-visitor.c | 6 ++---- 10 files changed, 24 insertions(+), 108 deletions(-) (limited to 'scripts/qapi-types.py') diff --git a/cpus.c b/cpus.c index 898426ca56..9592163ff4 100644 --- a/cpus.c +++ b/cpus.c @@ -1568,28 +1568,22 @@ CpuInfoList *qmp_query_cpus(Error **errp) info->value->thread_id = cpu->thread_id; #if defined(TARGET_I386) info->value->arch = CPU_INFO_ARCH_X86; - info->value->u.x86 = g_new0(CpuInfoX86, 1); - info->value->u.x86->pc = env->eip + env->segs[R_CS].base; + info->value->u.x86.pc = env->eip + env->segs[R_CS].base; #elif defined(TARGET_PPC) info->value->arch = CPU_INFO_ARCH_PPC; - info->value->u.ppc = g_new0(CpuInfoPPC, 1); - info->value->u.ppc->nip = env->nip; + info->value->u.ppc.nip = env->nip; #elif defined(TARGET_SPARC) info->value->arch = CPU_INFO_ARCH_SPARC; - info->value->u.q_sparc = g_new0(CpuInfoSPARC, 1); - info->value->u.q_sparc->pc = env->pc; - info->value->u.q_sparc->npc = env->npc; + info->value->u.q_sparc.pc = env->pc; + info->value->u.q_sparc.npc = env->npc; #elif defined(TARGET_MIPS) info->value->arch = CPU_INFO_ARCH_MIPS; - info->value->u.q_mips = g_new0(CpuInfoMIPS, 1); - info->value->u.q_mips->PC = env->active_tc.PC; + info->value->u.q_mips.PC = env->active_tc.PC; #elif defined(TARGET_TRICORE) info->value->arch = CPU_INFO_ARCH_TRICORE; - info->value->u.tricore = g_new0(CpuInfoTricore, 1); - info->value->u.tricore->PC = env->PC; + info->value->u.tricore.PC = env->PC; #else info->value->arch = CPU_INFO_ARCH_OTHER; - info->value->u.other = g_new0(CpuInfoOther, 1); #endif /* XXX: waiting for the qapi to support GSList */ diff --git a/hmp.c b/hmp.c index 996cb91027..bfbd667033 100644 --- a/hmp.c +++ b/hmp.c @@ -314,22 +314,22 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) switch (cpu->value->arch) { case CPU_INFO_ARCH_X86: - monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc); + monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc); break; case CPU_INFO_ARCH_PPC: - monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc->nip); + monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip); break; case CPU_INFO_ARCH_SPARC: monitor_printf(mon, " pc=0x%016" PRIx64, - cpu->value->u.q_sparc->pc); + cpu->value->u.q_sparc.pc); monitor_printf(mon, " npc=0x%016" PRIx64, - cpu->value->u.q_sparc->npc); + cpu->value->u.q_sparc.npc); break; case CPU_INFO_ARCH_MIPS: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips->PC); + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC); break; case CPU_INFO_ARCH_TRICORE: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore->PC); + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC); break; default: break; diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 7905a2845e..c4af3e042f 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -58,8 +58,6 @@ struct Visitor /* May be NULL; most useful for input visitors. */ void (*optional)(Visitor *v, const char *name, bool *present); - - bool (*start_union)(Visitor *v, bool data_present, Error **errp); }; void input_type_enum(Visitor *v, const char *name, int *obj, diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 8a2d5ccacc..a6678b16ef 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -79,6 +79,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); void visit_type_number(Visitor *v, const char *name, double *obj, Error **errp); void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp); -bool visit_start_union(Visitor *v, bool data_present, Error **errp); #endif diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 6667e8ca43..4eae555ace 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj, { } -/* If there's no data present, the dealloc visitor has nothing to free. - * Thus, indicate to visitor code that the subsequent union fields can - * be skipped. This is not an error condition, since the cleanup of the - * rest of an object can continue unhindered, so leave errp unset in - * these cases. - * - * NOTE: In cases where we're attempting to deallocate an object that - * may have missing fields, the field indicating the union type may - * be missing. In such a case, it's possible we don't have enough - * information to differentiate data_present == false from a case where - * data *is* present but happens to be a scalar with a value of 0. - * This is okay, since in the case of the dealloc visitor there's no - * work that needs to done in either situation. - * - * The current inability in QAPI code to more thoroughly verify a union - * type in such cases will likely need to be addressed if we wish to - * implement this interface for other types of visitors in the future, - * however. - */ -static bool qapi_dealloc_start_union(Visitor *v, bool data_present, - Error **errp) -{ - return data_present; -} - Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) { return &v->visitor; @@ -224,7 +199,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_str = qapi_dealloc_type_str; v->visitor.type_number = qapi_dealloc_type_number; v->visitor.type_any = qapi_dealloc_type_anything; - v->visitor.start_union = qapi_dealloc_start_union; QTAILQ_INIT(&v->stack); diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index b4a0f21b36..f7b99807b0 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -61,14 +61,6 @@ void visit_end_list(Visitor *v) v->end_list(v); } -bool visit_start_union(Visitor *v, bool data_present, Error **errp) -{ - if (v->start_union) { - return v->start_union(v, data_present, errp); - } - return true; -} - bool visit_optional(Visitor *v, const char *name, bool *present) { if (v->optional) { diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 4dabe91c92..eac90d2fe9 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): - # HACK: Determine if this is an alternate (at least one variant - # is not an object); unions have all branches as objects. - unboxed = False - for v in variants.variants: - if not isinstance(v.type, QAPISchemaObjectType): - unboxed = True - break - # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -140,11 +132,12 @@ def gen_variants(variants): for var in variants.variants: # Ugly special case for simple union TODO get rid of it - typ = var.simple_union_type() or var.type + simple_union_type = var.simple_union_type() + typ = simple_union_type or var.type ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(is_unboxed=unboxed), + c_type=typ.c_type(is_unboxed=not simple_union_type), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index f4e38d1068..3a3918f952 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -15,10 +15,6 @@ from qapi import * import re -# visit_type_FOO_implicit() is emitted as needed; track if it has already -# been output. -implicit_structs_seen = set() - # visit_type_FOO_fields() is always emitted; track if a forward declaration # or implementation has already been output. struct_fields_seen = set() @@ -45,31 +41,6 @@ static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **er c_type=typ.c_name()) -def gen_visit_implicit_struct(typ): - if typ in implicit_structs_seen: - return '' - implicit_structs_seen.add(typ) - - ret = gen_visit_fields_decl(typ) - - ret += mcgen(''' - -static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp) -{ - Error *err = NULL; - - visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err); - if (!err) { - visit_type_%(c_type)s_fields(v, *obj, errp); - visit_end_implicit_struct(v); - } - error_propagate(errp, err); -} -''', - c_type=typ.c_name()) - return ret - - def gen_visit_struct_fields(name, base, members, variants): ret = '' @@ -79,7 +50,7 @@ def gen_visit_struct_fields(name, base, members, variants): for var in variants.variants: # Ugly special case for simple union TODO get rid of it if not var.simple_union_type(): - ret += gen_visit_implicit_struct(var.type) + ret += gen_visit_fields_decl(var.type) struct_fields_seen.add(name) ret += mcgen(''' @@ -102,9 +73,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er if variants: ret += mcgen(''' - if (!visit_start_union(v, !!obj->u.data, &err) || err) { - goto out; - } switch (obj->%(c_name)s) { ''', c_name=c_name(variants.tag_member.name)) @@ -126,7 +94,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er c_name=c_name(var.name)) else: ret += mcgen(''' - visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err); + visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err); ''', c_type=var.type.c_name(), c_name=c_name(var.name)) diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 47cf6aaff0..b05da5baa1 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 fe2f1a1326..a7f8b45c77 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 1.4.1