[Qemu-devel] [PULL 00/15] QAPI patches for 2016-02-19

[Qemu-devel] [PULL 00/15] QAPI patches for 2016-02-19

[Markus Armbruster]

The following changes since commit dd5e38b19d7cb07d317e1285941d8245c01da540:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160218-1' into staging (2016-02-18 15:20:35 +0000)

are available in the git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2016-02-19

for you to fetch changes up to dbf11922622685934bfb41e7cf2be9bd4a0405c0:

  qapi: Change visit_start_implicit_struct to visit_start_alternate (2016-02-19 11:08:57 +0100)

----------------------------------------------------------------
QAPI patches for 2016-02-19

----------------------------------------------------------------
Eric Blake (13):
      qapi-visit: Honor prefix of discriminator enum
      qapi: Simplify excess input reporting in input visitors
      qapi: Forbid empty unions and useless alternates
      qapi: Forbid 'any' inside an alternate
      qapi: Add tests of complex objects within alternate
      qapi: Visit variants in visit_type_FOO_fields()
      qapi-visit: Less indirection in visit_type_Foo_fields()
      qapi: Adjust layout of FooList types
      qapi: Emit structs used as variants in topological order
      qapi-visit: Use common idiom in gen_visit_fields_decl()
      qapi: Don't box struct branch of alternate
      qapi: Don't box branches of flat unions
      qapi: Change visit_start_implicit_struct to visit_start_alternate

Markus Armbruster (2):
      qapi-visit: Simplify how we visit common union members
      qapi-visit: Unify struct and union visit

 cpus.c                                  |  18 +--
 docs/qapi-code-gen.txt                  |  15 +-
 hmp.c                                   |  12 +-
 include/qapi/visitor-impl.h             |  21 ++-
 include/qapi/visitor.h                  |  63 +++++---
 qapi/opts-visitor.c                     |  16 +-
 qapi/qapi-dealloc-visitor.c             |  42 +-----
 qapi/qapi-visit-core.c                  |  45 ++----
 qapi/qmp-input-visitor.c                |  43 +++---
 qapi/qmp-output-visitor.c               |   3 +-
 qapi/string-input-visitor.c             |   4 +-
 qapi/string-output-visitor.c            |   2 +-
 scripts/qapi-types.py                   |  33 +++--
 scripts/qapi-visit.py                   | 253 ++++++++++++--------------------
 scripts/qapi.py                         |  29 +++-
 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
 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/qapi-schema-test.json |  13 +-
 tests/qapi-schema/qapi-schema-test.out  |  11 +-
 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 -
 tests/test-qmp-input-visitor.c          |  39 ++++-
 tests/test-qmp-output-visitor.c         |  27 +++-
 36 files changed, 358 insertions(+), 373 deletions(-)
 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

-- 
2.4.3

[Qemu-devel] [PULL 01/15] qapi-visit: Honor prefix of discriminator enum

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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>
---
 scripts/qapi-visit.py                   | 3 ++-
 tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
 tests/qapi-schema/qapi-schema-test.out  | 7 +++++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0cc9b08..2bdb5a1 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -293,7 +293,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     case %(case)s:
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
-                                       var.name))
+                                       var.name,
+                                       variants.tag_member.type.prefix))
         if simple_union_type:
             ret += mcgen('''
         visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..353a34e 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 2c546b7..241aadb 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
-- 
2.4.3

[Qemu-devel] [PULL 02/15] qapi: Simplify excess input reporting in input visitors

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

When reporting that an unvisited member remains at the end of an
input visit for a struct, we were using g_hash_table_find()
coupled with a callback function that always returns true, to
locate an arbitrary member of the hash table.  But if all we
need is an arbitrary entry, we can get that from a single-use
iterator, without needing a tautological callback function.

Technically, our cast of &(GQueue *) to (void **) is not strict
C (while void * must be able to hold all other pointers, nothing
says a void ** has to be the same width or representation as a
GQueue **).  The kosher way to write it would be the verbose:

    void *tmp;
    GQueue *any;
    if (g_hash_table_iter_next(&iter, NULL, &tmp)) {
        any = tmp;

But our code base (not to mention glib itself) already has other
cases of assuming that ALL pointers have the same width and
representation, where a compiler would have to go out of its way
to mis-compile our borderline behavior.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1455778109-6278-2-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/opts-visitor.c      | 12 +++---------
 qapi/qmp-input-visitor.c | 14 +++++---------
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index d54f75b..ae5b955 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -157,17 +157,11 @@ opts_start_struct(Visitor *v, const char *name, void **obj,
 }
 
 
-static gboolean
-ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
-{
-    return TRUE;
-}
-
-
 static void
 opts_end_struct(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = to_ov(v);
+    GHashTableIter iter;
     GQueue *any;
 
     if (--ov->depth > 0) {
@@ -175,8 +169,8 @@ opts_end_struct(Visitor *v, Error **errp)
     }
 
     /* we should have processed all (distinct) QemuOpt instances */
-    any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
-    if (any) {
+    g_hash_table_iter_init(&iter, ov->unprocessed_opts);
+    if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) {
         const QemuOpt *first;
 
         first = g_queue_peek_head(any);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 362a1a3..2f48b95 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -90,12 +90,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
     qiv->nb_stack++;
 }
 
-/** Only for qmp_input_pop. */
-static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
-{
-    *(const char **)user_pkey = (const char *)key;
-    return TRUE;
-}
 
 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
@@ -104,9 +98,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
     if (qiv->strict) {
         GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
         if (top_ht) {
-            if (g_hash_table_size(top_ht)) {
-                const char *key;
-                g_hash_table_find(top_ht, always_true, &key);
+            GHashTableIter iter;
+            const char *key;
+
+            g_hash_table_iter_init(&iter, top_ht);
+            if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
                 error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
             }
             g_hash_table_unref(top_ht);
-- 
2.4.3

[Qemu-devel] [PULL 03/15] qapi: Forbid empty unions and useless alternates

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 128f074..999f3b9 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 f40dc9e..f97236f 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 e69de29..bb06c5b 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 573541a..d00491f 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 db3820f..fff15ba 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 f78f174..e69de29 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 e69de29..15754f5 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 573541a..d00491f 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 67dd297..77f1d9a 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 eade2d5..e69de29 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 e69de29..12c2022 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 573541a..d00491f 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 1785007..1f0b13c 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 bdf17e5..e69de29 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 []
-- 
2.4.3

[Qemu-devel] [PULL 04/15] qapi: Forbid 'any' inside an alternate

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f97236f..17bf633 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 c1c605f..7c66d16 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 0000000..aaa0154
--- /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 0000000..d00491f
--- /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 0000000..e47a73a
--- /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 0000000..e69de29
-- 
2.4.3

[Qemu-devel] [PULL 05/15] qapi: Add tests of complex objects within alternate

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 353a34e..632964a 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 241aadb..f5e2a73 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 c72cdad..ef836d5 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 965f298..2b0f7e9 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,
-- 
2.4.3

[Qemu-devel] [PULL 06/15] qapi-visit: Simplify how we visit common union members

[Markus Armbruster]

For a simple union SU, gen_visit_union() generates a visit of its
single tag member, like this:

    visit_type_SUKind(v, "type", &(*obj)->type, &err);

For a flat union FU with base B, it generates a visit of its base
fields:

    visit_type_B_fields(v, (B **)obj, &err);

Instead, we can simply visit the common members using the same fields
visit function we use for structs, generated with
gen_visit_struct_fields().  This function visits the base if any, then
the local members.

For a simple union SU, visit_type_SU_fields() contains exactly the old
tag member visit, because there is no base, and the tag member is the
only member.  For instance, the code generated for qapi-schema.json's
KeyValue changes like this:

    +static void visit_type_KeyValue_fields(Visitor *v, KeyValue **obj, Error **errp)
    +{
    +    Error *err = NULL;
    +
    +    visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
    +    if (err) {
    +        goto out;
    +    }
    +
    +out:
    +    error_propagate(errp, err);
    +}
    +
     void visit_type_KeyValue(Visitor *v, const char *name, KeyValue **obj, Error **errp)
     {
         Error *err = NULL;
    @@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor *v, con
         if (!*obj) {
             goto out_obj;
         }
    -    visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
    +    visit_type_KeyValue_fields(v, obj, &err);
         if (err) {
             goto out_obj;
         }

For a flat union FU, visit_type_FU_fields() contains exactly the old
base fields visit, because there is a base, but no members.  For
instance, the code generated for qapi-schema.json's CpuInfo changes
like this:

     static void visit_type_CpuInfoBase_fields(Visitor *v, CpuInfoBase **obj, Error **errp);

    +static void visit_type_CpuInfo_fields(Visitor *v, CpuInfo **obj, Error **errp)
    +{
    +    Error *err = NULL;
    +
    +    visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
    +    if (err) {
    +        goto out;
    +    }
    +
    +out:
    +    error_propagate(errp, err);
    +}
    +
     static void visit_type_CpuInfoX86_fields(Visitor *v, CpuInfoX86 **obj, Error **errp)
...
    @@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor *v, cons
         if (!*obj) {
             goto out_obj;
         }
    -    visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
    +    visit_type_CpuInfo_fields(v, obj, &err);
         if (err) {
             goto out_obj;
         }

As you see, the generated code grows a bit, but in practice, it's lost
in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%.

This simplification became possible with commit 441cbac "qapi-visit:
Convert to QAPISchemaVisitor, fixing bugs".  It's a step towards
unifying gen_struct() and gen_union().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1453902888-20457-2-git-send-email-armbru@redhat.com>
[improve commit message examples]
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-6-git-send-email-eblake@redhat.com>
[Commit message tweaked]
---
 scripts/qapi-visit.py | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2bdb5a1..1051710 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -238,11 +238,8 @@ out:
     return ret
 
 
-def gen_visit_union(name, base, variants):
-    ret = ''
-
-    if base:
-        ret += gen_visit_fields_decl(base)
+def gen_visit_union(name, base, members, variants):
+    ret = gen_visit_struct_fields(name, base, members)
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
@@ -262,21 +259,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     if (!*obj) {
         goto out_obj;
     }
+    visit_type_%(c_name)s_fields(v, obj, &err);
 ''',
                  c_name=c_name(name))
-
-    if base:
-        ret += mcgen('''
-    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
-''',
-                     c_name=base.c_name())
-    else:
-        ret += mcgen('''
-    visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err);
-''',
-                     c_type=variants.tag_member.type.c_name(),
-                     c_name=c_name(variants.tag_member.name),
-                     name=variants.tag_member.name)
     ret += gen_err_check(label='out_obj')
     ret += mcgen('''
     if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
@@ -377,11 +362,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_decl(name)
         if variants:
-            if members:
-                # Members other than variants.tag_member not implemented
-                assert len(members) == 1
-                assert members[0] == variants.tag_member
-            self.defn += gen_visit_union(name, base, variants)
+            self.defn += gen_visit_union(name, base, members, variants)
         else:
             self.defn += gen_visit_struct(name, base, members)
 
-- 
2.4.3

[Qemu-devel] [PULL 07/15] qapi: Visit variants in visit_type_FOO_fields()

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

We initially created the static visit_type_FOO_fields() helper
function for reuse of code - we have cases where the initial
setup for a visit has different allocation (depending on whether
the fields represent a stand-alone type or are embedded as part
of a larger type), but where the actual field visits are
identical once a pointer is available.

Up until the previous patch, visit_type_FOO_fields() was only
used for structs (no variants), so it was covering every field
for each type where it was emitted.

Meanwhile, the code for visiting unions looks like:

static visit_type_U_fields() {
    visit base;
    visit local_members;
}
visit_type_U() {
    visit_start_struct();
    visit_type_U_fields();
    visit variants;
    visit_end_struct();
}

which splits the fields of the union visit across two functions.
Move the code to visit variants to live inside visit_type_U_fields(),
while making it conditional on having variants so that all other
instances of the helper function remain unchanged.  This is also
a step closer towards unifying struct and union visits, and towards
allowing one union type to be the branch of another flat union.

The resulting diff to the generated code is a bit hard to read,
but it can be verified that it touches only union types, and that
the end result is the following general structure:

static visit_type_U_fields() {
    visit base;
    visit local_members;
    visit variants;
}
visit_type_U() {
    visit_start_struct();
    visit_type_U_fields();
    visit_end_struct();
}

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com>
[gen_visit_struct_fields() parameter variants made mandatory]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 106 +++++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1051710..5e91342 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -71,11 +71,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     return ret
 
 
-def gen_visit_struct_fields(name, base, members):
+def gen_visit_struct_fields(name, base, members, variants):
     ret = ''
 
     if base:
         ret += gen_visit_fields_decl(base)
+    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)
 
     struct_fields_seen.add(name)
     ret += mcgen('''
@@ -96,8 +101,49 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 
     ret += gen_visit_fields(members, prefix='(*obj)->')
 
-    # 'goto out' produced for base, and by gen_visit_fields() for each member
-    if base or members:
+    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))
+
+        for var in variants.variants:
+            # TODO ugly special case for simple union
+            simple_union_type = var.simple_union_type()
+            ret += mcgen('''
+    case %(case)s:
+''',
+                         case=c_enum_const(variants.tag_member.type.name,
+                                           var.name,
+                                           variants.tag_member.type.prefix))
+            if simple_union_type:
+                ret += mcgen('''
+        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
+''',
+                             c_type=simple_union_type.c_name(),
+                             c_name=c_name(var.name))
+            else:
+                ret += mcgen('''
+        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
+''',
+                             c_type=var.type.c_name(),
+                             c_name=c_name(var.name))
+            ret += mcgen('''
+        break;
+''')
+
+        ret += mcgen('''
+    default:
+        abort();
+    }
+''')
+
+    # 'goto out' produced for base, by gen_visit_fields() for each member,
+    # and if variants were present
+    if base or members or variants:
         ret += mcgen('''
 
 out:
@@ -110,7 +156,7 @@ out:
 
 
 def gen_visit_struct(name, base, members):
-    ret = gen_visit_struct_fields(name, base, members)
+    ret = gen_visit_struct_fields(name, base, members, None)
 
     # 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
@@ -239,12 +285,7 @@ out:
 
 
 def gen_visit_union(name, base, members, variants):
-    ret = gen_visit_struct_fields(name, base, members)
-
-    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_struct_fields(name, base, members, variants)
 
     ret += mcgen('''
 
@@ -260,54 +301,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         goto out_obj;
     }
     visit_type_%(c_name)s_fields(v, obj, &err);
-''',
-                 c_name=c_name(name))
-    ret += gen_err_check(label='out_obj')
-    ret += mcgen('''
-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
-        goto out_obj;
-    }
-    switch ((*obj)->%(c_name)s) {
-''',
-                 c_name=c_name(variants.tag_member.name))
-
-    for var in variants.variants:
-        # TODO ugly special case for simple union
-        simple_union_type = var.simple_union_type()
-        ret += mcgen('''
-    case %(case)s:
-''',
-                     case=c_enum_const(variants.tag_member.type.name,
-                                       var.name,
-                                       variants.tag_member.type.prefix))
-        if simple_union_type:
-            ret += mcgen('''
-        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
-''',
-                         c_type=simple_union_type.c_name(),
-                         c_name=c_name(var.name))
-        else:
-            ret += mcgen('''
-        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
-''',
-                         c_type=var.type.c_name(),
-                         c_name=c_name(var.name))
-        ret += mcgen('''
-        break;
-''')
-
-    ret += mcgen('''
-    default:
-        abort();
-    }
-out_obj:
     error_propagate(errp, err);
     err = NULL;
+out_obj:
     visit_end_struct(v, &err);
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 c_name=c_name(name))
 
     return ret
 
-- 
2.4.3

[Qemu-devel] [PULL 08/15] qapi-visit: Unify struct and union visit

[Markus Armbruster]

gen_visit_union() is now just like gen_visit_struct().  Rename
it to gen_visit_object(), use it for structs, and drop
gen_visit_struct().  Output is unchanged.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1453902888-20457-4-git-send-email-armbru@redhat.com>
[split out variant handling, rebase to earlier changes]
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-8-git-send-email-eblake@redhat.com>
---
 scripts/qapi-visit.py | 45 ++++++---------------------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 5e91342..4a04354 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -155,40 +155,6 @@ out:
     return ret
 
 
-def gen_visit_struct(name, base, members):
-    ret = gen_visit_struct_fields(name, base, members, None)
-
-    # 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
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-    ret += mcgen('''
-
-void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
-{
-    Error *err = NULL;
-
-    visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
-    if (err) {
-        goto out;
-    }
-    if (!*obj) {
-        goto out_obj;
-    }
-    visit_type_%(c_name)s_fields(v, obj, &err);
-    error_propagate(errp, err);
-    err = NULL;
-out_obj:
-    visit_end_struct(v, &err);
-out:
-    error_propagate(errp, err);
-}
-''',
-                 c_name=c_name(name))
-
-    return ret
-
-
 def gen_visit_list(name, element_type):
     # FIXME: if *obj is NULL on entry, and the first visit_next_list()
     # assigns to *obj, while a later one fails, we should clean up *obj
@@ -284,9 +250,13 @@ out:
     return ret
 
 
-def gen_visit_union(name, base, members, variants):
+def gen_visit_object(name, base, members, variants):
     ret = gen_visit_struct_fields(name, base, members, variants)
 
+    # 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
+    # rather than leaving it non-NULL. As currently written, the caller must
+    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
@@ -363,10 +333,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
 
     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_decl(name)
-        if variants:
-            self.defn += gen_visit_union(name, base, members, variants)
-        else:
-            self.defn += gen_visit_struct(name, base, members)
+        self.defn += gen_visit_object(name, base, members, variants)
 
     def visit_alternate_type(self, name, info, variants):
         self.decl += gen_visit_decl(name)
-- 
2.4.3

[Qemu-devel] [PULL 09/15] qapi-visit: Less indirection in visit_type_Foo_fields()

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

We were passing 'Foo **obj' to the internal helper function, but
all uses within the helper were via reads of '*obj'.  Refactor
things to pass one less level of indirection, by having the
callers dereference before calling.

For an example of the generated code change:

|-static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo **obj, Error **errp)
|+static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo *obj, Error **errp)
| {
|     Error *err = NULL;
|
|-    visit_type_int(v, "actual", &(*obj)->actual, &err);
|+    visit_type_int(v, "actual", &obj->actual, &err);
|     error_propagate(errp, err);
| }
|
|@@ -261,7 +261,7 @@ void visit_type_BalloonInfo(Visitor *v,
|     if (!*obj) {
|         goto out_obj;
|     }
|-    visit_type_BalloonInfo_fields(v, obj, &err);
|+    visit_type_BalloonInfo_fields(v, *obj, &err);
| out_obj:

The refactoring will also make it easier to reuse the helpers in
a future patch when implicit structs are stored directly in the
parent struct rather than boxed through a pointer.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-9-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4a04354..a8b1057 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -39,7 +39,7 @@ def gen_visit_fields_decl(typ):
     if typ.name not in struct_fields_seen:
         ret += mcgen('''
 
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
+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)
@@ -61,7 +61,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
 
     visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
-        visit_type_%(c_type)s_fields(v, obj, errp);
+        visit_type_%(c_type)s_fields(v, *obj, errp);
         visit_end_implicit_struct(v);
     }
     error_propagate(errp, err);
@@ -85,7 +85,7 @@ def gen_visit_struct_fields(name, base, members, variants):
     struct_fields_seen.add(name)
     ret += mcgen('''
 
-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)
 {
     Error *err = NULL;
 
@@ -94,19 +94,19 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 
     if base:
         ret += mcgen('''
-    visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
+    visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, &err);
 ''',
                      c_type=base.c_name())
         ret += gen_err_check()
 
-    ret += gen_visit_fields(members, prefix='(*obj)->')
+    ret += gen_visit_fields(members, prefix='obj->')
 
     if variants:
         ret += mcgen('''
-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
+    if (!visit_start_union(v, !!obj->u.data, &err) || err) {
         goto out;
     }
-    switch ((*obj)->%(c_name)s) {
+    switch (obj->%(c_name)s) {
 ''',
                      c_name=c_name(variants.tag_member.name))
 
@@ -121,13 +121,13 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
                                            variants.tag_member.type.prefix))
             if simple_union_type:
                 ret += mcgen('''
-        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
+        visit_type_%(c_type)s(v, "data", &obj->u.%(c_name)s, &err);
 ''',
                              c_type=simple_union_type.c_name(),
                              c_name=c_name(var.name))
             else:
                 ret += mcgen('''
-        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
+        visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err);
 ''',
                              c_type=var.type.c_name(),
                              c_name=c_name(var.name))
@@ -270,7 +270,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     if (!*obj) {
         goto out_obj;
     }
-    visit_type_%(c_name)s_fields(v, obj, &err);
+    visit_type_%(c_name)s_fields(v, *obj, &err);
     error_propagate(errp, err);
     err = NULL;
 out_obj:
-- 
2.4.3

[Qemu-devel] [PULL 10/15] qapi: Adjust layout of FooList types

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types.  On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.

It requires visit_next_list() to gain a size parameter, to know
what size element to allocate; comparable to the size parameter
of visit_start_struct().

I debated about going one step further, to allow for fewer casts,
by doing:
    typedef GenericList GenericList;
    struct GenericList {
        GenericList *next;
    };
    struct FooList {
        GenericList base;
        Foo *value;
    };
so that you convert to 'GenericList *' by '&foolist->base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.

Note that for lists of objects, the 'value' payload is still
hidden behind a boxed pointer.  Someday, it would be nice to do:

struct FooList {
    FooList *next;
    Foo value;
};

for one less level of malloc for each list element.  This patch
is a step in that direction (now that 'next' is no longer at a
fixed non-zero offset within the struct, we can store more than
just a pointer's-worth of data as the value payload), but the
actual conversion would be a task for another series, as it will
touch a lot of code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-10-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/visitor-impl.h  |  2 +-
 include/qapi/visitor.h       | 13 ++++++-------
 qapi/opts-visitor.c          |  4 ++--
 qapi/qapi-dealloc-visitor.c  |  3 ++-
 qapi/qapi-visit-core.c       |  5 +++--
 qapi/qmp-input-visitor.c     |  5 +++--
 qapi/qmp-output-visitor.c    |  3 ++-
 qapi/string-input-visitor.c  |  4 ++--
 qapi/string-output-visitor.c |  2 +-
 scripts/qapi-types.py        |  5 +----
 scripts/qapi-visit.py        |  2 +-
 11 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index ea252f8..7905a28 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -29,7 +29,7 @@ struct Visitor
 
     void (*start_list)(Visitor *v, const char *name, Error **errp);
     /* Must be set */
-    GenericList *(*next_list)(Visitor *v, GenericList **list);
+    GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
     /* Must be set */
     void (*end_list)(Visitor *v);
 
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5e581dc..8a2d5cc 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,13 +19,12 @@
 #include "qapi/error.h"
 #include <stdlib.h>
 
-typedef struct GenericList
-{
-    union {
-        void *value;
-        uint64_t padding;
-    };
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator.  It is used as a typical
+ * singly-linked list. */
+typedef struct GenericList {
     struct GenericList *next;
+    char padding[];
 } GenericList;
 
 void visit_start_struct(Visitor *v, const char *name, void **obj,
@@ -36,7 +35,7 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
 void visit_end_implicit_struct(Visitor *v);
 
 void visit_start_list(Visitor *v, const char *name, Error **errp);
-GenericList *visit_next_list(Visitor *v, GenericList **list);
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
 void visit_end_list(Visitor *v);
 
 /**
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ae5b955..73e4ace 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -215,7 +215,7 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
 
 
 static GenericList *
-opts_next_list(Visitor *v, GenericList **list)
+opts_next_list(Visitor *v, GenericList **list, size_t size)
 {
     OptsVisitor *ov = to_ov(v);
     GenericList **link;
@@ -258,7 +258,7 @@ opts_next_list(Visitor *v, GenericList **list)
         abort();
     }
 
-    *link = g_malloc0(sizeof **link);
+    *link = g_malloc0(size);
     return *link;
 }
 
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 2659d3f..6667e8c 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -100,7 +100,8 @@ static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp)
     qapi_dealloc_push(qov, NULL);
 }
 
-static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp)
+static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
+                                           size_t size)
 {
     GenericList *list = *listp;
     QapiDeallocVisitor *qov = to_qov(v);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f856286..b4a0f21 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -50,9 +50,10 @@ void visit_start_list(Visitor *v, const char *name, Error **errp)
     v->start_list(v, name, errp);
 }
 
-GenericList *visit_next_list(Visitor *v, GenericList **list)
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
 {
-    return v->next_list(v, list);
+    assert(list && size >= sizeof(GenericList));
+    return v->next_list(v, list, size);
 }
 
 void visit_end_list(Visitor *v)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 2f48b95..2621660 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -165,7 +165,8 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
     qmp_input_push(qiv, qobj, errp);
 }
 
-static GenericList *qmp_input_next_list(Visitor *v, GenericList **list)
+static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
+                                        size_t size)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     GenericList *entry;
@@ -184,7 +185,7 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list)
         return NULL;
     }
 
-    entry = g_malloc0(sizeof(*entry));
+    entry = g_malloc0(size);
     if (first) {
         *list = entry;
     } else {
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f47eefa..d44c676 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -126,7 +126,8 @@ static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
     qmp_output_push(qov, list);
 }
 
-static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp)
+static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
+                                         size_t size)
 {
     GenericList *list = *listp;
     QmpOutputVisitor *qov = to_qov(v);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 18b9339..59eb5dc 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -139,7 +139,7 @@ start_list(Visitor *v, const char *name, Error **errp)
     }
 }
 
-static GenericList *next_list(Visitor *v, GenericList **list)
+static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
 {
     StringInputVisitor *siv = to_siv(v);
     GenericList **link;
@@ -173,7 +173,7 @@ static GenericList *next_list(Visitor *v, GenericList **list)
         link = &(*list)->next;
     }
 
-    *link = g_malloc0(sizeof **link);
+    *link = g_malloc0(size);
     return *link;
 }
 
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b980bd3..c2e5c5b 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -276,7 +276,7 @@ start_list(Visitor *v, const char *name, Error **errp)
     sov->head = true;
 }
 
-static GenericList *next_list(Visitor *v, GenericList **list)
+static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
 {
     StringOutputVisitor *sov = to_sov(v);
     GenericList *ret = NULL;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7b0dca8..83f230a 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -26,11 +26,8 @@ def gen_array(name, element_type):
     return mcgen('''
 
 struct %(c_name)s {
-    union {
-        %(c_type)s value;
-        uint64_t padding;
-    };
     %(c_name)s *next;
+    %(c_type)s value;
 };
 ''',
                  c_name=c_name(name), c_type=element_type.c_type())
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a8b1057..dfeef71 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -173,7 +173,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     }
 
     for (prev = (GenericList **)obj;
-         !err && (i = visit_next_list(v, prev)) != NULL;
+         !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
          prev = &i) {
         %(c_name)s *native_i = (%(c_name)s *)i;
         visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
-- 
2.4.3

[Qemu-devel] [PULL 11/15] qapi: Emit structs used as variants in topological order

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

Right now, we emit the branches of union types as a boxed pointer,
and it suffices to have a forward declaration of the type.  However,
a future patch will swap things to directly use the branch type,
instead of hiding it behind a pointer.  For this to work, the
compiler needs the full definition of the type, not just a forward
declaration, prior to the union that is including the branch type.
This patch just adds topological sorting to hoist all types
mentioned in a branch of a union to be fully declared before the
union itself.  The sort is always possible, because we do not
allow circular union types that include themselves as a direct
branch (it is, however, still possible to include a branch type
that itself has a pointer to the union, for a type that can
indirectly recursively nest itself - that remains safe, because
that the member of the branch type will remain a pointer, and the
QMP representation of such a type adds another {} for each recurring
layer of the union type).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-11-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 83f230a..6ea0ae6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -2,7 +2,7 @@
 # QAPI types generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (c) 2013-2015 Red Hat Inc.
+# Copyright (c) 2013-2016 Red Hat Inc.
 #
 # Authors:
 #  Anthony Liguori <aliguori@us.ibm.com>
@@ -14,6 +14,11 @@
 from qapi import *
 
 
+# variants must be emitted before their container; track what has already
+# been output
+objects_seen = set()
+
+
 def gen_fwd_object_or_array(name):
     return mcgen('''
 
@@ -49,11 +54,23 @@ def gen_struct_fields(members):
 
 
 def gen_object(name, base, members, variants):
-    ret = mcgen('''
+    if name in objects_seen:
+        return ''
+    objects_seen.add(name)
+
+    ret = ''
+    if variants:
+        for v in variants.variants:
+            if (isinstance(v.type, QAPISchemaObjectType) and
+                    not v.type.is_implicit()):
+                ret += gen_object(v.type.name, v.type.base,
+                                  v.type.local_members, v.type.variants)
+
+    ret += mcgen('''
 
 struct %(c_name)s {
 ''',
-                c_name=c_name(name))
+                 c_name=c_name(name))
 
     if base:
         ret += mcgen('''
-- 
2.4.3

[Qemu-devel] [PULL 12/15] qapi-visit: Use common idiom in gen_visit_fields_decl()

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

We have several instances of methods that do an early exit if
output is not needed, then log that output is being generated,
and finally produce the output; see qapi-types.py:gen_object()
and qapi-visit.py:gen_visit_implicit_struct().  The odd man
out was gen_visit_fields_decl(); rearrange it to be more like
the others.  No semantic change or difference to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-12-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index dfeef71..d7d7ed5 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -35,15 +35,14 @@ 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.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
+                 c_type=typ.c_name())
 
 
 def gen_visit_implicit_struct(typ):
-- 
2.4.3

[Qemu-devel] [PULL 13/15] qapi: Don't box struct branch of alternate

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 6ea0ae6..4dabe91 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 d7d7ed5..f4e38d1 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 17bf633..8497777 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 ef836d5..47cf6aa 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 2b0f7e9..fe2f1a1 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);
-- 
2.4.3

[Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

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

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 996cb91..bfbd667 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 7905a28..c4af3e0 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 8a2d5cc..a6678b1 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 6667e8c..4eae555 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 b4a0f21..f7b9980 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 4dabe91..eac90d2 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 f4e38d1..3a3918f 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 47cf6aa..b05da5b 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 fe2f1a1..a7f8b45 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);
-- 
2.4.3

[Qemu-devel] [PULL 15/15] qapi: Change visit_start_implicit_struct to visit_start_alternate

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

After recent changes, the only remaining use of
visit_start_implicit_struct() is for allocating the space needed
when visiting an alternate.  Since the term 'implicit struct' is
hard to explain, rename the function to its current usage.  While
at it, we can merge the functionality of visit_get_next_type()
into the same function, making it more like visit_start_struct().

Generated code is now slightly smaller:

| {
|     Error *err = NULL;
|
|-    visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), &err);
|+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
|+                          true, &err);
|     if (err) {
|         goto out;
|     }
|-    visit_get_next_type(v, name, &(*obj)->type, true, &err);
|-    if (err) {
|-        goto out_obj;
|-    }
|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|         visit_start_struct(v, name, NULL, 0, &err);
...
|     }
|-out_obj:
|-    visit_end_implicit_struct(v);
|+    visit_end_alternate(v);
| out:
|     error_propagate(errp, err);
| }

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-16-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/visitor-impl.h | 17 ++++++++--------
 include/qapi/visitor.h      | 49 +++++++++++++++++++++++++++++++++++----------
 qapi/qapi-dealloc-visitor.c | 13 ++++++------
 qapi/qapi-visit-core.c      | 40 ++++++++++++++++--------------------
 qapi/qmp-input-visitor.c    | 24 +++++++++-------------
 scripts/qapi-visit.py       | 10 +++------
 6 files changed, 82 insertions(+), 71 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index c4af3e0..6a1ddfb 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -22,22 +22,23 @@ struct Visitor
                          size_t size, Error **errp);
     void (*end_struct)(Visitor *v, Error **errp);
 
-    void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
-                                  Error **errp);
-    /* May be NULL */
-    void (*end_implicit_struct)(Visitor *v);
-
     void (*start_list)(Visitor *v, const char *name, Error **errp);
     /* Must be set */
     GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
     /* Must be set */
     void (*end_list)(Visitor *v);
 
+    /* Optional, needed for input and dealloc visitors.  */
+    void (*start_alternate)(Visitor *v, const char *name,
+                            GenericAlternate **obj, size_t size,
+                            bool promote_int, Error **errp);
+
+    /* Optional, needed for dealloc visitor.  */
+    void (*end_alternate)(Visitor *v);
+
+    /* Must be set. */
     void (*type_enum)(Visitor *v, const char *name, int *obj,
                       const char *const strings[], Error **errp);
-    /* May be NULL; only needed for input visitors. */
-    void (*get_next_type)(Visitor *v, const char *name, QType *type,
-                          bool promote_int, Error **errp);
 
     /* Must be set. */
     void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a6678b1..83cad74 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,17 +27,52 @@ typedef struct GenericList {
     char padding[];
 } GenericList;
 
+/* This struct is layout-compatible with all Alternate types
+ * created by the qapi generator. */
+typedef struct GenericAlternate {
+    QType type;
+    char padding[];
+} GenericAlternate;
+
 void visit_start_struct(Visitor *v, const char *name, void **obj,
                         size_t size, Error **errp);
 void visit_end_struct(Visitor *v, Error **errp);
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
-                                 Error **errp);
-void visit_end_implicit_struct(Visitor *v);
 
 void visit_start_list(Visitor *v, const char *name, Error **errp);
 GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
 void visit_end_list(Visitor *v);
 
+/*
+ * Start the visit of an alternate @obj with the given @size.
+ *
+ * @name specifies the relationship to the containing struct (ignored
+ * for a top level visit, the name of the key if this alternate is
+ * part of an object, or NULL if this alternate is part of a list).
+ *
+ * @obj must not be NULL. Input visitors will allocate @obj and
+ * determine the qtype of the next thing to be visited, stored in
+ * (*@obj)->type.  Other visitors will leave @obj unchanged.
+ *
+ * If @promote_int, treat integers as QTYPE_FLOAT.
+ *
+ * If successful, this must be paired with visit_end_alternate(), even
+ * if visiting the contents of the alternate fails.
+ */
+void visit_start_alternate(Visitor *v, const char *name,
+                           GenericAlternate **obj, size_t size,
+                           bool promote_int, Error **errp);
+
+/*
+ * Finish visiting an alternate type.
+ *
+ * Must be called after a successful visit_start_alternate(), even if
+ * an error occurred in the meantime.
+ *
+ * TODO: Should all the visit_end_* interfaces take obj parameter, so
+ * that dealloc visitor need not track what was passed in visit_start?
+ */
+void visit_end_alternate(Visitor *v);
+
 /**
  * Check if an optional member @name of an object needs visiting.
  * For input visitors, set *@present according to whether the
@@ -46,14 +81,6 @@ void visit_end_list(Visitor *v);
  */
 bool visit_optional(Visitor *v, const char *name, bool *present);
 
-/**
- * Determine the qtype of the item @name in the current object visit.
- * For input visitors, set *@type to the correct qtype of a qapi
- * alternate type; for other visitors, leave *@type unchanged.
- * If @promote_int, treat integers as QTYPE_FLOAT.
- */
-void visit_get_next_type(Visitor *v, const char *name, QType *type,
-                         bool promote_int, Error **errp);
 void visit_type_enum(Visitor *v, const char *name, int *obj,
                      const char *const strings[], Error **errp);
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 4eae555..6922179 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -76,16 +76,15 @@ static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
     }
 }
 
