From 8712fa5333ad348da20034b717dd814219d1ec11 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:41 -0600
Subject: qapi: More idiomatic string operations

Rather than slicing the end of a string, we can use python's
endswith().  And rather than creating a set of characters,
we can search for a character within a string.

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

(limited to 'scripts')

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9d53255320..3af4c2c737 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -172,7 +172,7 @@ class QAPISchemaParser(object):
 
             if self.tok == '#':
                 self.cursor = self.src.find('\n', self.cursor)
-            elif self.tok in ['{', '}', ':', ',', '[', ']']:
+            elif self.tok in "{}:,[]":
                 return
             elif self.tok == "'":
                 string = ''
@@ -390,7 +390,7 @@ def add_name(name, info, meta, implicit=False):
         raise QAPIExprError(info,
                             "%s '%s' is already defined"
                             % (all_names[name], name))
-    if not implicit and name[-4:] == 'Kind':
+    if not implicit and name.endswith('Kind'):
         raise QAPIExprError(info,
                             "%s '%s' should not end in 'Kind'"
                             % (meta, name))
@@ -910,7 +910,7 @@ class QAPISchemaEnumType(QAPISchemaType):
 
     def is_implicit(self):
         # See QAPISchema._make_implicit_enum_type()
-        return self.name[-4:] == 'Kind'
+        return self.name.endswith('Kind')
 
     def c_type(self, is_param=False):
         return c_name(self.name)
-- 
cgit v1.2.3-55-g7522


From f9e6102b48f21e464a847a858a456c521e7a83e5 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:42 -0600
Subject: qapi: More robust conditions for when labels are needed

We were using regular expressions to see if ret included
any earlier text that emitted a 'goto out;' line, to decide
whether we needed to output an 'out:' label.  But this is
fragile, if the ret text can possibly combine more than one
generated function body, where the first function used a
goto but the second does not.  Change the code to just check
for the known conditions which cause an error check to be
needed.  Besides, it's slightly more efficient to use plain
checks than regular expression searching.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-4-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 4 +++-
 scripts/qapi-visit.py    | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 43a893b4eb..561e47a42b 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -175,7 +175,9 @@ def gen_marshal(name, arg_type, ret_type):
     ret += gen_marshal_input_visit(arg_type)
     ret += gen_call(name, arg_type, ret_type)
 
-    if re.search('^ *goto out;', ret, re.MULTILINE):
+    # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
+    # for each arg_type member, and by gen_call() for ret_type
+    if (arg_type and arg_type.members) or ret_type:
         ret += mcgen('''
 
 out:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d0759d739a..2dc3aed220 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -87,7 +87,8 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 
     ret += gen_visit_fields(members, prefix='(*obj)->')
 
-    if re.search('^ *goto out;', ret, re.MULTILINE):
+    # 'goto out' produced for base, and by gen_visit_fields() for each member
+    if base or members:
         ret += mcgen('''
 
 out:
-- 
cgit v1.2.3-55-g7522


From 255960dd374d4497d6ea537305f1b0d8a3433789 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:43 -0600
Subject: qapi: Reserve '*List' type names for list types

Type names ending in 'List' can clash with qapi list types in
generated C.  We don't currently use such names. It is easier to
outlaw them now than to worry about how to resolve such a clash
in the future. For precedence, see commit 4dc2e69, which did the
same for names ending in 'Kind' versus implicit enum types for
qapi unions.

Update the testsuite to match.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt                    |  3 ++-
 scripts/qapi.py                           | 10 ++++------
 tests/qapi-schema/reserved-type-list.err  |  1 +
 tests/qapi-schema/reserved-type-list.exit |  2 +-
 tests/qapi-schema/reserved-type-list.json |  6 +++---
 tests/qapi-schema/reserved-type-list.out  |  3 ---
 6 files changed, 11 insertions(+), 14 deletions(-)

(limited to 'scripts')

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2afab20f55..c4264a819b 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -106,7 +106,8 @@ Types, commands, and events share a common namespace.  Therefore,
 generally speaking, type definitions should always use CamelCase for
 user-defined type names, while built-in types are lowercase. Type
 definitions should not end in 'Kind', as this namespace is used for
-creating implicit C enums for visiting union types.  Command names,
+creating implicit C enums for visiting union types, or in 'List', as
+this namespace is used for creating array types.  Command names,
 and field names within a type, should be all lower case with words
 separated by a hyphen.  However, some existing older commands and
 complex types use underscore; when extending such expressions,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3af4c2c737..d53b5c4b45 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -390,10 +390,10 @@ def add_name(name, info, meta, implicit=False):
         raise QAPIExprError(info,
                             "%s '%s' is already defined"
                             % (all_names[name], name))
-    if not implicit and name.endswith('Kind'):
+    if not implicit and (name.endswith('Kind') or name.endswith('List')):
         raise QAPIExprError(info,
-                            "%s '%s' should not end in 'Kind'"
-                            % (meta, name))
+                            "%s '%s' should not end in '%s'"
+                            % (meta, name, name[-4:]))
     all_names[name] = meta
 
 
@@ -1196,9 +1196,7 @@ class QAPISchema(object):
         return name
 
     def _make_array_type(self, element_type, info):
-        # TODO fooList namespace is not reserved; user can create collisions,
-        # or abuse our type system with ['fooList'] for 2D array
-        name = element_type + 'List'
+        name = element_type + 'List'   # Use namespace reserved by add_name()
         if not self.lookup_type(name):
             self._def_entity(QAPISchemaArrayType(name, info, element_type))
         return name
diff --git a/tests/qapi-schema/reserved-type-list.err b/tests/qapi-schema/reserved-type-list.err
index e69de29bb2..4510fa6d90 100644
--- a/tests/qapi-schema/reserved-type-list.err
+++ b/tests/qapi-schema/reserved-type-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-type-list.json:5: struct 'FooList' should not end in 'List'
diff --git a/tests/qapi-schema/reserved-type-list.exit b/tests/qapi-schema/reserved-type-list.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/reserved-type-list.exit
+++ b/tests/qapi-schema/reserved-type-list.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-type-list.json b/tests/qapi-schema/reserved-type-list.json
index 5b7d0f995f..98d53bf808 100644
--- a/tests/qapi-schema/reserved-type-list.json
+++ b/tests/qapi-schema/reserved-type-list.json
@@ -1,5 +1,5 @@
 # Potential C name collision
-# FIXME - This parses and compiles on its own, but prevents the user from
-# creating a type named 'Foo' and using ['Foo'] for an array.  We should
-# reject the use of any type names ending in 'List'.
+# We reserve names ending in 'List' for use by array types.
+# TODO - we could choose array names to avoid collision with user types,
+# in order to let this compile
 { 'struct': 'FooList', 'data': { 's': 'str' } }
diff --git a/tests/qapi-schema/reserved-type-list.out b/tests/qapi-schema/reserved-type-list.out
index 0406bfe319..e69de29bb2 100644
--- a/tests/qapi-schema/reserved-type-list.out
+++ b/tests/qapi-schema/reserved-type-list.out
@@ -1,3 +0,0 @@
-object :empty
-object FooList
-    member s: str optional=False
-- 
cgit v1.2.3-55-g7522


From 9fb081e0b98409556d023c7193eeb68947cd1211 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:44 -0600
Subject: qapi: Reserve 'q_*' and 'has_*' member names

c_name() produces names starting with 'q_' when protecting a
dictionary member name that would fail to directly compile, but
in doing so can cause clashes with any member name already
beginning with 'q-' or 'q_'.  Likewise, we create a C name 'has_'
for any optional member that can clash with any member name
beginning with 'has-' or 'has_'.

