qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressi


From: Max Reitz
Subject: Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
Date: Fri, 16 Jul 2021 16:46:55 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

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 <vsementsov@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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]