[Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate

[Eric Blake]

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; some testsuite coverage is added to avoid future
regressions.

Ultimately, we want to do the same treatment for QAPI unions, but
as that touches a lot more client code, it is better as a separate
patch.  So in the meantime, I had to hack in a way to test if we
are visiting an alternate type, within qapi-types:gen_variants();
the hack is possible because an earlier patch guaranteed that all
alternates have at least two branches, with at most one object
branch; the hack will go away in a later patch.

The generated code difference to qapi-types.h is relatively small,
made possible by a new c_type(is_member) parameter in qapi.py:

| struct BlockdevRef {
|     QType type;
|     union { /* union tag is @type */
|         void *data;
|-        BlockdevOptions *definition;
|+        BlockdevOptions definition;
|         char *reference;
|     } u;
| };

meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
comparable to visit_type_implicit_Foo():

|+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char 
*name, BlockdevOptions *obj, Error **errp)
|+{
|+    Error *err = NULL;
|+
|+    visit_start_struct(v, name, NULL, 0, &err);
|+    if (err) {
|+        goto out;
|+    }
|+    visit_type_BlockdevOptions_fields(v, obj, &err);
|+    error_propagate(errp, err);
|+    err = NULL;
|+    visit_end_struct(v, &err);
|+out:
|+    error_propagate(errp, err);
|+}

and use it like this:

|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|-        visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
|+        visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, 
&err);
|         break;
|     case QTYPE_QSTRING:
|         visit_type_str(v, name, &(*obj)->u.reference, &err);

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

---
v10: new patch
---
 scripts/qapi-types.py                   | 10 ++++++-
 scripts/qapi-visit.py                   | 49 +++++++++++++++++++++++++++++----
 scripts/qapi.py                         | 10 ++++---
 tests/test-qmp-input-visitor.c          | 29 ++++++++++++++++++-
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 6 files changed, 91 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2f23432..aba2847 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.
+    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
@@ -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_member=inline),
                      c_name=c_name(var.name))

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

-# visit_type_FOO_implicit() is emitted as needed; track if it has already
+# 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()
+
 # visit_type_FOO_fields() is always emitted; track if a forward declaration
 # or implementation has already been output.
 struct_fields_seen = set()
@@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
%(c_type)s **obj, Error *
     return ret


+def gen_visit_alternate_struct(typ):
+    if typ in alternate_structs_seen:
+        return ''
+    alternate_structs_seen.add(typ)
+
+    ret = gen_visit_fields_decl(typ)
+
+    ret += mcgen('''
+
+static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, 
%(c_type)s *obj, Error **errp)
+{
+    Error *err = NULL;
+
+    visit_start_struct(v, name, NULL, 0, &err);
+    if (err) {
+        goto out;
+    }
+    visit_type_%(c_type)s_fields(v, obj, &err);
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, &err);
+out:
+    error_propagate(errp, err);
+}
+''',
+                 c_type=typ.c_name())
+    return ret
+
+
 def gen_visit_struct_fields(name, base, members):
     ret = ''

@@ -158,11 +191,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_alternate_struct(var.type)

-    ret = mcgen('''
+    ret += mcgen('''

 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
Error **errp)
 {
@@ -178,15 +214,18 @@ 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:
+        prefix = 'visit_type_'
+        if isinstance(var.type, QAPISchemaObjectType):
+            prefix += 'alternate_'
         ret += mcgen('''
     case %(case)s:
-        visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
+        %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
         break;
 ''',
-                     case=var.type.alternate_qtype(),
+                     prefix=prefix, case=var.type.alternate_qtype(),
                      c_type=var.type.c_name(),
                      c_name=c_name(var.name))

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4d75d75..dbb58da 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -821,7 +821,7 @@ class QAPISchemaVisitor(object):


 class QAPISchemaType(QAPISchemaEntity):
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_member=False):
         return c_name(self.name) + pointer_suffix

     def c_null(self):
@@ -854,7 +854,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_member=False):
         if is_param and self.name == 'str':
             return 'const ' + self._c_type_name
         return self._c_type_name
@@ -888,7 +888,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_member=False):
         return c_name(self.name)

     def member_names(self):
@@ -984,8 +984,10 @@ 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_member=False):
         assert not self.is_implicit()
+        if is_member:
+            return c_name(self.name)
         return QAPISchemaType.c_type(self)

     def json_type(self):
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c72cdad..139ff7d 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -1,7 +1,7 @@
 /*
  * QMP Input Visitor unit-tests.
  *
- * Copyright (C) 2011, 2015 Red Hat Inc.
+ * Copyright (C) 2011-2016 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino <address@hidden>
@@ -309,6 +309,7 @@ static void test_visitor_in_alternate(TestInputVisitorData 
*data,
     Visitor *v;
     Error *err = NULL;
     UserDefAlternate *tmp;
+    WrapAlternate *wrap;

     v = visitor_input_test_init(data, "42");
     visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
@@ -322,10 +323,36 @@ static void 
test_visitor_in_alternate(TestInputVisitorData *data,
     g_assert_cmpstr(tmp->u.s, ==, "string");
     qapi_free_UserDefAlternate(tmp);

+    v = visitor_input_test_init(data, "{'boolean':true}");
+    visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
+    g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
+    g_assert_cmpint(tmp->u.uda.boolean, ==, true);
+    g_assert_cmpint(tmp->u.uda.has_a_b, ==, false);
+    qapi_free_UserDefAlternate(tmp);
+
     v = visitor_input_test_init(data, "false");
     visit_type_UserDefAlternate(v, NULL, &tmp, &err);
     error_free_or_abort(&err);
     qapi_free_UserDefAlternate(tmp);
+
+    v = visitor_input_test_init(data, "{ 'alt': 42 }");
+    visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+    g_assert_cmpint(wrap->alt->type, ==, QTYPE_QINT);
+    g_assert_cmpint(wrap->alt->u.i, ==, 42);
+    qapi_free_WrapAlternate(wrap);
+
+    v = visitor_input_test_init(data, "{ 'alt': 'string' }");
+    visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+    g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING);
+    g_assert_cmpstr(wrap->alt->u.s, ==, "string");
+    qapi_free_WrapAlternate(wrap);
+
+    v = visitor_input_test_init(data, "{ 'alt': {'boolean':true} }");
+    visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+    g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
+    g_assert_cmpint(wrap->alt->u.uda.boolean, ==, true);
+    g_assert_cmpint(wrap->alt->u.uda.has_a_b, ==, false);
+    qapi_free_WrapAlternate(wrap);
 }

 static void test_visitor_in_alternate_number(TestInputVisitorData *data,
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..b393572 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -82,6 +82,8 @@
             'value2' : 'UserDefB',
             'value3' : 'UserDefA' } }

+{ 'struct': 'WrapAlternate',
+  'data': { 'alt': 'UserDefAlternate' } }
 { 'alternate': 'UserDefAlternate',
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }

diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index 2c546b7..11cdc9a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -169,6 +169,8 @@ object UserDefUnionBase
     member enum1: EnumOne optional=False
 object UserDefZero
     member integer: int optional=False
+object WrapAlternate
+    member alt: UserDefAlternate optional=False
 event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
 alternate __org.qemu_x-Alt
     case __org.qemu_x-branch: str
-- 
2.5.0