Re: [Qemu-devel] [PATCH 21/21] iotests: Add test for different refcount widths

[Max Reitz]

On 2014-11-11 at 20:53, Eric Blake wrote:
> On 11/10/2014 06:45 AM, Max Reitz wrote:
> > Add a test for conversion between different refcount widths and errors
> > specific to certain widths (i.e. snapshots with refcount_width=1).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   tests/qemu-iotests/112     | 225 +++++++++++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/112.out | 123 +++++++++++++++++++++++++
> >   tests/qemu-iotests/group   |   1 +
> >   3 files changed, 349 insertions(+)
> >   create mode 100755 tests/qemu-iotests/112
> >   create mode 100644 tests/qemu-iotests/112.out
> >
> > +
> > +# This tests qocw2-specific low-level functionality
> > +_supported_fmt qcow2
> > +_supported_proto file
> > +_supported_os Linux
> Might work on more than just Linux, but then again, it's probably worth
> scrubbing the whole testsuite for situations like that, so don't worry
> about it here.
>
> > +# This test will set refcount_width on its own which would conflict with the
> > +# manual setting; compat will be overridden as well
> > +_unsupported_imgopts refcount_width 'compat=0.10'
> > +
> > +function print_refcount_width()
> > +{
> > +    $QEMU_IMG info "$TEST_IMG" | grep 'refcount width:' | sed -e 's/^ *//'
> grep|sed is almost always a waste.  This is equivalent:
>
>     $QEMU_IMG info "$TEST_IMG" | sed -n '/refcount width:/ s/^ *//p'

As said before, I just don't know sed well enough. My knowledge of sed 
is "you can use it for regex replacement with -e", and that's about it. 
Oh, and "you can do it in-place with -i" and "someone wrote Sokoban in 
sed". And maybe even "sed better fits the term Standard EDitor than ed 
does".

But thanks a lot for telling me, I'm just always afraid to learn sed 
because it seems even more unreadable than perl to me...

> > +echo
> > +echo '=== Snapshot limit on refcount_width=1 ==='
> > +echo
> > +
> > +IMGOPTS="$IMGOPTS,refcount_width=1" _make_test_img 64M
> > +print_refcount_width
> > +
> > +$QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
> > +
> > +# Should fail
> > +$QEMU_IMG snapshot -c foo "$TEST_IMG"
> > +
> > +# The new L1 table could/shoud be leaked
> s/shoud/should/

Right.

> > +_check_test_img
> > +
> > +echo
> > +echo '=== Snapshot limit on refcount_width=2 ==='
> > +echo
> > +
> > +IMGOPTS="$IMGOPTS,refcount_width=2" _make_test_img 64M
> > +print_refcount_width
> > +
> > +$QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
> > +
> > +# Should succeed
> > +$QEMU_IMG snapshot -c foo "$TEST_IMG"
> > +$QEMU_IMG snapshot -c bar "$TEST_IMG"
> > +# Should fail (4th reference)
> > +$QEMU_IMG snapshot -c baz "$TEST_IMG"
> > +
> > +# The new L1 table could/shoud be leaked
> again

yyp is dangerous.

> > +echo
> > +echo '=== Amend with snapshot ==='
> > +echo
> > +
> > +$QEMU_IMG snapshot -c foo "$TEST_IMG"
> > +# Just to have different refcounts across the image
> > +$QEMU_IO -c 'write 0 16M' "$TEST_IMG" | _filter_qemu_io
> > +
> > +# Should not work
> > +$QEMU_IMG amend -o refcount_width=1 "$TEST_IMG"
> > +_check_test_img
> > +print_refcount_width
> This matches your initial implementation. Someday, though, we may decide
> to auto-COW any overflowed cluster, and thus allow the conversion to
> succeed.  Worth a comment?

Yes, will do.

> > +echo '=== Testing too many references for check ==='
> > +echo
> > +
> > +IMGOPTS="$IMGOPTS,refcount_width=1" _make_test_img 64M
> > +print_refcount_width
> > +
> > +# This cluster should be created at 0x50000
> > +$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
> > +# Now make the second L2 entriy (the L2 table should be at 0x40000) point to
> s/entriy/entry/

I think this happened because at one point in time it said something 
about "L2 entries".

> > +# success, all done
> > +echo '*** done'
> > +rm -f $seq.full
> > +status=0
> Overall a nice set of tests!
>
>
> > +=== Snapshot limit on refcount_width=1 ===
> > +
> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> > +refcount width: 1
> > +wrote 512/512 bytes at offset 0
> > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +qemu-img: Could not create snapshot 'foo': -22 (Invalid argument)
> > +Leaked cluster 6 refcount=1 reference=0
> Bummer that the error message did not state WHY (because a cluster would
> overflow refcounts), but I'm not sure how hard it would be to make that
> better, and at least we correctly errored out.

Yes, I know, it's a really bad error. The problem is that no Error 
object is used in that path at all so it will be rather cumbersome, but 
I'll look into it one more time.

Max