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

[Vladimir Sementsov-Ogievskiy]

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.

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).



--
Best regards,
Vladimir