Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-12-17' into staging

QAPI patches for 2015-12-17

# gpg: Signature made Thu 17 Dec 2015 07:33:41 GMT using RSA key ID EB918653
# gpg: Good signature from "Markus Armbruster <armbru@redhat.com>"
# gpg:                 aka "Markus Armbruster <armbru@pond.sub.org>"

* remotes/armbru/tags/pull-qapi-2015-12-17: (40 commits)
  qapi: Detect base class loops
  qapi: Move duplicate collision checks to schema check()
  qapi: Enforce (or whitelist) case conventions on qapi members
  qapi: Track enum values by QAPISchemaMember, not string
  qapi: Prepare new QAPISchemaMember base class
  qapi: Shorter visits of optional fields
  qapi: Simplify visits of optional fields
  qapi: Fix alternates that accept 'number' but not 'int'
  qapi: Inline _make_implicit_tag()
  qapi-types: Drop unnedeed ._fwdefn
  qapi: Simplify visiting of alternate types
  qapi: Convert QType into QAPI built-in enum type
  qobject: Rename qtype_code to QType
  qobject: Simplify QObject
  qapi: Change munging of CamelCase enum values
  qapi: Add alias for ErrorClass
  cpu: Convert CpuInfo into flat union
  qapi: Remove obsolete tests for MAX collision
  qapi: Don't let implicit enum MAX member collide
  qapi: Tighten the regex on valid names
  ...

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

3 files changed, 232 insertions, 219 deletions

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b37900f..0d86269 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -36,54 +36,47 @@ struct %(c_name)s {
                  c_name=c_name(name), c_type=element_type.c_type())
 
 
-def gen_struct_field(member):
+def gen_struct_fields(members):
     ret = ''
-
-    if member.optional:
-        ret += mcgen('''
+    for memb in members:
+        if memb.optional:
+            ret += mcgen('''
     bool has_%(c_name)s;
 ''',
-                     c_name=c_name(member.name))
-    ret += mcgen('''
+                         c_name=c_name(memb.name))
+        ret += mcgen('''
     %(c_type)s %(c_name)s;
 ''',
-                 c_type=member.type.c_type(), c_name=c_name(member.name))
+                     c_type=memb.type.c_type(), c_name=c_name(memb.name))
     return ret
 
 
-def gen_struct_fields(local_members, base=None):
-    ret = ''
+def gen_object(name, base, members, variants):
+    ret = mcgen('''
+
+struct %(c_name)s {
+''',
+                c_name=c_name(name))
 
     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)
+        ret += gen_struct_fields(base.members)
         ret += mcgen('''
     /* Own members: */
 ''')
+    ret += gen_struct_fields(members)
 
-    for memb in local_members:
-        ret += gen_struct_field(memb)
-    return ret
-
-
-def gen_struct(name, base, members):
-    ret = mcgen('''
-
-struct %(c_name)s {
-''',
-                c_name=c_name(name))
-
-    ret += gen_struct_fields(members, base)
+    if variants:
+        ret += gen_variants(variants)
 
     # 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 base.members) and not members:
+    if not (base and base.members) and not members and not variants:
         ret += mcgen('''
     char qapi_dummy_field_for_empty_struct;
 ''')
@@ -108,49 +101,7 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
                  c_name=c_name(name), base=base.c_name())
 
 
-def gen_alternate_qtypes_decl(name):
-    return mcgen('''
-
-extern const int %(c_name)s_qtypes[];
-''',
-                 c_name=c_name(name))
-
-
-def gen_alternate_qtypes(name, variants):
-    ret = mcgen('''
-
-const int %(c_name)s_qtypes[QTYPE_MAX] = {
-''',
-                c_name=c_name(name))
-
-    for var in variants.variants:
-        qtype = var.type.alternate_qtype()
-        assert qtype
-
-        ret += mcgen('''
-    [%(qtype)s] = %(enum_const)s,
-''',
-                     qtype=qtype,
-                     enum_const=c_enum_const(variants.tag_member.type.name,
-                                             var.name))
-
-    ret += mcgen('''
-};
-''')
-    return ret
-
-
-def gen_union(name, base, variants):
-    ret = mcgen('''
-
-struct %(c_name)s {
-''',
-                c_name=c_name(name))
-    if base:
-        ret += gen_struct_fields([], base)
-    else:
-        ret += gen_struct_field(variants.tag_member)
-
+def gen_variants(variants):
     # 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
