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

QAPI patches

# gpg: Signature made Mon 12 Oct 2015 18:56:35 BST 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-10-12:
  qapi: Simplify gen_visit_fields() error handling
  qapi: Share gen_visit_fields()
  qapi: Share gen_err_check()
  qapi: Consistent generated code: minimize push_indent() usage
  qapi: Consistent generated code: prefer common indentation
  qapi: Consistent generated code: prefer common labels
  qapi: Consistent generated code: prefer visitor 'v'
  qapi: Consistent generated code: prefer error 'err'
  qapi: Reuse code for flat union base validation
  qapi: Test use of 'number' within alternates
  qapi: Add tests for empty unions
  qapi: Avoid assertion failure on union 'type' collision
  qapi: Test for various name collisions
  qapi: Clean up qapi.py per pep8
  qapi: Invoke exception superclass initializer
  qapi: Improve 'include' error message
  qapi: Sort qapi-schema tests
  MAINTAINERS: Specify QAPI include and test files
  MAINTAINERS: Specify QObject include and test files
  docs: Move files from docs/qmp/ to docs/

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

MAINTAINERS

@@ -1054,7 +1054,12 @@ M: Michael Roth <mdroth@linux.vnet.ibm.com>
 S: Supported
 F: qapi/
 X: qapi/*.json
+F: include/qapi/
+X: include/qapi/qmp/
+F: include/qapi/qmp/dispatch.h
 F: tests/qapi-schema/
+F: tests/test-*-visitor.c
+F: tests/test-qmp-*.c
 F: scripts/qapi*
 F: docs/qapi*
 T: git git://repo.or.cz/qemu/armbru.git qapi-next
@@ -1071,6 +1076,14 @@ QObject
 M: Luiz Capitulino <lcapitulino@redhat.com>
 S: Maintained
 F: qobject/
+F: include/qapi/qmp/
+X: include/qapi/qmp/dispatch.h
+F: tests/check-qdict.c
+F: tests/check-qfloat.c
+F: tests/check-qint.c
+F: tests/check-qjson.c
+F: tests/check-qlist.c
+F: tests/check-qstring.c
 T: git git://repo.or.cz/qemu/qmp-unstable.git queue/qmp
 
 QEMU Guest Agent
@@ -1095,7 +1108,7 @@ S: Supported
 F: qmp.c
 F: monitor.c
 F: qmp-commands.hx
-F: docs/qmp/
+F: docs/*qmp-*
 F: scripts/qmp/
 T: git git://repo.or.cz/qemu/armbru.git qapi-next
 

docs/qapi-code-gen.txt

@@ -740,32 +740,32 @@ Example:
 
     void qapi_free_UserDefOne(UserDefOne *obj)
     {
-        QapiDeallocVisitor *md;
+        QapiDeallocVisitor *qdv;
         Visitor *v;
 
         if (!obj) {
             return;
         }
 
-        md = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(md);
+        qdv = qapi_dealloc_visitor_new();
+        v = qapi_dealloc_get_visitor(qdv);
         visit_type_UserDefOne(v, &obj, NULL, NULL);
-        qapi_dealloc_visitor_cleanup(md);
+        qapi_dealloc_visitor_cleanup(qdv);
     }
 
     void qapi_free_UserDefOneList(UserDefOneList *obj)
     {
-        QapiDeallocVisitor *md;
+        QapiDeallocVisitor *qdv;
         Visitor *v;
 
         if (!obj) {
             return;
         }
 
-        md = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(md);
+        qdv = qapi_dealloc_visitor_new();
+        v = qapi_dealloc_get_visitor(qdv);
         visit_type_UserDefOneList(v, &obj, NULL, NULL);
-        qapi_dealloc_visitor_cleanup(md);
+        qapi_dealloc_visitor_cleanup(qdv);
     }
     $ cat qapi-generated/example-qapi-types.h
 [Uninteresting stuff omitted...]
@@ -823,15 +823,15 @@ Example:
     $ cat qapi-generated/example-qapi-visit.c
 [Uninteresting stuff omitted...]
 
-    static void visit_type_UserDefOne_fields(Visitor *m, UserDefOne **obj, Error **errp)
+    static void visit_type_UserDefOne_fields(Visitor *v, UserDefOne **obj, Error **errp)
     {
         Error *err = NULL;
 
-        visit_type_int(m, &(*obj)->integer, "integer", &err);
+        visit_type_int(v, &(*obj)->integer, "integer", &err);
         if (err) {
             goto out;
         }
-        visit_type_str(m, &(*obj)->string, "string", &err);
+        visit_type_str(v, &(*obj)->string, "string", &err);
         if (err) {
             goto out;
         }
@@ -840,40 +840,40 @@ Example:
         error_propagate(errp, err);
     }
 
-    void visit_type_UserDefOne(Visitor *m, UserDefOne **obj, const char *name, Error **errp)
+    void visit_type_UserDefOne(Visitor *v, UserDefOne **obj, const char *name, Error **errp)
     {
         Error *err = NULL;
 
-        visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
+        visit_start_struct(v, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
         if (!err) {
             if (*obj) {
-                visit_type_UserDefOne_fields(m, obj, errp);
+                visit_type_UserDefOne_fields(v, obj, errp);
             }
-            visit_end_struct(m, &err);
+            visit_end_struct(v, &err);
         }
         error_propagate(errp, err);
     }
 
-    void visit_type_UserDefOneList(Visitor *m, UserDefOneList **obj, const char *name, Error **errp)
+    void visit_type_UserDefOneList(Visitor *v, UserDefOneList **obj, const char *name, Error **errp)
     {
         Error *err = NULL;
         GenericList *i, **prev;
 
-        visit_start_list(m, name, &err);
+        visit_start_list(v, name, &err);
         if (err) {
             goto out;
         }
 
         for (prev = (GenericList **)obj;
-             !err && (i = visit_next_list(m, prev, &err)) != NULL;
+             !err && (i = visit_next_list(v, prev, &err)) != NULL;
              prev = &i) {
             UserDefOneList *native_i = (UserDefOneList *)i;
-            visit_type_UserDefOne(m, &native_i->value, NULL, &err);
+            visit_type_UserDefOne(v, &native_i->value, NULL, &err);
         }
 
         error_propagate(errp, err);
         err = NULL;
-        visit_end_list(m, &err);
+        visit_end_list(v, &err);
     out:
         error_propagate(errp, err);
     }
@@ -885,8 +885,8 @@ Example:
 
 [Visitors for built-in types omitted...]
 
-    void visit_type_UserDefOne(Visitor *m, UserDefOne **obj, const char *name, Error **errp);
-    void visit_type_UserDefOneList(Visitor *m, UserDefOneList **obj, const char *name, Error **errp);
+    void visit_type_UserDefOne(Visitor *v, UserDefOne **obj, const char *name, Error **errp);
+    void visit_type_UserDefOneList(Visitor *v, UserDefOneList **obj, const char *name, Error **errp);
 
     #endif
 
@@ -916,56 +916,56 @@ Example:
 
     static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject **ret_out, Error **errp)
     {
-        Error *local_err = NULL;
-        QmpOutputVisitor *mo = qmp_output_visitor_new();
-        QapiDeallocVisitor *md;
+        Error *err = NULL;
+        QmpOutputVisitor *qov = qmp_output_visitor_new();
+        QapiDeallocVisitor *qdv;
         Visitor *v;
 
-        v = qmp_output_get_visitor(mo);
-        visit_type_UserDefOne(v, &ret_in, "unused", &local_err);
-        if (local_err) {
+        v = qmp_output_get_visitor(qov);
+        visit_type_UserDefOne(v, &ret_in, "unused", &err);
+        if (err) {
             goto out;
         }
-        *ret_out = qmp_output_get_qobject(mo);
+        *ret_out = qmp_output_get_qobject(qov);
 
     out:
-        error_propagate(errp, local_err);
-        qmp_output_visitor_cleanup(mo);
-        md = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(md);
+        error_propagate(errp, err);
+        qmp_output_visitor_cleanup(qov);
+        qdv = qapi_dealloc_visitor_new();
+        v = qapi_dealloc_get_visitor(qdv);
         visit_type_UserDefOne(v, &ret_in, "unused", NULL);
-        qapi_dealloc_visitor_cleanup(md);
+        qapi_dealloc_visitor_cleanup(qdv);
     }
 
     static void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
     {
-        Error *local_err = NULL;
+        Error *err = NULL;
         UserDefOne *retval;
-        QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
-        QapiDeallocVisitor *md;
+        QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+        QapiDeallocVisitor *qdv;
         Visitor *v;
         UserDefOne *arg1 = NULL;
 
-        v = qmp_input_get_visitor(mi);
-        visit_type_UserDefOne(v, &arg1, "arg1", &local_err);
-        if (local_err) {
+        v = qmp_input_get_visitor(qiv);
+        visit_type_UserDefOne(v, &arg1, "arg1", &err);
+        if (err) {
             goto out;
         }
 
-        retval = qmp_my_command(arg1, &local_err);
-        if (local_err) {
+        retval = qmp_my_command(arg1, &err);
+        if (err) {
             goto out;
         }
 
-        qmp_marshal_output_UserDefOne(retval, ret, &local_err);
+        qmp_marshal_output_UserDefOne(retval, ret, &err);
 
     out:
-        error_propagate(errp, local_err);
-        qmp_input_visitor_cleanup(mi);
-        md = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(md);
+        error_propagate(errp, err);
+        qmp_input_visitor_cleanup(qiv);
+        qdv = qapi_dealloc_visitor_new();
+        v = qapi_dealloc_get_visitor(qdv);
         visit_type_UserDefOne(v, &arg1, "arg1", NULL);
-        qapi_dealloc_visitor_cleanup(md);
+        qapi_dealloc_visitor_cleanup(qdv);
     }
 
     static void qmp_init_marshal(void)
@@ -1007,7 +1007,7 @@ Example:
     void qapi_event_send_my_event(Error **errp)
     {
         QDict *qmp;
-        Error *local_err = NULL;
+        Error *err = NULL;
         QMPEventFuncEmit emit;
         emit = qmp_event_get_func_emit();
         if (!emit) {
@@ -1016,9 +1016,9 @@ Example:
 
         qmp = qmp_event_build_dict("MY_EVENT");
 
-        emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &local_err);
+        emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err);
 
-        error_propagate(errp, local_err);
+        error_propagate(errp, err);
         QDECREF(qmp);
     }
 

qom/object.c

@@ -1167,31 +1167,31 @@ out:
 void object_property_parse(Object *obj, const char *string,
                            const char *name, Error **errp)
 {
-    StringInputVisitor *mi;
-    mi = string_input_visitor_new(string);
-    object_property_set(obj, string_input_get_visitor(mi), name, errp);
+    StringInputVisitor *siv;
+    siv = string_input_visitor_new(string);
+    object_property_set(obj, string_input_get_visitor(siv), name, errp);
 
-    string_input_visitor_cleanup(mi);
+    string_input_visitor_cleanup(siv);
 }
 
 char *object_property_print(Object *obj, const char *name, bool human,
                             Error **errp)
 {
-    StringOutputVisitor *mo;
+    StringOutputVisitor *sov;
     char *string = NULL;
     Error *local_err = NULL;
 
-    mo = string_output_visitor_new(human);
-    object_property_get(obj, string_output_get_visitor(mo), name, &local_err);
+    sov = string_output_visitor_new(human);
+    object_property_get(obj, string_output_get_visitor(sov), name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }
 
-    string = string_output_get_string(mo);
+    string = string_output_get_string(sov);
 
 out:
-    string_output_visitor_cleanup(mo);
+    string_output_visitor_cleanup(sov);
     return string;
 }
 

qom/qom-qobject.c

@@ -19,11 +19,11 @@
 void object_property_set_qobject(Object *obj, QObject *value,
                                  const char *name, Error **errp)
 {
-    QmpInputVisitor *mi;
-    mi = qmp_input_visitor_new(value);
-    object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
+    QmpInputVisitor *qiv;
+    qiv = qmp_input_visitor_new(value);
+    object_property_set(obj, qmp_input_get_visitor(qiv), name, errp);
 
-    qmp_input_visitor_cleanup(mi);
+    qmp_input_visitor_cleanup(qiv);
 }
 
 QObject *object_property_get_qobject(Object *obj, const char *name,
@@ -31,14 +31,14 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
 {
     QObject *ret = NULL;
     Error *local_err = NULL;
-    QmpOutputVisitor *mo;
+    QmpOutputVisitor *qov;
 
-    mo = qmp_output_visitor_new();
-    object_property_get(obj, qmp_output_get_visitor(mo), name, &local_err);
+    qov = qmp_output_visitor_new();
+    object_property_get(obj, qmp_output_get_visitor(qov), name, &local_err);
     if (!local_err) {
-        ret = qmp_output_get_qobject(mo);
+        ret = qmp_output_get_qobject(qov);
     }
     error_propagate(errp, local_err);
-    qmp_output_visitor_cleanup(mo);
+    qmp_output_visitor_cleanup(qov);
     return ret;
 }

scripts/ordereddict.py

@@ -22,6 +22,7 @@
 
 from UserDict import DictMixin
 
+
 class OrderedDict(dict, DictMixin):
 
     def __init__(self, *args, **kwds):
@@ -117,7 +118,7 @@ def __eq__(self, other):
         if isinstance(other, OrderedDict):
             if len(self) != len(other):
                 return False
-            for p, q in  zip(self.items(), other.items()):
+            for p, q in zip(self.items(), other.items()):
                 if p != q:
                     return False
             return True

scripts/qapi-commands.py

@@ -25,17 +25,6 @@ def gen_command_decl(name, arg_type, ret_type):
                  params=gen_params(arg_type, 'Error **errp'))
 
 
-def gen_err_check(err):
-    if not err:
-        return ''
-    return mcgen('''
-if (%(err)s) {
-    goto out;
-}
-''',
-                 err=err)
-
-
 def gen_call(name, arg_type, ret_type):
     ret = ''
 
@@ -50,51 +39,47 @@ def gen_call(name, arg_type, ret_type):
     if ret_type:
         lhs = 'retval = '
 
-    push_indent()
     ret = mcgen('''
 
-%(lhs)sqmp_%(c_name)s(%(args)s&local_err);
+    %(lhs)sqmp_%(c_name)s(%(args)s&err);
 ''',
                 c_name=c_name(name), args=argstr, lhs=lhs)
     if ret_type:
-        ret += gen_err_check('local_err')
+        ret += gen_err_check()
         ret += mcgen('''
 
-qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
+    qmp_marshal_output_%(c_name)s(retval, ret, &err);
 ''',
                      c_name=ret_type.c_name())
-    pop_indent()
     return ret
 
 
 def gen_marshal_vars(arg_type, ret_type):
     ret = mcgen('''
-    Error *local_err = NULL;
+    Error *err = NULL;
 ''')
 
-    push_indent()
-
     if ret_type:
         ret += mcgen('''
-%(c_type)s retval;
+    %(c_type)s retval;
 ''',
                      c_type=ret_type.c_type())
 
     if arg_type:
         ret += mcgen('''
-QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *md;
-Visitor *v;
+    QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+    QapiDeallocVisitor *qdv;
+    Visitor *v;
 ''')
 
         for memb in arg_type.members:
             if memb.optional:
                 ret += mcgen('''
-bool has_%(c_name)s = false;
+    bool has_%(c_name)s = false;
 ''',
                              c_name=c_name(memb.name))
             ret += mcgen('''
-%(c_type)s %(c_name)s = %(c_null)s;
+    %(c_type)s %(c_name)s = %(c_null)s;
 ''',
                          c_name=c_name(memb.name),
                          c_type=memb.type.c_type(),
@@ -103,10 +88,9 @@ def gen_marshal_vars(arg_type, ret_type):
     else:
         ret += mcgen('''
 
-(void)args;
+    (void)args;
 ''')
 
-    pop_indent()
     return ret
 
 
@@ -116,53 +100,23 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
     if not arg_type:
         return ret
 
-    push_indent()
-
     if dealloc:
-        errparg = 'NULL'
-        errarg = None
         ret += mcgen('''
-qmp_input_visitor_cleanup(mi);
-md = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(md);
+    qmp_input_visitor_cleanup(qiv);
+    qdv = qapi_dealloc_visitor_new();
+    v = qapi_dealloc_get_visitor(qdv);
 ''')
     else:
-        errparg = '&local_err'
-        errarg = 'local_err'
         ret += mcgen('''
-v = qmp_input_get_visitor(mi);
+    v = qmp_input_get_visitor(qiv);
 ''')
 
-    for memb in arg_type.members:
-        if memb.optional:
-            ret += mcgen('''
-visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
-''',
-                         c_name=c_name(memb.name), name=memb.name,
-                         errp=errparg)
-            ret += gen_err_check(errarg)
-            ret += mcgen('''
-if (has_%(c_name)s) {
-''',
-                         c_name=c_name(memb.name))
-            push_indent()
-        ret += mcgen('''
-visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
-''',
-                     c_name=c_name(memb.name), name=memb.name,
-                     c_type=memb.type.c_name(), errp=errparg)
-        ret += gen_err_check(errarg)
-        if memb.optional:
-            pop_indent()
-            ret += mcgen('''
-}
-''')
+    ret += gen_visit_fields(arg_type.members, skiperr=dealloc)
 
     if dealloc:
         ret += mcgen('''
-qapi_dealloc_visitor_cleanup(md);
+    qapi_dealloc_visitor_cleanup(qdv);
 ''')
-    pop_indent()
     return ret
 
 
@@ -171,25 +125,25 @@ def gen_marshal_output(ret_type):
 
 static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
 {
-    Error *local_err = NULL;
-    QmpOutputVisitor *mo = qmp_output_visitor_new();
-    QapiDeallocVisitor *md;
+    Error *err = NULL;
+    QmpOutputVisitor *qov = qmp_output_visitor_new();
+    QapiDeallocVisitor *qdv;
     Visitor *v;
 
-    v = qmp_output_get_visitor(mo);
-    visit_type_%(c_name)s(v, &ret_in, "unused", &local_err);
-    if (local_err) {
+    v = qmp_output_get_visitor(qov);
+    visit_type_%(c_name)s(v, &ret_in, "unused", &err);
+    if (err) {
         goto out;
     }
-    *ret_out = qmp_output_get_qobject(mo);
+    *ret_out = qmp_output_get_qobject(qov);
 
 out:
-    error_propagate(errp, local_err);
-    qmp_output_visitor_cleanup(mo);
-    md = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(md);
+    error_propagate(errp, err);
+    qmp_output_visitor_cleanup(qov);
+    qdv = qapi_dealloc_visitor_new();
+    v = qapi_dealloc_get_visitor(qdv);
     visit_type_%(c_name)s(v, &ret_in, "unused", NULL);
-    qapi_dealloc_visitor_cleanup(md);
+    qapi_dealloc_visitor_cleanup(qdv);
 }
 ''',
                  c_type=ret_type.c_type(), c_name=ret_type.c_name())
@@ -227,7 +181,7 @@ def gen_marshal(name, arg_type, ret_type):
 out:
 ''')
     ret += mcgen('''
-    error_propagate(errp, local_err);
+    error_propagate(errp, err);
 ''')
     ret += gen_marshal_input_visit(arg_type, dealloc=True)
     ret += mcgen('''
@@ -237,17 +191,15 @@ def gen_marshal(name, arg_type, ret_type):
 
 
 def gen_register_command(name, success_response):
-    push_indent()
     options = 'QCO_NO_OPTIONS'
     if not success_response:
         options = 'QCO_NO_SUCCESS_RESP'
 
     ret = mcgen('''
-qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
+    qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
 ''',
                 name=name, c_name=c_name(name),
                 opts=options)
-    pop_indent()
     return ret
 
 

scripts/qapi-event.py

@@ -34,7 +34,7 @@ def gen_event_send(name, arg_type):
 %(proto)s
 {
     QDict *qmp;
-    Error *local_err = NULL;
+    Error *err = NULL;
     QMPEventFuncEmit emit;
 ''',
                 proto=gen_event_send_proto(name, arg_type))
@@ -67,50 +67,15 @@ def gen_event_send(name, arg_type):
     g_assert(v);
 
     /* Fake visit, as if all members are under a structure */
