[Qemu-devel] [RFC PATCH 2/2] qapi: unbox base members

[Eric Blake]

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 safely cast to its parent.  This gives
less malloc overhead, less pointer dereferencing, and even less
generated code.

Signed-off-by: Eric Blake <address@hidden>
---

Note that there are probably further cleanups possible (for example,
smarter ways of generating a forward declaration of visit_FOO_fields
only when needed, while still maintaining alphabetical sorting of
the overall file; rather than my first cut which results in topological
rather than alphabetical sorting in some cases). Likewise, it will
probably need a rebase on top of Markus' series once that is no
longer RFC.  But the idea of posting this now is to get a feel for
what this does to our code base.

 hmp.c                              |  6 +++---
 scripts/qapi-types.py              | 13 ++++++++++---
 scripts/qapi-visit.py              |  9 ++++++---
 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    | 12 ++++--------
 tests/test-visitor-serialization.c | 14 ++++++--------
 ui/spice-core.c                    | 23 +++++++++++++----------
 ui/vnc.c                           | 20 +++++++++-----------
 10 files changed, 60 insertions(+), 64 deletions(-)

diff --git a/hmp.c b/hmp.c
index f8fb72f..9f202df 100644
--- a/hmp.c
+++ b/hmp.c
@@ -558,8 +558,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");
@@ -627,7 +627,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 c1db5e2..f04b6cc 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -47,11 +47,18 @@ def gen_struct_field(name, typ, optional):
                  c_type=typ.c_type(), c_name=c_name(name))
     return ret

-def gen_struct_fields(members):
+def gen_struct_fields(members, base=None):
     ret = ''
+    if base and base.base:
+        ret += gen_struct_fields(base.base.local_members, base.base)

     for memb in members:
         ret += gen_struct_field(memb.name, memb.type, memb.optional)
+    if base:
+        ret += mcgen('''
+    /* End fields inherited from %(c_name)s. */
+''',
+                     c_name=base.c_name())
     return ret

 def gen_struct(name, base, members):
@@ -62,7 +69,7 @@ struct %(c_name)s {
                 c_name=c_name(name))

     if base:
-        ret += gen_struct_field('base', base, False)
+        ret += gen_struct_fields(base.local_members, base)

     ret += gen_struct_fields(members)

@@ -117,7 +124,7 @@ struct %(c_name)s {
 ''',
                 c_name=c_name(name))
     if base:
-        ret += gen_struct_fields(base.members)
+        ret += gen_struct_fields(base.local_members, base)
         ret += mcgen('''
     /* union tag is %(c_type)s %(c_name)s */
 ''',
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f7faa22..30bd471 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -59,12 +59,15 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, 
%(c_type)s **obj, Error *
     return ret

 def gen_visit_struct_fields(name, base, members):
+    if name in struct_fields_seen:
+        return ''
     struct_fields_seen.add(name)

     ret = ''

     if base:
-        ret += gen_visit_implicit_struct(base)
+        ret += gen_visit_struct_fields(base.name, base.base,
+                                       base.local_members)

     ret += mcgen('''

@@ -78,12 +81,12 @@ static void visit_type_%(c_name)s_fields(Visitor *m, 
%(c_name)s **obj, Error **e

     if base:
         ret += mcgen('''
-visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+visit_type_%(c_type)s_fields(m, (%(c_type)s **)obj, &err);
 if (err) {
     goto out;
 }
 ''',
-                     c_type=base.c_name(), c_name=c_name('base'))
+                     c_type=base.c_name())

     for memb in members:
         if memb.optional:
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 87a6c30..8350b29 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");
@@ -171,20 +169,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 8662a57..2360438 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -265,7 +265,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);
@@ -295,7 +295,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 2162a4a..ab4bc5d 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,7 @@ 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, *pu = &u;
     Error *err;
     int i;

@@ -416,8 +413,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 bf4fd07..86f4441 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
@@ -218,9 +216,11 @@ static void channel_event(int event, SpiceChannelEventInfo 
*info)
     }

     if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
-        add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
+        add_addr_info((SpiceBasicInfo *)client,
+                      (struct sockaddr *)&info->paddr_ext,
                       info->plen_ext);
-        add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
+        add_addr_info((SpiceBasicInfo *)server,
+                      (struct sockaddr *)&info->laddr_ext,
                       info->llen_ext);
     } else {
         error_report("spice: %s, extended address is expected",
@@ -229,7 +229,9 @@ static void channel_event(int event, SpiceChannelEventInfo 
*info)

     switch (event) {
     case SPICE_CHANNEL_EVENT_CONNECTED:
-        qapi_event_send_spice_connected(server->base, client->base, 
&error_abort);
+        qapi_event_send_spice_connected((SpiceBasicInfo *)server,
+                                        (SpiceBasicInfo *)client,
+                                        &error_abort);
         break;
     case SPICE_CHANNEL_EVENT_INITIALIZED:
         if (auth) {
@@ -242,7 +244,9 @@ static void channel_event(int event, SpiceChannelEventInfo 
*info)
         break;
     case SPICE_CHANNEL_EVENT_DISCONNECTED:
         channel_list_del(info);
-        qapi_event_send_spice_disconnected(server->base, client->base, 
&error_abort);
+        qapi_event_send_spice_disconnected((SpiceBasicInfo *)server,
+                                           (SpiceBasicInfo *)client,
+                                           &error_abort);
         break;
     default:
         break;
@@ -378,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 7e2a017..5b5b936 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -262,8 +262,7 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
     VncServerInfo *info;

     info = g_malloc(sizeof(*info));
-    info->base = g_malloc(sizeof(*info->base));
-    vnc_basic_info_get_from_server_addr(vd->lsock, info->base, NULL);
+    vnc_basic_info_get_from_server_addr(vd->lsock, (VncBasicInfo *)info, NULL);
     info->has_auth = true;
     info->auth = g_strdup(vnc_auth_name(vd));
     return info;
@@ -295,8 +294,8 @@ 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_basic_info_get_from_remote_addr(client->csock, client->info->base,
+    vnc_basic_info_get_from_remote_addr(client->csock,
+                                        (VncBasicInfo *)client->info,
                                         &err);
     if (err) {
         qapi_free_VncClientInfo(client->info);
@@ -312,7 +311,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) {
@@ -321,7 +319,8 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)

     switch (event) {
     case QAPI_EVENT_VNC_CONNECTED:
-        qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
+        qapi_event_send_vnc_connected(si, (VncBasicInfo *)vs->info,
+                                      &error_abort);
         break;
     case QAPI_EVENT_VNC_INITIALIZED:
         qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
@@ -356,11 +355,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;

 #ifdef CONFIG_VNC_TLS
     if (client->tls.session && client->tls.dname) {
-- 
2.4.3