Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
On 16.07.21 16:24, Vladimir Sementsov-Ogievskiy wrote: 16.07.2021 15:38, Max Reitz wrote: Subject: s/compressiont_type/compression_type/ On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote: Like it is done in iotests.py in qemu_img_create_prepare_args(), let's not follow compression_type=zstd of IMGOPTS if test creates image in old format. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/common.rc | 8 1 file changed, 8 insertions(+) diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index cbbf6d7c7f..4cae5b2d70 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -438,6 +438,14 @@ _make_test_img() backing_file=$param continue elif $opts_param; then + if [[ "$param" == *"compat=0"* ]]; then Like in patch 2, probably should be 0.10, and account for “v2”. + # If user specified zstd compression type in IMGOPTS, this will + # just not work. So, let's imply forcing zlib compression when + # test creates image in old version of the format. + # Similarly works qemu_img_create_prepare_args() in iotests.py + optstr=$(echo "$optstr" | $SED -e 's/compression_type=\w\+//') What about the surrounding comma, if compression_type is just one option among others? Do we need something like $SED -e 's/,compression_type=\w\+//' -e 's/compression_type=\w\+,\?//' ? Agree + optstr=$(_optstr_add "$optstr" "compression_type=zlib") As the comment says, this is for compression_type in $IMGOPTS and then compat=0.10 in the parameters. It won’t work if you have e.g. “_make_test_img -o compat=0.10,compression_type=zstd”, because then this generates the optstr “$IMGOPTS,compression_type=zlib,compat=0.10,compression_type=zstd”. Not sure if we want to care about this case, but, well... Then there’s the case where I have compat=0.10 in $IMGOPTS, and the test wants to use compression_type=zstd. I think it’s correct not to replace compression_type=zstd then, because the test should be skipped for compat=0.10 in $IMGOPTS. But that’s not what happens in the iotest.py version (qemu_img_create_prepare_args()), so I wonder whether the latter should be made to match this behavior here, if in any way possible. Now that I think about it more, I begin to wonder more... So this code doesn’t explicitly handle compression_type only in $IMGOPTS. If you have _make_test_img -o compression_type=zstd,compat=0.10 It’ll still keep the compression_type=zstd. However, for _make_test_img -o compression_type=zstd -o compat=0.10 It’ll replace it by zlib. So perhaps we should explicitly scan for compression_type only in $IMGOPTS and then drop it from the optstr if compat=0.10 is in the _make_test_img's -o options. But thinking further, this is not how $IMGOPTS work. So far they aren’t advisory, they are mandatory. If a test cannot work with something in $IMGOPTS, it has to be skipped. Like, when you have compat=0.10 in $IMGOPTS, I don’t want to run tests that use v3 features and have them just create v3 images for those tests. So my impression is that you’re giving compression_type special treatment here, and I don’t know why exactly. Tests that create v2 images should just have compression_type be an unsupported_imgopt. Max Hmm.. I have better idea: deprecate v2 and drop all iotest support for it now :)) What do you think? I haven’t yet understood the appeal of deprecating v2, because basically all code is shared between v2 and v3. So I don’t really see the appeal in dropping iotest support for it either. At least, this doesn’t appear like a better idea than to add _unsupported_imgopts where needed (in fact, _unsupported_imgopts should already be there for other v3-only options like lazy_refcounts). If not, than instead of this patch, we just should skip all tests that don't support compression_type=zstd due to using old version.. This means that we will skip some test-cases that can work with zstd just because we can't skip separate test cases in bash tests. The standard procedure for this is to have a quick look whether we actually lose (relevant) coverage for this imgopt if we skip the test (usually not), and if so, split that part out into a new file. But again, usually nothing of value is lost, so nothing is split off. (ohh, I'd deprecate bash tests too.. But that's kind of taste) So far I don’t think there is a pressing reason why bash tests would be harder to support than Python tests, and so the effort to port all bash tests to Python seems much more difficult to me than having to duplicate meta-work like this. (And in fact, as an example, I found it much easier to have bash tests support -o data_file than the Python tests, not least because the bash tests at least kind of all work the same, whereas we have like three variants of
Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
16.07.2021 15:38, Max Reitz wrote: Subject: s/compressiont_type/compression_type/ On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote: Like it is done in iotests.py in qemu_img_create_prepare_args(), let's not follow compression_type=zstd of IMGOPTS if test creates image in old format. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/common.rc | 8 1 file changed, 8 insertions(+) diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index cbbf6d7c7f..4cae5b2d70 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -438,6 +438,14 @@ _make_test_img() backing_file=$param continue elif $opts_param; then + if [[ "$param" == *"compat=0"* ]]; then Like in patch 2, probably should be 0.10, and account for “v2”. + # If user specified zstd compression type in IMGOPTS, this will + # just not work. So, let's imply forcing zlib compression when + # test creates image in old version of the format. + # Similarly works qemu_img_create_prepare_args() in iotests.py + optstr=$(echo "$optstr" | $SED -e 's/compression_type=\w\+//') What about the surrounding comma, if compression_type is just one option among others? Do we need something like $SED -e 's/,compression_type=\w\+//' -e 's/compression_type=\w\+,\?//' ? Agree + optstr=$(_optstr_add "$optstr" "compression_type=zlib") As the comment says, this is for compression_type in $IMGOPTS and then compat=0.10 in the parameters. It won’t work if you have e.g. “_make_test_img -o compat=0.10,compression_type=zstd”, because then this generates the optstr “$IMGOPTS,compression_type=zlib,compat=0.10,compression_type=zstd”. Not sure if we want to care about this case, but, well... Then there’s the case where I have compat=0.10 in $IMGOPTS, and the test wants to use compression_type=zstd. I think it’s correct not to replace compression_type=zstd then, because the test should be skipped for compat=0.10 in $IMGOPTS. But that’s not what happens in the iotest.py version (qemu_img_create_prepare_args()), so I wonder whether the latter should be made to match this behavior here, if in any way possible. Now that I think about it more, I begin to wonder more... So this code doesn’t explicitly handle compression_type only in $IMGOPTS. If you have _make_test_img -o compression_type=zstd,compat=0.10 It’ll still keep the compression_type=zstd. However, for _make_test_img -o compression_type=zstd -o compat=0.10 It’ll replace it by zlib. So perhaps we should explicitly scan for compression_type only in $IMGOPTS and then drop it from the optstr if compat=0.10 is in the _make_test_img's -o options. But thinking further, this is not how $IMGOPTS work. So far they aren’t advisory, they are mandatory. If a test cannot work with something in $IMGOPTS, it has to be skipped. Like, when you have compat=0.10 in $IMGOPTS, I don’t want to run tests that use v3 features and have them just create v3 images for those tests. So my impression is that you’re giving compression_type special treatment here, and I don’t know why exactly. Tests that create v2 images should just have compression_type be an unsupported_imgopt. Max Hmm.. I have better idea: deprecate v2 and drop all iotest support for it now :)) What do you think? If not, than instead of this patch, we just should skip all tests that don't support compression_type=zstd due to using old version.. This means that we will skip some test-cases that can work with zstd just because we can't skip separate test cases in bash tests. (ohh, I'd deprecate bash tests too.. But that's kind of taste) -- Best regards, Vladimir
Re: [PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling
Subject: s/compressiont_type/compression_type/ On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote: Like it is done in iotests.py in qemu_img_create_prepare_args(), let's not follow compression_type=zstd of IMGOPTS if test creates image in old format. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/common.rc | 8 1 file changed, 8 insertions(+) diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index cbbf6d7c7f..4cae5b2d70 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -438,6 +438,14 @@ _make_test_img() backing_file=$param continue elif $opts_param; then +if [[ "$param" == *"compat=0"* ]]; then Like in patch 2, probably should be 0.10, and account for “v2”. +# If user specified zstd compression type in IMGOPTS, this will +# just not work. So, let's imply forcing zlib compression when +# test creates image in old version of the format. +# Similarly works qemu_img_create_prepare_args() in iotests.py +optstr=$(echo "$optstr" | $SED -e 's/compression_type=\w\+//') What about the surrounding comma, if compression_type is just one option among others? Do we need something like $SED -e 's/,compression_type=\w\+//' -e 's/compression_type=\w\+,\?//' ? +optstr=$(_optstr_add "$optstr" "compression_type=zlib") As the comment says, this is for compression_type in $IMGOPTS and then compat=0.10 in the parameters. It won’t work if you have e.g. “_make_test_img -o compat=0.10,compression_type=zstd”, because then this generates the optstr “$IMGOPTS,compression_type=zlib,compat=0.10,compression_type=zstd”. Not sure if we want to care about this case, but, well... Then there’s the case where I have compat=0.10 in $IMGOPTS, and the test wants to use compression_type=zstd. I think it’s correct not to replace compression_type=zstd then, because the test should be skipped for compat=0.10 in $IMGOPTS. But that’s not what happens in the iotest.py version (qemu_img_create_prepare_args()), so I wonder whether the latter should be made to match this behavior here, if in any way possible. Now that I think about it more, I begin to wonder more... So this code doesn’t explicitly handle compression_type only in $IMGOPTS. If you have _make_test_img -o compression_type=zstd,compat=0.10 It’ll still keep the compression_type=zstd. However, for _make_test_img -o compression_type=zstd -o compat=0.10 It’ll replace it by zlib. So perhaps we should explicitly scan for compression_type only in $IMGOPTS and then drop it from the optstr if compat=0.10 is in the _make_test_img's -o options. But thinking further, this is not how $IMGOPTS work. So far they aren’t advisory, they are mandatory. If a test cannot work with something in $IMGOPTS, it has to be skipped. Like, when you have compat=0.10 in $IMGOPTS, I don’t want to run tests that use v3 features and have them just create v3 images for those tests. So my impression is that you’re giving compression_type special treatment here, and I don’t know why exactly. Tests that create v2 images should just have compression_type be an unsupported_imgopt. Max +fi optstr=$(_optstr_add "$optstr" "$param") opts_param=false continue