-    visit_start_struct(v, NULL, "", "%(name)s", 0, &local_err);
-    if (local_err) {
-        goto clean;
-    }
-
+    visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
 ''',
                      name=name)
-
-        for memb in arg_type.members:
-            if memb.optional:
-                ret += mcgen('''
-    if (has_%(c_name)s) {
-''',
-                             c_name=c_name(memb.name))
-                push_indent()
-
-            # Ugly: need to cast away the const
-            if memb.type.name == "str":
-                cast = '(char **)'
-            else:
-                cast = ''
-
-            ret += mcgen('''
-    visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &local_err);
-    if (local_err) {
-        goto clean;
-    }
-''',
-                         cast=cast,
-                         c_name=c_name(memb.name),
-                         c_type=memb.type.c_name(),
-                         name=memb.name)
-
-            if memb.optional:
-                pop_indent()
-                ret += mcgen('''
-    }
-''')
-
+        ret += gen_err_check()
+        ret += gen_visit_fields(arg_type.members, need_cast=True)
         ret += mcgen('''
-
-    visit_end_struct(v, &local_err);
-    if (local_err) {
-        goto clean;
+    visit_end_struct(v, &err);
+    if (err) {
+        goto out;
     }
 
     obj = qmp_output_get_qobject(qov);
@@ -120,18 +85,18 @@ def gen_event_send(name, arg_type):
 ''')
 
     ret += mcgen('''
-    emit(%(c_enum)s, qmp, &local_err);
+    emit(%(c_enum)s, qmp, &err);
 
 ''',
                  c_enum=c_enum_const(event_enum_name, name))
 
     if arg_type and arg_type.members:
         ret += mcgen('''
- clean:
+out:
     qmp_output_visitor_cleanup(qov);
 ''')
     ret += mcgen('''
-    error_propagate(errp, local_err);
+    error_propagate(errp, err);
     QDECREF(qmp);
 }
 ''')

scripts/qapi-types.py

@@ -188,17 +188,17 @@ def gen_type_cleanup(name):
 
 void qapi_free_%(c_name)s(%(c_name)s *obj)
 {
-    QapiDeallocVisitor *md;
+    QapiDeallocVisitor *qdv;
     Visitor *v;
 
     if (!obj) {
         return;
     }
 
-    md = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(md);
+    qdv = qapi_dealloc_visitor_new();
+    v = qapi_dealloc_get_visitor(qdv);
     visit_type_%(c_name)s(v, &obj, NULL, NULL);
-    qapi_dealloc_visitor_cleanup(md);
+    qapi_dealloc_visitor_cleanup(qdv);
 }
 ''',
                 c_name=c_name(name))

