Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Dave Chinner <david@fromorbit.com>]

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@gmail.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
>  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...

-Dave.

-- 
Dave Chinner
david@fromorbit.com