[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/5] qcow2: Work with bytes in qcow2_get_cluster
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 1/5] qcow2: Work with bytes in qcow2_get_cluster_offset() |
Date: |
Fri, 3 Jun 2016 11:44:39 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block/qcow2-cluster.c | 43 ++++++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> 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;
>
> - index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
> - nb_needed = *num + index_in_cluster;
> + bytes = *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 = MIN(*num * BDRV_SECTOR_SIZE, UINT32_MAX);
Then again, the interface is about to change, so this may just be a
transient problem.
>
> - l1_bits = 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 = offset_into_cluster(s, offset);
> + bytes_needed = 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.
>
> - nb_available = (1ULL << l1_bits) - (offset & ((1ULL << l1_bits) - 1));
> -
> - /* compute the number of available sectors */
> + l1_bits = s->l2_bits + s->cluster_bits;
>
> - nb_available = (nb_available >> 9) + index_in_cluster;
> + /* compute how many bytes there are between the start of the cluster
> + * containing offset and the end of the l1 entry */
> + bytes_available = (1ULL << l1_bits) - (offset & ((1ULL << l1_bits) - 1))
> + + offset_in_cluster;
>
> - if (nb_needed > nb_available) {
> - nb_needed = nb_available;
> + if (bytes_needed > bytes_available) {
> + bytes_needed = bytes_available;
> }
> - assert(nb_needed <= INT_MAX);
> + assert(bytes_needed <= INT_MAX);
Is this assertion too late given the spots I pointed out above as
possible overflow points?
>
> *cluster_offset = 0;
>
> @@ -542,7 +540,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
> *cluster_offset = be64_to_cpu(l2_table[l2_index]);
>
> /* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */
> - nb_clusters = size_to_clusters(s, nb_needed << 9);
> + nb_clusters = size_to_clusters(s, bytes_needed);
>
> ret = qcow2_get_cluster_type(*cluster_offset);
> switch (ret) {
> @@ -589,13 +587,16 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
>
> qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
>
> - nb_available = (c * s->cluster_sectors);
> + bytes_available = (c * s->cluster_size);
>
> out:
> - if (nb_available > nb_needed)
> - nb_available = nb_needed;
> + if (bytes_available > bytes_needed) {
> + bytes_available = bytes_needed;
> + }
>
> - *num = nb_available - index_in_cluster;
> + bytes = bytes_available - offset_in_cluster;
> + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> + *num = bytes >> BDRV_SECTOR_BITS;
>
> return ret;
Looks like it is headed in the right direction, though.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 0/5] qcow2: Implement .bdrv_co_preadv/pwritev, Kevin Wolf, 2016/06/03
- [Qemu-devel] [PATCH 1/5] qcow2: Work with bytes in qcow2_get_cluster_offset(), Kevin Wolf, 2016/06/03
- Re: [Qemu-devel] [PATCH 1/5] qcow2: Work with bytes in qcow2_get_cluster_offset(),
Eric Blake <=
- [Qemu-devel] [PATCH 4/5] qcow2: Use bytes instead of sectors for QCowL2Meta, Kevin Wolf, 2016/06/03
- [Qemu-devel] [PATCH 3/5] qcow2: Make copy_sectors() byte based, Kevin Wolf, 2016/06/03
- [Qemu-devel] [PATCH 2/5] qcow2: Implement .bdrv_co_preadv(), Kevin Wolf, 2016/06/03
- [Qemu-devel] [PATCH 5/5] qcow2: Implement .bdrv_co_pwritev, Kevin Wolf, 2016/06/03