scripts/qapi-visit.py

@@ -24,7 +24,7 @@ def gen_visit_decl(name, scalar=False):
     if not scalar:
         c_type += '*'
     return mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_type)sobj, const char *name, Error **errp);
+void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **errp);
 ''',
                  c_name=c_name(name), c_type=c_type)
 
@@ -39,20 +39,20 @@ def gen_visit_implicit_struct(typ):
         # Need a forward declaration
         ret += mcgen('''
 
-static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp);
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
 ''',
                      c_type=typ.c_name())
 
     ret += mcgen('''
 
-static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
+static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
-        visit_type_%(c_type)s_fields(m, obj, errp);
-        visit_end_implicit_struct(m, &err);
+        visit_type_%(c_type)s_fields(v, obj, errp);
+        visit_end_implicit_struct(v, &err);
     }
     error_propagate(errp, err);
 }
@@ -71,50 +71,22 @@ def gen_visit_struct_fields(name, base, members):
 
     ret += mcgen('''
 
-static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
 {
     Error *err = NULL;
 
 ''',
                  c_name=c_name(name))
-    push_indent()
 
     if base:
         ret += mcgen('''
-visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
-if (err) {
-    goto out;
-}
+    visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
 ''',
                      c_type=base.c_name(), c_name=c_name('base'))
+        ret += gen_err_check()
 
-    for memb in members:
-        if memb.optional:
-            ret += mcgen('''
-visit_optional(m, &(*obj)->has_%(c_name)s, "%(name)s", &err);
-if (!err && (*obj)->has_%(c_name)s) {
-''',
-                         c_name=c_name(memb.name), name=memb.name)
-            push_indent()
-
-        ret += mcgen('''
-visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
-''',
-                     c_type=memb.type.c_name(), c_name=c_name(memb.name),
-                     name=memb.name)
-
-        if memb.optional:
-            pop_indent()
-            ret += mcgen('''
-}
-''')
-        ret += mcgen('''
-if (err) {
-    goto out;
-}
-''')
+    ret += gen_visit_fields(members, prefix='(*obj)->')
 
-    pop_indent()
     if re.search('^ *goto out;', ret, re.MULTILINE):
         ret += mcgen('''
 
@@ -136,16 +108,16 @@ def gen_visit_struct(name, base, members):
     # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''
 
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (!err) {
         if (*obj) {
-            visit_type_%(c_name)s_fields(m, obj, errp);
+            visit_type_%(c_name)s_fields(v, obj, errp);
         }
-        visit_end_struct(m, &err);
+        visit_end_struct(v, &err);
     }
     error_propagate(errp, err);
 }
@@ -158,26 +130,26 @@ def gen_visit_struct(name, base, members):
 def gen_visit_list(name, element_type):
     return mcgen('''
 
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
     GenericList *i, **prev;
 
-    visit_start_list(m, name, &err);
+    visit_start_list(v, name, &err);
     if (err) {
         goto out;
     }
 
     for (prev = (GenericList **)obj;
-         !err && (i = visit_next_list(m, prev, &err)) != NULL;
+         !err && (i = visit_next_list(v, prev, &err)) != NULL;
          prev = &i) {
         %(c_name)s *native_i = (%(c_name)s *)i;
-        visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err);
+        visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
     }
 
     error_propagate(errp, err);
     err = NULL;
-    visit_end_list(m, &err);
+    visit_end_list(v, &err);
 out:
     error_propagate(errp, err);
 }
@@ -188,9 +160,9 @@ def gen_visit_list(name, element_type):
 def gen_visit_enum(name):
     return mcgen('''
 
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
 {
-    visit_type_enum(m, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
+    visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
 }
 ''',
                  c_name=c_name(name), name=name)
