On 16.07.21 16:24, Vladimir Sementsov-Ogievskiy wrote:
16.07.2021 15:38, Max Reitz wrote:
Subject: s/compressiont_type/compression_type/
On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
Like it is done in iotests.py in qemu_img_create_prepare_args(), let's
not follow compression_type=zstd of IMGOPTS if test creates image in
old format.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
tests/qemu-iotests/common.rc | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/tests/qemu-iotests/common.rc
b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..4cae5b2d70 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -438,6 +438,14 @@ _make_test_img()
backing_file=$param
continue
elif $opts_param; then
+ if [[ "$param" == *"compat=0"* ]]; then
Like in patch 2, probably should be 0.10, and account for “v2”.
+ # If user specified zstd compression type in
IMGOPTS, this will
+ # just not work. So, let's imply forcing zlib
compression when
+ # test creates image in old version of the format.
+ # Similarly works qemu_img_create_prepare_args() in
iotests.py
+ optstr=$(echo "$optstr" | $SED -e
's/compression_type=\w\+//')
What about the surrounding comma, if compression_type is just one
option among others? Do we need something like
$SED -e 's/,compression_type=\w\+//' -e 's/compression_type=\w\+,\?//'
?
Agree
+ optstr=$(_optstr_add "$optstr"
"compression_type=zlib")
As the comment says, this is for compression_type in $IMGOPTS and
then compat=0.10 in the parameters. It won’t work if you have e.g.
“_make_test_img -o compat=0.10,compression_type=zstd”, because then
this generates the optstr
“$IMGOPTS,compression_type=zlib,compat=0.10,compression_type=zstd”.
Not sure if we want to care about this case, but, well...
Then there’s the case where I have compat=0.10 in $IMGOPTS, and the
test wants to use compression_type=zstd. I think it’s correct not to
replace compression_type=zstd then, because the test should be
skipped for compat=0.10 in $IMGOPTS. But that’s not what happens in
the iotest.py version (qemu_img_create_prepare_args()), so I wonder
whether the latter should be made to match this behavior here, if in
any way possible.
Now that I think about it more, I begin to wonder more...
So this code doesn’t explicitly handle compression_type only in
$IMGOPTS. If you have
_make_test_img -o compression_type=zstd,compat=0.10
It’ll still keep the compression_type=zstd. However, for
_make_test_img -o compression_type=zstd -o compat=0.10
It’ll replace it by zlib.
So perhaps we should explicitly scan for compression_type only in
$IMGOPTS and then drop it from the optstr if compat=0.10 is in the
_make_test_img's -o options.
But thinking further, this is not how $IMGOPTS work. So far they
aren’t advisory, they are mandatory. If a test cannot work with
something in $IMGOPTS, it has to be skipped. Like, when you have
compat=0.10 in $IMGOPTS, I don’t want to run tests that use v3
features and have them just create v3 images for those tests.
So my impression is that you’re giving compression_type special
treatment here, and I don’t know why exactly. Tests that create v2
images should just have compression_type be an unsupported_imgopt.
Max
Hmm.. I have better idea: deprecate v2 and drop all iotest support for
it now :)) What do you think?
I haven’t yet understood the appeal of deprecating v2, because basically
all code is shared between v2 and v3. So I don’t really see the appeal
in dropping iotest support for it either.
At least, this doesn’t appear like a better idea than to add
_unsupported_imgopts where needed (in fact, _unsupported_imgopts should
already be there for other v3-only options like lazy_refcounts).
If not, than instead of this patch, we just should skip all tests that
don't support compression_type=zstd due to using old version.. This
means that we will skip some test-cases that can work with zstd just
because we can't skip separate test cases in bash tests.
The standard procedure for this is to have a quick look whether we
actually lose (relevant) coverage for this imgopt if we skip the test
(usually not), and if so, split that part out into a new file. But
again, usually nothing of value is lost, so nothing is split off.
(ohh, I'd deprecate bash tests too.. But that's kind of taste)
So far I don’t think there is a pressing reason why bash tests would be
harder to support than Python tests, and so the effort to port all bash
tests to Python seems much more difficult to me than having to duplicate
meta-work like this.
(And in fact, as an example, I found it much easier to have bash tests
support -o data_file than the Python tests, not least because the bash
tests at least kind of all work the same, whereas we have like three
variants of Python test styles. Which brings me back to “bash tests
don’t support separately skippable test cases” – well, same for most of
our Python tests, I guess.)
Max