From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53597) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJQcM-00079B-IE for qemu-devel@nongnu.org; Thu, 05 Feb 2015 12:53:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJQcH-0000cL-8o for qemu-devel@nongnu.org; Thu, 05 Feb 2015 12:53:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJQcH-0000bt-2N for qemu-devel@nongnu.org; Thu, 05 Feb 2015 12:53:25 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t15HrNsD017823 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 5 Feb 2015 12:53:23 -0500 Message-ID: <54D3AE12.4070306@redhat.com> Date: Thu, 05 Feb 2015 10:53:22 -0700 From: Eric Blake MIME-Version: 1.0 References: <1423151912-24863-1-git-send-email-mreitz@redhat.com> In-Reply-To: <1423151912-24863-1-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="n43DGf7c8XXOeVjPTgRpWBqvNLaBPhTMi" Subject: Re: [Qemu-devel] [PATCH v2] qcow2: Rewrite qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --n43DGf7c8XXOeVjPTgRpWBqvNLaBPhTMi Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/05/2015 08:58 AM, Max Reitz wrote: > qcow2_alloc_bytes() is a function with insufficient error handling and > an unnecessary goto. This patch rewrites it. >=20 > Signed-off-by: Max Reitz > --- > v2: > - s/free_cluster_index/free_byte_index/ [Eric] > - added an assertion at the start of the function that > s->free_byte_offset is either 0 or points to the tail of a cluster > (but never to the start) > - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric] > - added an assertion that s->free_byte_offset is set before using it > [Eric] Looks nicer. > + cluster_end =3D ROUND_UP(s->free_byte_offset, s->cluster_size)= ; > + if (!s->free_byte_offset || cluster_end !=3D new_cluster) { Here, I can prove that the left side of the || makes no difference to the outcome (using just the right half is sufficient, since !s->free_byte_offset implies cluster_end =3D=3D 0, but new_cluster will b= e non-zero because we never allocate the header cluster). I'd be fine if you wanted to respin that and add the comment for the micro-optimized shorter code; but for maintenance purposes, it's easier to rely on the explicit condition (even if redundant) than to think about whether a proof listed in a comment is correct, so I'm also fine leaving things as = is. > + s->free_byte_offset =3D new_cluster; At this point, s->free_byte_offset violates the precondition if we just allocated a cluster, so we have to make sure that we restore the precondition before exiting... > + if (offset_into_cluster(s, s->free_byte_offset)) { > + int ret =3D qcow2_update_cluster_refcount(bs, > + s->free_byte_offset >> s->cluster_bits, 1, > + QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + if (new_cluster > 0) { > + qcow2_free_clusters(bs, new_cluster, s->cluster_size, > + QCOW2_DISCARD_OTHER); > + } > + return ret; =2E..this early exit only happens if s->free_byte_offset was pointing to = a tail, and there are no other early exits, with the final exit properly restoring the precondition. Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --n43DGf7c8XXOeVjPTgRpWBqvNLaBPhTMi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJU064SAAoJEKeha0olJ0Nq1F0H/2ZMK/smkw7LpikI0GUMDt9i rXypABozchWwB6Tgw61iKNZr/MjAsEASIcaLwL9Teh0hnDJ7ZLx5yFmGenUkoASL bSWMCJ+AdMtfD9qFo2ByBoRunKNlq5mHyCGlQX8EIFBzMhXAa7a70kuqzefRHc25 +lqI6a/hTcQ9GdyUb7FuYfWBOPrUNiXoGOa9rNIYzpl31rBgSJ426efBN0Xptw+X FgQx78wZQEqesKTbrTVyWc/FJexZQrMZEkk5RLDBlaQQZ1HoyzNls1mMMJm2qe0/ NQR/5IhuJvKDrhrDMDtgDPke+Rr/yDcxePDmS5iODLIVDJ/Sbal/2NtGJ0sTTfY= =ucw7 -----END PGP SIGNATURE----- --n43DGf7c8XXOeVjPTgRpWBqvNLaBPhTMi--