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

[Max Reitz]

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 <address@hidden>
>> ---
>>  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

signature.asc
Description: OpenPGP digital signature