From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37327) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI3LV-0005lM-B8 for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZI3LU-0001ay-7O for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI3LT-0001au-W0 for qemu-devel@nongnu.org; Wed, 22 Jul 2015 19:22:40 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-30-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B025B9.2060004@redhat.com> Date: Wed, 22 Jul 2015 17:22:33 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-30-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="MhLMAjkj5H3XM5b1S47VKTv0nOxpEu30n" Subject: Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --MhLMAjkj5H3XM5b1S47VKTv0nOxpEu30n Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > is_c_ptr() looks whether the end of the C text for the type looks like > a pointer. Works, but is fragile. >=20 > We now have a better tool: use QAPISchemaType method c_null(). The > initializers for non-pointers become prettier: 0, false or the > enumeration constant with the value 0 instead of {0}. >=20 > One place looks suspicious: we initialize pointers, but not > non-pointers. Either the initialization is superfluous and should be > deleted, or the non-pointers need it as well, or something subtle is > going on and needs a comment. Since I lack the time to figure it out > now, mark it FIXME. Commenting on just this part for now (out of time to review this patch proper for today): > @@ -214,7 +208,8 @@ def gen_marshal_input(name, args, ret_type, middle_= mode): > header=3Dhdr) > =20 > if ret_type: > - if is_c_ptr(ret_type.c_type()): > + # FIXME fishy: only pointers are initialized > + if ret_type.c_null() =3D=3D 'NULL': > retval =3D " %s retval =3D NULL;" % ret_type.c_type() > else: > retval =3D " %s retval;" % ret_type.c_type() Here's an example function impacted by this: static void qmp_marshal_input_guest_file_open(QDict *args, QObject **ret, Error **errp) { Error *local_err =3D NULL; int64_t retval; QmpInputVisitor *mi =3D qmp_input_visitor_new_strict(QOBJECT(args)); QapiDeallocVisitor *md; Visitor *v; char *path =3D NULL; bool has_mode =3D false; char *mode =3D NULL; =2E.. retval =3D qmp_guest_file_open(path, has_mode, mode, &local_err); if (local_err) { goto out; } qmp_marshal_output_guest_file_open(retval, ret, &local_err); But compare that to any other function that returns a pointer, and you'll see the same pattern (the only use of retval is in the final call to qmp_marshal_output..., right after assigning retval); that is, initializing to NULL is dead code, and you could get away with just always declaring it instead of worrying about c_null() in this code. Or maybe we have a leak - if the 'if (local_err)' can ever trigger even when a function returned non-NULL, then our out: label is missing a free(retval), but only when retval is a pointer. Or maybe we change the code to assert that retval is NULL if local_err is set after calling the user's function, to prove we don't have a leak. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --MhLMAjkj5H3XM5b1S47VKTv0nOxpEu30n 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/ iQEcBAEBCAAGBQJVsCW+AAoJEKeha0olJ0NqcAcH/1CqEjPRdQorR5DkaRC20SRA aZuK4HuX7cIf0Si7q1FKc4u9Jwy7VnrFer1LRavE8RlmnyQCRJjh5uptbn2jOgl0 LZcg1l51ml6F8sdrRwX84DTztdilEPLPmU7VTxY2u7jPGhWlWAGMbfLMblbQ6O6I JD7wpayCcXEbznV82uOmQ5FPzdjNekBJs0T7JicWp3FR3fya5XmJxAKmxshAcBoS SvcVgTou93iusZWNqAmhA3Tj4bVOxgkGAidEWRVHBiuATwINR6EKRD8sS9tjc7Yz Xx9sr0V7KIE+hxg4nItXf4efzaNz8GAyuDhtfcp3KxFZY/wWJFBaCmB9MWTGdqA= =uPF4 -----END PGP SIGNATURE----- --MhLMAjkj5H3XM5b1S47VKTv0nOxpEu30n--