On Fri, Apr 25, 2025 at 05:33:24PM +0530, Nirjhar Roy (IBM) wrote: > > On 4/25/25 16:57, Zorro Lang wrote: > > On Thu, Apr 24, 2025 at 02:39:39PM +0530, Nirjhar Roy (IBM) wrote: > > > On 4/23/25 19:48, Zorro Lang wrote: > > > > On Wed, Apr 23, 2025 at 06:41:34AM +0000, Nirjhar Roy (IBM) wrote: > > > > > Introduce a new file common/exit that will contain all the exit > > > > > related functions. This will remove the dependencies these functions > > > > > have on other non-related helper files and they can be indepedently > > > > > sourced. This was suggested by Dave Chinner[1]. > > > > > > > > > > [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@xxxxxxxxxxxxxxxxxxx/ > > > > > Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx> > > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@xxxxxxxxx> > > > > > --- > > > > > check | 1 + > > > > > common/btrfs | 2 +- > > > > > common/ceph | 2 ++ > > > > > common/config | 17 +---------------- > > > > > common/dump | 1 + > > > > > common/exit | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > common/ext4 | 2 +- > > > > > common/populate | 2 +- > > > > > common/preamble | 1 + > > > > > common/punch | 6 +----- > > > > > common/rc | 29 +--------------------------- > > > > > common/repair | 1 + > > > > > common/xfs | 1 + > > > > I think if you define exit helpers in common/exit, and import common/exit > > > > in common/config, then you don't need to source it(common/exit) in other > > > > common files (.e.g common/xfs, common/rc, etc). Due to when we call the > > > > helpers in these common files, the process should already imported > > > > common/rc -> common/config -> common/exit. right? > > > Oh, right. I can remove the redundant imports from > > > common/{btrfs,ceph,dump,ext4,populate,preamble,punch,rc,repair,xfs} in v2. I > > > will keep ". common/exit" only in common/config and check. The reason for me > > > to keep it in check is that before common/rc is sourced in check, we might > > > need _exit() (which is present is common/exit). Do you agree? > > I thought "check" might not need that either. I didn't give it a test, but I found > > before importing common/rc, there're only command arguments initialization, and > > "check" calls "exit" directly if the initialization fails (except you want to call > > _exit, but I didn't see you change that). > > Yes, I have changed the exit() to _exit() in "check" in the next patch [1] > of this series. Can you please take a look at that patch[1] and suggest > whether I should have ". common/exit" in "check" or not? > > > [1] https://lore.kernel.org/all/7d8587b8342ee2cbe226fb691b372ac7df5fdb71.1745390030.git.nirjhar.roy.lists@xxxxxxxxx/ Oh, as "check" has: if $OPTIONS_HAVE_SECTIONS; then trap "_summary; exit \$status" 0 1 2 3 15 else trap "_wrapup; exit \$status" 0 1 2 3 15 fi So I think it makes sense to use _exit() to deal with status variable :) > > --NR > > > > > Thanks, > > Zorro > > > > > --NR > > > > > > > Thanks, > > > > Zorro > > > > > > > > > 13 files changed, 63 insertions(+), 52 deletions(-) > > > > > create mode 100644 common/exit > > > > > > > > > > diff --git a/check b/check > > > > > index 9451c350..67355c52 100755 > > > > > --- a/check > > > > > +++ b/check > > > > > @@ -51,6 +51,7 @@ rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist > > > > > SRC_GROUPS="generic" > > > > > export SRC_DIR="tests" > > > > > +. common/exit > > > > > usage() > > > > > { > > > > > diff --git a/common/btrfs b/common/btrfs > > > > > index 3725632c..9e91ee71 100644 > > > > > --- a/common/btrfs > > > > > +++ b/common/btrfs > > > > > @@ -1,7 +1,7 @@ > > > > > # > > > > > # Common btrfs specific functions > > > > > # > > > > > - > > > > > +. common/exit > > > > > . common/module > > > > > # The recommended way to execute simple "btrfs" command. > > > > > diff --git a/common/ceph b/common/ceph > > > > > index df7a6814..89e36403 100644 > > > > > --- a/common/ceph > > > > > +++ b/common/ceph > > > > > @@ -2,6 +2,8 @@ > > > > > # CephFS specific common functions. > > > > > # > > > > > +. common/exit > > > > > + > > > > > # _ceph_create_file_layout <filename> <stripe unit> <stripe count> <object size> > > > > > # This function creates a new empty file and sets the file layout according to > > > > > # parameters. It will exit if the file already exists. > > > > > diff --git a/common/config b/common/config > > > > > index eada3971..6a60d144 100644 > > > > > --- a/common/config > > > > > +++ b/common/config > > > > > @@ -38,7 +38,7 @@ > > > > > # - this script shouldn't make any assertions about filesystem > > > > > # validity or mountedness. > > > > > # > > > > > - > > > > > +. common/exit > > > > > . common/test_names > > > > > # all tests should use a common language setting to prevent golden > > > > > @@ -96,15 +96,6 @@ 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() > > > > > { > > > > > @@ -121,12 +112,6 @@ set_mkfs_prog_path_with_opts() > > > > > fi > > > > > } > > > > > -_fatal() > > > > > -{ > > > > > - echo "$*" > > > > > - _exit 1 > > > > > -} > > > > > - > > > > > export MKFS_PROG="$(type -P mkfs)" > > > > > [ "$MKFS_PROG" = "" ] && _fatal "mkfs not found" > > > > > diff --git a/common/dump b/common/dump > > > > > index 09859006..4701a956 100644 > > > > > --- a/common/dump > > > > > +++ b/common/dump > > > > > @@ -3,6 +3,7 @@ > > > > > # Copyright (c) 2000-2002,2005 Silicon Graphics, Inc. All Rights Reserved. > > > > > # > > > > > # Functions useful for xfsdump/xfsrestore tests > > > > > +. common/exit > > > > > # --- initializations --- > > > > > rm -f $seqres.full > > > > > diff --git a/common/exit b/common/exit > > > > > new file mode 100644 > > > > > index 00000000..ad7e7498 > > > > > --- /dev/null > > > > > +++ b/common/exit > > > > > @@ -0,0 +1,50 @@ > > > > > +##/bin/bash > > > > > + > > > > > +# 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" > > > > > +} > > > > > + > > > > > +_fatal() > > > > > +{ > > > > > + echo "$*" > > > > > + _exit 1 > > > > > +} > > > > > + > > > > > +_die() > > > > > +{ > > > > > + echo $@ > > > > > + _exit 1 > > > > > +} > > > > > + > > > > > +die_now() > > > > > +{ > > > > > + _exit 1 > > > > > +} > > > > > + > > > > > +# just plain bail out > > > > > +# > > > > > +_fail() > > > > > +{ > > > > > + echo "$*" | tee -a $seqres.full > > > > > + echo "(see $seqres.full for details)" > > > > > + _exit 1 > > > > > +} > > > > > + > > > > > +# bail out, setting up .notrun file. Need to kill the filesystem check files > > > > > +# here, otherwise they are set incorrectly for the next test. > > > > > +# > > > > > +_notrun() > > > > > +{ > > > > > + echo "$*" > $seqres.notrun > > > > > + echo "$seq not run: $*" > > > > > + rm -f ${RESULT_DIR}/require_test* > > > > > + rm -f ${RESULT_DIR}/require_scratch* > > > > > + > > > > > + _exit 0 > > > > > +} > > > > > + > > > > > diff --git a/common/ext4 b/common/ext4 > > > > > index f88fa532..ab566c41 100644 > > > > > --- a/common/ext4 > > > > > +++ b/common/ext4 > > > > > @@ -1,7 +1,7 @@ > > > > > # > > > > > # ext4 specific common functions > > > > > # > > > > > - > > > > > +. common/exit > > > > > __generate_ext4_report_vars() { > > > > > __generate_blockdev_report_vars TEST_LOGDEV > > > > > __generate_blockdev_report_vars SCRATCH_LOGDEV > > > > > diff --git a/common/populate b/common/populate > > > > > index 50dc75d3..a17acc9e 100644 > > > > > --- a/common/populate > > > > > +++ b/common/populate > > > > > @@ -4,7 +4,7 @@ > > > > > # > > > > > # Routines for populating a scratch fs, and helpers to exercise an FS > > > > > # once it's been fuzzed. > > > > > - > > > > > +. common/exit > > > > > . ./common/quota > > > > > _require_populate_commands() { > > > > > diff --git a/common/preamble b/common/preamble > > > > > index ba029a34..0f306412 100644 > > > > > --- a/common/preamble > > > > > +++ b/common/preamble > > > > > @@ -3,6 +3,7 @@ > > > > > # Copyright (c) 2021 Oracle. All Rights Reserved. > > > > > # Boilerplate fstests functionality > > > > > +. common/exit > > > > > # Standard cleanup function. Individual tests can override this. > > > > > _cleanup() > > > > > diff --git a/common/punch b/common/punch > > > > > index 64d665d8..637f463f 100644 > > > > > --- a/common/punch > > > > > +++ b/common/punch > > > > > @@ -3,6 +3,7 @@ > > > > > # Copyright (c) 2007 Silicon Graphics, Inc. All Rights Reserved. > > > > > # > > > > > # common functions for excersizing hole punches with extent size hints etc. > > > > > +. common/exit > > > > > _spawn_test_file() { > > > > > echo "# spawning test file with $*" > > > > > @@ -222,11 +223,6 @@ _filter_bmap() > > > > > _coalesce_extents > > > > > } > > > > > -die_now() > > > > > -{ > > > > > - _exit 1 > > > > > -} > > > > > - > > > > > # test the different corner cases for zeroing a range: > > > > > # > > > > > # 1. into a hole > > > > > diff --git a/common/rc b/common/rc > > > > > index 9bed6dad..945f5134 100644 > > > > > --- a/common/rc > > > > > +++ b/common/rc > > > > > @@ -2,6 +2,7 @@ > > > > > # SPDX-License-Identifier: GPL-2.0+ > > > > > # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. > > > > > +. common/exit > > > > > . common/config > > > > > BC="$(type -P bc)" || BC= > > > > > @@ -1798,28 +1799,6 @@ _do() > > > > > return $ret > > > > > } > > > > > -# bail out, setting up .notrun file. Need to kill the filesystem check files > > > > > -# here, otherwise they are set incorrectly for the next test. > > > > > -# > > > > > -_notrun() > > > > > -{ > > > > > - echo "$*" > $seqres.notrun > > > > > - echo "$seq not run: $*" > > > > > - rm -f ${RESULT_DIR}/require_test* > > > > > - rm -f ${RESULT_DIR}/require_scratch* > > > > > - > > > > > - _exit 0 > > > > > -} > > > > > - > > > > > -# just plain bail out > > > > > -# > > > > > -_fail() > > > > > -{ > > > > > - echo "$*" | tee -a $seqres.full > > > > > - echo "(see $seqres.full for details)" > > > > > - _exit 1 > > > > > -} > > > > > - > > > > > # > > > > > # Tests whether $FSTYP should be exclude from this test. > > > > > # > > > > > @@ -3835,12 +3814,6 @@ _link_out_file() > > > > > _link_out_file_named $seqfull.out "$features" > > > > > } > > > > > -_die() > > > > > -{ > > > > > - echo $@ > > > > > - _exit 1 > > > > > -} > > > > > - > > > > > # convert urandom incompressible data to compressible text data > > > > > _ddt() > > > > > { > > > > > diff --git a/common/repair b/common/repair > > > > > index fd206f8e..db6a1b5c 100644 > > > > > --- a/common/repair > > > > > +++ b/common/repair > > > > > @@ -3,6 +3,7 @@ > > > > > # Copyright (c) 2000-2002 Silicon Graphics, Inc. All Rights Reserved. > > > > > # > > > > > # Functions useful for xfs_repair tests > > > > > +. common/exit > > > > > _zero_position() > > > > > { > > > > > diff --git a/common/xfs b/common/xfs > > > > > index 96c15f3c..c236146c 100644 > > > > > --- a/common/xfs > > > > > +++ b/common/xfs > > > > > @@ -1,6 +1,7 @@ > > > > > # > > > > > # XFS specific common functions. > > > > > # > > > > > +. common/exit > > > > > __generate_xfs_report_vars() { > > > > > __generate_blockdev_report_vars TEST_RTDEV > > > > > -- > > > > > 2.34.1 > > > > > > > > -- > > > Nirjhar Roy > > > Linux Kernel Developer > > > IBM, Bangalore > > > > -- > Nirjhar Roy > Linux Kernel Developer > IBM, Bangalore >
