[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 20/21] iotests: Disable data_file where it cannot be used
|
From: |
Maxim Levitsky |
|
Subject: |
Re: [PATCH v2 20/21] iotests: Disable data_file where it cannot be used |
|
Date: |
Thu, 07 Nov 2019 17:19:26 +0200 |
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 <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.
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