Re: [Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Kevin Wolf <kwolf@redhat.com>]

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 <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@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.

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.

>  {
>      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.

>          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?

Kevin