[PATCH v2 0/3] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps

[PATCH v2 0/3] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps

[Eric Blake]

In v2:
- patch 1: clean up leftover file [Vladimir]
- patch 2: new, to fix badness with leaving corrupt destination on
  failed bitmap copy [Vladimir]
- patch 3: spell new option --skip-broken-bitmaps (note that --skip-broken
  as an abbreviation still works) [Vladimir]

Eric Blake (3):
  iotests: Improve and rename test 291 to qemu-img-bitmap
  qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
  qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

 docs/tools/qemu-img.rst                       |  8 ++-
 block/dirty-bitmap.c                          |  2 +-
 qemu-img.c                                    | 50 +++++++++++++--
 .../{291 => tests/qemu-img-bitmaps}           | 27 +++++++-
 .../{291.out => tests/qemu-img-bitmaps.out}   | 63 ++++++++++++++++++-
 5 files changed, 141 insertions(+), 9 deletions(-)
 rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (82%)
 rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (69%)

-- 
2.31.1

[PATCH v2 1/3] iotests: Improve and rename test 291 to qemu-img-bitmap

[Eric Blake]

Enhance the test to demonstrate existing less-than-stellar behavior of
qemu-img with a qcow2 image containing an inconsistent bitmap: we
don't diagnose the problem until after copying the entire image (a
potentially long time), and when we do diagnose the failure, we still
end up leaving an empty bitmap in the destination.  This mess will be
cleaned up in the next patch.

While at it, rename the test now that we support useful iotest names,
and fix a missing newline in the error message thus exposed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/dirty-bitmap.c                          |  2 +-
 .../{291 => tests/qemu-img-bitmaps}           | 19 +++++++-
 .../{291.out => tests/qemu-img-bitmaps.out}   | 48 ++++++++++++++++++-
 3 files changed, 66 insertions(+), 3 deletions(-)
 rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (88%)
 rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (75%)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 68d295d6e3ed..0ef46163e3ea 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -193,7 +193,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
         error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
                    bitmap->name);
         error_append_hint(errp, "Try block-dirty-bitmap-remove to delete"
-                          " this bitmap from disk");
+                          " this bitmap from disk\n");
         return -1;
     }

diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/tests/qemu-img-bitmaps
similarity index 88%
rename from tests/qemu-iotests/291
rename to tests/qemu-iotests/tests/qemu-img-bitmaps
index 20efb080a6c0..2f51651d0ce5 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
@@ -3,7 +3,7 @@
 #
 # Test qemu-img bitmap handling
 #
