From patchwork Mon Sep 7 10:16:22 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 515058 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 693A8140281 for ; Mon, 7 Sep 2015 20:20:34 +1000 (AEST) Received: from localhost ([::1]:55472 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYtXM-00047d-AQ for incoming@patchwork.ozlabs.org; Mon, 07 Sep 2015 06:20:32 -0400 Received: from eggs.gnu.org ([208.118.235.92]:34232) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYtTq-0001WG-IK for qemu-devel@nongnu.org; Mon, 07 Sep 2015 06:16:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZYtTo-0000Ph-0j for qemu-devel@nongnu.org; Mon, 07 Sep 2015 06:16:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53026) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYtTn-0000PD-Nn for qemu-devel@nongnu.org; Mon, 07 Sep 2015 06:16:51 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 506A1C0B9862; Mon, 7 Sep 2015 10:16:51 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t87AGlTm031713 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 7 Sep 2015 06:16:49 -0400 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 90387303E3F3; Mon, 7 Sep 2015 12:16:44 +0200 (CEST) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Mon, 7 Sep 2015 12:16:22 +0200 Message-Id: <1441621003-2434-12-git-send-email-armbru@redhat.com> In-Reply-To: <1441621003-2434-1-git-send-email-armbru@redhat.com> References: <1441621003-2434-1-git-send-email-armbru@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: mdroth@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH RFC v5 11/32] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs 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 Fixes flat unions to visit the base's base members (the previous commit merely added them to the struct). Same test case. Patch's effect on visit_type_UserDefFlatUnion(): static void visit_type_UserDefFlatUnion_fields(Visitor *m, UserDefFlatUnion **obj, Error **errp) { Error *err = NULL; + visit_type_int(m, &(*obj)->integer, "integer", &err); + if (err) { + goto out; + } visit_type_str(m, &(*obj)->string, "string", &err); if (err) { goto out; Test cases updated for the bug fix. Fixes alternates to generate a visitor for their implicit enumeration type. None of them are currently used, obviously. Example: block-core.json's BlockdevRef now generates visit_type_BlockdevRefKind(). The previous commit's two ugly special cases exist here, too. Mark both TODO. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- scripts/qapi-visit.py | 270 +++++++++++++------------------- tests/qapi-schema/qapi-schema-test.json | 3 - tests/test-qmp-input-strict.c | 4 +- tests/test-qmp-input-visitor.c | 4 +- 4 files changed, 117 insertions(+), 164 deletions(-) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index c493964..cfebcb7 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -12,7 +12,6 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. -from ordereddict import OrderedDict from qapi import * import re @@ -24,13 +23,13 @@ def generate_visit_implicit_struct(type): return '' implicit_structs_seen.add(type) ret = '' - if type not in struct_fields_seen: + if type.name not in struct_fields_seen: # Need a forward declaration ret += mcgen(''' static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp); ''', - c_type=type_name(type)) + c_type=type.c_name()) ret += mcgen(''' @@ -46,7 +45,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error * error_propagate(errp, err); } ''', - c_type=type_name(type)) + c_type=type.c_name()) return ret def generate_visit_struct_fields(name, members, base = None): @@ -74,24 +73,24 @@ if (err) { goto out; } ''', - type=type_name(base), c_name=c_name('base')) + type=base.c_name(), c_name=c_name('base')) - for argname, argentry, optional in parse_args(members): - if optional: + for memb in members: + if memb.optional: ret += mcgen(''' visit_optional(m, &(*obj)->has_%(c_name)s, "%(name)s", &err); if (!err && (*obj)->has_%(c_name)s) { ''', - c_name=c_name(argname), name=argname) + c_name=c_name(memb.name), name=memb.name) push_indent() ret += mcgen(''' visit_type_%(type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err); ''', - type=type_name(argentry), c_name=c_name(argname), - name=argname) + type=memb.type.c_name(), c_name=c_name(memb.name), + name=memb.name) - if optional: + if memb.optional: pop_indent() ret += mcgen(''' } @@ -136,12 +135,7 @@ def generate_visit_struct_body(name): return ret -def generate_visit_struct(expr): - - name = expr['struct'] - members = expr['data'] - base = expr.get('base') - +def gen_visit_struct(name, base, members): ret = generate_visit_struct_fields(name, members, base) ret += mcgen(''' @@ -158,10 +152,10 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e ''') return ret -def generate_visit_list(name): +def gen_visit_list(name, element_type): return mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp) +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) { Error *err = NULL; GenericList *i, **prev; @@ -174,8 +168,8 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, E for (prev = (GenericList **)obj; !err && (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) { - %(name)sList *native_i = (%(name)sList *)i; - visit_type_%(name)s(m, &native_i->value, NULL, &err); + %(name)s *native_i = (%(name)s *)i; + visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err); } error_propagate(errp, err); @@ -185,7 +179,8 @@ out: error_propagate(errp, err); } ''', - name=type_name(name)) + name=c_name(name), + c_elt_type=element_type.c_name()) def generate_visit_enum(name): return mcgen(''' @@ -197,7 +192,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error ''', c_name=c_name(name), name=name) -def generate_visit_alternate(name, members): +def gen_visit_alternate(name, variants): ret = mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) @@ -216,25 +211,17 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e ''', name=c_name(name)) - # For alternate, always use the default enum type automatically generated - # as name + 'Kind' - disc_type = c_name(name) + 'Kind' - - for key in members: - assert (members[key] in builtin_types.keys() - or find_struct(members[key]) - or find_union(members[key]) - or find_enum(members[key])), "Invalid alternate member" - - enum_full_value = c_enum_const(disc_type, key) + for var in variants.variants: + enum_full_value = c_enum_const(variants.tag_member.type.name, + var.name) ret += mcgen(''' case %(enum_full_value)s: visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err); break; ''', enum_full_value = enum_full_value, - c_type = type_name(members[key]), - c_name = c_name(key)) + c_type=var.type.c_name(), + c_name=c_name(var.name)) ret += mcgen(''' default: @@ -251,35 +238,17 @@ out: return ret - -def generate_visit_union(expr): - - name = expr['union'] - members = expr['data'] - - base = expr.get('base') - discriminator = expr.get('discriminator') - - enum_define = discriminator_find_enum_define(expr) - if enum_define: - # Use the enum type as discriminator - ret = "" - disc_type = c_name(enum_define['enum_name']) - else: - # There will always be a discriminator in the C switch code, by default - # it is an enum type generated silently - ret = generate_visit_enum(name + 'Kind') - disc_type = c_name(name) + 'Kind' +def gen_visit_union(name, base, variants): + ret = '' if base: - assert discriminator - base_fields = find_struct(base)['data'].copy() - del base_fields[discriminator] - ret += generate_visit_struct_fields(name, base_fields) + members = [m for m in base.members if m != variants.tag_member] + ret += generate_visit_struct_fields(name, members) - if discriminator: - for key in members: - ret += generate_visit_implicit_struct(members[key]) + for var in variants.variants: + # Ugly special case for simple union TODO get rid of it + if not var.simple_union_type(): + ret += generate_visit_implicit_struct(var.type) ret += mcgen(''' @@ -304,41 +273,44 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error ''', name=c_name(name)) - if not discriminator: - tag = 'kind' - disc_key = "type" - else: - tag = discriminator - disc_key = discriminator + disc_key = variants.tag_member.name + if not variants.tag_name: + # we pointlessly use a different key for simple unions + disc_key = 'type' ret += mcgen(''' - visit_type_%(disc_type)s(m, &(*obj)->%(c_tag)s, "%(disc_key)s", &err); + visit_type_%(disc_type)s(m, &(*obj)->%(c_name)s, "%(disc_key)s", &err); if (err) { goto out_obj; } if (!visit_start_union(m, !!(*obj)->data, &err) || err) { goto out_obj; } - switch ((*obj)->%(c_tag)s) { + switch ((*obj)->%(c_name)s) { ''', - disc_type = disc_type, - c_tag=c_name(tag), + disc_type=variants.tag_member.type.c_name(), + # TODO ugly special case for simple union + # Use same tag name in C as on the wire to get rid of + # it, then: c_name=c_name(variants.tag_member.name) + c_name=c_name(variants.tag_name or 'kind'), disc_key = disc_key) - for key in members: - if not discriminator: + for var in variants.variants: + # TODO ugly special case for simple union + simple_union_type = var.simple_union_type() + if simple_union_type: fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);' else: fmt = 'visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);' - enum_full_value = c_enum_const(disc_type, key) + enum_full_value = c_enum_const(variants.tag_member.type.name, var.name) ret += mcgen(''' case %(enum_full_value)s: ''' + fmt + ''' break; ''', enum_full_value = enum_full_value, - c_type=type_name(members[key]), - c_name=c_name(key)) + c_type=(simple_union_type or var.type).c_name(), + c_name=c_name(var.name)) ret += mcgen(''' default: @@ -359,38 +331,68 @@ out: return ret -def generate_declaration(name, builtin_type=False): - ret = "" - if not builtin_type: - name = c_name(name) - ret += mcgen(''' - -void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); -''', - name=name) - - ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', - name=name) - - return ret - -def generate_enum_declaration(name): - ret = mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', - name=c_name(name)) - - return ret - -def generate_decl_enum(name): +def gen_visit_decl(name, scalar=False): + c_type = c_name(name) + ' *' + if not scalar: + c_type += '*' return mcgen(''' - -void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); +void visit_type_%(c_name)s(Visitor *m, %(c_type)sobj, const char *name, Error **errp); ''', - name=c_name(name)) + c_name=c_name(name), c_type=c_type) + +class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): + def __init__(self): + self.decl = None + self.defn = None + self._btin = None + + def visit_begin(self, schema): + self.decl = '' + self.defn = '' + self._btin = guardstart('QAPI_VISIT_BUILTIN') + + def visit_end(self): + # To avoid header dependency hell, we always generate + # declarations for built-in types in our header files and + # simply guard them. See also do_builtins (command line + # option -b). + self._btin += guardend('QAPI_VISIT_BUILTIN') + self.decl = self._btin + self.decl + self._btin = None + + def visit_enum_type(self, name, info, values): + self.decl += gen_visit_decl(name, scalar=True) + self.defn += generate_visit_enum(name) + + def visit_array_type(self, name, info, element_type): + decl = gen_visit_decl(name) + defn = gen_visit_list(name, element_type) + if isinstance(element_type, QAPISchemaBuiltinType): + self._btin += decl + if do_builtins: + self.defn += defn + else: + self.decl += decl + self.defn += defn + + def visit_object_type(self, name, info, base, members, variants): + if info: + self.decl += gen_visit_decl(name) + if variants: + assert not members # not implemented + self.defn += gen_visit_union(name, base, variants) + else: + self.defn += gen_visit_struct(name, base, members) + + def visit_alternate_type(self, name, info, variants): + self.decl += gen_visit_decl(name) + self.defn += gen_visit_alternate(name, variants) + +# If you link code generated from multiple schemata, you want only one +# instance of the code for built-in types. Generate it only when +# do_builtins, enabled by command line option -b. See also +# QAPISchemaGenVisitVisitor.visit_end(). do_builtins = False (input_file, output_dir, do_c, do_h, prefix, opts) = \ @@ -446,56 +448,10 @@ fdecl.write(mcgen(''' ''', prefix=prefix)) -exprs = QAPISchema(input_file).get_exprs() - -# to avoid header dependency hell, we always generate declarations -# for built-in types in our header files and simply guard them -fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) -for typename in builtin_types.keys(): - fdecl.write(generate_declaration(typename, builtin_type=True)) -fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) - -# ...this doesn't work for cases where we link in multiple objects that -# have the functions defined, so we use -b option to provide control -# over these cases -if do_builtins: - for typename in builtin_types.keys(): - fdef.write(generate_visit_list(typename)) - -for expr in exprs: - if expr.has_key('struct'): - ret = generate_visit_struct(expr) - ret += generate_visit_list(expr['struct']) - fdef.write(ret) - - ret = generate_declaration(expr['struct']) - fdecl.write(ret) - elif expr.has_key('union'): - ret = generate_visit_union(expr) - ret += generate_visit_list(expr['union']) - fdef.write(ret) - - enum_define = discriminator_find_enum_define(expr) - ret = "" - if not enum_define: - ret = generate_decl_enum('%sKind' % expr['union']) - ret += generate_declaration(expr['union']) - fdecl.write(ret) - elif expr.has_key('alternate'): - ret = generate_visit_alternate(expr['alternate'], expr['data']) - ret += generate_visit_list(expr['alternate']) - fdef.write(ret) - - ret = generate_decl_enum('%sKind' % expr['alternate']) - ret += generate_declaration(expr['alternate']) - fdecl.write(ret) - elif expr.has_key('enum'): - ret = generate_visit_list(expr['enum']) - ret += generate_visit_enum(expr['enum']) - fdef.write(ret) - - ret = generate_decl_enum(expr['enum']) - ret += generate_enum_declaration(expr['enum']) - fdecl.write(ret) +schema = QAPISchema(input_file) +gen = QAPISchemaGenVisitVisitor() +schema.visit(gen) +fdef.write(gen.defn) +fdecl.write(gen.decl) close_output(fdef, fdecl) diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 257b4d4..b182174 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -39,8 +39,6 @@ 'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } } -# FIXME generated visit_type_UserDefFlatUnion_fields() fails to visit -# members of indirect base UserDefZero { 'struct': 'UserDefUnionBase', 'base': 'UserDefZero', @@ -57,7 +55,6 @@ { 'alternate': 'UserDefAlternate', 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } -# FIXME only a declaration of visit_type_UserDefAlternateKind() generated { 'struct': 'UserDefC', 'data': { 'string1': 'str', 'string2': 'str' } } diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index 68f855b..a2ae786 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -167,9 +167,9 @@ static void test_validate_union_flat(TestInputVisitorData *data, v = validate_test_init(data, "{ 'enum1': 'value1', " + "'integer': 41, " "'string': 'str', " "'boolean': true }"); - /* TODO when generator bug is fixed, add 'integer': 41 */ visit_type_UserDefFlatUnion(v, &tmp, NULL, &err); g_assert(!err); @@ -272,7 +272,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, Visitor *v; /* test situation where discriminator field ('enum1' here) is missing */ - v = validate_test_init(data, "{ 'string': 'c', 'string1': 'd', 'string2': 'e' }"); + v = validate_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }"); visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err); g_assert(err); diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index a5cfefa..508c11a 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -307,15 +307,15 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data, v = visitor_input_test_init(data, "{ 'enum1': 'value1', " + "'integer': 41, " "'string': 'str', " "'boolean': true }"); - /* TODO when generator bug is fixed, add 'integer': 41 */ visit_type_UserDefFlatUnion(v, &tmp, NULL, &err); g_assert(err == NULL); g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1); g_assert_cmpstr(tmp->string, ==, "str"); - /* TODO g_assert_cmpint(tmp->integer, ==, 41); */ + g_assert_cmpint(tmp->integer, ==, 41); g_assert_cmpint(tmp->value1->boolean, ==, true); qapi_free_UserDefFlatUnion(tmp); }