@@ -159,11 +110,11 @@ 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('''
+    ret = mcgen('''
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
-                 c_name=c_name(variants.tag_member.name))
+                c_name=c_name(variants.tag_member.name))
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
@@ -176,7 +127,6 @@ struct %(c_name)s {
 
     ret += mcgen('''
     } u;
-};
 ''')
 
     return ret
@@ -218,21 +168,17 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.decl = None
         self.defn = None
         self._fwdecl = None
-        self._fwdefn = None
         self._btin = None
 
     def visit_begin(self, schema):
         self.decl = ''
         self.defn = ''
         self._fwdecl = ''
-        self._fwdefn = ''
         self._btin = guardstart('QAPI_TYPES_BUILTIN')
 
     def visit_end(self):
         self.decl = self._fwdecl + self.decl
         self._fwdecl = None
-        self.defn = self._fwdefn + self.defn
-        self._fwdefn = None
         # To avoid header dependency hell, we always generate
         # declarations for built-in types in our header files and
         # simply guard them.  See also do_builtins (command line
@@ -251,8 +197,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.defn += gen_type_cleanup(name)
 
     def visit_enum_type(self, name, info, values, prefix):
-        self._fwdecl += gen_enum(name, values, prefix)
-        self._fwdefn += gen_enum_lookup(name, values, prefix)
+        # Special case for our lone builtin enum type
+        # TODO use something cleaner than existence of info
+        if not info:
+            self._btin += gen_enum(name, values, prefix)
+            if do_builtins:
+                self.defn += gen_enum_lookup(name, values, prefix)
+        else:
+            self._fwdecl += gen_enum(name, values, prefix)
+            self.defn += gen_enum_lookup(name, values, prefix)
 
     def visit_array_type(self, name, info, element_type):
         if isinstance(element_type, QAPISchemaBuiltinType):
@@ -268,20 +221,14 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 
     def visit_object_type(self, name, info, base, members, variants):
         self._fwdecl += gen_fwd_object_or_array(name)
-        if variants:
-            assert not members      # not implemented
-            self.decl += gen_union(name, base, variants)
-        else:
-            self.decl += gen_struct(name, base, members)
+        self.decl += gen_object(name, base, members, variants)
         if base:
             self.decl += gen_upcast(name, base)
         self._gen_type_cleanup(name)
 
     def visit_alternate_type(self, name, info, variants):
         self._fwdecl += gen_fwd_object_or_array(name)
-        self._fwdefn += gen_alternate_qtypes(name, variants)
-        self.decl += gen_union(name, None, variants)
-        self.decl += gen_alternate_qtypes_decl(name)
+        self.decl += gen_object(name, None, [variants.tag_member], variants)
         self._gen_type_cleanup(name)
 
 # If you link code generated from multiple schemata, you want only one
@@ -338,10 +285,11 @@ fdef.write(mcgen('''
 ''',
                  prefix=prefix))
 
+# To avoid circular headers, use only typedefs.h here, not qobject.h
 fdecl.write(mcgen('''
 #include <stdbool.h>
 #include <stdint.h>
-#include "qapi/qmp/qobject.h"
+#include "qemu/typedefs.h"
 '''))
 
 schema = QAPISchema(input_file)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3ef5c16..b93690b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,7 @@ out:
 
 
 def gen_visit_enum(name):
+    # FIXME cast from enum *obj to int * invalidly assumes enum is int
     return mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
@@ -183,6 +184,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error
 
 
 def gen_visit_alternate(name, variants):
+    promote_int = 'true'
+    for var in variants.variants:
+        if var.type.alternate_qtype() == 'QTYPE_QINT':
+            promote_int = 'false'
+
     ret = mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
@@ -193,13 +199,13 @@ 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)->type, %(c_name)s_qtypes, name, &err);
+    visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err);
     if (err) {
         goto out_obj;
     }
     switch ((*obj)->type) {
 ''',
-                c_name=c_name(name))
+                c_name=c_name(name), promote_int=promote_int)
 
     for var in variants.variants:
         ret += mcgen('''
@@ -207,14 +213,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
         break;
 ''',
-                     case=c_enum_const(variants.tag_member.type.name,
-                                       var.name),
+                     case=var.type.alternate_qtype(),
                      c_type=var.type.c_name(),
                      c_name=c_name(var.name))
 
     ret += mcgen('''
     default:
-        abort();
+        error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "%(name)s");
     }
 out_obj:
     error_propagate(errp, err);
