From 2ee6c960ee5228b2f3968e6866e9bb128d94876f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 19 Feb 2018 21:51:35 -0500 Subject: [PATCH] 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 | 1 + qemu/include/qapi/visitor-impl.h | 3 ++- qemu/include/qapi/visitor.h | 7 ++++++- qemu/qapi/qapi-visit-core.c | 4 ++-- qemu/qapi/qmp-input-visitor.c | 4 ++-- qemu/scripts/qapi-visit.py | 15 ++++++++++----- qemu/scripts/qapi.py | 18 +++++++++++++----- 7 files changed, 36 insertions(+), 16 deletions(-) diff --git a/msvc/unicorn/qapi-visit.h b/msvc/unicorn/qapi-visit.h index f7bba48b..3d695733 100644 --- a/msvc/unicorn/qapi-visit.h +++ b/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" diff --git a/qemu/include/qapi/visitor-impl.h b/qemu/include/qapi/visitor-impl.h index 6a45e11e..16181ad2 100644 --- a/qemu/include/qapi/visitor-impl.h +++ b/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, diff --git a/qemu/include/qapi/visitor.h b/qemu/include/qapi/visitor.h index 6ca0ef98..94019d7a 100644 --- a/qemu/include/qapi/visitor.h +++ b/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); diff --git a/qemu/qapi/qapi-visit-core.c b/qemu/qapi/qapi-visit-core.c index 7cc341d5..5d77b487 100644 --- a/qemu/qapi/qapi-visit-core.c +++ b/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); } } diff --git a/qemu/qapi/qmp-input-visitor.c b/qemu/qapi/qmp-input-visitor.c index 5b2cf452..d5886491 100644 --- a/qemu/qapi/qmp-input-visitor.c +++ b/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, diff --git a/qemu/scripts/qapi-visit.py b/qemu/scripts/qapi-visit.py index 233c9fcb..5180e9ca 100644 --- a/qemu/scripts/qapi-visit.py +++ b/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, - var.name), + case=var.type.alternate_qtype(), c_type=var.type.c_name(), c_name=c_name(var.name)) ret += mcgen(''' default: - abort(); + error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "%(name)s"); } out_obj: error_propagate(errp, err); @@ -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" ''', diff --git a/qemu/scripts/qapi.py b/qemu/scripts/qapi.py index 9b3fdef0..be1e0852 100644 --- a/qemu/scripts/qapi.py +++ b/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 - c_key = camel_to_upper(key) + # Check for conflicts in the branch names + 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) - assert v.name in self.tag_member.type.values - if isinstance(v.type, QAPISchemaObjectType): + # 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 + 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,