Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-02-19' into staging

QAPI patches for 2016-02-19

# gpg: Signature made Fri 19 Feb 2016 10:10:18 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-2016-02-19:
  qapi: Change visit_start_implicit_struct to visit_start_alternate
  qapi: Don't box branches of flat unions
  qapi: Don't box struct branch of alternate
  qapi-visit: Use common idiom in gen_visit_fields_decl()
  qapi: Emit structs used as variants in topological order
  qapi: Adjust layout of FooList types
  qapi-visit: Less indirection in visit_type_Foo_fields()
  qapi-visit: Unify struct and union visit
  qapi: Visit variants in visit_type_FOO_fields()
  qapi-visit: Simplify how we visit common union members
  qapi: Add tests of complex objects within alternate
  qapi: Forbid 'any' inside an alternate
  qapi: Forbid empty unions and useless alternates
  qapi: Simplify excess input reporting in input visitors
  qapi-visit: Honor prefix of discriminator enum

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

3 files changed, 139 insertions, 176 deletions

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7b0dca8..eac90d2 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -2,7 +2,7 @@
 # QAPI types generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (c) 2013-2015 Red Hat Inc.
+# Copyright (c) 2013-2016 Red Hat Inc.
 #
 # Authors:
 #  Anthony Liguori <aliguori@us.ibm.com>
@@ -14,6 +14,11 @@
 from qapi import *
 
 
