From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8t9N-0007TG-FT for qemu-devel@nongnu.org; Fri, 03 Jun 2016 13:44:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8t9L-00030f-6i for qemu-devel@nongnu.org; Fri, 03 Jun 2016 13:44:48 -0400 References: <1464974478-23598-1-git-send-email-kwolf@redhat.com> <1464974478-23598-2-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <5751C207.6000709@redhat.com> Date: Fri, 3 Jun 2016 11:44:39 -0600 MIME-Version: 1.0 In-Reply-To: <1464974478-23598-2-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="nmBvsLo3Cvm86i0nxNFDtEesLCh75C7Xf" Subject: Re: [Qemu-devel] [PATCH 1/5] qcow2: Work with bytes in qcow2_get_cluster_offset() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: jsnow@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --nmBvsLo3Cvm86i0nxNFDtEesLCh75C7Xf Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/03/2016 11:21 AM, Kevin Wolf wrote: > This patch changes the units that qcow2_get_cluster_offset() uses > internally, without touching the interface just yet. This will be done > in another patch. >=20 > Signed-off-by: Kevin Wolf > --- > block/qcow2-cluster.c | 43 ++++++++++++++++++++++---------------------= > 1 file changed, 22 insertions(+), 21 deletions(-) >=20 > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index d901d89..b2405b1 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -482,29 +482,27 @@ int qcow2_get_cluster_offset(BlockDriverState *bs= , uint64_t offset, > unsigned int l2_index; > uint64_t l1_index, l2_offset, *l2_table; > int l1_bits, c; > - unsigned int index_in_cluster, nb_clusters; > - uint64_t nb_available, nb_needed; > + unsigned int offset_in_cluster, nb_clusters; > + uint64_t bytes_available, bytes_needed; > int ret; > + unsigned int bytes; > =20 > - index_in_cluster =3D (offset >> 9) & (s->cluster_sectors - 1); > - nb_needed =3D *num + index_in_cluster; > + bytes =3D *num * BDRV_SECTOR_SIZE; bytes can overflow if num > BDRV_REQUEST_MAX_SECTORS. Is that ever a problem? Would an assert() help, or else clamping it (since it is really just a hint of how far we are allowed to look for similar clusters, but we can always quit looking early): bytes =3D MIN(*num * BDRV_SECTOR_SIZE, UINT32_MAX); Then again, the interface is about to change, so this may just be a transient problem. > =20 > - l1_bits =3D s->l2_bits + s->cluster_bits; > - > - /* compute how many bytes there are between the offset and > - * the end of the l1 entry > - */ > + offset_in_cluster =3D offset_into_cluster(s, offset); > + bytes_needed =3D bytes + offset_in_cluster; Hmm, my idea for clamped bytes above may need tweaking (that is, UINT32_MAX is inappropriate as the clamp value, if you are going to round it up here). Or, since bytes_needed is 64-bits, make either bytes or offset_in_cluster also be 64 bits, or start with '0ULL +', to avoid overflow. > =20 > - nb_available =3D (1ULL << l1_bits) - (offset & ((1ULL << l1_bits) = - 1)); > - > - /* compute the number of available sectors */ > + l1_bits =3D s->l2_bits + s->cluster_bits; > =20 > - nb_available =3D (nb_available >> 9) + index_in_cluster; > + /* compute how many bytes there are between the start of the clust= er > + * containing offset and the end of the l1 entry */ > + bytes_available =3D (1ULL << l1_bits) - (offset & ((1ULL << l1_bit= s) - 1)) > + + offset_in_cluster; > =20 > - if (nb_needed > nb_available) { > - nb_needed =3D nb_available; > + if (bytes_needed > bytes_available) { > + bytes_needed =3D bytes_available; > } > - assert(nb_needed <=3D INT_MAX); > + assert(bytes_needed <=3D INT_MAX); Is this assertion too late given the spots I pointed out above as possible overflow points? > =20 > *cluster_offset =3D 0; > =20 > @@ -542,7 +540,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, = uint64_t offset, > *cluster_offset =3D be64_to_cpu(l2_table[l2_index]); > =20 > /* nb_needed <=3D INT_MAX, thus nb_clusters <=3D INT_MAX, too */ > - nb_clusters =3D size_to_clusters(s, nb_needed << 9); > + nb_clusters =3D size_to_clusters(s, bytes_needed); > =20 > ret =3D qcow2_get_cluster_type(*cluster_offset); > switch (ret) { > @@ -589,13 +587,16 @@ int qcow2_get_cluster_offset(BlockDriverState *bs= , uint64_t offset, > =20 > qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); > =20 > - nb_available =3D (c * s->cluster_sectors); > + bytes_available =3D (c * s->cluster_size); > =20 > out: > - if (nb_available > nb_needed) > - nb_available =3D nb_needed; > + if (bytes_available > bytes_needed) { > + bytes_available =3D bytes_needed; > + } > =20 > - *num =3D nb_available - index_in_cluster; > + bytes =3D bytes_available - offset_in_cluster; > + assert((bytes & (BDRV_SECTOR_SIZE - 1)) =3D=3D 0); > + *num =3D bytes >> BDRV_SECTOR_BITS; > =20 > return ret; Looks like it is headed in the right direction, though. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --nmBvsLo3Cvm86i0nxNFDtEesLCh75C7Xf 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/ iQEcBAEBCAAGBQJXUcIHAAoJEKeha0olJ0NqmOUIAIsdJn+WHfsipMCaQckrMfdt kyJB3k0WM5lz34NnLW/879zDfX7wXhzbfG2wD2rf5qkPuZsIGH+uf8kIwbmcyBtR nTKJ41E9A3bTSvUHlaJPnq9xSW57TBvCYdgT1nTNx5KhhTrhB2qb2naPdwTI0cv1 zAZPpfqwzApoH/NXJwG7gK+p01c6Q5Zwkz08PVzC6rZmCcSjYy4jH8qgMKiAuOcs 8oolhcYTLIOR/O8T+i+SF5qPpgJLZkrj4YuUef25cUW3N92w/LUsdlsVDvUgSgSz XKElNrxCzJb9RPaLYrpsFL/JkhbP9mp/H1WFkf2ozRkWsquoA0h1/lMp34LHjJY= =cLGw -----END PGP SIGNATURE----- --nmBvsLo3Cvm86i0nxNFDtEesLCh75C7Xf--