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

[Hanna Reitz]

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.