-static void qapi_dealloc_start_implicit_struct(Visitor *v,
-                                               void **obj,
-                                               size_t size,
-                                               Error **errp)
+static void qapi_dealloc_start_alternate(Visitor *v, const char *name,
+                                         GenericAlternate **obj, size_t size,
+                                         bool promote_int, Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, obj);
 }
 
-static void qapi_dealloc_end_implicit_struct(Visitor *v)
+static void qapi_dealloc_end_alternate(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
@@ -187,8 +186,8 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
 
     v->visitor.start_struct = qapi_dealloc_start_struct;
     v->visitor.end_struct = qapi_dealloc_end_struct;
-    v->visitor.start_implicit_struct = qapi_dealloc_start_implicit_struct;
-    v->visitor.end_implicit_struct = qapi_dealloc_end_implicit_struct;
+    v->visitor.start_alternate = qapi_dealloc_start_alternate;
+    v->visitor.end_alternate = qapi_dealloc_end_alternate;
     v->visitor.start_list = qapi_dealloc_start_list;
     v->visitor.next_list = qapi_dealloc_next_list;
     v->visitor.end_list = qapi_dealloc_end_list;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f7b9980..856606b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -30,21 +30,6 @@ void visit_end_struct(Visitor *v, Error **errp)
     v->end_struct(v, errp);
 }
 
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
-                                 Error **errp)
-{
-    if (v->start_implicit_struct) {
-        v->start_implicit_struct(v, obj, size, errp);
-    }
-}
-
-void visit_end_implicit_struct(Visitor *v)
-{
-    if (v->end_implicit_struct) {
-        v->end_implicit_struct(v);
-    }
-}
-
 void visit_start_list(Visitor *v, const char *name, Error **errp)
 {
     v->start_list(v, name, errp);
@@ -61,6 +46,23 @@ void visit_end_list(Visitor *v)
     v->end_list(v);
 }
 
+void visit_start_alternate(Visitor *v, const char *name,
+                           GenericAlternate **obj, size_t size,
+                           bool promote_int, Error **errp)
+{
+    assert(obj && size >= sizeof(GenericAlternate));
+    if (v->start_alternate) {
+        v->start_alternate(v, name, obj, size, promote_int, errp);
+    }
+}
+
+void visit_end_alternate(Visitor *v)
+{
+    if (v->end_alternate) {
+        v->end_alternate(v);
+    }
+}
+
 bool visit_optional(Visitor *v, const char *name, bool *present)
 {
     if (v->optional) {
@@ -69,14 +71,6 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
     return *present;
 }
 
-void visit_get_next_type(Visitor *v, const char *name, QType *type,
-                         bool promote_int, Error **errp)
-{
-    if (v->get_next_type) {
-        v->get_next_type(v, name, type, promote_int, errp);
-    }
-}
-
 void visit_type_enum(Visitor *v, const char *name, int *obj,
                      const char *const strings[], Error **errp)
 {
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 2621660..e659832 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -143,14 +143,6 @@ static void qmp_input_end_struct(Visitor *v, Error **errp)
     qmp_input_pop(qiv, errp);
 }
 
-static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
-                                            size_t size, Error **errp)
-{
-    if (obj) {
-        *obj = g_malloc0(size);
-    }
-}
-
 static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -202,19 +194,22 @@ static void qmp_input_end_list(Visitor *v)
     qmp_input_pop(qiv, &error_abort);
 }
 
-static void qmp_input_get_next_type(Visitor *v, const char *name, QType *type,
-                                    bool promote_int, Error **errp)
+static void qmp_input_start_alternate(Visitor *v, const char *name,
+                                      GenericAlternate **obj, size_t size,
+                                      bool promote_int, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, false);
 
     if (!qobj) {
+        *obj = NULL;
         error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
         return;
     }
-    *type = qobject_type(qobj);
-    if (promote_int && *type == QTYPE_QINT) {
-        *type = QTYPE_QFLOAT;
+    *obj = g_malloc0(size);
+    (*obj)->type = qobject_type(qobj);
+    if (promote_int && (*obj)->type == QTYPE_QINT) {
+        (*obj)->type = QTYPE_QFLOAT;
     }
 }
 
@@ -345,10 +340,10 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
 
     v->visitor.start_struct = qmp_input_start_struct;
     v->visitor.end_struct = qmp_input_end_struct;
-    v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
     v->visitor.start_list = qmp_input_start_list;
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
+    v->visitor.start_alternate = qmp_input_start_alternate;
     v->visitor.type_enum = input_type_enum;
     v->visitor.type_int64 = qmp_input_type_int64;
     v->visitor.type_uint64 = qmp_input_type_uint64;
@@ -357,7 +352,6 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.type_number = qmp_input_type_number;
     v->visitor.type_any = qmp_input_type_any;
     v->visitor.optional = qmp_input_optional;
-    v->visitor.get_next_type = qmp_input_get_next_type;
 
     qmp_input_push(v, obj, NULL);
     qobject_incref(obj);
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3a3918f..2308268 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -182,14 +182,11 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
 {
     Error *err = NULL;
 
-    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
+                          %(promote_int)s, &err);
     if (err) {
         goto out;
     }
