On Mon, 01/25 14:04, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is referring to. Its value should be ignored unless
> > BDRV_BLOCK_OFFSET_VALID in ret is set.
> >
> > Until block drivers fill in the right value, let's clear it explicitly
> > right before calling .bdrv_get_block_status.
> >
> > The "bs->file" condition in bdrv_co_get_block_status is kept now to keep
> > iotest
> > case 102 passing, and will be fixed once all drivers return the right file
> > pointer.
> >
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > Reviewed-by: Max Reitz <mre...@redhat.com>
> > ---
> > block/io.c | 38 ++++++++++++++++++++++++++------------
> > block/iscsi.c | 6 ++++--
> > block/mirror.c | 3 ++-
> > block/parallels.c | 2 +-
> > block/qcow.c | 2 +-
> > block/qcow2.c | 2 +-
> > block/qed.c | 3 ++-
> > block/raw-posix.c | 3 ++-
> > block/raw_bsd.c | 3 ++-
> > block/sheepdog.c | 2 +-
> > block/vdi.c | 2 +-
> > block/vmdk.c | 2 +-
> > block/vpc.c | 2 +-
> > block/vvfat.c | 2 +-
> > include/block/block.h | 11 +++++++----
> > include/block/block_int.h | 3 ++-
> > qemu-img.c | 13 +++++++++----
> > 17 files changed, 64 insertions(+), 35 deletions(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index 5bb353a..0836991 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t
> > sector_num,
> > int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> > {
> > int64_t target_sectors, ret, nb_sectors, sector_num = 0;
> > + BlockDriverState *file;
> > int n;
> >
> > target_sectors = bdrv_nb_sectors(bs);
> > @@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs,
> > BdrvRequestFlags flags)
> > if (nb_sectors <= 0) {
> > return 0;
> > }
> > - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> > + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
> > if (ret < 0) {
> > error_report("error getting block status at sector %" PRId64
> > ": %s",
> > sector_num, strerror(-ret));
> > @@ -1466,6 +1467,7 @@ int bdrv_flush_all(void)
> > typedef struct BdrvCoGetBlockStatusData {
> > BlockDriverState *bs;
> > BlockDriverState *base;
> > + BlockDriverState **file;
> > int64_t sector_num;
> > int nb_sectors;
> > int *pnum;
> > @@ -1490,7 +1492,8 @@ typedef struct BdrvCoGetBlockStatusData {
> > */
> > static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> > int64_t sector_num,
> > - int nb_sectors, int
> > *pnum)
> > + int nb_sectors, int
> > *pnum,
> > + BlockDriverState
> > **file)
>
> This function has a comment explaining all parameters. Except **file
> now.
Will add comment for @file too.
>
> My impression at the moment is that it generally returns bs->file for
> format drivers and bs itself for protocol drivers if the sector is
> allocated in this layer.
Yes.
>
> > {
> > int64_t total_sectors;
> > int64_t n;
> > @@ -1520,16 +1523,19 @@ static int64_t coroutine_fn
> > bdrv_co_get_block_status(BlockDriverState *bs,
> > return ret;
> > }
> >
> > - ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors,
> > pnum);
> > + *file = NULL;
> > + ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors,
> > pnum,
> > + file);
> > if (ret < 0) {
> > *pnum = 0;
> > + *file = NULL;
>
> You specified that file is to be ignored unless BDRV_BLOCK_OFFSET_VALID
> is set. Why reset it here then? It's not reset in other error cases.
No particular reason, and it can be safely removed.
>
> > return ret;
> > }
> >
> > if (ret & BDRV_BLOCK_RAW) {
> > assert(ret & BDRV_BLOCK_OFFSET_VALID);
> > return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> > - *pnum, pnum);
> > + *pnum, pnum, file);
> > }
> >
> > if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 2ea5a4a..b8d29e1 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -2884,7 +2884,7 @@ static coroutine_fn int
> > vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
> > }
> >
> > static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> > - int64_t sector_num, int nb_sectors, int* n)
> > + int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
> > {
> > BDRVVVFATState* s = bs->opaque;
> > *n = s->sector_count - sector_num;
>
> This still returns NULL at the end of the series. Shouldn't it return bs
> like other protocol drivers do?
Yes, we need another patch for vvfat.
Thanks!
Fam