@@ -199,17 +171,17 @@ def gen_visit_enum(name):
 def gen_visit_alternate(name, variants):
     ret = mcgen('''
 
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
-    visit_get_next_type(m, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+    visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
     if (err) {
-        goto out_end;
+        goto out_obj;
     }
     switch ((*obj)->kind) {
 ''',
@@ -218,7 +190,7 @@ def gen_visit_alternate(name, variants):
     for var in variants.variants:
         ret += mcgen('''
     case %(case)s:
-        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
+        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
         break;
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
@@ -230,10 +202,10 @@ def gen_visit_alternate(name, variants):
     default:
         abort();
     }
-out_end:
+out_obj:
     error_propagate(errp, err);
     err = NULL;
-    visit_end_implicit_struct(m, &err);
+    visit_end_implicit_struct(v, &err);
 out:
     error_propagate(errp, err);
 }
@@ -256,40 +228,40 @@ def gen_visit_union(name, base, variants):
 
     ret += mcgen('''
 
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
-    if (*obj) {
+    if (!*obj) {
+        goto out_obj;
+    }
 ''',
                  c_name=c_name(name), name=name)
 
     if base:
         ret += mcgen('''
-        visit_type_%(c_name)s_fields(m, obj, &err);
-        if (err) {
-            goto out_obj;
-        }
+    visit_type_%(c_name)s_fields(v, 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('''
-        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
-        if (err) {
-            goto out_obj;
-        }
-        if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
-            goto out_obj;
-        }
-        switch ((*obj)->%(c_name)s) {
+    visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
+    if (err) {
+        goto out_obj;
+    }
+    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
@@ -302,38 +274,37 @@ def gen_visit_union(name, base, variants):
         # TODO ugly special case for simple union
         simple_union_type = var.simple_union_type()
         ret += mcgen('''
-        case %(case)s:
+    case %(case)s:
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
                                        var.name))
         if simple_union_type:
             ret += mcgen('''
-            visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+        visit_type_%(c_type)s(v, &(*obj)->%(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(m, &(*obj)->%(c_name)s, &err);
+        visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
 ''',
                          c_type=var.type.c_name(),
                          c_name=c_name(var.name))
         ret += mcgen('''
-            break;
+        break;
 ''')
 
     ret += mcgen('''
-        default:
-            abort();
-        }
-out_obj:
-        error_propagate(errp, err);
-        err = NULL;
-        visit_end_union(m, !!(*obj)->data, &err);
-        error_propagate(errp, err);
-        err = NULL;
+    default:
+        abort();
     }
-    visit_end_struct(m, &err);
+out_obj:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_union(v, !!(*obj)->data, &err);
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, &err);
 out:
     error_propagate(errp, err);
 }

scripts/qapi.py

@@ -71,6 +71,7 @@
 # Parsing the schema into expressions
 #
 
+
 def error_path(parent):
     res = ""
     while parent:
@@ -79,8 +80,10 @@ def error_path(parent):
         parent = parent['parent']
     return res
 
+
 class QAPISchemaError(Exception):
     def __init__(self, schema, msg):
+        Exception.__init__(self)
         self.fname = schema.fname
         self.msg = msg
         self.col = 1
@@ -96,8 +99,10 @@ def __str__(self):
         return error_path(self.info) + \
             "%s:%d:%d: %s" % (self.fname, self.line, self.col, self.msg)
 
+
 class QAPIExprError(Exception):
     def __init__(self, expr_info, msg):
+        Exception.__init__(self)
         self.info = expr_info
         self.msg = msg
 
@@ -105,9 +110,10 @@ def __str__(self):
         return error_path(self.info['parent']) + \
             "%s:%d: %s" % (self.info['file'], self.info['line'], self.msg)
 
+
 class QAPISchemaParser(object):
 
-    def __init__(self, fp, previously_included = [], incl_info = None):
+    def __init__(self, fp, previously_included=[], incl_info=None):
         abs_fname = os.path.abspath(fp.name)
         fname = fp.name
         self.fname = fname
@@ -122,18 +128,18 @@ def __init__(self, fp, previously_included = [], incl_info = None):
         self.exprs = []
         self.accept()
 
-        while self.tok != None:
+        while self.tok is not None:
             expr_info = {'file': fname, 'line': self.line,
                          'parent': self.incl_info}
             expr = self.get_expr(False)
             if isinstance(expr, dict) and "include" in expr:
                 if len(expr) != 1:
-                    raise QAPIExprError(expr_info, "Invalid 'include' directive")
+                    raise QAPIExprError(expr_info,
+                                        "Invalid 'include' directive")
                 include = expr["include"]
                 if not isinstance(include, str):
                     raise QAPIExprError(expr_info,
-                                        'Expected a file name (string), got: %s'
-                                        % include)
+                                        "Value of 'include' must be a string")
                 incl_abs_fname = os.path.join(os.path.dirname(abs_fname),
                                               include)
                 # catch inclusion cycle
@@ -192,7 +198,7 @@ def accept(self):
                             string += '\t'
                         elif ch == 'u':
                             value = 0
-                            for x in range(0, 4):
+                            for _ in range(0, 4):
                                 ch = self.src[self.cursor]
                                 self.cursor += 1
                                 if ch not in "0123456789abcdefABCDEF":
@@ -214,7 +220,7 @@ def accept(self):
                             string += ch
                         else:
                             raise QAPISchemaError(self,
-                                                  "Unknown escape \\%s" %ch)
+                                                  "Unknown escape \\%s" % ch)
                         esc = False
                     elif ch == "\\":
                         esc = True
@@ -274,7 +280,7 @@ def get_values(self):
         if self.tok == ']':
             self.accept()
             return expr
-        if not self.tok in "{['tfn":
+        if self.tok not in "{['tfn":
             raise QAPISchemaError(self, 'Expected "{", "[", "]", string, '
                                   'boolean or "null"')
         while True:
@@ -308,15 +314,17 @@ def get_expr(self, nested):
 # TODO catching name collisions in generated code would be nice
 #
 
+
 def find_base_fields(base):
     base_struct_define = find_struct(base)
     if not base_struct_define:
         return None
     return base_struct_define['data']
 
+
 # Return the qtype of an alternate branch, or None on error.
 def find_alternate_member_qtype(qapi_type):
-    if builtin_types.has_key(qapi_type):
+    if qapi_type in builtin_types:
         return builtin_types[qapi_type]
     elif find_struct(qapi_type):
         return "QTYPE_QDICT"
@@ -326,6 +334,7 @@ def find_alternate_member_qtype(qapi_type):
         return "QTYPE_QDICT"
     return None
 
+
 # Return the discriminator enum define if discriminator is specified as an
 # enum type, otherwise return None.
 def discriminator_find_enum_define(expr):
@@ -345,11 +354,14 @@ 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_.-]*$')
-def check_name(expr_info, source, name, allow_optional = False,
-               enum_member = False):
+
+
+def check_name(expr_info, source, name, allow_optional=False,
+               enum_member=False):
     global valid_name
     membername = name
 
@@ -370,7 +382,8 @@ def check_name(expr_info, source, name, allow_optional = False,
         raise QAPIExprError(expr_info,
                             "%s uses invalid name '%s'" % (source, name))
 
-def add_name(name, info, meta, implicit = False):
+
+def add_name(name, info, meta, implicit=False):
     global all_names
     check_name(info, "'%s'" % meta, name)
     # FIXME should reject names that differ only in '_' vs. '.'
@@ -385,12 +398,14 @@ def add_name(name, info, meta, implicit = False):
                             % (meta, name))
     all_names[name] = meta
 
+
 def add_struct(definition, info):
     global struct_types
     name = definition['struct']
     add_name(name, info, 'struct')
     struct_types.append(definition)
 
+
 def find_struct(name):
     global struct_types
     for struct in struct_types:
@@ -398,12 +413,14 @@ def find_struct(name):
             return struct
     return None
 
+
 def add_union(definition, info):
     global union_types
     name = definition['union']
     add_name(name, info, 'union')
     union_types.append(definition)
 
+
 def find_union(name):
     global union_types
     for union in union_types:
@@ -411,11 +428,13 @@ def find_union(name):
             return union
     return None
 
-def add_enum(name, info, enum_values = None, implicit = False):
+
+def add_enum(name, info, enum_values=None, implicit=False):
     global enum_types
     add_name(name, info, 'enum', implicit)
     enum_types.append({"enum_name": name, "enum_values": enum_values})
 
+
 def find_enum(name):
     global enum_types
     for enum in enum_types:
@@ -423,12 +442,14 @@ def find_enum(name):
             return enum
     return None
 
+
 def is_enum(name):
-    return find_enum(name) != None
+    return find_enum(name) is not None
+
 
-def check_type(expr_info, source, value, allow_array = False,
-               allow_dict = False, allow_optional = False,
-               allow_metas = []):
+def check_type(expr_info, source, value, allow_array=False,
+               allow_dict=False, allow_optional=False,
+               allow_metas=[]):
     global all_names
 
     if value is None:
@@ -447,7 +468,7 @@ def check_type(expr_info, source, value, allow_array = False,
 
     # Check if type name for value is okay
     if isinstance(value, str):
-        if not value in all_names:
+        if value not in all_names:
             raise QAPIExprError(expr_info,
                                 "%s uses unknown type '%s'"
                                 % (source, value))
@@ -476,7 +497,8 @@ def check_type(expr_info, source, value, allow_array = False,
                    allow_metas=['built-in', 'union', 'alternate', 'struct',
                                 'enum'])
 
-def check_member_clash(expr_info, base_name, data, source = ""):
+
+def check_member_clash(expr_info, base_name, data, source=""):
     base = find_struct(base_name)
     assert base
     base_members = base['data']
@@ -490,6 +512,7 @@ def check_member_clash(expr_info, base_name, data, source = ""):
     if base.get('base'):
         check_member_clash(expr_info, base['base'], data, source)
 
+
 def check_command(expr, expr_info):
     name = expr['command']
 
@@ -503,6 +526,7 @@ def check_command(expr, expr_info):
                expr.get('returns'), allow_array=True,
                allow_optional=True, allow_metas=returns_meta)
 
+
 def check_event(expr, expr_info):
     global events
     name = expr['event']
@@ -514,19 +538,20 @@ def check_event(expr, expr_info):
                expr.get('data'), allow_dict=True, allow_optional=True,
                allow_metas=['struct'])
 
+
 def check_union(expr, expr_info):
     name = expr['union']
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = { 'MAX': '(automatic)' }
+    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
 
     # Two types of unions, determined by discriminator.
 
     # With no discriminator it is a simple union.
     if discriminator is None:
         enum_define = None
-        allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']
+        allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum']
         if base is not None:
             raise QAPIExprError(expr_info,
                                 "Simple union '%s' must not have a base"
@@ -535,15 +560,14 @@ def check_union(expr, expr_info):
     # Else, it's a flat union.
     else:
         # The object must have a string member 'base'.
-        if not isinstance(base, str):
+        check_type(expr_info, "'base' for union '%s'" % name,
+                   base, allow_metas=['struct'])
+        if not base:
             raise QAPIExprError(expr_info,
-                                "Flat union '%s' must have a string base field"
+                                "Flat union '%s' must have a base"
                                 % name)
         base_fields = find_base_fields(base)
-        if not base_fields:
-            raise QAPIExprError(expr_info,
-                                "Base '%s' is not a valid struct"
-                                % base)
+        assert base_fields
 
         # The value of member 'discriminator' must name a non-optional
         # member of the base struct.
@@ -556,7 +580,7 @@ def check_union(expr, expr_info):
                                 "struct '%s'"
                                 % (discriminator, base))
         enum_define = find_enum(discriminator_type)
-        allow_metas=['struct']
+        allow_metas = ['struct']
         # Do not allow string discriminator
         if not enum_define:
             raise QAPIExprError(expr_info,
@@ -578,13 +602,19 @@ def check_union(expr, expr_info):
                                " of branch '%s'" % key)
 
         # If the discriminator names an enum type, then all members
-        # of 'data' must also be members of the enum type.
+        # of 'data' must also be members of the enum type, which in turn
+        # must not collide with the discriminator name.
         if enum_define:
-            if not key in enum_define['enum_values']:
+            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:
@@ -595,10 +625,11 @@ def check_union(expr, expr_info):
                                     % (name, key, values[c_key]))
             values[c_key] = key
 
+
 def check_alternate(expr, expr_info):
     name = expr['alternate']
     members = expr['data']
-    values = { 'MAX': '(automatic)' }
+    values = {'MAX': '(automatic)'}
     types_seen = {}
 
     # Check every branch
@@ -626,11 +657,12 @@ def check_alternate(expr, expr_info):
                                 % (name, key, types_seen[qtype]))
         types_seen[qtype] = key
 
+
 def check_enum(expr, expr_info):
     name = expr['enum']
     members = expr.get('data')
     prefix = expr.get('prefix')
-    values = { 'MAX': '(automatic)' }
+    values = {'MAX': '(automatic)'}
 
     if not isinstance(members, list):
         raise QAPIExprError(expr_info,
@@ -639,7 +671,7 @@ def check_enum(expr, expr_info):
         raise QAPIExprError(expr_info,
                             "Enum '%s' requires a string for 'prefix'" % name)
     for member in members:
-        check_name(expr_info, "Member of enum '%s'" %name, member,
+        check_name(expr_info, "Member of enum '%s'" % name, member,
                    enum_member=True)
         key = camel_to_upper(member)
         if key in values:
@@ -648,6 +680,7 @@ def check_enum(expr, expr_info):
                                 % (name, member, values[key]))
         values[key] = member
 
+
 def check_struct(expr, expr_info):
     name = expr['struct']
     members = expr['data']
@@ -659,6 +692,7 @@ def check_struct(expr, expr_info):
     if expr.get('base'):
         check_member_clash(expr_info, expr['base'], expr['data'])
 
+
 def check_keys(expr_elem, meta, required, optional=[]):
     expr = expr_elem['expr']
     info = expr_elem['info']
@@ -666,22 +700,23 @@ def check_keys(expr_elem, meta, required, optional=[]):
     if not isinstance(name, str):
         raise QAPIExprError(info,
                             "'%s' key must have a string value" % meta)
-    required = required + [ meta ]
+    required = required + [meta]
     for (key, value) in expr.items():
-        if not key in required and not key in optional:
+        if key not in required and key not in optional:
             raise QAPIExprError(info,
                                 "Unknown key '%s' in %s '%s'"
                                 % (key, meta, name))
-        if (key == 'gen' or key == 'success-response') and value != False:
+        if (key == 'gen' or key == 'success-response') and value is not False:
             raise QAPIExprError(info,
                                 "'%s' of %s '%s' should only use false value"
                                 % (key, meta, name))
     for key in required:
-        if not expr.has_key(key):
+        if key not in expr:
             raise QAPIExprError(info,
                                 "Key '%s' is missing from %s '%s'"
                                 % (key, meta, name))
 
+
 def check_exprs(exprs):
     global all_names
 
@@ -691,24 +726,24 @@ def check_exprs(exprs):
     for expr_elem in exprs:
         expr = expr_elem['expr']
         info = expr_elem['info']
-        if expr.has_key('enum'):
+        if 'enum' in expr:
             check_keys(expr_elem, 'enum', ['data'], ['prefix'])
             add_enum(expr['enum'], info, expr['data'])
-        elif expr.has_key('union'):
+        elif 'union' in expr:
             check_keys(expr_elem, 'union', ['data'],
                        ['base', 'discriminator'])
             add_union(expr, info)
-        elif expr.has_key('alternate'):
+        elif 'alternate' in expr:
             check_keys(expr_elem, 'alternate', ['data'])
             add_name(expr['alternate'], info, 'alternate')
-        elif expr.has_key('struct'):
+        elif 'struct' in expr:
             check_keys(expr_elem, 'struct', ['data'], ['base'])
             add_struct(expr, info)
-        elif expr.has_key('command'):
+        elif 'command' in expr:
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response'])
             add_name(expr['command'], info, 'command')
-        elif expr.has_key('event'):
+        elif 'event' in expr:
             check_keys(expr_elem, 'event', [], ['data'])
             add_name(expr['event'], info, 'event')
         else:
@@ -718,11 +753,11 @@ def check_exprs(exprs):
     # Try again for hidden UnionKind enum
     for expr_elem in exprs:
         expr = expr_elem['expr']
-        if expr.has_key('union'):
+        if 'union' in expr:
             if not discriminator_find_enum_define(expr):
                 add_enum('%sKind' % expr['union'], expr_elem['info'],
                          implicit=True)
-        elif expr.has_key('alternate'):
+        elif 'alternate' in expr:
             add_enum('%sKind' % expr['alternate'], expr_elem['info'],
                      implicit=True)
 
@@ -731,17 +766,17 @@ def check_exprs(exprs):
         expr = expr_elem['expr']
         info = expr_elem['info']
 
-        if expr.has_key('enum'):
+        if 'enum' in expr:
             check_enum(expr, info)
-        elif expr.has_key('union'):
+        elif 'union' in expr:
             check_union(expr, info)
-        elif expr.has_key('alternate'):
+        elif 'alternate' in expr:
             check_alternate(expr, info)
-        elif expr.has_key('struct'):
+        elif 'struct' in expr:
             check_struct(expr, info)
-        elif expr.has_key('command'):
+        elif 'command' in expr:
             check_command(expr, info)
-        elif expr.has_key('event'):
+        elif 'event' in expr:
             check_event(expr, info)
         else:
             assert False, 'unexpected meta type'
@@ -993,6 +1028,7 @@ def check(self, schema, members, seen):
             vseen = dict(seen)
             v.check(schema, self.tag_member.type, vseen)
 
+
 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
@@ -1292,6 +1328,7 @@ def camel_case(name):
             new_name += ch.lower()
     return new_name
 
+
 # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
 # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
 # ENUM24_Name -> ENUM24_NAME
@@ -1306,14 +1343,14 @@ def camel_to_upper(value):
         c = c_fun_str[i]
         # When c is upper and no "_" appears before, do more checks
         if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
-            # Case 1: next string is lower
-            # Case 2: previous string is digit
-            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
-            c_fun_str[i - 1].isdigit():
+            if i < l - 1 and c_fun_str[i + 1].islower():
+                new_name += '_'
+            elif c_fun_str[i - 1].isdigit():
                 new_name += '_'
         new_name += c
     return new_name.lstrip('_').upper()
 
+
 def c_enum_const(type_name, const_name, prefix=None):
     if prefix is not None:
         type_name = prefix
@@ -1321,6 +1358,7 @@ def c_enum_const(type_name, const_name, prefix=None):
 
 c_name_trans = string.maketrans('.-', '__')
 
+
 # Map @name to a valid C identifier.
 # If @protect, avoid returning certain ticklish identifiers (like
 # C keywords) by prepending "q_".
@@ -1333,15 +1371,16 @@ def c_enum_const(type_name, const_name, prefix=None):
 def c_name(name, protect=True):
     # ANSI X3J11/88-090, 3.1.1
     c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
-                     'default', 'do', 'double', 'else', 'enum', 'extern', 'float',
-                     'for', 'goto', 'if', 'int', 'long', 'register', 'return',
-                     'short', 'signed', 'sizeof', 'static', 'struct', 'switch',
-                     'typedef', 'union', 'unsigned', 'void', 'volatile', 'while'])
+                     'default', 'do', 'double', 'else', 'enum', 'extern',
+                     'float', 'for', 'goto', 'if', 'int', 'long', 'register',
+                     'return', 'short', 'signed', 'sizeof', 'static',
+                     'struct', 'switch', 'typedef', 'union', 'unsigned',
+                     'void', 'volatile', 'while'])
     # ISO/IEC 9899:1999, 6.4.1
     c99_words = set(['inline', 'restrict', '_Bool', '_Complex', '_Imaginary'])
     # ISO/IEC 9899:2011, 6.4.1
-    c11_words = set(['_Alignas', '_Alignof', '_Atomic', '_Generic', '_Noreturn',
-                     '_Static_assert', '_Thread_local'])
+    c11_words = set(['_Alignas', '_Alignof', '_Atomic', '_Generic',
+                     '_Noreturn', '_Static_assert', '_Thread_local'])
     # GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
     # excluding _.*
     gcc_words = set(['asm', 'typeof'])
@@ -1357,29 +1396,34 @@ def c_name(name, protect=True):
                      'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
     # namespace pollution:
     polluted_words = set(['unix', 'errno'])
-    if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
+    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)
 
 eatspace = '\033EATSPACE.'
 pointer_suffix = ' *' + eatspace
 
+
 def genindent(count):
     ret = ""
-    for i in range(count):
+    for _ in range(count):
         ret += " "
     return ret
 
 indent_level = 0
 
+
 def push_indent(indent_amount=4):
     global indent_level
     indent_level += indent_amount
 
+
 def pop_indent(indent_amount=4):
     global indent_level
     indent_level -= indent_amount
 
+
 # Generate @code with @kwds interpolated.
 # Obey indent_level, and strip eatspace.
 def cgen(code, **kwds):
@@ -1392,6 +1436,7 @@ def cgen(code, **kwds):
         raw = raw[0]
     return re.sub(re.escape(eatspace) + ' *', '', raw)
 
+
 def mcgen(code, **kwds):
     if code[0] == '\n':
         code = code[1:]
@@ -1401,6 +1446,7 @@ def mcgen(code, **kwds):
 def guardname(filename):
     return c_name(filename, protect=False).upper()
 
+
 def guardstart(name):
     return mcgen('''
 
@@ -1410,6 +1456,7 @@ def guardstart(name):
 ''',
                  name=guardname(name))
 
+
 def guardend(name):
     return mcgen('''
 
@@ -1418,6 +1465,7 @@ def guardend(name):
 ''',
                  name=guardname(name))
 
+
 def gen_enum_lookup(name, values, prefix=None):
     ret = mcgen('''
 
@@ -1439,6 +1487,7 @@ def gen_enum_lookup(name, values, prefix=None):
                  max_index=max_index)
     return ret
 
+
 def gen_enum(name, values, prefix=None):
     # append automatically generated _MAX value
     enum_values = values + ['MAX']
@@ -1470,6 +1519,7 @@ def gen_enum(name, values, prefix=None):
                  c_name=c_name(name))
     return ret
 
+
 def gen_params(arg_type, extra):
     if not arg_type:
         return extra
@@ -1486,11 +1536,67 @@ def gen_params(arg_type, extra):
         ret += sep + extra
     return ret
 
+
+def gen_err_check(label='out', skiperr=False):
+    if skiperr:
+        return ''
+    return mcgen('''
+    if (err) {
+        goto %(label)s;
+    }
+''',
+                 label=label)
+
+
+def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
+    ret = ''
+    if skiperr:
+        errparg = 'NULL'
+    else:
+        errparg = '&err'
+
+    for memb in members:
+        if memb.optional:
+            ret += mcgen('''
+    visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)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
+        if need_cast and memb.type.name == 'str':
+            cast = '(char **)'
+        else:
+            cast = ''
+
+        ret += mcgen('''
+    visit_type_%(c_type)s(v, %(cast)s&%(prefix)s%(c_name)s, "%(name)s", %(errp)s);
+''',
+                     c_type=memb.type.c_name(), prefix=prefix, cast=cast,
+                     c_name=c_name(memb.name), name=memb.name,
+                     errp=errparg)
+        ret += gen_err_check(skiperr=skiperr)
+
+        if memb.optional:
+            pop_indent()
+            ret += mcgen('''
+    }
+''')
+    return ret
+
+
 #
 # Common command line parsing
 #
 
-def parse_command_line(extra_options = "", extra_long_options = []):
+
+def parse_command_line(extra_options="", extra_long_options=[]):
 
     try:
         opts, args = getopt.gnu_getopt(sys.argv[1:],
@@ -1541,6 +1647,7 @@ def parse_command_line(extra_options = "", extra_long_options = []):
 # Generate output files with boilerplate
 #
 
+
 def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
                 c_comment, h_comment):
     guard = guardname(prefix + h_file)
@@ -1568,7 +1675,7 @@ def maybe_open(really, name, opt):
 /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
 %(comment)s
 ''',
-                     comment = c_comment))
+                     comment=c_comment))
 
     fdecl.write(mcgen('''
 /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
@@ -1577,10 +1684,11 @@ def maybe_open(really, name, opt):
 #define %(guard)s
 
 ''',
-                      comment = h_comment, guard = guard))
+                      comment=h_comment, guard=guard))
 
     return (fdef, fdecl)
 
