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

[Hu Tao]

On Thu, Jan 23, 2014 at 03:29:04PM +0100, Kevin Wolf wrote:
> Am 23.01.2014 um 04:04 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 <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);
> >  
> >  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..0a310cc 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -992,7 +992,6 @@ static coroutine_fn int 
> > qcow2_co_writev(BlockDriverState *bs,
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      int index_in_cluster;
> > -    int n_end;
> >      int ret;
> >      int cur_nr_sectors; /* number of sectors in current iteration */
> >      uint64_t cluster_offset;
> > @@ -1016,14 +1015,10 @@ 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;
> > -        if (s->crypt_method &&
> > -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
> > -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> > -        }
> > +        cur_nr_sectors = remaining_sectors;
> 
> You still need to limit cur_nr_sectors for the encrypted case, otherwise
> you get a buffer overflow of cluster_data later in the function. My
> complaint in v3 was not that you have the limiting, but that applying it
> to n_end doesn't have any effect any more, you need to apply it to
> cur_nr_sectors.

Thanks! I didn't understand you completely:-P.