Technically, rather than blindly reserving the namespace,
we could try to complain about user names only when an actual
collision occurs, or even teach c_name() how to munge names
to avoid collisions.  But it is not trivial, especially when
collisions can occur across multiple types (such as via
inheritance or flat unions).  Besides, no existing .json
files are trying to use these names.  So it's easier to just
outright forbid the potential for collision.  We can always
relax things in the future if a real need arises for QMP to
express member names that have been forbidden here.

'has_' only has to be reserved for struct/union member names,
while 'q_' is reserved everywhere (matching the fact that
only members can be optional, while we use c_name() for munging
both members and entities).  Note that we could relax 'q_'
restrictions on entities independently from member names; for
example, c_name('qmp_' + 'unix') would result in a different
function name than our current 'qmp_' + c_name('unix').

Update and add tests to cover the new error messages.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-6-git-send-email-eblake@redhat.com>
[Consistently pass protect=False to c_name(); commit message tweaked
slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt                     | 11 +++++++----
 scripts/qapi.py                            |  8 +++++++-
 tests/qapi-schema/reserved-command-q.err   |  1 +
 tests/qapi-schema/reserved-command-q.exit  |  2 +-
 tests/qapi-schema/reserved-command-q.json  |  6 ++----
 tests/qapi-schema/reserved-command-q.out   |  5 -----
 tests/qapi-schema/reserved-member-has.err  |  1 +
 tests/qapi-schema/reserved-member-has.exit |  2 +-
 tests/qapi-schema/reserved-member-has.json |  7 +++----
 tests/qapi-schema/reserved-member-has.out  |  6 ------
 tests/qapi-schema/reserved-member-q.err    |  1 +
 tests/qapi-schema/reserved-member-q.exit   |  2 +-
 tests/qapi-schema/reserved-member-q.json   |  6 ++----
 tests/qapi-schema/reserved-member-q.out    |  4 ----
 14 files changed, 27 insertions(+), 35 deletions(-)

(limited to 'scripts')

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c4264a819b..4d8c2fcf02 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -112,7 +112,9 @@ and field names within a type, should be all lower case with words
 separated by a hyphen.  However, some existing older commands and
 complex types use underscore; when extending such expressions,
 consistency is preferred over blindly avoiding underscore.  Event
-names should be ALL_CAPS with words separated by underscore.
+names should be ALL_CAPS with words separated by underscore.  Field
+names cannot start with 'has-' or 'has_', as this is reserved for
+tracking optional fields.
 
 Any name (command, event, type, field, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
@@ -123,9 +125,10 @@ vendor), even if the rest of the name uses dash (example:
 __com.redhat_drive-mirror).  Other than downstream extensions (with
 leading underscore and the use of dots), all names should begin with a
 letter, and contain only ASCII letters, digits, dash, and underscore.
-It is okay to reuse names that match C keywords; the generator will
-rename a field named "default" in the QAPI to "q_default" in the
-generated C code.
+Names beginning with 'q_' are reserved for the generator: QMP names
+that resemble C keywords or other problematic strings will be munged
+in C to use this prefix.  For example, a field named "default" in
+qapi becomes "q_default" in the generated C code.
 
 In the rest of this document, usage lines are given for each
 expression type, with literal strings written in lower case and
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d53b5c4b45..3ff7b11e61 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -376,7 +376,9 @@ def check_name(expr_info, source, name, allow_optional=False,
     # code always prefixes it with the enum name
     if enum_member:
         membername = '_' + membername
-    if not valid_name.match(membername):
+    # Reserve the entire 'q_' namespace for c_name()
+    if not valid_name.match(membername) or \
+       c_name(membername, False).startswith('q_'):
         raise QAPIExprError(expr_info,
                             "%s uses invalid name '%s'" % (source, name))
 
@@ -488,6 +490,10 @@ def check_type(expr_info, source, value, allow_array=False,
     for (key, arg) in value.items():
         check_name(expr_info, "Member of %s" % source, key,
                    allow_optional=allow_optional)
+        if c_name(key, False).startswith('has_'):
+            raise QAPIExprError(expr_info,
+                                "Member of %s uses reserved name '%s'"
+                                % (source, key))
         # Todo: allow dictionaries to represent default values of
         # an optional argument.
         check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
diff --git a/tests/qapi-schema/reserved-command-q.err b/tests/qapi-schema/reserved-command-q.err
index e69de29bb2..f939e044eb 100644
--- a/tests/qapi-schema/reserved-command-q.err
+++ b/tests/qapi-schema/reserved-command-q.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-command-q.json:5: 'command' uses invalid name 'q-unix'
diff --git a/tests/qapi-schema/reserved-command-q.exit b/tests/qapi-schema/reserved-command-q.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/reserved-command-q.exit
+++ b/tests/qapi-schema/reserved-command-q.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-command-q.json b/tests/qapi-schema/reserved-command-q.json
index be9944c68a..99f8aae314 100644
--- a/tests/qapi-schema/reserved-command-q.json
+++ b/tests/qapi-schema/reserved-command-q.json
@@ -1,7 +1,5 @@
 # C entity name collision
-# FIXME - This parses, but fails to compile, because it attempts to declare
-# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_name()
-# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
-# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+# We reject names like 'q-unix', because they can collide with the mangled
+# name for 'unix' in generated C.
 { 'command': 'unix' }
 { 'command': 'q-unix' }
diff --git a/tests/qapi-schema/reserved-command-q.out b/tests/qapi-schema/reserved-command-q.out
index b31b38ff0d..e69de29bb2 100644
--- a/tests/qapi-schema/reserved-command-q.out
+++ b/tests/qapi-schema/reserved-command-q.out
@@ -1,5 +0,0 @@
-object :empty
-command q-unix None -> None
-   gen=True success_response=True
-command unix None -> None
-   gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-has.err b/tests/qapi-schema/reserved-member-has.err
index e69de29bb2..e755771446 100644
--- a/tests/qapi-schema/reserved-member-has.err
+++ b/tests/qapi-schema/reserved-member-has.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-has.json:5: Member of 'data' for command 'oops' uses reserved name 'has-a'
diff --git a/tests/qapi-schema/reserved-member-has.exit b/tests/qapi-schema/reserved-member-has.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/reserved-member-has.exit
+++ b/tests/qapi-schema/reserved-member-has.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-member-has.json b/tests/qapi-schema/reserved-member-has.json
index a2197de6b5..45b9109bdc 100644
--- a/tests/qapi-schema/reserved-member-has.json
+++ b/tests/qapi-schema/reserved-member-has.json
@@ -1,6 +1,5 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'has_a' members, one from the flag for optional 'a', and the other
-# from member 'has-a'.  Either reject this at parse time, or munge the C
-# names to avoid the collision.
+# We reject names like 'has-a', because they can collide with the flag
+# for an optional 'a' in generated C.
+# TODO we could munge the optional flag name to avoid the collision.
 { 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-has.out b/tests/qapi-schema/reserved-member-has.out
index 5a18b6be8c..e69de29bb2 100644
--- a/tests/qapi-schema/reserved-member-has.out
+++ b/tests/qapi-schema/reserved-member-has.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
-    member a: str optional=True
-    member has-a: str optional=False
-command oops :obj-oops-arg -> None
-   gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-q.err b/tests/qapi-schema/reserved-member-q.err
index e69de29bb2..f3d5dd7818 100644
--- a/tests/qapi-schema/reserved-member-q.err
+++ b/tests/qapi-schema/reserved-member-q.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-q.json:4: Member of 'data' for struct 'Foo' uses invalid name 'q-unix'
diff --git a/tests/qapi-schema/reserved-member-q.exit b/tests/qapi-schema/reserved-member-q.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/reserved-member-q.exit
+++ b/tests/qapi-schema/reserved-member-q.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-member-q.json b/tests/qapi-schema/reserved-member-q.json
index 1602ed3281..62fed8fddf 100644
--- a/tests/qapi-schema/reserved-member-q.json
+++ b/tests/qapi-schema/reserved-member-q.json
@@ -1,6 +1,4 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because it attempts to declare
-# two 'q_unix' members (one for 'q-unix', the other because c_name()
-# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
-# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+# We reject names like 'q-unix', because they can collide with the mangled
+# name for 'unix' in generated C.
 { 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } }
diff --git a/tests/qapi-schema/reserved-member-q.out b/tests/qapi-schema/reserved-member-q.out
index 0d8685aeb0..e69de29bb2 100644
--- a/tests/qapi-schema/reserved-member-q.out
+++ b/tests/qapi-schema/reserved-member-q.out
@@ -1,4 +0,0 @@
-object :empty
-object Foo
-    member unix: int optional=False
-    member q-unix: bool optional=False
-- 
cgit v1.2.3-55-g7522


From d02cf37766ba3cf918d7085aa7848c9dc05fd11a Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:46 -0600
Subject: qapi-visit: Split off visit_type_FOO_fields forward decl

We generate a static visit_type_FOO_fields() for every type
FOO.  However, sometimes we need a forward declaration. Split
the code to generate the forward declaration out of
gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
and also prepare for a forward declaration to be emitted
during gen_visit_struct(), so that a future patch can switch
from using visit_type_FOO_implicit() to the simpler
visit_type_FOO_fields() as part of unboxing the base class
of a struct.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-8-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2dc3aed220..d4408f25b1 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,7 +15,12 @@
 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()
 
 
@@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **
                  c_name=c_name(name), c_type=c_type)
 
 
-def gen_visit_implicit_struct(typ):
-    if typ in implicit_structs_seen:
-        return ''
-    implicit_structs_seen.add(typ)
-
+def gen_visit_fields_decl(typ):
     ret = ''
     if typ.name not in struct_fields_seen:
-        # Need a forward declaration
         ret += mcgen('''
 
 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
 ''',
                      c_type=typ.c_name())
+        struct_fields_seen.add(typ.name)
+    return ret
+
+
+def gen_visit_implicit_struct(typ):
+    if typ in implicit_structs_seen:
+        return ''
+    implicit_structs_seen.add(typ)
+
+    ret = gen_visit_fields_decl(typ)
 
     ret += mcgen('''
 
-- 
cgit v1.2.3-55-g7522


From f87ab7f9bd956250c48b5c6e9b607b537fd21543 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:47 -0600
Subject: qapi-types: Refactor base fields output

Move code from gen_union() into gen_struct_fields() in order for
a later patch to share code when enumerating inherited fields
for struct types.

No change to generated code.

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

(limited to 'scripts')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4fe618ef3c..40e9f79b63 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -51,10 +51,21 @@ def gen_struct_field(name, typ, optional):
     return ret
 
 
-def gen_struct_fields(members):
+def gen_struct_fields(local_members, base=None):
     ret = ''
 
-    for memb in members:
+    if base:
+        ret += mcgen('''
+    /* Members inherited from %(c_name)s: */
+''',
+                     c_name=base.c_name())
+        for memb in base.members:
+            ret += gen_struct_field(memb.name, memb.type, memb.optional)
+        ret += mcgen('''
+    /* Own members: */
+''')
+
+    for memb in local_members:
         ret += gen_struct_field(memb.name, memb.type, memb.optional)
     return ret
 
@@ -126,14 +137,7 @@ struct %(c_name)s {
 ''',
                 c_name=c_name(name))
     if base:
-        ret += mcgen('''
-    /* Members inherited from %(c_name)s: */
-''',
-                     c_name=c_name(base.name))
-        ret += gen_struct_fields(base.members)
-        ret += mcgen('''
-    /* Own members: */
-''')
+        ret += gen_struct_fields([], base)
     else:
         ret += mcgen('''
     %(c_type)s kind;
-- 
cgit v1.2.3-55-g7522


From 30594fe1cd4355626e73b80645428105d0df3cf6 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:48 -0600
Subject: qapi: Prefer typesafe upcasts to qapi base classes

A previous patch (commit 1e6c1616) made it possible to
directly cast from a qapi flat union type to its base type.
However, it requires the use of a C cast, which turns off
compiler type-safety checks.  Fortunately, no such casts
exist, just yet.

Regardless, add inline type-safe wrappers named
qapi_FOO_base() for any union type FOO that has a base,
which can be used for a safer upcast, and enhance the
testsuite to cover the new functionality.

A future patch will extend the upcast support to structs,
where such conversions do exist already.

Note that C makes const-correct upcasts annoying because
it lacks overloads; these functions cast away const so that
they can accept user pointers whether const or not, and the
result in turn can be assigned to normal or const pointers.
Alternatively, this could have been done with macros, but
type-safe macros are hairy, and not worthwhile here.

This patch just adds upcasts.  None of our code needed to
downcast from a base qapi class to a child.  Also, in the
case of grandchildren (such as BlockdevOptionsQcow2), the
caller will need to call two functions to get to the inner
base (although it wouldn't be too hard to generate a
qapi_FOO_base_base() if desired).  If a user changes qapi
to alter the base class hierarchy, such as going from
'A -> C' to 'A -> B -> C', it will change the type of
'qapi_C_base()', and the compiler will point out the places
that are affected by the new base.

One alternative was proposed, but was deemed too ugly to use
in practice: the generators could output redundant
information using anonymous types:
| struct Child {
|     union {
|         struct {
|             Type1 parent_member1;
|             Type2 parent_member2;
|         };
|         Parent base;
|     };
| };
With that ugly proposal, for a given qapi type, obj->member
and obj->base.member would refer to the same storage; allowing
convenience in working with members without needing 'base.'
allowing typesafe upcast without needing a C cast by accessing
'&obj->base', and allowing downcasts from the parent back to
the child possible through container_of(obj, Child, base).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-10-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py          | 16 ++++++++++++++++
 tests/test-qmp-input-visitor.c |  5 +++++
 2 files changed, 21 insertions(+)

(limited to 'scripts')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 40e9f79b63..f9fcf150a4 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -98,6 +98,19 @@ struct %(c_name)s {
     return ret
 
 
+def gen_upcast(name, base):
+    # C makes const-correctness ugly.  We have to cast away const to let
+    # this function work for both const and non-const obj.
+    return mcgen('''
+
+static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
+{
+    return (%(base)s *)obj;
+}
+''',
+                 c_name=c_name(name), base=base.c_name())
+
+
 def gen_alternate_qtypes_decl(name):
     return mcgen('''
 
@@ -267,6 +280,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         if variants:
             assert not members      # not implemented
             self.decl += gen_union(name, base, variants)
+            # TODO Use gen_upcast on structs too, once they have sane layout
+            if base:
+                self.decl += gen_upcast(name, base)
         else:
             self.decl += gen_struct(name, base, members)
         self._gen_type_cleanup(name)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 8941963c8d..da21709714 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -347,6 +347,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
     Visitor *v;
     Error *err = NULL;
     UserDefFlatUnion *tmp;
+    UserDefUnionBase *base;
 
     v = visitor_input_test_init(data,
                                 "{ 'enum1': 'value1', "
@@ -360,6 +361,10 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
     g_assert_cmpstr(tmp->string, ==, "str");
     g_assert_cmpint(tmp->integer, ==, 41);
     g_assert_cmpint(tmp->value1->boolean, ==, true);
+
+    base = qapi_UserDefFlatUnion_base(tmp);
+    g_assert(&base->enum1 == &tmp->enum1);
+
     qapi_free_UserDefFlatUnion(tmp);
 }
 
-- 
cgit v1.2.3-55-g7522


From ddf21908961073199f3d186204da4810f2ea150b Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:49 -0600
Subject: qapi: Unbox base members

Rather than storing a base class as a pointer to a box, just
store the fields of that base class in the same order, so that
a child struct can be directly cast to its parent.  This gives
less malloc overhead, less pointer dereferencing, and even less
generated code.  Compare to the earlier commit 1e6c1616a "qapi:
Generate a nicer struct for flat unions" (although that patch
had fewer places to change, as less of qemu was directly using
qapi structs for flat unions).  It also allows us to turn on
automatic type-safe wrappers for upcasting to the base class
of a struct.

Changes to the generated code look like this in qapi-types.h:

| struct SpiceChannel {
|-    SpiceBasicInfo *base;
|+    /* Members inherited from SpiceBasicInfo: */
|+    char *host;
|+    char *port;
|+    NetworkAddressFamily family;
|+    /* Own members: */
|     int64_t connection_id;

as well as additional upcast functions like qapi_SpiceChannel_base().
Meanwhile, changes to qapi-visit.c look like:

| static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp)
| {
|     Error *err = NULL;
|
|-    visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
|+    visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err);
|     if (err) {

(the cast is necessary, since our upcast wrappers only deal with a
single pointer, not pointer-to-pointer); plus the wholesale
elimination of some now-unused visit_type_implicit_FOO() functions.

Without boxing, the corner case of one empty struct having
another empty struct as its base type now requires inserting a
dummy member (previously, the 'Base *base' member sufficed).

And now that we no longer consume a 'base' member in the generated
C struct, we can delete the former negative struct-base-clash-base
test.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-11-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp.c                                         |  6 +++---
 scripts/qapi-types.py                         | 12 ++++--------
 scripts/qapi-visit.py                         |  9 ++++-----
 tests/Makefile                                |  1 -
 tests/qapi-schema/struct-base-clash-base.err  |  0
 tests/qapi-schema/struct-base-clash-base.exit |  1 -
 tests/qapi-schema/struct-base-clash-base.json |  9 ---------
 tests/qapi-schema/struct-base-clash-base.out  |  5 -----
 tests/test-qmp-commands.c                     | 15 +++++----------
 tests/test-qmp-event.c                        |  8 ++------
 tests/test-qmp-input-visitor.c                |  4 ++--
 tests/test-qmp-output-visitor.c               | 13 +++++--------
 tests/test-visitor-serialization.c            | 14 ++++++--------
 ui/spice-core.c                               | 23 +++++++++++++----------
 ui/vnc.c                                      | 21 ++++++++++-----------
 15 files changed, 54 insertions(+), 87 deletions(-)
 delete mode 100644 tests/qapi-schema/struct-base-clash-base.err
 delete mode 100644 tests/qapi-schema/struct-base-clash-base.exit
 delete mode 100644 tests/qapi-schema/struct-base-clash-base.json
 delete mode 100644 tests/qapi-schema/struct-base-clash-base.out

(limited to 'scripts')

diff --git a/hmp.c b/hmp.c
index 5048eeeb2d..88fd804a9e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -569,8 +569,8 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
         for (client = info->clients; client; client = client->next) {
             monitor_printf(mon, "Client:\n");
             monitor_printf(mon, "     address: %s:%s\n",
-                           client->value->base->host,
-                           client->value->base->service);
+                           client->value->host,
+                           client->value->service);
             monitor_printf(mon, "  x509_dname: %s\n",
                            client->value->x509_dname ?
                            client->value->x509_dname : "none");
@@ -638,7 +638,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
         for (chan = info->channels; chan; chan = chan->next) {
             monitor_printf(mon, "Channel:\n");
             monitor_printf(mon, "     address: %s:%s%s\n",
-                           chan->value->base->host, chan->value->base->port,
+                           chan->value->host, chan->value->port,
                            chan->value->tls ? " [tls]" : "");
             monitor_printf(mon, "     session: %" PRId64 "\n",
                            chan->value->connection_id);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f9fcf150a4..faf7022e2c 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -77,16 +77,13 @@ struct %(c_name)s {
 ''',
                 c_name=c_name(name))
 
-    if base:
-        ret += gen_struct_field('base', base, False)
-
-    ret += gen_struct_fields(members)
+    ret += gen_struct_fields(members, base)
 
     # Make sure that all structs have at least one field; this avoids
     # potential issues with attempting to malloc space for zero-length
     # structs in C, and also incompatibility with C++ (where an empty
     # struct is size 1).
-    if not base and not members:
+    if not (base and base.members) and not members:
         ret += mcgen('''
     char qapi_dummy_field_for_empty_struct;
 ''')
@@ -280,11 +277,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         if variants:
             assert not members      # not implemented
             self.decl += gen_union(name, base, variants)
-            # TODO Use gen_upcast on structs too, once they have sane layout
-            if base:
-                self.decl += gen_upcast(name, base)
         else:
             self.decl += gen_struct(name, base, members)
+        if base:
+            self.decl += gen_upcast(name, base)
         self._gen_type_cleanup(name)
 
     def visit_alternate_type(self, name, info, variants):
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d4408f25b1..f711a720f3 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -72,13 +72,12 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
 
 
 def gen_visit_struct_fields(name, base, members):
-    struct_fields_seen.add(name)
-
     ret = ''
 
     if base:
-        ret += gen_visit_implicit_struct(base)
+        ret += gen_visit_fields_decl(base)
 
+    struct_fields_seen.add(name)
     ret += mcgen('''
 
 static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
@@ -90,9 +89,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 
     if base:
         ret += mcgen('''
-    visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+    visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
 ''',
-                     c_type=base.c_name(), c_name=c_name('base'))
+                     c_type=base.c_name())
         ret += gen_err_check()
 
     ret += gen_visit_fields(members, prefix='(*obj)->')
diff --git a/tests/Makefile b/tests/Makefile
index 652294cccd..fd4ec03e09 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -325,7 +325,6 @@ qapi-schema += returns-array-bad.json
 qapi-schema += returns-dict.json
 qapi-schema += returns-unknown.json
 qapi-schema += returns-whitelist.json
-qapi-schema += struct-base-clash-base.json
 qapi-schema += struct-base-clash-deep.json
 qapi-schema += struct-base-clash.json
 qapi-schema += struct-data-invalid.json
diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err
deleted file mode 100644
index e69de29bb2..0000000000
diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit
deleted file mode 100644
index 573541ac97..0000000000
--- a/tests/qapi-schema/struct-base-clash-base.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json
deleted file mode 100644
index 0c840258c9..0000000000
--- a/tests/qapi-schema/struct-base-clash-base.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# Struct member 'base'
-# FIXME: this parses, but then fails to compile due to a duplicate 'base'
-# (one explicit in QMP, the other used to box the base class members).
-# We should either reject the collision at parse time, or change the
-# generated struct to allow this to compile.
-{ 'struct': 'Base', 'data': {} }
-{ 'struct': 'Sub',
-  'base': 'Base',
-  'data': { 'base': 'str' } }
diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out
deleted file mode 100644
index e69a416560..0000000000
--- a/tests/qapi-schema/struct-base-clash-base.out
+++ /dev/null
@@ -1,5 +0,0 @@
-object :empty
-object Base
-object Sub
-    base Base
-    member base: str optional=False
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index bc59835835..ea700d890d 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -25,11 +25,9 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
     UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
 
     ud1c->string = strdup(ud1a->string);
-    ud1c->base = g_new0(UserDefZero, 1);
-    ud1c->base->integer = ud1a->base->integer;
+    ud1c->integer = ud1a->integer;
     ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
-    ud1d->base = g_new0(UserDefZero, 1);
-    ud1d->base->integer = has_udb1 ? ud1b->base->integer : 0;
+    ud1d->integer = has_udb1 ? ud1b->integer : 0;
 
     ret = g_new0(UserDefTwo, 1);
     ret->string0 = strdup("blah1");
@@ -176,20 +174,17 @@ static void test_dealloc_types(void)
     UserDefOneList *ud1list;
 
     ud1test = g_malloc0(sizeof(UserDefOne));
-    ud1test->base = g_new0(UserDefZero, 1);
-    ud1test->base->integer = 42;
+    ud1test->integer = 42;
     ud1test->string = g_strdup("hi there 42");
 
     qapi_free_UserDefOne(ud1test);
 
     ud1a = g_malloc0(sizeof(UserDefOne));
-    ud1a->base = g_new0(UserDefZero, 1);
-    ud1a->base->integer = 43;
+    ud1a->integer = 43;
     ud1a->string = g_strdup("hi there 43");
 
     ud1b = g_malloc0(sizeof(UserDefOne));
-    ud1b->base = g_new0(UserDefZero, 1);
-    ud1b->base->integer = 44;
+    ud1b->integer = 44;
     ud1b->string = g_strdup("hi there 44");
 
     ud1list = g_malloc0(sizeof(UserDefOneList));
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 28f146d4b7..035c65cfdf 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -179,9 +179,7 @@ static void test_event_c(TestEventData *data,
     QDict *d, *d_data, *d_b;
 
     UserDefOne b;
-    UserDefZero z;
-    z.integer = 2;
-    b.base = &z;
+    b.integer = 2;
     b.string = g_strdup("test1");
     b.has_enum1 = false;
 
@@ -209,11 +207,9 @@ static void test_event_d(TestEventData *data,
 {
     UserDefOne struct1;
     EventStructOne a;
-    UserDefZero z;
     QDict *d, *d_data, *d_a, *d_struct1;
 
-    z.integer = 2;
-    struct1.base = &z;
+    struct1.integer = 2;
     struct1.string = g_strdup("test1");
     struct1.has_enum1 = true;
     struct1.enum1 = ENUM_ONE_VALUE1;
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index da21709714..2d95db94e6 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -262,7 +262,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
 
     check_and_free_str(udp->string0, "string0");
     check_and_free_str(udp->dict1->string1, "string1");
-    g_assert_cmpint(udp->dict1->dict2->userdef->base->integer, ==, 42);
+    g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42);
     check_and_free_str(udp->dict1->dict2->userdef->string, "string");
     check_and_free_str(udp->dict1->dict2->string, "string2");
     g_assert(udp->dict1->has_dict3 == false);
@@ -292,7 +292,7 @@ static void test_visitor_in_list(TestInputVisitorData *data,
 
         snprintf(string, sizeof(string), "string%d", i);
         g_assert_cmpstr(item->value->string, ==, string);
-        g_assert_cmpint(item->value->base->integer, ==, 42 + i);
+        g_assert_cmpint(item->value->integer, ==, 42 + i);
     }
 
     qapi_free_UserDefOneList(head);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index c84002e2f2..cfb06bbbc6 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -250,16 +250,14 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
     ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
     ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1);
     ud2->dict1->dict2->userdef->string = g_strdup(string);
-    ud2->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
-    ud2->dict1->dict2->userdef->base->integer = value;
+    ud2->dict1->dict2->userdef->integer = value;
     ud2->dict1->dict2->string = g_strdup(strings[2]);
 
     ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
     ud2->dict1->has_dict3 = true;
     ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1);
     ud2->dict1->dict3->userdef->string = g_strdup(string);
-    ud2->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
-    ud2->dict1->dict3->userdef->base->integer = value;
+    ud2->dict1->dict3->userdef->integer = value;
     ud2->dict1->dict3->string = g_strdup(strings[3]);
 
     visit_type_UserDefTwo(data->ov, &ud2, "unused", &err);
@@ -301,8 +299,8 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
                                            const void *unused)
 {
     EnumOne bad_values[] = { ENUM_ONE_MAX, -1 };
-    UserDefZero b;
-    UserDefOne u = { .base = &b }, *pu = &u;
+    UserDefOne u = {0};
+    UserDefOne *pu = &u;
     Error *err;
     int i;
 
@@ -416,8 +414,7 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
         p->value->dict1->dict2 = g_new0(UserDefTwoDictDict, 1);
         p->value->dict1->dict2->userdef = g_new0(UserDefOne, 1);
         p->value->dict1->dict2->userdef->string = g_strdup(string);
-        p->value->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
-        p->value->dict1->dict2->userdef->base->integer = 42;
+        p->value->dict1->dict2->userdef->integer = 42;
         p->value->dict1->dict2->string = g_strdup(string);
         p->value->dict1->has_dict3 = false;
 
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index fa86cae88a..634563bae4 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -258,15 +258,13 @@ static UserDefTwo *nested_struct_create(void)
     udnp->dict1->string1 = strdup("test_string1");
     udnp->dict1->dict2 = g_malloc0(sizeof(*udnp->dict1->dict2));
     udnp->dict1->dict2->userdef = g_new0(UserDefOne, 1);
-    udnp->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
-    udnp->dict1->dict2->userdef->base->integer = 42;
+    udnp->dict1->dict2->userdef->integer = 42;
     udnp->dict1->dict2->userdef->string = strdup("test_string");
     udnp->dict1->dict2->string = strdup("test_string2");
     udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3));
     udnp->dict1->has_dict3 = true;
     udnp->dict1->dict3->userdef = g_new0(UserDefOne, 1);
-    udnp->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
-    udnp->dict1->dict3->userdef->base->integer = 43;
+    udnp->dict1->dict3->userdef->integer = 43;
     udnp->dict1->dict3->userdef->string = strdup("test_string");
     udnp->dict1->dict3->string = strdup("test_string3");
     return udnp;
@@ -278,15 +276,15 @@ static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2)
     g_assert(udnp2);
     g_assert_cmpstr(udnp1->string0, ==, udnp2->string0);
     g_assert_cmpstr(udnp1->dict1->string1, ==, udnp2->dict1->string1);
-    g_assert_cmpint(udnp1->dict1->dict2->userdef->base->integer, ==,
-                    udnp2->dict1->dict2->userdef->base->integer);
+    g_assert_cmpint(udnp1->dict1->dict2->userdef->integer, ==,
+                    udnp2->dict1->dict2->userdef->integer);
     g_assert_cmpstr(udnp1->dict1->dict2->userdef->string, ==,
                     udnp2->dict1->dict2->userdef->string);
     g_assert_cmpstr(udnp1->dict1->dict2->string, ==,
                     udnp2->dict1->dict2->string);
     g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3);
