From 9d3524b39e1fe5f3bb7a990ad7841e469e954a3b Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Tue, 16 Feb 2016 16:39:25 -0700
Subject: qapi-visit: Honor prefix of discriminator enum

When we added support for a user-specified prefix for an enum
type (commit 351d36e), we forgot to teach the qapi-visit code
to honor that prefix in the case of using a prefixed enum as
the discriminator for a flat union.  While there is still some
on-list debate on whether we want to keep prefixes, we should
at least make it work as long as it is still part of the code
base.

Reported-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455665965-27638-1-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
 tests/qapi-schema/qapi-schema-test.out  | 7 +++++--
 2 files changed, 11 insertions(+), 5 deletions(-)

(limited to 'tests')

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4b895275c7..353a34eee4 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -73,14 +73,17 @@
   'base': 'UserDefZero',
   'data': { 'string': 'str', 'enum1': 'EnumOne' } }
 
+{ 'struct': 'UserDefUnionBase2',
+  'base': 'UserDefZero',
+  'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
+
 # this variant of UserDefFlatUnion defaults to a union that uses fields with
 # allocated types to test corner cases in the cleanup/dealloc visitor
 { 'union': 'UserDefFlatUnion2',
-  'base': 'UserDefUnionBase',
+  'base': 'UserDefUnionBase2',
   'discriminator': 'enum1',
   'data': { 'value1' : 'UserDefC', # intentional forward reference
-            'value2' : 'UserDefB',
-            'value3' : 'UserDefA' } }
+            'value2' : 'UserDefB' } }
 
 { '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 2c546b708a..241aadbca0 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -121,11 +121,10 @@ object UserDefFlatUnion
     case value2: UserDefB
     case value3: UserDefB
 object UserDefFlatUnion2
-    base UserDefUnionBase
+    base UserDefUnionBase2
     tag enum1
     case value1: UserDefC
     case value2: UserDefB
-    case value3: UserDefA
 object UserDefNativeListUnion
     member type: UserDefNativeListUnionKind optional=False
     case integer: :obj-intList-wrapper
@@ -167,6 +166,10 @@ object UserDefUnionBase
     base UserDefZero
     member string: str optional=False
     member enum1: EnumOne optional=False
+object UserDefUnionBase2
+    base UserDefZero
+    member string: str optional=False
+    member enum1: QEnumTwo optional=False
 object UserDefZero
     member integer: int optional=False
 event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
-- 
cgit v1.2.3-55-g7522


From 02a57ae32b08e8981b59979b80e682c9a153e94d Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Wed, 17 Feb 2016 23:48:16 -0700
Subject: qapi: Forbid empty unions and useless alternates

Empty unions serve no purpose, and while we compile with gcc
which permits them, strict C99 forbids them.  We happen to inject
a dummy 'void *data' member into the C unions that represent QAPI
unions and alternates, but we want to get rid of that member (it
pollutes the namespace for no good reason), which would leave us
with an empty union if the user didn't provide any branches.  While
empty structs make sense in QAPI, empty unions don't add any
expressiveness to the QMP language.  So prohibit them at parse
time.  Update the documentation and testsuite to match.

Note that the documentation already mentioned that alternates
should have "two or more JSON data types"; so this also fixes
the code to enforce that.  However, we have existing uses of a
union type with only one branch, so the 2-or-more strictness
is intentionally limited to alternates.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-3-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt                  | 15 ++++++++-------
 scripts/qapi.py                         | 12 ++++++++++--
 tests/qapi-schema/alternate-empty.err   |  1 +
 tests/qapi-schema/alternate-empty.exit  |  2 +-
 tests/qapi-schema/alternate-empty.json  |  2 +-
 tests/qapi-schema/alternate-empty.out   |  5 -----
 tests/qapi-schema/flat-union-empty.err  |  1 +
 tests/qapi-schema/flat-union-empty.exit |  2 +-
 tests/qapi-schema/flat-union-empty.json |  2 +-
 tests/qapi-schema/flat-union-empty.out  |  9 ---------
 tests/qapi-schema/union-empty.err       |  1 +
 tests/qapi-schema/union-empty.exit      |  2 +-
 tests/qapi-schema/union-empty.json      |  2 +-
 tests/qapi-schema/union-empty.out       |  6 ------
 14 files changed, 27 insertions(+), 35 deletions(-)

(limited to 'tests')

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 128f074a2d..999f3b98f0 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -187,11 +187,11 @@ prevent incomplete include files.
 
 Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME }
 
