Re: [Qemu-block] [PATCH v6 30/42] qemu-img: Use child access functions

[Vladimir Sementsov-Ogievskiy]

09.08.2019 19:13, Max Reitz wrote:
> This changes iotest 204's output, because blkdebug on top of a COW node
> used to make qemu-img map disregard the rest of the backing chain (the
> backing chain was broken by the filter).  With this patch, the
> allocation in the base image is reported correctly.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   qemu-img.c                 | 33 ++++++++++++++++++++-------------
>   tests/qemu-iotests/204.out |  1 +
>   2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..3b30c5ae70 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1012,7 +1012,7 @@ static int img_commit(int argc, char **argv)
>           /* This is different from QMP, which by default uses the deepest 
> file in
>            * the backing chain (i.e., the very base); however, the traditional
>            * behavior of qemu-img commit is using the immediate backing file. 
> */
> -        base_bs = backing_bs(bs);
> +        base_bs = bdrv_backing_chain_next(bs);
>           if (!base_bs) {
>               error_setg(&local_err, "Image does not have a backing file");
>               goto done;
> @@ -1632,18 +1632,20 @@ static int convert_iteration_sectors(ImgConvertState 
> *s, int64_t sector_num)
>       if (s->sector_next_status <= sector_num) {
>           uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE;
>           int64_t count;
> +        BlockDriverState *src_bs = blk_bs(s->src[src_cur]);
> +        BlockDriverState *base;
> +
> +        if (s->target_has_backing) {
> +            base = bdrv_filtered_cow_bs(bdrv_skip_rw_filters(src_bs));
> +        } else {
> +            base = NULL;
> +        }
>   
>           do {
>               count = n * BDRV_SECTOR_SIZE;
>   
> -            if (s->target_has_backing) {
> -                ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset,
> -                                        count, &count, NULL, NULL);
> -            } else {
> -                ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
> -                                              offset, count, &count, NULL,
> -                                              NULL);
> -            }
> +            ret = bdrv_block_status_above(src_bs, base, offset, count, 
> &count,
> +                                          NULL, NULL);
>   
>               if (ret < 0) {
>                   if (s->salvage) {
> @@ -2490,7 +2492,8 @@ static int img_convert(int argc, char **argv)
>            * s.target_backing_sectors has to be negative, which it will
>            * be automatically).  The backing file length is used only
>            * for optimizations, so such a case is not fatal. */
> -        s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs);
> +        s.target_backing_sectors =
> +            bdrv_nb_sectors(bdrv_filtered_cow_bs(out_bs));

bdrv_nb_sectors(bdrv_backing_chain_next(out_bs))

>       } else {
>           s.target_backing_sectors = -1;
>       }
> @@ -2853,6 +2856,7 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t offset,
>   
>       depth = 0;
>       for (;;) {
> +        bs = bdrv_skip_rw_filters(bs);
>           ret = bdrv_block_status(bs, offset, bytes, &bytes, &map, &file);
>           if (ret < 0) {
>               return ret;
> @@ -2861,7 +2865,7 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t offset,
>           if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
>               break;
>           }
> -        bs = backing_bs(bs);
> +        bs = bdrv_filtered_cow_bs(bs);
>           if (bs == NULL) {
>               ret = 0;
>               break;
> @@ -3216,6 +3220,7 @@ static int img_rebase(int argc, char **argv)
>       uint8_t *buf_old = NULL;
>       uint8_t *buf_new = NULL;
>       BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
> +    BlockDriverState *unfiltered_bs;
>       char *filename;
>       const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
>       int c, flags, src_flags, ret;
> @@ -3350,6 +3355,8 @@ static int img_rebase(int argc, char **argv)
>       }
>       bs = blk_bs(blk);
>   
> +    unfiltered_bs = bdrv_skip_rw_filters(bs);
> +
>       if (out_basefmt != NULL) {
>           if (bdrv_find_format(out_basefmt) == NULL) {
>               error_report("Invalid format name: '%s'", out_basefmt);
> @@ -3361,7 +3368,7 @@ static int img_rebase(int argc, char **argv)
>       /* For safe rebasing we need to compare old and new backing file */
>       if (!unsafe) {
>           QDict *options = NULL;
> -        BlockDriverState *base_bs = backing_bs(bs);
> +        BlockDriverState *base_bs = bdrv_filtered_cow_bs(unfiltered_bs);
>   
>           if (base_bs) {
>               blk_old_backing = blk_new(qemu_get_aio_context(),
> @@ -3517,7 +3524,7 @@ static int img_rebase(int argc, char **argv)
>                    * If cluster wasn't changed since prefix_chain, we don't 
> need
>                    * to take action
>                    */
> -                ret = bdrv_is_allocated_above(backing_bs(bs), 
> prefix_chain_bs,
> +                ret = bdrv_is_allocated_above(unfiltered_bs, prefix_chain_bs,

s/unfiltered_bs/bdrv_filtered_cow_bs(unfiltered_bs)/

>                                                 false, offset, n, &n);
>                   if (ret < 0) {
>                       error_report("error while reading image metadata: %s",
> diff --git a/tests/qemu-iotests/204.out b/tests/qemu-iotests/204.out
> index f3a10fbe90..684774d763 100644
> --- a/tests/qemu-iotests/204.out
> +++ b/tests/qemu-iotests/204.out
> @@ -59,5 +59,6 @@ Offset          Length          File
>   0x900000        0x2400000       TEST_DIR/t.IMGFMT
>   0x3c00000       0x1100000       TEST_DIR/t.IMGFMT
>   0x6a00000       0x400000        TEST_DIR/t.IMGFMT
> +0x6e00000       0x1200000       TEST_DIR/t.IMGFMT.base
>   No errors were found on the image.
>   *** done
> 

With two fixes:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir