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

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Apr 08, 2025 at 09:45:53PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/8/25 14:43, Ritesh Harjani (IBM) wrote:
> > "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> > 
> > > 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.
> > ...as it might not set the value of "$status" correctly, which is used
> > as an exit code in the trap handler routine set up by the check script.
> > 
> > > +_exit()
> > > +{
> > > +	status="$1"
> > > +	exit "$status"
> > > +}
> > > +
> > I agree with Darrick’s suggestion here. It’s safer to update status only
> > when an argument is passed - otherwise, it’s easy to trip over this.
> > 
> > Let’s also avoid defaulting status to 0 inside _exit(). That way, if the
> > caller forgets to pass an argument but has explicitly set status
> > earlier, we preserve the intended value.
> > 
> > We should update _exit() with...
> > 
> > test -n "$1" && status="$1"
> 
> Okay, so in that case if someone does "status=<value>;_exit", we should end
> up with the "<value>" instead of something else, right?

Right.  I think.  AFAICT the following simple program actually does
return 5 despite the cleanup:

trap 'echo cleanup' INT QUIT TERM EXIT
exit 5

But since fstests set a variable named "status" and then "exit $status"
from cleanup, I think it doesn't matter how status gets set as long as
it /does/ get set somewhere.

--D

> --NR
> 
> > 
> > -ritesh
> > 
> > 
> > >   # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
> > >   set_mkfs_prog_path_with_opts()
> > >   {
> > > -- 
> > > 2.34.1
> 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
>