diff mbox

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

Message ID 1388942528-10453-5-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven Jan. 5, 2014, 5:21 p.m. UTC
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 tests/qemu-iotests/013     |    9 ++++-----
 tests/qemu-iotests/013.out |    2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

Comments

Fam Zheng Jan. 6, 2014, 5:31 a.m. UTC | #1
On 2014年01月06日 01:21, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   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.

>   $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 Lieven Jan. 6, 2014, 6:48 a.m. UTC | #2
On 06.01.2014 06:31, Fam Zheng wrote:
> On 2014年01月06日 01:21, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   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.

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
Fam Zheng Jan. 6, 2014, 10:09 a.m. UTC | #3
On 2014年01月06日 14:48, 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 <pl@kamp.de>
>>> ---
>>>   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.
>

Why? The overriding is temporary and subsequent commands are not affected.

My proposal above doesn't work, though, because an empty new image 
doesn't contain the right data, what is needed here is copy. So maybe 
change the "mv" line to:

$QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG" "$TEST_IMG.orig"

could do the work, but I'm not sure if this fits every case.

> 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.
>
> 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.

I'll leave maintainers to decide.

Fam
Peter Lieven Jan. 6, 2014, 12:21 p.m. UTC | #4
On 06.01.2014 11:09, Fam Zheng wrote:
> On 2014年01月06日 14:48, 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 <pl@kamp.de>
>>>> ---
>>>>   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.
>>
>
> Why? The overriding is temporary and subsequent commands are not affected.
If you put in a singe

TEST_IMG=$TEST_IMG.orig

line, this affects all further commands in the same test script.

If you put the TEST_IMG=$TEST_IMG.orig before a command it affectes only this single command.

>
> My proposal above doesn't work, though, because an empty new image doesn't contain the right data, what is needed here is copy. So maybe change the "mv" line to:
>
> $QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG" "$TEST_IMG.orig"
>
> could do the work, but I'm not sure if this fits every case.
This is unnecessary (copy) overhead and in some cases it could falsify the test. The convert process
does not guarantee to create identical copies. You could use raw format, but in this case the image
can only be a multiple of 512 byte.
>
>> 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.
>>
>> 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.
>
> I'll leave maintainers to decide.
That is the best. I from my perspective have checked that the NFS driver is working great and provided fixes for a lot
of tests to make the iotests work with NFS and QCOW2/VMDK. Most of the tests are there to test the formats actually
and errors are likely to happen with every protocol. I would be totally fine if maintainers pick up patches 1,2,3,16,17
and leave the rest as is.

Peter
Fam Zheng Jan. 6, 2014, 12:47 p.m. UTC | #5
On 2014年01月06日 20:21, Peter Lieven wrote:
> On 06.01.2014 11:09, Fam Zheng wrote:
>> On 2014年01月06日 14:48, 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 <pl@kamp.de>
>>>>> ---
>>>>>   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.
>>>
>>
>> Why? The overriding is temporary and subsequent commands are not
>> affected.
> If you put in a singe
>
> TEST_IMG=$TEST_IMG.orig
>
> line, this affects all further commands in the same test script.
>
> If you put the TEST_IMG=$TEST_IMG.orig before a command it affectes only
> this single command.
>
>>
>> My proposal above doesn't work, though, because an empty new image
>> doesn't contain the right data, what is needed here is copy. So maybe
>> change the "mv" line to:
>>
>> $QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG" "$TEST_IMG.orig"
>>
>> could do the work, but I'm not sure if this fits every case.
> This is unnecessary (copy) overhead and in some cases it could falsify
> the test. The convert process
> does not guarantee to create identical copies. You could use raw format,
> but in this case the image
> can only be a multiple of 512 byte.

OK, thanks for clarification.

Fam
Jeff Cody Jan. 6, 2014, 8:40 p.m. UTC | #6
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 <pl@kamp.de>
> >>---
> >>  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.

> 
> 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
Peter Lieven Jan. 6, 2014, 10:35 p.m. UTC | #7
> Am 06.01.2014 um 21:40 schrieb Jeff Cody <jcody@redhat.com>:
> 
>> 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 <pl@kamp.de>
>>>> ---
>>>> 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
diff mbox

Patch

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
 
-_make_test_img 6G
+TEST_IMG=$TEST_IMG.orig _make_test_img 6G
 
 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
     done
-    _check_test_img
+    TEST_IMG=$TEST_IMG.orig _check_test_img
 done
 
 
 echo "Compressing image"
 echo
 
-mv "$TEST_IMG" "$TEST_IMG.orig"
 $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 
 Testing empty image
 
 At offset 0: