Re: [PATCH v14 4/4] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

On 31.03.2020 21:43, Eric Blake wrote:
> On 3/31/20 12:44 PM, Denis Plotnikov wrote:
> > The test checks fulfilling qcow2 requiriements for the compression
> > type feature and zstd compression type operability.
> >
> > Signed-off-by: Denis Plotnikov <address@hidden>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   tests/qemu-iotests/287     | 128 +++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/287.out |  43 +++++++++++++
> >   tests/qemu-iotests/group   |   1 +
> >   3 files changed, 172 insertions(+)
> >   create mode 100755 tests/qemu-iotests/287
> >   create mode 100644 tests/qemu-iotests/287.out
>
> > +
> > +# Check if we can run this test.
> > +
> > +IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid 
> > parameter 'zstd'" 2>&1 1>/dev/null
> > +
> > +ZSTD_SUPPORTED=$?
> > +
> > +if (($ZSTD_SUPPORTED==0)); then
>
> This is a bash script, so (()) works; but the bash manual documents 
> that (()) is not idiomatic.  Also, what you have is rather verbose...
>
> > +    _notrun "ZSTD is disabled"
> > +fi
>
> ...I might have written:
>
> if IMGOPTS='compression_type=zstd' _make_test_img 64M |
>     grep "Invalid parameter 'zstd'"; then
>     _notrun "ZSTD is disabled"
> fi
yes, this on is shorter
> > +# Test: an image can't be openned if compression type is zlib and
>
> opened
>
> > +#       incompatible feature compression type is set
> > +echo
> > +echo "=== Testing zlib with incompatible bit set  ==="
> > +echo
> > +
> > +IMGOPTS='compression_type=zlib' _make_test_img 64M
> > +$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
> > +# to make sure the bit was actually set
> > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> > +$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
> > +if (($?==0)); then
> > +    echo "Error: The image openned successfully. The image must not 
> > be openned"
>
> twice more
>
> > +fi
> > +
> > +# Test: an image can't be openned if compression type is NOT zlib and
>
> and again.  Multiple spots in the file, I'll quit pointing them out.
>
> > +#       incompatible feature compression type is UNSET
> > +echo
> > +echo "=== Testing zstd with incompatible bit unset  ==="
>
> Why the double space?
>
> > +# Test: using zstd compression, write to and read from an image
> > +echo
> > +echo "=== Testing reading and writing with zstd ==="
> > +echo
> > +
> > +IMGOPTS='compression_type=zstd' _make_test_img 64M
> > +$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
>
> That's only one cluster.  Wouldn't it be better to write more than 
> one, to prove that we handle back-to-back compressed clusters 
> resulting from back-to-back inputs?
I'll add a case for back-to-back compressed clusters
> > +$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
>
> And if you do compress more than one cluster, you may also want to use 
> different patterns over those various clusters.
>
> > +$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
> > +$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
> > +
> > +# success, all done
> > +echo "*** done"
>
> Is it also worth trying to generate an image with (pseudo-)random 
> contents, and trying qemu-img convert to prove that uncompressable 
> clusters are handled sanely?  Ideally, there would be a way to use a 
> fixed PRNG and seed that produces a deterministic sequence that cannot 
> be compressed, but even if we can't meet the ideal, having a test that 
> non-deterministically is likely to generate an uncompressable cluster 
> in most runs is better than nothing (such as 1M of data copied from 
> /dev/urandom, then qemu-img convert on that data).
And, I'll try to add a case for incompressible data.

Thanks