| Message ID | 20240123162647.210424-4-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | shell: fix regression since 1878502f6 | expand |
Hi! > diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh > index 6bc305f2c..234bf06dd 100755 > --- a/testcases/kernel/device-drivers/zram/zram01.sh > +++ b/testcases/kernel/device-drivers/zram/zram01.sh > @@ -82,7 +82,8 @@ zram_makefs() > mkfs.$fs /dev/zram$i > err.log 2>&1 > if [ $? -ne 0 ]; then > cat err.log > - tst_brk TFAIL "failed to make $fs on /dev/zram$i" > + tst_res TFAIL "failed to make $fs on /dev/zram$i" > + return I do not think that this one is correct, unlike other tests the steps in zram depends on each other if this fails and we attempt to continue we will get failues from zram_mount and zram_fill_fs as well. The zram tests are unfortunatelly messy in a sense that a testcase is a setup for the next one so we really need to exit the whole test if one of the testcases fails. I guess the clearest solution would be TFAIL followed by a TBROK in this case. Something as: tst_res TFAIL "Failed to make $fs on /dev/zram$i" tst_brk TBROK "Can't continue with mounting the FS" > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > index b675a20a1..2a28562e6 100755 > --- a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > @@ -159,7 +159,8 @@ get_pcr10_aggregate() > $cmd > hash.txt 2>&1 > ret=$? > elif [ $ret -ne 0 -a "$MISSING_EVMCTL" = 1 ]; then > - tst_brk TFAIL "evmctl failed $msg" > + tst_res TFAIL "evmctl failed $msg" > + return > fi Again here, I'm not sure if it's safe to continue with the rest of the test.
> Hi! > > diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh > > index 6bc305f2c..234bf06dd 100755 > > --- a/testcases/kernel/device-drivers/zram/zram01.sh > > +++ b/testcases/kernel/device-drivers/zram/zram01.sh > > @@ -82,7 +82,8 @@ zram_makefs() > > mkfs.$fs /dev/zram$i > err.log 2>&1 > > if [ $? -ne 0 ]; then > > cat err.log > > - tst_brk TFAIL "failed to make $fs on /dev/zram$i" > > + tst_res TFAIL "failed to make $fs on /dev/zram$i" > > + return > I do not think that this one is correct, unlike other tests the steps in > zram depends on each other if this fails and we attempt to continue we > will get failues from zram_mount and zram_fill_fs as well. > The zram tests are unfortunatelly messy in a sense that a testcase is > a setup for the next one so we really need to exit the whole test if one > of the testcases fails. I guess the clearest solution would be TFAIL > followed by a TBROK in this case. Something as: > tst_res TFAIL "Failed to make $fs on /dev/zram$i" > tst_brk TBROK "Can't continue with mounting the FS" You're right! I'll just slightly modify tst_brk TBROK "Can't continue with mounting $fs" > > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > > index b675a20a1..2a28562e6 100755 > > --- a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh > > @@ -159,7 +159,8 @@ get_pcr10_aggregate() > > $cmd > hash.txt 2>&1 > > ret=$? > > elif [ $ret -ne 0 -a "$MISSING_EVMCTL" = 1 ]; then > > - tst_brk TFAIL "evmctl failed $msg" > > + tst_res TFAIL "evmctl failed $msg" > > + return > > fi > Again here, I'm not sure if it's safe to continue with the rest of the > test. Yes, this is safe, because the output of get_pcr10_aggregate() is later checked: get_pcr10_aggregate > tmp.txt pcr_aggregate="$(cat tmp.txt)" if [ -z "$pcr_aggregate" ]; then return fi I made ima_tpm.sh quite hard to understand. I guess soon I should drop support for older evmctl to make test easier to understand. Kind regards, Petr
Hi! > > tst_res TFAIL "Failed to make $fs on /dev/zram$i" > > tst_brk TBROK "Can't continue with mounting the FS" > > You're right! I'll just slightly modify > tst_brk TBROK "Can't continue with mounting $fs" You can add my Reviewed-by: with this change.
Hi Cyril, all, > Hi! > > > tst_res TFAIL "Failed to make $fs on /dev/zram$i" > > > tst_brk TBROK "Can't continue with mounting the FS" > > You're right! I'll just slightly modify > > tst_brk TBROK "Can't continue with mounting $fs" > You can add my Reviewed-by: with this change. I checked that the other functions should not depend on each other, so I dared to merge the patchset. We still have few days to check the result in CI if I'm wrong. Thanks! Kind regards, Petr
diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh index a5757309f..bedc48110 100755 --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh @@ -190,21 +190,27 @@ do_test() ROD_SILENT mkdir ${root_cpuset_dir}/testdir # Creat an exclusive cpuset. - echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive} - [ $? -ne 0 ] && tst_brk TFAIL "'echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}' failed" + if ! echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}; then + tst_res TFAIL "'echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}' failed" + return + fi cpu_exclusive_tmp=$(cat ${root_cpuset_dir}/testdir/${cpu_exclusive}) if [ "${cpu_exclusive_tmp}" != "1" ]; then - tst_brk TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}', expected '1'" + tst_res TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}', expected '1'" + return fi # This may trigger the kernel crash - echo 0 > ${root_cpuset_dir}/testdir/${cpus} - [ $? -ne 0 ] && tst_brk TFAIL "'echo 0 > ${root_cpuset_dir}/testdir/${cpus}' failed" + if ! echo 0 > ${root_cpuset_dir}/testdir/${cpus}; then + tst_res TFAIL "'echo 0 > ${root_cpuset_dir}/testdir/${cpus}' failed" + return + fi cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus}) if [ "${cpus_value}" != "0" ]; then - tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0'" + tst_res TFAIL "${cpus} is '${cpus_value}', expected '0'" + return fi tst_res TPASS "Bug is not reproducible" diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh index 6bc305f2c..234bf06dd 100755 --- a/testcases/kernel/device-drivers/zram/zram01.sh +++ b/testcases/kernel/device-drivers/zram/zram01.sh @@ -82,7 +82,8 @@ zram_makefs() mkfs.$fs /dev/zram$i > err.log 2>&1 if [ $? -ne 0 ]; then cat err.log - tst_brk TFAIL "failed to make $fs on /dev/zram$i" + tst_res TFAIL "failed to make $fs on /dev/zram$i" + return fi i=$(($i + 1)) diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh index e94d7db11..e94f9244d 100755 --- a/testcases/kernel/device-drivers/zram/zram_lib.sh +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh @@ -108,12 +108,16 @@ zram_max_streams() for max_s in $zram_max_streams; do local sys_path="/sys/block/zram${i}/max_comp_streams" - echo $max_s > $sys_path || \ - tst_brk TFAIL "failed to set '$max_s' to $sys_path" + if ! echo $max_s > $sys_path; then + tst_res TFAIL "failed to set '$max_s' to $sys_path" + return + fi local max_streams=$(cat $sys_path) - [ "$max_s" -ne "$max_streams" ] && \ - tst_brk TFAIL "can't set max_streams '$max_s', get $max_stream" + if [ "$max_s" -ne "$max_streams" ]; then + tst_res TFAIL "can't set max_streams '$max_s', get $max_stream" + return + fi i=$(($i + 1)) tst_res TINFO "$sys_path = '$max_streams'" @@ -140,8 +144,10 @@ zram_compress_alg() for i in $(seq $dev_start $dev_end); do for alg in $algs; do local sys_path="/sys/block/zram${i}/comp_algorithm" - echo "$alg" > $sys_path || \ - tst_brk TFAIL "can't set '$alg' to $sys_path" + if ! echo "$alg" > $sys_path; then + tst_res TFAIL "can't set '$alg' to $sys_path" + return + fi tst_res TINFO "$sys_path = '$alg'" done done @@ -157,8 +163,10 @@ zram_set_disksizes() tst_res TINFO "set disk size to zram device(s)" for ds in $zram_sizes; do local sys_path="/sys/block/zram${i}/disksize" - echo "$ds" > $sys_path || \ - tst_brk TFAIL "can't set '$ds' to $sys_path" + if ! echo "$ds" > $sys_path; then + tst_res TFAIL "can't set '$ds' to $sys_path" + return + fi i=$(($i + 1)) tst_res TINFO "$sys_path = '$ds'" @@ -183,8 +191,10 @@ zram_set_memlimit() for ds in $zram_mem_limits; do local sys_path="/sys/block/zram${i}/mem_limit" - echo "$ds" > $sys_path || \ - tst_brk TFAIL "can't set '$ds' to $sys_path" + if ! echo "$ds" > $sys_path; then + tst_res TFAIL "can't set '$ds' to $sys_path" + return + fi i=$(($i + 1)) tst_res TINFO "$sys_path = '$ds'" diff --git a/testcases/kernel/fs/quota_remount/quota_remount_test01.sh b/testcases/kernel/fs/quota_remount/quota_remount_test01.sh index 25d9f8fcc..7e20b3608 100755 --- a/testcases/kernel/fs/quota_remount/quota_remount_test01.sh +++ b/testcases/kernel/fs/quota_remount/quota_remount_test01.sh @@ -67,7 +67,8 @@ do_test() newblocks=$(get_blocks) if [ $blocks -eq $newblocks ]; then - tst_brk TFAIL "usage did not change after remount" + tst_res TFAIL "usage did not change after remount" + return fi tst_res TPASS "quota on remount passed" diff --git a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh index b675a20a1..2a28562e6 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_tpm.sh @@ -159,7 +159,8 @@ get_pcr10_aggregate() $cmd > hash.txt 2>&1 ret=$? elif [ $ret -ne 0 -a "$MISSING_EVMCTL" = 1 ]; then - tst_brk TFAIL "evmctl failed $msg" + tst_res TFAIL "evmctl failed $msg" + return fi [ $ret -ne 0 ] && tst_res TWARN "evmctl failed, trying to continue $msg"
It needs to be replaced with tst_res TFAIL and return Fixes: 1878502f6 ("tst_test.sh/tst_brk(): Allow only TBROK and TCONF") Signed-off-by: Petr Vorel <pvorel@suse.cz> --- .../cpuset/cpuset_regression_test.sh | 18 +++++++---- .../kernel/device-drivers/zram/zram01.sh | 3 +- .../kernel/device-drivers/zram/zram_lib.sh | 30 ++++++++++++------- .../fs/quota_remount/quota_remount_test01.sh | 3 +- .../security/integrity/ima/tests/ima_tpm.sh | 3 +- 5 files changed, 38 insertions(+), 19 deletions(-)