Re: [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Kevin Wolf <kwolf@redhat.com>]

Am 22.01.2014 um 07:57 hat Hu Tao geschrieben:
> n_start can be actually calculated from offset. The number of
> sectors to be allocated(n_end - n_start) can be passed in in
> num. By removing n_start and n_end, we can save two parameters.
> 
> The side effect is there is a bug in qcow2.c:preallocate() that
> passes incorrect n_start to qcow2_alloc_cluster_offset() is
> fixed. The bug can be triggerred by a larger cluster size than
> the default value(65536), for example:
> 
> ./qemu-img create -f qcow2 \
>   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-cluster.c | 14 ++++++--------
>  block/qcow2.c         |  6 +++---
>  block/qcow2.h         |  2 +-
>  trace-events          |  2 +-
>  4 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8534084..c57f39d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1182,7 +1182,7 @@ fail:
>   * Return 0 on success and -errno in error cases
>   */
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> +    int *num, uint64_t *host_offset, QCowL2Meta **m)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t start, remaining;
> @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      uint64_t cur_bytes;
>      int ret;
>  
> -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> -                                      n_start, n_end);
> +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
>  
> -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> -    offset = start_of_cluster(s, offset);
> +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
>  
>  again:
> -    start = offset + (n_start << BDRV_SECTOR_BITS);
> -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> +    start = offset;
> +    remaining = *num << BDRV_SECTOR_BITS;
>      cluster_offset = 0;
>      *host_offset = 0;
>      cur_bytes = 0;
> @@ -1284,7 +1282,7 @@ again:
>          }
>      }
>  
> -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> +    *num -= remaining >> BDRV_SECTOR_BITS;
>      assert(*num > 0);
>      assert(*host_offset != 0);
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8ec9db1..a0596ec 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + remaining_sectors;
> +        cur_nr_sectors = remaining_sectors;
>          if (s->crypt_method &&
>              n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
>              n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
>          }

You don't want to change n_end here any more, this should affect
cur_nr_sectors now. n_end becomes completely unused then and can be
removed.

I wonder why the compiler doesn't complain here, this is uninitialised
use and a write-only variable at the same time.

Kevin