From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51305) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZPT2-00043t-GI for qemu-devel@nongnu.org; Tue, 08 Sep 2015 16:26:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZPT1-0000f2-E2 for qemu-devel@nongnu.org; Tue, 08 Sep 2015 16:26:12 -0400 References: <1441742995-11794-1-git-send-email-mreitz@redhat.com> <1441742995-11794-2-git-send-email-mreitz@redhat.com> <55EF4387.9060503@redhat.com> From: Max Reitz Message-ID: <55EF445B.50108@redhat.com> Date: Tue, 8 Sep 2015 22:26:03 +0200 MIME-Version: 1.0 In-Reply-To: <55EF4387.9060503@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JghKJDpFEf6OKdETSauFnhgaMUNsScCqQ" Subject: Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JghKJDpFEf6OKdETSauFnhgaMUNsScCqQ Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08.09.2015 22:22, Eric Blake wrote: > On 09/08/2015 02:09 PM, Max Reitz wrote: >> Sadly, some images may have more clusters than what can be represented= >> using a plain int. We should be prepared for that case (in >> qcow2_check_refcounts() we actually were trying to catch that case, bu= t >> since size_to_clusters() truncated the returned value, that check neve= r >> did anything useful). >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-cluster.c | 20 +++++++++++--------- >> block/qcow2.h | 2 +- >> 2 files changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 2975b83..a34f0b1 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -473,8 +473,8 @@ 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 index_in_cluster; >> + uint64_t nb_available, nb_needed, nb_clusters; >=20 > Most uses are storing the results unsigned... >=20 >> =20 >> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size) >> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t siz= e) >> { >> return (size + (s->cluster_size - 1)) >> s->cluster_bits; >> } >=20 > ...and the function itself doesn't appear to intentionally return > negative (unless size was passed in as negative, but then that may be > accidental). Should it just return uint64_t instead? It won't matter in practice because we generally don't support any offsets bigger than INT64_MAX anyway; the @size parameter has been an int64_t all along, too. If I have to respin for some reason (i.e. maintainer not willing to fix up the comment in patch 2), I'll probably change the type, though. > At any rate, I agree that 'int' is too small, so: > Reviewed-by: Eric Blake Thanks! Max --JghKJDpFEf6OKdETSauFnhgaMUNsScCqQ 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 iQEcBAEBCAAGBQJV70RbAAoJEDuxQgLoOKytNZMIAJm3GCqGWy+qtIFhh2IdH1CN 21buHiXq7PDHFJL+P6v2iAzOoewmbJ9TlFznL68CZkhX7XybQP/im1TWVH1aRweY MXBBf+nwxv2e5Jfis/EJgYhcEC/4uRKzlhvAR11O8HPLpy74jFOJwGpjJ/tabYL5 uuD5O/bM1glZNu+u2iJzglu9Jmb5A8YOTUpxdFzKs+NhscARLRq0BYXXvYtw5on/ ylj1e9j8lu0cYbBp9wq8n875py+u35zGbQlHRsJyd9POubq2X8zwnhOZlTnfEQTp OVbXfLt9O5q66Tg/CMns93zZuv4ryQSyDzkG77OmG9Tk1IrdV+VXyM4khK5DfXs= =aYAP -----END PGP SIGNATURE----- --JghKJDpFEf6OKdETSauFnhgaMUNsScCqQ--