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

[Jeff Cody]

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.

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