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

[Hanna Reitz <hreitz@redhat.com>]

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.

Hanna