-# Copyright (C) 2018-2020 Red Hat, Inc.
+# Copyright (C) 2018-2021 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -27,11 +27,13 @@ status=1 # failure is the default!
 _cleanup()
 {
     _cleanup_test_img
+    _rm_test_img "$TEST_IMG.copy"
     nbd_server_stop
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15

 # get standard environment, filters and checks
+cd ..
 . ./common.rc
 . ./common.filter
 . ./common.nbd
@@ -129,6 +131,21 @@ $QEMU_IMG map --output=json --image-opts \

 nbd_server_stop

+echo
+echo "=== Check handling of inconsistent bitmap ==="
+echo
+
+$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null
+$QEMU_IMG bitmap --add "$TEST_IMG" b4
+$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
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
similarity index 75%
rename from tests/qemu-iotests/291.out
rename to tests/qemu-iotests/tests/qemu-img-bitmaps.out
index 23411c0ff4d9..b762362075d1 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
@@ -1,4 +1,4 @@
-QA output created by 291
+QA output created by qemu-img-bitmaps

 === Initial image setup ===

@@ -115,4 +115,50 @@ Format specific information:
 [{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
 { "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+
+=== Check handling of inconsistent bitmap ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+Format specific information:
+    bitmaps:
+        [0]:
+            flags:
+                [0]: in-use
+                [1]: auto
+            name: b2
+            granularity: 65536
+        [1]:
+            flags:
+                [0]: in-use
+            name: b0
+            granularity: 65536
+        [2]:
+            flags:
+                [0]: auto
+            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
 *** done
-- 
2.31.1

Re: [PATCH v2 1/3] iotests: Improve and rename test 291 to qemu-img-bitmap

[Vladimir Sementsov-Ogievskiy]

09.07.2021 18:39, Eric Blake wrote:
> Enhance the test to demonstrate existing less-than-stellar behavior of
> qemu-img with a qcow2 image containing an inconsistent bitmap: we
> don't diagnose the problem until after copying the entire image (a
> potentially long time), and when we do diagnose the failure, we still
> end up leaving an empty bitmap in the destination.  This mess will be
> cleaned up in the next patch.
> 
> While at it, rename the test now that we support useful iotest names,
> and fix a missing newline in the error message thus exposed.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH v2 1/3] iotests: Improve and rename test 291 to qemu-img-bitmap

[Nir Soffer]

On 7/9/21 6:39 PM, Eric Blake wrote:
> Enhance the test to demonstrate existing less-than-stellar behavior of
> qemu-img with a qcow2 image containing an inconsistent bitmap: we
> don't diagnose the problem until after copying the entire image (a
> potentially long time), and when we do diagnose the failure, we still
> end up leaving an empty bitmap in the destination.  This mess will be
> cleaned up in the next patch.
> 
> While at it, rename the test now that we support useful iotest names,
> and fix a missing newline in the error message thus exposed.

Much nicer with a meaningful name!

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/dirty-bitmap.c                          |  2 +-
>   .../{291 => tests/qemu-img-bitmaps}           | 19 +++++++-
>   .../{291.out => tests/qemu-img-bitmaps.out}   | 48 ++++++++++++++++++-
>   3 files changed, 66 insertions(+), 3 deletions(-)
>   rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (88%)
>   rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (75%)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 68d295d6e3ed..0ef46163e3ea 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -193,7 +193,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>           error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>                      bitmap->name);
>           error_append_hint(errp, "Try block-dirty-bitmap-remove to delete"
> -                          " this bitmap from disk");
> +                          " this bitmap from disk\n");
>           return -1;
>       }
> 
> diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/tests/qemu-img-bitmaps
> similarity index 88%
> rename from tests/qemu-iotests/291
> rename to tests/qemu-iotests/tests/qemu-img-bitmaps
> index 20efb080a6c0..2f51651d0ce5 100755
> --- a/tests/qemu-iotests/291
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> @@ -3,7 +3,7 @@
>   #
>   # Test qemu-img bitmap handling
>   #
> -# Copyright (C) 2018-2020 Red Hat, Inc.
> +# Copyright (C) 2018-2021 Red Hat, Inc.
>   #
>   # This program is free software; you can redistribute it and/or modify
>   # it under the terms of the GNU General Public License as published by
> @@ -27,11 +27,13 @@ status=1 # failure is the default!
>   _cleanup()
>   {
>       _cleanup_test_img
> +    _rm_test_img "$TEST_IMG.copy"
>       nbd_server_stop
>   }
>   trap "_cleanup; exit \$status" 0 1 2 3 15
> 
>   # get standard environment, filters and checks
> +cd ..
>   . ./common.rc
>   . ./common.filter
>   . ./common.nbd
> @@ -129,6 +131,21 @@ $QEMU_IMG map --output=json --image-opts \
> 
>   nbd_server_stop
> 
> +echo
> +echo "=== Check handling of inconsistent bitmap ==="
> +echo
> +
> +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null
> +$QEMU_IMG bitmap --add "$TEST_IMG" b4
> +$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
> +
>   # success, all done
>   echo '*** done'
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> similarity index 75%
> rename from tests/qemu-iotests/291.out
> rename to tests/qemu-iotests/tests/qemu-img-bitmaps.out
> index 23411c0ff4d9..b762362075d1 100644
> --- a/tests/qemu-iotests/291.out
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> @@ -1,4 +1,4 @@
> -QA output created by 291
> +QA output created by qemu-img-bitmaps
> 
>   === Initial image setup ===
> 
> @@ -115,4 +115,50 @@ Format specific information:
>   [{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>   { "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
>   { "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
> +
> +=== Check handling of inconsistent bitmap ===
> +
> +image: TEST_DIR/t.IMGFMT
> +file format: IMGFMT
> +virtual size: 10 MiB (10485760 bytes)
> +cluster_size: 65536
> +backing file: TEST_DIR/t.IMGFMT.base
> +backing file format: IMGFMT
> +Format specific information:
> +    bitmaps:
> +        [0]:
> +            flags:
> +                [0]: in-use
> +                [1]: auto
> +            name: b2
> +            granularity: 65536
> +        [1]:
> +            flags:
> +                [0]: in-use
> +            name: b0
> +            granularity: 65536
> +        [2]:
> +            flags:
> +                [0]: auto
> +            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

In this context a more useful error message would be:

     Try "qemu-img bitmap --remove" ...

but this is not a new issue.

> +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
>   *** done
> 

Reviewed-by: Nir Soffer <nsoffer@redhat.com>

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

[Eric Blake]

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>
---
 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)) {
+            continue;
+        }
+        name = bdrv_dirty_bitmap_name(bm);
+        if (bdrv_dirty_bitmap_inconsistent(bm)) {
+            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);
             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
-- 
2.31.1

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

[Nir Soffer]

On 7/9/21 6:39 PM, 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.

I don't think this is an issue since qemu-img terminate with non-zero
exit code, and we cannot ensure that image is complete if we fail in
the middle of the operation for all image formats and protocols.

For files we could use a temporary file and rename after successful
conversion for for raw format on block device we don't have any way
to mark the contents as temporary.

But failing fast is very important.

> This fixes the problems exposed in the previous patch to the iotest.
> 
> Signed-off-by: Eric Blake <eblake@redhat.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)) {
> +            continue;
> +        }
> +        name = bdrv_dirty_bitmap_name(bm);
> +        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> +            error_report("Cannot copy inconsistent bitmap '%s'", name);

We can add a useful hint:

     Try "qemu-img bitmap --remove" to delete this bitmap from disk.

> +            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);

This may fail for the same reason populate failed (e.g. storage became 
inaccessibel in the middle of the copy). Since we fail the convert, I 
don't think it worth to try to do this kind of cleanup.

If we have a way to disable the bitmap before merge, and enable it after
successful merge it make more sense, since if the operation fails we are
left with disabled bitmap.

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

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

[Eric Blake]

On Sat, Jul 10, 2021 at 09:06:24PM +0300, Nir Soffer wrote:
> On 7/9/21 6:39 PM, 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.
> 
> I don't think this is an issue since qemu-img terminate with non-zero
> exit code, and we cannot ensure that image is complete if we fail in
> the middle of the operation for all image formats and protocols.
> 
> For files we could use a temporary file and rename after successful
> conversion for for raw format on block device we don't have any way
> to mark the contents as temporary.

Atomic rename into place for files is nice, but as you point out, it
doesn't help when targetting block devices.  So whatever we do to keep
block devices robust even across temporary state changes is also
sufficient for files, even if we can indeed improve the situation for
files in a later patch.

> 
> But failing fast is very important.
> 
> > This fixes the problems exposed in the previous patch to the iotest.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.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)) {
> > +            continue;
> > +        }
> > +        name = bdrv_dirty_bitmap_name(bm);
> > +        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > +            error_report("Cannot copy inconsistent bitmap '%s'", name);
> 
> We can add a useful hint:
> 
>     Try "qemu-img bitmap --remove" to delete this bitmap from disk.

Yeah, that might be worthwhile.

> 
> > +            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);
> 
> This may fail for the same reason populate failed (e.g. storage became
> inaccessibel in the middle of the copy). Since we fail the convert, I don't
> think it worth to try to do this kind of cleanup.
> 
> If we have a way to disable the bitmap before merge, and enable it after
> successful merge it make more sense, since if the operation fails we are
> left with disabled bitmap.

If we got this far, the guest-visible data WAS copied successfully.
'qemu-img compare' will report success.  The only thing broken at this
point is a bogus bitmap, and leaving a just-created (but empty) bitmap
in place rather than erasing it (since we just created it a few lines
above) is not nice.  I see no problem with keeping this cleanup path
intact, even if it is seldom reached, and even though we still exit
the overall qemu-img convert with an error.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Nir Soffer]

On Tue, Jul 13, 2021 at 8:48 PM Eric Blake <eblake@redhat.com> wrote:
>
> On Sat, Jul 10, 2021 at 09:06:24PM +0300, Nir Soffer wrote:
> > On 7/9/21 6:39 PM, 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.
> >
> > I don't think this is an issue since qemu-img terminate with non-zero
> > exit code, and we cannot ensure that image is complete if we fail in
> > the middle of the operation for all image formats and protocols.
> >
> > For files we could use a temporary file and rename after successful
> > conversion for for raw format on block device we don't have any way
> > to mark the contents as temporary.
>
> Atomic rename into place for files is nice, but as you point out, it
> doesn't help when targetting block devices.  So whatever we do to keep
> block devices robust even across temporary state changes is also
> sufficient for files, even if we can indeed improve the situation for
> files in a later patch.

I think management tools should handle this. In oVirt we keep metadata
and cluster locks for any kind of volume and we use them to mark volumes
being copied as temporary, so from our point of view proper cleanup in
failure flows is non-issue.

> > But failing fast is very important.
> >
> > > This fixes the problems exposed in the previous patch to the iotest.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.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)) {
> > > +            continue;
> > > +        }
> > > +        name = bdrv_dirty_bitmap_name(bm);
> > > +        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > > +            error_report("Cannot copy inconsistent bitmap '%s'", name);
> >
> > We can add a useful hint:
> >
> >     Try "qemu-img bitmap --remove" to delete this bitmap from disk.
>
> Yeah, that might be worthwhile.
>
> >
> > > +            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);
> >
> > This may fail for the same reason populate failed (e.g. storage became
> > inaccessibel in the middle of the copy). Since we fail the convert, I don't
> > think it worth to try to do this kind of cleanup.
> >
> > If we have a way to disable the bitmap before merge, and enable it after
> > successful merge it make more sense, since if the operation fails we are
> > left with disabled bitmap.
>
> If we got this far, the guest-visible data WAS copied successfully.
> 'qemu-img compare' will report success.  The only thing broken at this
> point is a bogus bitmap, and leaving a just-created (but empty) bitmap
> in place rather than erasing it (since we just created it a few lines
> above) is not nice.  I see no problem with keeping this cleanup path
> intact, even if it is seldom reached, and even though we still exit
> the overall qemu-img convert with an error.

