Re: [Qemu-devel] [PATCH v2 2/7] qcow2: Use unsigned int for both members of Qcow2COWRegion

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]

On 06/07/2017 09:08 AM, Alberto Garcia wrote:
> Qcow2COWRegion has two attributes:
> 
> - The offset of the COW region from the start of the first cluster
>   touched by the I/O request. Since it's always going to be positive
>   and the maximum request size is at most INT_MAX, we can use a
>   regular unsigned int to store this offset.

I don't know if we will ever get to the point that we allow a 64-bit
request at the block layer (and then the block layer guarantees it is
split down to the driver's limits, which works when a driver is still
bound by 32-bit limits).  But we ALSO know that a cluster is at most 2M
(in our current implementation of qcow2), so the offset of where the COW
region starts in relation to the start of a cluster is < 2M.

> 
> - The size of the COW region in bytes. This is guaranteed to be >= 0,
>   so we should use an unsigned type instead.

And likewise, since a COW region is a sub-cluster, and clusters are
bounded at 2M, we also have a sub-int upper bound on the size of the region.

> 
> In x86_64 this reduces the size of Qcow2COWRegion from 16 to 8 bytes.
> It will also help keep some assertions simpler now that we know that
> there are no negative numbers.
> 
> The prototype of do_perform_cow() is also updated to reflect these
> changes.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 4 ++--
>  block/qcow2.h         | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index d1c419f52b..a86c5a75a9 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -406,8 +406,8 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
>  static int coroutine_fn do_perform_cow(BlockDriverState *bs,
>                                         uint64_t src_cluster_offset,
>                                         uint64_t cluster_offset,
> -                                       int offset_in_cluster,
> -                                       int bytes)
> +                                       unsigned offset_in_cluster,
> +                                       unsigned bytes)

I don't know if the code base has a strong preference for 'unsigned int'
over 'unsigned', but it doesn't bother me.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]