[PATCH 0/2] iotests: Check for zstd support

[PATCH 0/2] iotests: Check for zstd support

[Hanna Reitz]

Hi,

We have two tests (as far as I know) that use compression_type=zstd for
qcow2 but do not check whether that is actually supported.  Thomas
reported this for 065, but it’s also the case for 303.

This series makes these tests be skipped when qemu-img reports this
compression type to not be supported.


Hanna Reitz (2):
  iotests/065: Check for zstd support
  iotests/303: Check for zstd support

 tests/qemu-iotests/065 | 11 ++++++++---
 tests/qemu-iotests/303 | 15 +++++++++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] iotests/065: Check for zstd support

[Hanna Reitz]

Some test cases run in iotest 065 require zstd support.  Skip them if
qemu-img reports it not to be available.

Reported-by: Thomas Huth <thuth@redhat.com>
Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/065 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dad..b68df84642 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img_pipe, qemu_img_pipe_and_status
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
     def setUp(self):
         if self.img_options is None:
             self.skipTest('Skipping abstract test class')
-        qemu_img('create', '-f', iotests.imgfmt, '-o', self.img_options,
-                 test_img, '128K')
+        output, status = qemu_img_pipe_and_status('create',
+                                                  '-f', iotests.imgfmt,
+                                                  '-o', self.img_options,
+                                                  test_img, '128K')
+        if status == 1 and \
+                "'compression-type' does not accept value 'zstd'" in output:
+            self.case_skip('zstd compression not supported')
 
     def tearDown(self):
         os.remove(test_img)
-- 
2.34.1

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

[Hanna Reitz]

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')
+
     add_bitmap(1, 0, 6, False)
     add_bitmap(2, 6, 8, True)
 
-- 
2.34.1

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

[Thomas Huth]

On 21/02/2022 18.08, Hanna Reitz wrote:
> Some test cases run in iotest 065 require zstd support.  Skip them if
> qemu-img reports it not to be available.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>   tests/qemu-iotests/065 | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
> index f7c1b68dad..b68df84642 100755
> --- a/tests/qemu-iotests/065
> +++ b/tests/qemu-iotests/065
> @@ -24,7 +24,7 @@ import os
>   import re
>   import json
>   import iotests
> -from iotests import qemu_img, qemu_img_pipe
> +from iotests import qemu_img_pipe, qemu_img_pipe_and_status
>   import unittest
>   
>   test_img = os.path.join(iotests.test_dir, 'test.img')
> @@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
>       def setUp(self):
>           if self.img_options is None:
>               self.skipTest('Skipping abstract test class')
> -        qemu_img('create', '-f', iotests.imgfmt, '-o', self.img_options,
> -                 test_img, '128K')
> +        output, status = qemu_img_pipe_and_status('create',
> +                                                  '-f', iotests.imgfmt,
> +                                                  '-o', self.img_options,
> +                                                  test_img, '128K')
> +        if status == 1 and \
> +                "'compression-type' does not accept value 'zstd'" in output:
> +            self.case_skip('zstd compression not supported')
>   
>       def tearDown(self):
>           os.remove(test_img)

Thanks, that fixes 065 for me!

Tested-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 21/02/2022 18.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')
> +
>       add_bitmap(1, 0, 6, False)
>       add_bitmap(2, 6, 8, True)
>   

303 was failing on my system, too, but I did not get around to investigate 
on that one yet - so thanks for fixing this pro-actively! Now the test is 
correctly skipped on my system.

Tested-by: Thomas Huth <thuth@redhat.com>

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

[Vladimir Sementsov-Ogievskiy]

