From 0426d53c6530606bf7641b83f2b755fe61c280ee Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 1 Dec 2015 22:20:48 -0700 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). Signed-off-by: Eric Blake Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- docs/qapi-code-gen.txt | 3 --- include/qapi/visitor-impl.h | 3 ++- include/qapi/visitor.h | 8 +++++- qapi/qapi-visit-core.c | 4 +-- qapi/qmp-input-visitor.c | 4 +-- scripts/qapi-types.py | 34 -------------------------- scripts/qapi-visit.py | 15 ++++++++---- scripts/qapi.py | 18 ++++++++++---- tests/qapi-schema/alternate-empty.out | 1 - tests/qapi-schema/qapi-schema-test.out | 8 ------ tests/test-qmp-input-visitor.c | 31 +++++++++++------------ tests/test-qmp-output-visitor.c | 4 +-- 12 files changed, 54 insertions(+), 79 deletions(-) diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 79bf072695..128f074a2d 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -384,9 +384,6 @@ where each branch of the union names a QAPI type. For example: 'data': { 'definition': 'BlockdevOptions', 'reference': 'str' } } -Just like for a simple union, an implicit C enum 'NameKind' is created -to enumerate the branches for the alternate 'Name'. - Unlike a union, the discriminator string is never passed on the wire for the Client JSON Protocol. Instead, the value's JSON type serves as an implicit discriminator, which in turn means that an alternate diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 8c0ba57292..7cd1313129 100644 --- a/include/qapi/visitor-impl.h +++ b/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); void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp); diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index a2ad66cf8f..6d25ad27a2 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -38,7 +38,13 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp); void visit_end_list(Visitor *v, Error **errp); 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/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 59ed5067fa..850ca034cb 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -81,11 +81,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/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index eb6e110300..d398de743d 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp) qmp_input_pop(qiv, errp); } -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_int(Visitor *v, int64_t *obj, const char *name, diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 2071846250..84ec858419 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -101,38 +101,6 @@ def gen_upcast(name, base): c_name=c_name(name), base=base.c_name()) -def gen_alternate_qtypes_decl(name): - return mcgen(''' - -extern const int %(c_name)s_qtypes[]; -''', - c_name=c_name(name)) - - -def gen_alternate_qtypes(name, variants): - ret = mcgen(''' - -const int %(c_name)s_qtypes[QTYPE__MAX] = { -''', - c_name=c_name(name)) - - for var in variants.variants: - qtype = var.type.alternate_qtype() - assert qtype - - ret += mcgen(''' - [%(qtype)s] = %(enum_const)s, -''', - qtype=qtype, - enum_const=c_enum_const(variants.tag_member.type.name, - var.name)) - - ret += mcgen(''' -}; -''') - return ret - - def gen_variants(variants): # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide @@ -264,9 +232,7 @@ def visit_object_type(self, name, info, base, members, variants): def visit_alternate_type(self, name, info, variants): self._fwdecl += gen_fwd_object_or_array(name) - self._fwdefn += gen_alternate_qtypes(name, variants) self.decl += gen_object(name, None, [variants.tag_member], variants) - self.decl += gen_alternate_qtypes_decl(name) self._gen_type_cleanup(name) # If you link code generated from multiple schemata, you want only one diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 7ceda18bdb..4797d6e050 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -172,6 +172,7 @@ def gen_visit_list(name, element_type): 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) @@ -193,7 +194,7 @@ def gen_visit_alternate(name, variants): 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; } @@ -201,20 +202,22 @@ def gen_visit_alternate(name, variants): ''', 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); @@ -223,7 +226,8 @@ def gen_visit_alternate(name, variants): out: error_propagate(errp, err); } -''') +''', + name=name) return ret @@ -437,6 +441,7 @@ def visit_alternate_type(self, name, info, variants): fdecl.write(mcgen(''' #include "qapi/visitor.h" +#include "qapi/qmp/qerror.h" #include "%(prefix)sqapi-types.h" ''', diff --git a/scripts/qapi.py b/scripts/qapi.py index c9e4ad2d9e..2b46dd095f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -635,8 +635,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'" @@ -1092,8 +1092,11 @@ def check(self, schema, seen): 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): @@ -1135,6 +1138,11 @@ def check(self, schema): # Not calling self.variants.check_clash(), because there's nothing # to clash with self.variants.check(schema, {}) + # Alternate branch names have no relation to the tag enum values; + # so we have to check for potential name collisions ourselves. + seen = {} + for v in self.variants.variants: + v.check_clash(self.info, seen) def json_type(self): return 'value' @@ -1342,7 +1350,7 @@ def _def_alternate_type(self, expr, info): 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, diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out index 02b9876e76..f78f174111 100644 --- a/tests/qapi-schema/alternate-empty.out +++ b/tests/qapi-schema/alternate-empty.out @@ -1,6 +1,5 @@ object :empty alternate Alt case i: int -enum AltKind ['i'] enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] prefix QTYPE diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index b87cfba599..2c546b708a 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -56,27 +56,21 @@ object :obj-user_def_cmd2-arg 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 @@ -114,7 +108,6 @@ alternate UserDefAlternate case uda: UserDefA case s: str case i: int -enum UserDefAlternateKind ['uda', 's', 'i'] object UserDefB member intb: int optional=False member a-b: bool optional=True @@ -180,7 +173,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct alternate __org.qemu_x-Alt case __org.qemu_x-branch: str case b: __org.qemu_x-Base -enum __org.qemu_x-AltKind ['__org.qemu_x-branch', 'b'] object __org.qemu_x-Base member __org.qemu_x-member1: __org.qemu_x-Enum optional=False enum __org.qemu_x-Enum ['__org.qemu_x-value'] diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index d48ebdd62f..43b9e18912 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -312,13 +312,13 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, v = visitor_input_test_init(data, "42"); visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); - g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I); + g_assert_cmpint(tmp->type, ==, QTYPE_QINT); g_assert_cmpint(tmp->u.i, ==, 42); qapi_free_UserDefAlternate(tmp); v = visitor_input_test_init(data, "'string'"); visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); - g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S); + g_assert_cmpint(tmp->type, ==, QTYPE_QSTRING); g_assert_cmpstr(tmp->u.s, ==, "string"); qapi_free_UserDefAlternate(tmp); @@ -347,36 +347,37 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, error_free_or_abort(&err); qapi_free_AltStrBool(asb); - /* FIXME: Order of alternate should not affect semantics; asn should - * parse the same as ans */ + /* FIXME: integer should parse as number */ v = visitor_input_test_init(data, "42"); visit_type_AltStrNum(v, &asn, NULL, &err); - /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */ + /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */ /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */ error_free_or_abort(&err); qapi_free_AltStrNum(asn); + /* FIXME: integer should parse as number */ v = visitor_input_test_init(data, "42"); - visit_type_AltNumStr(v, &ans, NULL, &error_abort); - g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); - g_assert_cmpfloat(ans->u.n, ==, 42); + visit_type_AltNumStr(v, &ans, NULL, &err); + /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */ + /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */ + error_free_or_abort(&err); qapi_free_AltNumStr(ans); v = visitor_input_test_init(data, "42"); visit_type_AltStrInt(v, &asi, NULL, &error_abort); - g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I); + g_assert_cmpint(asi->type, ==, QTYPE_QINT); g_assert_cmpint(asi->u.i, ==, 42); qapi_free_AltStrInt(asi); v = visitor_input_test_init(data, "42"); visit_type_AltIntNum(v, &ain, NULL, &error_abort); - g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I); + g_assert_cmpint(ain->type, ==, QTYPE_QINT); g_assert_cmpint(ain->u.i, ==, 42); qapi_free_AltIntNum(ain); v = visitor_input_test_init(data, "42"); visit_type_AltNumInt(v, &ani, NULL, &error_abort); - g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I); + g_assert_cmpint(ani->type, ==, QTYPE_QINT); g_assert_cmpint(ani->u.i, ==, 42); qapi_free_AltNumInt(ani); @@ -389,13 +390,13 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, v = visitor_input_test_init(data, "42.5"); visit_type_AltStrNum(v, &asn, NULL, &error_abort); - g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N); + g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(asn->u.n, ==, 42.5); qapi_free_AltStrNum(asn); v = visitor_input_test_init(data, "42.5"); visit_type_AltNumStr(v, &ans, NULL, &error_abort); - g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); + g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(ans->u.n, ==, 42.5); qapi_free_AltNumStr(ans); @@ -406,13 +407,13 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, v = visitor_input_test_init(data, "42.5"); visit_type_AltIntNum(v, &ain, NULL, &error_abort); - g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N); + g_assert_cmpint(ain->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(ain->u.n, ==, 42.5); qapi_free_AltIntNum(ain); v = visitor_input_test_init(data, "42.5"); visit_type_AltNumInt(v, &ani, NULL, &error_abort); - g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N); + g_assert_cmpint(ani->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(ani->u.n, ==, 42.5); qapi_free_AltNumInt(ani); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 4196e66dde..30784424e4 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -428,7 +428,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, UserDefAlternate *tmp; tmp = g_new0(UserDefAlternate, 1); - tmp->type = USER_DEF_ALTERNATE_KIND_I; + tmp->type = QTYPE_QINT; tmp->u.i = 42; visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort); @@ -441,7 +441,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qobject_decref(arg); tmp = g_new0(UserDefAlternate, 1); - tmp->type = USER_DEF_ALTERNATE_KIND_S; + tmp->type = QTYPE_QSTRING; tmp->u.s = g_strdup("hello"); visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);