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 <ebl...@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