@@ -223,7 +229,8 @@ out_obj:
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 name=name)
 
     return ret
 
@@ -347,8 +354,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
                     isinstance(entity, QAPISchemaObjectType))
 
     def visit_enum_type(self, name, info, values, prefix):
-        self.decl += gen_visit_decl(name, scalar=True)
-        self.defn += gen_visit_enum(name)
+        # Special case for our lone builtin enum type
+        # TODO use something cleaner than existence of info
+        if not info:
+            self._btin += gen_visit_decl(name, scalar=True)
+            if do_builtins:
+                self.defn += gen_visit_enum(name)
+        else:
+            self.decl += gen_visit_decl(name, scalar=True)
+            self.defn += gen_visit_enum(name)
 
     def visit_array_type(self, name, info, element_type):
         decl = gen_visit_decl(name)
@@ -364,7 +378,10 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_decl(name)
         if variants:
-            assert not members      # not implemented
+            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)
         else:
             self.defn += gen_visit_struct(name, base, members)
@@ -427,6 +444,7 @@ fdef.write(mcgen('''
 
 fdecl.write(mcgen('''
 #include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7c50cc4..7dec611 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -33,7 +33,8 @@ builtin_types = {
     'uint32':   'QTYPE_QINT',
     'uint64':   'QTYPE_QINT',
     'size':     'QTYPE_QINT',
-    'any':      None,           # any qtype_code possible, actually
+    'any':      None,           # any QType possible, actually
+    'QType':    'QTYPE_QSTRING',
 }
 
 # Whitelist of commands allowed to return a non-dictionary
@@ -58,6 +59,20 @@ returns_whitelist = [
     'guest-sync-delimited',
 ]
 
+# Whitelist of entities allowed to violate case conventions
+case_whitelist = [
+    # From QMP:
+    'ACPISlotType',         # DIMM, visible through query-acpi-ospm-status
+    'CpuInfoBase',          # CPU, visible through query-cpu
+    'CpuInfoMIPS',          # PC, visible through query-cpu
+    'CpuInfoTricore',       # PC, visible through query-cpu
+    'InputAxis',            # TODO: drop when x-input-send-event is fixed
+    'InputButton',          # TODO: drop when x-input-send-event is fixed
+    'QapiErrorClass',       # all members, visible through errors
+    'UuidInfo',             # UUID, visible through query-uuid
+    'X86CPURegister32',     # all members, visible indirectly through qom-get
+]
+
 enum_types = []
 struct_types = []
 union_types = []
@@ -353,9 +368,11 @@ def discriminator_find_enum_define(expr):
     return find_enum(discriminator_type)
 
 
-# FIXME should enforce "other than downstream extensions [...], all
-# names should begin with a letter".
-valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
+# Names must be letters, numbers, -, and _.  They must start with letter,
+# except for downstream extensions which must start with __RFQDN_.
+# Dots are only valid in the downstream extension prefix.
+valid_name = re.compile('^(__[a-zA-Z0-9.-]+_)?'
+                        '[a-zA-Z][a-zA-Z0-9_-]*$')
 
 
 def check_name(expr_info, source, name, allow_optional=False,
@@ -374,8 +391,8 @@ def check_name(expr_info, source, name, allow_optional=False,
                                 % (source, name))
     # Enum members can start with a digit, because the generated C
     # code always prefixes it with the enum name
-    if enum_member:
-        membername = '_' + membername
+    if enum_member and membername[0].isdigit():
+        membername = 'D' + membername
     # Reserve the entire 'q_' namespace for c_name()
     if not valid_name.match(membername) or \
        c_name(membername, False).startswith('q_'):
@@ -502,21 +519,6 @@ def check_type(expr_info, source, value, allow_array=False,
                                 'enum'])
 
 
-def check_member_clash(expr_info, base_name, data, source=""):
-    base = find_struct(base_name)
-    assert base
-    base_members = base['data']
-    for key in data.keys():
-        if key.startswith('*'):
-            key = key[1:]
-        if key in base_members or "*" + key in base_members:
-            raise QAPIExprError(expr_info,
-                                "Member name '%s'%s clashes with base '%s'"
-                                % (key, source, base_name))
-    if base.get('base'):
-        check_member_clash(expr_info, base['base'], data, source)
-
-
 def check_command(expr, expr_info):
     name = expr['command']
 
@@ -535,8 +537,6 @@ def check_event(expr, expr_info):
     global events
     name = expr['event']
 
-    if name.upper() == 'MAX':
-        raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
     events.append(name)
     check_type(expr_info, "'data' for event '%s'" % name,
                expr.get('data'), allow_dict=True, allow_optional=True,
@@ -548,8 +548,6 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = {'MAX': '(automatic)', 'KIND': '(automatic)',
-              'TYPE': '(automatic)'}
 
     # Two types of unions, determined by discriminator.
 
@@ -596,59 +594,29 @@ def check_union(expr, expr_info):
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)
 
-        # Each value must name a known type; furthermore, in flat unions,
-        # branches must be a struct with no overlapping member names
+        # Each value must name a known type
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
                    value, allow_array=not base, allow_metas=allow_metas)
