| Message ID | 1d07e5657c2817c74e939894bb554424199fd290.1741248214.git.nirjhar.roy.lists@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Minor cleanups in common/rc | expand |
On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > Silently executing scripts during sourcing common/rc doesn't look good > and also causes unnecessary script execution. Decouple init_rc() call > and call init_rc() explicitly where required. > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > --- > check | 10 ++-------- > common/preamble | 1 + > common/rc | 2 -- > soak | 1 + > 4 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/check b/check > index ea92b0d6..d30af1ba 100755 > --- a/check > +++ b/check > @@ -840,16 +840,8 @@ function run_section() > _prepare_test_list > elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then > _test_unmount 2> /dev/null > - if ! _test_mount > - then > - echo "check: failed to mount $TEST_DEV on $TEST_DIR" > - status=1 > - exit > - fi Unrelated change? I was expecting a mechanical ". ./common/rc" => ". ./common/rc ; init_rc" change in this patch. > fi > > - init_rc Why remove init_rc here? > - > seq="check.$$" > check="$RESULT_BASE/check" > > @@ -870,6 +862,8 @@ function run_section() > needwrap=true > > if [ ! -z "$SCRATCH_DEV" ]; then > + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT > + [ $? -le 1 ] || exit 1 > _scratch_unmount 2> /dev/null > # call the overridden mkfs - make sure the FS is built > # the same as we'll create it later. > diff --git a/common/preamble b/common/preamble > index 0c9ee2e0..c92e55bb 100644 > --- a/common/preamble > +++ b/common/preamble > @@ -50,6 +50,7 @@ _begin_fstest() > _register_cleanup _cleanup > > . ./common/rc > + init_rc > > # remove previous $seqres.full before test > rm -f $seqres.full $seqres.hints > diff --git a/common/rc b/common/rc > index d2de8588..f153ad81 100644 > --- a/common/rc > +++ b/common/rc > @@ -5754,8 +5754,6 @@ _require_program() { > _have_program "$1" || _notrun "$tag required" > } > > -init_rc > - > ################################################################################ > # make sure this script returns success > /bin/true > diff --git a/soak b/soak > index d5c4229a..5734d854 100755 > --- a/soak > +++ b/soak > @@ -5,6 +5,7 @@ > > # get standard environment, filters and checks > . ./common/rc > +# ToDo: Do we need an init_rc() here? How is soak used? I have no idea what soak does and have never used it, but I think for continuity's sake you should call init_rc here. --D > . ./common/filter > > tmp=/tmp/$$ > -- > 2.34.1 > >
On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > Silently executing scripts during sourcing common/rc doesn't look good > and also causes unnecessary script execution. Decouple init_rc() call > and call init_rc() explicitly where required. > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > --- > check | 10 ++-------- > common/preamble | 1 + > common/rc | 2 -- > soak | 1 + > 4 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/check b/check > index ea92b0d6..d30af1ba 100755 > --- a/check > +++ b/check > @@ -840,16 +840,8 @@ function run_section() > _prepare_test_list > elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then > _test_unmount 2> /dev/null > - if ! _test_mount > - then > - echo "check: failed to mount $TEST_DEV on $TEST_DIR" > - status=1 > - exit > - fi Why remove these lines? > fi > > - init_rc Doesn't the "check" need init_rc at here? > - > seq="check.$$" > check="$RESULT_BASE/check" > > @@ -870,6 +862,8 @@ function run_section() > needwrap=true > > if [ ! -z "$SCRATCH_DEV" ]; then > + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT > + [ $? -le 1 ] || exit 1 ^^^^^^^ Different indent with below code. This looks like part of init_rc. If you don't remove above init_rc, can this change be saved? > _scratch_unmount 2> /dev/null > # call the overridden mkfs - make sure the FS is built > # the same as we'll create it later. > diff --git a/common/preamble b/common/preamble > index 0c9ee2e0..c92e55bb 100644 > --- a/common/preamble > +++ b/common/preamble > @@ -50,6 +50,7 @@ _begin_fstest() > _register_cleanup _cleanup > > . ./common/rc > + init_rc > > # remove previous $seqres.full before test > rm -f $seqres.full $seqres.hints > diff --git a/common/rc b/common/rc > index d2de8588..f153ad81 100644 > --- a/common/rc > +++ b/common/rc > @@ -5754,8 +5754,6 @@ _require_program() { > _have_program "$1" || _notrun "$tag required" > } > > -init_rc > - > ################################################################################ > # make sure this script returns success > /bin/true > diff --git a/soak b/soak > index d5c4229a..5734d854 100755 > --- a/soak > +++ b/soak > @@ -5,6 +5,7 @@ > > # get standard environment, filters and checks > . ./common/rc > +# ToDo: Do we need an init_rc() here? How is soak used? I never noticed we have this file... this file was create by: commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247 Author: Nathan Scott <nathans@sgi.com> Date: Mon Jan 15 05:01:19 2001 +0000 cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001 I can't understand the relationship of this commit with this file. Does anyone learn about the history of it. I tried to "grep" the whole fstests, looks like nothing uses this file. Maybe we should remove it? Thanks, Zorro > . ./common/filter > > tmp=/tmp/$$ > -- > 2.34.1 > >
On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > Silently executing scripts during sourcing common/rc doesn't look good > and also causes unnecessary script execution. Decouple init_rc() call > and call init_rc() explicitly where required. > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> FWIW, I've just done somethign similar for check-parallel. I need to decouple common/config from common/rc and not run any code from either common/config or common/rc. I've included the patch below (it won't apply because there's all sorts of refactoring for test list and config-section parsing in the series before it), but it should give you an idea of how I think we should be separating one-off initialisation environment varaibles, common code inclusion and the repeated initialisation of section specific parameters.... ..... > diff --git a/soak b/soak > index d5c4229a..5734d854 100755 > --- a/soak > +++ b/soak > @@ -5,6 +5,7 @@ > > # get standard environment, filters and checks > . ./common/rc > +# ToDo: Do we need an init_rc() here? How is soak used? > . ./common/filter I've also go a patch series that removes all these old 2000-era SGI QE scripts that have not been used by anyone for the last 15 years. I did that to get rid of the technical debt that these scripts have gathered over years of neglect. They aren't used, we shouldn't even attempt to maintain them anymore. -Dave.
On 3/6/25 23:16, Darrick J. Wong wrote: > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >> Silently executing scripts during sourcing common/rc doesn't look good >> and also causes unnecessary script execution. Decouple init_rc() call >> and call init_rc() explicitly where required. >> >> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> >> --- >> check | 10 ++-------- >> common/preamble | 1 + >> common/rc | 2 -- >> soak | 1 + >> 4 files changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/check b/check >> index ea92b0d6..d30af1ba 100755 >> --- a/check >> +++ b/check >> @@ -840,16 +840,8 @@ function run_section() >> _prepare_test_list >> elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then >> _test_unmount 2> /dev/null >> - if ! _test_mount >> - then >> - echo "check: failed to mount $TEST_DEV on $TEST_DIR" >> - status=1 >> - exit >> - fi > Unrelated change? I was expecting a mechanical ". ./common/rc" => > ". ./common/rc ; init_rc" change in this patch. This patch adds an init_rc() call to _begin_fstests() in common/preamble and hence the above _test_mount() will be executed during that call. So this _test_mount isn't necessary here, right? _test_mount() will be executed (as a part of init_rc() call) before every test run. Please let me know if my understanding isn't correct. >> fi >> >> - init_rc > Why remove init_rc here? Same reason as above. > >> - >> seq="check.$$" >> check="$RESULT_BASE/check" >> >> @@ -870,6 +862,8 @@ function run_section() >> needwrap=true >> >> if [ ! -z "$SCRATCH_DEV" ]; then >> + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT >> + [ $? -le 1 ] || exit 1 >> _scratch_unmount 2> /dev/null >> # call the overridden mkfs - make sure the FS is built >> # the same as we'll create it later. >> diff --git a/common/preamble b/common/preamble >> index 0c9ee2e0..c92e55bb 100644 >> --- a/common/preamble >> +++ b/common/preamble >> @@ -50,6 +50,7 @@ _begin_fstest() >> _register_cleanup _cleanup >> >> . ./common/rc >> + init_rc >> >> # remove previous $seqres.full before test >> rm -f $seqres.full $seqres.hints >> diff --git a/common/rc b/common/rc >> index d2de8588..f153ad81 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -5754,8 +5754,6 @@ _require_program() { >> _have_program "$1" || _notrun "$tag required" >> } >> >> -init_rc >> - >> ################################################################################ >> # make sure this script returns success >> /bin/true >> diff --git a/soak b/soak >> index d5c4229a..5734d854 100755 >> --- a/soak >> +++ b/soak >> @@ -5,6 +5,7 @@ >> >> # get standard environment, filters and checks >> . ./common/rc >> +# ToDo: Do we need an init_rc() here? How is soak used? > I have no idea what soak does and have never used it, but I think for > continuity's sake you should call init_rc here. Okay. I think Dave has suggested removing this file[1]. This doesn't seem to used anymore. [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/ --NR > > --D > >> . ./common/filter >> >> tmp=/tmp/$$ >> -- >> 2.34.1 >> >>
On 3/7/25 02:43, Zorro Lang wrote: > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >> Silently executing scripts during sourcing common/rc doesn't look good >> and also causes unnecessary script execution. Decouple init_rc() call >> and call init_rc() explicitly where required. >> >> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> >> --- >> check | 10 ++-------- >> common/preamble | 1 + >> common/rc | 2 -- >> soak | 1 + >> 4 files changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/check b/check >> index ea92b0d6..d30af1ba 100755 >> --- a/check >> +++ b/check >> @@ -840,16 +840,8 @@ function run_section() >> _prepare_test_list >> elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then >> _test_unmount 2> /dev/null >> - if ! _test_mount >> - then >> - echo "check: failed to mount $TEST_DEV on $TEST_DIR" >> - status=1 >> - exit >> - fi > Why remove these lines? Darrick has asked the same question [1]. Basically I have already added init_rc() call to _begin_fstests() which will do the _test_mount() so we don't need the above lines, right? [1] https://lore.kernel.org/all/716e0d26-7728-42bb-981d-aae89ef50d7f@gmail.com/ > >> fi >> >> - init_rc > Doesn't the "check" need init_rc at here? Same reason as above. > >> - >> seq="check.$$" >> check="$RESULT_BASE/check" >> >> @@ -870,6 +862,8 @@ function run_section() >> needwrap=true >> >> if [ ! -z "$SCRATCH_DEV" ]; then >> + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT >> + [ $? -le 1 ] || exit 1 > ^^^^^^^ > Different indent with below code. > > This looks like part of init_rc. If you don't remove above init_rc, can this > change be saved? > >> _scratch_unmount 2> /dev/null >> # call the overridden mkfs - make sure the FS is built >> # the same as we'll create it later. >> diff --git a/common/preamble b/common/preamble >> index 0c9ee2e0..c92e55bb 100644 >> --- a/common/preamble >> +++ b/common/preamble >> @@ -50,6 +50,7 @@ _begin_fstest() >> _register_cleanup _cleanup >> >> . ./common/rc >> + init_rc >> >> # remove previous $seqres.full before test >> rm -f $seqres.full $seqres.hints >> diff --git a/common/rc b/common/rc >> index d2de8588..f153ad81 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -5754,8 +5754,6 @@ _require_program() { >> _have_program "$1" || _notrun "$tag required" >> } >> >> -init_rc >> - >> ################################################################################ >> # make sure this script returns success >> /bin/true >> diff --git a/soak b/soak >> index d5c4229a..5734d854 100755 >> --- a/soak >> +++ b/soak >> @@ -5,6 +5,7 @@ >> >> # get standard environment, filters and checks >> . ./common/rc >> +# ToDo: Do we need an init_rc() here? How is soak used? > I never noticed we have this file... this file was create by: > > commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247 > Author: Nathan Scott <nathans@sgi.com> > Date: Mon Jan 15 05:01:19 2001 +0000 > > cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001 > > I can't understand the relationship of this commit with this file. Does > anyone learn about the history of it. > > I tried to "grep" the whole fstests, looks like nothing uses this file. > Maybe we should remove it? Okay. I can see Dave suggesting something similar and has also given a sample patch where he is planning to do the same[2]. [2] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/ --NR > > Thanks, > Zorro > >> . ./common/filter >> >> tmp=/tmp/$$ >> -- >> 2.34.1 >> >>
On 3/7/25 03:00, Dave Chinner wrote: > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >> Silently executing scripts during sourcing common/rc doesn't look good >> and also causes unnecessary script execution. Decouple init_rc() call >> and call init_rc() explicitly where required. >> >> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > FWIW, I've just done somethign similar for check-parallel. I need to > decouple common/config from common/rc and not run any code from > either common/config or common/rc. > > I've included the patch below (it won't apply because there's all > sorts of refactoring for test list and config-section parsing in the > series before it), but it should give you an idea of how I think we > should be separating one-off initialisation environment varaibles, > common code inclusion and the repeated initialisation of section > specific parameters.... Thank you so much. I can a look at this. > > ..... >> diff --git a/soak b/soak >> index d5c4229a..5734d854 100755 >> --- a/soak >> +++ b/soak >> @@ -5,6 +5,7 @@ >> >> # get standard environment, filters and checks >> . ./common/rc >> +# ToDo: Do we need an init_rc() here? How is soak used? >> . ./common/filter > I've also go a patch series that removes all these old 2000-era SGI > QE scripts that have not been used by anyone for the last 15 > years. I did that to get rid of the technical debt that these > scripts have gathered over years of neglect. They aren't used, we > shouldn't even attempt to maintain them anymore. Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do you mean some kind of CI/automation-test script? --NR > > -Dave. >
On Fri, Mar 07, 2025 at 11:21:15AM +0530, Nirjhar Roy (IBM) wrote: > > On 3/6/25 23:16, Darrick J. Wong wrote: > > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > > > Silently executing scripts during sourcing common/rc doesn't look good > > > and also causes unnecessary script execution. Decouple init_rc() call > > > and call init_rc() explicitly where required. > > > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > > > --- > > > check | 10 ++-------- > > > common/preamble | 1 + > > > common/rc | 2 -- > > > soak | 1 + > > > 4 files changed, 4 insertions(+), 10 deletions(-) > > > > > > diff --git a/check b/check > > > index ea92b0d6..d30af1ba 100755 > > > --- a/check > > > +++ b/check > > > @@ -840,16 +840,8 @@ function run_section() > > > _prepare_test_list > > > elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then > > > _test_unmount 2> /dev/null > > > - if ! _test_mount > > > - then > > > - echo "check: failed to mount $TEST_DEV on $TEST_DIR" > > > - status=1 > > > - exit > > > - fi > > Unrelated change? I was expecting a mechanical ". ./common/rc" => > > ". ./common/rc ; init_rc" change in this patch. > This patch adds an init_rc() call to _begin_fstests() in common/preamble and > hence the above _test_mount() will be executed during that call. So this > _test_mount isn't necessary here, right? _test_mount() will be executed (as > a part of init_rc() call) before every test run. Please let me know if my > understanding isn't correct. It's true that in terms of getting the test filesystem mounted, the _test_mount here and in init_rc are redundant. But look at what happens on error here -- we print "check: failed to mount..." to signal that the new section's TEST_FS_MOUNT_OPTS are not valid, and exit the ./check process. By deferring the mount to the init_rc in _preamble, that means that we'll run the whole section with bad mount options, most likely resulting in every test spewing "common/rc: could not mount..." and appearing to fail. I think. I'm not sure what "status=1; exit" does as compared to "exit 1"; AFAICT the former actually results in an exit code of 0 because the (otherwise pointless) assignment succeeds. Granted, the init_rc that you remove below would also catch that case and exit ./check > > > fi > > > - init_rc > > Why remove init_rc here? > Same reason as above. But that's an additional change in behavior. If there's no reason for calling init_rc() from run_section() then that should be a separate patch with a separate justification. --D > > > > > - > > > seq="check.$$" > > > check="$RESULT_BASE/check" > > > @@ -870,6 +862,8 @@ function run_section() > > > needwrap=true > > > if [ ! -z "$SCRATCH_DEV" ]; then > > > + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT > > > + [ $? -le 1 ] || exit 1 > > > _scratch_unmount 2> /dev/null > > > # call the overridden mkfs - make sure the FS is built > > > # the same as we'll create it later. > > > diff --git a/common/preamble b/common/preamble > > > index 0c9ee2e0..c92e55bb 100644 > > > --- a/common/preamble > > > +++ b/common/preamble > > > @@ -50,6 +50,7 @@ _begin_fstest() > > > _register_cleanup _cleanup > > > . ./common/rc > > > + init_rc > > > # remove previous $seqres.full before test > > > rm -f $seqres.full $seqres.hints > > > diff --git a/common/rc b/common/rc > > > index d2de8588..f153ad81 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -5754,8 +5754,6 @@ _require_program() { > > > _have_program "$1" || _notrun "$tag required" > > > } > > > -init_rc > > > - > > > ################################################################################ > > > # make sure this script returns success > > > /bin/true > > > diff --git a/soak b/soak > > > index d5c4229a..5734d854 100755 > > > --- a/soak > > > +++ b/soak > > > @@ -5,6 +5,7 @@ > > > # get standard environment, filters and checks > > > . ./common/rc > > > +# ToDo: Do we need an init_rc() here? How is soak used? > > I have no idea what soak does and have never used it, but I think for > > continuity's sake you should call init_rc here. > > Okay. I think Dave has suggested removing this file[1]. This doesn't seem to > used anymore. > > [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/ > > --NR > > > > > --D > > > > > . ./common/filter > > > tmp=/tmp/$$ > > > -- > > > 2.34.1 > > > > > > > -- > Nirjhar Roy > Linux Kernel Developer > IBM, Bangalore > >
On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote: > > On 3/7/25 03:00, Dave Chinner wrote: > > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > > > Silently executing scripts during sourcing common/rc doesn't look good > > > and also causes unnecessary script execution. Decouple init_rc() call > > > and call init_rc() explicitly where required. > > > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > > FWIW, I've just done somethign similar for check-parallel. I need to > > decouple common/config from common/rc and not run any code from > > either common/config or common/rc. > > > > I've included the patch below (it won't apply because there's all > > sorts of refactoring for test list and config-section parsing in the > > series before it), but it should give you an idea of how I think we > > should be separating one-off initialisation environment varaibles, > > common code inclusion and the repeated initialisation of section > > specific parameters.... > Thank you so much. I can a look at this. > > > > ..... > > > diff --git a/soak b/soak > > > index d5c4229a..5734d854 100755 > > > --- a/soak > > > +++ b/soak > > > @@ -5,6 +5,7 @@ > > > # get standard environment, filters and checks > > > . ./common/rc > > > +# ToDo: Do we need an init_rc() here? How is soak used? > > > . ./common/filter > > I've also go a patch series that removes all these old 2000-era SGI > > QE scripts that have not been used by anyone for the last 15 > > years. I did that to get rid of the technical debt that these > > scripts have gathered over years of neglect. They aren't used, we > > shouldn't even attempt to maintain them anymore. > > Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do > you mean some kind of CI/automation-test script? SGI is Silicon Graphics International Corp. : https://en.wikipedia.org/wiki/Silicon_Graphics_International xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX) of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert of all these things, and knows lots of past details :) Thanks, Zorro > > --NR > > > > > -Dave. > > > -- > Nirjhar Roy > Linux Kernel Developer > IBM, Bangalore > >
On Sat, Mar 08, 2025 at 03:20:34PM +0800, Zorro Lang wrote: > On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote: > > > > On 3/7/25 03:00, Dave Chinner wrote: > > > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: > > > > Silently executing scripts during sourcing common/rc doesn't look good > > > > and also causes unnecessary script execution. Decouple init_rc() call > > > > and call init_rc() explicitly where required. > > > > > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> > > > FWIW, I've just done somethign similar for check-parallel. I need to > > > decouple common/config from common/rc and not run any code from > > > either common/config or common/rc. > > > > > > I've included the patch below (it won't apply because there's all > > > sorts of refactoring for test list and config-section parsing in the > > > series before it), but it should give you an idea of how I think we > > > should be separating one-off initialisation environment varaibles, > > > common code inclusion and the repeated initialisation of section > > > specific parameters.... > > Thank you so much. I can a look at this. > > > > > > ..... > > > > diff --git a/soak b/soak > > > > index d5c4229a..5734d854 100755 > > > > --- a/soak > > > > +++ b/soak > > > > @@ -5,6 +5,7 @@ > > > > # get standard environment, filters and checks > > > > . ./common/rc > > > > +# ToDo: Do we need an init_rc() here? How is soak used? > > > > . ./common/filter > > > I've also go a patch series that removes all these old 2000-era SGI > > > QE scripts that have not been used by anyone for the last 15 > > > years. I did that to get rid of the technical debt that these > > > scripts have gathered over years of neglect. They aren't used, we > > > shouldn't even attempt to maintain them anymore. > > > > Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do > > you mean some kind of CI/automation-test script? > > SGI is Silicon Graphics International Corp. : > https://en.wikipedia.org/wiki/Silicon_Graphics_International > > xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX) > of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert > of all these things, and knows lots of past details :) Hi Nirjhar, I've merged Dave's "[PATCH 0/5] fstests: remove old SGI QE scripts" into patches-in-queue branch. You can base on that to write your V2, to avoid dealing with the "soak" file. Thanks, Zorro > > Thanks, > Zorro > > > > > --NR > > > > > > > > -Dave. > > > > > -- > > Nirjhar Roy > > Linux Kernel Developer > > IBM, Bangalore > > > >
On 3/10/25 13:36, Zorro Lang wrote: > On Sat, Mar 08, 2025 at 03:20:34PM +0800, Zorro Lang wrote: >> On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote: >>> On 3/7/25 03:00, Dave Chinner wrote: >>>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >>>>> Silently executing scripts during sourcing common/rc doesn't look good >>>>> and also causes unnecessary script execution. Decouple init_rc() call >>>>> and call init_rc() explicitly where required. >>>>> >>>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> >>>> FWIW, I've just done somethign similar for check-parallel. I need to >>>> decouple common/config from common/rc and not run any code from >>>> either common/config or common/rc. >>>> >>>> I've included the patch below (it won't apply because there's all >>>> sorts of refactoring for test list and config-section parsing in the >>>> series before it), but it should give you an idea of how I think we >>>> should be separating one-off initialisation environment varaibles, >>>> common code inclusion and the repeated initialisation of section >>>> specific parameters.... >>> Thank you so much. I can a look at this. >>>> ..... >>>>> diff --git a/soak b/soak >>>>> index d5c4229a..5734d854 100755 >>>>> --- a/soak >>>>> +++ b/soak >>>>> @@ -5,6 +5,7 @@ >>>>> # get standard environment, filters and checks >>>>> . ./common/rc >>>>> +# ToDo: Do we need an init_rc() here? How is soak used? >>>>> . ./common/filter >>>> I've also go a patch series that removes all these old 2000-era SGI >>>> QE scripts that have not been used by anyone for the last 15 >>>> years. I did that to get rid of the technical debt that these >>>> scripts have gathered over years of neglect. They aren't used, we >>>> shouldn't even attempt to maintain them anymore. >>> Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do >>> you mean some kind of CI/automation-test script? >> SGI is Silicon Graphics International Corp. : >> https://en.wikipedia.org/wiki/Silicon_Graphics_International >> >> xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX) >> of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert >> of all these things, and knows lots of past details :) > Hi Nirjhar, > > I've merged Dave's "[PATCH 0/5] fstests: remove old SGI QE scripts" into > patches-in-queue branch. You can base on that to write your V2, to avoid > dealing with the "soak" file. > > Thanks, > Zorro Okay, thank you for the pointer. I will send the v2 after rebasing. --NR > >> Thanks, >> Zorro >> >>> --NR >>> >>>> -Dave. >>>> >>> -- >>> Nirjhar Roy >>> Linux Kernel Developer >>> IBM, Bangalore >>> >>>
On 3/8/25 12:50, Zorro Lang wrote: > On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote: >> On 3/7/25 03:00, Dave Chinner wrote: >>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >>>> Silently executing scripts during sourcing common/rc doesn't look good >>>> and also causes unnecessary script execution. Decouple init_rc() call >>>> and call init_rc() explicitly where required. >>>> >>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> >>> FWIW, I've just done somethign similar for check-parallel. I need to >>> decouple common/config from common/rc and not run any code from >>> either common/config or common/rc. >>> >>> I've included the patch below (it won't apply because there's all >>> sorts of refactoring for test list and config-section parsing in the >>> series before it), but it should give you an idea of how I think we >>> should be separating one-off initialisation environment varaibles, >>> common code inclusion and the repeated initialisation of section >>> specific parameters.... >> Thank you so much. I can a look at this. >>> ..... >>>> diff --git a/soak b/soak >>>> index d5c4229a..5734d854 100755 >>>> --- a/soak >>>> +++ b/soak >>>> @@ -5,6 +5,7 @@ >>>> # get standard environment, filters and checks >>>> . ./common/rc >>>> +# ToDo: Do we need an init_rc() here? How is soak used? >>>> . ./common/filter >>> I've also go a patch series that removes all these old 2000-era SGI >>> QE scripts that have not been used by anyone for the last 15 >>> years. I did that to get rid of the technical debt that these >>> scripts have gathered over years of neglect. They aren't used, we >>> shouldn't even attempt to maintain them anymore. >> Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do >> you mean some kind of CI/automation-test script? > SGI is Silicon Graphics International Corp. : > https://en.wikipedia.org/wiki/Silicon_Graphics_International > > xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX) > of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert > of all these things, and knows lots of past details :) > > Thanks, > Zorro Okay, got it. Thank you. --NR > >> --NR >> >>> -Dave. >>> >> -- >> Nirjhar Roy >> Linux Kernel Developer >> IBM, Bangalore >> >>
On 3/7/25 23:10, Darrick J. Wong wrote: > On Fri, Mar 07, 2025 at 11:21:15AM +0530, Nirjhar Roy (IBM) wrote: >> On 3/6/25 23:16, Darrick J. Wong wrote: >>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote: >>>> Silently executing scripts during sourcing common/rc doesn't look good >>>> and also causes unnecessary script execution. Decouple init_rc() call >>>> and call init_rc() explicitly where required. >>>> >>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> >>>> --- >>>> check | 10 ++-------- >>>> common/preamble | 1 + >>>> common/rc | 2 -- >>>> soak | 1 + >>>> 4 files changed, 4 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/check b/check >>>> index ea92b0d6..d30af1ba 100755 >>>> --- a/check >>>> +++ b/check >>>> @@ -840,16 +840,8 @@ function run_section() >>>> _prepare_test_list >>>> elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then >>>> _test_unmount 2> /dev/null >>>> - if ! _test_mount >>>> - then >>>> - echo "check: failed to mount $TEST_DEV on $TEST_DIR" >>>> - status=1 >>>> - exit >>>> - fi >>> Unrelated change? I was expecting a mechanical ". ./common/rc" => >>> ". ./common/rc ; init_rc" change in this patch. >> This patch adds an init_rc() call to _begin_fstests() in common/preamble and >> hence the above _test_mount() will be executed during that call. So this >> _test_mount isn't necessary here, right? _test_mount() will be executed (as >> a part of init_rc() call) before every test run. Please let me know if my >> understanding isn't correct. > It's true that in terms of getting the test filesystem mounted, the > _test_mount here and in init_rc are redundant. But look at what happens > on error here -- we print "check: failed to mount..." to signal that the > new section's TEST_FS_MOUNT_OPTS are not valid, and exit the ./check > process. > > By deferring the mount to the init_rc in _preamble, that means that > we'll run the whole section with bad mount options, most likely > resulting in every test spewing "common/rc: could not mount..." and > appearing to fail. Aah, right. The exit should be at the check level if some parameter is not correct in a section. I will make the change in v2. > > I think. I'm not sure what "status=1; exit" does as compared to > "exit 1"; AFAICT the former actually results in an exit code of 0 > because the (otherwise pointless) assignment succeeds. I think "status=0; exit" has a reason. 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". However, when I executed 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 TEST_FS_MOUNT_OPTS="-o invalidss" This caused the "elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then" to be executed. Now, when I checked the value of "echo $?", it showed 1. IMO, this is the correct behavior, and we should always use "status=<value>; exit" and NOT "exit 1" directly. Even if 1 section fails, "./check <test-list>" command should return a non-zero value. Can you please let me know if my understanding is correct? If yes, maybe we can have a function like _set_status_and_exit() { status="$1" exit } and replace all the "status <value>; exit" and "exit <value>" with "_set_status_and_exit <value>" --NR > > Granted, the init_rc that you remove below would also catch that case > and exit ./check Yes. init_rc can catch that case with an additional difference that it will attempt another mount "retrying test device mount with external set" > >>>> fi >>>> - init_rc >>> Why remove init_rc here? >> Same reason as above. > But that's an additional change in behavior. If there's no reason for > calling init_rc() from run_section() then that should be a separate > patch with a separate justification. Since the check for _test_mount should be at the check script level and not at the _begin_fstest(), maybe we should 1. Keep the init_rc call here 2. Remove the _test_mount above (the one with "elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then" ) and have a separate patch for it with proper justification. 3. NOT have any init_rc call in _begin_fstest(), since the _test_mount related checks would already been done by the time _begin_fstests() gets executed. The above changes will also not change any existing behavior. Can you please let me know your thoughts and I can send a V2 accordingly? --NR > > --D > >>>> - >>>> seq="check.$$" >>>> check="$RESULT_BASE/check" >>>> @@ -870,6 +862,8 @@ function run_section() >>>> needwrap=true >>>> if [ ! -z "$SCRATCH_DEV" ]; then >>>> + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT >>>> + [ $? -le 1 ] || exit 1 >>>> _scratch_unmount 2> /dev/null >>>> # call the overridden mkfs - make sure the FS is built >>>> # the same as we'll create it later. >>>> diff --git a/common/preamble b/common/preamble >>>> index 0c9ee2e0..c92e55bb 100644 >>>> --- a/common/preamble >>>> +++ b/common/preamble >>>> @@ -50,6 +50,7 @@ _begin_fstest() >>>> _register_cleanup _cleanup >>>> . ./common/rc >>>> + init_rc >>>> # remove previous $seqres.full before test >>>> rm -f $seqres.full $seqres.hints >>>> diff --git a/common/rc b/common/rc >>>> index d2de8588..f153ad81 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -5754,8 +5754,6 @@ _require_program() { >>>> _have_program "$1" || _notrun "$tag required" >>>> } >>>> -init_rc >>>> - >>>> ################################################################################ >>>> # make sure this script returns success >>>> /bin/true >>>> diff --git a/soak b/soak >>>> index d5c4229a..5734d854 100755 >>>> --- a/soak >>>> +++ b/soak >>>> @@ -5,6 +5,7 @@ >>>> # get standard environment, filters and checks >>>> . ./common/rc >>>> +# ToDo: Do we need an init_rc() here? How is soak used? >>> I have no idea what soak does and have never used it, but I think for >>> continuity's sake you should call init_rc here. >> Okay. I think Dave has suggested removing this file[1]. This doesn't seem to >> used anymore. >> >> [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/ >> >> --NR >> >>> --D >>> >>>> . ./common/filter >>>> tmp=/tmp/$$ >>>> -- >>>> 2.34.1 >>>> >>>> >> -- >> Nirjhar Roy >> Linux Kernel Developer >> IBM, Bangalore >> >>
diff --git a/check b/check index ea92b0d6..d30af1ba 100755 --- a/check +++ b/check @@ -840,16 +840,8 @@ function run_section() _prepare_test_list elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then _test_unmount 2> /dev/null - if ! _test_mount - then - echo "check: failed to mount $TEST_DEV on $TEST_DIR" - status=1 - exit - fi fi - init_rc - seq="check.$$" check="$RESULT_BASE/check" @@ -870,6 +862,8 @@ function run_section() needwrap=true if [ ! -z "$SCRATCH_DEV" ]; then + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT + [ $? -le 1 ] || exit 1 _scratch_unmount 2> /dev/null # call the overridden mkfs - make sure the FS is built # the same as we'll create it later. diff --git a/common/preamble b/common/preamble index 0c9ee2e0..c92e55bb 100644 --- a/common/preamble +++ b/common/preamble @@ -50,6 +50,7 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc + init_rc # remove previous $seqres.full before test rm -f $seqres.full $seqres.hints diff --git a/common/rc b/common/rc index d2de8588..f153ad81 100644 --- a/common/rc +++ b/common/rc @@ -5754,8 +5754,6 @@ _require_program() { _have_program "$1" || _notrun "$tag required" } -init_rc - ################################################################################ # make sure this script returns success /bin/true diff --git a/soak b/soak index d5c4229a..5734d854 100755 --- a/soak +++ b/soak @@ -5,6 +5,7 @@ # get standard environment, filters and checks . ./common/rc +# ToDo: Do we need an init_rc() here? How is soak used? . ./common/filter tmp=/tmp/$$
Silently executing scripts during sourcing common/rc doesn't look good and also causes unnecessary script execution. Decouple init_rc() call and call init_rc() explicitly where required. Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> --- check | 10 ++-------- common/preamble | 1 + common/rc | 2 -- soak | 1 + 4 files changed, 4 insertions(+), 10 deletions(-)