Sure, no reason to delay this fix. With or without hint on errors,

Reviewed-by: Nir Soffer <nsoffer@redhat.com>

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

[PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Eric Blake]

The point of 'qemu-img convert --bitmaps' is to be a convenience for
actions that are already possible through a string of smaller
'qemu-img bitmap' sub-commands.  One situation not accounted for
already is that if a source image contains an inconsistent bitmap (for
example, because a qemu process died abruptly before flushing bitmap
state), the user MUST delete those inconsistent bitmaps before
anything else useful can be done with the image.

We don't want to delete inconsistent bitmaps by default: although a
corrupt bitmap is only a loss of optimization rather than a corruption
of user-visible data, it is still nice to require the user to opt in
to the fact that they are aware of the loss of the bitmap.  Still,
requiring the user to check 'qemu-img info' to see whether bitmaps are
consistent, then use 'qemu-img bitmap --remove' to remove offenders,
all before using 'qemu-img convert', is a lot more work than just
adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which
opts in to skipping the broken bitmaps.

After testing the new option, also demonstrate the way to manually fix
things (either deleting bad bitmaps, or re-creating them as empty) so
that it is possible to convert without the option.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-img.rst                       |  8 ++++-
 qemu-img.c                                    | 26 +++++++++++++---
 tests/qemu-iotests/tests/qemu-img-bitmaps     | 10 ++++++
 tests/qemu-iotests/tests/qemu-img-bitmaps.out | 31 +++++++++++++++++++
 4 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index cfe11478791f..4d407b180450 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -414,7 +414,7 @@ Command description:
   4
     Error on reading data

-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

   Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
   to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
@@ -456,6 +456,12 @@ Command description:
   *NUM_COROUTINES* specifies how many coroutines work in parallel during
   the convert process (defaults to 8).

+  Use of ``--bitmaps`` requests that any persistent bitmaps present in
+  the original are also copied to the destination.  If any bitmap is
+  inconsistent in the source, the conversion will fail unless
+  ``--skip-broken-bitmaps`` is also specified to copy only the
+  consistent bitmaps.
+
 .. option:: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE] [-F BACKING_FMT] [-u] [-o OPTIONS] FILENAME [SIZE]

   Create the new disk image *FILENAME* of size *SIZE* and format
diff --git a/qemu-img.c b/qemu-img.c
index e84b3c530155..661538edd785 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -82,6 +82,7 @@ enum {
     OPTION_MERGE = 274,
     OPTION_BITMAPS = 275,
     OPTION_FORCE = 276,
+    OPTION_SKIP_BROKEN = 277,
 };

 typedef enum OutputFormat {
@@ -2102,7 +2103,7 @@ static int convert_do_copy(ImgConvertState *s)
 }

 /* Check that bitmaps can be copied, or output an error */
-static int convert_check_bitmaps(BlockDriverState *src)
+static int convert_check_bitmaps(BlockDriverState *src, bool skip_broken)
 {
     BdrvDirtyBitmap *bm;

@@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src)
             continue;
         }
         name = bdrv_dirty_bitmap_name(bm);
-        if (bdrv_dirty_bitmap_inconsistent(bm)) {
+        if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
             error_report("Cannot copy inconsistent bitmap '%s'", name);
             return -1;
         }
@@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src)
     return 0;
 }

-static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
+static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst,
+                                bool skip_broken)
 {
     BdrvDirtyBitmap *bm;
     Error *err = NULL;
@@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
             continue;
         }
         name = bdrv_dirty_bitmap_name(bm);