21.02.2022 20:08, Hanna Reitz wrote:
> Some test cases run in iotest 065 require zstd support.  Skip them if
> qemu-img reports it not to be available.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>   tests/qemu-iotests/065 | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
> index f7c1b68dad..b68df84642 100755
> --- a/tests/qemu-iotests/065
> +++ b/tests/qemu-iotests/065
> @@ -24,7 +24,7 @@ import os
>   import re
>   import json
>   import iotests
> -from iotests import qemu_img, qemu_img_pipe
> +from iotests import qemu_img_pipe, qemu_img_pipe_and_status
>   import unittest
>   
>   test_img = os.path.join(iotests.test_dir, 'test.img')
> @@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
>       def setUp(self):
>           if self.img_options is None:
>               self.skipTest('Skipping abstract test class')
> -        qemu_img('create', '-f', iotests.imgfmt, '-o', self.img_options,
> -                 test_img, '128K')
> +        output, status = qemu_img_pipe_and_status('create',
> +                                                  '-f', iotests.imgfmt,
> +                                                  '-o', self.img_options,
> +                                                  test_img, '128K')
> +        if status == 1 and \
> +                "'compression-type' does not accept value 'zstd'" in output:
> +            self.case_skip('zstd compression not supported')
>   
>       def tearDown(self):
>           os.remove(test_img)


Hmm. Actually you fix the commit 12a936171d71f in a meaning that test passes now. But that only stresses the fact that 12a936171d71f brings a degradation in test-count for no-zstd builds. Is it bad?
The simplest solution is to duplicate TestQCow3NotLazy and TestQCow3LazyQMP with s/zstd/zlib/.. More complicated is to add generic function to detect is zstd supported or not, and use zstd in TestQCow3NotLazy and TestQCow3NotLazy only if zstd is supported (and otherwise use zlib).

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

>       add_bitmap(1, 0, 6, False)
>       add_bitmap(2, 6, 8, True)
>   



-- 
Best regards,
Vladimir

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

[Hanna Reitz]

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

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

[Hanna Reitz]

On 22.02.22 16:44, Vladimir Sementsov-Ogievskiy wrote:
> 21.02.2022 20:08, Hanna Reitz wrote:
>> Some test cases run in iotest 065 require zstd support.  Skip them if
>> qemu-img reports it not to be available.
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/065 | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
>> index f7c1b68dad..b68df84642 100755
>> --- a/tests/qemu-iotests/065
>> +++ b/tests/qemu-iotests/065
>> @@ -24,7 +24,7 @@ import os
>>   import re
>>   import json
>>   import iotests
>> -from iotests import qemu_img, qemu_img_pipe
>> +from iotests import qemu_img_pipe, qemu_img_pipe_and_status
>>   import unittest
>>     test_img = os.path.join(iotests.test_dir, 'test.img')
>> @@ -35,8 +35,13 @@ class TestImageInfoSpecific(iotests.QMPTestCase):
>>       def setUp(self):
>>           if self.img_options is None:
>>               self.skipTest('Skipping abstract test class')
>> -        qemu_img('create', '-f', iotests.imgfmt, '-o', 
>> self.img_options,
>> -                 test_img, '128K')
>> +        output, status = qemu_img_pipe_and_status('create',
>> +                                                  '-f', iotests.imgfmt,
>> +                                                  '-o', 
>> self.img_options,
>> +                                                  test_img, '128K')
>> +        if status == 1 and \
>> +                "'compression-type' does not accept value 'zstd'" in 
>> output:
>> +            self.case_skip('zstd compression not supported')
>>         def tearDown(self):
>>           os.remove(test_img)
>
>
> Hmm. Actually you fix the commit 12a936171d71f in a meaning that test 
> passes now. But that only stresses the fact that 12a936171d71f brings 
> a degradation in test-count for no-zstd builds. Is it bad?

Probably not really, considering that no-zstd builds shouldn’t be 
happening very often.  But since it’s something that can absolutely be 
worked around, it should be worked around. :)

> The simplest solution is to duplicate TestQCow3NotLazy and 
> TestQCow3LazyQMP with s/zstd/zlib/.. More complicated is to add 
> generic function to detect is zstd supported or not, and use zstd in 
> TestQCow3NotLazy and TestQCow3NotLazy only if zstd is supported (and 
> otherwise use zlib).

I think using zstd only if zstd is supported makes the most sense so we 
don’t increase the number of test cases for the more common case where 
zstd is compiled in.

Hanna

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

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.