Re: [Qemu-devel] [PATCH v3 6/7] iotests: Test qemu-img convert --salvage

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10718 bytes --]

On 16.04.19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> 13.04.2019 19:53, Max Reitz wrote:
>> This test converts a simple image to another, but blkdebug injects
>> block_status and read faults at some offsets.  The resulting image
>> should be the same as the input image, except that sectors that could
>> not be read have to be 0.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/249     | 163 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/249.out |  43 ++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 207 insertions(+)
>>   create mode 100755 tests/qemu-iotests/249
>>   create mode 100644 tests/qemu-iotests/249.out
>>
>> diff --git a/tests/qemu-iotests/249 b/tests/qemu-iotests/249
>> new file mode 100755
>> index 0000000000..d3883d75f3
>> --- /dev/null
>> +++ b/tests/qemu-iotests/249
>> @@ -0,0 +1,163 @@
>> +#!/bin/bash
>> +#
>> +# Test qemu-img convert --salvage
>> +#
>> +# Copyright (C) 2018 Red Hat, Inc.
> 
> 2019
> 
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +# creator
>> +owner=mreitz@redhat.com
>> +
>> +seq=$(basename $0)
>> +echo "QA output created by $seq"
>> +
>> +here=$PWD
> 
> this variable is not defined in latest bash-based iotests, is it needed really?

Hm, no, it’s just here to remind me that this test really is from 2018. :-)

>> +status=1	# failure is the default!
>> +
>> +_cleanup()
>> +{
>> +    _cleanup_test_img
> 
> as I understand, you also need to remove source img which is $TEST_IMG.orig

That’s done automatically by _cleanup_test_img().

(It removes $TEST_IMG, $TEST_IMG.orig, and $TEST_IMG.base.)

>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.qemu
>> +
>> +_supported_fmt generic
>> +_supported_proto file
>> +_supported_os Linux
>> +
>> +
>> +TEST_IMG="$TEST_IMG.orig" _make_test_img 64M
>> +
>> +$QEMU_IO -c 'write -P 42 0 64M' "$TEST_IMG.orig" | _filter_qemu_io
>> +
>> +
>> +sector_size=512
>> +
>> +# Offsets on which to fail block-status.  Keep in ascending order so
>> +# the indexing done by _filter_offsets will appear in ascending order
>> +# in the output as well.
>> +status_fail_offsets="$((16 * 1024 * 1024 + 8192))
>> +                     $((33 * 1024 * 1024 + 512))"
>> +
>> +# Offsets on which to fail reads.  Keep in ascending order for the
>> +# same reason.
>> +# Element 1 is shared with $status_fail_offsets on purpose.
> 
> you mean element 2
> 
>> +# Elements 2 and later test what happens when a continuous range of
> 
> and 3..
> 
> ah no, you just counting from zero..

Of course I am. :-)

I can rewrite the comment to "The second element" and "Starting with the
third element, we test...".

