From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4EuN-0004lZ-LU for qemu-devel@nongnu.org; Wed, 02 Dec 2015 16:25:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4EuK-0000tE-7T for qemu-devel@nongnu.org; Wed, 02 Dec 2015 16:25:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56498) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4EuJ-0000tA-Vn for qemu-devel@nongnu.org; Wed, 02 Dec 2015 16:25:48 -0500 References: <1448497401-27784-1-git-send-email-eblake@redhat.com> <1448497401-27784-3-git-send-email-eblake@redhat.com> <87si3r4phq.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <565F61D6.3050907@redhat.com> Date: Wed, 2 Dec 2015 14:25:42 -0700 MIME-Version: 1.0 In-Reply-To: <87si3r4phq.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dhI0lJL65HXslT5uqanMJ8n4iNk1W5bLu" Subject: Re: [Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation 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) --dhI0lJL65HXslT5uqanMJ8n4iNk1W5bLu Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/27/2015 05:05 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Now that all visitors supply both type_int64() and type_uint64() >> callbacks, we can drop the redundant type_int() callback (the >> public interface visit_type_int() remains, but calls into >> type_int64() under the hood). >> >> Signed-off-by: Eric Blake >> >> void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Err= or **errp) >> { >> - int64_t value; >> + uint64_t value; >> >> if (v->type_uint8) { >> v->type_uint8(v, obj, name, errp); >> } else { >> value =3D *obj; >> - v->type_int(v, &value, name, errp); >> - if (value < 0 || value > UINT8_MAX) { >> + v->type_uint64(v, &value, name, errp); >> + if (value > UINT8_MAX) { >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, >> name ? name : "null", "uint8_t"); >> return; >=20 > Note that this relies on value being in range after type_uint64() fails= =2E > If it isn't, we call error_setg() with non-null *errp. >=20 > Two solutions: >=20 > 1. Stipulate that type_uint64() & friends leave value alone on error. > Works, because its initial value *obj is in range. Pre-existing and simpler, but sets a poor example for the rest of the code base (not everyone is going to read the fine print for why it works here), and requires cross-file audits to ensure visitors comply. >=20 > 2. Avoid using value on error. A clean way to do this: >=20 > Error *err =3D NULL; >=20 > value =3D *obj; > v->type_uint64(v, &value, name, &err); > if (err) { > error_propagate(errp, err); > return; > } > if (value < 0 || value > UINT8_MAX) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > name ? name : "null", "uint8_t"); > return; > } > *obj =3D value; >=20 > More boilerplate. If we pick this solution, we'll want a separate > PATCH 1.5 cleaning up the preexisting instances. Of course, if I do the cleanup as 1.5, then patch 3/23 reindents everything, that's a lot of churn. So I may end up rearranging 2 and 3 after all, and then do the cleanup as 3.5. Or maybe option 3, write a pair of helper functions containing the boilerplate for checking against min and max: void visit_type_intN(Visitor *v, int64_t *obj, const char *name, int64_t min, int64_t max, Error **errp); void visit_type_uintN(Visitor *v, int64_t *obj, const char *name, uint64_t max, Error **errp); leaving us with simpler clients: visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp) { int64_t value =3D *obj; visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp); *obj =3D value; } and here, because the helpers are in the same file, it's easier to prove that value was unchanged on error. Or I may even squash 2 and 3 into a single patch now. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --dhI0lJL65HXslT5uqanMJ8n4iNk1W5bLu 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/ iQEcBAEBCAAGBQJWX2HWAAoJEKeha0olJ0NqMvsH/jStyRV4iswP3x5rMtstucS0 77veTiHiyATGegZVkNEkga57GbSbBY1oOJVYv+28HiOv2lqn9t5ftYPapXoY8B6n gFPgP7nySHpRwZ/wuQir8bsGLgq1LOsB6QHllx8Im5hisrxjm04Xgfl0kovUaRCz yqgOUe8+7eqtrdj0ebvl+0sWukBFVVi4gykzSou2vylYhGcq49RSMf+g5JfLgWRD 1fpCQcnPpIw6udxzEbGNg5BHbR0cOa/RvS1DtGwEWaTEBPjpLtK4NOvOSUfBL9vu yKQZZioS6bZoAZF1/dJRLf6w1uJKjxTMBGSFdcBWwR5jIP5q7Iy1cVPkXoPj10g= =FArD -----END PGP SIGNATURE----- --dhI0lJL65HXslT5uqanMJ8n4iNk1W5bLu--