From patchwork Tue Feb 16 00:20:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 8319921 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id F0032C02AA for ; Tue, 16 Feb 2016 00:21:27 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E229D20375 for ; Tue, 16 Feb 2016 00:21:26 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A92192025A for ; Tue, 16 Feb 2016 00:21:25 +0000 (UTC) Received: from localhost ([::1]:37559 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTOO-0006YR-WC for patchwork-qemu-devel@patchwork.kernel.org; Mon, 15 Feb 2016 19:21:25 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTO8-0006RP-5y for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVTO6-0002PZ-2w for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38962) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTO5-0002P9-LC for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:05 -0500 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 54C79E1B87; Tue, 16 Feb 2016 00:21:05 +0000 (UTC) Received: from red.redhat.com (ovpn-113-167.phx2.redhat.com [10.3.113.167]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1G0L0sv011124; Mon, 15 Feb 2016 19:21:04 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 15 Feb 2016 17:20:54 -0700 Message-Id: <1455582057-27565-11-git-send-email-eblake@redhat.com> In-Reply-To: <1455582057-27565-1-git-send-email-eblake@redhat.com> References: <1455582057-27565-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] [PATCH v10 10/13] qapi: Don't box struct branch of alternate 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+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch; some testsuite coverage is added to avoid future regressions. Ultimately, we want to do the same treatment for QAPI unions, but as that touches a lot more client code, it is better as a separate patch. So in the meantime, I had to hack in a way to test if we are visiting an alternate type, within qapi-types:gen_variants(); the hack is possible because an earlier patch guaranteed that all alternates have at least two branches, with at most one object branch; the hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small, made possible by a new c_type(is_member) parameter in qapi.py: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(), comparable to visit_type_implicit_Foo(): |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, Error **errp) |+{ |+ Error *err = NULL; |+ |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ goto out; |+ } |+ visit_type_BlockdevOptions_fields(v, obj, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); |+out: |+ error_propagate(errp, err); |+} and use it like this: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); Signed-off-by: Eric Blake --- v10: new patch --- scripts/qapi-types.py | 10 ++++++- scripts/qapi-visit.py | 49 +++++++++++++++++++++++++++++---- scripts/qapi.py | 10 ++++--- tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++- tests/qapi-schema/qapi-schema-test.json | 2 ++ tests/qapi-schema/qapi-schema-test.out | 2 ++ 6 files changed, 91 insertions(+), 11 deletions(-) diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 2f23432..aba2847 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): + # HACK: Determine if this is an alternate (at least one variant + # is not an object); unions have all branches as objects. + inline = False + for v in variants.variants: + if not isinstance(v.type, QAPISchemaObjectType): + inline = True + break + # 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 # whether to bypass the switch statement if visiting the discriminator @@ -136,7 +144,7 @@ def gen_variants(variants): ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(), + c_type=typ.c_type(is_member=inline), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 9ff0337..948bde4 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -15,10 +15,14 @@ from qapi import * import re -# visit_type_FOO_implicit() is emitted as needed; track if it has already +# visit_type_implicit_FOO() is emitted as needed; track if it has already # been output. implicit_structs_seen = set() +# visit_type_alternate_FOO() is emitted as needed; track if it has already +# been output. +alternate_structs_seen = set() + # visit_type_FOO_fields() is always emitted; track if a forward declaration # or implementation has already been output. struct_fields_seen = set() @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * return ret +def gen_visit_alternate_struct(typ): + if typ in alternate_structs_seen: + return '' + alternate_structs_seen.add(typ) + + ret = gen_visit_fields_decl(typ) + + ret += mcgen(''' + +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, %(c_type)s *obj, Error **errp) +{ + Error *err = NULL; + + visit_start_struct(v, name, NULL, 0, &err); + if (err) { + goto out; + } + visit_type_%(c_type)s_fields(v, obj, &err); + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); +out: + error_propagate(errp, err); +} +''', + c_type=typ.c_name()) + return ret + + def gen_visit_struct_fields(name, base, members): ret = '' @@ -158,11 +191,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error def gen_visit_alternate(name, variants): promote_int = 'true' + ret = '' for var in variants.variants: if var.type.alternate_qtype() == 'QTYPE_QINT': promote_int = 'false' + if isinstance(var.type, QAPISchemaObjectType): + ret += gen_visit_alternate_struct(var.type) - ret = mcgen(''' + ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { @@ -178,15 +214,18 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } switch ((*obj)->type) { ''', - c_name=c_name(name), promote_int=promote_int) + c_name=c_name(name), promote_int=promote_int) for var in variants.variants: + prefix = 'visit_type_' + if isinstance(var.type, QAPISchemaObjectType): + prefix += 'alternate_' ret += mcgen(''' case %(case)s: - visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); + %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); break; ''', - case=var.type.alternate_qtype(), + prefix=prefix, case=var.type.alternate_qtype(), c_type=var.type.c_name(), c_name=c_name(var.name)) diff --git a/scripts/qapi.py b/scripts/qapi.py index 4d75d75..dbb58da 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): return c_name(self.name) + pointer_suffix def c_null(self): @@ -854,7 +854,7 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): if is_param and self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -888,7 +888,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): return c_name(self.name) def member_names(self): @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): assert not self.is_implicit() + if is_member: + return c_name(self.name) return QAPISchemaType.c_type(self) def json_type(self): diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index c72cdad..139ff7d 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -1,7 +1,7 @@ /* * QMP Input Visitor unit-tests. * - * Copyright (C) 2011, 2015 Red Hat Inc. + * Copyright (C) 2011-2016 Red Hat Inc. * * Authors: * Luiz Capitulino @@ -309,6 +309,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, Visitor *v; Error *err = NULL; UserDefAlternate *tmp; + WrapAlternate *wrap; v = visitor_input_test_init(data, "42"); visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); @@ -322,10 +323,36 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpstr(tmp->u.s, ==, "string"); qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "{'boolean':true}"); + visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); + g_assert_cmpint(tmp->type, ==, QTYPE_QDICT); + g_assert_cmpint(tmp->u.uda.boolean, ==, true); + g_assert_cmpint(tmp->u.uda.has_a_b, ==, false); + qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "false"); visit_type_UserDefAlternate(v, NULL, &tmp, &err); error_free_or_abort(&err); qapi_free_UserDefAlternate(tmp); + + v = visitor_input_test_init(data, "{ 'alt': 42 }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QINT); + g_assert_cmpint(wrap->alt->u.i, ==, 42); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': 'string' }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING); + g_assert_cmpstr(wrap->alt->u.s, ==, "string"); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': {'boolean':true} }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT); + g_assert_cmpint(wrap->alt->u.uda.boolean, ==, true); + g_assert_cmpint(wrap->alt->u.uda.has_a_b, ==, false); + qapi_free_WrapAlternate(wrap); } static void test_visitor_in_alternate_number(TestInputVisitorData *data, diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 4b89527..b393572 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -82,6 +82,8 @@ 'value2' : 'UserDefB', 'value3' : 'UserDefA' } } +{ 'struct': 'WrapAlternate', + 'data': { 'alt': 'UserDefAlternate' } } { 'alternate': 'UserDefAlternate', 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 2c546b7..11cdc9a 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -169,6 +169,8 @@ object UserDefUnionBase member enum1: EnumOne optional=False object UserDefZero member integer: int optional=False +object WrapAlternate + member alt: UserDefAlternate optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct alternate __org.qemu_x-Alt case __org.qemu_x-branch: str