* [PATCH 0/2] iotests: Check for zstd support @ 2022-02-21 17:08 Hanna Reitz 2022-02-21 17:08 ` [PATCH 1/2] iotests/065: " Hanna Reitz 2022-02-21 17:08 ` [PATCH 2/2] iotests/303: " Hanna Reitz 0 siblings, 2 replies; 11+ messages in thread From: Hanna Reitz @ 2022-02-21 17:08 UTC (permalink / raw) To: qemu-block Cc: Thomas Huth, Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] iotests/065: Check for zstd support 2022-02-21 17:08 [PATCH 0/2] iotests: Check for zstd support Hanna Reitz @ 2022-02-21 17:08 ` Hanna Reitz 2022-02-21 17:20 ` Thomas Huth 2022-02-22 15:44 ` Vladimir Sementsov-Ogievskiy 2022-02-21 17:08 ` [PATCH 2/2] iotests/303: " Hanna Reitz 1 sibling, 2 replies; 11+ messages in thread From: Hanna Reitz @ 2022-02-21 17:08 UTC (permalink / raw) To: qemu-block Cc: Thomas Huth, Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iotests/065: Check for zstd support 2022-02-21 17:08 ` [PATCH 1/2] iotests/065: " Hanna Reitz @ 2022-02-21 17:20 ` Thomas Huth 2022-02-22 15:44 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 11+ messages in thread From: Thomas Huth @ 2022-02-21 17:20 UTC (permalink / raw) To: Hanna Reitz, qemu-block Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iotests/065: Check for zstd support 2022-02-21 17:08 ` [PATCH 1/2] iotests/065: " Hanna Reitz 2022-02-21 17:20 ` Thomas Huth @ 2022-02-22 15:44 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:14 ` Hanna Reitz 1 sibling, 1 reply; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2022-02-22 15:44 UTC (permalink / raw) To: Hanna Reitz, qemu-block; +Cc: qemu-devel, Thomas Huth, Kevin Wolf 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iotests/065: Check for zstd support 2022-02-22 15:44 ` Vladimir Sementsov-Ogievskiy @ 2022-03-02 11:14 ` Hanna Reitz 0 siblings, 0 replies; 11+ messages in thread From: Hanna Reitz @ 2022-03-02 11:14 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: Kevin Wolf, Thomas Huth, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] iotests/303: Check for zstd support 2022-02-21 17:08 [PATCH 0/2] iotests: Check for zstd support Hanna Reitz 2022-02-21 17:08 ` [PATCH 1/2] iotests/065: " Hanna Reitz @ 2022-02-21 17:08 ` Hanna Reitz 2022-02-21 17:27 ` Thomas Huth 2022-02-22 15:55 ` Vladimir Sementsov-Ogievskiy 1 sibling, 2 replies; 11+ messages in thread From: Hanna Reitz @ 2022-02-21 17:08 UTC (permalink / raw) To: qemu-block Cc: Thomas Huth, Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, qemu-devel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iotests/303: Check for zstd support 2022-02-21 17:08 ` [PATCH 2/2] iotests/303: " Hanna Reitz @ 2022-02-21 17:27 ` Thomas Huth 2022-02-22 15:55 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 11+ messages in thread From: Thomas Huth @ 2022-02-21 17:27 UTC (permalink / raw) To: Hanna Reitz, qemu-block Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iotests/303: Check for zstd support 2022-02-21 17:08 ` [PATCH 2/2] iotests/303: " Hanna Reitz 2022-02-21 17:27 ` Thomas Huth @ 2022-02-22 15:55 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:09 ` Hanna Reitz 1 sibling, 1 reply; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2022-02-22 15:55 UTC (permalink / raw) To: Hanna Reitz, qemu-block; +Cc: qemu-devel, Thomas Huth, Kevin Wolf 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iotests/303: Check for zstd support 2022-02-22 15:55 ` Vladimir Sementsov-Ogievskiy @ 2022-03-02 11:09 ` Hanna Reitz 2022-03-02 11:31 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 11+ messages in thread From: Hanna Reitz @ 2022-03-02 11:09 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: Kevin Wolf, Thomas Huth, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iotests/303: Check for zstd support 2022-03-02 11:09 ` Hanna Reitz @ 2022-03-02 11:31 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:37 ` Hanna Reitz 0 siblings, 1 reply; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2022-03-02 11:31 UTC (permalink / raw) To: Hanna Reitz, qemu-block; +Cc: qemu-devel, Thomas Huth, Kevin Wolf 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iotests/303: Check for zstd support 2022-03-02 11:31 ` Vladimir Sementsov-Ogievskiy @ 2022-03-02 11:37 ` Hanna Reitz 0 siblings, 0 replies; 11+ messages in thread From: Hanna Reitz @ 2022-03-02 11:37 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: Kevin Wolf, Thomas Huth, qemu-devel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-03-02 13:23 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-21 17:08 [PATCH 0/2] iotests: Check for zstd support Hanna Reitz 2022-02-21 17:08 ` [PATCH 1/2] iotests/065: " Hanna Reitz 2022-02-21 17:20 ` Thomas Huth 2022-02-22 15:44 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:14 ` Hanna Reitz 2022-02-21 17:08 ` [PATCH 2/2] iotests/303: " Hanna Reitz 2022-02-21 17:27 ` Thomas Huth 2022-02-22 15:55 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:09 ` Hanna Reitz 2022-03-02 11:31 ` Vladimir Sementsov-Ogievskiy 2022-03-02 11:37 ` Hanna Reitz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.