From patchwork Fri Oct 23 05:09:43 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 534789 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 5A1AC1402D6 for ; Fri, 23 Oct 2015 16:18:44 +1100 (AEDT) Received: from localhost ([::1]:36104 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpUkT-0005zJ-WF for incoming@patchwork.ozlabs.org; Fri, 23 Oct 2015 01:18:42 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55560) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpUcC-0006ya-VC for qemu-devel@nongnu.org; Fri, 23 Oct 2015 01:10:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpUcA-0003HF-M1 for qemu-devel@nongnu.org; Fri, 23 Oct 2015 01:10:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57486) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpUcA-0003GV-BF for qemu-devel@nongnu.org; Fri, 23 Oct 2015 01:10:06 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id E32D38E23F; Fri, 23 Oct 2015 05:10:05 +0000 (UTC) Received: from red.redhat.com (ovpn-113-176.phx2.redhat.com [10.3.113.176]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9N59xEB026589; Fri, 23 Oct 2015 01:10:05 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Thu, 22 Oct 2015 23:09:43 -0600 Message-Id: <1445576998-2921-11-git-send-email-eblake@redhat.com> In-Reply-To: <1445576998-2921-1-git-send-email-eblake@redhat.com> References: <1445576998-2921-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Michael Roth , Gerd Hoffmann , armbru@redhat.com, Luiz Capitulino Subject: [Qemu-devel] [PATCH v10 10/25] qapi: Unbox base members X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Rather than storing a base class as a pointer to a box, just store the fields of that base class in the same order, so that a child struct can be directly cast to its parent. This gives less malloc overhead, less pointer dereferencing, and even less generated code. Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer struct for flat unions" (although that patch had fewer places to change, as less of qemu was directly using qapi structs for flat unions). It also drops a hack that was needed for type-safe casting to the base class of a struct. Changes to the generated code look like this in qapi-types.h: | struct SpiceChannel { |- SpiceBasicInfo *base; |+ /* Members inherited from SpiceBasicInfo: */ |+ char *host; |+ char *port; |+ NetworkAddressFamily family; |+ /* Own members: */ | int64_t connection_id; as well as: | static inline SpiceBasicInfo *qapi_SpiceChannel_base(const SpiceChannel *obj) | { |- return (SpiceBasicInfo *)obj->base; |+ return (SpiceBasicInfo *)obj; | } and this in qapi-visit.c: | static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp) | { | Error *err = NULL; | |- visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err); |+ visit_type_SpiceBasiInfo_fields(v, (SpiceBasicInfo **)obj, &err); | if (err) { plus the wholesale elimination of some now-unused visit_type_implicit_FOO() functions. Without boxing, the corner case of one empty struct having another empty struct as its base type now requires inserting a dummy member (previously, the pointer to the base would have sufficed). And now that we no longer consume a 'base' member in the generated C struct, we can delete the former negative struct-base-clash-base test. Signed-off-by: Eric Blake --- v10: split off some of the cleanups into earlier patches, improve commit message, don't emit visit_type_FOO_fields out of order, save positive tests in qapi-schema-tests for later v9: (no v6-8): hoist from v5 34/46, rebase to master --- hmp.c | 6 +++--- scripts/qapi-types.py | 20 ++++++-------------- scripts/qapi-visit.py | 13 ++++--------- tests/Makefile | 1 - tests/qapi-schema/struct-base-clash-base.err | 0 tests/qapi-schema/struct-base-clash-base.exit | 1 - tests/qapi-schema/struct-base-clash-base.json | 9 --------- tests/qapi-schema/struct-base-clash-base.out | 5 ----- tests/test-qmp-commands.c | 15 +++++---------- tests/test-qmp-event.c | 8 ++------ tests/test-qmp-input-visitor.c | 4 ++-- tests/test-qmp-output-visitor.c | 13 +++++-------- tests/test-visitor-serialization.c | 14 ++++++-------- ui/spice-core.c | 9 +++------ ui/vnc.c | 12 ++++-------- 15 files changed, 40 insertions(+), 90 deletions(-) delete mode 100644 tests/qapi-schema/struct-base-clash-base.err delete mode 100644 tests/qapi-schema/struct-base-clash-base.exit delete mode 100644 tests/qapi-schema/struct-base-clash-base.json delete mode 100644 tests/qapi-schema/struct-base-clash-base.out diff --git a/hmp.c b/hmp.c index 5048eee..88fd804 100644 --- a/hmp.c +++ b/hmp.c @@ -569,8 +569,8 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict) for (client = info->clients; client; client = client->next) { monitor_printf(mon, "Client:\n"); monitor_printf(mon, " address: %s:%s\n", - client->value->base->host, - client->value->base->service); + client->value->host, + client->value->service); monitor_printf(mon, " x509_dname: %s\n", client->value->x509_dname ? client->value->x509_dname : "none"); @@ -638,7 +638,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict) for (chan = info->channels; chan; chan = chan->next) { monitor_printf(mon, "Channel:\n"); monitor_printf(mon, " address: %s:%s%s\n", - chan->value->base->host, chan->value->base->port, + chan->value->host, chan->value->port, chan->value->tls ? " [tls]" : ""); monitor_printf(mon, " session: %" PRId64 "\n", chan->value->connection_id); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index ed4a11c..9c51d8f 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -76,16 +76,13 @@ struct %(c_name)s { ''', c_name=c_name(name)) - if base: - ret += gen_struct_field('base', base, False) - - ret += gen_struct_fields(members) + ret += gen_struct_fields(members, base) # Make sure that all structs have at least one field; this avoids # potential issues with attempting to malloc space for zero-length # structs in C, and also incompatibility with C++ (where an empty # struct is size 1). - if not base and not members: + if not (base and base.members) and not members: ret += mcgen(''' char qapi_dummy_field_for_empty_struct; ''') @@ -97,21 +94,17 @@ struct %(c_name)s { return ret -def gen_upcast(name, base, struct): +def gen_upcast(name, base): # C makes const-correctness ugly. We have to cast away const to let # this function work for both const and non-const obj. - # TODO Ugly difference between struct and flat union bases - member = '' - if struct: - member = '->base' return mcgen(''' static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) { - return (%(base)s *)obj%(member)s; + return (%(base)s *)obj; } ''', - c_name=c_name(name), base=base.c_name(), member=member) + c_name=c_name(name), base=base.c_name()) def gen_alternate_qtypes_decl(name): @@ -286,8 +279,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): else: self.decl += gen_struct(name, base, members) if base: - # TODO Drop last param once structs have sane layout - self.decl += gen_upcast(name, base, not variants) + self.decl += gen_upcast(name, base) self._gen_type_cleanup(name) def visit_alternate_type(self, name, info, variants): diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 7204ed0..e30062b 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -72,26 +72,21 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * def gen_visit_struct_fields(name, base, members): - ret = '' - - if base: - ret += gen_visit_implicit_struct(base) - struct_fields_seen.add(name) - ret += mcgen(''' + ret = mcgen(''' static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp) { Error *err = NULL; ''', - c_name=c_name(name)) + c_name=c_name(name)) if base: ret += mcgen(''' - visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err); + visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err); ''', - c_type=base.c_name(), c_name=c_name('base')) + c_type=base.c_name()) ret += gen_err_check() ret += gen_visit_fields(members, prefix='(*obj)->') diff --git a/tests/Makefile b/tests/Makefile index cc6b24f..9194264 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -322,7 +322,6 @@ qapi-schema += returns-array-bad.json qapi-schema += returns-dict.json qapi-schema += returns-unknown.json qapi-schema += returns-whitelist.json -qapi-schema += struct-base-clash-base.json qapi-schema += struct-base-clash-deep.json qapi-schema += struct-base-clash.json qapi-schema += struct-data-invalid.json diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err deleted file mode 100644 index e69de29..0000000 diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit deleted file mode 100644 index 573541a..0000000 --- a/tests/qapi-schema/struct-base-clash-base.exit +++ /dev/null @@ -1 +0,0 @@ -0 diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json deleted file mode 100644 index 0c84025..0000000 --- a/tests/qapi-schema/struct-base-clash-base.json +++ /dev/null @@ -1,9 +0,0 @@ -# Struct member 'base' -# FIXME: this parses, but then fails to compile due to a duplicate 'base' -# (one explicit in QMP, the other used to box the base class members). -# We should either reject the collision at parse time, or change the -# generated struct to allow this to compile. -{ 'struct': 'Base', 'data': {} } -{ 'struct': 'Sub', - 'base': 'Base', - 'data': { 'base': 'str' } } diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out deleted file mode 100644 index e69a416..0000000 --- a/tests/qapi-schema/struct-base-clash-base.out +++ /dev/null @@ -1,5 +0,0 @@ -object :empty -object Base -object Sub - base Base - member base: str optional=False diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index bc59835..ea700d8 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -25,11 +25,9 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a, UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne)); ud1c->string = strdup(ud1a->string); - ud1c->base = g_new0(UserDefZero, 1); - ud1c->base->integer = ud1a->base->integer; + ud1c->integer = ud1a->integer; ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0"); - ud1d->base = g_new0(UserDefZero, 1); - ud1d->base->integer = has_udb1 ? ud1b->base->integer : 0; + ud1d->integer = has_udb1 ? ud1b->integer : 0; ret = g_new0(UserDefTwo, 1); ret->string0 = strdup("blah1"); @@ -176,20 +174,17 @@ static void test_dealloc_types(void) UserDefOneList *ud1list; ud1test = g_malloc0(sizeof(UserDefOne)); - ud1test->base = g_new0(UserDefZero, 1); - ud1test->base->integer = 42; + ud1test->integer = 42; ud1test->string = g_strdup("hi there 42"); qapi_free_UserDefOne(ud1test); ud1a = g_malloc0(sizeof(UserDefOne)); - ud1a->base = g_new0(UserDefZero, 1); - ud1a->base->integer = 43; + ud1a->integer = 43; ud1a->string = g_strdup("hi there 43"); ud1b = g_malloc0(sizeof(UserDefOne)); - ud1b->base = g_new0(UserDefZero, 1); - ud1b->base->integer = 44; + ud1b->integer = 44; ud1b->string = g_strdup("hi there 44"); ud1list = g_malloc0(sizeof(UserDefOneList)); diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c index 28f146d..035c65c 100644 --- a/tests/test-qmp-event.c +++ b/tests/test-qmp-event.c @@ -179,9 +179,7 @@ static void test_event_c(TestEventData *data, QDict *d, *d_data, *d_b; UserDefOne b; - UserDefZero z; - z.integer = 2; - b.base = &z; + b.integer = 2; b.string = g_strdup("test1"); b.has_enum1 = false; @@ -209,11 +207,9 @@ static void test_event_d(TestEventData *data, { UserDefOne struct1; EventStructOne a; - UserDefZero z; QDict *d, *d_data, *d_a, *d_struct1; - z.integer = 2; - struct1.base = &z; + struct1.integer = 2; struct1.string = g_strdup("test1"); struct1.has_enum1 = true; struct1.enum1 = ENUM_ONE_VALUE1; diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 8941963..514cfd0 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -262,7 +262,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data, check_and_free_str(udp->string0, "string0"); check_and_free_str(udp->dict1->string1, "string1"); - g_assert_cmpint(udp->dict1->dict2->userdef->base->integer, ==, 42); + 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(udp->dict1->has_dict3 == false); @@ -292,7 +292,7 @@ static void test_visitor_in_list(TestInputVisitorData *data, snprintf(string, sizeof(string), "string%d", i); g_assert_cmpstr(item->value->string, ==, string); - g_assert_cmpint(item->value->base->integer, ==, 42 + i); + g_assert_cmpint(item->value->integer, ==, 42 + i); } qapi_free_UserDefOneList(head); diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index c84002e..cfb06bb 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -250,16 +250,14 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data, ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2)); ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1); ud2->dict1->dict2->userdef->string = g_strdup(string); - ud2->dict1->dict2->userdef->base = g_new0(UserDefZero, 1); - ud2->dict1->dict2->userdef->base->integer = value; + ud2->dict1->dict2->userdef->integer = value; ud2->dict1->dict2->string = g_strdup(strings[2]); ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3)); ud2->dict1->has_dict3 = true; ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1); ud2->dict1->dict3->userdef->string = g_strdup(string); - ud2->dict1->dict3->userdef->base = g_new0(UserDefZero, 1); - ud2->dict1->dict3->userdef->base->integer = value; + ud2->dict1->dict3->userdef->integer = value; ud2->dict1->dict3->string = g_strdup(strings[3]); visit_type_UserDefTwo(data->ov, &ud2, "unused", &err); @@ -301,8 +299,8 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, const void *unused) { EnumOne bad_values[] = { ENUM_ONE_MAX, -1 }; - UserDefZero b; - UserDefOne u = { .base = &b }, *pu = &u; + UserDefOne u = {0}; + UserDefOne *pu = &u; Error *err; int i; @@ -416,8 +414,7 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, p->value->dict1->dict2 = g_new0(UserDefTwoDictDict, 1); p->value->dict1->dict2->userdef = g_new0(UserDefOne, 1); p->value->dict1->dict2->userdef->string = g_strdup(string); - p->value->dict1->dict2->userdef->base = g_new0(UserDefZero, 1); - p->value->dict1->dict2->userdef->base->integer = 42; + p->value->dict1->dict2->userdef->integer = 42; p->value->dict1->dict2->string = g_strdup(string); p->value->dict1->has_dict3 = false; diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index fa86cae..634563b 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -258,15 +258,13 @@ static UserDefTwo *nested_struct_create(void) udnp->dict1->string1 = strdup("test_string1"); udnp->dict1->dict2 = g_malloc0(sizeof(*udnp->dict1->dict2)); udnp->dict1->dict2->userdef = g_new0(UserDefOne, 1); - udnp->dict1->dict2->userdef->base = g_new0(UserDefZero, 1); - udnp->dict1->dict2->userdef->base->integer = 42; + udnp->dict1->dict2->userdef->integer = 42; udnp->dict1->dict2->userdef->string = strdup("test_string"); udnp->dict1->dict2->string = strdup("test_string2"); udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3)); udnp->dict1->has_dict3 = true; udnp->dict1->dict3->userdef = g_new0(UserDefOne, 1); - udnp->dict1->dict3->userdef->base = g_new0(UserDefZero, 1); - udnp->dict1->dict3->userdef->base->integer = 43; + udnp->dict1->dict3->userdef->integer = 43; udnp->dict1->dict3->userdef->string = strdup("test_string"); udnp->dict1->dict3->string = strdup("test_string3"); return udnp; @@ -278,15 +276,15 @@ static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2) g_assert(udnp2); g_assert_cmpstr(udnp1->string0, ==, udnp2->string0); g_assert_cmpstr(udnp1->dict1->string1, ==, udnp2->dict1->string1); - g_assert_cmpint(udnp1->dict1->dict2->userdef->base->integer, ==, - udnp2->dict1->dict2->userdef->base->integer); + g_assert_cmpint(udnp1->dict1->dict2->userdef->integer, ==, + udnp2->dict1->dict2->userdef->integer); g_assert_cmpstr(udnp1->dict1->dict2->userdef->string, ==, udnp2->dict1->dict2->userdef->string); g_assert_cmpstr(udnp1->dict1->dict2->string, ==, udnp2->dict1->dict2->string); g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3); - g_assert_cmpint(udnp1->dict1->dict3->userdef->base->integer, ==, - udnp2->dict1->dict3->userdef->base->integer); + g_assert_cmpint(udnp1->dict1->dict3->userdef->integer, ==, + udnp2->dict1->dict3->userdef->integer); g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==, udnp2->dict1->dict3->userdef->string); g_assert_cmpstr(udnp1->dict1->dict3->string, ==, diff --git a/ui/spice-core.c b/ui/spice-core.c index d43cfbe..6a62d71 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -200,8 +200,6 @@ static void channel_event(int event, SpiceChannelEventInfo *info) { SpiceServerInfo *server = g_malloc0(sizeof(*server)); SpiceChannel *client = g_malloc0(sizeof(*client)); - server->base = g_malloc0(sizeof(*server->base)); - client->base = g_malloc0(sizeof(*client->base)); /* * Spice server might have called us from spice worker thread @@ -384,16 +382,15 @@ static SpiceChannelList *qmp_query_spice_channels(void) chan = g_malloc0(sizeof(*chan)); chan->value = g_malloc0(sizeof(*chan->value)); - chan->value->base = g_malloc0(sizeof(*chan->value->base)); paddr = (struct sockaddr *)&item->info->paddr_ext; plen = item->info->plen_ext; getnameinfo(paddr, plen, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV); - chan->value->base->host = g_strdup(host); - chan->value->base->port = g_strdup(port); - chan->value->base->family = inet_netfamily(paddr->sa_family); + chan->value->host = g_strdup(host); + chan->value->port = g_strdup(port); + chan->value->family = inet_netfamily(paddr->sa_family); chan->value->connection_id = item->info->connection_id; chan->value->channel_type = item->info->type; diff --git a/ui/vnc.c b/ui/vnc.c index 9473b32..cec2cee 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -262,7 +262,6 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd) Error *err = NULL; info = g_malloc(sizeof(*info)); - info->base = g_malloc(sizeof(*info->base)); vnc_init_basic_info_from_server_addr(vd->lsock, qapi_VncServerInfo_base(info), &err); info->has_auth = true; @@ -301,7 +300,6 @@ static void vnc_client_cache_addr(VncState *client) Error *err = NULL; client->info = g_malloc0(sizeof(*client->info)); - client->info->base = g_malloc0(sizeof(*client->info->base)); vnc_init_basic_info_from_remote_addr(client->csock, qapi_VncClientInfo_base(client->info), &err); @@ -319,7 +317,6 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event) if (!vs->info) { return; } - g_assert(vs->info->base); si = vnc_server_info_get(vs->vd); if (!si) { @@ -364,11 +361,10 @@ static VncClientInfo *qmp_query_vnc_client(const VncState *client) } info = g_malloc0(sizeof(*info)); - info->base = g_malloc0(sizeof(*info->base)); - info->base->host = g_strdup(host); - info->base->service = g_strdup(serv); - info->base->family = inet_netfamily(sa.ss_family); - info->base->websocket = client->websocket; + info->host = g_strdup(host); + info->service = g_strdup(serv); + info->family = inet_netfamily(sa.ss_family); + info->websocket = client->websocket; if (client->tls) { info->x509_dname = qcrypto_tls_session_get_peer_name(client->tls);