[Qemu-devel] [PATCH RFC v4 05/29] 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.

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).

Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer
struct for flat unions".

Signed-off-by: Eric Blake <address@hidden>
---
 hmp.c                              |  6 +++---
 scripts/qapi-types.py              | 29 +++++++++++++++--------------
 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, 65 insertions(+), 75 deletions(-)

diff --git a/hmp.c b/hmp.c
index 849b292..ab8e700 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 99bf00a..457f6a7 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -51,8 +51,19 @@ def gen_struct_field(name, typ, optional):
     return ret


-def gen_struct_fields(members):
+def gen_struct_fields(members, base, nested=False):
     ret = ''
+    if base:
+        if not nested:
+            ret += mcgen('''
+    /* Members inherited from %(c_name)s: */
+''',
+                         c_name=base.c_name())
+        ret += gen_struct_fields(base.local_members, base.base, True)
+        if not nested:
+            ret += mcgen('''
+    /* Own members: */
+''')

     for memb in members:
         ret += gen_struct_field(memb.name, memb.type, memb.optional)
@@ -66,16 +77,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;
 ''')
@@ -126,14 +134,7 @@ struct %(c_name)s {
 ''',
                 c_name=c_name(name))
     if base:
-        ret += mcgen('''
-    /* Members inherited from %(c_name)s: */
-''',
-                     c_name=c_name(base.name))
-        ret += gen_struct_fields(base.members)
-        ret += mcgen('''
-    /* Own members: */
-''')
+        ret += gen_struct_fields([], base)
     else:
         ret += mcgen('''
     %(c_type)s type;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 59b46e9..f5dc257 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -62,12 +62,15 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, 
%(c_type)s **obj, Error *


 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('''

@@ -81,12 +84,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 6a67e73..5181823 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 da84993..b6deee3 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 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 84eab321..45c6ad1 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