-        if base:
-            branch_struct = find_struct(value)
-            assert branch_struct
-            check_member_clash(expr_info, base, branch_struct['data'],
-                               " of branch '%s'" % key)
 
         # If the discriminator names an enum type, then all members
-        # of 'data' must also be members of the enum type, which in turn
-        # must not collide with the discriminator name.
+        # of 'data' must also be members of the enum type.
         if enum_define:
             if key not in enum_define['enum_values']:
                 raise QAPIExprError(expr_info,
                                     "Discriminator value '%s' is not found in "
                                     "enum '%s'" %
                                     (key, enum_define["enum_name"]))
-            if discriminator in enum_define['enum_values']:
-                raise QAPIExprError(expr_info,
-                                    "Discriminator name '%s' collides with "
-                                    "enum value in '%s'" %
-                                    (discriminator, enum_define["enum_name"]))
-
-        # Otherwise, check for conflicts in the generated enum
-        else:
-            c_key = camel_to_upper(key)
-            if c_key in values:
-                raise QAPIExprError(expr_info,
-                                    "Union '%s' member '%s' clashes with '%s'"
-                                    % (name, key, values[c_key]))
-            values[c_key] = key
 
 
 def check_alternate(expr, expr_info):
     name = expr['alternate']
     members = expr['data']
-    values = {'MAX': '(automatic)'}
     types_seen = {}
 
     # Check every branch
     for (key, value) in members.items():
         check_name(expr_info, "Member of alternate '%s'" % name, key)
 
-        # Check for conflicts in the generated enum
-        c_key = camel_to_upper(key)
-        if c_key in values:
-            raise QAPIExprError(expr_info,
-                                "Alternate '%s' member '%s' clashes with '%s'"
-                                % (name, key, values[c_key]))
-        values[c_key] = key
-
         # Ensure alternates have no type conflicts.
         check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
                    value,
@@ -667,7 +635,6 @@ def check_enum(expr, expr_info):
     name = expr['enum']
     members = expr.get('data')
     prefix = expr.get('prefix')
-    values = {'MAX': '(automatic)'}
 
     if not isinstance(members, list):
         raise QAPIExprError(expr_info,
@@ -678,12 +645,6 @@ def check_enum(expr, expr_info):
     for member in members:
         check_name(expr_info, "Member of enum '%s'" % name, member,
                    enum_member=True)
-        key = camel_to_upper(member)
-        if key in values:
-            raise QAPIExprError(expr_info,
-                                "Enum '%s' member '%s' clashes with '%s'"
-                                % (name, member, values[key]))
-        values[key] = member
 
 
 def check_struct(expr, expr_info):
@@ -694,8 +655,6 @@ def check_struct(expr, expr_info):
                allow_dict=True, allow_optional=True)
     check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
-    if expr.get('base'):
-        check_member_clash(expr_info, expr['base'], expr['data'])
 
 
 def check_keys(expr_elem, meta, required, optional=[]):
@@ -907,13 +866,16 @@ class QAPISchemaEnumType(QAPISchemaType):
     def __init__(self, name, info, values, prefix):
         QAPISchemaType.__init__(self, name, info)
         for v in values:
-            assert isinstance(v, str)
+            assert isinstance(v, QAPISchemaMember)
+            v.set_owner(name)
         assert prefix is None or isinstance(prefix, str)
         self.values = values
         self.prefix = prefix
 
     def check(self, schema):
-        assert len(set(self.values)) == len(self.values)
+        seen = {}
+        for v in self.values:
+            v.check_clash(self.info, seen)
 
     def is_implicit(self):
         # See QAPISchema._make_implicit_enum_type()