-    g_assert_cmpint(udnp1->dict1->dict3->userdef->base->integer, ==,
-                    udnp2->dict1->dict3->userdef->base->integer);
+    g_assert_cmpint(udnp1->dict1->dict3->userdef->integer, ==,
+                    udnp2->dict1->dict3->userdef->integer);
     g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==,
                     udnp2->dict1->dict3->userdef->string);
     g_assert_cmpstr(udnp1->dict1->dict3->string, ==,
diff --git a/ui/spice-core.c b/ui/spice-core.c
index bf4fd07499..6a62d712fe 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -200,8 +200,6 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
 {
     SpiceServerInfo *server = g_malloc0(sizeof(*server));
     SpiceChannel *client = g_malloc0(sizeof(*client));
-    server->base = g_malloc0(sizeof(*server->base));
-    client->base = g_malloc0(sizeof(*client->base));
 
     /*
      * Spice server might have called us from spice worker thread
@@ -218,9 +216,11 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
     }
 
     if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
-        add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
+        add_addr_info(qapi_SpiceChannel_base(client),
+                      (struct sockaddr *)&info->paddr_ext,
                       info->plen_ext);
-        add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
+        add_addr_info(qapi_SpiceServerInfo_base(server),
+                      (struct sockaddr *)&info->laddr_ext,
                       info->llen_ext);
     } else {
         error_report("spice: %s, extended address is expected",
@@ -229,7 +229,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
 
     switch (event) {
     case SPICE_CHANNEL_EVENT_CONNECTED:
-        qapi_event_send_spice_connected(server->base, client->base, &error_abort);
+        qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
+                                        qapi_SpiceChannel_base(client),
+                                        &error_abort);
         break;
     case SPICE_CHANNEL_EVENT_INITIALIZED:
         if (auth) {
@@ -242,7 +244,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
         break;
     case SPICE_CHANNEL_EVENT_DISCONNECTED:
         channel_list_del(info);
-        qapi_event_send_spice_disconnected(server->base, client->base, &error_abort);
+        qapi_event_send_spice_disconnected(qapi_SpiceServerInfo_base(server),
+                                           qapi_SpiceChannel_base(client),
+                                           &error_abort);
         break;
     default:
         break;
@@ -378,16 +382,15 @@ static SpiceChannelList *qmp_query_spice_channels(void)
 
         chan = g_malloc0(sizeof(*chan));
         chan->value = g_malloc0(sizeof(*chan->value));
-        chan->value->base = g_malloc0(sizeof(*chan->value->base));
 
         paddr = (struct sockaddr *)&item->info->paddr_ext;
         plen = item->info->plen_ext;
         getnameinfo(paddr, plen,
                     host, sizeof(host), port, sizeof(port),
                     NI_NUMERICHOST | NI_NUMERICSERV);
-        chan->value->base->host = g_strdup(host);
-        chan->value->base->port = g_strdup(port);
-        chan->value->base->family = inet_netfamily(paddr->sa_family);
+        chan->value->host = g_strdup(host);
+        chan->value->port = g_strdup(port);
+        chan->value->family = inet_netfamily(paddr->sa_family);
 
         chan->value->connection_id = item->info->connection_id;
         chan->value->channel_type = item->info->type;
diff --git a/ui/vnc.c b/ui/vnc.c
index 502a10a07b..cec2cee993 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -262,8 +262,8 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
     Error *err = NULL;
 
     info = g_malloc(sizeof(*info));
-    info->base = g_malloc(sizeof(*info->base));
-    vnc_init_basic_info_from_server_addr(vd->lsock, info->base, &err);
+    vnc_init_basic_info_from_server_addr(vd->lsock,
+                                         qapi_VncServerInfo_base(info), &err);
     info->has_auth = true;
     info->auth = g_strdup(vnc_auth_name(vd));
     if (err) {
@@ -300,8 +300,8 @@ static void vnc_client_cache_addr(VncState *client)
     Error *err = NULL;
 
     client->info = g_malloc0(sizeof(*client->info));
-    client->info->base = g_malloc0(sizeof(*client->info->base));
-    vnc_init_basic_info_from_remote_addr(client->csock, client->info->base,
+    vnc_init_basic_info_from_remote_addr(client->csock,
+                                         qapi_VncClientInfo_base(client->info),
                                          &err);
     if (err) {
         qapi_free_VncClientInfo(client->info);
@@ -317,7 +317,6 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
     if (!vs->info) {
         return;
     }
-    g_assert(vs->info->base);
 
     si = vnc_server_info_get(vs->vd);
     if (!si) {
@@ -326,7 +325,8 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
 
     switch (event) {
     case QAPI_EVENT_VNC_CONNECTED:
-        qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
+        qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info),
+                                      &error_abort);
         break;
     case QAPI_EVENT_VNC_INITIALIZED:
         qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
@@ -361,11 +361,10 @@ static VncClientInfo *qmp_query_vnc_client(const VncState *client)
     }
 
     info = g_malloc0(sizeof(*info));
-    info->base = g_malloc0(sizeof(*info->base));
-    info->base->host = g_strdup(host);
-    info->base->service = g_strdup(serv);
-    info->base->family = inet_netfamily(sa.ss_family);
-    info->base->websocket = client->websocket;
+    info->host = g_strdup(host);
+    info->service = g_strdup(serv);
+    info->family = inet_netfamily(sa.ss_family);
+    info->websocket = client->websocket;
 
     if (client->tls) {
         info->x509_dname = qcrypto_tls_session_get_peer_name(client->tls);
-- 
cgit v1.2.3-55-g7522


From 5c5e51a05b567fd48fb155d94ca6f7679dd0d478 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:50 -0600
Subject: qapi-visit: Remove redundant functions for flat union base

The code for visiting the base class of a child struct created
visit_type_Base_fields() which covers all fields of Base; while
the code for visiting the base class of a flat union created
visit_type_Union_fields() covering all fields of the base
except the discriminator.  But since the base class includes
the discriminator of a flat union, we can just visit the entire
base, without needing a separate visit of the discriminator.
Not only is consistently visiting all fields easier to
understand, it lets us share code.

The generated code in qapi-visit.c loses several now-unused
visit_type_UNION_fields(), along with changes like:

|@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor
|     if (!*obj) {
|         goto out_obj;
|     }
|-    visit_type_BlockdevOptions_fields(v, obj, &err);
|-    if (err) {
|-        goto out_obj;
|-    }
|-    visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err);
|+    visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj, &err);
|     if (err) {
|         goto out_obj;
|     }

and forward declarations where needed.  Note that the cast of obj
to BASE ** is necessary to call visit_type_BASE_fields() (and we
can't use our upcast wrappers, because those work on pointers while
we have a pointer-to-pointer).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-12-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f711a720f3..33c013a7fb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -228,8 +228,7 @@ def gen_visit_union(name, base, variants):
     ret = ''
 
     if base:
-        members = [m for m in base.members if m != variants.tag_member]
-        ret += gen_visit_struct_fields(name, None, members)
+        ret += gen_visit_fields_decl(base)
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
@@ -254,31 +253,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
 
     if base:
         ret += mcgen('''
-    visit_type_%(c_name)s_fields(v, obj, &err);
+    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
 ''',
-                     c_name=c_name(name))
-        ret += gen_err_check(label='out_obj')
-
-    tag_key = variants.tag_member.name
-    if not variants.tag_name:
-        # we pointlessly use a different key for simple unions
-        tag_key = 'type'
-    ret += mcgen('''
+                     c_name=base.c_name())
+    else:
+        ret += mcgen('''
     visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-    if (err) {
-        goto out_obj;
-    }
+''',
+                     c_type=variants.tag_member.type.c_name(),
+                     # TODO ugly special case for simple union
+                     # Use same tag name in C as on the wire to get rid of
+                     # it, then: c_name=c_name(variants.tag_member.name)
+                     c_name='kind',
+                     name=variants.tag_member.name)
+    ret += gen_err_check(label='out_obj')
+    ret += mcgen('''
     if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
         goto out_obj;
     }
     switch ((*obj)->%(c_name)s) {
 ''',
-                 c_type=variants.tag_member.type.c_name(),
                  # TODO ugly special case for simple union
                  # Use same tag name in C as on the wire to get rid of
                  # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'),
-                 name=tag_key)
+                 c_name=c_name(variants.tag_name or 'kind'))
 
     for var in variants.variants:
         # TODO ugly special case for simple union
-- 
cgit v1.2.3-55-g7522


From f51d8fab44b231aa299d8de24cfdf9ba41ef4a21 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:51 -0600
Subject: qapi: Start converting to new qapi union layout

We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a non-variant
member's name.

This patch is the front end for a series that converts to a
saner qapi union layout.  By the end of the series, we will no
longer have the type/kind mismatch, and all tag values will be
under a named union, which requires clients to access
'obj->u.value' instead of 'obj->value'.  But since the
conversion touches a number of files, it is easiest if we
temporarily support BOTH layouts simultaneously.

Given a simple union qapi type:

{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }

make the following changes in generated qapi-types.h:

| struct Foo {
|-    FooKind kind;
|-    union { /* union tag is @kind */
|+    union {
|+        FooKind kind;
|+        FooKind type;
|+    };
|+    union { /* union tag is @type */
|         void *data;
|         int64_t a;
|         bool b;
|+        union { /* union tag is @type */
|+            void *data;
|+            int64_t a;
|+            bool b;
|+        } u;
|     };
| };

Flat unions do not need the anonymous union for the tag member,
as we already fixed that to use the member name instead of 'kind'
back in commit 0f61af3e.

One additional change is needed in qapi.py: check_union() now
needs to check for collisions with 'type' in addition to those
with 'kind'.

Later, when the conversions are complete, we will remove the
duplication hacks, and also drop the check_union() restrictions.

Note, however, that we do not rename the generated enum, which
is still 'FooKind'.  A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-13-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 26 +++++++++++++++++++-------
 scripts/qapi.py       |  3 ++-
 2 files changed, 21 insertions(+), 8 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index faf7022e2c..1420e00ffb 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -149,11 +149,23 @@ struct %(c_name)s {
     if base:
         ret += gen_struct_fields([], base)
     else:
+        # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
+        # want to use only 'type', but the conversion is large enough to
+        # require staging over several commits.
         ret += mcgen('''
-    %(c_type)s kind;
+    union {
+        %(c_type)s kind;
+        %(c_type)s type;
+    };
 ''',
                      c_type=c_name(variants.tag_member.type.name))
 
+    # TODO As a hack, we emit the union twice, once as an anonymous union
+    # and once as a named union.  Ultimately, we want to use only the
+    # named union version (as it avoids conflicts between tag values as
+    # branch names competing with non-variant QMP names), but the conversion
+    # is large enough to require staging over several commits.
+    tmp = ''
     # 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
@@ -162,25 +174,25 @@ struct %(c_name)s {
     # should not be any data leaks even without a data pointer.  Or, if
     # 'data' is merely added to guarantee we don't have an empty union,
     # shouldn't we enforce that at .json parse time?
-    ret += mcgen('''
+    tmp += mcgen('''
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'))
+                 c_name=c_name(variants.tag_member.name))
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
         typ = var.simple_union_type() or var.type
-        ret += mcgen('''
+        tmp += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
                      c_type=typ.c_type(),
                      c_name=c_name(var.name))
 
+    ret += tmp
+    ret += '    ' + '\n    '.join(tmp.split('\n'))
     ret += mcgen('''
+    } u;
     };
 };
 ''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3ff7b11e61..00a16203df 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -548,7 +548,8 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
+    values = {'MAX': '(automatic)', 'KIND': '(automatic)',
+              'TYPE': '(automatic)'}
 
     # Two types of unions, determined by discriminator.
 
-- 
cgit v1.2.3-55-g7522


From 150d0564a4c626642897c748f7906260a13c14e1 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:34:52 -0600
Subject: qapi-visit: Convert to new qapi union layout

We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a non-variant
member's name.

Make the conversion to the new layout for qapi-visit.py.

Generated code changes look like:

|@@ -4912,16 +4912,16 @@ void visit_type_MemoryDeviceInfo(Visitor
|     if (!*obj) {
|         goto out_obj;
|     }
|-    visit_type_MemoryDeviceInfoKind(v, &(*obj)->kind, "type", &err);
|+    visit_type_MemoryDeviceInfoKind(v, &(*obj)->type, "type", &err);
|     if (err) {
|         goto out_obj;
|     }
|-    if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
|+    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
|         goto out_obj;
|     }
|-    switch ((*obj)->kind) {
|+    switch ((*obj)->type) {
|     case MEMORY_DEVICE_INFO_KIND_DIMM:
|-        visit_type_PCDIMMDeviceInfo(v, &(*obj)->dimm, "data", &err);
|+        visit_type_PCDIMMDeviceInfo(v, &(*obj)->u.dimm, "data", &err);
|         break;
|     default:
|         abort();
|@@ -4930,7 +4930,7 @@ out_obj:
|     error_propagate(errp, err);
|     err = NULL;
|     if (*obj) {
|-        visit_end_union(v, !!(*obj)->data, &err);
|+        visit_end_union(v, !!(*obj)->u.data, &err);
|     }
|     error_propagate(errp, err);
|     err = NULL;

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-14-git-send-email-eblake@redhat.com>
[Commit message tweaked slightly]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 33c013a7fb..f40c3c792f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -189,18 +189,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     if (err) {
         goto out;
     }
-    visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+    visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
     if (err) {
         goto out_obj;
     }
-    switch ((*obj)->kind) {
+    switch ((*obj)->type) {
 ''',
                 c_name=c_name(name))
 
     for var in variants.variants:
         ret += mcgen('''
     case %(case)s:
-        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
+        visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
         break;
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
@@ -261,22 +261,16 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
 ''',
                      c_type=variants.tag_member.type.c_name(),
-                     # TODO ugly special case for simple union
-                     # Use same tag name in C as on the wire to get rid of
-                     # it, then: c_name=c_name(variants.tag_member.name)
-                     c_name='kind',
+                     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)->data, &err) || err) {
+    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
         goto out_obj;
     }
     switch ((*obj)->%(c_name)s) {
 ''',
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'))
+                 c_name=c_name(variants.tag_member.name))
 
     for var in variants.variants:
         # TODO ugly special case for simple union
@@ -288,13 +282,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                                        var.name))
         if simple_union_type:
             ret += mcgen('''
-        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
+        visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &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)->%(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))
@@ -310,7 +304,7 @@ out_obj:
     error_propagate(errp, err);
     err = NULL;
     if (*obj) {
-        visit_end_union(v, !!(*obj)->data, &err);
+        visit_end_union(v, !!(*obj)->u.data, &err);
     }
     error_propagate(errp, err);
     err = NULL;
-- 
cgit v1.2.3-55-g7522


From e4ba22b31943ab02373359555bd7bcd66442632f Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:35:01 -0600
Subject: qapi: Finish converting to new qapi union layout

We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a non-variant
member's name.

This patch is the back end for a series that converts to a
saner qapi union layout.  Now that all clients have been
converted to use 'type' and 'obj->u.value', we can drop the
temporary parallel support for 'kind' and 'obj->value'.

Given a simple union qapi type:

{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }

this is the overall effect, when compared to the state before
this series of patches:

| struct Foo {
|-    FooKind kind;
|-    union { /* union tag is @kind */
|+    FooKind type;
|+    union { /* union tag is @type */
|         void *data;
|         int64_t a;
|         bool b;
|-    };
|+    } u;
| };

The testsuite still contains some examples of artificial restrictions
(see flat-union-clash-type.json, for example) that are no longer
technically necessary, now that there is no longer a collision between
enum tag values and non-variant member names; but fixing this will be
done in later patches, in part because some further changes are required
to keep QAPISchema*.check() from asserting.  Also, a later patch will
add a reservation for the member name 'u' to avoid a collision between a
user's non-variant names and our internal choice of C union name.

Note, however, that we do not rename the generated enum, which
is still 'FooKind'.  A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-23-git-send-email-eblake@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 1420e00ffb..7e35bb6ac7 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -149,23 +149,10 @@ struct %(c_name)s {
     if base:
         ret += gen_struct_fields([], base)
     else:
-        # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
-        # want to use only 'type', but the conversion is large enough to
-        # require staging over several commits.
-        ret += mcgen('''
-    union {
-        %(c_type)s kind;
-        %(c_type)s type;
-    };
-''',
-                     c_type=c_name(variants.tag_member.type.name))
-
-    # TODO As a hack, we emit the union twice, once as an anonymous union
-    # and once as a named union.  Ultimately, we want to use only the
-    # named union version (as it avoids conflicts between tag values as
-    # branch names competing with non-variant QMP names), but the conversion
-    # is large enough to require staging over several commits.
-    tmp = ''
+        ret += gen_struct_field(variants.tag_member.name,
+                                variants.tag_member.type,
+                                False)
+
     # 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
@@ -174,7 +161,7 @@ struct %(c_name)s {
     # should not be any data leaks even without a data pointer.  Or, if
     # 'data' is merely added to guarantee we don't have an empty union,
     # shouldn't we enforce that at .json parse time?
-    tmp += mcgen('''
+    ret += mcgen('''
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
@@ -183,17 +170,14 @@ struct %(c_name)s {
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
         typ = var.simple_union_type() or var.type
-        tmp += mcgen('''
+        ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
                      c_type=typ.c_type(),
                      c_name=c_name(var.name))
 
-    ret += tmp
-    ret += '    ' + '\n    '.join(tmp.split('\n'))
     ret += mcgen('''
     } u;
-    };
 };
 ''')
 
-- 
cgit v1.2.3-55-g7522


From 5e59baf90a72cd25d38a3134edc029f4f022da74 Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:35:02 -0600
Subject: qapi: Reserve 'u' member name

Now that we have separated union tag values from colliding with
non-variant C names, by naming the union 'u', we should reserve
this name for our use.  Note that we want to forbid 'u' even in
a struct with no variants, because it is possible for a future
qemu release to extend QMP in a backwards-compatible manner while
converting from a struct to a flat union.  Fortunately, no
existing clients were using this member name.  If we ever find
the need for QMP to have a member 'u', we could at that time
relax things, perhaps by having c_name() munge the QMP member to
'q_u'.

Note that we cannot forbid 'u' everywhere (by adding the
rejection code to check_name()), because the existing QKeyCode
enum already uses it; therefore we only reserve it as a struct
type member name.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-24-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                          | 2 +-
 tests/Makefile                           | 1 +
 tests/qapi-schema/reserved-member-u.err  | 1 +
 tests/qapi-schema/reserved-member-u.exit | 1 +
 tests/qapi-schema/reserved-member-u.json | 7 +++++++
 tests/qapi-schema/reserved-member-u.out  | 0
 6 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/reserved-member-u.err
 create mode 100644 tests/qapi-schema/reserved-member-u.exit
 create mode 100644 tests/qapi-schema/reserved-member-u.json
 create mode 100644 tests/qapi-schema/reserved-member-u.out

(limited to 'scripts')

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 00a16203df..7c50cc4c87 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -490,7 +490,7 @@ def check_type(expr_info, source, value, allow_array=False,
     for (key, arg) in value.items():
         check_name(expr_info, "Member of %s" % source, key,
                    allow_optional=allow_optional)
-        if c_name(key, False).startswith('has_'):
+        if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
             raise QAPIExprError(expr_info,
                                 "Member of %s uses reserved name '%s'"
                                 % (source, key))
diff --git a/tests/Makefile b/tests/Makefile
index fd4ec03e09..92969e8288 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -318,6 +318,7 @@ qapi-schema += redefined-type.json
 qapi-schema += reserved-command-q.json
 qapi-schema += reserved-member-has.json
 qapi-schema += reserved-member-q.json
+qapi-schema += reserved-member-u.json
 qapi-schema += reserved-type-kind.json
 qapi-schema += reserved-type-list.json
 qapi-schema += returns-alternate.json
diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err
new file mode 100644
index 0000000000..87d42296cc
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-u.json:7: Member of 'data' for struct 'Oops' uses reserved name 'u'
diff --git a/tests/qapi-schema/reserved-member-u.exit b/tests/qapi-schema/reserved-member-u.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json
new file mode 100644
index 0000000000..1eaf0f301c
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.json
@@ -0,0 +1,7 @@
+# Potential C member name collision
+# We reject use of 'u' as a member name, to allow it for internal use in
+# putting union branch members in a separate namespace from QMP members.
+# This is true even for non-unions, because it is possible to convert a
+# struct to flat union while remaining backwards compatible in QMP.
+# TODO - we could munge the member name to 'q_u' to avoid the collision
+{ 'struct': 'Oops', 'data': { 'u': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out
new file mode 100644
index 0000000000..e69de29bb2
-- 
cgit v1.2.3-55-g7522


From 32bc6879beea0b0cac6196cb15a71d206401e96d Mon Sep 17 00:00:00 2001
From: Eric Blake
Date: Mon, 26 Oct 2015 16:35:03 -0600
Subject: qapi: Simplify gen_struct_field()

Rather than having all callers pass a name, type, and optional
flag, have them instead pass a QAPISchemaObjectTypeMember which
already has all that information.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1445898903-12082-25-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

(limited to 'scripts')

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7e35bb6ac7..b37900f6fc 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -36,18 +36,18 @@ struct %(c_name)s {
                  c_name=c_name(name), c_type=element_type.c_type())
 
 
-def gen_struct_field(name, typ, optional):
+def gen_struct_field(member):
     ret = ''
 
-    if optional:
+    if member.optional:
         ret += mcgen('''
     bool has_%(c_name)s;
 ''',
-                     c_name=c_name(name))
+                     c_name=c_name(member.name))
     ret += mcgen('''
     %(c_type)s %(c_name)s;
 ''',
-                 c_type=typ.c_type(), c_name=c_name(name))
+                 c_type=member.type.c_type(), c_name=c_name(member.name))
     return ret
 
 
@@ -60,13 +60,13 @@ def gen_struct_fields(local_members, base=None):
 ''',
                      c_name=base.c_name())
         for memb in base.members:
-            ret += gen_struct_field(memb.name, memb.type, memb.optional)
+            ret += gen_struct_field(memb)
         ret += mcgen('''
     /* Own members: */
 ''')
 
     for memb in local_members:
-        ret += gen_struct_field(memb.name, memb.type, memb.optional)
+        ret += gen_struct_field(memb)
     return ret
 
 
@@ -149,9 +149,7 @@ struct %(c_name)s {
     if base:
         ret += gen_struct_fields([], base)
     else:
-        ret += gen_struct_field(variants.tag_member.name,
-                                variants.tag_member.type,
-                                False)
+        ret += gen_struct_field(variants.tag_member)
 
     # 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
-- 
cgit v1.2.3-55-g7522