On 4/8/25 17:05, Dave Chinner wrote:
On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote:We should always set the value of status correctly when we are exiting. Else, "$?" might not give us the correct value. If we see the following trap handler registration in the check script: if $OPTIONS_HAVE_SECTIONS; then trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15 else trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15 fi So, "exit 1" will exit the check script without setting the correct return value. I ran with the following local.config file: [xfs_4k_valid] FSTYP=xfs TEST_DEV=/dev/loop0 TEST_DIR=/mnt1/test SCRATCH_DEV=/dev/loop1 SCRATCH_MNT=/mnt1/scratch [xfs_4k_invalid] FSTYP=xfs TEST_DEV=/dev/loop0 TEST_DIR=/mnt1/invalid_dir SCRATCH_DEV=/dev/loop1 SCRATCH_MNT=/mnt1/scratch This caused the init_rc() to catch the case of invalid _test_mount options. Although the check script correctly failed during the execution of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?" returned 0. This is because init_rc exits with "exit 1" without correctly setting the value of "status". IMO, the correct behavior should have been that "$?" should have been non-zero. The next patch will replace exit with _exit. Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@xxxxxxxxx> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> --- common/config | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/common/config b/common/config index 79bec87f..eb6af35a 100644 --- a/common/config +++ b/common/config @@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false} +# This functions sets the exit code to status and then exits. Don't use+# exit directly, as it might not set the value of "status" correctly. +_exit() +{ + status="$1" + exit "$status" +}The only issue with putting this helper in common/config is that calling _exit() requires sourcing common/config from the shell context that calls it. This means every test must source common/config and re-execute the environment setup, even though we already have all the environment set up because it was exported from check when it sourced common/config. We have the same problem with _fatal() - it is defined in common/config instead of an independent common file. If we put such functions in their own common file, they can be sourced without dependencies on any other common file being included first. e.g. we create common/exit and place all the functions that terminate tests in it - _fatal, _notrun, _exit, etc and source that file once per shell context before we source common/config, common/rc, etc. This means we can source and call those termination functions from any context without having to worry about dependencies...
Yes, I agree to the above. Do you want this refactoring to be done as a part of this patch series in the further revisions, or can this be sent as a separate series?
--NR
-Dave.
-- Nirjhar Roy Linux Kernel Developer IBM, Bangalore
