diff mbox series

[v2,20/21] iotests: Disable data_file where it cannot be used

Message ID 20191015142729.18123-21-mreitz@redhat.com
State New
Headers show
Series iotests: Allow ./check -o data_file | expand

Commit Message

Max Reitz Oct. 15, 2019, 2:27 p.m. UTC 
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/007 | 5 +++--
 tests/qemu-iotests/014 | 2 ++
 tests/qemu-iotests/015 | 5 +++--
 tests/qemu-iotests/026 | 5 ++++-
 tests/qemu-iotests/029 | 5 +++--
 tests/qemu-iotests/031 | 6 +++---
 tests/qemu-iotests/036 | 5 +++--
 tests/qemu-iotests/039 | 3 +++
 tests/qemu-iotests/046 | 2 ++
 tests/qemu-iotests/048 | 2 ++
 tests/qemu-iotests/051 | 5 +++--
 tests/qemu-iotests/058 | 5 +++--
 tests/qemu-iotests/060 | 6 ++++--
 tests/qemu-iotests/061 | 6 ++++--
 tests/qemu-iotests/062 | 2 +-
 tests/qemu-iotests/066 | 2 +-
 tests/qemu-iotests/067 | 6 ++++--
 tests/qemu-iotests/068 | 5 +++--
 tests/qemu-iotests/071 | 3 +++
 tests/qemu-iotests/073 | 2 ++
 tests/qemu-iotests/074 | 2 ++
 tests/qemu-iotests/080 | 5 +++--
 tests/qemu-iotests/090 | 2 ++
 tests/qemu-iotests/098 | 6 ++++--
 tests/qemu-iotests/099 | 3 ++-
 tests/qemu-iotests/103 | 5 +++--
 tests/qemu-iotests/108 | 6 ++++--
 tests/qemu-iotests/112 | 5 +++--
 tests/qemu-iotests/114 | 2 ++
 tests/qemu-iotests/121 | 3 +++
 tests/qemu-iotests/138 | 2 ++
 tests/qemu-iotests/156 | 2 ++
 tests/qemu-iotests/176 | 7 +++++--
 tests/qemu-iotests/191 | 2 ++
 tests/qemu-iotests/201 | 6 +++---
 tests/qemu-iotests/214 | 3 ++-
 tests/qemu-iotests/217 | 3 ++-
 tests/qemu-iotests/220 | 5 +++--
 tests/qemu-iotests/243 | 6 ++++--
 tests/qemu-iotests/244 | 5 +++--
 tests/qemu-iotests/250 | 2 ++
 tests/qemu-iotests/267 | 5 +++--
 42 files changed, 117 insertions(+), 52 deletions(-)

Comments

Maxim Levitsky Nov. 6, 2019, 3:52 p.m. UTC | #1 
On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/007 | 5 +++--
>  tests/qemu-iotests/014 | 2 ++
>  tests/qemu-iotests/015 | 5 +++--
>  tests/qemu-iotests/026 | 5 ++++-
>  tests/qemu-iotests/029 | 5 +++--
>  tests/qemu-iotests/031 | 6 +++---
>  tests/qemu-iotests/036 | 5 +++--
>  tests/qemu-iotests/039 | 3 +++
>  tests/qemu-iotests/046 | 2 ++
>  tests/qemu-iotests/048 | 2 ++
>  tests/qemu-iotests/051 | 5 +++--
>  tests/qemu-iotests/058 | 5 +++--
>  tests/qemu-iotests/060 | 6 ++++--
>  tests/qemu-iotests/061 | 6 ++++--
>  tests/qemu-iotests/062 | 2 +-
>  tests/qemu-iotests/066 | 2 +-
>  tests/qemu-iotests/067 | 6 ++++--
>  tests/qemu-iotests/068 | 5 +++--
>  tests/qemu-iotests/071 | 3 +++
>  tests/qemu-iotests/073 | 2 ++
>  tests/qemu-iotests/074 | 2 ++
>  tests/qemu-iotests/080 | 5 +++--
>  tests/qemu-iotests/090 | 2 ++
>  tests/qemu-iotests/098 | 6 ++++--
>  tests/qemu-iotests/099 | 3 ++-
>  tests/qemu-iotests/103 | 5 +++--
>  tests/qemu-iotests/108 | 6 ++++--
>  tests/qemu-iotests/112 | 5 +++--
>  tests/qemu-iotests/114 | 2 ++
>  tests/qemu-iotests/121 | 3 +++
>  tests/qemu-iotests/138 | 2 ++
>  tests/qemu-iotests/156 | 2 ++
>  tests/qemu-iotests/176 | 7 +++++--
>  tests/qemu-iotests/191 | 2 ++
>  tests/qemu-iotests/201 | 6 +++---
>  tests/qemu-iotests/214 | 3 ++-
>  tests/qemu-iotests/217 | 3 ++-
>  tests/qemu-iotests/220 | 5 +++--
>  tests/qemu-iotests/243 | 6 ++++--
>  tests/qemu-iotests/244 | 5 +++--
>  tests/qemu-iotests/250 | 2 ++
>  tests/qemu-iotests/267 | 5 +++--
>  42 files changed, 117 insertions(+), 52 deletions(-)
> 


> diff --git a/tests/qemu-iotests/007 b/tests/qemu-iotests/007
> index 7d3544b479..160683adf8 100755
> --- a/tests/qemu-iotests/007
> +++ b/tests/qemu-iotests/007
> @@ -41,8 +41,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  # refcount_bits must be at least 4 so we can create ten internal snapshots
> -# (1 bit supports none, 2 bits support two, 4 bits support 14)
> -_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]'
> +# (1 bit supports none, 2 bits support two, 4 bits support 14);
> +# snapshot are generally impossible with external data files
> +_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]' data_file
ACK
>  
>  echo
>  echo "creating image"



> diff --git a/tests/qemu-iotests/014 b/tests/qemu-iotests/014
> index 2f728a1956..e1221c0fff 100755
> --- a/tests/qemu-iotests/014
> +++ b/tests/qemu-iotests/014
> @@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> +# Compression and snapshots do not work with external data files
> +_unsupported_imgopts data_file
ACK
>  
>  TEST_OFFSETS="0 4294967296"
>  TEST_OPS="writev read write readv"



> diff --git a/tests/qemu-iotests/015 b/tests/qemu-iotests/015
> index eec5387f3d..4d8effd0ae 100755
> --- a/tests/qemu-iotests/015
> +++ b/tests/qemu-iotests/015
> @@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # actually any format that supports snapshots
>  _supported_fmt qcow2
>  _supported_proto generic
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
ACK
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
>  
>  echo
>  echo "creating image"


> diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
> index 3430029ed6..a4aa74764f 100755
> --- a/tests/qemu-iotests/026
> +++ b/tests/qemu-iotests/026
> @@ -49,7 +49,10 @@ _supported_cache_modes writethrough none
>  # 32 and 64 bits do not work either, however, due to different leaked cluster
>  # count on error.
>  # Thus, the only remaining option is refcount_bits=16.
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +#
> +# As for data_file, none of the refcount tests can work for it.
ACK
> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' \
> +    data_file
>  
>  echo "Errors while writing 128 kB"
>  echo



> diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
> index 9254ede5e5..2161a4b87a 100755
> --- a/tests/qemu-iotests/029
> +++ b/tests/qemu-iotests/029
> @@ -42,8 +42,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  _unsupported_proto vxhs
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
ACK
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
>  
>  offset_size=24
>  offset_l1_size=36



> diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
> index c44fcf91bb..646ecd593f 100755
> --- a/tests/qemu-iotests/031
> +++ b/tests/qemu-iotests/031
> @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
> -# We want to test compat=0.10, which does not support refcount widths
> -# other than 16
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# We want to test compat=0.10, which does not support external data
> +# files or refcount widths other than 16
> +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'

This is maybe another reason to split this test for compat=0.10 and for compat=1.1
But still can be done later of course.

>  
>  CLUSTER_SIZE=65536
>  



> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> index bbaf0ef45b..512598421c 100755
> --- a/tests/qemu-iotests/036
> +++ b/tests/qemu-iotests/036
> @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
> -# Only qcow2v3 and later supports feature bits
> -_unsupported_imgopts 'compat=0.10'
> +# Only qcow2v3 and later supports feature bits;
> +# qcow2.py does not support external data files

Minor nitpick, maybe tag this with TODO or so. No need to do now.

> +_unsupported_imgopts 'compat=0.10' data_file
>  
>  echo
>  echo === Image with unknown incompatible feature bit ===



> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 99563bf126..ddce48ab47 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -44,6 +44,9 @@ _supported_proto file
>  _supported_os Linux
>  _default_cache_mode writethrough
>  _supported_cache_modes writethrough
> +# Some of these test cases expect no external data file so that all
> +# clusters are part of the qcow2 image and refcounted
> +_unsupported_imgopts data_file
ACK
>  
>  size=128M
>  


> diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
> index 4e03ead7b1..a066eec605 100755
> --- a/tests/qemu-iotests/046
> +++ b/tests/qemu-iotests/046
> @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  _supported_fmt qcow2
>  _supported_proto file
> +# data_file does not support compressed clusters
> +_unsupported_imgopts data_file
This is a very nice test, which doesn't seem to  use compressed
clusters that much. I think it should be split as well.
No need to do this now of course, but maybe mark with TODO to 
avoid loosing that info.

>  
>  CLUSTER_SIZE=64k
>  size=128M



> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
> index a8feb76184..2af6b74b41 100755
> --- a/tests/qemu-iotests/048
> +++ b/tests/qemu-iotests/048
> @@ -49,6 +49,8 @@ _compare()
>  _supported_fmt raw qcow2 qed luks
>  _supported_proto file
>  _supported_os Linux
> +# Using 'cp' is incompatible with external data files
> +_unsupported_imgopts data_file
You could compare the external files instead in theory *I think*.
Another item on some TODO list I guess.

>  
>  # Remove once all tests are fixed to use TEST_IMG_FILE
>  # correctly and common.rc sets it unconditionally



> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 9cd1d60d45..0053bad46a 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  # A compat=0.10 image is created in this test which does not support anything
> -# other than refcount_bits=16
Here also the compat=0.10 image is only a small part of the test,
although it seems to get used later in the rest of the test,

so the test I think should be split so that rest of the test could run in all
configurations. 


> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# other than refcount_bits=16;
> +# it also will not support an external data file
> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
>  
>  do_run_qemu()
>  {



> diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
> index ed01115fa3..d5304bb404 100755
> --- a/tests/qemu-iotests/058
> +++ b/tests/qemu-iotests/058
> @@ -56,8 +56,9 @@ _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
>  _require_command QEMU_NBD
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
ACK
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
>  
>  nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
>  




> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 92243c2edd..8ad0d7a904 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -48,8 +48,10 @@ _filter_io_error()
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# These tests only work for compat=1.1 images with refcount_bits=16
> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# These tests only work for compat=1.1 images without an external
> +# data file with refcount_bits=16
Yea, with all hardcoded offsets, that isn't going to work.
ACK.
> +_unsupported_imgopts 'compat=0.10' data_file \
> +    'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>  
>  rt_offset=65536  # 0x10000 (XXX: just an assumption)
>  rb_offset=131072 # 0x20000 (XXX: just an assumption)



> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index b4076d8e8b..4e218798d8 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -42,8 +42,10 @@ _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
>  # Conversion between different compat versions can only really work
> -# with refcount_bits=16
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# with refcount_bits=16;
> +# we have explicit tests for data_file here, but the whole test does
> +# not work with it
> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
>  
>  echo
>  echo "=== Testing version downgrade with zero expansion ==="



> diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
> index ac0d2a9a3b..68e52a6402 100755
> --- a/tests/qemu-iotests/062
> +++ b/tests/qemu-iotests/062
> @@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  # We need zero clusters and snapshots
> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
>  
>  IMG_SIZE=64M
>  
Maybe split that test as well in the long run.
ACK.


> diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
> index 00eb80d89e..0fff3e3a52 100755
> --- a/tests/qemu-iotests/066
> +++ b/tests/qemu-iotests/066
> @@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  # We need zero clusters and snapshots
> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
Yet again, one small test case forcing the whole test to be skipped.
This should be split as well eventually.
>  
>  # Intentionally create an unaligned image
>  IMG_SIZE=$((64 * 1024 * 1024 + 512))



> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> index 926c79b37c..3bc6e719eb 100755
> --- a/tests/qemu-iotests/067
> +++ b/tests/qemu-iotests/067
> @@ -32,8 +32,10 @@ status=1	# failure is the default!
>  
>  _supported_fmt qcow2
>  _supported_proto file
> -# Because anything other than 16 would change the output of query-block
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# Because anything other than 16 would change the output of query-block,
> +# and external data files would change the output of
> +# query-named-block-ndoes
> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
OK. There probably is a way to filter that, but I don't know if this is worth it.
>  
>  do_run_qemu()
>  {



> diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> index 65650fca9a..c8748f5b02 100755
> --- a/tests/qemu-iotests/068
> +++ b/tests/qemu-iotests/068
> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # This tests qocw2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto generic
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
ACK
>  
>  IMG_SIZE=128K
>  



> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
> index 4e31943244..88faebcc1d 100755
> --- a/tests/qemu-iotests/071
> +++ b/tests/qemu-iotests/071
> @@ -39,6 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _require_drivers blkdebug blkverify
> +# blkdebug can only inject errors on bs->file, not on the data_file,
> +# so thie test does not work with external data files
s/thie/this
ACK. I guess that it would be too much trouble to extend it to 
inject errors on the data file as well.

> +_unsupported_imgopts data_file
>  
>  do_run_qemu()
>  {



> diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
> index e684b1b780..903dc9c9ab 100755
> --- a/tests/qemu-iotests/073
> +++ b/tests/qemu-iotests/073
> @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  _unsupported_proto vxhs
> +# External data files do not support compressed clusters
> +_unsupported_imgopts data_file
This test should be split as well eventually.

>  
>  CLUSTER_SIZE=64k
>  size=128M



> diff --git a/tests/qemu-iotests/074 b/tests/qemu-iotests/074
> index 62be89a0d9..db03edf0b0 100755
> --- a/tests/qemu-iotests/074
> +++ b/tests/qemu-iotests/074
> @@ -50,6 +50,8 @@ _compare()
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> +# blkdebug can only inject errors on bs->file
> +_unsupported_imgopts data_file
ACK, as above.
>  
>  # Setup test basic parameters
>  TEST_IMG2=$TEST_IMG.2



> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> index b1ecafb41e..a3d13c414e 100755
> --- a/tests/qemu-iotests/080
> +++ b/tests/qemu-iotests/080
> @@ -40,9 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# - Internal snapshots are (currently) impossible with refcount_bits=1
> +# - Internal snapshots are (currently) impossible with refcount_bits=1,
> +#   and generally impossible with external data files
>  # - This is generally a test for compat=1.1 images
> -_unsupported_imgopts 'refcount_bits=1[^0-9]' 'compat=0.10'
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file 'compat=0.10'
I would more say that the test is too hardcoded for more that exact
settings it expects. It is all right in this case IMHO.
ACK.

>  
>  header_size=104
>  



> diff --git a/tests/qemu-iotests/090 b/tests/qemu-iotests/090
> index 9f8cfbb80f..1246e4f910 100755
> --- a/tests/qemu-iotests/090
> +++ b/tests/qemu-iotests/090
> @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  _supported_fmt qcow2
>  _supported_proto file nfs
> +# External data files do not support compressed clusters
> +_unsupported_imgopts data_file
ACK
>  
>  IMG_SIZE=128K
>  



> diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
> index 700068b328..1e29d96b3d 100755
> --- a/tests/qemu-iotests/098
> +++ b/tests/qemu-iotests/098
> @@ -40,8 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  _supported_fmt qcow2
>  _supported_proto file
> -# The code path we want to test here only works for compat=1.1 images
> -_unsupported_imgopts 'compat=0.10'
> +# The code path we want to test here only works for compat=1.1 images;
> +# blkdebug can only inject errors on bs->file, so external data files
> +# do not work with this test
> +_unsupported_imgopts 'compat=0.10' data_file
ACK, but this is already 3rd test we loose. Maybe add to a TODO to extend blkdebug
to access data file as well.
>  
>  for event in l1_update empty_image_prepare reftable_update refblock_alloc; do
>  




> diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
> index b383c11e6a..65e8e92572 100755
> --- a/tests/qemu-iotests/099
> +++ b/tests/qemu-iotests/099
> @@ -46,8 +46,9 @@ _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
>  _supported_proto file
>  _supported_os Linux
>  _require_drivers blkdebug blkverify
> +# data_file would change the json:{} filenames
True but maybe still worth it to support the case?

>  _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
> -    "subformat=twoGbMaxExtentSparse"
> +    "subformat=twoGbMaxExtentSparse" data_file
>  
>  do_run_qemu()
>  {



> diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
> index 554b9de054..8c1ebe0443 100755
> --- a/tests/qemu-iotests/103
> +++ b/tests/qemu-iotests/103
> @@ -38,8 +38,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  _supported_fmt qcow2
>  _supported_proto file nfs
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
ACK.
The test also only needs the snapshot in a part of it, so maybe split as well
later.
>  
>  IMG_SIZE=64K
>  




> diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> index b0a6ae597b..6bbeb4f996 100755
> --- a/tests/qemu-iotests/108
> +++ b/tests/qemu-iotests/108
> @@ -41,8 +41,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# This test directly modifies a refblock so it relies on refcount_bits being 16
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# This test directly modifies a refblock so it relies on refcount_bits being 16;
> +# and the low-level modification it performs are not tuned for external data
> +# files
ACK
> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
>  
>  echo
>  echo '=== Repairing an image without any refcount table ==='



> diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
> index 6850225939..20ff5c224a 100755
> --- a/tests/qemu-iotests/112
> +++ b/tests/qemu-iotests/112
> @@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  # This test will set refcount_bits on its own which would conflict with the
> -# manual setting; compat will be overridden as well
> -_unsupported_imgopts refcount_bits 'compat=0.10'
> +# manual setting; compat will be overridden as well;
> +# and external data files do not work well with our refcount testing
ACK.
> +_unsupported_imgopts refcount_bits 'compat=0.10' data_file
>  
>  print_refcount_bits()
>  {



> diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
> index f90a744fc0..26104fff6c 100755
> --- a/tests/qemu-iotests/114
> +++ b/tests/qemu-iotests/114
> @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto generic
>  _unsupported_proto vxhs
> +# qcow2.py does not work too well with external data files
ACK, but should be fixed later.
> +_unsupported_imgopts data_file
>  
>  
>  TEST_IMG="$TEST_IMG.base" _make_test_img 64M



> diff --git a/tests/qemu-iotests/121 b/tests/qemu-iotests/121
> index 10db813d94..90ea0db737 100755
> --- a/tests/qemu-iotests/121
> +++ b/tests/qemu-iotests/121
> @@ -39,6 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> +# Refcount structures are used much differently with external data
> +# files
ACK.
> +_unsupported_imgopts data_file
>  
>  echo
>  echo '=== New refcount structures may not conflict with existing structures ==='



> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
> index 66ae9d5e78..7b0bc62a74 100755
> --- a/tests/qemu-iotests/138
> +++ b/tests/qemu-iotests/138
> @@ -40,6 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> +# These refcount calculations do not work with external data files
> +_unsupported_imgopts data_file
Thats why I don't like the hardcoded tests that much.
ACK.

>  
>  echo
>  echo '=== Check on an image with a multiple of 2^32 clusters ==='



> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index 3f27db71f2..5559df63a5 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -51,6 +51,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2 qed
>  _supported_proto generic
>  _unsupported_proto vxhs
> +# Copying files around with cp does not work with external data files
> +_unsupported_imgopts data_file
Another place to fix later I guess.
ACK.

>  
>  # Create source disk
>  TEST_IMG="$TEST_IMG.backing" _make_test_img 1M




> diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176
> index 50df4c00fa..117c8b6954 100755
> --- a/tests/qemu-iotests/176
> +++ b/tests/qemu-iotests/176
> @@ -47,8 +47,11 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# Persistent dirty bitmaps require compat=1.1
> -_unsupported_imgopts 'compat=0.10'
> +# Persistent dirty bitmaps require compat=1.1;
> +# Internal snapshots forbid using an external data file
> +# (they work with refcount_bits=1 here, though, because there actually
> +# is no data when creating the snapshot)
ACK
> +_unsupported_imgopts 'compat=0.10' data_file
>  
>  run_qemu()
>  {



> diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
> index 23ab0ce899..b05db68141 100755
> --- a/tests/qemu-iotests/191
> +++ b/tests/qemu-iotests/191
> @@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  
>  _supported_fmt qcow2
>  _supported_proto file
> +# An external data file would change the query-named-block-nodes output
> +_unsupported_imgopts data_file
ACK.
>  
>  size=64M
>  



> diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201
> index 7abf740fe4..3a458f18a0 100755
> --- a/tests/qemu-iotests/201
> +++ b/tests/qemu-iotests/201
> @@ -43,9 +43,9 @@ _supported_fmt qcow2
>  _supported_proto generic
>  _supported_os Linux
>  
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -# This was taken from test 080
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
ACK
>  
>  size=64M
>  _make_test_img $size



> diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
> index 21ec8a2ad8..0f2e61280a 100755
> --- a/tests/qemu-iotests/214
> +++ b/tests/qemu-iotests/214
> @@ -39,7 +39,8 @@ _supported_proto file
>  
>  # Repairing the corrupted image requires qemu-img check to store a
>  # refcount up to 3, which requires at least two refcount bits.
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# External data files do not support compressed clusters.
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
ACK
>  
>  
>  echo



> diff --git a/tests/qemu-iotests/217 b/tests/qemu-iotests/217
> index 58a78a6098..d89116ccad 100755
> --- a/tests/qemu-iotests/217
> +++ b/tests/qemu-iotests/217
> @@ -40,7 +40,8 @@ _supported_proto file
>  
>  # This test needs clusters with at least a refcount of 2 so that
>  # OFLAG_COPIED is not set.  refcount_bits=1 is therefore unsupported.
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# (As are external data files.)
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
>  
>  echo
>  echo '=== Simulating an I/O error during snapshot deletion ==='
ACK


> diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
> index 3769f372cb..a88c59152b 100755
> --- a/tests/qemu-iotests/220
> +++ b/tests/qemu-iotests/220
> @@ -37,8 +37,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# To use a different refcount width but 16 bits we need compat=1.1
> -_unsupported_imgopts 'compat=0.10'
> +# To use a different refcount width but 16 bits we need compat=1.1,
> +# and external data files do not support compressed clusters.
> +_unsupported_imgopts 'compat=0.10' data_file
>  
>  echo "== Creating huge file =="
>  
ACK.


> diff --git a/tests/qemu-iotests/243 b/tests/qemu-iotests/243
> index 3dc3b6a711..a61852f6d9 100755
> --- a/tests/qemu-iotests/243
> +++ b/tests/qemu-iotests/243
> @@ -40,8 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# External data files do not work with compat=0.10
> -_unsupported_imgopts 'compat=0.10'
> +# External data files do not work with compat=0.10, and because there
> +# is an explicit case for external data files here, we cannot allow
> +# the user to specify whether to use one
> +_unsupported_imgopts 'compat=0.10' data_file
>  
>  for mode in off metadata falloc full; do
>  



> diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
> index 13263292b0..0d1efee6ef 100755
> --- a/tests/qemu-iotests/244
> +++ b/tests/qemu-iotests/244
> @@ -41,8 +41,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> -# External data files do not work with compat=0.10
> -_unsupported_imgopts 'compat=0.10'
> +# External data files do not work with compat=0.10, and because we use
> +# our own external data file, we cannot let the user specify one
> +_unsupported_imgopts 'compat=0.10' data_file
>  
>  echo
>  echo "=== Create and open image with external data file ==="



> diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
> index 670cf19076..9bb6b94d74 100755
> --- a/tests/qemu-iotests/250
> +++ b/tests/qemu-iotests/250
> @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> +# This test does not make much sense with external data files
> +_unsupported_imgopts data_file
>  
>  # This test checks that qcow2_process_discards does not truncate a discard
>  # request > 2G.



> diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
> index eda45449d4..d2f0e5df59 100755
> --- a/tests/qemu-iotests/267
> +++ b/tests/qemu-iotests/267
> @@ -41,8 +41,9 @@ _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
>  
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +# Internal snapshots are (currently) impossible with refcount_bits=1,
> +# and generally impossible with external data files
> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
>  
>  do_run_qemu()
>  {


Overall I think that this patch reveals that we tend to have tests that have too many subtests
which force us to restrict the test to only the common configuration denominator of the
subtests, so it would be a good idea to go over these tests,
especially these that are blacklisted by this patch and try to split them.

Also there are too many tests that hardcode location of various metadata structures in the qcow2 file,
which make both the debug and addition of new features harder that it should be.
These tests should instead be able to query the qcow2 file for the actual metadata location
and cope with that. Having said that, its easier to say that than to do that, especially for tests
that try to do various corruptions, unusual metadata object sizes, etc. So I just want to note this.

Also there are various things like qcow2.py not working with external files that should be eventually
be fixed.

Overall I think that this patch is 100% good enough for now, so thanks a lot, and

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
Max Reitz Nov. 7, 2019, 11:36 a.m. UTC | #2 
On 06.11.19 16:52, Maxim Levitsky wrote:
> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/007 | 5 +++--
>>  tests/qemu-iotests/014 | 2 ++
>>  tests/qemu-iotests/015 | 5 +++--
>>  tests/qemu-iotests/026 | 5 ++++-
>>  tests/qemu-iotests/029 | 5 +++--
>>  tests/qemu-iotests/031 | 6 +++---
>>  tests/qemu-iotests/036 | 5 +++--
>>  tests/qemu-iotests/039 | 3 +++
>>  tests/qemu-iotests/046 | 2 ++
>>  tests/qemu-iotests/048 | 2 ++
>>  tests/qemu-iotests/051 | 5 +++--
>>  tests/qemu-iotests/058 | 5 +++--
>>  tests/qemu-iotests/060 | 6 ++++--
>>  tests/qemu-iotests/061 | 6 ++++--
>>  tests/qemu-iotests/062 | 2 +-
>>  tests/qemu-iotests/066 | 2 +-
>>  tests/qemu-iotests/067 | 6 ++++--
>>  tests/qemu-iotests/068 | 5 +++--
>>  tests/qemu-iotests/071 | 3 +++
>>  tests/qemu-iotests/073 | 2 ++
>>  tests/qemu-iotests/074 | 2 ++
>>  tests/qemu-iotests/080 | 5 +++--
>>  tests/qemu-iotests/090 | 2 ++
>>  tests/qemu-iotests/098 | 6 ++++--
>>  tests/qemu-iotests/099 | 3 ++-
>>  tests/qemu-iotests/103 | 5 +++--
>>  tests/qemu-iotests/108 | 6 ++++--
>>  tests/qemu-iotests/112 | 5 +++--
>>  tests/qemu-iotests/114 | 2 ++
>>  tests/qemu-iotests/121 | 3 +++
>>  tests/qemu-iotests/138 | 2 ++
>>  tests/qemu-iotests/156 | 2 ++
>>  tests/qemu-iotests/176 | 7 +++++--
>>  tests/qemu-iotests/191 | 2 ++
>>  tests/qemu-iotests/201 | 6 +++---
>>  tests/qemu-iotests/214 | 3 ++-
>>  tests/qemu-iotests/217 | 3 ++-
>>  tests/qemu-iotests/220 | 5 +++--
>>  tests/qemu-iotests/243 | 6 ++++--
>>  tests/qemu-iotests/244 | 5 +++--
>>  tests/qemu-iotests/250 | 2 ++
>>  tests/qemu-iotests/267 | 5 +++--
>>  42 files changed, 117 insertions(+), 52 deletions(-)

[...]

>> diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
>> index c44fcf91bb..646ecd593f 100755
>> --- a/tests/qemu-iotests/031
>> +++ b/tests/qemu-iotests/031
>> @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# We want to test compat=0.10, which does not support refcount widths
>> -# other than 16
>> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>> +# We want to test compat=0.10, which does not support external data
>> +# files or refcount widths other than 16
>> +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> 
> This is maybe another reason to split this test for compat=0.10 and for compat=1.1
> But still can be done later of course.

Hm, but I don’t really think this test is important for external data
files.  There is no I/O.

[...]

>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index bbaf0ef45b..512598421c 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# Only qcow2v3 and later supports feature bits
>> -_unsupported_imgopts 'compat=0.10'
>> +# Only qcow2v3 and later supports feature bits;
>> +# qcow2.py does not support external data files
> 
> Minor nitpick, maybe tag this with TODO or so. No need to do now.

Hm, well, and the same applies here.  (Just not a very important test.)

[...]

>> diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
>> index 4e03ead7b1..a066eec605 100755
>> --- a/tests/qemu-iotests/046
>> +++ b/tests/qemu-iotests/046
>> @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file
>> +# data_file does not support compressed clusters
>> +_unsupported_imgopts data_file
> This is a very nice test, which doesn't seem to  use compressed
> clusters that much. I think it should be split as well.
> No need to do this now of course, but maybe mark with TODO to 
> avoid loosing that info.

The other problem is that blkdebug doesn’t work so well with external
data files, so basically this whole test doesn’t work.

[...]

>> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
>> index a8feb76184..2af6b74b41 100755
>> --- a/tests/qemu-iotests/048
>> +++ b/tests/qemu-iotests/048
>> @@ -49,6 +49,8 @@ _compare()
>>  _supported_fmt raw qcow2 qed luks
>>  _supported_proto file
>>  _supported_os Linux
>> +# Using 'cp' is incompatible with external data files
>> +_unsupported_imgopts data_file
> You could compare the external files instead in theory *I think*.
> Another item on some TODO list I guess.

This is a test of qemu-img compare, not of the image format.  So it
doesn’t make much sense to me to compare the external files, and also it
should be completely sufficient to run this test only without external
data files.

[...]

>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>> index 9cd1d60d45..0053bad46a 100755
>> --- a/tests/qemu-iotests/051
>> +++ b/tests/qemu-iotests/051
>> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  # A compat=0.10 image is created in this test which does not support anything
>> -# other than refcount_bits=16
> Here also the compat=0.10 image is only a small part of the test,
> although it seems to get used later in the rest of the test,
> 
> so the test I think should be split so that rest of the test could run in all
> configurations. 

This too isn’t an image format test (specifically, there’s no I/O but
for the snapshotting test, which mostly uses a different image anyway),
so I don’t think it’s necessary to allow this test for data_file.

[...]

>> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
>> index 92243c2edd..8ad0d7a904 100755
>> --- a/tests/qemu-iotests/060
>> +++ b/tests/qemu-iotests/060
>> @@ -48,8 +48,10 @@ _filter_io_error()
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  _supported_os Linux
>> -# These tests only work for compat=1.1 images with refcount_bits=16
>> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>> +# These tests only work for compat=1.1 images without an external
>> +# data file with refcount_bits=16
> Yea, with all hardcoded offsets, that isn't going to work.

This isn’t about hardcoded offsets, it’s about the fact that the test
references data cluster that are part of the qcow2 file (i.e., not in an
external file).

[...]

>> diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
>> index ac0d2a9a3b..68e52a6402 100755
>> --- a/tests/qemu-iotests/062
>> +++ b/tests/qemu-iotests/062
>> @@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto generic
>>  # We need zero clusters and snapshots
>> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
>>  
>>  IMG_SIZE=64M
>>  
> Maybe split that test as well in the long run.

How would that be possible, though?  There is only a single test case here.

>> diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
>> index 00eb80d89e..0fff3e3a52 100755
>> --- a/tests/qemu-iotests/066
>> +++ b/tests/qemu-iotests/066
>> @@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto generic
>>  # We need zero clusters and snapshots
>> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
> Yet again, one small test case forcing the whole test to be skipped.
> This should be split as well eventually.

This I agree with.

>> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
>> index 926c79b37c..3bc6e719eb 100755
>> --- a/tests/qemu-iotests/067
>> +++ b/tests/qemu-iotests/067
>> @@ -32,8 +32,10 @@ status=1	# failure is the default!
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# Because anything other than 16 would change the output of query-block
>> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>> +# Because anything other than 16 would change the output of query-block,
>> +# and external data files would change the output of
>> +# query-named-block-ndoes
>> +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
> OK. There probably is a way to filter that, but I don't know if this is worth it.

Not really, because this again isn’t really a test of the image format.

[...]

>> diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
>> index e684b1b780..903dc9c9ab 100755
>> --- a/tests/qemu-iotests/073
>> +++ b/tests/qemu-iotests/073
>> @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto generic
>>  _unsupported_proto vxhs
>> +# External data files do not support compressed clusters
>> +_unsupported_imgopts data_file
> This test should be split as well eventually.

Hm, yes.  I don’t know if it can be fully split.  I think what would
work is a trimmed down copy just for external data files, but, well...

[...]

>> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
>> index b1ecafb41e..a3d13c414e 100755
>> --- a/tests/qemu-iotests/080
>> +++ b/tests/qemu-iotests/080
>> @@ -40,9 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  _supported_os Linux
>> -# - Internal snapshots are (currently) impossible with refcount_bits=1
>> +# - Internal snapshots are (currently) impossible with refcount_bits=1,
>> +#   and generally impossible with external data files
>>  # - This is generally a test for compat=1.1 images
>> -_unsupported_imgopts 'refcount_bits=1[^0-9]' 'compat=0.10'
>> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file 'compat=0.10'
> I would more say that the test is too hardcoded for more that exact
> settings it expects. It is all right in this case IMHO.
> ACK.

I suppose we’d want a different test for data file validation, if
anything, but I don’t think there is anything to validate there...

[...]

>> diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
>> index 700068b328..1e29d96b3d 100755
>> --- a/tests/qemu-iotests/098
>> +++ b/tests/qemu-iotests/098
>> @@ -40,8 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# The code path we want to test here only works for compat=1.1 images
>> -_unsupported_imgopts 'compat=0.10'
>> +# The code path we want to test here only works for compat=1.1 images;
>> +# blkdebug can only inject errors on bs->file, so external data files
>> +# do not work with this test
>> +_unsupported_imgopts 'compat=0.10' data_file
> ACK, but this is already 3rd test we loose. Maybe add to a TODO to extend blkdebug
> to access data file as well.

That won’t work, though.  The problem is that in the block graph,
blkdebug just exists between the format and the file node.  You’d need a
second instance above the external data file node, but then those
instances wouldn’t share data, and qcow2 only issues blkdebug events to
the file node.

One could make qcow2 duplicate all events to the data file, but then you
still wouldn’t share the same state in both instances.

In all, it would just be a mess.

>> diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
>> index b383c11e6a..65e8e92572 100755
>> --- a/tests/qemu-iotests/099
>> +++ b/tests/qemu-iotests/099
>> @@ -46,8 +46,9 @@ _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
>>  _supported_proto file
>>  _supported_os Linux
>>  _require_drivers blkdebug blkverify
>> +# data_file would change the json:{} filenames
> True but maybe still worth it to support the case?

I don’t think so, because this is specifically a test to check those
filenames.

[...]

>> diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
>> index 554b9de054..8c1ebe0443 100755
>> --- a/tests/qemu-iotests/103
>> +++ b/tests/qemu-iotests/103
>> @@ -38,8 +38,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file nfs
>> -# Internal snapshots are (currently) impossible with refcount_bits=1
>> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
>> +# Internal snapshots are (currently) impossible with refcount_bits=1,
>> +# and generally impossible with external data files
>> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
> ACK.
> The test also only needs the snapshot in a part of it, so maybe split as well
> later.

But this test too is just an interface test, so I don’t quite see the need.

[...]

>> diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
>> index f90a744fc0..26104fff6c 100755
>> --- a/tests/qemu-iotests/114
>> +++ b/tests/qemu-iotests/114
>> @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto generic
>>  _unsupported_proto vxhs
>> +# qcow2.py does not work too well with external data files
> ACK, but should be fixed later.

Probably, but I don’t think this test would benefit much from it.
(Because it isn’t too important to be able to run this with an external
data file)

[...]

>> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
>> index 66ae9d5e78..7b0bc62a74 100755
>> --- a/tests/qemu-iotests/138
>> +++ b/tests/qemu-iotests/138
>> @@ -40,6 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  _supported_os Linux
>> +# These refcount calculations do not work with external data files
>> +_unsupported_imgopts data_file
> Thats why I don't like the hardcoded tests that much.

Good point.  I was wondering what the problem was here because I was
sure it didn’t have anything to do with something hard-coded, and it
doesn’t.

The actual reason is simply that there is no refcounting for external
data files.  I’ll fix the comment.

>> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
>> index 3f27db71f2..5559df63a5 100755
>> --- a/tests/qemu-iotests/156
>> +++ b/tests/qemu-iotests/156
>> @@ -51,6 +51,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _supported_fmt qcow2 qed
>>  _supported_proto generic
>>  _unsupported_proto vxhs
>> +# Copying files around with cp does not work with external data files
>> +_unsupported_imgopts data_file
> Another place to fix later I guess.

I don’t know, this type of storage migration simply doesn’t work this
way with external data files.

Max
Maxim Levitsky Nov. 7, 2019, 3:19 p.m. UTC | #3 
On Thu, 2019-11-07 at 12:36 +0100, Max Reitz wrote:
> On 06.11.19 16:52, Maxim Levitsky wrote:
> > On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >  tests/qemu-iotests/007 | 5 +++--
> > >  tests/qemu-iotests/014 | 2 ++
> > >  tests/qemu-iotests/015 | 5 +++--
> > >  tests/qemu-iotests/026 | 5 ++++-
> > >  tests/qemu-iotests/029 | 5 +++--
> > >  tests/qemu-iotests/031 | 6 +++---
> > >  tests/qemu-iotests/036 | 5 +++--
> > >  tests/qemu-iotests/039 | 3 +++
> > >  tests/qemu-iotests/046 | 2 ++
> > >  tests/qemu-iotests/048 | 2 ++
> > >  tests/qemu-iotests/051 | 5 +++--
> > >  tests/qemu-iotests/058 | 5 +++--
> > >  tests/qemu-iotests/060 | 6 ++++--
> > >  tests/qemu-iotests/061 | 6 ++++--
> > >  tests/qemu-iotests/062 | 2 +-
> > >  tests/qemu-iotests/066 | 2 +-
> > >  tests/qemu-iotests/067 | 6 ++++--
> > >  tests/qemu-iotests/068 | 5 +++--
> > >  tests/qemu-iotests/071 | 3 +++
> > >  tests/qemu-iotests/073 | 2 ++
> > >  tests/qemu-iotests/074 | 2 ++
> > >  tests/qemu-iotests/080 | 5 +++--
> > >  tests/qemu-iotests/090 | 2 ++
> > >  tests/qemu-iotests/098 | 6 ++++--
> > >  tests/qemu-iotests/099 | 3 ++-
> > >  tests/qemu-iotests/103 | 5 +++--
> > >  tests/qemu-iotests/108 | 6 ++++--
> > >  tests/qemu-iotests/112 | 5 +++--
> > >  tests/qemu-iotests/114 | 2 ++
> > >  tests/qemu-iotests/121 | 3 +++
> > >  tests/qemu-iotests/138 | 2 ++
> > >  tests/qemu-iotests/156 | 2 ++
> > >  tests/qemu-iotests/176 | 7 +++++--
> > >  tests/qemu-iotests/191 | 2 ++
> > >  tests/qemu-iotests/201 | 6 +++---
> > >  tests/qemu-iotests/214 | 3 ++-
> > >  tests/qemu-iotests/217 | 3 ++-
> > >  tests/qemu-iotests/220 | 5 +++--
> > >  tests/qemu-iotests/243 | 6 ++++--
> > >  tests/qemu-iotests/244 | 5 +++--
> > >  tests/qemu-iotests/250 | 2 ++
> > >  tests/qemu-iotests/267 | 5 +++--
> > >  42 files changed, 117 insertions(+), 52 deletions(-)
> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
> > > index c44fcf91bb..646ecd593f 100755
> > > --- a/tests/qemu-iotests/031
> > > +++ b/tests/qemu-iotests/031
> > > @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# We want to test compat=0.10, which does not support refcount widths
> > > -# other than 16
> > > -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > > +# We want to test compat=0.10, which does not support external data
> > > +# files or refcount widths other than 16
> > > +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > 
> > This is maybe another reason to split this test for compat=0.10 and for compat=1.1
> > But still can be done later of course.
> 
> Hm, but I don’t really think this test is important for external data
> files.  There is no I/O.
I guess both yes and no, the external data file is a header extension as well.
I am looking at the tests from the point of view of someone that
doesn't know the qcow2 internally well yet, so I noted all the tests
that looked like they can still catch something.


> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index bbaf0ef45b..512598421c 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# Only qcow2v3 and later supports feature bits
> > > -_unsupported_imgopts 'compat=0.10'
> > > +# Only qcow2v3 and later supports feature bits;
> > > +# qcow2.py does not support external data files
> > 
> > Minor nitpick, maybe tag this with TODO or so. No need to do now.
> 
> Hm, well, and the same applies here.  (Just not a very important test.)
Same here, in theory external data file is a feature, and it could
'interact' with other features, but most likely you are right here as well.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
> > > index 4e03ead7b1..a066eec605 100755
> > > --- a/tests/qemu-iotests/046
> > > +++ b/tests/qemu-iotests/046
> > > @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > +# data_file does not support compressed clusters
> > > +_unsupported_imgopts data_file
> > 
> > This is a very nice test, which doesn't seem to  use compressed
> > clusters that much. I think it should be split as well.
> > No need to do this now of course, but maybe mark with TODO to 
> > avoid loosing that info.
> 
> The other problem is that blkdebug doesn’t work so well with external
> data files, so basically this whole test doesn’t work.
Yes, I see now that the test uses the blkdebug.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
> > > index a8feb76184..2af6b74b41 100755
> > > --- a/tests/qemu-iotests/048
> > > +++ b/tests/qemu-iotests/048
> > > @@ -49,6 +49,8 @@ _compare()
> > >  _supported_fmt raw qcow2 qed luks
> > >  _supported_proto file
> > >  _supported_os Linux
> > > +# Using 'cp' is incompatible with external data files
> > > +_unsupported_imgopts data_file
> > 
> > You could compare the external files instead in theory *I think*.
> > Another item on some TODO list I guess.
> 
> This is a test of qemu-img compare, not of the image format.  So it
> doesn’t make much sense to me to compare the external files, and also it
> should be completely sufficient to run this test only without external
> data files.
Yes but on the other hand, its is kind of nice to test that it can compare correctly
two qcow2 files which have external data files attached.


> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> > > index 9cd1d60d45..0053bad46a 100755
> > > --- a/tests/qemu-iotests/051
> > > +++ b/tests/qemu-iotests/051
> > > @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  # A compat=0.10 image is created in this test which does not support anything
> > > -# other than refcount_bits=16
> > 
> > Here also the compat=0.10 image is only a small part of the test,
> > although it seems to get used later in the rest of the test,
> > 
> > so the test I think should be split so that rest of the test could run in all
> > configurations. 
> 
> This too isn’t an image format test (specifically, there’s no I/O but
> for the snapshotting test, which mostly uses a different image anyway),
> so I don’t think it’s necessary to allow this test for data_file.
Same here, I agree but still what if there is some interaction between backing
file and data file?

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> > > index 92243c2edd..8ad0d7a904 100755
> > > --- a/tests/qemu-iotests/060
> > > +++ b/tests/qemu-iotests/060
> > > @@ -48,8 +48,10 @@ _filter_io_error()
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  _supported_os Linux
> > > -# These tests only work for compat=1.1 images with refcount_bits=16
> > > -_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > > +# These tests only work for compat=1.1 images without an external
> > > +# data file with refcount_bits=16
> > 
> > Yea, with all hardcoded offsets, that isn't going to work.
> 
> This isn’t about hardcoded offsets, it’s about the fact that the test
> references data cluster that are part of the qcow2 file (i.e., not in an
> external file).
That true too! In theory, the test could have asked the qcow2 image
where the data clusters are instead.

I think that in the long term, it would add a value to have some kind
of qcow2 specific test framework that would allow to query the qcow2 contents,
and manipulate it in the out of spec ways. Not now of course.



> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
> > > index ac0d2a9a3b..68e52a6402 100755
> > > --- a/tests/qemu-iotests/062
> > > +++ b/tests/qemu-iotests/062
> > > @@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > >  # We need zero clusters and snapshots
> > > -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
> > >  
> > >  IMG_SIZE=64M
> > >  
> > 
> > Maybe split that test as well in the long run.
> 
> How would that be possible, though?  There is only a single test case here.
Oops, I probably meant test 061, which might have a value to be split in 
two tests, one that upgrades/downgrades the version (and thus indeed can't use
any compat=1.1 features) and other test that works only on changing features of compat=1.1
images, where it could have used both different refcount bit settings and external data file.
(I suppose you can't take a regular qcow2 image and move its data clusters to an external data file, but still)


> 
> > > diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
> > > index 00eb80d89e..0fff3e3a52 100755
> > > --- a/tests/qemu-iotests/066
> > > +++ b/tests/qemu-iotests/066
> > > @@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > >  # We need zero clusters and snapshots
> > > -_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
> > 
> > Yet again, one small test case forcing the whole test to be skipped.
> > This should be split as well eventually.
> 
> This I agree with.
> 
> > > diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> > > index 926c79b37c..3bc6e719eb 100755
> > > --- a/tests/qemu-iotests/067
> > > +++ b/tests/qemu-iotests/067
> > > @@ -32,8 +32,10 @@ status=1	# failure is the default!
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# Because anything other than 16 would change the output of query-block
> > > -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > > +# Because anything other than 16 would change the output of query-block,
> > > +# and external data files would change the output of
> > > +# query-named-block-ndoes
> > > +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
> > 
> > OK. There probably is a way to filter that, but I don't know if this is worth it.
> 
> Not really, because this again isn’t really a test of the image format.
I think I agree on this test now. Still in theory it adds/removes lot of block devices,
thus the qcow2 driver now has to open/close the external file, which might reveal some bugs.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
> > > index e684b1b780..903dc9c9ab 100755
> > > --- a/tests/qemu-iotests/073
> > > +++ b/tests/qemu-iotests/073
> > > @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > >  _unsupported_proto vxhs
> > > +# External data files do not support compressed clusters
> > > +_unsupported_imgopts data_file
> > 
> > This test should be split as well eventually.
> 
> Hm, yes.  I don’t know if it can be fully split.  I think what would
> work is a trimmed down copy just for external data files, but, well...
That what I was thinking as well...

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> > > index b1ecafb41e..a3d13c414e 100755
> > > --- a/tests/qemu-iotests/080
> > > +++ b/tests/qemu-iotests/080
> > > @@ -40,9 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  _supported_os Linux
> > > -# - Internal snapshots are (currently) impossible with refcount_bits=1
> > > +# - Internal snapshots are (currently) impossible with refcount_bits=1,
> > > +#   and generally impossible with external data files
> > >  # - This is generally a test for compat=1.1 images
> > > -_unsupported_imgopts 'refcount_bits=1[^0-9]' 'compat=0.10'
> > > +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file 'compat=0.10'
> > 
> > I would more say that the test is too hardcoded for more that exact
> > settings it expects. It is all right in this case IMHO.
> > ACK.
> 
> I suppose we’d want a different test for data file validation, if
> anything, but I don’t think there is anything to validate there...
> 
Well the external data file as I understand is an extension on its own,
so in theory if you put some garbage there, it could reveal some bugs.
What if for example data file points to the same qcow2 file?
(or points through a symlink?) or it is NULL, or whatever.
The test for example tests for 'Invalid backing file size'
I guess that is what you mean here.

> [...]
> 
> > > diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
> > > index 700068b328..1e29d96b3d 100755
> > > --- a/tests/qemu-iotests/098
> > > +++ b/tests/qemu-iotests/098
> > > @@ -40,8 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# The code path we want to test here only works for compat=1.1 images
> > > -_unsupported_imgopts 'compat=0.10'
> > > +# The code path we want to test here only works for compat=1.1 images;
> > > +# blkdebug can only inject errors on bs->file, so external data files
> > > +# do not work with this test
> > > +_unsupported_imgopts 'compat=0.10' data_file
> > 
> > ACK, but this is already 3rd test we loose. Maybe add to a TODO to extend blkdebug
> > to access data file as well.
> 
> That won’t work, though.  The problem is that in the block graph,
> blkdebug just exists between the format and the file node.  You’d need a
> second instance above the external data file node, but then those
> instances wouldn’t share data, and qcow2 only issues blkdebug events to
> the file node.
> 
> One could make qcow2 duplicate all events to the data file, but then you
> still wouldn’t share the same state in both instances.
> 
> In all, it would just be a mess.

I agree. That is more or less what I suspected.
In theory you could make single blkdebug instance have 2 inputs and 2 outputs
so it could filter both IO channels. Interesting how we deal with backing files,
since even without data file the qcow2 driver accesses two block devices, and
with data file it can access 3.




> 
> > > diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
> > > index b383c11e6a..65e8e92572 100755
> > > --- a/tests/qemu-iotests/099
> > > +++ b/tests/qemu-iotests/099
> > > @@ -46,8 +46,9 @@ _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
> > >  _supported_proto file
> > >  _supported_os Linux
> > >  _require_drivers blkdebug blkverify
> > > +# data_file would change the json:{} filenames
> > 
> > True but maybe still worth it to support the case?
> 
> I don’t think so, because this is specifically a test to check those
> filenames.
All right!
> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
> > > index 554b9de054..8c1ebe0443 100755
> > > --- a/tests/qemu-iotests/103
> > > +++ b/tests/qemu-iotests/103
> > > @@ -38,8 +38,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file nfs
> > > -# Internal snapshots are (currently) impossible with refcount_bits=1
> > > -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> > > +# Internal snapshots are (currently) impossible with refcount_bits=1,
> > > +# and generally impossible with external data files
> > > +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
> > 
> > ACK.
> > The test also only needs the snapshot in a part of it, so maybe split as well
> > later.
> 
> But this test too is just an interface test, so I don’t quite see the need.
All right.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
> > > index f90a744fc0..26104fff6c 100755
> > > --- a/tests/qemu-iotests/114
> > > +++ b/tests/qemu-iotests/114
> > > @@ -39,6 +39,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto generic
> > >  _unsupported_proto vxhs
> > > +# qcow2.py does not work too well with external data files
> > 
> > ACK, but should be fixed later.
> 
> Probably, but I don’t think this test would benefit much from it.
> (Because it isn’t too important to be able to run this with an external
> data file)
Looking again, I agree, but like I mentioned earlier, it might be worth
it to have test that fuzzes the backing file location (if we don't have one that is).

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
> > > index 66ae9d5e78..7b0bc62a74 100755
> > > --- a/tests/qemu-iotests/138
> > > +++ b/tests/qemu-iotests/138
> > > @@ -40,6 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  _supported_os Linux
> > > +# These refcount calculations do not work with external data files
> > > +_unsupported_imgopts data_file
> > 
> > Thats why I don't like the hardcoded tests that much.
> 
> Good point.  I was wondering what the problem was here because I was
> sure it didn’t have anything to do with something hard-coded, and it
> doesn’t.
> 
> The actual reason is simply that there is no refcounting for external
> data files.  I’ll fix the comment.
Thanks!

> 
> > > diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> > > index 3f27db71f2..5559df63a5 100755
> > > --- a/tests/qemu-iotests/156
> > > +++ b/tests/qemu-iotests/156
> > > @@ -51,6 +51,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  _supported_fmt qcow2 qed
> > >  _supported_proto generic
> > >  _unsupported_proto vxhs
> > > +# Copying files around with cp does not work with external data files
> > > +_unsupported_imgopts data_file
> > 
> > Another place to fix later I guess.
> 
> I don’t know, this type of storage migration simply doesn’t work this
> way with external data files.
I am curios why? The test seems only to use external snapshots, so why it
would be different, other that copying the external files.

> 
> Max
> 

I want to note again, that I noted all the tests just in case,
most if not all of them probably indeed are best to be
blacklisted for external files.


Best regards,
	Maxim Levitsky
Max Reitz Nov. 7, 2019, 4:55 p.m. UTC | #4 
On 07.11.19 16:19, Maxim Levitsky wrote:
> On Thu, 2019-11-07 at 12:36 +0100, Max Reitz wrote:
>> On 06.11.19 16:52, Maxim Levitsky wrote:
>>> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/007 | 5 +++--
>>>>  tests/qemu-iotests/014 | 2 ++
>>>>  tests/qemu-iotests/015 | 5 +++--
>>>>  tests/qemu-iotests/026 | 5 ++++-
>>>>  tests/qemu-iotests/029 | 5 +++--
>>>>  tests/qemu-iotests/031 | 6 +++---
>>>>  tests/qemu-iotests/036 | 5 +++--
>>>>  tests/qemu-iotests/039 | 3 +++
>>>>  tests/qemu-iotests/046 | 2 ++
>>>>  tests/qemu-iotests/048 | 2 ++
>>>>  tests/qemu-iotests/051 | 5 +++--
>>>>  tests/qemu-iotests/058 | 5 +++--
>>>>  tests/qemu-iotests/060 | 6 ++++--
>>>>  tests/qemu-iotests/061 | 6 ++++--
>>>>  tests/qemu-iotests/062 | 2 +-
>>>>  tests/qemu-iotests/066 | 2 +-
>>>>  tests/qemu-iotests/067 | 6 ++++--
>>>>  tests/qemu-iotests/068 | 5 +++--
>>>>  tests/qemu-iotests/071 | 3 +++
>>>>  tests/qemu-iotests/073 | 2 ++
>>>>  tests/qemu-iotests/074 | 2 ++
>>>>  tests/qemu-iotests/080 | 5 +++--
>>>>  tests/qemu-iotests/090 | 2 ++
>>>>  tests/qemu-iotests/098 | 6 ++++--
>>>>  tests/qemu-iotests/099 | 3 ++-
>>>>  tests/qemu-iotests/103 | 5 +++--
>>>>  tests/qemu-iotests/108 | 6 ++++--
>>>>  tests/qemu-iotests/112 | 5 +++--
>>>>  tests/qemu-iotests/114 | 2 ++
>>>>  tests/qemu-iotests/121 | 3 +++
>>>>  tests/qemu-iotests/138 | 2 ++
>>>>  tests/qemu-iotests/156 | 2 ++
>>>>  tests/qemu-iotests/176 | 7 +++++--
>>>>  tests/qemu-iotests/191 | 2 ++
>>>>  tests/qemu-iotests/201 | 6 +++---
>>>>  tests/qemu-iotests/214 | 3 ++-
>>>>  tests/qemu-iotests/217 | 3 ++-
>>>>  tests/qemu-iotests/220 | 5 +++--
>>>>  tests/qemu-iotests/243 | 6 ++++--
>>>>  tests/qemu-iotests/244 | 5 +++--
>>>>  tests/qemu-iotests/250 | 2 ++
>>>>  tests/qemu-iotests/267 | 5 +++--
>>>>  42 files changed, 117 insertions(+), 52 deletions(-)
>>
>> [...]
>>
>>>> diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
>>>> index c44fcf91bb..646ecd593f 100755
>>>> --- a/tests/qemu-iotests/031
>>>> +++ b/tests/qemu-iotests/031
>>>> @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  # This tests qcow2-specific low-level functionality
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>> -# We want to test compat=0.10, which does not support refcount widths
>>>> -# other than 16
>>>> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>>> +# We want to test compat=0.10, which does not support external data
>>>> +# files or refcount widths other than 16
>>>> +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>>
>>> This is maybe another reason to split this test for compat=0.10 and for compat=1.1
>>> But still can be done later of course.
>>
>> Hm, but I don’t really think this test is important for external data
>> files.  There is no I/O.
> I guess both yes and no, the external data file is a header extension as well.

Yes, but the test already involves a header extension.

>>>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>>>> index bbaf0ef45b..512598421c 100755
>>>> --- a/tests/qemu-iotests/036
>>>> +++ b/tests/qemu-iotests/036
>>>> @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  # This tests qcow2-specific low-level functionality
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>> -# Only qcow2v3 and later supports feature bits
>>>> -_unsupported_imgopts 'compat=0.10'
>>>> +# Only qcow2v3 and later supports feature bits;
>>>> +# qcow2.py does not support external data files
>>>
>>> Minor nitpick, maybe tag this with TODO or so. No need to do now.
>>
>> Hm, well, and the same applies here.  (Just not a very important test.)
> Same here, in theory external data file is a feature, and it could
> 'interact' with other features, but most likely you are right here as well.

Well, but the test currently doesn’t involve any known feature bits.
It’s mostly about checking what our qcow2 driver does with unknown
feature bits.

(If it wanted to involve known feature bits, it could have easily used
e.g. the dirty feature.)

>>>> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
>>>> index a8feb76184..2af6b74b41 100755
>>>> --- a/tests/qemu-iotests/048
>>>> +++ b/tests/qemu-iotests/048
>>>> @@ -49,6 +49,8 @@ _compare()
>>>>  _supported_fmt raw qcow2 qed luks
>>>>  _supported_proto file
>>>>  _supported_os Linux
>>>> +# Using 'cp' is incompatible with external data files
>>>> +_unsupported_imgopts data_file
>>>
>>> You could compare the external files instead in theory *I think*.
>>> Another item on some TODO list I guess.
>>
>> This is a test of qemu-img compare, not of the image format.  So it
>> doesn’t make much sense to me to compare the external files, and also it
>> should be completely sufficient to run this test only without external
>> data files.
> Yes but on the other hand, its is kind of nice to test that it can compare correctly
> two qcow2 files which have external data files attached.

But then we need to compare the qcow2 files themselves, and that doesn’t
work precisely because of the cp.

(I suppose we could work around it by sprinkling a dose of option
parsing, qemu-img amend, etc. on it, but I don’t see the point.)

>>>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>>>> index 9cd1d60d45..0053bad46a 100755
>>>> --- a/tests/qemu-iotests/051
>>>> +++ b/tests/qemu-iotests/051
>>>> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>>  # A compat=0.10 image is created in this test which does not support anything
>>>> -# other than refcount_bits=16
>>>
>>> Here also the compat=0.10 image is only a small part of the test,
>>> although it seems to get used later in the rest of the test,
>>>
>>> so the test I think should be split so that rest of the test could run in all
>>> configurations. 
>>
>> This too isn’t an image format test (specifically, there’s no I/O but
>> for the snapshotting test, which mostly uses a different image anyway),
>> so I don’t think it’s necessary to allow this test for data_file.
> Same here, I agree but still what if there is some interaction between backing
> file and data file?

I’d suspect 244 is the better place to put such tests.

>>>> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
>>>> index 92243c2edd..8ad0d7a904 100755
>>>> --- a/tests/qemu-iotests/060
>>>> +++ b/tests/qemu-iotests/060
>>>> @@ -48,8 +48,10 @@ _filter_io_error()
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>>  _supported_os Linux
>>>> -# These tests only work for compat=1.1 images with refcount_bits=16
>>>> -_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>>> +# These tests only work for compat=1.1 images without an external
>>>> +# data file with refcount_bits=16
>>>
>>> Yea, with all hardcoded offsets, that isn't going to work.
>>
>> This isn’t about hardcoded offsets, it’s about the fact that the test
>> references data cluster that are part of the qcow2 file (i.e., not in an
>> external file).
> That true too! In theory, the test could have asked the qcow2 image
> where the data clusters are instead.

That isn’t what I mean.  This test lets a data cluster point into the
refcount structure; with an external data file, it wouldn’t actually
point into the metadata, because all data clusters must be in the
external file, so there’s no corruption then.

[...]

>>>> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
>>>> index b1ecafb41e..a3d13c414e 100755
>>>> --- a/tests/qemu-iotests/080
>>>> +++ b/tests/qemu-iotests/080
>>>> @@ -40,9 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>>  _supported_os Linux
>>>> -# - Internal snapshots are (currently) impossible with refcount_bits=1
>>>> +# - Internal snapshots are (currently) impossible with refcount_bits=1,
>>>> +#   and generally impossible with external data files
>>>>  # - This is generally a test for compat=1.1 images
>>>> -_unsupported_imgopts 'refcount_bits=1[^0-9]' 'compat=0.10'
>>>> +_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file 'compat=0.10'
>>>
>>> I would more say that the test is too hardcoded for more that exact
>>> settings it expects. It is all right in this case IMHO.
>>> ACK.
>>
>> I suppose we’d want a different test for data file validation, if
>> anything, but I don’t think there is anything to validate there...
>>
> Well the external data file as I understand is an extension on its own,
> so in theory if you put some garbage there, it could reveal some bugs.
> What if for example data file points to the same qcow2 file?
> (or points through a symlink?)

To my knowledge, we’ve kind of given up on detecting loops because you
can always make one if you really want to...

> or it is NULL, or whatever.
> The test for example tests for 'Invalid backing file size'
> I guess that is what you mean here.

Well, in any case it’d be something for a completely new test case.

>>>> diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
>>>> index 700068b328..1e29d96b3d 100755
>>>> --- a/tests/qemu-iotests/098
>>>> +++ b/tests/qemu-iotests/098
>>>> @@ -40,8 +40,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  
>>>>  _supported_fmt qcow2
>>>>  _supported_proto file
>>>> -# The code path we want to test here only works for compat=1.1 images
>>>> -_unsupported_imgopts 'compat=0.10'
>>>> +# The code path we want to test here only works for compat=1.1 images;
>>>> +# blkdebug can only inject errors on bs->file, so external data files
>>>> +# do not work with this test
>>>> +_unsupported_imgopts 'compat=0.10' data_file
>>>
>>> ACK, but this is already 3rd test we loose. Maybe add to a TODO to extend blkdebug
>>> to access data file as well.
>>
>> That won’t work, though.  The problem is that in the block graph,
>> blkdebug just exists between the format and the file node.  You’d need a
>> second instance above the external data file node, but then those
>> instances wouldn’t share data, and qcow2 only issues blkdebug events to
>> the file node.
>>
>> One could make qcow2 duplicate all events to the data file, but then you
>> still wouldn’t share the same state in both instances.
>>
>> In all, it would just be a mess.
> 
> I agree. That is more or less what I suspected.
> In theory you could make single blkdebug instance have 2 inputs and 2 outputs
> so it could filter both IO channels.

This wouldn’t work because no node is supposed to know its parents.  So
it can’t distinguish between the two “outputs”.

We’d need a shared state somewhere and then two different blkdebug
instances that access it with some runtime option to tell each one what
it’s supposed to do.  But that’s what I mean by “mess”. :-)

> Interesting how we deal with backing files,
> since even without data file the qcow2 driver accesses two block devices, and
> with data file it can access 3.

We simply never inject errors on the backing file. (AFAIK)

[...]

>>>> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
>>>> index 3f27db71f2..5559df63a5 100755
>>>> --- a/tests/qemu-iotests/156
>>>> +++ b/tests/qemu-iotests/156
>>>> @@ -51,6 +51,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>>  _supported_fmt qcow2 qed
>>>>  _supported_proto generic
>>>>  _unsupported_proto vxhs
>>>> +# Copying files around with cp does not work with external data files
>>>> +_unsupported_imgopts data_file
>>>
>>> Another place to fix later I guess.
>>
>> I don’t know, this type of storage migration simply doesn’t work this
>> way with external data files.
> I am curios why? The test seems only to use external snapshots, so why it
> would be different, other that copying the external files.

One would also need to adjust the qcow2 file to point to the copied data
file.

> I want to note again, that I noted all the tests just in case,
> most if not all of them probably indeed are best to be
> blacklisted for external files.

Yep, and it’s definitely good to have a kind of external perspective on
it. :-)

Max
diff mbox series

Patch

diff --git a/tests/qemu-iotests/007 b/tests/qemu-iotests/007
index 7d3544b479..160683adf8 100755
--- a/tests/qemu-iotests/007
+++ b/tests/qemu-iotests/007
@@ -41,8 +41,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 # refcount_bits must be at least 4 so we can create ten internal snapshots
-# (1 bit supports none, 2 bits support two, 4 bits support 14)
-_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]'
+# (1 bit supports none, 2 bits support two, 4 bits support 14);
+# snapshot are generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]' data_file
 
 echo
 echo "creating image"
diff --git a/tests/qemu-iotests/014 b/tests/qemu-iotests/014
index 2f728a1956..e1221c0fff 100755
--- a/tests/qemu-iotests/014
+++ b/tests/qemu-iotests/014
@@ -43,6 +43,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# Compression and snapshots do not work with external data files
+_unsupported_imgopts data_file
 
 TEST_OFFSETS="0 4294967296"
 TEST_OPS="writev read write readv"
diff --git a/tests/qemu-iotests/015 b/tests/qemu-iotests/015
index eec5387f3d..4d8effd0ae 100755
--- a/tests/qemu-iotests/015
+++ b/tests/qemu-iotests/015
@@ -40,8 +40,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 # actually any format that supports snapshots
 _supported_fmt qcow2
 _supported_proto generic
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 echo
 echo "creating image"
diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index 3430029ed6..a4aa74764f 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -49,7 +49,10 @@  _supported_cache_modes writethrough none
 # 32 and 64 bits do not work either, however, due to different leaked cluster
 # count on error.
 # Thus, the only remaining option is refcount_bits=16.
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+#
+# As for data_file, none of the refcount tests can work for it.
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' \
+    data_file
 
 echo "Errors while writing 128 kB"
 echo
diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
index 9254ede5e5..2161a4b87a 100755
--- a/tests/qemu-iotests/029
+++ b/tests/qemu-iotests/029
@@ -42,8 +42,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _unsupported_proto vxhs
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 offset_size=24
 offset_l1_size=36
diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
index c44fcf91bb..646ecd593f 100755
--- a/tests/qemu-iotests/031
+++ b/tests/qemu-iotests/031
@@ -40,9 +40,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
-# We want to test compat=0.10, which does not support refcount widths
-# other than 16
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# We want to test compat=0.10, which does not support external data
+# files or refcount widths other than 16
+_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 CLUSTER_SIZE=65536
 
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index bbaf0ef45b..512598421c 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -43,8 +43,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
-# Only qcow2v3 and later supports feature bits
-_unsupported_imgopts 'compat=0.10'
+# Only qcow2v3 and later supports feature bits;
+# qcow2.py does not support external data files
+_unsupported_imgopts 'compat=0.10' data_file
 
 echo
 echo === Image with unknown incompatible feature bit ===
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 99563bf126..ddce48ab47 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -44,6 +44,9 @@  _supported_proto file
 _supported_os Linux
 _default_cache_mode writethrough
 _supported_cache_modes writethrough
+# Some of these test cases expect no external data file so that all
+# clusters are part of the qcow2 image and refcounted
+_unsupported_imgopts data_file
 
 size=128M
 
diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index 4e03ead7b1..a066eec605 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -38,6 +38,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file
+# data_file does not support compressed clusters
+_unsupported_imgopts data_file
 
 CLUSTER_SIZE=64k
 size=128M
diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index a8feb76184..2af6b74b41 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -49,6 +49,8 @@  _compare()
 _supported_fmt raw qcow2 qed luks
 _supported_proto file
 _supported_os Linux
+# Using 'cp' is incompatible with external data files
+_unsupported_imgopts data_file
 
 # Remove once all tests are fixed to use TEST_IMG_FILE
 # correctly and common.rc sets it unconditionally
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 9cd1d60d45..0053bad46a 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -39,8 +39,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 # A compat=0.10 image is created in this test which does not support anything
-# other than refcount_bits=16
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# other than refcount_bits=16;
+# it also will not support an external data file
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index ed01115fa3..d5304bb404 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -56,8 +56,9 @@  _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 _require_command QEMU_NBD
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
 
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 92243c2edd..8ad0d7a904 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -48,8 +48,10 @@  _filter_io_error()
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# These tests only work for compat=1.1 images with refcount_bits=16
-_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# These tests only work for compat=1.1 images without an external
+# data file with refcount_bits=16
+_unsupported_imgopts 'compat=0.10' data_file \
+    'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 rt_offset=65536  # 0x10000 (XXX: just an assumption)
 rb_offset=131072 # 0x20000 (XXX: just an assumption)
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index b4076d8e8b..4e218798d8 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -42,8 +42,10 @@  _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 # Conversion between different compat versions can only really work
-# with refcount_bits=16
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# with refcount_bits=16;
+# we have explicit tests for data_file here, but the whole test does
+# not work with it
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
 
 echo
 echo "=== Testing version downgrade with zero expansion ==="
diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
index ac0d2a9a3b..68e52a6402 100755
--- a/tests/qemu-iotests/062
+++ b/tests/qemu-iotests/062
@@ -41,7 +41,7 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 # We need zero clusters and snapshots
-_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
 
 IMG_SIZE=64M
 
diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
index 00eb80d89e..0fff3e3a52 100755
--- a/tests/qemu-iotests/066
+++ b/tests/qemu-iotests/066
@@ -40,7 +40,7 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 # We need zero clusters and snapshots
-_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
 
 # Intentionally create an unaligned image
 IMG_SIZE=$((64 * 1024 * 1024 + 512))
diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
index 926c79b37c..3bc6e719eb 100755
--- a/tests/qemu-iotests/067
+++ b/tests/qemu-iotests/067
@@ -32,8 +32,10 @@  status=1	# failure is the default!
 
 _supported_fmt qcow2
 _supported_proto file
-# Because anything other than 16 would change the output of query-block
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# Because anything other than 16 would change the output of query-block,
+# and external data files would change the output of
+# query-named-block-ndoes
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 65650fca9a..c8748f5b02 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -39,8 +39,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qocw2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
 
 IMG_SIZE=128K
 
diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 4e31943244..88faebcc1d 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -39,6 +39,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _require_drivers blkdebug blkverify
+# blkdebug can only inject errors on bs->file, not on the data_file,
+# so thie test does not work with external data files
+_unsupported_imgopts data_file
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
index e684b1b780..903dc9c9ab 100755
--- a/tests/qemu-iotests/073
+++ b/tests/qemu-iotests/073
@@ -39,6 +39,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _unsupported_proto vxhs
+# External data files do not support compressed clusters
+_unsupported_imgopts data_file
 
 CLUSTER_SIZE=64k
 size=128M
diff --git a/tests/qemu-iotests/074 b/tests/qemu-iotests/074
index 62be89a0d9..db03edf0b0 100755
--- a/tests/qemu-iotests/074
+++ b/tests/qemu-iotests/074
@@ -50,6 +50,8 @@  _compare()
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# blkdebug can only inject errors on bs->file
+_unsupported_imgopts data_file
 
 # Setup test basic parameters
 TEST_IMG2=$TEST_IMG.2
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index b1ecafb41e..a3d13c414e 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -40,9 +40,10 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# - Internal snapshots are (currently) impossible with refcount_bits=1
+# - Internal snapshots are (currently) impossible with refcount_bits=1,
+#   and generally impossible with external data files
 # - This is generally a test for compat=1.1 images
-_unsupported_imgopts 'refcount_bits=1[^0-9]' 'compat=0.10'
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file 'compat=0.10'
 
 header_size=104
 
diff --git a/tests/qemu-iotests/090 b/tests/qemu-iotests/090
index 9f8cfbb80f..1246e4f910 100755
--- a/tests/qemu-iotests/090
+++ b/tests/qemu-iotests/090
@@ -38,6 +38,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file nfs
+# External data files do not support compressed clusters
+_unsupported_imgopts data_file
 
 IMG_SIZE=128K
 
diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
index 700068b328..1e29d96b3d 100755
--- a/tests/qemu-iotests/098
+++ b/tests/qemu-iotests/098
@@ -40,8 +40,10 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file
-# The code path we want to test here only works for compat=1.1 images
-_unsupported_imgopts 'compat=0.10'
+# The code path we want to test here only works for compat=1.1 images;
+# blkdebug can only inject errors on bs->file, so external data files
+# do not work with this test
+_unsupported_imgopts 'compat=0.10' data_file
 
 for event in l1_update empty_image_prepare reftable_update refblock_alloc; do
 
diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index b383c11e6a..65e8e92572 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -46,8 +46,9 @@  _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
 _supported_proto file
 _supported_os Linux
 _require_drivers blkdebug blkverify
+# data_file would change the json:{} filenames
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
-    "subformat=twoGbMaxExtentSparse"
+    "subformat=twoGbMaxExtentSparse" data_file
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103
index 554b9de054..8c1ebe0443 100755
--- a/tests/qemu-iotests/103
+++ b/tests/qemu-iotests/103
@@ -38,8 +38,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file nfs
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 IMG_SIZE=64K
 
diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
index b0a6ae597b..6bbeb4f996 100755
--- a/tests/qemu-iotests/108
+++ b/tests/qemu-iotests/108
@@ -41,8 +41,10 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# This test directly modifies a refblock so it relies on refcount_bits being 16
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# This test directly modifies a refblock so it relies on refcount_bits being 16;
+# and the low-level modification it performs are not tuned for external data
+# files
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
 
 echo
 echo '=== Repairing an image without any refcount table ==='
diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
index 6850225939..20ff5c224a 100755
--- a/tests/qemu-iotests/112
+++ b/tests/qemu-iotests/112
@@ -40,8 +40,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 # This test will set refcount_bits on its own which would conflict with the
-# manual setting; compat will be overridden as well
-_unsupported_imgopts refcount_bits 'compat=0.10'
+# manual setting; compat will be overridden as well;
+# and external data files do not work well with our refcount testing
+_unsupported_imgopts refcount_bits 'compat=0.10' data_file
 
 print_refcount_bits()
 {
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
index f90a744fc0..26104fff6c 100755
--- a/tests/qemu-iotests/114
+++ b/tests/qemu-iotests/114
@@ -39,6 +39,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _unsupported_proto vxhs
+# qcow2.py does not work too well with external data files
+_unsupported_imgopts data_file
 
 
 TEST_IMG="$TEST_IMG.base" _make_test_img 64M
diff --git a/tests/qemu-iotests/121 b/tests/qemu-iotests/121
index 10db813d94..90ea0db737 100755
--- a/tests/qemu-iotests/121
+++ b/tests/qemu-iotests/121
@@ -39,6 +39,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# Refcount structures are used much differently with external data
+# files
+_unsupported_imgopts data_file
 
 echo
 echo '=== New refcount structures may not conflict with existing structures ==='
diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 66ae9d5e78..7b0bc62a74 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -40,6 +40,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# These refcount calculations do not work with external data files
+_unsupported_imgopts data_file
 
 echo
 echo '=== Check on an image with a multiple of 2^32 clusters ==='
diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index 3f27db71f2..5559df63a5 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -51,6 +51,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2 qed
 _supported_proto generic
 _unsupported_proto vxhs
+# Copying files around with cp does not work with external data files
+_unsupported_imgopts data_file
 
 # Create source disk
 TEST_IMG="$TEST_IMG.backing" _make_test_img 1M
diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176
index 50df4c00fa..117c8b6954 100755
--- a/tests/qemu-iotests/176
+++ b/tests/qemu-iotests/176
@@ -47,8 +47,11 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# Persistent dirty bitmaps require compat=1.1
-_unsupported_imgopts 'compat=0.10'
+# Persistent dirty bitmaps require compat=1.1;
+# Internal snapshots forbid using an external data file
+# (they work with refcount_bits=1 here, though, because there actually
+# is no data when creating the snapshot)
+_unsupported_imgopts 'compat=0.10' data_file
 
 run_qemu()
 {
diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
index 23ab0ce899..b05db68141 100755
--- a/tests/qemu-iotests/191
+++ b/tests/qemu-iotests/191
@@ -43,6 +43,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file
+# An external data file would change the query-named-block-nodes output
+_unsupported_imgopts data_file
 
 size=64M
 
diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201
index 7abf740fe4..3a458f18a0 100755
--- a/tests/qemu-iotests/201
+++ b/tests/qemu-iotests/201
@@ -43,9 +43,9 @@  _supported_fmt qcow2
 _supported_proto generic
 _supported_os Linux
 
-# Internal snapshots are (currently) impossible with refcount_bits=1
-# This was taken from test 080
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 size=64M
 _make_test_img $size
diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2ad8..0f2e61280a 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -39,7 +39,8 @@  _supported_proto file
 
 # Repairing the corrupted image requires qemu-img check to store a
 # refcount up to 3, which requires at least two refcount bits.
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# External data files do not support compressed clusters.
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 
 echo
diff --git a/tests/qemu-iotests/217 b/tests/qemu-iotests/217
index 58a78a6098..d89116ccad 100755
--- a/tests/qemu-iotests/217
+++ b/tests/qemu-iotests/217
@@ -40,7 +40,8 @@  _supported_proto file
 
 # This test needs clusters with at least a refcount of 2 so that
 # OFLAG_COPIED is not set.  refcount_bits=1 is therefore unsupported.
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# (As are external data files.)
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 echo
 echo '=== Simulating an I/O error during snapshot deletion ==='
diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
index 3769f372cb..a88c59152b 100755
--- a/tests/qemu-iotests/220
+++ b/tests/qemu-iotests/220
@@ -37,8 +37,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# To use a different refcount width but 16 bits we need compat=1.1
-_unsupported_imgopts 'compat=0.10'
+# To use a different refcount width but 16 bits we need compat=1.1,
+# and external data files do not support compressed clusters.
+_unsupported_imgopts 'compat=0.10' data_file
 
 echo "== Creating huge file =="
 
diff --git a/tests/qemu-iotests/243 b/tests/qemu-iotests/243
index 3dc3b6a711..a61852f6d9 100755
--- a/tests/qemu-iotests/243
+++ b/tests/qemu-iotests/243
@@ -40,8 +40,10 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# External data files do not work with compat=0.10
-_unsupported_imgopts 'compat=0.10'
+# External data files do not work with compat=0.10, and because there
+# is an explicit case for external data files here, we cannot allow
+# the user to specify whether to use one
+_unsupported_imgopts 'compat=0.10' data_file
 
 for mode in off metadata falloc full; do
 
diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index 13263292b0..0d1efee6ef 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -41,8 +41,9 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-# External data files do not work with compat=0.10
-_unsupported_imgopts 'compat=0.10'
+# External data files do not work with compat=0.10, and because we use
+# our own external data file, we cannot let the user specify one
+_unsupported_imgopts 'compat=0.10' data_file
 
 echo
 echo "=== Create and open image with external data file ==="
diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
index 670cf19076..9bb6b94d74 100755
--- a/tests/qemu-iotests/250
+++ b/tests/qemu-iotests/250
@@ -39,6 +39,8 @@  trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# This test does not make much sense with external data files
+_unsupported_imgopts data_file
 
 # This test checks that qcow2_process_discards does not truncate a discard
 # request > 2G.
diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index eda45449d4..d2f0e5df59 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -41,8 +41,9 @@  _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 do_run_qemu()
 {