Re: [Qemu-devel] [PATCHv2 04/18] qemu-iotests: fix test 013 to work with any protocol

[Peter Lieven]

> Am 06.01.2014 um 21:40 schrieb Jeff Cody <address@hidden>:
> 
>> On Mon, Jan 06, 2014 at 07:48:25AM +0100, Peter Lieven wrote:
>>> On 06.01.2014 06:31, Fam Zheng wrote:
>>>> On 2014年01月06日 01:21, Peter Lieven wrote:
>>>> Signed-off-by: Peter Lieven <address@hidden>
>>>> ---
>>>> tests/qemu-iotests/013     |    9 ++++-----
>>>> tests/qemu-iotests/013.out |    2 +-
>>>> 2 files changed, 5 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/tests/qemu-iotests/013 b/tests/qemu-iotests/013
>>>> index ea3cab9..0dbc934 100755
>>>> --- a/tests/qemu-iotests/013
>>>> +++ b/tests/qemu-iotests/013
>>>> @@ -41,14 +41,14 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>> 
>>>> # much of this could be generic for any format supporting compression.
>>>> _supported_fmt qcow qcow2
>>>> -_supported_proto file
>>>> +_supported_proto generic
>>>> _supported_os Linux
>>>> 
>>>> TEST_OFFSETS="0 4294967296"
>>>> TEST_OPS="writev read write readv"
>>>> CLUSTER_SIZE=4096
>>> 
>>> I think dropping these three TEST_IMG overriding change...
>>> 
>>>> -_make_test_img 6G
>>>> +TEST_IMG=$TEST_IMG.orig _make_test_img 6G
>>> 
>>> #1
>>> 
>>>> 
>>>> echo "Testing empty image"
>>>> echo
>>>> @@ -56,16 +56,15 @@ echo
>>>> for offset in $TEST_OFFSETS; do
>>>>     echo "At offset $offset:"
>>>>     for op in $TEST_OPS; do
>>>> -        io_test $op $offset $CLUSTER_SIZE 8
>>>> +        TEST_IMG=$TEST_IMG.orig io_test $op $offset $CLUSTER_SIZE 8
>>> 
>>> #2
>>> 
>>>>     done
>>>> -    _check_test_img
>>>> +    TEST_IMG=$TEST_IMG.orig _check_test_img
>>> 
>>> #3
>>> 
>>>> done
>>>> 
>>>> 
>>>> echo "Compressing image"
>>>> echo
>>>> 
>>>> -mv "$TEST_IMG" "$TEST_IMG.orig"
>>> 
>>> and changing this to
>>> 
>>> TEST_IMG=$TEST_IMG.orig _make_test_img 6G
>>> 
>>> Should work.
>> Unfortunately it doesn't. All subsequent commands will then work
>> on $TEST_IMG.orig altough they shouldn't. In case of
>> 013 this is io_test, _check_test_img and the cleanup at the end.
>> 
>> There are 3 options:
>> - override it in every line that should use an alternate $TEST_IMG
>> - save the original $TEST_IMG and restore it.
>> - rework all commands to take the file as parameter and not use
>>   a global variable for it.
>> 
>> I choosed the first one because it makes clear which $TEST_IMG is acutally
>> used. You see from the output and the code that you are dealing with the
>> file that is later used as $TEST_IMG.orig. If you see $TEST_IMG there you
>> can't distinguish if its the backing or original file or the actual image.
> 
> There more I've read through the patches, my opinion is that something
> more along the lines of option #3 would be best.  If that is done, it
> may be nice for the file to be an optional argument to the function.
> That way, for tests that only support IMGPROTO=file, the current
> default operation does not need to change.
> 
> My fear is the current method (#1) seems a bit unwieldy; I
> foresee scripts often forgetting to do these manual override steps.
> Then again, the default IMGPROTO was set to 'file', so perhaps my fear
> is unfounded.

another question would be if we really want that all these tests work with all 
protocols. i think their main purpose is to test the IMGFORMAT, but maybe they 
could help to trigger subtile bugs in IMGPROTO != file that the more generic 
tests don't...

> 
>> 
>> But I thought that this would be controversal. This is I why I splitted the 
>> patch
>> into individual ones. So its possible to drop all these patches and still be 
>> able
>> to proceed with the integration of the NFS protocol driver.
>>> 
>>>> $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c "$TEST_IMG.orig" "$TEST_IMG"
>>>> 
>>>> echo "Testing compressed image"
>>>> diff --git a/tests/qemu-iotests/013.out b/tests/qemu-iotests/013.out
>>>> index 43a414c..763cb0c 100644
>>>> --- a/tests/qemu-iotests/013.out
>>>> +++ b/tests/qemu-iotests/013.out
>>>> @@ -1,5 +1,5 @@
>>>> QA output created by 013
>>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944
>>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=6442450944
>>> 
>>> So this is not necessary.
>>> 
>>> Fam
>> Peter