+
 def close_output(fdef, fdecl):
     fdecl.write('''
 #endif

tests/Makefile

@@ -224,52 +224,130 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 
-check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
-	comments.json empty.json enum-empty.json enum-missing-data.json \
-	enum-wrong-data.json enum-int-member.json enum-dict-member.json \
-	enum-clash-member.json enum-max-member.json enum-union-clash.json \
-	enum-bad-name.json enum-bad-prefix.json \
-	funny-char.json indented-expr.json \
-	missing-type.json bad-ident.json ident-with-escape.json \
-	escape-outside-string.json unknown-escape.json \
-	escape-too-short.json escape-too-big.json unicode-str.json \
-	double-type.json bad-base.json bad-type-bool.json bad-type-int.json \
-	bad-type-dict.json double-data.json unknown-expr-key.json \
-	redefined-type.json redefined-command.json redefined-builtin.json \
-	redefined-event.json command-int.json bad-data.json event-max.json \
-	type-bypass-bad-gen.json \
-	args-invalid.json \
-	args-array-empty.json args-array-unknown.json args-int.json \
-	args-unknown.json args-member-unknown.json args-member-array.json \
-	args-member-array-bad.json args-alternate.json args-union.json \
-	args-any.json \
-	returns-array-bad.json returns-int.json returns-dict.json \
-	returns-unknown.json returns-alternate.json returns-whitelist.json \
-	missing-colon.json missing-comma-list.json missing-comma-object.json \
-	struct-data-invalid.json struct-member-invalid.json \
-	nested-struct-data.json non-objects.json \
-	qapi-schema-test.json quoted-structural-chars.json \
-	leading-comma-list.json leading-comma-object.json \
-	trailing-comma-list.json trailing-comma-object.json \
-	unclosed-list.json unclosed-object.json unclosed-string.json \
-	duplicate-key.json union-invalid-base.json union-bad-branch.json \
-	union-optional-branch.json union-unknown.json union-max.json \
-	flat-union-optional-discriminator.json flat-union-no-base.json \
-	flat-union-invalid-discriminator.json flat-union-inline.json \
-	flat-union-invalid-branch-key.json flat-union-reverse-define.json \
-	flat-union-string-discriminator.json union-base-no-discriminator.json \
-	flat-union-bad-discriminator.json flat-union-bad-base.json \
-	flat-union-base-any.json \
-	flat-union-array-branch.json flat-union-int-branch.json \
-	flat-union-base-union.json flat-union-branch-clash.json \
-	alternate-nested.json alternate-unknown.json alternate-clash.json \
-	alternate-good.json alternate-base.json alternate-array.json \
-	alternate-conflict-string.json alternate-conflict-dict.json \
-	include-simple.json include-relpath.json include-format-err.json \
-	include-non-file.json include-no-file.json include-before-err.json \
-	include-nested-err.json include-self-cycle.json include-cycle.json \
-	include-repetition.json event-nest-struct.json event-case.json \
-	struct-base-clash.json struct-base-clash-deep.json )
+qapi-schema += alternate-array.json
+qapi-schema += alternate-base.json
+qapi-schema += alternate-clash.json
+qapi-schema += alternate-conflict-dict.json
+qapi-schema += alternate-conflict-string.json
+qapi-schema += alternate-empty.json
+qapi-schema += alternate-good.json
+qapi-schema += alternate-nested.json
+qapi-schema += alternate-unknown.json
+qapi-schema += args-alternate.json
+qapi-schema += args-any.json
+qapi-schema += args-array-empty.json
+qapi-schema += args-array-unknown.json
+qapi-schema += args-int.json
+qapi-schema += args-invalid.json
+qapi-schema += args-member-array-bad.json
+qapi-schema += args-member-array.json
+qapi-schema += args-member-unknown.json
+qapi-schema += args-name-clash.json
+qapi-schema += args-union.json
+qapi-schema += args-unknown.json
+qapi-schema += bad-base.json
+qapi-schema += bad-data.json
+qapi-schema += bad-ident.json
+qapi-schema += bad-type-bool.json
+qapi-schema += bad-type-dict.json
+qapi-schema += bad-type-int.json
+qapi-schema += command-int.json
+qapi-schema += comments.json
+qapi-schema += double-data.json
+qapi-schema += double-type.json
+qapi-schema += duplicate-key.json
+qapi-schema += empty.json
+qapi-schema += enum-bad-name.json
+qapi-schema += enum-bad-prefix.json
+qapi-schema += enum-clash-member.json
+qapi-schema += enum-dict-member.json
+qapi-schema += enum-empty.json
+qapi-schema += enum-int-member.json
+qapi-schema += enum-max-member.json
+qapi-schema += enum-missing-data.json
+qapi-schema += enum-union-clash.json
+qapi-schema += enum-wrong-data.json
+qapi-schema += escape-outside-string.json
+qapi-schema += escape-too-big.json
+qapi-schema += escape-too-short.json
+qapi-schema += event-case.json
+qapi-schema += event-max.json
+qapi-schema += event-nest-struct.json
+qapi-schema += flat-union-array-branch.json
+qapi-schema += flat-union-bad-base.json
+qapi-schema += flat-union-bad-discriminator.json
+qapi-schema += flat-union-base-any.json
+qapi-schema += flat-union-base-union.json
+qapi-schema += flat-union-clash-branch.json
+qapi-schema += flat-union-clash-member.json
+qapi-schema += flat-union-clash-type.json
+qapi-schema += flat-union-empty.json
+qapi-schema += flat-union-inline.json
+qapi-schema += flat-union-int-branch.json
+qapi-schema += flat-union-invalid-branch-key.json
+qapi-schema += flat-union-invalid-discriminator.json
+qapi-schema += flat-union-no-base.json
+qapi-schema += flat-union-optional-discriminator.json
+qapi-schema += flat-union-reverse-define.json
+qapi-schema += flat-union-string-discriminator.json
+qapi-schema += funny-char.json
+qapi-schema += ident-with-escape.json
+qapi-schema += include-before-err.json
+qapi-schema += include-cycle.json
+qapi-schema += include-format-err.json
+qapi-schema += include-nested-err.json
+qapi-schema += include-no-file.json
+qapi-schema += include-non-file.json
+qapi-schema += include-relpath.json
+qapi-schema += include-repetition.json
+qapi-schema += include-self-cycle.json
+qapi-schema += include-simple.json
+qapi-schema += indented-expr.json
+qapi-schema += leading-comma-list.json
+qapi-schema += leading-comma-object.json
+qapi-schema += missing-colon.json
+qapi-schema += missing-comma-list.json
+qapi-schema += missing-comma-object.json
+qapi-schema += missing-type.json
+qapi-schema += nested-struct-data.json
+qapi-schema += non-objects.json
+qapi-schema += qapi-schema-test.json
+qapi-schema += quoted-structural-chars.json
+qapi-schema += redefined-builtin.json
+qapi-schema += redefined-command.json
+qapi-schema += redefined-event.json
+qapi-schema += redefined-type.json
+qapi-schema += returns-alternate.json
+qapi-schema += returns-array-bad.json
+qapi-schema += returns-dict.json
+qapi-schema += returns-int.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
+qapi-schema += struct-member-invalid.json
+qapi-schema += trailing-comma-list.json
+qapi-schema += trailing-comma-object.json
+qapi-schema += type-bypass-bad-gen.json
+qapi-schema += unclosed-list.json
+qapi-schema += unclosed-object.json
+qapi-schema += unclosed-string.json
+qapi-schema += unicode-str.json
+qapi-schema += union-bad-branch.json
+qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-clash-branches.json
+qapi-schema += union-clash-data.json
+qapi-schema += union-clash-type.json
+qapi-schema += union-empty.json
+qapi-schema += union-invalid-base.json
+qapi-schema += union-max.json
+qapi-schema += union-optional-branch.json
+qapi-schema += union-unknown.json
+qapi-schema += unknown-escape.json
+qapi-schema += unknown-expr-key.json
+check-qapi-schema-y := $(addprefix tests/qapi-schema/, $(qapi-schema))
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
 	tests/test-qmp-commands.h tests/test-qapi-event.h \

tests/qapi-schema/alternate-clash.err

@@ -1 +1 @@
-tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE' clashes with 'one'
+tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'

tests/qapi-schema/alternate-clash.json

@@ -1,3 +1,8 @@
-# we detect C enum collisions in an alternate
+# Alternate branch name collision
+# Reject an alternate that would result in a collision in generated C
+# names (this would try to generate two enum values 'ALT1_KIND_A_B').
+# TODO: In the future, if alternates are simplified to not generate
+# the implicit Alt1Kind enum, we would still have a collision with the
+# resulting C union trying to have two members named 'a_b'.
 { 'alternate': 'Alt1',
-  'data': { 'one': 'str', 'ONE': 'int' } }
+  'data': { 'a-b': 'str', 'a_b': 'int' } }

tests/qapi-schema/alternate-empty.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/alternate-empty.json

@@ -0,0 +1,2 @@
+# FIXME - alternates should list at least two types to be useful
+{ 'alternate': 'Alt', 'data': { 'i': 'int' } }

tests/qapi-schema/alternate-empty.out

@@ -0,0 +1,4 @@
+object :empty
+alternate Alt
+    case i: int
+enum AltKind ['i']

tests/qapi-schema/alternate-nested.json

@@ -2,4 +2,4 @@
 { 'alternate': 'Alt1',
   'data': { 'name': 'str', 'value': 'int' } }
 { 'alternate': 'Alt2',
-  'data': { 'nested': 'Alt1' } }
+  'data': { 'nested': 'Alt1', 'b': 'bool' } }

tests/qapi-schema/alternate-unknown.json

@@ -1,3 +1,3 @@
 # we reject an alternate with unknown type in branch
 { 'alternate': 'Alt',
-  'data': { 'unknown': 'MissingType' } }
+  'data': { 'unknown': 'MissingType', 'i': 'int' } }

tests/qapi-schema/args-name-clash.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/args-name-clash.json

@@ -0,0 +1,5 @@
+# C member name collision
+# FIXME - This parses, but fails to compile, because the C struct is given
+# two 'a_b' members.  Either reject this at parse time, or munge the C names
+# to avoid the collision.
+{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }

tests/qapi-schema/args-name-clash.out

@@ -0,0 +1,6 @@
+object :empty
+object :obj-oops-arg
+    member a-b: str optional=False
+    member a_b: str optional=False
+command oops :obj-oops-arg -> None
+   gen=True success_response=True

tests/qapi-schema/duplicate-key.err

@@ -1 +1 @@
-tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key"
+tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key"

tests/qapi-schema/duplicate-key.json

@@ -1,2 +1,3 @@
+# QAPI cannot include the same key more than once in any {}
 { 'key': 'value',
   'key': 'value' }

tests/qapi-schema/flat-union-bad-base.err

@@ -1 +1 @@
-tests/qapi-schema/flat-union-bad-base.json:9: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-bad-base.json:9: 'base' for union 'TestUnion' should be a type name

tests/qapi-schema/flat-union-base-any.err

@@ -1 +1 @@
-tests/qapi-schema/flat-union-base-any.json:8: Base 'any' is not a valid struct
+tests/qapi-schema/flat-union-base-any.json:8: 'base' for union 'TestUnion' cannot use built-in type 'any'

tests/qapi-schema/flat-union-base-union.err

@@ -1 +1 @@
-tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a valid struct
+tests/qapi-schema/flat-union-base-union.json:14: 'base' for union 'TestUnion' cannot use union type 'UnionBase'

tests/qapi-schema/flat-union-base-union.json

@@ -1,4 +1,7 @@
-# we require the base to be a struct
+# For now, we require the base to be a struct without variants
+# TODO: It would be possible to allow a union as a base, as long as all
+# permutations of QMP names exposed by base do not clash with any QMP
+# member names added by local variants.
 { 'enum': 'TestEnum',
   'data': [ 'value1', 'value2' ] }
 { 'struct': 'TestTypeA',

tests/qapi-schema/flat-union-branch-clash.err

@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'

tests/qapi-schema/flat-union-clash-branch.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/flat-union-clash-branch.json

@@ -0,0 +1,18 @@
+# Flat union branch name collision
+# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
+# (one from the base member, the other from the branch name).  We should
+# either reject the collision at parse time, or munge the generated branch
+# name to allow this to compile.
+{ 'enum': 'TestEnum',
+  'data': [ 'base', 'c-d' ] }
+{ 'struct': 'Base',
+  'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
+{ 'struct': 'Branch1',
+  'data': { 'string': 'str' } }
+{ 'struct': 'Branch2',
+  'data': { 'value': 'int' } }
+{ 'union': 'TestUnion',
+  'base': 'Base',
+  'discriminator': 'enum1',
+  'data': { 'base': 'Branch1',
+            'c-d': 'Branch2' } }

tests/qapi-schema/flat-union-clash-branch.out

@@ -0,0 +1,14 @@
+object :empty
+object Base
+    member enum1: TestEnum optional=False
+    member c_d: str optional=True
+object Branch1
+    member string: str optional=False
+object Branch2
+    member value: int optional=False
+enum TestEnum ['base', 'c-d']
+object TestUnion
+    base Base
+    tag enum1
+    case base: Branch1
+    case c-d: Branch2

tests/qapi-schema/flat-union-clash-member.err

@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'

tests/qapi-schema/flat-union-branch-clash.json → tests/qapi-schema/flat-union-clash-member.json

@@ -1,4 +1,5 @@
-# we check for no duplicate keys between branches and base
+# We check for no duplicate keys between branch members and base
+# base's member 'name' clashes with Branch1's
 { 'enum': 'TestEnum',
   'data': [ 'value1', 'value2' ] }
 { 'struct': 'Base',

tests/qapi-schema/flat-union-clash-type.err

@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'

tests/qapi-schema/flat-union-clash-type.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/flat-union-clash-type.json

@@ -0,0 +1,14 @@
+# Flat union branch 'type'
+# Reject this, because we would have a clash in generated C, between the
+# outer tag 'type' and the branch name 'type' within the union.
+# TODO: We could munge the generated C branch name to let it compile.
+{ 'enum': 'TestEnum',
+  'data': [ 'type' ] }
+{ 'struct': 'Base',
+  'data': { 'type': 'TestEnum' } }
+{ 'struct': 'Branch1',
+  'data': { 'string': 'str' } }
+{ 'union': 'TestUnion',
+  'base': 'Base',
+  'discriminator': 'type',
+  'data': { 'type': 'Branch1' } }

tests/qapi-schema/flat-union-empty.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/flat-union-empty.json

@@ -0,0 +1,4 @@
+# FIXME - flat unions should not be empty
+{ 'enum': 'Empty', 'data': [ ] }
+{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
+{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }

tests/qapi-schema/flat-union-empty.out

@@ -0,0 +1,7 @@
+object :empty
+object Base
+    member type: Empty optional=False
+enum Empty []
+object Union
+    base Base
+    tag type

tests/qapi-schema/flat-union-inline.err

@@ -1 +1 @@
-tests/qapi-schema/flat-union-inline.json:7: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 'TestUnion' should be a type name

tests/qapi-schema/flat-union-inline.json

@@ -1,11 +1,11 @@
 # we require branches to be a struct name
-# TODO: should we allow anonymous inline types?
+# TODO: should we allow anonymous inline branch types?
 { 'enum': 'TestEnum',
   'data': [ 'value1', 'value2' ] }
 { 'struct': 'Base',
   'data': { 'enum1': 'TestEnum', 'kind': 'str' } }
 { 'union': 'TestUnion',
-  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
+  'base': 'Base',
   'discriminator': 'enum1',
   'data': { 'value1': { 'string': 'str' },
             'value2': { 'integer': 'int' } } }

tests/qapi-schema/flat-union-no-base.err

@@ -1 +1 @@
-tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a base

tests/qapi-schema/include-non-file.err

@@ -1 +1 @@
-tests/qapi-schema/include-non-file.json:1: Expected a file name (string), got: ['foo', 'bar']
+tests/qapi-schema/include-non-file.json:1: Value of 'include' must be a string

tests/qapi-schema/include-non-file.json

@@ -1 +1 @@
-{ 'include': [ 'foo', 'bar' ] }
+{ 'include': {} }

tests/qapi-schema/qapi-schema-test.json

@@ -32,11 +32,14 @@
             'dict1': 'UserDefTwoDict' } }
 
 # for testing unions
+# Among other things, test that a name collision between branches does
+# not cause any problems (since only one branch can be in use at a time),
+# by intentionally using two branches that both have a C member 'a_b'
 { 'struct': 'UserDefA',
-  'data': { 'boolean': 'bool' } }
+  'data': { 'boolean': 'bool', '*a_b': 'int' } }
 
 { 'struct': 'UserDefB',
-  'data': { 'intb': 'int' } }
+  'data': { 'intb': 'int', '*a-b': 'bool' } }
 
 { 'union': 'UserDefFlatUnion',
   'base': 'UserDefUnionBase',   # intentional forward reference
@@ -64,6 +67,14 @@
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
+# for testing use of 'number' within alternates
+{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } }
+{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } }
+{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
+{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } }
+{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
+{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],

tests/qapi-schema/qapi-schema-test.out

@@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg
 object :obj-user_def_cmd3-arg
     member a: int optional=False
     member b: int optional=True
+alternate AltIntNum
+    case i: int
+    case n: number
+enum AltIntNumKind ['i', 'n']
+alternate AltNumInt
+    case n: number
+    case i: int
+enum AltNumIntKind ['n', 'i']
+alternate AltNumStr
+    case n: number
+    case s: str
+enum AltNumStrKind ['n', 's']
+alternate AltStrBool
+    case s: str
+    case b: bool
+enum AltStrBoolKind ['s', 'b']
+alternate AltStrInt
+    case s: str
+    case i: int
+enum AltStrIntKind ['s', 'i']
+alternate AltStrNum
+    case s: str
+    case n: number
+enum AltStrNumKind ['s', 'n']
 event EVENT_A None
 event EVENT_B None
 event EVENT_C :obj-EVENT_C-arg
@@ -71,6 +95,7 @@ enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 object UserDefA
     member boolean: bool optional=False
+    member a_b: int optional=True
 alternate UserDefAlternate
     case uda: UserDefA
     case s: str
@@ -78,6 +103,7 @@ alternate UserDefAlternate
 enum UserDefAlternateKind ['uda', 's', 'i']
 object UserDefB
     member intb: int optional=False
+    member a-b: bool optional=True
 object UserDefC
     member string1: str optional=False
     member string2: str optional=False

tests/qapi-schema/struct-base-clash-base.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/struct-base-clash-base.json

@@ -0,0 +1,9 @@
+# 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' } }

tests/qapi-schema/struct-base-clash-base.out

@@ -0,0 +1,5 @@
+object :empty
+object Base
+object Sub
+    base Base
+    member base: str optional=False

tests/qapi-schema/struct-base-clash-deep.err

@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:7: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'

tests/qapi-schema/struct-base-clash-deep.json

@@ -1,4 +1,7 @@
-# we check for no duplicate keys with indirect base
+# Reject attempts to duplicate QMP members
+# Here, 'name' would have to appear twice on the wire, locally and
+# indirectly for the grandparent base; the collision doesn't care that
+# one instance is optional.
 { 'struct': 'Base',
   'data': { 'name': 'str' } }
 { 'struct': 'Mid',

tests/qapi-schema/struct-base-clash.err

@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:4: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'

tests/qapi-schema/struct-base-clash.json

@@ -1,4 +1,5 @@
-# we check for no duplicate keys with base
+# Reject attempts to duplicate QMP members
+# Here, 'name' would have to appear twice on the wire, locally and for base.
 { 'struct': 'Base',
   'data': { 'name': 'str' } }
 { 'struct': 'Sub',

tests/qapi-schema/union-clash-branches.err

@@ -0,0 +1 @@
+tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'

tests/qapi-schema/union-clash-branches.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/union-clash-branches.json

@@ -0,0 +1,5 @@
+# Union branch name collision
+# Reject a union that would result in a collision in generated C names (this
+# would try to generate two enum values 'TEST_UNION_KIND_A_B').
+{ 'union': 'TestUnion',
+  'data': { 'a-b': 'int', 'a_b': 'str' } }

tests/qapi-schema/union-clash-data.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/union-clash-data.json

@@ -0,0 +1,7 @@
+# Union branch 'data'
+# FIXME: this parses, but then fails to compile due to a duplicate 'data'
+# (one from the branch name, another as a filler to avoid an empty union).
+# we should either detect the collision at parse time, or change the
+# generated struct to allow this to compile.
+{ 'union': 'TestUnion',
+  'data': { 'data': 'int' } }

tests/qapi-schema/union-clash-data.out

@@ -0,0 +1,6 @@
+object :empty
+object :obj-int-wrapper
+    member data: int optional=False
+object TestUnion
+    case data: :obj-int-wrapper
+enum TestUnionKind ['data']

tests/qapi-schema/union-clash-type.err

@@ -0,0 +1 @@
+tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)'

tests/qapi-schema/union-clash-type.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/union-clash-type.json

@@ -0,0 +1,9 @@
+# Union branch 'type'
+# Reject this, because we would have a clash in generated C, between the
+# simple union's implicit tag member 'kind' and the branch name 'kind'
+# within the union.
+# TODO: Even when the generated C is switched to use 'type' rather than
+# 'kind', to match the QMP spelling, the collision should still be detected.
+# Or, we could munge the branch name to allow compilation.
+{ 'union': 'TestUnion',
+  'data': { 'kind': 'int', 'type': 'str' } }

tests/qapi-schema/union-empty.exit

@@ -0,0 +1 @@
+0

tests/qapi-schema/union-empty.json

@@ -0,0 +1,2 @@
+# FIXME - unions should not be empty
+{ 'union': 'Union', 'data': { } }

tests/qapi-schema/union-empty.out

@@ -0,0 +1,3 @@
+object :empty
+object Union
+enum UnionKind []

tests/qapi-schema/union-invalid-base.err

@@ -1 +1 @@
-tests/qapi-schema/union-invalid-base.json:8: Base 'int' is not a valid struct
+tests/qapi-schema/union-invalid-base.json:8: 'base' for union 'TestUnion' cannot use built-in type 'int'

tests/test-qmp-input-visitor.c

@@ -371,12 +371,135 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
     UserDefAlternate *tmp;
 
     v = visitor_input_test_init(data, "42");
-
-    visit_type_UserDefAlternate(v, &tmp, NULL, &err);
-    g_assert(err == NULL);
+    visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
     g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
     g_assert_cmpint(tmp->i, ==, 42);
     qapi_free_UserDefAlternate(tmp);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "'string'");
+    visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
+    g_assert_cmpstr(tmp->s, ==, "string");
+    qapi_free_UserDefAlternate(tmp);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "false");
+    visit_type_UserDefAlternate(v, &tmp, NULL, &err);
+    g_assert(err);
+    error_free(err);
+    err = NULL;
+    qapi_free_UserDefAlternate(tmp);
+    visitor_input_teardown(data, NULL);
+}
+
+static void test_visitor_in_alternate_number(TestInputVisitorData *data,
+                                             const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    AltStrBool *asb;
+    AltStrNum *asn;
+    AltNumStr *ans;
+    AltStrInt *asi;
+    AltIntNum *ain;
+    AltNumInt *ani;
+
+    /* Parsing an int */
+
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltStrBool(v, &asb, NULL, &err);
+    g_assert(err);
+    error_free(err);
+    err = NULL;
+    qapi_free_AltStrBool(asb);
+    visitor_input_teardown(data, NULL);
+
+    /* FIXME: Order of alternate should not affect semantics; asn should
+     * parse the same as ans */
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltStrNum(v, &asn, NULL, &err);
+    /* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */
+    /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
+    g_assert(err);
+    error_free(err);
+    err = NULL;
+    qapi_free_AltStrNum(asn);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+    g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
+    g_assert_cmpfloat(ans->n, ==, 42);
+    qapi_free_AltNumStr(ans);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltStrInt(v, &asi, NULL, &error_abort);
+    g_assert_cmpint(asi->kind, ==, ALT_STR_INT_KIND_I);
+    g_assert_cmpint(asi->i, ==, 42);
+    qapi_free_AltStrInt(asi);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltIntNum(v, &ain, NULL, &error_abort);
+    g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_I);
+    g_assert_cmpint(ain->i, ==, 42);
+    qapi_free_AltIntNum(ain);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltNumInt(v, &ani, NULL, &error_abort);
+    g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_I);
+    g_assert_cmpint(ani->i, ==, 42);
+    qapi_free_AltNumInt(ani);
+    visitor_input_teardown(data, NULL);
+
+    /* Parsing a double */
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltStrBool(v, &asb, NULL, &err);
+    g_assert(err);
+    error_free(err);
+    err = NULL;
+    qapi_free_AltStrBool(asb);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltStrNum(v, &asn, NULL, &error_abort);
+    g_assert_cmpint(asn->kind, ==, ALT_STR_NUM_KIND_N);
+    g_assert_cmpfloat(asn->n, ==, 42.5);
+    qapi_free_AltStrNum(asn);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+    g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
+    g_assert_cmpfloat(ans->n, ==, 42.5);
+    qapi_free_AltNumStr(ans);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltStrInt(v, &asi, NULL, &err);
+    g_assert(err);
+    error_free(err);
+    err = NULL;
+    qapi_free_AltStrInt(asi);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltIntNum(v, &ain, NULL, &error_abort);
+    g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_N);
+    g_assert_cmpfloat(ain->n, ==, 42.5);
+    qapi_free_AltIntNum(ain);
+    visitor_input_teardown(data, NULL);
+
+    v = visitor_input_test_init(data, "42.5");
+    visit_type_AltNumInt(v, &ani, NULL, &error_abort);
+    g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_N);
+    g_assert_cmpfloat(ani->n, ==, 42.5);
+    qapi_free_AltNumInt(ani);
+    visitor_input_teardown(data, NULL);
 }
 
 static void test_native_list_integer_helper(TestInputVisitorData *data,
@@ -720,6 +843,8 @@ int main(int argc, char **argv)
                            &in_visitor_data, test_visitor_in_alternate);
     input_visitor_test_add("/visitor/input/errors",
                            &in_visitor_data, test_visitor_in_errors);
+    input_visitor_test_add("/visitor/input/alternate-number",
+                           &in_visitor_data, test_visitor_in_alternate_number);
     input_visitor_test_add("/visitor/input/native_list/int",
                            &in_visitor_data,
                            test_visitor_in_native_list_int);