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

[Benoît Canet]

Le Friday 24 Jan 2014 à 17:32:40 (+0800), Hu Tao a écrit :
> On Thu, Jan 23, 2014 at 06:02:08PM +0100, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 11:04:05 (+0800), Hu Tao a écrit :
> > > 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 <address@hidden>
> > > Signed-off-by: Hu Tao <address@hidden>
> > > ---
> > >  block/qcow2-cluster.c | 14 ++++++--------
> > >  block/qcow2.c         | 11 +++--------
> > >  block/qcow2.h         |  2 +-
> > >  trace-events          |  2 +-
> > >  4 files changed, 11 insertions(+), 18 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);
> > 
> > Why replace something that would round gently an unaligned offset
> > (start_of_cluster) by an assert that would make QEMU exit ?
> 
> It is equivalent to the removed assert().
Oh sorry I didn't see the removed assert() when reviewing :(


> 
>