Re: [Qemu-devel] [PATCHv2 05/18] qemu-iotests: fix tests 014 and 023 to work with any protocol

[Kevin Wolf]

Am 10.01.2014 um 20:06 hat Peter Lieven geschrieben:
> Am 10.01.2014 20:04, schrieb Kevin Wolf:
> > Am 06.01.2014 um 07:49 hat Peter Lieven geschrieben:
> >> On 06.01.2014 06:40, Fam Zheng wrote:
> >>> On 2014年01月06日 01:21, Peter Lieven wrote:
> >>>> Signed-off-by: Peter Lieven <address@hidden>
> >>>> ---
> >>>>  tests/qemu-iotests/014            |    4 ++--
> >>>>  tests/qemu-iotests/014.out        |    2 +-
> >>>>  tests/qemu-iotests/023            |   11 +++++------
> >>>>  tests/qemu-iotests/023.out        |   16 ++++++++--------
> >>>>  tests/qemu-iotests/common.pattern |    7 +++----
> >>>>  5 files changed, 19 insertions(+), 21 deletions(-)
> >>>>
> >>>> diff --git a/tests/qemu-iotests/014 b/tests/qemu-iotests/014
> >>>> index b23c2db..01fb614 100755
> >>>> --- a/tests/qemu-iotests/014
> >>>> +++ b/tests/qemu-iotests/014
> >>>> @@ -43,14 +43,14 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >>>>
> >>>>  # much of this could be generic for any format supporting snapshots
> >>>>  _supported_fmt 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:"
> >>>>  for offset in $TEST_OFFSETS; do
> >>>> diff --git a/tests/qemu-iotests/014.out b/tests/qemu-iotests/014.out
> >>>> index 4744b4b..6459af0 100644
> >>>> --- a/tests/qemu-iotests/014.out
> >>>> +++ b/tests/qemu-iotests/014.out
> >>>> @@ -1,5 +1,5 @@
> >>>>  QA output created by 014
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=6442450944
> >>>>  Testing empty image:
> >>>>  test2: With offset 0
> >>>>  === Clusters to be compressed [1]
> >>>> diff --git a/tests/qemu-iotests/023 b/tests/qemu-iotests/023
> >>>> index 9ad06b9..2357696 100755
> >>>> --- a/tests/qemu-iotests/023
> >>>> +++ b/tests/qemu-iotests/023
> >>>> @@ -41,7 +41,7 @@ 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"
> >>>> @@ -55,7 +55,7 @@ for CLUSTER_SIZE in $CLUSTER_SIZES; do
> >>>>      echo "Creating new image; cluster size: $CLUSTER_SIZE"
> >>>>      echo
> >>>>
> >>>> -    _make_test_img 8G
> >>>> +    TEST_IMG=$TEST_IMG.orig _make_test_img 8G
> >>>>
> >>>>      echo "Testing empty image"
> >>>>      echo
> >>>> @@ -63,15 +63,14 @@ for CLUSTER_SIZE in $CLUSTER_SIZES; do
> >>>>      for offset in $TEST_OFFSETS; do
> >>>>          echo "At offset $offset:"
> >>>>          for op in $TEST_OPS; do
> >>>> -            io_test $op $offset $CLUSTER_SIZE 3
> >>>> +            TEST_IMG=$TEST_IMG.orig io_test $op $offset $CLUSTER_SIZE 3
> >>>>          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"
> >>>> @@ -101,7 +100,7 @@ for CLUSTER_SIZE in $CLUSTER_SIZES; do
> >>>>      echo "Creating another new image"
> >>>>      echo
> >>>>
> >>>> -    _make_test_img 8G
> >>>> +    TEST_IMG=$TEST_IMG.orig _make_test_img 8G
> >>>>
> >>>>      echo "More complex patterns"
> >>>>      echo
> >>>> diff --git a/tests/qemu-iotests/023.out b/tests/qemu-iotests/023.out
> >>>> index ec32341..b80836d 100644
> >>>> --- a/tests/qemu-iotests/023.out
> >>>> +++ b/tests/qemu-iotests/023.out
> >>>> @@ -1,7 +1,7 @@
> >>>>  QA output created by 023
> >>>>  Creating new image; cluster size: 1024
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  Testing empty image
> >>>>
> >>>>  At offset 0:
> >>>> @@ -5664,7 +5664,7 @@ read 3072/3072 bytes at offset 4295491072
> >>>>  No errors were found on the image.
> >>>>  Creating another new image
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  More complex patterns
> >>>>
> >>>>  test2: With offset 0
> >>>> @@ -5887,7 +5887,7 @@ read 2048/2048 bytes at offset 4295001088
> >>>>  No errors were found on the image.
> >>>>  Creating new image; cluster size: 4096
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  Testing empty image
> >>>>
> >>>>  At offset 0:
> >>>> @@ -12270,7 +12270,7 @@ read 12288/12288 bytes at offset 4301256704
> >>>>  No errors were found on the image.
> >>>>  Creating another new image
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  More complex patterns
> >>>>
> >>>>  test2: With offset 0
> >>>> @@ -12493,7 +12493,7 @@ read 8192/8192 bytes at offset 4295102464
> >>>>  No errors were found on the image.
> >>>>  Creating new image; cluster size: 16384
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  Testing empty image
> >>>>
> >>>>  At offset 0:
> >>>> @@ -18876,7 +18876,7 @@ read 49152/49152 bytes at offset 4395622400
> >>>>  No errors were found on the image.
> >>>>  Creating another new image
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  More complex patterns
> >>>>
> >>>>  test2: With offset 0
> >>>> @@ -19099,7 +19099,7 @@ read 32768/32768 bytes at offset 4295507968
> >>>>  No errors were found on the image.
> >>>>  Creating new image; cluster size: 65536
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  Testing empty image
> >>>>
> >>>>  At offset 0:
> >>>> @@ -25482,7 +25482,7 @@ read 196608/196608 bytes at offset 5905547264
> >>>>  No errors were found on the image.
> >>>>  Creating another new image
> >>>>
> >>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=8589934592
> >>>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=8589934592
> >>>>  More complex patterns
> >>>>
> >>> The same comment for this as for [04/18], there are two many overrides 
> >>> which are not necessary because you can replace "mv $TEST_IMG 
> >>> $TEST_IMG.orig" with "TEST_IMG=$TEST_IMG.orig _make_test_img 8G" without 
> >>> touching other parts.
> >>>
> >>>>  test2: With offset 0
> >>>> diff --git a/tests/qemu-iotests/common.pattern 
> >>>> b/tests/qemu-iotests/common.pattern
> >>>> index ddfbca1..d6ed5c1 100644
> >>>> --- a/tests/qemu-iotests/common.pattern
> >>>> +++ b/tests/qemu-iotests/common.pattern
> >>>> @@ -111,13 +111,12 @@ function io_test2() {
> >>>>
> >>>>      # Write the clusters to be compressed
> >>>>      echo === Clusters to be compressed [1]
> >>>> -    io_pattern writev $((offset + 4 * $cluster_size)) $cluster_size 
> >>>> $((9 * $cluster_size)) $num 165
> >>>> +    TEST_IMG=$TEST_IMG.orig io_pattern writev $((offset + 4 * 
> >>>> $cluster_size)) $cluster_size $((9 * $cluster_size)) $num 165
> >>>>      echo === Clusters to be compressed [2]
> >>>> -    io_pattern writev $((offset + 5 * $cluster_size)) $cluster_size 
> >>>> $((9 * $cluster_size)) $num 165
> >>>> +    TEST_IMG=$TEST_IMG.orig io_pattern writev $((offset + 5 * 
> >>>> $cluster_size)) $cluster_size $((9 * $cluster_size)) $num 165
> >>>>      echo === Clusters to be compressed [3]
> >>>> -    io_pattern writev $((offset + 8 * $cluster_size)) $cluster_size 
> >>>> $((9 * $cluster_size)) $num 165
> >>>> +    TEST_IMG=$TEST_IMG.orig io_pattern writev $((offset + 8 * 
> >>>> $cluster_size)) $cluster_size $((9 * $cluster_size)) $num 165
> >>>>
> >>>> -    mv "$TEST_IMG" "$TEST_IMG.orig"
> >>>>      $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c "$TEST_IMG.orig" 
> >>>> "$TEST_IMG"
> >>>>
> >>>>      # Write the used clusters
> >>>>
> >>> Especially here, overriding TEST_IMG inside a utility function is not 
> >>> right.
> >> The problem is, that in its original form io_test2 only works with 
> >> protocol file. Maybe its better to split io_test2 into
> >> 2 parts.
> > Perhaps you should leave $TEST_IMG as it is and instead of using mv to
> > move the old version away create a new one with a different name? I
> > guess io_pattern would need to get a filename then, but that shouldn't
> > be hard.
> The problem is not only io_pattern, also all the other commands. Or are you 
> just referring to the changes in io_test2
> and are fine with the overrides outside a function?

I haven't looked at the full series yet, but yes, probably all the other
functions in common.pattern as well.

In general, I think the primary test image should be $TEST_IMG, not
$TEST_IMG.orig. Hiding the creation of a copy in common.pattern, after
which $TEST_IMG becomes valid, doesn't make it easier to understand
either.

Kevin