| Message ID | 352a430ecbcb4800d31dc5a33b2b4a9f97fc810a.1744090313.git.nirjhar.roy.lists@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Minor cleanups in common/ | expand |
"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" -ritesh > # Handle mkfs.$fstyp which does (or does not) require -f to overwrite > set_mkfs_prog_path_with_opts() > { > -- > 2.34.1
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.
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? --NR > > -ritesh > > >> # Handle mkfs.$fstyp which does (or does not) require -f to overwrite >> set_mkfs_prog_path_with_opts() >> { >> -- >> 2.34.1
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 >
On 4/8/25 22:02, Darrick J. Wong wrote: > 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. Right, I have got the idea. I will make the changes in next revision. --NR > > --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 >>
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@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... 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. >
On Tue, Apr 08, 2025 at 10:13:54PM +0530, Nirjhar Roy (IBM) wrote: > On 4/8/25 17:05, Dave Chinner wrote: > > On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote: > > > 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? Seperate series is fine. You're not making the dependency mess any worse than it already is with this change... -Dave.
On 4/9/25 06:14, Dave Chinner wrote: > On Tue, Apr 08, 2025 at 10:13:54PM +0530, Nirjhar Roy (IBM) wrote: >> On 4/8/25 17:05, Dave Chinner wrote: >>> On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote: >>>> 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? > Seperate series is fine. You're not making the dependency mess any > worse than it already is with this change... Okay, got it. I will add this list of ToDos. Thank you so much for this suggestion. --NR > > -Dave.
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" +} + # 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 | 8 ++++++++ 1 file changed, 8 insertions(+)