Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, kwolf@redhat.com, den@openvz.org,
	jsnow@redhat.com
Subject: Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
Date: Fri, 16 Jul 2021 17:24:26 +0300	[thread overview]
Message-ID: <a8c71445-a7cb-7d10-6b24-449861f9d39f@virtuozzo.com> (raw)
In-Reply-To: <d983ae4a-dce5-35d3-1c63-7285651eae56@redhat.com>

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?

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.

(ohh, I'd deprecate bash tests too.. But that's kind of taste)

-- 
Best regards,
Vladimir