Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Kevin Wolf]

Am 20.07.2020 um 16:47 hat Nir Soffer geschrieben:
> On Mon, Jul 20, 2020 at 4:18 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/122.out | 17 +++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> > index dfd1cd05d6..1112fc0730 100755
> > --- a/tests/qemu-iotests/122
> > +++ b/tests/qemu-iotests/122
> > @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" 
> > "$TEST_IMG".orig
> >
> >  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
> >
> > +echo
> > +echo '=== -n to an empty image ==='
> > +echo
> > +
> > +_make_test_img 64M
> 
> Why make a test image here? We create it again below twice

This is a different image because the invocations below change the
TEST_IMG variable.

> > +
> > +# Convert with -n, which should not result in a fully allocated image, not 
> > even
> > +# with compat=0.10 (because the target doesn't have a backing file)
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> This looks reversed - "$TEST_IMG".orig is the original image, and
> "$TEST_IMG" is the target image. So maybe use "$TEST_IMG".target?

I'll use .orig for the source and without a suffix for the target (which
are filenames that _cleanup_test_img covers automatically).

> > +
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> Since the only difference is the compat, why not use a loop?
> 
> for compat in 0.10 1.1; do
> ...

Makes sense.

> > +
> > +echo
> > +echo '=== -n to an empty image with a backing file ==='
> > +echo
> > +
> > +_make_test_img 64M
> > +TEST_IMG="$TEST_IMG".base _make_test_img 64M
> > +
> > +# Convert with -n, which should still not result in a fully allocated 
> > image for
> > +# compat=1.1 (because it can use zero clusters), but it should be fully
> > +# allocated with compat=0.10
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o 
> > compat=1.1 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> Do we have a real use case for this convert? Doesn't this hide all the
> data in the backing file by data from source?

There is probably no real use case for this. But it has a defined
behaviour and it's always good to cover corner cases with tests so that
unintentional changes can be found (which may potentially affect more
relevant cases, too).

Kevin