[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t |
Date: |
Wed, 9 Sep 2015 10:45:19 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 08.09.2015 um 22:09 hat Max Reitz geschrieben:
> 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, but
> since size_to_clusters() truncated the returned value, that check never
> did anything useful).
>
> Signed-off-by: Max Reitz <address@hidden>
You seem to fix a few of the callers as well, which is a good thing.
However, what about realloc_refcount_array()? It uses size_t, which can
be 32 bits, whereas the comment in refcount_array_byte_size() suggests
that we could get as much as 2^55.
> 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;
> int ret;
>
> index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
We're probably better off adding an assertion here. The type change is
useless because nb_clusters is only used as a parameter for calling
count_contiguous_(free_)clusters, which is a function that takes int64_t
and returns int (which totally makes sense). In the overflow case it
seems to have an endless loop.
Of course, all of that doesn't really matter because nb_needed never
exceeds a single L2 table.
> @@ -837,10 +837,10 @@ err:
> * write, but require COW to be performed (this includes yet unallocated
> space,
> * which must copy from the backing file)
> */
> -static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
> +static int count_cow_clusters(BDRVQcow2State *s, uint64_t nb_clusters,
> uint64_t *l2_table, int l2_index)
> {
> - int i;
> + uint64_t i;
>
> for (i = 0; i < nb_clusters; i++) {
> uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);
The return value is still int, so this changes the behaviour from an
endless loop (same thing as mentioned above) to a truncated return
value. Questionable whether that is an improvement (I'd say no).
> @@ -960,7 +960,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t
> guest_offset,
> int l2_index;
> uint64_t cluster_offset;
> uint64_t *l2_table;
> - unsigned int nb_clusters;
> + uint64_t nb_clusters;
> unsigned int keep_clusters;
> int ret;
It looks like size isn't limited to a single L2 table there yet, so this
is an important fix. However, handle_alloc() needs the same.
> @@ -1426,7 +1426,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs,
> uint64_t cluster_offset)
> * clusters.
> */
> static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
> - unsigned int nb_clusters, enum qcow2_discard_type type, bool
> full_discard)
> + uint64_t nb_clusters, enum qcow2_discard_type type, bool full_discard)
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t *l2_table;
> @@ -1441,6 +1441,7 @@ static int discard_single_l2(BlockDriverState *bs,
> uint64_t offset,
>
> /* Limit nb_clusters to one L2 table */
> nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> + assert(nb_clusters <= INT_MAX);
>
> for (i = 0; i < nb_clusters; i++) {
> uint64_t old_l2_entry;
> @@ -1503,7 +1504,7 @@ int qcow2_discard_clusters(BlockDriverState *bs,
> uint64_t offset,
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t end_offset;
> - unsigned int nb_clusters;
> + uint64_t nb_clusters;
> int ret;
>
> end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
We can actually assert nb_clusters <= INT_MAX directly after assigning
it and before limiting it to a single L2 table. nb_sectors is already
int, so nb_clusters can never be larger.
I'm not objecting to uint64_t and an assertion, though, being explicit
is always nice.
> @@ -1545,7 +1546,7 @@ fail:
> * clusters.
> */
> static int zero_single_l2(BlockDriverState *bs, uint64_t offset,
> - unsigned int nb_clusters)
> + uint64_t nb_clusters)
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t *l2_table;
> @@ -1560,6 +1561,7 @@ static int zero_single_l2(BlockDriverState *bs,
> uint64_t offset,
>
> /* Limit nb_clusters to one L2 table */
> nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
> + assert(nb_clusters <= INT_MAX);
>
> for (i = 0; i < nb_clusters; i++) {
> uint64_t old_offset;
> @@ -1584,7 +1586,7 @@ static int zero_single_l2(BlockDriverState *bs,
> uint64_t offset,
> int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors)
> {
> BDRVQcow2State *s = bs->opaque;
> - unsigned int nb_clusters;
> + uint64_t nb_clusters;
> int ret;
>
> /* The zero flag is only supported by version 3 and newer */
Same thing really.
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 61f1b57..ce292a0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -415,7 +415,7 @@ static inline int64_t offset_into_cluster(BDRVQcow2State
> *s, int64_t offset)
> return offset & (s->cluster_size - 1);
> }
>
> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
> {
> return (size + (s->cluster_size - 1)) >> s->cluster_bits;
> }
Kevin