Re: [PATCH 2/2] iotests/303: Check for zstd support

[Hanna Reitz <hreitz@redhat.com>]

On 02.03.22 12:31, Vladimir Sementsov-Ogievskiy wrote:
> 02.03.2022 14:09, Hanna Reitz wrote:
>> On 22.02.22 16:55, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.02.2022 20:08, Hanna Reitz wrote:
>>>> 303 runs two test cases, one of which requires zstd support.
>>>> Unfortunately, given that this is not a unittest-style test, we cannot
>>>> easily skip that single case, and instead can only skip the whole 
>>>> test.
>>>>
>>>> (Alternatively, we could split this test into a zlib and a zstd part,
>>>> but that seems excessive, given that this test is not in auto and thus
>>>> likely only run by developers who have zstd support compiled in.)
>>>>
>>>> Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression 
>>>> type")
>>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/303 | 15 +++++++++++----
>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
>>>> index 16c2e10827..5a3efb4ba3 100755
>>>> --- a/tests/qemu-iotests/303
>>>> +++ b/tests/qemu-iotests/303
>>>> @@ -21,7 +21,8 @@
>>>>     import iotests
>>>>   import subprocess
>>>> -from iotests import qemu_img_create, qemu_io, file_path, log, 
>>>> filter_qemu_io
>>>> +from iotests import notrun, qemu_img_pipe_and_status, qemu_io, 
>>>> file_path, \
>>>> +        log, filter_qemu_io
>>>>     iotests.script_initialize(supported_fmts=['qcow2'],
>>>> unsupported_imgopts=['refcount_bits', 'compat'])
>>>> @@ -55,9 +56,15 @@ def add_bitmap(num, begin, end, disabled):
>>>>       def test(compression_type: str, json_output: bool) -> None:
>>>> -    qemu_img_create('-f', iotests.imgfmt,
>>>> -                    '-o', f'compression_type={compression_type}',
>>>> -                    disk, '10M')
>>>> +    opts = f'compression_type={compression_type}'
>>>> +    output, status = qemu_img_pipe_and_status('create',
>>>> +                                              '-f', iotests.imgfmt,
>>>> +                                              '-o', opts,
>>>> +                                              disk, '10M')
>>>> +    if status == 1 and \
>>>> +            "'compression-type' does not accept value 'zstd'" in 
>>>> output:
>>>> +        notrun('zstd compression not supported')
>>>> +
>>>
>>> Could we make from it a function in iotests.py like 
>>> has_working_luks() / verify_working_luks() ?
>>>
>>> Then in this test we'll simply have at start iotests.verify_zstd(), 
>>> and in previous test we'll do something like "comp_type = 'zstd' if 
>>> iotests.has_zstd() else 'zlib'" in classes that wants zstd
>>
>> Well, to be honest, I deliberately didn’t do that.  We have qemu-img 
>> create calls in both of these places, where we can simply check the 
>> returned error string for free, basically. Adding a function for this 
>> check doesn’t really reduce complexity (now), because adding such 
>> functions probably has some more LoC than just checking the returned 
>> error string in these two places (I say “now” because it’s possible 
>> that more places are added in the future, though, of course).  With 
>> zstd support compiled in, such functions would effectively also just 
>> add a superfluous qemu-img create call wherever they’re used.
>>
>> I’m not saying I’m right about that judgment, because it does feel a 
>> bit like premature optimization.  Just saying why I didn’t add such 
>> functions.
>>
>> So, with that said, would you still prefer such iotests.py functions? 
>> I’ll absolutely add them in v2 if you do.
>>
>
> Yes, I think better is to have generic function for such thing:
>
> 1. These modifications makes tests more complicated to read (OK only a 
> bit more complicated, but still). And they break common practice that, 
> necessary features are controlled by verify_* functions, parameters to 
> iotests.main and so on.

OK.

> 2. I understand your doubt that we just do an extra qemu_img call. But 
> for sure there are solutions if we want:
>
>  - At least, we can call verify_* function once and cache its result 
> for the whole test run.
>  - More, we can group several verify_* things into one qemu process 
> call, to do different checks by qmp commands in context of that one 
> qemu process test run, before running real tests.
>  - And about zstd support - we can even generate a file with some 
> environment variables as we do before (remember 
> tests/qemu-iotests/common.env.in), and meson will set a CONFIG_ZSTD 
> variable in it, and then in iotests.py we'll know, do we have zstd 
> support without any extra qemu-img call. Hmm, and probably when we run 
> tests through make check, we already have this information (I mean 
> CONFIG_ZSTD).

I thought of similar stuff, but that seems so complicated that it kind 
of breaks argument 1.  Caching in iotests.py is simple enough[1], but 
caching across different test instances isn’t really.

[1] Well.  If I add it to has_qcow2_zstd_compression(), then the 
question will arise why I’m not adding it to has_working_luks() also.  
So that way, just caching in iotests.py would make this series already 
more complicated than it was here in v1.  Which is why I’d rather not 
add any caching at all.