+        if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
+            warn_report("Skipping inconsistent bitmap %s", name);
+            continue;
+        }
         qmp_block_dirty_bitmap_add(dst->node_name, name,
                                    true, bdrv_dirty_bitmap_granularity(bm),
                                    true, true,
@@ -2192,6 +2198,7 @@ static int img_convert(int argc, char **argv)
     bool force_share = false;
     bool explict_min_sparse = false;
     bool bitmaps = false;
+    bool skip_broken = false;
     int64_t rate_limit = 0;

     ImgConvertState s = (ImgConvertState) {
@@ -2213,6 +2220,7 @@ static int img_convert(int argc, char **argv)
             {"salvage", no_argument, 0, OPTION_SALVAGE},
             {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
             {"bitmaps", no_argument, 0, OPTION_BITMAPS},
+            {"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WUr:",
@@ -2341,6 +2349,9 @@ static int img_convert(int argc, char **argv)
         case OPTION_BITMAPS:
             bitmaps = true;
             break;
+        case OPTION_SKIP_BROKEN:
+            skip_broken = true;
+            break;
         }
     }

@@ -2348,6 +2359,11 @@ static int img_convert(int argc, char **argv)
         out_fmt = "raw";
     }

+    if (skip_broken && !bitmaps) {
+        error_report("Use of --skip-broken-bitmaps requires --bitmaps");
+        goto fail_getopt;
+    }
+
     if (s.compressed && s.copy_range) {
         error_report("Cannot enable copy offloading when -c is used");
         goto fail_getopt;
@@ -2577,7 +2593,7 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        ret = convert_check_bitmaps(blk_bs(s.src[0]));
+        ret = convert_check_bitmaps(blk_bs(s.src[0]), skip_broken);
         if (ret < 0) {
             goto out;
         }
@@ -2702,7 +2718,7 @@ static int img_convert(int argc, char **argv)

     /* Now copy the bitmaps */
     if (bitmaps && ret == 0) {
-        ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
+        ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs, skip_broken);
     }

 out:
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps b/tests/qemu-iotests/tests/qemu-img-bitmaps
index 3fde95907515..20f3dffa8e5e 100755
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
@@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
     echo "unexpected success"
 TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
     | _filter_irrelevant_img_info
+$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \
+    -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"
+TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
+    | _filter_irrelevant_img_info
+_rm_test_img "$TEST_IMG.copy"
+$QEMU_IMG bitmap --remove "$TEST_IMG" b0
+$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2
+$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"
+TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
+    | _filter_irrelevant_img_info

 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
index 546aaa404bba..4d196e24d0fb 100644
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
@@ -145,4 +145,35 @@ Format specific information:
     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
+qemu-img: warning: Skipping inconsistent bitmap b0
+qemu-img: warning: Skipping inconsistent bitmap b2
+image: TEST_DIR/t.IMGFMT.copy
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+Format specific information:
+    bitmaps:
+        [0]:
+            flags:
+                [0]: auto
+            name: b4
+            granularity: 65536
+    corrupt: false
+image: TEST_DIR/t.IMGFMT.copy
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+Format specific information:
+    bitmaps:
+        [0]:
+            flags:
+                [0]: auto
+            name: b4
+            granularity: 65536
+        [1]:
+            flags:
+                [0]: auto
+            name: b2
+            granularity: 65536
+    corrupt: false
 *** done
-- 
2.31.1

Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Nir Soffer]

On 7/9/21 6:39 PM, Eric Blake wrote:
> The point of 'qemu-img convert --bitmaps' is to be a convenience for
> actions that are already possible through a string of smaller
> 'qemu-img bitmap' sub-commands.  One situation not accounted for
> already is that if a source image contains an inconsistent bitmap (for
> example, because a qemu process died abruptly before flushing bitmap
> state), the user MUST delete those inconsistent bitmaps before
> anything else useful can be done with the image.

The only thing affected by inconsistent bitmap is creating incremental 
backup, and taking some space on storage. Anything else should not be
affected by having such bitmap so the user does not need to remove it.

In oVirt we don't check or repair images after unclean guest shutdown.
Maybe this is a good idea for future version. Inconsistent bitmaps are 
removed only when the user ask to remove the related checkpoint.

> We don't want to delete inconsistent bitmaps by default: although a
> corrupt bitmap is only a loss of optimization rather than a corruption
> of user-visible data, it is still nice to require the user to opt in
> to the fact that they are aware of the loss of the bitmap.  Still,
> requiring the user to check 'qemu-img info' to see whether bitmaps are
> consistent, then use 'qemu-img bitmap --remove' to remove offenders,
> all before using 'qemu-img convert', is a lot more work than just
> adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which
> opts in to skipping the broken bitmaps.

I think this is more than convenience. During live storage migration in
oVirt, we mirror the top layer to the destination using libvirt 
blockCopy, and copy the rest of the chain using qemu-img convert with 
the --bitmaps option.

If we have to remove inconsistent bitmaps at this point we need to 
modify images opened for reading by qemu, which is likely not possible 
and even if it is possible, sounds like a bad idea.

> 
> After testing the new option, also demonstrate the way to manually fix
> things (either deleting bad bitmaps, or re-creating them as empty) so
> that it is possible to convert without the option.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/tools/qemu-img.rst                       |  8 ++++-
>   qemu-img.c                                    | 26 +++++++++++++---
>   tests/qemu-iotests/tests/qemu-img-bitmaps     | 10 ++++++
>   tests/qemu-iotests/tests/qemu-img-bitmaps.out | 31 +++++++++++++++++++
>   4 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index cfe11478791f..4d407b180450 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -414,7 +414,7 @@ Command description:
>     4
>       Error on reading data
> 
> -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME

I liked --skip-broken more, but Vladimir is right that this is not 
really a sub-option.