>> +# sectors is inaccessible.
>> +read_fail_offsets="$((32 * 1024 * 1024 - 65536))
>> +                   $((33 * 1024 * 1024 + 512))
>> +                   $(seq $((34 * 1024 * 1024)) $sector_size \
>> +                         $((34 * 1024 * 1024 + 4096 - $sector_size)))"
>> +
>> +
>> +# blkdebug must be above the format layer so it can intercept all
>> +# block-status events
>> +source_img="json:{'driver': 'blkdebug',
>> +                  'image': {
>> +                      'driver': '$IMGFMT',
>> +                      'file': {
>> +                          'driver': 'file',
>> +                          'filename': '$TEST_IMG.orig'
>> +                      }
>> +                  },
>> +                  'inject-error': ["
>> +
>> +for ofs in $status_fail_offsets
>> +do
>> +    source_img+="{ 'event': 'none',
> 
> idea: may be make @event optional with default of 'none'?

I don’t know whether that makes sense.  'none' is a special case, so my
personal feeling is not to make it the default.

>> +                   'iotype': 'block-status',
>> +                   'errno': 5,
>> +                   'sector': $((ofs / sector_size)) },"
>> +done
>> +
>> +for ofs in $read_fail_offsets
>> +do
>> +    source_img+="{ 'event': 'none',
>> +                   'iotype': 'read',
>> +                   'errno': 5,
>> +                   'sector': $((ofs / sector_size)) },"
>> +done
>> +
>> +# Remove the trailing comma and terminate @inject-error and json:{}
>> +source_img="${source_img%,} ] }"
>> +
>> +
>> +echo
>> +
>> +
>> +_filter_offsets() {
> 
> why to filter them? They are constant..

So the reference output actually tells you what’s happening.

I personally don’t like reference outputs that are full of numbers which
I have a hard time associating with anything meaningful.  This function
here does it for me.

Max

>> +    filters=
>> +
>> +    index=0
>> +    for ofs in $2
>> +    do
>> +        filters+=" -e s/$(printf "$1" $ofs)/status_fail_offset_$index/"
>> +        index=$((index + 1))
>> +    done
>> +
>> +    index=0
>> +    for ofs in $3
>> +    do
>> +        filters+=" -e s/$(printf "$1" $ofs)/read_fail_offset_$index/"
>> +        index=$((index + 1))
>> +    done
>> +
>> +    sed $filters
>> +}
>> +
>> +# While determining the number of allocated sectors in the input
>> +# image, we should see one block status warning per element of
>> +# $status_fail_offsets.
>> +#
>> +# Then, the image is read.  Since the block status is queried in
>> +# basically the same way, the same warnings as in the previous step
>> +# should reappear.  Interleaved with those we should see a read
>> +# warning per element of $read_fail_offsets.
>> +# Note that $read_fail_offsets and $status_fail_offsets share an
>> +# element (read_fail_offset_1 == status_fail_offset_1), so
>> +# "status_fail_offset_1" in the output is the same as
>> +# "read_fail_offset_1".
>> +$QEMU_IMG convert --salvage "$source_img" "$TEST_IMG" 2>&1 \
>> +    | _filter_offsets '%i' "$status_fail_offsets" "$read_fail_offsets"
>> +
>> +echo
>> +
>> +# The offsets where the block status could not be determined should
>> +# have been treated as containing data and thus should be correct in
>> +# the output image.
>> +# The offsets where reading failed altogether should be 0.  Make them
>> +# 0 in the input image, too, so we can compare both images.
>> +for ofs in $read_fail_offsets
>> +do
>> +    $QEMU_IO -c "write -z $ofs $sector_size" "$TEST_IMG.orig" \
>> +        | _filter_qemu_io \
>> +        | _filter_offsets '%i' '' "$read_fail_offsets"
>> +done
>> +
>> +echo
>> +
>> +# These should be equal now.
>> +$QEMU_IMG compare "$TEST_IMG.orig" "$TEST_IMG"
>> +
>> +
>> +# success, all done
>> +echo "*** done"
>> +rm -f $seq.full
>> +status=0
>> diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
>> new file mode 100644
>> index 0000000000..9213d3e450
>> --- /dev/null
>> +++ b/tests/qemu-iotests/249.out
>> @@ -0,0 +1,43 @@
>> +QA output created by 249
>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
>> +wrote 67108864/67108864 bytes at offset 0
>> +64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +
>> +qemu-img: warning: error while reading block status at offset status_fail_offset_0: Input/output error
>> +qemu-img: warning: error while reading block status at offset status_fail_offset_1: Input/output error
>> +qemu-img: warning: error while reading block status at offset status_fail_offset_0: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_0: Input/output error
>> +qemu-img: warning: error while reading block status at offset status_fail_offset_1: Input/output error
>> +qemu-img: warning: error while reading offset status_fail_offset_1: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_2: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_3: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_4: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_5: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_6: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_7: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_8: Input/output error
>> +qemu-img: warning: error while reading offset read_fail_offset_9: Input/output error
>> +
>> +wrote 512/512 bytes at offset read_fail_offset_0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_1
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_2
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_3
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_4
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_5
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_6
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_7
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_8
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 512/512 bytes at offset read_fail_offset_9
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +
>> +Images are identical.
>> +*** done
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index bae7718380..a2e2f507e8 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -248,3 +248,4 @@
>>   246 rw auto quick
>>   247 rw auto quick
>>   248 rw auto quick
>> +249 auto quick
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]