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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12476 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]