From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHjaA-0005RB-F4 for qemu-devel@nongnu.org; Mon, 27 Jun 2016 23:21:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHja4-0001rg-Ov for qemu-devel@nongnu.org; Mon, 27 Jun 2016 23:21:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHja4-0001rc-GQ for qemu-devel@nongnu.org; Mon, 27 Jun 2016 23:20:56 -0400 References: <1463784024-17242-1-git-send-email-eblake@redhat.com> <1463784024-17242-11-git-send-email-eblake@redhat.com> <8760tbag5x.fsf@dusky.pond.sub.org> <871t3x723b.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <5771ED16.9070100@redhat.com> Date: Mon, 27 Jun 2016 21:20:54 -0600 MIME-Version: 1.0 In-Reply-To: <871t3x723b.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fO0MJ8ol1ssOIlftuc1Gu76R0R0bK7bE0" Subject: Re: [Qemu-devel] [PATCH v7 10/15] qapi-event: Reduce chance of collision with event data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --fO0MJ8ol1ssOIlftuc1Gu76R0R0bK7bE0 From: Eric Blake To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth Message-ID: <5771ED16.9070100@redhat.com> Subject: Re: [Qemu-devel] [PATCH v7 10/15] qapi-event: Reduce chance of collision with event data References: <1463784024-17242-1-git-send-email-eblake@redhat.com> <1463784024-17242-11-git-send-email-eblake@redhat.com> <8760tbag5x.fsf@dusky.pond.sub.org> <871t3x723b.fsf@dusky.pond.sub.org> In-Reply-To: <871t3x723b.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/16/2016 06:25 AM, Markus Armbruster wrote: > Markus Armbruster writes: >=20 >> Eric Blake writes: >> >>> When an event has data that is not boxed, we are exposing all of >>> its members alongside our local variables. So far, we haven't >>> hit a collision, but it may be a matter of time before someone >>> wants to name a QMP data element 'err' or similar. We can separate >>> the names by making the public function a shell that creates a >>> simple wrapper, then calls a worker that operates on only the >>> boxed version and thus has no user-supplied names to worry about >>> in naming its local variables. For boxed events, we don't need >>> the wrapper. >>> >>> There is still a chance for collision with 'errp' (if that happens, >>> tweak c_name() to rename a QMP member 'errp' into the C member >>> 'q_errp'), and with 'param' (if that happens, tweak gen_event_send() >>> and gen_param_var() to name the temporary variable 'q_param'). But >>> with the division done here, the real worker function no longer has >>> to worry about collisions. >>> >>> +++ b/scripts/qapi.py >>> @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType): >>> return QAPISchemaType.c_name(self) >>> >>> def c_type(self): >>> - assert not self.is_implicit() >> >> Huh? Required, because we now pass a pointer to an implicit type from qapi_event_send_FOO() to do_qapi_event_send_FOO(), so the c_type() of that implicit type is required for generating the C type for that parameter. Will document it better in the commit message. >>> @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): >>> ret +=3D mcgen(''' >>> v =3D qmp_output_visitor_new(&obj); >>> >>> -''') >>> - >>> - if box: >>> - ret +=3D mcgen(''' >>> - visit_type_%(c_name)s(v, NULL, &arg, &err); >>> -''', >>> - c_name=3Darg_type.c_name(), name=3Darg_type= =2Ename) >>> - else: >>> - ret +=3D mcgen(''' >>> visit_start_struct(v, "%(name)s", NULL, 0, &err); >>> if (err) { >>> goto out; >>> } >>> - visit_type_%(c_name)s_members(v, ¶m, &err); >>> + visit_type_%(c_name)s_members(v, arg, &err); >>> if (!err) { >>> visit_check_struct(v, &err); >>> } >> >> Getting confused... why are we getting rid of the box case here? No good reason. The visit_type_FOO() is more compact than visit_type_FOO_members(), but only when we have a non-implicit type. So for v8, I'm switching the conditional from 'if box:' to 'if arg_type.is_implicit():', more or less. >> >> Too many conditionals... gen_event_send() has three cases: empty >> arg_type, non-empty arg_type and box, non-empty arg_type and not box. >> The commit message shows the change to generated code for the second >> case. It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) goin= g >> away. >=20 > Case empty arg_type: no change > Example: POWERDOWN Good. >=20 > Case non-empty arg_type and box: visit gets open-coded > Example: EVENT_E Fixed in v8 so that it no longer changes. > The open-coded visit drops the !*obj check (okay, @arg isn't going > anywhere), skips the visit_check_struct() differently, and drops the > qapi_free_FOO() (okay, condition is always false here). >=20 > So this isn't wrong. But why open-code? No need to add new open-coding, but we already had existing open-coding for anonymous non-boxed 'data' (in part because commit 7ce106a9 intentionally chose not to create visit_type_FOO() for implicit types). >=20 > Case non-empty arg_type and not box: > Example: ACPI_DEVICE_OST >=20 >=20 > This is the case the commit message advertises. >=20 > There is no visit_type_FOO() we could compare too, since FOO is an > implicit type And in reviewing your message, I realize we have NO testsuite coverage of= : { 'event': 'EVENT', 'data': 'NamedStruct' } Guess I get to add that first. Such a usage will then be improved by using visit_type_NamedStruct() rather than open-coding around visit_type_NamedStruct_members(). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --fO0MJ8ol1ssOIlftuc1Gu76R0R0bK7bE0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXce0WAAoJEKeha0olJ0Nq9+YH/34Olwq9zMwr2i7iCdQuaXXZ J+aby0hSWRTGm07+xy7JD8dDSfHRWxxTPZ3HfM8603reLiGkg2OaVWF+K+A2j2Ch UqLokhvuSeFcdsP5HKh3/swJeqBY8heGKxU7wCbykxbzdOqwl/nBSrwvc9i8DeOn zxCNb8IBNBu1FZjH6z79da7+4rVam5YGfkp09yGpZv7W/wlaOKT0dI3leTpp4WYY 4aMNZ0aK6RVImJQZsMhU5ba5Pk9TyMSv0szYAYs9DXdtrF5w9U91Cxo2Du3tkT+A Xb/c/771N4VK8YsoZY17YTvkCNV3t6i2d1/dtOeqR19ZhxTKc/n2AdfSQ57tr9I= =jVcc -----END PGP SIGNATURE----- --fO0MJ8ol1ssOIlftuc1Gu76R0R0bK7bE0--