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

From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@openvz.org, jsnow@redhat.com,
	qemu-devel@nongnu.org
Subject: Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
Date: Fri, 16 Jul 2021 16:46:55 +0200	[thread overview]
Message-ID: <0a0b0ce5-3a9a-ca25-7f2f-9976b76cd1f6@redhat.com> (raw)
In-Reply-To: <a8c71445-a7cb-7d10-6b24-449861f9d39f@virtuozzo.com>

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