From patchwork Fri Aug 7 03:52:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 504994 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B205014029E for ; Fri, 7 Aug 2015 14:00:38 +1000 (AEST) Received: from localhost ([::1]:47985 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNYpg-0000iC-P4 for incoming@patchwork.ozlabs.org; Fri, 07 Aug 2015 00:00:36 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46359) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNYiG-0005Ay-QP for qemu-devel@nongnu.org; Thu, 06 Aug 2015 23:52:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNYiE-0002Zs-8w for qemu-devel@nongnu.org; Thu, 06 Aug 2015 23:52:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40580) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNYiD-0002ZX-W2 for qemu-devel@nongnu.org; Thu, 06 Aug 2015 23:52:54 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 9056891D05; Fri, 7 Aug 2015 03:52:53 +0000 (UTC) Received: from red.redhat.com (ovpn-113-187.phx2.redhat.com [10.3.113.187]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t773qlec007760; Thu, 6 Aug 2015 23:52:52 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Thu, 6 Aug 2015 21:52:35 -0600 Message-Id: <1438919561-27750-7-git-send-email-eblake@redhat.com> In-Reply-To: <1438919561-27750-1-git-send-email-eblake@redhat.com> References: <1438919561-27750-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [RFC PATCH v2 06/12] qapi: Simplify visiting of alternate types X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Previously, working with alternates required two enums, and some indirection: for type Foo, we created Foo_qtypes[] which maps each qtype to a member of FooKind_lookup[], then use FooKind_lookup[] like we do for other union types. This has a subtle bug: since the values of FooKind_lookup start at zero, all entries of Foo_qtypes that were not explicitly initialized map to the same branch of the union as the first member of the alternate, rather than triggering a 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 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, it IS observable in one case: the behavior of an alternate that contains a 'number' member but no 'int' member differs according to whether the 'number' was first in the qapi definition, and when the input being parsed is an integer; this is because the 'number' parser accepts QTYPE_QINT in addition to the expected QTYPE_QFLOAT. A later patch will worry about fixing alternates to parse all inputs that a non-alternate 'number' would accept. This patch fixes the validation bug by deleting the indirection, and modifying get_next_type() to directly return a qtype code. There is no longer a need to generate an implicit FooKind array associated with the alternate type (since the QMP wire format never uses the stringized counterparts of the C union member names). Next, the generated visitor is fixed to properly detect unexpected qtypes in the switch statement. Things got a bit tricky with validating QAPISchemaObjectTypeVariants, which now has three different initialization paths; but I didn't think it was confusing enough to need to create different sub-classes. 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. If that gets too confusing, we could reintroduce FooKind, but initialize it 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->kind. But without a current client, I didn't see the point of doing it now. 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"}} Now it fails with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}} Signed-off-by: Eric Blake --- docs/qapi-code-gen.txt | 3 --- include/qapi/visitor-impl.h | 2 +- include/qapi/visitor.h | 2 +- qapi/qapi-visit-core.c | 4 ++-- qapi/qmp-input-visitor.c | 4 ++-- scripts/qapi-types.py | 39 ++++++---------------------------- scripts/qapi-visit.py | 12 ++++++----- scripts/qapi.py | 21 +++++++++++------- tests/qapi-schema/alternate-good.out | 1 - tests/qapi-schema/qapi-schema-test.out | 8 ------- tests/test-qmp-input-visitor.c | 26 +++++++++++------------ tests/test-qmp-output-visitor.c | 21 +++++++++++++----- 12 files changed, 61 insertions(+), 82 deletions(-) diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 3a78cf4..5dd08e5 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -372,9 +372,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 8c0ba57..7098b93 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -32,7 +32,7 @@ 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, + void (*get_next_type)(Visitor *v, qtype_code *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 cfc19a6..f1ac5c4 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -41,7 +41,7 @@ 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, +void visit_get_next_type(Visitor *v, qtype_code *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 59ed506..3f24daa 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_code *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 5dd9ed5..803ffad 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_code *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 27ad0c1..b0785a5 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -88,36 +88,6 @@ struct %(c_name)s { return ret -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_union(name, base, variants): ret = mcgen(''' @@ -126,6 +96,10 @@ struct %(c_name)s { c_name=c_name(name)) if base: ret += gen_struct_fields([], base) + elif not variants.tag_member: + ret += mcgen(''' + qtype_code type; +''') else: ret += mcgen(''' %(c_type)s type; @@ -144,7 +118,8 @@ struct %(c_name)s { union { /* union tag is @%(c_name)s */ void *data; ''', - c_name=c_name(variants.tag_member.name)) + c_name=c_name((variants.tag_member and + variants.tag_member.name) or 'type')) for var in variants.variants: # TODO ugly special case for simple union @@ -244,9 +219,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self._gen_type_cleanup(name) 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_union(name, None, 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 b131c0d..06a27c5 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -203,7 +203,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error if (err) { goto out; } - visit_get_next_type(m, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err); + visit_get_next_type(m, &(*obj)->type, name, &err); if (err) { goto out_end; } @@ -217,14 +217,14 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error visit_type_%(c_type)s(m, &(*obj)->%(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_end: error_propagate(errp, err); @@ -233,7 +233,8 @@ out_end: out: error_propagate(errp, err); } -''') +''', + name=name) return ret @@ -427,6 +428,7 @@ fdef.write(mcgen(''' 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 99ea61d..6f41ae0 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -929,29 +929,35 @@ class QAPISchemaObjectTypeVariants(object): for v in variants: assert isinstance(v, QAPISchemaObjectTypeVariant) self.tag_name = tag_name - if tag_name: + if tag_name: # flat union assert not tag_enum self.tag_member = None - else: + elif tag_enum: # simple union self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum, False) + else: # alternate + self.tag_member = None self.variants = variants def check(self, schema, members, seen): if self.tag_name: self.tag_member = seen[self.tag_name] - else: + elif self.tag_member: self.tag_member.check(schema, members, seen) - assert isinstance(self.tag_member.type, QAPISchemaEnumType) + typ = None + if self.tag_member: + assert isinstance(self.tag_member.type, QAPISchemaEnumType) + typ = self.tag_member.type for v in self.variants: vseen = dict(seen) - v.check(schema, self.tag_member.type, vseen) + v.check(schema, typ, vseen) class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) def check(self, schema, tag_type, seen): QAPISchemaObjectTypeMember.check(self, schema, [], seen) - assert self.name in tag_type.values + if tag_type: + assert self.name in tag_type.values # TODO try to get rid of .simple_union_type() def simple_union_type(self): if isinstance(self.type, QAPISchemaObjectType) and not self.type.info: @@ -1146,10 +1152,9 @@ class QAPISchema(object): data = expr['data'] variants = [self._make_variant(key, data[key]) for key in data.keys()] - tag_enum = self._make_tag_enum(name, variants) self._def_entity(QAPISchemaAlternateType(name, info, QAPISchemaObjectTypeVariants(None, - tag_enum, + None, variants))) self._make_array_type(name) # TODO really needed? diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out index 65af727..d211aba 100644 --- a/tests/qapi-schema/alternate-good.out +++ b/tests/qapi-schema/alternate-good.out @@ -3,7 +3,6 @@ alternate Alt case value: int case string: Enum case struct: Data -enum AltKind ['value', 'string', 'struct'] object Data member number: int optional=True member name: str optional=True diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index ba70ea7..f062a15 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -54,27 +54,21 @@ object :obj-user_def_cmd3-arg alternate AltFive case i: int case n: number -enum AltFiveKind ['i', 'n'] alternate AltFour case s: str case i: int -enum AltFourKind ['s', 'i'] alternate AltOne case s: str case b: bool -enum AltOneKind ['s', 'b'] alternate AltSix case n: number case i: int -enum AltSixKind ['n', 'i'] alternate AltThree case n: number case s: str -enum AltThreeKind ['n', 's'] alternate AltTwo case s: str case n: number -enum AltTwoKind ['s', 'n'] event EVENT_A None event EVENT_B None event EVENT_C :obj-EVENT_C-arg @@ -95,7 +89,6 @@ alternate UserDefAlternate case uda: UserDefA case s: str case i: int -enum UserDefAlternateKind ['uda', 's', 'i'] object UserDefB member intb: int optional=False object UserDefC @@ -158,7 +151,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 3ee1a1a..cd833a4 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -373,7 +373,7 @@ 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->i, ==, 42); qapi_free_UserDefAlternate(tmp); tmp = NULL; @@ -381,7 +381,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, 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->s, ==, "string"); qapi_free_UserDefAlternate(tmp); tmp = NULL; @@ -424,31 +424,31 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, qapi_free_AltTwo(two); one = NULL; - /* FIXME: Order of alternate should not affect semantics */ v = visitor_input_test_init(data, "42"); - visit_type_AltThree(v, &three, NULL, &error_abort); - g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N); - g_assert_cmpfloat(three->n, ==, 42); + visit_type_AltThree(v, &three, NULL, &err); + g_assert(err); + error_free(err); + err = NULL; qapi_free_AltThree(three); one = NULL; v = visitor_input_test_init(data, "42"); visit_type_AltFour(v, &four, NULL, &error_abort); - g_assert_cmpint(four->type, ==, ALT_FOUR_KIND_I); + g_assert_cmpint(four->type, ==, QTYPE_QINT); g_assert_cmpint(four->i, ==, 42); qapi_free_AltFour(four); one = NULL; v = visitor_input_test_init(data, "42"); visit_type_AltFive(v, &five, NULL, &error_abort); - g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_I); + g_assert_cmpint(five->type, ==, QTYPE_QINT); g_assert_cmpint(five->i, ==, 42); qapi_free_AltFive(five); one = NULL; v = visitor_input_test_init(data, "42"); visit_type_AltSix(v, &six, NULL, &error_abort); - g_assert_cmpint(six->type, ==, ALT_SIX_KIND_I); + g_assert_cmpint(six->type, ==, QTYPE_QINT); g_assert_cmpint(six->i, ==, 42); qapi_free_AltSix(six); one = NULL; @@ -465,14 +465,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, v = visitor_input_test_init(data, "42.5"); visit_type_AltTwo(v, &two, NULL, &error_abort); - g_assert_cmpint(two->type, ==, ALT_TWO_KIND_N); + g_assert_cmpint(two->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(two->n, ==, 42.5); qapi_free_AltTwo(two); two = NULL; v = visitor_input_test_init(data, "42.5"); visit_type_AltThree(v, &three, NULL, &error_abort); - g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N); + g_assert_cmpint(three->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(three->n, ==, 42.5); qapi_free_AltThree(three); three = NULL; @@ -487,14 +487,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, v = visitor_input_test_init(data, "42.5"); visit_type_AltFive(v, &five, NULL, &error_abort); - g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_N); + g_assert_cmpint(five->type, ==, QTYPE_QFLOAT); g_assert_cmpfloat(five->n, ==, 42.5); qapi_free_AltFive(five); five = NULL; v = visitor_input_test_init(data, "42.5"); visit_type_AltSix(v, &six, NULL, &error_abort); - g_assert_cmpint(six->type, ==, ALT_SIX_KIND_N); + g_assert_cmpint(six->type, ==, QTYPE_QFLOAT); g_assert_cmpint(six->n, ==, 42.5); qapi_free_AltSix(six); six = NULL; diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index ab4bc5d..2b5c8d5 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -510,20 +510,31 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, const void *unused) { QObject *arg; - Error *err = NULL; + UserDefAlternate *tmp; - UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate)); - tmp->type = USER_DEF_ALTERNATE_KIND_I; + tmp = g_malloc0(sizeof(UserDefAlternate)); + tmp->type = QTYPE_QINT; tmp->i = 42; - visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err); - g_assert(err == NULL); + visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort); arg = qmp_output_get_qobject(data->qov); g_assert(qobject_type(arg) == QTYPE_QINT); g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42); qapi_free_UserDefAlternate(tmp); + + tmp = g_malloc0(sizeof(UserDefAlternate)); + tmp->type = QTYPE_QSTRING; + tmp->s = g_strdup("hello"); + + visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort); + arg = qmp_output_get_qobject(data->qov); + + g_assert(qobject_type(arg) == QTYPE_QSTRING); + g_assert_cmpstr(qstring_get_str(qobject_to_qstring(arg)), ==, "hello"); + + qapi_free_UserDefAlternate(tmp); } static void test_visitor_out_empty(TestOutputVisitorData *data,