-A struct is a dictionary containing a single 'data' key whose
-value is a dictionary.  This corresponds to a struct in C or an Object
-in JSON. Each value of the 'data' dictionary must be the name of a
-type, or a one-element array containing a type name.  An example of a
-struct is:
+A struct is a dictionary containing a single 'data' key whose value is
+a dictionary; the dictionary may be empty.  This corresponds to a
+struct in C or an Object in JSON. Each value of the 'data' dictionary
+must be the name of a type, or a one-element array containing a type
+name.  An example of a struct is:
 
  { 'struct': 'MyType',
    'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } }
@@ -288,9 +288,10 @@ or:    { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME,
 
 Union types are used to let the user choose between several different
 variants for an object.  There are two flavors: simple (no
-discriminator or base), flat (both discriminator and base).  A union
+discriminator or base), and flat (both discriminator and base).  A union
 type is defined using a data dictionary as explained in the following
-paragraphs.
+paragraphs.  The data dictionary for either type of union must not
+be empty.
 
 A simple union type defines a mapping from automatic discriminator
 values to data types like in this example:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f40dc9e777..f97236f509 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -590,7 +590,10 @@ def check_union(expr, expr_info):
                                 "Discriminator '%s' must be of enumeration "
                                 "type" % discriminator)
 
-    # Check every branch
+    # Check every branch; don't allow an empty union
+    if len(members) == 0:
+        raise QAPIExprError(expr_info,
+                            "Union '%s' cannot have empty 'data'" % name)
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)
 
@@ -613,7 +616,11 @@ def check_alternate(expr, expr_info):
     members = expr['data']
     types_seen = {}
 
-    # Check every branch
+    # Check every branch; require at least two branches
+    if len(members) < 2:
+        raise QAPIExprError(expr_info,
+                            "Alternate '%s' should have at least two branches "
+                            "in 'data'" % name)
     for (key, value) in members.items():
         check_name(expr_info, "Member of alternate '%s'" % name, key)
 
@@ -1059,6 +1066,7 @@ class QAPISchemaObjectTypeVariants(object):
         assert bool(tag_member) != bool(tag_name)
         assert (isinstance(tag_name, str) or
                 isinstance(tag_member, QAPISchemaObjectTypeMember))
+        assert len(variants) > 0
         for v in variants:
             assert isinstance(v, QAPISchemaObjectTypeVariant)
         self.tag_name = tag_name
diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err
index e69de29bb2..bb06c5bfec 100644
--- a/tests/qapi-schema/alternate-empty.err
+++ b/tests/qapi-schema/alternate-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data'
diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/alternate-empty.exit
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json
index db3820f841..fff15baf16 100644
--- a/tests/qapi-schema/alternate-empty.json
+++ b/tests/qapi-schema/alternate-empty.json
@@ -1,2 +1,2 @@
-# FIXME - alternates should list at least two types to be useful
+# alternates must list at least two types to be useful
 { 'alternate': 'Alt', 'data': { 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index f78f174111..e69de29bb2 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -1,5 +0,0 @@
-object :empty
-alternate Alt
-    case i: int
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
-    prefix QTYPE
diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err
index e69de29bb2..15754f54eb 100644
--- a/tests/qapi-schema/flat-union-empty.err
+++ b/tests/qapi-schema/flat-union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-empty.json:4: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/flat-union-empty.exit
+++ b/tests/qapi-schema/flat-union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json
index 67dd2978eb..77f1d9abfb 100644
--- a/tests/qapi-schema/flat-union-empty.json
+++ b/tests/qapi-schema/flat-union-empty.json
@@ -1,4 +1,4 @@
-# FIXME - flat unions should not be empty
+# flat unions cannot be empty
 { 'enum': 'Empty', 'data': [ ] }
 { 'struct': 'Base', 'data': { 'type': 'Empty' } }
 { 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
index eade2d5ac6..e69de29bb2 100644
--- a/tests/qapi-schema/flat-union-empty.out
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -1,9 +0,0 @@
-object :empty
-object Base
-    member type: Empty optional=False
-enum Empty []
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
-    prefix QTYPE
-object Union
-    base Base
-    tag type
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
index e69de29bb2..12c20221bd 100644
--- a/tests/qapi-schema/union-empty.err
+++ b/tests/qapi-schema/union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-empty.json:2: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/union-empty.exit
+++ b/tests/qapi-schema/union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json
index 1785007113..1f0b13ca21 100644
--- a/tests/qapi-schema/union-empty.json
+++ b/tests/qapi-schema/union-empty.json
@@ -1,2 +1,2 @@
-# FIXME - unions should not be empty
+# unions cannot be empty
 { 'union': 'Union', 'data': { } }
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
index bdf17e5b29..e69de29bb2 100644
--- a/tests/qapi-schema/union-empty.out
+++ b/tests/qapi-schema/union-empty.out
@@ -1,6 +0,0 @@
-object :empty
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
-    prefix QTYPE
-object Union
-    member type: UnionKind optional=False
-enum UnionKind []
-- 
cgit v1.2.3-55-g7522


From 46534309e667fd860720f983c2c9aefe0354340d Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Wed, 17 Feb 2016 23:48:17 -0700
Subject: qapi: Forbid 'any' inside an alternate

The whole point of an alternate is to allow some type-safety while
still accepting more than one JSON type.  Meanwhile, the 'any'
type exists to bypass type-safety altogether.  The two are
incompatible: you can't accept every type, and still tell which
branch of the alternate to use for the parse; fix this to give a
sane error instead of a Python stack trace.

Note that other types that can't be alternate members are caught
earlier, by check_type().

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-4-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                      | 5 ++++-
 tests/Makefile                       | 1 +
 tests/qapi-schema/alternate-any.err  | 1 +
 tests/qapi-schema/alternate-any.exit | 1 +
 tests/qapi-schema/alternate-any.json | 4 ++++
 tests/qapi-schema/alternate-any.out  | 0
 6 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/alternate-any.err
 create mode 100644 tests/qapi-schema/alternate-any.exit
 create mode 100644 tests/qapi-schema/alternate-any.json
 create mode 100644 tests/qapi-schema/alternate-any.out

(limited to 'tests')

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f97236f509..17bf633e39 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -629,7 +629,10 @@ def check_alternate(expr, expr_info):
                    value,
                    allow_metas=['built-in', 'union', 'struct', 'enum'])
         qtype = find_alternate_member_qtype(value)
-        assert qtype
+        if not qtype:
+            raise QAPIExprError(expr_info,
+                                "Alternate '%s' member '%s' cannot use "
+                                "type '%s'" % (name, key, value))
         if qtype in types_seen:
             raise QAPIExprError(expr_info,
                                 "Alternate '%s' member '%s' can't "
diff --git a/tests/Makefile b/tests/Makefile
index c1c605fc63..7c66d1636e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 
+qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json
 qapi-schema += alternate-base.json
 qapi-schema += alternate-clash.json
diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err
new file mode 100644
index 0000000000..aaa0154731
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any'
diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json
new file mode 100644
index 0000000000..e47a73a116
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.json
@@ -0,0 +1,4 @@
+# we do not allow the 'any' type as an alternate branch
+{ 'alternate': 'Alt',
+  'data': { 'one': 'any',
+            'two': 'int' } }
diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out
new file mode 100644
index 0000000000..e69de29bb2
-- 
cgit v1.2.3-55-g7522


From 68d078395d8233ca8455a95f05e5c23e367777c2 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Wed, 17 Feb 2016 23:48:18 -0700
Subject: qapi: Add tests of complex objects within alternate

Upcoming patches will adjust how we visit an object branch of an
alternate; but we were completely lacking testsuite coverage.
Rectify this, so that the future patches will be able to highlight
the changes and still prove that we avoided regressions.

In particular, the use of a flat union UserDefFlatUnion rather
than a simple struct UserDefA as the branch will give us coverage
of an object with variants.  And visiting an alternate as both
the top level and as a nested member gives confidence in correct
memory allocation handling, especially if the test is run under
valgrind.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  4 +++-
 tests/qapi-schema/qapi-schema-test.out  |  4 +++-
 tests/test-qmp-input-visitor.c          | 37 ++++++++++++++++++++++++++++++++-
 tests/test-qmp-output-visitor.c         | 26 ++++++++++++++++++++++-
 4 files changed, 67 insertions(+), 4 deletions(-)

(limited to 'tests')

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 353a34eee4..632964a6ec 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -85,8 +85,10 @@
   'data': { 'value1' : 'UserDefC', # intentional forward reference
             'value2' : 'UserDefB' } }
 
+{ 'struct': 'WrapAlternate',
+  'data': { 'alt': 'UserDefAlternate' } }
 { 'alternate': 'UserDefAlternate',
-  'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
+  'data': { 'udfu': 'UserDefFlatUnion', 's': 'str', 'i': 'int' } }
 
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 241aadbca0..f5e2a73c30 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -105,7 +105,7 @@ object UserDefA
     member boolean: bool optional=False
     member a_b: int optional=True
 alternate UserDefAlternate
-    case uda: UserDefA
+    case udfu: UserDefFlatUnion
     case s: str
     case i: int
 object UserDefB
@@ -172,6 +172,8 @@ object UserDefUnionBase2
     member enum1: QEnumTwo 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
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c72cdad563..ef836d5fbe 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 <lcapitulino@redhat.com>
@@ -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,44 @@ 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, "{'integer':1, 'string':'str', "
+                                "'enum1':'value1', 'boolean':true}");
+    visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
+    g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
+    g_assert_cmpint(tmp->u.udfu->integer, ==, 1);
+    g_assert_cmpstr(tmp->u.udfu->string, ==, "str");
+    g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true);
+    g_assert_cmpint(tmp->u.udfu->u.value1->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': {'integer':1, 'string':'str', "
+                                "'enum1':'value1', 'boolean':true} }");
+    visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+    g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
+    g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1);
+    g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str");
+    g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true);
+    g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false);
+    qapi_free_WrapAlternate(wrap);
 }
 
 static void test_visitor_in_alternate_number(TestInputVisitorData *data,
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 965f298e11..2b0f7e9c53 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>
@@ -427,6 +427,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
 {
     QObject *arg;
     UserDefAlternate *tmp;
+    QDict *qdict;
 
     tmp = g_new0(UserDefAlternate, 1);
     tmp->type = QTYPE_QINT;
@@ -453,6 +454,29 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
 
     qapi_free_UserDefAlternate(tmp);
     qobject_decref(arg);
+
+    tmp = g_new0(UserDefAlternate, 1);
+    tmp->type = QTYPE_QDICT;
+    tmp->u.udfu = g_new0(UserDefFlatUnion, 1);
+    tmp->u.udfu->integer = 1;
+    tmp->u.udfu->string = g_strdup("str");
+    tmp->u.udfu->enum1 = ENUM_ONE_VALUE1;
+    tmp->u.udfu->u.value1 = g_new0(UserDefA, 1);
+    tmp->u.udfu->u.value1->boolean = true;
+
+    visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert_cmpint(qobject_type(arg), ==, QTYPE_QDICT);
+    qdict = qobject_to_qdict(arg);
+    g_assert_cmpint(qdict_size(qdict), ==, 4);
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 1);
+    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
+    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
+    g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
+
+    qapi_free_UserDefAlternate(tmp);
+    qobject_decref(arg);
 }
 
 static void test_visitor_out_empty(TestOutputVisitorData *data,
-- 
cgit v1.2.3-55-g7522


From becceedc4d9bc1435099c90a0514945a89844d3a Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Wed, 17 Feb 2016 23:48:26 -0700
Subject: qapi: Don't box struct branch of alternate

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 outside of the testsuite; an earlier patch in the series
added some testsuite coverage to make the effect of this patch more
obvious.

In qapi.py, c_type() gains a new is_unboxed flag to control when we
are emitting a C struct unboxed within the context of an outer
struct (different from our other two modes of usage with no flags
for normal local variable declarations, and with is_param for adding
'const' in a parameter list).  I don't know if there is any more
pythonic way of collapsing the two flags into a single parameter,
as we never have a caller setting both flags at once.

Ultimately, we want to also unbox branches for QAPI unions, but as
that touches a lot more client code, it is better as separate
patches.  But since unions and alternates share gen_variants(), I
had to hack in a way to test if we are visiting an alternate type
for setting the is_unboxed flag: look for a non-object branch.
This works because alternates have at least two branches, with at
most one object branch, while unions have only object branches.
The hack will go away in a later patch.

The generated code difference to qapi-types.h is relatively small:

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

The corresponding spot in qapi-visit.c calls visit_type_FOO(), which
first calls visit_start_struct() to allocate or deallocate the member
and handle a layer of {} from the JSON stream, then visits the
members.  To peel off the indirection and the memory management that
comes with it, we inline this call, then suppress allocation /
deallocation by passing NULL to visit_start_struct(), and adjust the
member visit:

|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|-        visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
|+        visit_start_struct(v, name, NULL, 0, &err);
|+        if (err) {
|+            break;
|+        }
|+        visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err);
|+        error_propagate(errp, err);
|+        err = NULL;
|+        visit_end_struct(v, &err);
|         break;
|     case QTYPE_QSTRING:
|         visit_type_str(v, name, &(*obj)->u.reference, &err);

