qapi: Simplify visiting of alternate types

Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.

This has a couple of subtle bugs. First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS.

Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.

Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type(). Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).

However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it). A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.

This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter. This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names). Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.

Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.

There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work). Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).

Backports commit 0426d53c6530606bf7641b83f2b755fe61c280ee from qemu

msvc/unicorn/qapi-visit.h

@@ -17,6 +17,7 @@
 #define QAPI_VISIT_H
 
 #include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
 #include "qapi-types.h"
 
 

qemu/include/qapi/visitor-impl.h

@@ -32,7 +32,8 @@ struct Visitor
 
     void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);
-    void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
+    /* May be NULL; only needed for input visitors. */
+    void (*get_next_type)(Visitor *v, QType *type,
                           const char *name, Error **errp);
     /* Must be set. */
     void (*type_int64)(Visitor *v, int64_t *obj, const char *name,

qemu/include/qapi/visitor.h

@@ -38,7 +38,12 @@ GenericList *visit_next_list(Visitor *v, GenericList **list);
 void visit_end_list(Visitor *v);
 void visit_optional(Visitor *v, bool *present, const char *name,
                     Error **errp);
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+/**
+ * Determine the qtype of the item @name in the current object visit.
+ * For input visitors, set *@type to the correct qtype of a qapi
+ * alternate type; for other visitors, leave *@type unchanged.
+ */
+void visit_get_next_type(Visitor *v, QType *type,
                          const char *name, Error **errp);
 void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);

qemu/qapi/qapi-visit-core.c

@@ -82,11 +82,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
     }
 }
 
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+void visit_get_next_type(Visitor *v, QType *type,
                          const char *name, Error **errp)
 {
     if (v->get_next_type) {
-        v->get_next_type(v, obj, qtypes, name, errp);
+        v->get_next_type(v, type, name, errp);
     }
 }
 

qemu/qapi/qmp-input-visitor.c

@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v)
     qmp_input_pop(qiv, &error_abort);
 }
 
-static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
+static void qmp_input_get_next_type(Visitor *v, QType *type,
                                     const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
         error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
         return;
     }
-    *kind = qobjects[qobject_type(qobj)];
+    *type = qobject_type(qobj);
 }
 
 static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name,

qemu/scripts/qapi-visit.py

@@ -169,6 +169,7 @@ out:
 
 
 def gen_visit_enum(name):
+    # FIXME cast from enum *obj to int * invalidly assumes enum is int
     return mcgen('''
 
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
@@ -190,7 +191,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     if (err) {
         goto out;
     }
-    visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
+    visit_get_next_type(v, &(*obj)->type, name, &err);
     if (err) {
         goto out_obj;
     }
@@ -198,20 +199,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
 ''',
                 c_name=c_name(name))
 
+    # FIXME: When 'number' but not 'int' is present in the alternate, we
+    # should allow QTYPE_INT to promote to QTYPE_FLOAT.
     for var in variants.variants:
         ret += mcgen('''
     case %(case)s:
         visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
         break;
 ''',
-                     case=c_enum_const(variants.tag_member.type.name,
+                     case=var.type.alternate_qtype(),
-                                       var.name),
                      c_type=var.type.c_name(),
                      c_name=c_name(var.name))
 
     ret += mcgen('''
     default:
-        abort();
+        error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "%(name)s");
     }
 out_obj:
     error_propagate(errp, err);
@@ -220,7 +223,8 @@ out_obj:
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 name=name)
 
     return ret
 
@@ -436,6 +440,7 @@ fdef.write(mcgen('''
 
 fdecl.write(mcgen('''
 #include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',

qemu/scripts/qapi.py

@@ -637,8 +637,8 @@ def check_alternate(expr, expr_info):
     for (key, value) in members.items():
         check_name(expr_info, "Member of alternate '%s'" % name, key)
 
-        # Check for conflicts in the generated enum
+        # Check for conflicts in the branch names
-        c_key = camel_to_upper(key)
+        c_key = c_name(key)
         if c_key in values:
             raise QAPIExprError(expr_info,
                                 "Alternate '%s' member '%s' clashes with '%s'"
@@ -1095,8 +1095,11 @@ class QAPISchemaObjectTypeVariants(object):
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
             v.check(schema)
+            # Union names must match enum values; alternate names are
+            # checked separately. Use 'seen' to tell the two apart.
+            if seen:
                 assert v.name in self.tag_member.type.values
-            if isinstance(v.type, QAPISchemaObjectType):
+                assert isinstance(v.type, QAPISchemaObjectType)
                 v.type.check(schema)
 
     def check_clash(self, schema, info, seen):
@@ -1136,6 +1139,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
     def check(self, schema):
         self.variants.tag_member.check(schema)
         self.variants.check(schema, {})
+        # Alternate branch names have no relation to the tag enum values;
+        # so we have to check for potential name collisions ourselves.
+        seen = {}
+        for v in self.variants.variants:
+            v.check_clash(self.info, seen)
 
     def json_type(self):
         return 'value'
@@ -1343,7 +1351,7 @@ class QAPISchema(object):
         data = expr['data']
         variants = [self._make_variant(key, value)
                     for (key, value) in data.iteritems()]
-        tag_member = self._make_implicit_tag(name, info, variants)
+        tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
         self._def_entity(
             QAPISchemaAlternateType(name, info,
                                     QAPISchemaObjectTypeVariants(None,