@@ -922,8 +884,11 @@ class QAPISchemaEnumType(QAPISchemaType):
     def c_type(self, is_param=False):
         return c_name(self.name)
 
+    def member_names(self):
+        return [v.name for v in self.values]
+
     def c_null(self):
-        return c_enum_const(self.name, (self.values + ['MAX'])[0],
+        return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0],
                             self.prefix)
 
     def json_type(self):
@@ -931,7 +896,7 @@ class QAPISchemaEnumType(QAPISchemaType):
 
     def visit(self, visitor):
         visitor.visit_enum_type(self.name, self.info,
-                                self.values, self.prefix)
+                                self.member_names(), self.prefix)
 
 
 class QAPISchemaArrayType(QAPISchemaType):
@@ -957,12 +922,17 @@ class QAPISchemaArrayType(QAPISchemaType):
 
 class QAPISchemaObjectType(QAPISchemaType):
     def __init__(self, name, info, base, local_members, variants):
+        # struct has local_members, optional base, and no variants
+        # flat union has base, variants, and no local_members
+        # simple union has local_members, variants, and no base
         QAPISchemaType.__init__(self, name, info)
         assert base is None or isinstance(base, str)
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
-        assert (variants is None or
-                isinstance(variants, QAPISchemaObjectTypeVariants))
+            m.set_owner(name)
+        if variants is not None:
+            assert isinstance(variants, QAPISchemaObjectTypeVariants)
+            variants.set_owner(name)
         self._base_name = base
         self.base = None
         self.local_members = local_members
@@ -970,27 +940,34 @@ class QAPISchemaObjectType(QAPISchemaType):
         self.members = None
 
     def check(self, schema):
-        assert self.members is not False        # not running in cycles
+        if self.members is False:               # check for cycles
+            raise QAPIExprError(self.info,
+                                "Object %s contains itself" % self.name)
         if self.members:
             return
         self.members = False                    # mark as being checked
+        seen = OrderedDict()
         if self._base_name:
             self.base = schema.lookup_type(self._base_name)
             assert isinstance(self.base, QAPISchemaObjectType)
-            assert not self.base.variants       # not implemented
             self.base.check(schema)
-            members = list(self.base.members)
-        else:
-            members = []
-        seen = {}
-        for m in members:
-            assert c_name(m.name) not in seen
-            seen[m.name] = m
+            self.base.check_clash(schema, self.info, seen)
         for m in self.local_members:
-            m.check(schema, members, seen)
+            m.check(schema)
+            m.check_clash(self.info, seen)
+        self.members = seen.values()
         if self.variants:
-            self.variants.check(schema, members, seen)
-        self.members = members
+            self.variants.check(schema, seen)
+            assert self.variants.tag_member in self.members
+            self.variants.check_clash(schema, self.info, seen)
+
+    # Check that the members of this type do not cause duplicate JSON fields,
+    # and update seen to track the members seen so far. Report any errors
+    # on behalf of info, which is not necessarily self.info
+    def check_clash(self, schema, info, seen):
+        assert not self.variants       # not implemented
+        for m in self.members:
+            m.check_clash(info, seen)
 
     def is_implicit(self):
         # See QAPISchema._make_implicit_object_type()
@@ -1014,22 +991,63 @@ class QAPISchemaObjectType(QAPISchemaType):
                                        self.members, self.variants)
 
 
-class QAPISchemaObjectTypeMember(object):
-    def __init__(self, name, typ, optional):
+class QAPISchemaMember(object):
+    role = 'member'
+
+    def __init__(self, name):
         assert isinstance(name, str)
+        self.name = name
+        self.owner = None
+
+    def set_owner(self, name):
+        assert not self.owner
+        self.owner = name
+
+    def check_clash(self, info, seen):
+        cname = c_name(self.name)
+        if cname.lower() != cname and self.owner not in case_whitelist:
+            raise QAPIExprError(info,
+                                "%s should not use uppercase" % self.describe())
+        if cname in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(), seen[cname].describe()))
+        seen[cname] = self
+
+    def _pretty_owner(self):
+        owner = self.owner
+        if owner.startswith(':obj-'):
+            # See QAPISchema._make_implicit_object_type() - reverse the
+            # mapping there to create a nice human-readable description
+            owner = owner[5:]
+            if owner.endswith('-arg'):
+                return '(parameter of %s)' % owner[:-4]
+            else:
+                assert owner.endswith('-wrapper')
+                # Unreachable and not implemented
+                assert False
+        if owner.endswith('Kind'):
+            # See QAPISchema._make_implicit_enum_type()
+            return '(branch of %s)' % owner[:-4]
+        return '(%s of %s)' % (self.role, owner)
+
+    def describe(self):
+        return "'%s' %s" % (self.name, self._pretty_owner())
+
+
+class QAPISchemaObjectTypeMember(QAPISchemaMember):
+    def __init__(self, name, typ, optional):
+        QAPISchemaMember.__init__(self, name)
         assert isinstance(typ, str)
         assert isinstance(optional, bool)
