Re: [PATCH 3/3] iotests/138: Test leaks/corruptions fixed report

[Max Reitz]

On 27.02.20 19:56, Eric Blake wrote:
> On 2/27/20 11:02 AM, Max Reitz wrote:
>> Test that qemu-img check reports the number of leaks and corruptions
>> fixed in its JSON report (after a successful run).
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>   tests/qemu-iotests/138     | 41 ++++++++++++++++++++++++++++++++++++--
>>   tests/qemu-iotests/138.out | 14 +++++++++++++
>>   2 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
>> index 54b01046ad..25bfbd4cca 100755
>> --- a/tests/qemu-iotests/138
>> +++ b/tests/qemu-iotests/138
>> @@ -41,8 +41,10 @@ _supported_fmt qcow2
>>   _supported_proto file
>>   _supported_os Linux
>>   # With an external data file, data clusters are not refcounted
>> -# (and so qemu-img check does not check their refcount)
>> -_unsupported_imgopts data_file
>> +# (and so qemu-img check does not check their refcount);
> 
> Not this patch's problem, but is that a bug in 'qemu-img check' for not
> validating refcounts on an external data file?  Or is it merely this
> comment wording is not quite perfect?

There are no refcounts for an external data file, because every cluster
is refcounted exactly once and we don’t need refcounts to allocated
clusters (the offset in the data file is the same as the guest offset in
the image).

It kind of is what the comment says, but I suppose we could drop the
part about qemu-img check?

>> +# we want to modify the refcounts, so we need them to have a specific
>> +# format (namely u16)
>> +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>     echo
>>   echo '=== Check on an image with a multiple of 2^32 clusters ==='
>> @@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8))
>> "\x00\x80\x00\x00\x00\x00\x00\x00"
>>   # allocate memory", we have an error showing that l2 entry is invalid.
>>   _check_test_img
>>   +echo
>> +echo '=== Check leaks-fixed/corruptions-fixed report'
>> +echo
>> +
>> +# After leaks and corruptions were fixed, those numbers should be
>> +# reported by qemu-img check
>> +_make_test_img 64k
>> +
>> +# Allocate data cluster
>> +$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
>> +
>> +reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
>> +refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
>> +
>> +# Introduce a leak: Make the image header's refcount 2
>> +poke_file "$TEST_IMG" "$refblock_ofs" "\x00\x02"
> 
> Why not use your brand-new poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2

Because I didn’t need it at this point.  I only needed it for the next
line, so I wrote it in between. :)

But yes, it does make sense to use it wherever possible now that we have it.

>> +
>> +l1_ofs=$(peek_file_be "$TEST_IMG" 40 8)
>> +
>> +# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry
>> +l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8)
>> +l1_entry=$((l1_entry & ~(1 << 63)))
>> +poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry
> 
> Yep, the new function makes this task easier.  (You could also just peek
> 1 byte at $((l1_ofs+7)) then write it back out with poke_file
> "$TEST_IMG" $((l1_ofs + 7)) $(printf '\\x%02x' $((val & 0xfe)))", but
> that just doesn't look as nice)
> 
>> +
>> +echo
>> +# Should print the number of corruptions and leaks fixed
>> +# (Filter out all JSON fields (recognizable by their four-space
>> +# indentation), but keep the "-fixed" fields (by removing two spaces
>> +# from their indentation))
>> +# (Also filter out the L1 entry, because why not)
>> +_check_test_img -r all --output=json \
>> +    | sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
>> +    | grep -v '^    ' \
>> +    | sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"
> 
> sed | grep | sed can often be done with a single sed:
> 
> ... | sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
>        -e '/^    /d' \
>        -e "s/\\..."

Nice.

> Using \\< and \\> in the sed regex is a GNUism; do we want this test to
> run on BSD?

Hm.  I suppose we can just use [^0-9a-f] instead.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!

Max

signature.asc
Description: OpenPGP digital signature