+# variants must be emitted before their container; track what has already
+# been output
+objects_seen = set()
+
+
 def gen_fwd_object_or_array(name):
     return mcgen('''
 
@@ -26,11 +31,8 @@ def gen_array(name, element_type):
     return mcgen('''
 
 struct %(c_name)s {
-    union {
-        %(c_type)s value;
-        uint64_t padding;
-    };
     %(c_name)s *next;
+    %(c_type)s value;
 };
 ''',
                  c_name=c_name(name), c_type=element_type.c_type())
@@ -52,11 +54,23 @@ def gen_struct_fields(members):
 
 
 def gen_object(name, base, members, variants):
-    ret = mcgen('''
+    if name in objects_seen:
+        return ''
+    objects_seen.add(name)
+
+    ret = ''
+    if variants:
+        for v in variants.variants:
+            if (isinstance(v.type, QAPISchemaObjectType) and
+                    not v.type.is_implicit()):
+                ret += gen_object(v.type.name, v.type.base,
+                                  v.type.local_members, v.type.variants)
+
+    ret += mcgen('''
 
 struct %(c_name)s {
 ''',
-                c_name=c_name(name))
+                 c_name=c_name(name))
 
     if base:
         ret += mcgen('''
@@ -118,11 +132,12 @@ def gen_variants(variants):
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
-        typ = var.simple_union_type() or var.type
+        simple_union_type = var.simple_union_type()
+        typ = simple_union_type or var.type
         ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
-                     c_type=typ.c_type(),
+                     c_type=typ.c_type(is_unboxed=not simple_union_type),
                      c_name=c_name(var.name))
 
     ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0cc9b08..2308268 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
 from qapi import *
 import re
 
-# visit_type_FOO_implicit() is emitted as needed; track if it has already
-# been output.
-implicit_structs_seen = set()
-
 # visit_type_FOO_fields() is always emitted; track if a forward declaration
 # or implementation has already been output.
 struct_fields_seen = set()
@@ -35,52 +31,31 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **
 
 
 def gen_visit_fields_decl(typ):
-    ret = ''
-    if typ.name not in struct_fields_seen:
-        ret += mcgen('''
-
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
-''',
-                     c_type=typ.c_name())
-        struct_fields_seen.add(typ.name)
-    return ret
-
-
-def gen_visit_implicit_struct(typ):
-    if typ in implicit_structs_seen:
+    if typ.name in struct_fields_seen:
         return ''
-    implicit_structs_seen.add(typ)
-
-    ret = gen_visit_fields_decl(typ)
-
-    ret += mcgen('''
-
-static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
-{
-    Error *err = NULL;
+    struct_fields_seen.add(typ.name)
+    return mcgen('''
 
-    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
-    if (!err) {
-        visit_type_%(c_type)s_fields(v, obj, errp);
-        visit_end_implicit_struct(v);
-    }
-    error_propagate(errp, err);
-}
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp);
 ''',
                  c_type=typ.c_name())
-    return ret
 
 
-def gen_visit_struct_fields(name, base, members):
+def gen_visit_struct_fields(name, base, members, variants):
     ret = ''
 
     if base:
         ret += gen_visit_fields_decl(base)
+    if variants:
+        for var in variants.variants:
+            # Ugly special case for simple union TODO get rid of it
+            if not var.simple_union_type():
+                ret += gen_visit_fields_decl(var.type)
 
     struct_fields_seen.add(name)
     ret += mcgen('''
 
-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)
 {
     Error *err = NULL;
 
@@ -89,57 +64,61 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 
     if base:
         ret += mcgen('''
-    visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
+    visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, &err);
 ''',
                      c_type=base.c_name())
         ret += gen_err_check()
 
-    ret += gen_visit_fields(members, prefix='(*obj)->')
+    ret += gen_visit_fields(members, prefix='obj->')
 
-    # 'goto out' produced for base, and by gen_visit_fields() for each member
-    if base or members:
+    if variants:
         ret += mcgen('''
+    switch (obj->%(c_name)s) {
+''',
+                     c_name=c_name(variants.tag_member.name))
 
-out:
-''')
-    ret += mcgen('''
-    error_propagate(errp, err);
-}
+        for var in variants.variants:
+            # TODO ugly special case for simple union
+            simple_union_type = var.simple_union_type()
+            ret += mcgen('''
+    case %(case)s:
+''',
+                         case=c_enum_const(variants.tag_member.type.name,
+                                           var.name,
+                                           variants.tag_member.type.prefix))
+            if simple_union_type:
+                ret += mcgen('''
+        visit_type_%(c_type)s(v, "data", &obj->u.%(c_name)s, &err);
+''',
+                             c_type=simple_union_type.c_name(),
+                             c_name=c_name(var.name))
+            else:
+                ret += mcgen('''
+        visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err);
+''',
+                             c_type=var.type.c_name(),
+                             c_name=c_name(var.name))
+            ret += mcgen('''
+        break;
 ''')
-    return ret
-
 
-def gen_visit_struct(name, base, members):
-    ret = gen_visit_struct_fields(name, base, members)
-
-    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
-    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-    ret += mcgen('''
+        ret += mcgen('''
+    default:
+        abort();
+    }
+''')
 
-void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
-{
-    Error *err = NULL;
+    # 'goto out' produced for base, by gen_visit_fields() for each member,
+    # and if variants were present
+    if base or members or variants:
+        ret += mcgen('''
 
-    visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
-    if (err) {
-        goto out;
-    }
-    if (!*obj) {
-        goto out_obj;
-    }
-    visit_type_%(c_name)s_fields(v, obj, &err);
-    error_propagate(errp, err);
-    err = NULL;
-out_obj:
-    visit_end_struct(v, &err);
 out:
+''')
+    ret += mcgen('''
     error_propagate(errp, err);
 }
-''',
-                 c_name=c_name(name))
-
+''')
     return ret
 
 
@@ -161,7 +140,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     }
 
     for (prev = (GenericList **)obj;
-         !err && (i = visit_next_list(v, prev)) != NULL;
+         !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
          prev = &i) {
         %(c_name)s *native_i = (%(c_name)s *)i;
         visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
@@ -190,45 +169,62 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error
 
 def gen_visit_alternate(name, variants):
     promote_int = 'true'
+    ret = ''
     for var in variants.variants:
         if var.type.alternate_qtype() == 'QTYPE_QINT':
             promote_int = 'false'
+        if isinstance(var.type, QAPISchemaObjectType):
+            ret += gen_visit_fields_decl(var.type)
 
-    ret = mcgen('''
+    ret += mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
+                          %(promote_int)s, &err);
     if (err) {
         goto out;
     }
-    visit_get_next_type(v, name, &(*obj)->type, %(promote_int)s, &err);
-    if (err) {
-        goto out_obj;
-    }
     switch ((*obj)->type) {
 ''',
-                c_name=c_name(name), promote_int=promote_int)
+                 c_name=c_name(name), promote_int=promote_int)
 
     for var in variants.variants:
         ret += mcgen('''
     case %(case)s:
+''',
+                     case=var.type.alternate_qtype())
+        if isinstance(var.type, QAPISchemaObjectType):
+            ret += mcgen('''
+        visit_start_struct(v, name, NULL, 0, &err);
+        if (err) {
+            break;
+        }
+        visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err);
+        error_propagate(errp, err);
+        err = NULL;
+        visit_end_struct(v, &err);
+''',
+                         c_type=var.type.c_name(),
+                         c_name=c_name(var.name))
+        else:
+            ret += mcgen('''
         visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
-        break;
 ''',
-                     case=var.type.alternate_qtype(),
-                     c_type=var.type.c_name(),
-                     c_name=c_name(var.name))
+                         c_type=var.type.c_name(),
+                         c_name=c_name(var.name))
+        ret += mcgen('''
+        break;
+''')
 
     ret += mcgen('''
     default:
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
     }
-out_obj:
-    visit_end_implicit_struct(v);
+    visit_end_alternate(v);
 out:
     error_propagate(errp, err);
 }
@@ -238,17 +234,13 @@ out:
     return ret
 
 
-def gen_visit_union(name, base, variants):
-    ret = ''
-
-    if base:
-        ret += gen_visit_fields_decl(base)
-
-    for var in variants.variants:
-        # Ugly special case for simple union TODO get rid of it
-        if not var.simple_union_type():
-            ret += gen_visit_implicit_struct(var.type)
+def gen_visit_object(name, base, members, variants):
+    ret = gen_visit_struct_fields(name, base, members, variants)
 
+    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+    # rather than leaving it non-NULL. As currently written, the caller must
+    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
@@ -262,66 +254,16 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     if (!*obj) {
         goto out_obj;
     }
-''',
-                 c_name=c_name(name))
-
-    if base:
-        ret += mcgen('''
-    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
-''',
-                     c_name=base.c_name())
-    else:
-        ret += mcgen('''
-    visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err);
-''',
-                     c_type=variants.tag_member.type.c_name(),
-                     c_name=c_name(variants.tag_member.name),
-                     name=variants.tag_member.name)
-    ret += gen_err_check(label='out_obj')
-    ret += mcgen('''
-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
-        goto out_obj;
-    }
-    switch ((*obj)->%(c_name)s) {
-''',
-                 c_name=c_name(variants.tag_member.name))
-
-    for var in variants.variants:
-        # TODO ugly special case for simple union
-        simple_union_type = var.simple_union_type()
-        ret += mcgen('''
-    case %(case)s:
-''',
-                     case=c_enum_const(variants.tag_member.type.name,
-                                       var.name))
-        if simple_union_type:
-            ret += mcgen('''
-        visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
-''',
-                         c_type=simple_union_type.c_name(),
-                         c_name=c_name(var.name))
-        else:
-            ret += mcgen('''
-        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
-''',
-                         c_type=var.type.c_name(),
-                         c_name=c_name(var.name))
-        ret += mcgen('''
-        break;
-''')
-
-    ret += mcgen('''
-    default:
-        abort();
-    }
-out_obj:
+    visit_type_%(c_name)s_fields(v, *obj, &err);
     error_propagate(errp, err);
     err = NULL;
+out_obj:
     visit_end_struct(v, &err);
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 c_name=c_name(name))
 
     return ret
 
@@ -375,14 +317,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
 
     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_decl(name)
-        if variants:
-            if members:
-                # Members other than variants.tag_member not implemented
-                assert len(members) == 1
-                assert members[0] == variants.tag_member
-            self.defn += gen_visit_union(name, base, variants)
-        else:
-            self.defn += gen_visit_struct(name, base, members)
+        self.defn += gen_visit_object(name, base, members, variants)
 
     def visit_alternate_type(self, name, info, variants):
         self.decl += gen_visit_decl(name)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f40dc9e..8497777 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -590,7 +590,10 @@ def check_union(expr, expr_info):
                                 "Discriminator '%s' must be of enumeration "
                                 "type" % discriminator)
 
-    # Check every branch
+    # Check every branch; don't allow an empty union
+    if len(members) == 0:
+        raise QAPIExprError(expr_info,
+                            "Union '%s' cannot have empty 'data'" % name)
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)
 
@@ -613,7 +616,11 @@ def check_alternate(expr, expr_info):
     members = expr['data']
     types_seen = {}
 
-    # Check every branch
+    # Check every branch; require at least two branches
+    if len(members) < 2:
+        raise QAPIExprError(expr_info,
+                            "Alternate '%s' should have at least two branches "
+                            "in 'data'" % name)
     for (key, value) in members.items():
         check_name(expr_info, "Member of alternate '%s'" % name, key)
 
@@ -622,7 +629,10 @@ def check_alternate(expr, expr_info):
                    value,
                    allow_metas=['built-in', 'union', 'struct', 'enum'])
         qtype = find_alternate_member_qtype(value)
-        assert qtype
+        if not qtype:
+            raise QAPIExprError(expr_info,
+                                "Alternate '%s' member '%s' cannot use "
+                                "type '%s'" % (name, key, value))
         if qtype in types_seen:
             raise QAPIExprError(expr_info,
                                 "Alternate '%s' member '%s' can't "
@@ -814,7 +824,7 @@ class QAPISchemaVisitor(object):
 
 
 class QAPISchemaType(QAPISchemaEntity):
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_unboxed=False):
         return c_name(self.name) + pointer_suffix
 
     def c_null(self):
@@ -847,7 +857,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
     def c_name(self):
         return self.name
 
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_unboxed=False):
         if is_param and self.name == 'str':
             return 'const ' + self._c_type_name
         return self._c_type_name
@@ -881,7 +891,7 @@ class QAPISchemaEnumType(QAPISchemaType):
         # See QAPISchema._make_implicit_enum_type()
         return self.name.endswith('Kind')
 
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_unboxed=False):
         return c_name(self.name)
 
     def member_names(self):
@@ -977,9 +987,11 @@ class QAPISchemaObjectType(QAPISchemaType):
         assert not self.is_implicit()
         return QAPISchemaType.c_name(self)
 
-    def c_type(self, is_param=False):
+    def c_type(self, is_param=False, is_unboxed=False):
         assert not self.is_implicit()
-        return QAPISchemaType.c_type(self)
+        if is_unboxed:
+            return c_name(self.name)
+        return c_name(self.name) + pointer_suffix
 
     def json_type(self):
         return 'object'
@@ -1059,6 +1071,7 @@ class QAPISchemaObjectTypeVariants(object):
         assert bool(tag_member) != bool(tag_name)
         assert (isinstance(tag_name, str) or
                 isinstance(tag_member, QAPISchemaObjectTypeMember))
+        assert len(variants) > 0
         for v in variants:
             assert isinstance(v, QAPISchemaObjectTypeVariant)
         self.tag_name = tag_name