-        self.name = name
         self._type_name = typ
         self.type = None
         self.optional = optional
 
-    def check(self, schema, all_members, seen):
-        assert self.name not in seen
+    def check(self, schema):
+        assert self.owner
         self.type = schema.lookup_type(self._type_name)
         assert self.type
-        all_members.append(self)
-        seen[self.name] = self
 
 
 class QAPISchemaObjectTypeVariants(object):
@@ -1047,25 +1065,38 @@ class QAPISchemaObjectTypeVariants(object):
         self.tag_member = tag_member
         self.variants = variants
 
-    def check(self, schema, members, seen):
-        if self.tag_name:
-            self.tag_member = seen[self.tag_name]
-        else:
-            self.tag_member.check(schema, members, seen)
+    def set_owner(self, name):
+        for v in self.variants:
+            v.set_owner(name)
+
+    def check(self, schema, seen):
+        if not self.tag_member:    # flat union
+            self.tag_member = seen[c_name(self.tag_name)]
+            assert self.tag_name == self.tag_member.name
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
-            vseen = dict(seen)
-            v.check(schema, self.tag_member.type, vseen)
+            v.check(schema)
+            # Union names must match enum values; alternate names are
+            # checked separately. Use 'seen' to tell the two apart.
+            if seen:
+                assert v.name in self.tag_member.type.member_names()
+                assert isinstance(v.type, QAPISchemaObjectType)
+                v.type.check(schema)
+
+    def check_clash(self, schema, info, seen):
+        for v in self.variants:
+            # Reset seen map for each variant, since qapi names from one
+            # branch do not affect another branch
+            assert isinstance(v.type, QAPISchemaObjectType)
+            v.type.check_clash(schema, info, dict(seen))
 
 
 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
+    role = 'branch'
+
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
 
-    def check(self, schema, tag_type, seen):
-        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
-        assert self.name in tag_type.values
-
     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
     def simple_union_type(self):
@@ -1082,10 +1113,20 @@ class QAPISchemaAlternateType(QAPISchemaType):
         QAPISchemaType.__init__(self, name, info)
         assert isinstance(variants, QAPISchemaObjectTypeVariants)
         assert not variants.tag_name
+        variants.set_owner(name)
+        variants.tag_member.set_owner(self.name)
         self.variants = variants
 
     def check(self, schema):
-        self.variants.check(schema, [], {})
+        self.variants.tag_member.check(schema)
+        # Not calling self.variants.check_clash(), because there's nothing
+        # to clash with
+        self.variants.check(schema, {})
+        # Alternate branch names have no relation to the tag enum values;
+        # so we have to check for potential name collisions ourselves.
+        seen = {}
+        for v in self.variants.variants:
+            v.check_clash(self.info, seen)
 
     def json_type(self):
         return 'value'
@@ -1196,10 +1237,20 @@ class QAPISchema(object):
         self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
                                                           [], None)
         self._def_entity(self.the_empty_object_type)
+        qtype_values = self._make_enum_members(['none', 'qnull', 'qint',
+                                                'qstring', 'qdict', 'qlist',
+                                                'qfloat', 'qbool'])
+        self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
+                                            'QTYPE'))
+
+    def _make_enum_members(self, values):
+        return [QAPISchemaMember(v) for v in values]
 
     def _make_implicit_enum_type(self, name, info, values):
+        # See also QAPISchemaObjectTypeMember._pretty_owner()
         name = name + 'Kind'   # Use namespace reserved by add_name()
-        self._def_entity(QAPISchemaEnumType(name, info, values, None))
+        self._def_entity(QAPISchemaEnumType(
+            name, info, self._make_enum_members(values), None))
         return name
 
     def _make_array_type(self, element_type, info):