> 
>     Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
>     to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
> @@ -456,6 +456,12 @@ Command description:
>     *NUM_COROUTINES* specifies how many coroutines work in parallel during
>     the convert process (defaults to 8).
> 
> +  Use of ``--bitmaps`` requests that any persistent bitmaps present in
> +  the original are also copied to the destination.  If any bitmap is
> +  inconsistent in the source, the conversion will fail unless
> +  ``--skip-broken-bitmaps`` is also specified to copy only the
> +  consistent bitmaps.
> +
>   .. option:: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE] [-F BACKING_FMT] [-u] [-o OPTIONS] FILENAME [SIZE]
> 
>     Create the new disk image *FILENAME* of size *SIZE* and format
> diff --git a/qemu-img.c b/qemu-img.c
> index e84b3c530155..661538edd785 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -82,6 +82,7 @@ enum {
>       OPTION_MERGE = 274,
>       OPTION_BITMAPS = 275,
>       OPTION_FORCE = 276,
> +    OPTION_SKIP_BROKEN = 277,
>   };
> 
>   typedef enum OutputFormat {
> @@ -2102,7 +2103,7 @@ static int convert_do_copy(ImgConvertState *s)
>   }
> 
>   /* Check that bitmaps can be copied, or output an error */
> -static int convert_check_bitmaps(BlockDriverState *src)
> +static int convert_check_bitmaps(BlockDriverState *src, bool skip_broken)
>   {
>       BdrvDirtyBitmap *bm;
> 
> @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src)
>               continue;
>           }
>           name = bdrv_dirty_bitmap_name(bm);
> -        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> +        if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
>               error_report("Cannot copy inconsistent bitmap '%s'", name);

We can add another hint:

     Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap
     --remove" to delete it from disk.

>               return -1;
>           }
> @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src)
>       return 0;
>   }
> 
> -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst,
> +                                bool skip_broken)
>   {
>       BdrvDirtyBitmap *bm;
>       Error *err = NULL;
> @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
>               continue;
>           }
>           name = bdrv_dirty_bitmap_name(bm);
> +        if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
> +            warn_report("Skipping inconsistent bitmap %s", name);

In other logs we quote the bitmap name:'%s'

> +            continue;
> +        }
>           qmp_block_dirty_bitmap_add(dst->node_name, name,
>                                      true, bdrv_dirty_bitmap_granularity(bm),
>                                      true, true,
> @@ -2192,6 +2198,7 @@ static int img_convert(int argc, char **argv)
>       bool force_share = false;
>       bool explict_min_sparse = false;
>       bool bitmaps = false;
> +    bool skip_broken = false;
>       int64_t rate_limit = 0;
> 
>       ImgConvertState s = (ImgConvertState) {
> @@ -2213,6 +2220,7 @@ static int img_convert(int argc, char **argv)
>               {"salvage", no_argument, 0, OPTION_SALVAGE},
>               {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>               {"bitmaps", no_argument, 0, OPTION_BITMAPS},
> +            {"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN},
>               {0, 0, 0, 0}
>           };
>           c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WUr:",
> @@ -2341,6 +2349,9 @@ static int img_convert(int argc, char **argv)
>           case OPTION_BITMAPS:
>               bitmaps = true;
>               break;
> +        case OPTION_SKIP_BROKEN:
> +            skip_broken = true;
> +            break;
>           }
>       }
> 
> @@ -2348,6 +2359,11 @@ static int img_convert(int argc, char **argv)
>           out_fmt = "raw";
>       }
> 
> +    if (skip_broken && !bitmaps) {
> +        error_report("Use of --skip-broken-bitmaps requires --bitmaps");
> +        goto fail_getopt;
> +    }
> +
>       if (s.compressed && s.copy_range) {
>           error_report("Cannot enable copy offloading when -c is used");
>           goto fail_getopt;
> @@ -2577,7 +2593,7 @@ static int img_convert(int argc, char **argv)
>               ret = -1;
>               goto out;
>           }
> -        ret = convert_check_bitmaps(blk_bs(s.src[0]));
> +        ret = convert_check_bitmaps(blk_bs(s.src[0]), skip_broken);
>           if (ret < 0) {
>               goto out;
>           }
> @@ -2702,7 +2718,7 @@ static int img_convert(int argc, char **argv)
> 
>       /* Now copy the bitmaps */
>       if (bitmaps && ret == 0) {
> -        ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
> +        ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs, skip_broken);
>       }
> 
>   out:
> diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps b/tests/qemu-iotests/tests/qemu-img-bitmaps
> index 3fde95907515..20f3dffa8e5e 100755
> --- a/tests/qemu-iotests/tests/qemu-img-bitmaps
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
>       echo "unexpected success"
>   TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
>       | _filter_irrelevant_img_info

A new title here will make the test output much more clear.

> +$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \
> +    -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"
> +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> +    | _filter_irrelevant_img_info

New title will make both the test and output more clear. Maybe also 
using more descriptive names for the bitmaps (e.g bad, good).