The visit of non-object fields is unchanged.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py           | 10 +++++++++-
 scripts/qapi-visit.py           | 33 +++++++++++++++++++++++++++------
 scripts/qapi.py                 | 12 +++++++-----
 tests/test-qmp-input-visitor.c  | 20 ++++++++++----------
 tests/test-qmp-output-visitor.c | 11 +++++------
 5 files changed, 58 insertions(+), 28 deletions(-)

(limited to 'tests')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 6ea0ae6fb7..4dabe91c92 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.
+    unboxed = False
+    for v in variants.variants:
+        if not isinstance(v.type, QAPISchemaObjectType):
+            unboxed = 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_unboxed=unboxed),
                      c_name=c_name(var.name))
 
     ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d7d7ed5a32..f4e38d1068 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -201,11 +201,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_fields_decl(var.type)
 
-    ret = mcgen('''
+    ret += mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
 {
@@ -221,17 +224,35 @@ 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:
         ret += mcgen('''
     case %(case)s:
+''',
+                     case=var.type.alternate_qtype())
+        if isinstance(var.type, QAPISchemaObjectType):
+            ret += mcgen('''
+        visit_start_struct(v, name, NULL, 0, &err);
+        if (err) {
+            break;
+        }
+        visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err);
+        error_propagate(errp, err);
+        err = NULL;
+        visit_end_struct(v, &err);
+''',
+                         c_type=var.type.c_name(),
+                         c_name=c_name(var.name))
+        else:
+            ret += mcgen('''
         visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
-        break;
 ''',
-                     case=var.type.alternate_qtype(),
-                     c_type=var.type.c_name(),
-                     c_name=c_name(var.name))
+                         c_type=var.type.c_name(),
+                         c_name=c_name(var.name))
+        ret += mcgen('''
+        break;
+''')
 
     ret += mcgen('''
     default:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 17bf633e39..8497777d94 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -824,7 +824,7 @@ class QAPISchemaVisitor(object):
 
 
 class QAPISchemaType(QAPISchemaEntity):
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_unboxed=False):
         return c_name(self.name) + pointer_suffix
 
     def c_null(self):
@@ -857,7 +857,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_unboxed=False):
         if is_param and self.name == 'str':
             return 'const ' + self._c_type_name
         return self._c_type_name
@@ -891,7 +891,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_unboxed=False):
         return c_name(self.name)
 
     def member_names(self):
@@ -987,9 +987,11 @@ 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_unboxed=False):
         assert not self.is_implicit()
-        return QAPISchemaType.c_type(self)
+        if is_unboxed:
+            return c_name(self.name)
+        return c_name(self.name) + pointer_suffix
 
     def json_type(self):
         return 'object'
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index ef836d5fbe..47cf6aaff0 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -327,11 +327,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
                                 "'enum1':'value1', 'boolean':true}");
     visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
     g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
