Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap

[Vladimir Sementsov-Ogievskiy]

09.07.2021 18:39, Eric Blake wrote:
> Waiting until the end of the convert operation (a potentially
> time-consuming task) to finally detect that we can't copy a bitmap is
> bad, comparing to failing fast up front.  Furthermore, this prevents
> us from leaving a file behind with a bitmap that is not marked as
> inconsistent even though it does not have sane contents.
>
> This fixes the problems exposed in the previous patch to the iotest.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

I'm OK as is (still some notes below):

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> ---
>   qemu-img.c                                    | 30 +++++++++++++++++--
>   tests/qemu-iotests/tests/qemu-img-bitmaps     |  2 --
>   tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++-----------
>   3 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 7956a8996512..e84b3c530155 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
>       return s->ret;
>   }
>
> +/* Check that bitmaps can be copied, or output an error */
> +static int convert_check_bitmaps(BlockDriverState *src)
> +{
> +    BdrvDirtyBitmap *bm;
> +
> +    if (!bdrv_supports_persistent_dirty_bitmap(src)) {
> +        error_report("Source lacks bitmap support");
> +        return -1;
> +    }
> +    FOR_EACH_DIRTY_BITMAP(src, bm) {
> +        const char *name;
> +
> +        if (!bdrv_dirty_bitmap_get_persistence(bm)) {

hmm it should be impossible in context of qemu-img... Still, not sure that we 
need an assertion instead.. Who knows, may be we'll use some intermediate 
non-persistent bitmap during convert.

> +            continue;
> +        }
> +        name = bdrv_dirty_bitmap_name(bm);

Nitpicking: a bit strange that you care to not initialize name before previous 
check (as it's not needed), but initialize here, before next check, when name 
is needed only if bitmap is inconsistent.

> +        if (bdrv_dirty_bitmap_inconsistent(bm)) {

You can define and intialize name here.. Or inside error_report(..);

> +            error_report("Cannot copy inconsistent bitmap '%s'", name);
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>   static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
>   {
>       BdrvDirtyBitmap *bm;
> @@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState *src, 
> BlockDriverState *dst)
>                                 &err);
>           if (err) {
>               error_reportf_err(err, "Failed to populate bitmap %s: ", name);
> +            qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);

Good change, but not covered by commit message

>               return -1;
>           }
>       }
> @@ -2552,9 +2577,8 @@ static int img_convert(int argc, char **argv)
>               ret = -1;
>               goto out;
>           }
> -        if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
> -            error_report("Source lacks bitmap support");
> -            ret = -1;
> +        ret = convert_check_bitmaps(blk_bs(s.src[0]));
> +        if (ret < 0) {
>               goto out;
>           }
>       }
> diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps 
> b/tests/qemu-iotests/tests/qemu-img-bitmaps
> index 2f51651d0ce5..3fde95907515 100755
> --- a/tests/qemu-iotests/tests/qemu-img-bitmaps
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> @@ -141,8 +141,6 @@ $QEMU_IMG bitmap --remove "$TEST_IMG" b1
>   _img_info --format-specific | _filter_irrelevant_img_info
>   $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
>       echo "unexpected success"
> -# Bug - even though we failed at conversion, we left a file around with
> -# a bitmap marked as not corrupt
>   TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
>       | _filter_irrelevant_img_info
>
> diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out 
> b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> index b762362075d1..546aaa404bba 100644
> --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> @@ -143,22 +143,6 @@ Format specific information:
>               name: b4
>               granularity: 65536
>       corrupt: false
> -qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot 
> be used
> -Try block-dirty-bitmap-remove to delete this bitmap from disk
> -image: TEST_DIR/t.IMGFMT.copy
> -file format: IMGFMT
> -virtual size: 10 MiB (10485760 bytes)
> -cluster_size: 65536
> -Format specific information:
> -    bitmaps:
> -        [0]:
> -            flags:
> -            name: b0
> -            granularity: 65536
> -        [1]:
> -            flags:
> -                [0]: auto
> -            name: b4
> -            granularity: 65536
> -    corrupt: false
> +qemu-img: Cannot copy inconsistent bitmap 'b0'
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open 
> 'TEST_DIR/t.IMGFMT.copy': No such file or directory
>   *** done


--
Best regards,
Vladimir