From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40458) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XpgYN-0004FN-Vy for qemu-devel@nongnu.org; Sat, 15 Nov 2014 11:50:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XpgYJ-0001vE-0b for qemu-devel@nongnu.org; Sat, 15 Nov 2014 11:50:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XpgYI-0001v5-PX for qemu-devel@nongnu.org; Sat, 15 Nov 2014 11:50:22 -0500 Message-ID: <54678444.8060009@redhat.com> Date: Sat, 15 Nov 2014 09:50:12 -0700 From: Eric Blake MIME-Version: 1.0 References: <1415970374-16811-1-git-send-email-mreitz@redhat.com> <1415970374-16811-7-git-send-email-mreitz@redhat.com> In-Reply-To: <1415970374-16811-7-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="p8N4j15xR3BQn9BKnWA2JdLxQGSdeMueF" Subject: Re: [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reallocation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --p8N4j15xR3BQn9BKnWA2JdLxQGSdeMueF Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/14/2014 06:05 AM, Max Reitz wrote: > Add a helper function for reallocating a refcount array, independently s/independently/independent/ > of the refcount order. The newly allocated space is zeroed and the > function handles failed reallocations gracefully. This patch is doing two things: it is refactoring things into a nice helper function (mentioned), AND it is adding a guarantee that you now always allocate a table on cluster boundaries, even when you aren't using the full table (hinted at elsewhere in the series, but noticeably absent here). I think you want to add more comments to the commit message making that more obvious, since it looks like you rely on that guarantee later. >=20 > Signed-off-by: Max Reitz > --- > block/qcow2-refcount.c | 121 +++++++++++++++++++++++++++++------------= -------- > 1 file changed, 72 insertions(+), 49 deletions(-) >=20 > + > +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array, > + int64_t *size, int64_t new_size) I think this function deserves a comment stating that *array is actually allocated to full cluster size with a 0 tail, so that it can be written straight to disk. > +{ > + /* Round to clusters so the array can be directly written to disk = */ > + size_t old_byte_size =3D ROUND_UP(refcount_array_byte_size(s, *siz= e), > + s->cluster_size); > + size_t new_byte_size =3D ROUND_UP(refcount_array_byte_size(s, new_= size), > + s->cluster_size); > + uint16_t *new_ptr; Can old_byte_size ever equal new_byte_size? Or are we guaranteed that this will only be called when we really need to add another cluster to the reftable? [reading further] Yes, it looks like *size and new_size are not necessarily cluster-aligned, so as an example, it is very likely that we might call realloc_refcount_array with the existing size of 20 and a new size of 21, both of which fit within the same byte size when rounded up to cluster boundary. But that means that the realloc is a no-op in that case; might it be worth special-casing rather than wasting time on the g_try_realloc and no-op memset? [at least the code works correctly even without a special case shortcut] > + > + new_ptr =3D g_try_realloc(*array, new_byte_size); > + if (new_byte_size && !new_ptr) { > + return -ENOMEM; > + } Is it worth asserting that new_byte_size is non-zero? Why would anyone ever call this to resize down to 0? (But I can see where you DO call it with old_byte_size of zero, when initializing data structures and using this function for the first allocation.) > + > + if (new_ptr) { If we assert that new_byte_size is non-zero, then at this point, new_ptr is non-NULL and this condition is pointless. > + memset((void *)((uintptr_t)new_ptr + old_byte_size), 0, > + new_byte_size - old_byte_size); > + } > + > + *array =3D new_ptr; > + *size =3D new_size; > + > + return 0; > +} > =20 Code looks correct as written, whether or not you also add more comments, asserts, and/or shortcuts for no-op situations. So: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --p8N4j15xR3BQn9BKnWA2JdLxQGSdeMueF 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 iQEcBAEBCAAGBQJUZ4REAAoJEKeha0olJ0NqvP0H/i6/bau2SMO+e4up4S1xFq9c igA5cOrCLIO6nHbWJMiq+GwAZy2E4ofI79rnEWfqeWZcUkgs5eRPx0OXYdbmu1ax KqzqZpcy+eaMH2pYse0jyA9ycx0KvKeRQhKW93vOa0hJooyiYdaXmxHc0oj3SjVV OgpbRRYyBGuGdsA7WwkWTfobop7TqdakONFmn+XSsOeLOvymN7KYM6zrHenJxYH5 PBigwqoIvn/9Jt1gY9lPVTgcDONRUbWxmPUF/hhL+LUxKju94p18skSe6UfwRkEe CwJ3xjsjBkrhJESA+x8sTAVG2+wX5Zags4gT8uTqFNpUT82g7/VyEDurmPVaNHs= =ZGXI -----END PGP SIGNATURE----- --p8N4j15xR3BQn9BKnWA2JdLxQGSdeMueF--