-    g_assert_cmpint(tmp->u.udfu->integer, ==, 1);
-    g_assert_cmpstr(tmp->u.udfu->string, ==, "str");
-    g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
-    g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true);
-    g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false);
+    g_assert_cmpint(tmp->u.udfu.integer, ==, 1);
+    g_assert_cmpstr(tmp->u.udfu.string, ==, "str");
+    g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true);
+    g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false);
     qapi_free_UserDefAlternate(tmp);
 
     v = visitor_input_test_init(data, "false");
@@ -355,11 +355,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
                                 "'enum1':'value1', 'boolean':true} }");
     visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
     g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
-    g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1);
-    g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str");
-    g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
-    g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true);
-    g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false);
+    g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1);
+    g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str");
+    g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true);
+    g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false);
     qapi_free_WrapAlternate(wrap);
 }
 
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 2b0f7e9c53..fe2f1a1326 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -457,12 +457,11 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
 
     tmp = g_new0(UserDefAlternate, 1);
     tmp->type = QTYPE_QDICT;
-    tmp->u.udfu = g_new0(UserDefFlatUnion, 1);
-    tmp->u.udfu->integer = 1;
-    tmp->u.udfu->string = g_strdup("str");
-    tmp->u.udfu->enum1 = ENUM_ONE_VALUE1;
-    tmp->u.udfu->u.value1 = g_new0(UserDefA, 1);
-    tmp->u.udfu->u.value1->boolean = true;
+    tmp->u.udfu.integer = 1;
+    tmp->u.udfu.string = g_strdup("str");
+    tmp->u.udfu.enum1 = ENUM_ONE_VALUE1;
+    tmp->u.udfu.u.value1 = g_new0(UserDefA, 1);
+    tmp->u.udfu.u.value1->boolean = true;
 
     visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
     arg = qmp_output_get_qobject(data->qov);
-- 
cgit v1.2.3-55-g7522


From 544a3731591f5d53e15f22de00ce5ac758d490b3 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Wed, 17 Feb 2016 23:48:27 -0700
Subject: qapi: Don't box branches of flat unions

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.