> +_rm_test_img "$TEST_IMG.copy"
> +$QEMU_IMG bitmap --remove "$TEST_IMG" b0
> +$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2
> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"
> +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> +    | _filter_irrelevant_img_info
> 
>   # success, all done
>   echo '*** done'
> diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> index 546aaa404bba..4d196e24d0fb 100644
> --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> @@ -145,4 +145,35 @@ Format specific information:
>       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

Why to we get this error? I guess it is part of the first copy that 
should fail?

> +qemu-img: warning: Skipping inconsistent bitmap b0
> +qemu-img: warning: Skipping inconsistent bitmap b2

Looks useful, I need to check that we log such warnings.

> +image: TEST_DIR/t.IMGFMT.copy
> +file format: IMGFMT
> +virtual size: 10 MiB (10485760 bytes)
> +cluster_size: 65536
> +Format specific information:
> +    bitmaps:
> +        [0]:
> +            flags:
> +                [0]: auto
> +            name: b4
> +            granularity: 65536
> +    corrupt: false
> +image: TEST_DIR/t.IMGFMT.copy
> +file format: IMGFMT
> +virtual size: 10 MiB (10485760 bytes)
> +cluster_size: 65536
> +Format specific information:
> +    bitmaps:
> +        [0]:
> +            flags:
> +                [0]: auto
> +            name: b4
> +            granularity: 65536
> +        [1]:
> +            flags:
> +                [0]: auto
> +            name: b2
> +            granularity: 65536
> +    corrupt: false
>   *** done
> 

Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Eric Blake]

On Sat, Jul 10, 2021 at 09:37:35PM +0300, Nir Soffer wrote:
> > We don't want to delete inconsistent bitmaps by default: although a
> > corrupt bitmap is only a loss of optimization rather than a corruption
> > of user-visible data, it is still nice to require the user to opt in
> > to the fact that they are aware of the loss of the bitmap.  Still,
> > requiring the user to check 'qemu-img info' to see whether bitmaps are
> > consistent, then use 'qemu-img bitmap --remove' to remove offenders,
> > all before using 'qemu-img convert', is a lot more work than just
> > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which
> > opts in to skipping the broken bitmaps.
> 
> I think this is more than convenience. During live storage migration in
> oVirt, we mirror the top layer to the destination using libvirt blockCopy,
> and copy the rest of the chain using qemu-img convert with the --bitmaps
> option.

Still, this feels like enough of a feature that I'd really like R-b in
time to prepare a pull request for inclusion in soft freeze; the
justification for it being a bug fix is a tough sell.

> > +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> 
> I liked --skip-broken more, but Vladimir is right that this is not really a
> sub-option.

getopt_long() lets you abbreviate; '--sk' and '--skip-broken' are both
unambiguous prefixes of '--skip-broken-bitmaps'.

> > @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src)
> >               continue;
> >           }
> >           name = bdrv_dirty_bitmap_name(bm);
> > -        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > +        if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
> >               error_report("Cannot copy inconsistent bitmap '%s'", name);
> 
> We can add another hint:
> 
>     Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap
>     --remove" to delete it from disk.

Sure, I can see about adding that.


> 
> >               return -1;
> >           }
> > @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src)
> >       return 0;
> >   }
> > 
> > -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> > +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst,
> > +                                bool skip_broken)
> >   {
> >       BdrvDirtyBitmap *bm;
> >       Error *err = NULL;
> > @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> >               continue;
> >           }
> >           name = bdrv_dirty_bitmap_name(bm);
> > +        if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
> > +            warn_report("Skipping inconsistent bitmap %s", name);
> 
> In other logs we quote the bitmap name:'%s'

Yes, will fix.

> > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
> >       echo "unexpected success"
> >   TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> >       | _filter_irrelevant_img_info
> 
> A new title here will make the test output much more clear.

Or even just a bare 'echo' to separate things with blank lines.  Will
improve.

> > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> > @@ -145,4 +145,35 @@ Format specific information:
> >       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
> 
> Why to we get this error? I guess it is part of the first copy that should
> fail?

Yes - proof that we no longer leave a broken file around, but instead
failed fast (in fact, that's part of the previous patch).

> 
> > +qemu-img: warning: Skipping inconsistent bitmap b0
> > +qemu-img: warning: Skipping inconsistent bitmap b2
> 
> Looks useful, I need to check that we log such warnings.
>

Anything else I should improve before sending a v2?


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Nir Soffer]

