Message-ID: <37155b56-34ba-4d5d-a023-242abbe525b5@gmail.com>
Date: Tue, 8 Apr 2025 22:13:54 +0530
From: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
To: Dave Chinner <david@...morbit.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
 linux-xfs@...r.kernel.org, ritesh.list@...il.com, ojaswin@...ux.ibm.com,
 djwong@...nel.org, zlang@...nel.org
Subject: Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit
 command


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

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

Powered by blists - more mailing lists