| Message ID | b7f56773095bea46c2b6d56c3d87197d900d9e2d.1744181682.git.nirjhar.roy.lists@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Minor cleanups in common/ | expand |
On Wed, Apr 09, 2025 at 07:00:51AM +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> Looks good now, Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > --- > common/config | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/common/config b/common/config > index 79bec87f..7dd78dbe 100644 > --- a/common/config > +++ b/common/config > @@ -96,6 +96,15 @@ 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, which is > +# used as an exit code in the trap handler routine set up by the check script. > +_exit() > +{ > + test -n "$1" && status="$1" > + exit "$status" > +} > + > # Handle mkfs.$fstyp which does (or does not) require -f to overwrite > set_mkfs_prog_path_with_opts() > { > -- > 2.34.1 > >
diff --git a/common/config b/common/config index 79bec87f..7dd78dbe 100644 --- a/common/config +++ b/common/config @@ -96,6 +96,15 @@ 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, which is +# used as an exit code in the trap handler routine set up by the check script. +_exit() +{ + test -n "$1" && status="$1" + exit "$status" +} + # Handle mkfs.$fstyp which does (or does not) require -f to overwrite set_mkfs_prog_path_with_opts() {
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 | 9 +++++++++ 1 file changed, 9 insertions(+)