Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] qemu-img: check block status of backing file when converting.

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3351 bytes --]

On 13.04.2016 15:24, Ren Kimura wrote:
> Signed-off-by: Ren Kimura <rkx1209dev@gmail.com>

How about you put what you wrote in the 0/1 email here?

(But with s/it's/its/ :-))

> ---
>  qemu-img.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 06264d9..53471a1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1451,6 +1451,21 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num)
>      }
>  }
>  
> +static int64_t get_backing_status(BlockDriverState *bs,
> +                                  int64_t sector_num,
> +                                  int nb_sectors, int *pnum,
> +                                  BlockDriverState **file)
> +{
> +    int64_t ret;
> +    if (bs->backing) {
> +        ret = get_backing_status(bs->backing->bs, sector_num,
> +                                 nb_sectors, pnum, file);

This won't really work. You may not fall through to the backing file if
the overlay file has an allocated range here.

So you'll have to call bdrv_get_block_status() unconditionally first,
and only if its return value has neither the BDRV_BLOCK_DATA nor the
BDRV_BLOCK_ZERO flag set you can (and then have to) fall through to its
backing file.

> +    } else {
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, file);
> +    }
> +    return ret;
> +}
> +

Apart from that, I'd suggest to drop the "file" parameter. We don't care
about whatever is returned in it anyway. Just declare a local
BlockDriverState pointer in this function which will receive this value.

The reason for why I'm suggesting this is because this would make it
clearer to the reader that this value is discarded.

Feel free to follow or ignore this suggestion, based on whether you
consider it useful or not.

Max

>  static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>  {
>      int64_t ret;
> @@ -1469,7 +1484,6 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>          if (ret < 0) {
>              return ret;
>          }
> -
>          if (ret & BDRV_BLOCK_ZERO) {
>              s->status = BLK_ZERO;
>          } else if (ret & BDRV_BLOCK_DATA) {
> @@ -1477,10 +1491,17 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>          } else if (!s->target_has_backing) {
>              /* Without a target backing file we must copy over the contents of
>               * the backing file as well. */
> -            /* TODO Check block status of the backing file chain to avoid
> +            /* Check block status of the backing file chain to avoid
>               * needlessly reading zeroes and limiting the iteration to the
>               * buffer size */
> -            s->status = BLK_DATA;
> +            ret = get_backing_status(blk_bs(s->src[s->src_cur]),
> +                                     sector_num - s->src_cur_offset,
> +                                     n, &n, &file);
> +            if (ret & BDRV_BLOCK_ZERO) {
> +                s->status = BLK_ZERO;
> +            } else {
> +                s->status = BLK_DATA;
> +            }
>          } else {
>              s->status = BLK_BACKING_FILE;
>          }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]