@@ -1211,6 +1262,7 @@ class QAPISchema(object):
     def _make_implicit_object_type(self, name, info, role, members):
         if not members:
             return None
+        # See also QAPISchemaObjectTypeMember._pretty_owner()
         name = ':obj-%s-%s' % (name, role)
         if not self.lookup_entity(name, QAPISchemaObjectType):
             self._def_entity(QAPISchemaObjectType(name, info, None,
@@ -1221,7 +1273,8 @@ class QAPISchema(object):
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
-        self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
+        self._def_entity(QAPISchemaEnumType(
+            name, info, self._make_enum_members(data), prefix))
 
     def _make_member(self, name, typ, info):
         optional = False
@@ -1256,11 +1309,6 @@ class QAPISchema(object):
             typ, info, 'wrapper', [self._make_member('data', typ, info)])
         return QAPISchemaObjectTypeVariant(case, typ)
 
-    def _make_implicit_tag(self, type_name, info, variants):
-        typ = self._make_implicit_enum_type(type_name, info,
-                                            [v.name for v in variants])
-        return QAPISchemaObjectTypeMember('type', typ, False)
-
     def _def_union_type(self, expr, info):
         name = expr['union']
         data = expr['data']
@@ -1270,13 +1318,16 @@ class QAPISchema(object):
         if tag_name:
             variants = [self._make_variant(key, value)
                         for (key, value) in data.iteritems()]
+            members = []
         else:
             variants = [self._make_simple_variant(key, value, info)
                         for (key, value) in data.iteritems()]
-            tag_member = self._make_implicit_tag(name, info, variants)
+            typ = self._make_implicit_enum_type(name, info,
+                                                [v.name for v in variants])
+            tag_member = QAPISchemaObjectTypeMember('type', typ, False)
+            members = [tag_member]
         self._def_entity(
-            QAPISchemaObjectType(name, info, base,
-                                 self._make_members(OrderedDict(), info),
+            QAPISchemaObjectType(name, info, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
                                                               variants)))
@@ -1286,7 +1337,7 @@ class QAPISchema(object):
         data = expr['data']
         variants = [self._make_variant(key, value)
                     for (key, value) in data.iteritems()]
-        tag_member = self._make_implicit_tag(name, info, variants)
+        tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
         self._def_entity(
             QAPISchemaAlternateType(name, info,
                                     QAPISchemaObjectTypeVariants(None,
@@ -1390,7 +1441,7 @@ def camel_to_upper(value):
 def c_enum_const(type_name, const_name, prefix=None):
     if prefix is not None:
         type_name = prefix
-    return camel_to_upper(type_name + '_' + const_name)
+    return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
 
 c_name_trans = string.maketrans('.-', '__')
 
@@ -1432,10 +1483,11 @@ def c_name(name, protect=True):
                      'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
     # namespace pollution:
     polluted_words = set(['unix', 'errno'])
+    name = name.translate(c_name_trans)
     if protect and (name in c89_words | c99_words | c11_words | gcc_words
                     | cpp_words | polluted_words):
         return "q_" + name
-    return name.translate(c_name_trans)
+    return name
 
 eatspace = '\033EATSPACE.'
 pointer_suffix = ' *' + eatspace
@@ -1515,7 +1567,7 @@ const char *const %(c_name)s_lookup[] = {
 ''',
                      index=index, value=value)
 
-    max_index = c_enum_const(name, 'MAX', prefix)
+    max_index = c_enum_const(name, '_MAX', prefix)
     ret += mcgen('''
     [%(max_index)s] = NULL,
 };
@@ -1526,7 +1578,7 @@ const char *const %(c_name)s_lookup[] = {
 
 def gen_enum(name, values, prefix=None):
     # append automatically generated _MAX value
-    enum_values = values + ['MAX']
+    enum_values = values + ['_MAX']
 
     ret = mcgen('''
 
@@ -1594,15 +1646,10 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
     for memb in members:
         if memb.optional:
             ret += mcgen('''
-    visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
+    if (visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s")) {
 ''',
                          prefix=prefix, c_name=c_name(memb.name),
                          name=memb.name, errp=errparg)
-            ret += gen_err_check(skiperr=skiperr)
-            ret += mcgen('''
-    if (%(prefix)shas_%(c_name)s) {
-''',
-                         prefix=prefix, c_name=c_name(memb.name))
             push_indent()
 
         # Ugly: sometimes we need to cast away const