On Tue, Jul 13, 2021 at 8:53 PM Eric Blake <eblake@redhat.com> wrote:
>
> On Sat, Jul 10, 2021 at 09:37:35PM +0300, Nir Soffer wrote:
> > > We don't want to delete inconsistent bitmaps by default: although a
> > > corrupt bitmap is only a loss of optimization rather than a corruption
> > > of user-visible data, it is still nice to require the user to opt in
> > > to the fact that they are aware of the loss of the bitmap.  Still,
> > > requiring the user to check 'qemu-img info' to see whether bitmaps are
> > > consistent, then use 'qemu-img bitmap --remove' to remove offenders,
> > > all before using 'qemu-img convert', is a lot more work than just
> > > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which
> > > opts in to skipping the broken bitmaps.
> >
> > I think this is more than convenience. During live storage migration in
> > oVirt, we mirror the top layer to the destination using libvirt blockCopy,
> > and copy the rest of the chain using qemu-img convert with the --bitmaps
> > option.
>
> Still, this feels like enough of a feature that I'd really like R-b in
> time to prepare a pull request for inclusion in soft freeze; the
> justification for it being a bug fix is a tough sell.

This is not a bug in the current code, more like missing handling
of important use case. Without this we cannot copy images in
some cases, or we must require downtime to check and repair
images before copying disks.

> > > +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> >
> > I liked --skip-broken more, but Vladimir is right that this is not really a
> > sub-option.
>
> getopt_long() lets you abbreviate; '--sk' and '--skip-broken' are both
> unambiguous prefixes of '--skip-broken-bitmaps'.

Nice to learn that

> > > @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src)
> > >               continue;
> > >           }
> > >           name = bdrv_dirty_bitmap_name(bm);
> > > -        if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > > +        if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
> > >               error_report("Cannot copy inconsistent bitmap '%s'", name);
> >
> > We can add another hint:
> >
> >     Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap
> >     --remove" to delete it from disk.
>
> Sure, I can see about adding that.
>
>
> >
> > >               return -1;
> > >           }
> > > @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src)
> > >       return 0;
> > >   }
> > >
> > > -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> > > +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst,
> > > +                                bool skip_broken)
> > >   {
> > >       BdrvDirtyBitmap *bm;
> > >       Error *err = NULL;
> > > @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> > >               continue;
> > >           }
> > >           name = bdrv_dirty_bitmap_name(bm);
> > > +        if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) {
> > > +            warn_report("Skipping inconsistent bitmap %s", name);
> >
> > In other logs we quote the bitmap name:'%s'
>
> Yes, will fix.
>
> > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> > > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
> > >       echo "unexpected success"
> > >   TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> > >       | _filter_irrelevant_img_info
> >
> > A new title here will make the test output much more clear.
>
> Or even just a bare 'echo' to separate things with blank lines.  Will
> improve.
>
> > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
> > > @@ -145,4 +145,35 @@ Format specific information:
> > >       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
> >
> > Why to we get this error? I guess it is part of the first copy that should
> > fail?
>
> Yes - proof that we no longer leave a broken file around, but instead
> failed fast (in fact, that's part of the previous patch).
>
> >
> > > +qemu-img: warning: Skipping inconsistent bitmap b0
> > > +qemu-img: warning: Skipping inconsistent bitmap b2
> >
> > Looks useful, I need to check that we log such warnings.
> >
>
> Anything else I should improve before sending a v2?

I think we covered everything, but Vladimir may want to comment.

Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Vladimir Sementsov-Ogievskiy]

09.07.2021 18:39, Eric Blake wrote:
> The point of 'qemu-img convert --bitmaps' is to be a convenience for
> actions that are already possible through a string of smaller
> 'qemu-img bitmap' sub-commands.  One situation not accounted for
> already is that if a source image contains an inconsistent bitmap (for
> example, because a qemu process died abruptly before flushing bitmap
> state), the user MUST delete those inconsistent bitmaps before
> anything else useful can be done with the image.
> 
> We don't want to delete inconsistent bitmaps by default: although a
> corrupt bitmap is only a loss of optimization rather than a corruption
> of user-visible data, it is still nice to require the user to opt in
> to the fact that they are aware of the loss of the bitmap.  Still,
> requiring the user to check 'qemu-img info' to see whether bitmaps are
> consistent, then use 'qemu-img bitmap --remove' to remove offenders,
> all before using 'qemu-img convert', is a lot more work than just
> adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which
> opts in to skipping the broken bitmaps.
> 
> After testing the new option, also demonstrate the way to manually fix
> things (either deleting bad bitmaps, or re-creating them as empty) so
> that it is possible to convert without the option.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084
> Signed-off-by: Eric Blake <eblake@redhat.com>

[..]

> --- a/tests/qemu-iotests/tests/qemu-img-bitmaps
> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
> @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
>       echo "unexpected success"
>   TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
>       | _filter_irrelevant_img_info
> +$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \
> +    -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"

Nitpicking: sometimes you quote filename variables, sometimes not..

> +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> +    | _filter_irrelevant_img_info
> +_rm_test_img "$TEST_IMG.copy"
> +$QEMU_IMG bitmap --remove "$TEST_IMG" b0
> +$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2
> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy"
> +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
> +    | _filter_irrelevant_img_info
> 

With or without adjustments you discussed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir