From patchwork Mon Jan 25 15:03:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 8109311 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 E1B2EBEEE5 for ; Mon, 25 Jan 2016 15:03:32 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 43736202BE for ; Mon, 25 Jan 2016 15:03:28 +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 A6F2F2024D for ; Mon, 25 Jan 2016 15:03:22 +0000 (UTC) Received: from localhost ([::1]:39129 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNifq-0001Af-4Q for patchwork-qemu-devel@patchwork.kernel.org; Mon, 25 Jan 2016 10:03:22 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNiff-0001AG-Ar for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:03:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNifa-0002fY-VW for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:03:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55101) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNifa-0002fS-M7 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:03:06 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 2760D192450; Mon, 25 Jan 2016 15:03:06 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-113-78.phx2.redhat.com [10.3.113.78]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0PF34V4031232 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 25 Jan 2016 10:03:05 -0500 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 8C74A303F81B; Mon, 25 Jan 2016 16:03:03 +0100 (CET) From: Markus Armbruster To: Eric Blake References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-29-git-send-email-eblake@redhat.com> Date: Mon, 25 Jan 2016 16:03:03 +0100 In-Reply-To: <1453219845-30939-29-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:36 -0700") Message-ID: <87mvrt4s7s.fsf@blackfin.pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth Subject: Re: [Qemu-devel] [PATCH v9 28/37] qapi: Fix command with named empty argument type 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 Eric Blake writes: > The generator special-cased > { 'command':'foo', 'data': {} } > to avoid emitting a visitor variable, but failed to see that > { 'struct':'NamedEmptyType, 'data': {} } > { 'command':'foo', 'data':'NamedEmptyType' } > needs the same treatment. Without a fix to the generator, the > change to qapi-schema-test.json fails to compile with: > > tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’: > tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable] > Visitor *v; > ^ > > Signed-off-by: Eric Blake > Reviewed-by: Marc-André Lureau > > --- > v9: no change > v8: no change > v7: no change > v6: new patch > --- > scripts/qapi-commands.py | 6 +++--- > tests/qapi-schema/qapi-schema-test.json | 2 ++ > tests/qapi-schema/qapi-schema-test.out | 2 ++ > tests/test-qmp-commands.c | 5 +++++ > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 91c5a4e..00ee565 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type): > ''', > c_type=ret_type.c_type()) > > - if arg_type: > + if arg_type and not arg_type.is_empty(): This guards generating the variables for getting arguments, first a few common ones, then one or two per argument. The patch changes it to effectively if arg_type and (arg_type.members or arg_type.variants) .variants is None (asserted in QAPISchemaCommand.check()). Thus, the patch effectively just adds and arg_type.members. Impact: when arg_type has no members, we still generate the common variables before the patch. The patch suppresses them. Whether that makes sense depends on the next hunk, which generates the code using these variables. > ret += mcgen(''' > QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); > QapiDeallocVisitor *qdv; > @@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type): > def gen_marshal_input_visit(arg_type, dealloc=False): > ret = '' > > - if not arg_type: > + if not arg_type or arg_type.is_empty(): > return ret This guards generating the code for getting arguments: common code, and per-argument code. Impact: when arg_type has no members, we still generate the common code before the patch. The patch suppresses it. Is that okay? We'll see below. > > if dealloc: > @@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type): > > # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields() > # for each arg_type member, and by gen_call() for ret_type > - if (arg_type and arg_type.members) or ret_type: > + if (arg_type and not arg_type.is_empty()) or ret_type: This guards generating label out. or's left operand is about the arguments. Unlike the guards above, it checks .members even before the patch. > ret += mcgen(''' > > out: > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index 4b89527..a0fdb88 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -18,6 +18,8 @@ > { 'struct': 'Empty1', 'data': { } } > { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } } > > +{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' } > + > # for testing override of default naming heuristic > { 'enum': 'QEnumTwo', > 'prefix': 'QENUM_TWO', > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 2c546b7..d8f9108 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -198,6 +198,8 @@ command guest-sync :obj-guest-sync-arg -> any > gen=True success_response=True > command user_def_cmd None -> None > gen=True success_response=True > +command user_def_cmd0 Empty2 -> Empty2 > + gen=True success_response=True > command user_def_cmd1 :obj-user_def_cmd1-arg -> None > gen=True success_response=True > command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > index 4d267b6..bc8085d 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -12,6 +12,11 @@ void qmp_user_def_cmd(Error **errp) > { > } > > +Empty2 *qmp_user_def_cmd0(Error **errp) > +{ > + return g_new0(Empty2, 1); > +} > + > void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp) > { > } Patch's effect on the test case: We drop a pair of "create a visitor, destroy it without having done anything with it". Okay. Commit message might mislead readers into assuming the patch merely drops unused variables. It actually drops creating useless visitors. Easy enough to clarify: The generator special-cased { 'command':'foo', 'data': {} } to avoid emitting a visitor variable, but failed to see that { 'struct':'NamedEmptyType, 'data': {} } { 'command':'foo', 'data':'NamedEmptyType' } needs the same treatment. There, the generator happily generates a visitor to get no arguments, and a visitor to destroy no arguments. The compiler isn't happy with that, as demonstrated by the updated qapi-schema-test.json: tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’: tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable] Visitor *v; ^ --- bld-x86/tests/test-qmp-marshal.c 2016-01-25 15:54:34.106265263 +0100 +++ bld-x86/tests/new-test-qmp-marshal.c 2016-01-25 15:54:08.312574713 +0100 @@ -254,11 +254,8 @@ { Error *err = NULL; Empty2 *retval; - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); - QapiDeallocVisitor *qdv; - Visitor *v; - v = qmp_input_get_visitor(qiv); + (void)args; retval = qmp_user_def_cmd0(&err); if (err) { @@ -269,10 +266,6 @@ out: error_propagate(errp, err); - qmp_input_visitor_cleanup(qiv); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); - qapi_dealloc_visitor_cleanup(qdv); } static void qmp_marshal_user_def_cmd1(QDict *args, QObject **ret, Error **errp)