The change to unboxed structs means that u.data (added in commit
cee2dedb) is now coincident with random fields of each branch of
the flat union, whereas beforehand it was only coincident with
pointers (since all branches of a flat union have to be objects).
Note that this was already the case for simple unions - but there
we got lucky.  Remember, visit_start_union() blindly returns true
for all visitors except for the dealloc visitor, where it returns
the value !!obj->u.data, and that this result then controls
whether to proceed with the visit to the variant.  Pre-patch,
this meant that flat unions were testing whether the boxed pointer
was still NULL, and thereby skipping visit_end_implicit_struct()
and avoiding a NULL dereference if the pointer had not been
allocated.  The same was true for simple unions where the current
branch had pointer type, except there we bypassed visit_type_FOO().
But for simple unions where the current branch had scalar type, the
contents of that scalar meant that the decision to call
visit_type_FOO() was data-dependent - the reason we got lucky there
is that visit_type_FOO() for all scalar types in the dealloc visitor
is a no-op (only the pointer variants had anything to free), so it
did not matter whether the dealloc visit was skipped.  But with this
patch, we would risk leaking memory if we could skip a call to
visit_type_FOO_fields() based solely on a data-dependent decision.

But notice: in the dealloc visitor, visit_type_FOO() already handles
a NULL obj - it was only the visit_type_implicit_FOO() that was
failing to check for NULL. And now that we have refactored things to
have the branch be part of the parent struct, we no longer have a
separate pointer that can be NULL in the first place.  So we can just
delete the call to visit_start_union() altogether, and blindly visit
the branch type; there is no change in behavior except to the dealloc
visitor, where we now unconditionally visit the branch, but where that
visit is now always safe (for a flat union, we can no longer
dereference NULL, and for a simple union, visit_type_FOO() was already
safely handling NULL on pointer types).

Unfortunately, simple unions are not as easy to switch to unboxed
layout; 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; although there
are some cleanups planned for simple unions in later patches.

visit_start_union() and gen_visit_implicit_struct() are now unused.
Drop them.

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

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-14-git-send-email-eblake@redhat.com>
[Dead code deletion squashed in, commit message updated accordingly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cpus.c                          | 18 ++++++------------
 hmp.c                           | 12 ++++++------
 include/qapi/visitor-impl.h     |  2 --
 include/qapi/visitor.h          |  1 -
 qapi/qapi-dealloc-visitor.c     | 26 --------------------------
 qapi/qapi-visit-core.c          |  8 --------
 scripts/qapi-types.py           | 13 +++----------
 scripts/qapi-visit.py           | 36 ++----------------------------------
 tests/test-qmp-input-visitor.c  | 10 +++++-----
 tests/test-qmp-output-visitor.c |  6 ++----
 10 files changed, 24 insertions(+), 108 deletions(-)

(limited to 'tests')

diff --git a/cpus.c b/cpus.c
index 898426ca56..9592163ff4 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 996cb91027..bfbd667033 100644
--- a/hmp.c
+++ b/hmp.c
@@ -314,22 +314,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/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7905a2845e..c4af3e042f 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -58,8 +58,6 @@ struct Visitor
 
     /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, const char *name, bool *present);
-
-    bool (*start_union)(Visitor *v, bool data_present, Error **errp);
 };
 
 void input_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 8a2d5ccacc..a6678b16ef 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -79,6 +79,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);
 
 #endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 6667e8ca43..4eae555ace 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj,
 {
 }
 
-/* If there's no data present, the dealloc visitor has nothing to free.
- * Thus, indicate to visitor code that the subsequent union fields can
- * be skipped. This is not an error condition, since the cleanup of the
- * rest of an object can continue unhindered, so leave errp unset in
- * these cases.
- *
- * NOTE: In cases where we're attempting to deallocate an object that
- * may have missing fields, the field indicating the union type may
- * be missing. In such a case, it's possible we don't have enough
- * information to differentiate data_present == false from a case where
- * data *is* present but happens to be a scalar with a value of 0.
- * This is okay, since in the case of the dealloc visitor there's no
- * work that needs to done in either situation.
- *
- * The current inability in QAPI code to more thoroughly verify a union
- * type in such cases will likely need to be addressed if we wish to
- * implement this interface for other types of visitors in the future,
- * however.
- */
-static bool qapi_dealloc_start_union(Visitor *v, bool data_present,
-                                     Error **errp)
-{
-    return data_present;
-}
-
 Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
 {
     return &v->visitor;
@@ -224,7 +199,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_str = qapi_dealloc_type_str;
     v->visitor.type_number = qapi_dealloc_type_number;
     v->visitor.type_any = qapi_dealloc_type_anything;
-    v->visitor.start_union = qapi_dealloc_start_union;
 
     QTAILQ_INIT(&v->stack);
 
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index b4a0f21b36..f7b99807b0 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -61,14 +61,6 @@ void visit_end_list(Visitor *v)
     v->end_list(v);
 }
 
