diff mbox

[v10,11/13] qapi: Don't box branches of flat unions

Message ID 1455582057-27565-12-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Feb. 16, 2016, 12:20 a.m. UTC 
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.  It also lets us get rid of all traces
of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
not all) special cases of simplie unions.

Unfortunately, simple unions are not as easy to convert; 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.  Hence,
gen_visit_fields_decl() has to special case implicit types (the
type for a simple union variant).

Note that after this patch, the only remaining use of
visit_start_implicit_struct() is for alternate types; the next
couple of patches will do further cleanups based on that fact.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v10: new patch

If anything, we could match our simple union wire format more closely
by teaching qapi-types to expose implicit types inline, and write:

struct SU {
    SUKind type;
    union {
        struct {
	    Branch1 *data;
	} branch1;
	struct {
	    Branch2 *data;
	} branch2;
    } u;
};

where we would then access su.u.branch1.data->member instead of
the current su.u.branch1->member.
---
 scripts/qapi-types.py           | 10 +--------
 scripts/qapi-visit.py           | 45 +++++++----------------------------------
 cpus.c                          | 18 ++++++-----------
 hmp.c                           | 12 +++++------
 tests/test-qmp-input-visitor.c  |  2 +-
 tests/test-qmp-output-visitor.c |  5 ++---
 6 files changed, 23 insertions(+), 69 deletions(-)
diff mbox

Patch

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index aba2847..5071817 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.
-    inline = False
-    for v in variants.variants:
-        if not isinstance(v.type, QAPISchemaObjectType):
-            inline = 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
@@ -144,7 +136,7 @@  def gen_variants(variants):
         ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
-                     c_type=typ.c_type(is_member=inline),
+                     c_type=typ.c_type(is_member=not var.simple_union_type()),
                      c_name=c_name(var.name))

     ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 948bde4..68354d8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@ 
 from qapi import *
 import re

-# visit_type_implicit_FOO() is emitted as needed; track if it has already
-# been output.
-implicit_structs_seen = set()
-
 # visit_type_alternate_FOO() is emitted as needed; track if it has already
 # been output.
 alternate_structs_seen = set()
@@ -39,40 +35,15 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **


 def gen_visit_fields_decl(typ):
-    ret = ''
-    if typ.name not in struct_fields_seen:
-        ret += mcgen('''
+    if typ.is_implicit() or typ.name in struct_fields_seen:
+        return ''
+    struct_fields_seen.add(typ.name)
+
+    return mcgen('''

 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp);
 ''',
-                     c_type=typ.c_name())
-        struct_fields_seen.add(typ.name)
-    return ret
-
-
-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_alternate_struct(typ):
@@ -250,9 +221,7 @@  def gen_visit_object(name, base, members, variants):

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

     # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
     # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
@@ -300,7 +269,7 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
                              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/cpus.c b/cpus.c
index 898426c..9592163 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 c6419da..1c16ed9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -313,22 +313,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/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 139ff7d..27e5ab9 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);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 965f298..c5514a1 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 <lcapitulino@redhat.com>
@@ -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);