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