From mboxrd@z Thu Jan 1 00:00:00 1970 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 From: Markus Armbruster 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 28/37] qapi: Fix command with named empty argument type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth 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 =E2=80=98qmp_marshal_user_def_cmd0= =E2=80=99: > tests/test-qmp-marshal.c:264:14: error: variable =E2=80=98v=E2=80=99 set = but not used [-Werror=3Dunused-but-set-variable] > Visitor *v; > ^ > > Signed-off-by: Eric Blake > Reviewed-by: Marc-Andr=C3=A9 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=3Dret_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 +=3D mcgen(''' > QmpInputVisitor *qiv =3D 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=3DFalse): > ret =3D '' > > - 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 +=3D 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/q= api-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=3DTrue success_response=3DTrue > command user_def_cmd None -> None > gen=3DTrue success_response=3DTrue > +command user_def_cmd0 Empty2 -> Empty2 > + gen=3DTrue success_response=3DTrue > command user_def_cmd1 :obj-user_def_cmd1-arg -> None > gen=3DTrue success_response=3DTrue > 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: --- 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 =3D NULL; Empty2 *retval; - QmpInputVisitor *qiv =3D qmp_input_visitor_new_strict(QOBJECT(args)); - QapiDeallocVisitor *qdv; - Visitor *v; =20 - v =3D qmp_input_get_visitor(qiv); + (void)args; =20 retval =3D qmp_user_def_cmd0(&err); if (err) { @@ -269,10 +266,6 @@ =20 out: error_propagate(errp, err); - qmp_input_visitor_cleanup(qiv); - qdv =3D qapi_dealloc_visitor_new(); - v =3D qapi_dealloc_get_visitor(qdv); - qapi_dealloc_visitor_cleanup(qdv); } =20 static void qmp_marshal_user_def_cmd1(QDict *args, QObject **ret, Error **= errp) 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 =E2=80=98qmp_marshal_user_def= _cmd0=E2=80=99: tests/test-qmp-marshal.c:264:14: error: variable =E2=80=98v=E2=80= =99 set but not used [-Werror=3Dunused-but-set-variable] Visitor *v; ^