-    visit_get_next_type(v, name, &(*obj)->type, %(promote_int)s, &err);
-    if (err) {
-        goto out_obj;
-    }
     switch ((*obj)->type) {
 ''',
                  c_name=c_name(name), promote_int=promote_int)
@@ -227,8 +224,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
     }
-out_obj:
-    visit_end_implicit_struct(v);
+    visit_end_alternate(v);
 out:
     error_propagate(errp, err);
 }
-- 
2.4.3

Re: [Qemu-devel] [PULL 00/15] QAPI patches for 2016-02-19

[Peter Maydell]

On 19 February 2016 at 12:17, Markus Armbruster <armbru@redhat.com> wrote:
> The following changes since commit dd5e38b19d7cb07d317e1285941d8245c01da540:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160218-1' into staging (2016-02-18 15:20:35 +0000)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2016-02-19
>
> for you to fetch changes up to dbf11922622685934bfb41e7cf2be9bd4a0405c0:
>
>   qapi: Change visit_start_implicit_struct to visit_start_alternate (2016-02-19 11:08:57 +0100)
>
> ----------------------------------------------------------------
> QAPI patches for 2016-02-19
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Daniel P. Berrange]

On Fri, Feb 19, 2016 at 01:18:05PM +0100, Markus Armbruster wrote:
> From: Eric Blake <eblake@redhat.com>
> 
> 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(-)

This change has broken my pending LUKS encryption patch series[1], and
I'm not entirely sure of the best way to resolve it.

I'll simplify the schema slighty for sake of illustration here.
Consider I have the following:

  { 'struct': 'QCryptoBlockOptionsBase',
    'data': { 'format': 'QCryptoBlockFormat' }}

  { 'struct': 'QCryptoBlockOptionsQCow',
    'data': { '*key-secret': 'str' }}

  { 'struct': 'QCryptoBlockOptionsLUKS',
    'data': { '*key-secret': 'str',
              '*cipher-alg': 'QCryptoCipherAlgorithm',
              '*cipher-mode': 'QCryptoCipherMode',
              '*ivgen-alg': 'QCryptoIVGenAlgorithm',
              '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
              '*hash-alg': 'QCryptoHashAlgorithm'}}

  { 'union': 'QCryptoBlockOptions',
    'base': 'QCryptoBlockOptionsBase',
    'discriminator': 'format',
    'data': { 'qcow': 'QCryptoBlockOptionsQCow',
              'luks': 'QCryptoBlockOptionsLUKS' } }

If I want to visit QCryptoBlockOptions, allocating a new struct
instance, I can still do so

   QCryptoBlockOptions *opts = NULL;

   visit_type_QCryptoBlockOptions(v, "block", &opts, errp)


Similarly, if I want to visit QCryptoBlockOptionsLUKS, allocating
a new struct instance, I can still do so

   QCryptoBlockOptionsLUKS *opts = NULL;

   visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts, errp)

My code needs todo something a little different though - it needs
to directly visit one of the union branches. Previously, I could
allocate a QCryptoBlockOptions and then visit a specific union
branch like this:

   QCryptoBlockOptions *opts = NULL;

   opts = g_new0(QCryptoBlockOptions, 1);
   opts.format = Q_CRYPTO_BLOCK_FORMAT_LUKS

   visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts.u.luks, errp)


I need todo this, because my visitor instance does not have any
'format' field to visit - we know the format upfront from the
block layer driver choice. So we need to directly visit the
appropriate inline union branch. This no longer works, because
the opts.u.luks field is now inlined instead of being boxed :-(

I could visit_type_QCryptoBlockOptionsLUKS_fields(v, opts.u.luks, errp)
but the '_fields' methods are all declared static in qapi-visit.c
preventing their use.

IMHO, now that QAPI inlines the flat unions, we should be making
the _fields() methods public.

Regards,
Daniel

[1] https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03176.html
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On 02/23/2016 07:50 AM, Daniel P. Berrange wrote:
> On Fri, Feb 19, 2016 at 01:18:05PM +0100, Markus Armbruster wrote:
>> From: Eric Blake <eblake@redhat.com>
>>
>> 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.
>>

> 
> My code needs todo something a little different though - it needs
> to directly visit one of the union branches. Previously, I could
> allocate a QCryptoBlockOptions and then visit a specific union
> branch like this:
> 
>    QCryptoBlockOptions *opts = NULL;
> 
>    opts = g_new0(QCryptoBlockOptions, 1);
>    opts.format = Q_CRYPTO_BLOCK_FORMAT_LUKS
> 
>    visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts.u.luks, errp)

which allocated a pointer and visited the fields.  We no longer need the
pointer, but still need the fields visited.

> 
> 
> I need todo this, because my visitor instance does not have any
> 'format' field to visit - we know the format upfront from the
> block layer driver choice. So we need to directly visit the
> appropriate inline union branch. This no longer works, because
> the opts.u.luks field is now inlined instead of being boxed :-(

You're using OptsVisitor, right? Is it possible to hack the QemuOpts
that you feed to opts_visitor_new() to include a 'format' field?

> 
> I could visit_type_QCryptoBlockOptionsLUKS_fields(v, opts.u.luks, errp)
> but the '_fields' methods are all declared static in qapi-visit.c
> preventing their use.
> 
> IMHO, now that QAPI inlines the flat unions, we should be making
> the _fields() methods public.

But my suggestion would just be a hack. Yours makes more sense in the
long run, so I'll go ahead and propose that patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

On 02/23/2016 08:18 AM, Eric Blake wrote:

>>
>> I could visit_type_QCryptoBlockOptionsLUKS_fields(v, opts.u.luks, errp)
>> but the '_fields' methods are all declared static in qapi-visit.c
>> preventing their use.
>>
>> IMHO, now that QAPI inlines the flat unions, we should be making
>> the _fields() methods public.
> 
> But my suggestion would just be a hack. Yours makes more sense in the
> long run, so I'll go ahead and propose that patch.

Another alternative, but not my preference (mentioning only for
completeness):

We could change the contract of visit_type_FOO(&obj) to say that if it
points to non-NULL, we just visit the fields in place.  All callers
would then be required to set *obj = NULL prior to visit_type_FOO(&obj)
for existing semantics, and you would set *obj = &parent.u.luks for new
semantics.

But this would entail an audit of existing callers (too many are passing
uninitialized pointers, which must now be initialized), and feels a bit
too complex.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Daniel P. Berrange]

On Tue, Feb 23, 2016 at 08:18:55AM -0700, Eric Blake wrote:
> On 02/23/2016 07:50 AM, Daniel P. Berrange wrote:
> > On Fri, Feb 19, 2016 at 01:18:05PM +0100, Markus Armbruster wrote:
> >> From: Eric Blake <eblake@redhat.com>
> >>
> >> 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.
> >>
> 
> > 
> > My code needs todo something a little different though - it needs
> > to directly visit one of the union branches. Previously, I could
> > allocate a QCryptoBlockOptions and then visit a specific union
> > branch like this:
> > 
> >    QCryptoBlockOptions *opts = NULL;
> > 
> >    opts = g_new0(QCryptoBlockOptions, 1);
> >    opts.format = Q_CRYPTO_BLOCK_FORMAT_LUKS
> > 
> >    visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts.u.luks, errp)
> 
> which allocated a pointer and visited the fields.  We no longer need the
> pointer, but still need the fields visited.
> 
> > 
> > 
> > I need todo this, because my visitor instance does not have any
> > 'format' field to visit - we know the format upfront from the
> > block layer driver choice. So we need to directly visit the
> > appropriate inline union branch. This no longer works, because
> > the opts.u.luks field is now inlined instead of being boxed :-(
> 
> You're using OptsVisitor, right? Is it possible to hack the QemuOpts
> that you feed to opts_visitor_new() to include a 'format' field?

Yes, using QemuOpts, so I could feed in a fake 'format' i guess.

The temp hack I chose was to use an intermediate variable and then
memcpy & free. ie

    QCryptoBlockOpenOptions *ret;
    QCryptoBlockOptionsLUKS *tmp;

    ret = g_new0(QCryptoBlockOpenOptions, 1);
    ret->format = format;

    visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov), "luks",
                                       &tmp, &local_err);
    if (!local_err) {
        memcpy(&ret->u.luks, tmp, sizeof(*tmp));
        g_free(tmp);
    }

> > I could visit_type_QCryptoBlockOptionsLUKS_fields(v, opts.u.luks, errp)
> > but the '_fields' methods are all declared static in qapi-visit.c
> > preventing their use.
> > 
> > IMHO, now that QAPI inlines the flat unions, we should be making
> > the _fields() methods public.
> 
> But my suggestion would just be a hack. Yours makes more sense in the
> long run, so I'll go ahead and propose that patch.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|