-bool visit_start_union(Visitor *v, bool data_present, Error **errp)
-{
-    if (v->start_union) {
-        return v->start_union(v, data_present, errp);
-    }
-    return true;
-}
-
 bool visit_optional(Visitor *v, const char *name, bool *present)
 {
     if (v->optional) {
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4dabe91c92..eac90d2fe9 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.
-    unboxed = False
-    for v in variants.variants:
-        if not isinstance(v.type, QAPISchemaObjectType):
-            unboxed = 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
@@ -140,11 +132,12 @@ def gen_variants(variants):
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
-        typ = var.simple_union_type() or var.type
+        simple_union_type = var.simple_union_type()
+        typ = simple_union_type or var.type
         ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
-                     c_type=typ.c_type(is_unboxed=unboxed),
+                     c_type=typ.c_type(is_unboxed=not simple_union_type),
                      c_name=c_name(var.name))
 
     ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f4e38d1068..3a3918f952 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
 from qapi import *
 import re
 
-# visit_type_FOO_implicit() is emitted as needed; track if it has already
-# been output.
-implicit_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()
@@ -45,31 +41,6 @@ static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **er
                  c_type=typ.c_name())
 
 
-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_struct_fields(name, base, members, variants):
     ret = ''
 
@@ -79,7 +50,7 @@ def gen_visit_struct_fields(name, base, members, 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)
 
     struct_fields_seen.add(name)
     ret += mcgen('''
@@ -102,9 +73,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
 
     if variants:
         ret += mcgen('''
-    if (!visit_start_union(v, !!obj->u.data, &err) || err) {
-        goto out;
-    }
     switch (obj->%(c_name)s) {
 ''',
                      c_name=c_name(variants.tag_member.name))
@@ -126,7 +94,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
                              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/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 47cf6aaff0..b05da5baa1 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);
@@ -330,8 +330,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
     g_assert_cmpint(tmp->u.udfu.integer, ==, 1);
     g_assert_cmpstr(tmp->u.udfu.string, ==, "str");
     g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
-    g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true);
-    g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false);
+    g_assert_cmpint(tmp->u.udfu.u.value1.boolean, ==, true);
+    g_assert_cmpint(tmp->u.udfu.u.value1.has_a_b, ==, false);
     qapi_free_UserDefAlternate(tmp);
 
     v = visitor_input_test_init(data, "false");
@@ -358,8 +358,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
     g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1);
     g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str");
     g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
-    g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true);
-    g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false);
+    g_assert_cmpint(wrap->alt->u.udfu.u.value1.boolean, ==, true);
+    g_assert_cmpint(wrap->alt->u.udfu.u.value1.has_a_b, ==, false);
     qapi_free_WrapAlternate(wrap);
 }
 
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index fe2f1a1326..a7f8b45c77 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -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);
@@ -460,8 +459,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
     tmp->u.udfu.integer = 1;
     tmp->u.udfu.string = g_strdup("str");
     tmp->u.udfu.enum1 = ENUM_ONE_VALUE1;
-    tmp->u.udfu.u.value1 = g_new0(UserDefA, 1);
-    tmp->u.udfu.u.value1->boolean = true;
+    tmp->u.udfu.u.value1.boolean = true;
 
     visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
     arg = qmp_output_get_qobject(data->qov);
-- 
cgit v1.2.3-55-g7522