On 20.12.2016 20:15, Eric Blake wrote: > Passing a byte offset, but sector count, when we ultimately > want to operate on cluster granularity, is madness. Clean up > the interfaces to take both offset and count as bytes, while > still keeping the assertion added previously that the caller > must align the values to a cluster. Then rename things to > make sure backports don't get confused by changed units: > instead of qcow2_discard_clusters() and qcow2_zero_clusters(), > we now have qcow2_cluster_discard and qcow2_cluster_zeroize(). > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v4: improve function names, split assertion additions into earlier patch > [no v3 or v2] > v1: https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg00339.html > --- > block/qcow2.h | 9 +++++---- > block/qcow2-cluster.c | 17 ++++++++--------- > block/qcow2-snapshot.c | 7 +++---- > block/qcow2.c | 21 +++++++++------------ > 4 files changed, 25 insertions(+), 29 deletions(-)
Nothing functionally bad, so:
Reviewed-by: Max Reitz <mre...@redhat.com>
Some notes, though:
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 1823414..a131b72 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -543,10 +543,11 @@ uint64_t
> qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
> int compressed_size);
>
> int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
> - int nb_sectors, enum qcow2_discard_type type, bool full_discard);
> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int
> nb_sectors,
> - int flags);
> +int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
> + uint64_t count, enum qcow2_discard_type type,
> + bool full_discard);
> +int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> + uint64_t count, int flags);
I know byte count parameters are often called "count", but I find it a
bit problematic if the function name contains a word that can be a unit.
In these cases, it's not entirely clear whether "count" refers to bytes
or clusters. Maybe "length" or "size" would be better?
Purely subjective and thus optional, of course.
Another thing: I'm not quite positive whether qcow2_cluster_zeroize()
can actually handle a byte count greater than INT_MAX. If you could pass
such a number to it, the multiplication "ret * s->cluster_size" might
overflow. It's only caller qcow2_co_pwrite_zeroes() will never exceed
that limit, but maybe this should be made clear somewhere -- either by
making the parameter an int instead of a uint64_t or by asserting it in
the function.
>
> int qcow2_expand_zero_clusters(BlockDriverState *bs,
> BlockDriverAmendStatusCB *status_cb,
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 3304a15..aad5183 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1511,16 +1511,15 @@ static int discard_single_l2(BlockDriverState *bs,
> uint64_t offset,
> return nb_clusters;
> }
>
> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
> - int nb_sectors, enum qcow2_discard_type type, bool full_discard)
> +int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
> + uint64_t count, enum qcow2_discard_type type,
> + bool full_discard)
> {
> BDRVQcow2State *s = bs->opaque;
> - uint64_t end_offset;
> + uint64_t end_offset = offset + count;
> uint64_t nb_clusters;
> int ret;
>
> - end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
> -
> /* Caller must pass aligned values */
> assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size));
Directly below this we have "offset - end_offset" which could be
shortened to "count" (or "length" or "size" or...).
Max
signature.asc
Description: OpenPGP digital signature
