[PATCH v3 0/2] common: Move exit related functions to common/exit

[PATCH v3 0/2] common: Move exit related functions to common/exit

[Nirjhar Roy (IBM)]

This patch series moves all the exit related functions to a separate file -
common/exit. This will remove the dependency to source non-related files to use
these exit related functions. Thanks to Dave for suggesting this[1]. The second
patch replaces exit with _exit in check file - I missed replacing them in [2].

[v2] -> v3
 Addressed Dave's feedbacks.
 In patch [1/2]
  - Removed _die() and die_now() from common/exit
  - Replaced die_now() with _fatal in common/punch
  - Removed sourcing of common/exit and common/test_names from common/config
    and moved them to the beginning of check.
  - Added sourcing of common/test_names in _begin_fstest() since common/config
    is no more sourcing common/test_names.
  - Added a blank line in _begin_fstest() after sourcing common/{exit,test_names}
 In patch [2/2]
  - Replaced "_exit 1" with _fatal and "echo <error message>; _exit 1" with
   _fatal <error message>.
  - Reverted to "exit \$status" in the trap handler registration in check - just
    to make it more obvious to the reader that we are capturing $status as the
    final exit value.

[v1] https://lore.kernel.org/all/cover.1745390030.git.nirjhar.roy.lists@gmail.com/
[v2] https://lore.kernel.org/all/cover.1745908976.git.nirjhar.roy.lists@gmail.com/
[1] https://lore.kernel.org/all/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
[2] https://lore.kernel.org/all/48dacdf636be19ae8bff66cc3852d27e28030613.1744181682.git.nirjhar.roy.lists@gmail.com/


Nirjhar Roy (IBM) (2):
  common: Move exit related functions to a common/exit
  check: Replace exit with _fatal and _exit in check

 check           | 54 ++++++++++++++++++-------------------------------
 common/config   | 17 ----------------
 common/exit     | 39 +++++++++++++++++++++++++++++++++++
 common/preamble |  3 +++
 common/punch    | 39 ++++++++++++++++-------------------
 common/rc       | 28 -------------------------
 6 files changed, 79 insertions(+), 101 deletions(-)
 create mode 100644 common/exit

--
2.34.1

[PATCH v3 1/2] common: Move exit related functions to a common/exit

[Nirjhar Roy (IBM)]

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].
While moving the exit related functions, remove _die() and die_now()
and replace die_now with _fatal(). It is of no use to keep the
unnecessary wrappers.

[1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 check           |  2 ++
 common/config   | 17 -----------------
 common/exit     | 39 +++++++++++++++++++++++++++++++++++++++
 common/preamble |  3 +++
 common/punch    | 39 +++++++++++++++++----------------------
 common/rc       | 28 ----------------------------
 6 files changed, 61 insertions(+), 67 deletions(-)
 create mode 100644 common/exit

diff --git a/check b/check
index 9451c350..bd84f213 100755
--- a/check
+++ b/check
@@ -46,6 +46,8 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
 
 # by default don't output timestamps
 timestamp=${TIMESTAMP:=false}
+. common/exit
+. common/test_names
 
 rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
 
diff --git a/common/config b/common/config
index eada3971..22b52432 100644
--- a/common/config
+++ b/common/config
@@ -39,8 +39,6 @@
 #   validity or mountedness.
 #
 
-. common/test_names
-
 # all tests should use a common language setting to prevent golden
 # output mismatches.
 export LANG=C
@@ -96,15 +94,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 +110,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/exit b/common/exit
new file mode 100644
index 00000000..222c79b7
--- /dev/null
+++ b/common/exit
@@ -0,0 +1,39 @@
+##/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
+}
+
+# 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/preamble b/common/preamble
index ba029a34..51d03396 100644
--- a/common/preamble
+++ b/common/preamble
@@ -33,6 +33,9 @@ _register_cleanup()
 # explicitly as a member of the 'all' group.
 _begin_fstest()
 {
+	. common/exit
+	. common/test_names
+
 	if [ -n "$seq" ]; then
 		echo "_begin_fstest can only be called once!"
 		_exit 1
diff --git a/common/punch b/common/punch
index 64d665d8..ddbe757a 100644
--- a/common/punch
+++ b/common/punch
@@ -222,11 +222,6 @@ _filter_bmap()
 	_coalesce_extents
 }
 
-die_now()
-{
-	_exit 1
-}
-
 # test the different corner cases for zeroing a range:
 #
 #	1. into a hole
@@ -305,7 +300,7 @@ _test_generic_punch()
 	$XFS_IO_PROG -f -c "truncate $_20k" \
 		-c "$zero_cmd $_4k $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	echo "	2. into allocated space"
@@ -316,7 +311,7 @@ _test_generic_punch()
 		-c "pwrite 0 $_20k" $sync_cmd \
 		-c "$zero_cmd $_4k $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	if [ "$unwritten_tests" ]; then
@@ -328,7 +323,7 @@ _test_generic_punch()
 			-c "$alloc_cmd 0 $_20k" \
 			-c "$zero_cmd $_4k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 	fi
 
@@ -340,7 +335,7 @@ _test_generic_punch()
 		-c "pwrite $_8k $_8k" $sync_cmd \
 		-c "$zero_cmd $_4k $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	if [ "$unwritten_tests" ]; then
@@ -352,7 +347,7 @@ _test_generic_punch()
 			-c "$alloc_cmd $_8k $_8k" \
 			-c "$zero_cmd $_4k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 	fi
 
@@ -364,7 +359,7 @@ _test_generic_punch()
 		-c "pwrite 0 $_8k" $sync_cmd \
 		 -c "$zero_cmd $_4k $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	if [ "$unwritten_tests" ]; then
@@ -377,7 +372,7 @@ _test_generic_punch()
 			-c "$alloc_cmd $_8k $_8k" \
 			-c "$zero_cmd $_4k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 
 		echo "	8. unwritten -> hole"
@@ -388,7 +383,7 @@ _test_generic_punch()
 			-c "$alloc_cmd 0 $_8k" \
 			-c "$zero_cmd $_4k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 
 		echo "	9. unwritten -> data"
@@ -400,7 +395,7 @@ _test_generic_punch()
 			-c "pwrite $_8k $_8k" $sync_cmd \
 			-c "$zero_cmd $_4k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 	fi
 
@@ -412,7 +407,7 @@ _test_generic_punch()
 		-c "pwrite $_8k $_4k" $sync_cmd \
 		-c "$zero_cmd $_4k $_12k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	echo "	11. data -> hole -> data"
@@ -426,7 +421,7 @@ _test_generic_punch()
 		-c "$punch_cmd $_8k $_4k" \
 		-c "$zero_cmd $_4k $_12k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	if [ "$unwritten_tests" ]; then
@@ -439,7 +434,7 @@ _test_generic_punch()
 			-c "pwrite $_8k $_4k" $sync_cmd \
 			-c "$zero_cmd $_4k $_12k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 
 		echo "	13. data -> unwritten -> data"
@@ -452,7 +447,7 @@ _test_generic_punch()
 			-c "pwrite $_12k $_8k" -c "fsync" \
 			-c "$zero_cmd $_4k $_12k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 	fi
 
@@ -466,7 +461,7 @@ _test_generic_punch()
 			-c "pwrite 0 $_20k" $sync_cmd \
 			-c "$zero_cmd $_12k $_8k" \
 			-c "$map_cmd -v" $testfile | $filter_cmd
-		[ $? -ne 0 ] && die_now
+		[ $? -ne 0 ] && _fatal
 		_md5_checksum $testfile
 	fi
 
@@ -483,7 +478,7 @@ _test_generic_punch()
 		-c "pwrite 0 $_20k" $sync_cmd \
 		-c "$zero_cmd 0 $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	_md5_checksum $testfile
 
 	# If zero_cmd is fcollpase, don't check unaligned offsets
@@ -512,7 +507,7 @@ _test_generic_punch()
 		-c "fadvise -d" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
 	diff $testfile $testfile.2
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	rm -f $testfile.2
 	_md5_checksum $testfile
 
@@ -532,7 +527,7 @@ _test_generic_punch()
 		-c "$zero_cmd 128 128" \
 		-c "$map_cmd -v" $testfile | $filter_cmd | \
 			 sed -e "s/\.\.[0-9]*\]/..7\]/"
-	[ $? -ne 0 ] && die_now
+	[ $? -ne 0 ] && _fatal
 	od -x $testfile | head -n -1
 }
 
diff --git a/common/rc b/common/rc
index 9bed6dad..fac9b6da 100644
--- a/common/rc
+++ b/common/rc
@@ -1798,28 +1798,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 +3813,6 @@ _link_out_file()
 	_link_out_file_named $seqfull.out "$features"
 }
 
-_die()
-{
-        echo $@
-        _exit 1
-}
-
 # convert urandom incompressible data to compressible text data
 _ddt()
 {
-- 
2.34.1

[PATCH v3 2/2] check: Replace exit with _fatal and _exit in check

[Nirjhar Roy (IBM)]

Some of the "status=<val>;exit" and "exit <val>" were not
replaced with _exit <val> and _fatal. Doing it now.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 check | 52 ++++++++++++++++++----------------------------------
 1 file changed, 18 insertions(+), 34 deletions(-)

diff --git a/check b/check
index bd84f213..ede54f69 100755
--- a/check
+++ b/check
@@ -123,7 +123,7 @@ examples:
  check -X .exclude -g auto
  check -E ~/.xfstests.exclude
 '
-	    exit 1
+	    _fatal
 }
 
 get_sub_group_list()
@@ -232,8 +232,7 @@ _prepare_test_list()
 		for group in $GROUP_LIST; do
 			list=$(get_group_list $group)
 			if [ -z "$list" ]; then
-				echo "Group \"$group\" is empty or not defined?"
-				exit 1
+				_fatal "Group \"$group\" is empty or not defined?"
 			fi
 
 			for t in $list; do
@@ -317,15 +316,13 @@ while [ $# -gt 0 ]; do
 	-n)	showme=true ;;
 	-r)
 		if $exact_order; then
-			echo "Cannot specify -r and --exact-order."
-			exit 1
+			_fatal "Cannot specify -r and --exact-order."
 		fi
 		randomize=true
 		;;
 	--exact-order)
 		if $randomize; then
-			echo "Cannnot specify --exact-order and -r."
-			exit 1
+			_fatal "Cannnot specify --exact-order and -r."
 		fi
 		exact_order=true
 		;;
@@ -362,8 +359,7 @@ done
 # we need common/rc, that also sources common/config. We need to source it
 # after processing args, overlay needs FSTYP set before sourcing common/config
 if ! . ./common/rc; then
-	echo "check: failed to source common/rc"
-	exit 1
+	_fatal "check: failed to source common/rc"
 fi
 
 init_rc
@@ -375,8 +371,7 @@ if [ -n "$SOAK_DURATION" ]; then
 		sed -e 's/^\([.0-9]*\)\([a-z]\)*/\1 \2/g' | \
 		$AWK_PROG -f $here/src/soak_duration.awk)"
 	if [ $? -ne 0 ]; then
-		status=1
-		exit 1
+		_fatal
 	fi
 fi
 
@@ -387,8 +382,7 @@ if [ -n "$FUZZ_REWRITE_DURATION" ]; then
 		sed -e 's/^\([.0-9]*\)\([a-z]\)*/\1 \2/g' | \
 		$AWK_PROG -f $here/src/soak_duration.awk)"
 	if [ $? -ne 0 ]; then
-		status=1
-		exit 1
+		_fatal
 	fi
 fi
 
@@ -405,9 +399,7 @@ fi
 if $have_test_arg; then
 	while [ $# -gt 0 ]; do
 		case "$1" in
-		-*)	echo "Arguments before tests, please!"
-			status=1
-			exit $status
+		-*)	_fatal "Arguments before tests, please!"
 			;;
 		*)	# Expand test pattern (e.g. xfs/???, *fs/001)
 			list=$(cd $SRC_DIR; echo $1)
@@ -439,8 +431,7 @@ fi
 
 if [ `id -u` -ne 0 ]
 then
-    echo "check: QA must be run as root"
-    exit 1
+    _fatal "check: QA must be run as root"
 fi
 
 _wipe_counters()
@@ -722,6 +713,9 @@ _detect_kmemleak
 _prepare_test_list
 fstests_start_time="$(date +"%F %T")"
 
+# We are not using _exit in the trap handler so that it is obvious to the reader
+# that we are using the last set value of "status" before we finally exit
+# from the check script.
 if $OPTIONS_HAVE_SECTIONS; then
 	trap "_summary; exit \$status" 0 1 2 3 15
 else
@@ -768,9 +762,7 @@ function run_section()
 
 	mkdir -p $RESULT_BASE
 	if [ ! -d $RESULT_BASE ]; then
-		echo "failed to create results directory $RESULT_BASE"
-		status=1
-		exit
+		_fatal "failed to create results directory $RESULT_BASE"
 	fi
 
 	if $OPTIONS_HAVE_SECTIONS; then
@@ -785,9 +777,7 @@ function run_section()
 		then
 			echo "our local _test_mkfs routine ..."
 			cat $tmp.err
-			echo "check: failed to mkfs \$TEST_DEV using specified options"
-			status=1
-			exit
+			_fatal "check: failed to mkfs \$TEST_DEV using specified options"
 		fi
 		# Previous FSTYP derived from TEST_DEV could be changed, source
 		# common/rc again with correct FSTYP to get FSTYP specific configs,
@@ -830,9 +820,7 @@ function run_section()
 	  then
 	      echo "our local _scratch_mkfs routine ..."
 	      cat $tmp.err
-	      echo "check: failed to mkfs \$SCRATCH_DEV using specified options"
-	      status=1
-	      exit
+	      _fatal "check: failed to mkfs \$SCRATCH_DEV using specified options"
 	  fi
 
 	  # call the overridden mount - make sure the FS mounts with
@@ -841,9 +829,7 @@ function run_section()
 	  then
 	      echo "our local mount routine ..."
 	      cat $tmp.err
-	      echo "check: failed to mount \$SCRATCH_DEV using specified options"
-	      status=1
-	      exit
+	      _fatal "check: failed to mount \$SCRATCH_DEV using specified options"
 	  else
 	      _scratch_unmount
 	  fi
@@ -1106,12 +1092,10 @@ for ((iters = 0; iters < $iterations; iters++)) do
 		run_section $section
 		if [ "$sum_bad" != 0 ] && [ "$istop" = true ]; then
 			interrupt=false
-			status=`expr $sum_bad != 0`
-			exit
+			_exit `expr $sum_bad != 0`
 		fi
 	done
 done
 
 interrupt=false
-status=`expr $sum_bad != 0`
-exit
+_exit `expr $sum_bad != 0`
-- 
2.34.1

Re: [PATCH v3 1/2] common: Move exit related functions to a common/exit

[Ritesh Harjani]

"Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:

> 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].
> While moving the exit related functions, remove _die() and die_now()
> and replace die_now with _fatal(). It is of no use to keep the
> unnecessary wrappers.
>
> [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  check           |  2 ++
>  common/config   | 17 -----------------
>  common/exit     | 39 +++++++++++++++++++++++++++++++++++++++
>  common/preamble |  3 +++
>  common/punch    | 39 +++++++++++++++++----------------------
>  common/rc       | 28 ----------------------------
>  6 files changed, 61 insertions(+), 67 deletions(-)
>  create mode 100644 common/exit
>
> diff --git a/check b/check
> index 9451c350..bd84f213 100755
> --- a/check
> +++ b/check
> @@ -46,6 +46,8 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>  
>  # by default don't output timestamps
>  timestamp=${TIMESTAMP:=false}
> +. common/exit
> +. common/test_names

So this gets sourced at the beginning of check script here.

>  
>  rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>  
<...>
> diff --git a/common/preamble b/common/preamble
> index ba029a34..51d03396 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -33,6 +33,9 @@ _register_cleanup()
>  # explicitly as a member of the 'all' group.
>  _begin_fstest()
>  {
> +	. common/exit
> +	. common/test_names
> +

Why do we need to source these files here again? 
Isn't check script already sourcing both of this in the beginning
itself?

-ritesh

Re: [PATCH v3 2/2] check: Replace exit with _fatal and _exit in check

[Ritesh Harjani]

"Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:

> Some of the "status=<val>;exit" and "exit <val>" were not
> replaced with _exit <val> and _fatal. Doing it now.
>

Indeed a nice cleanup. The changes in this patch looks good to me. 

Please feel free to add: 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


So I guess these couple of series was to cleanup exit routines from
common bash scripts. Do we plan to update the tests/ as well where
we call...
    status=X
    exit

...or updating tests/ is not needed since we didn't find any wrong usage of
"exit X" routines there?


Either ways - I think we might need to update the README at some point
in time which carries this snip. You might need to add that there are
helper routines like  _exit() and _fatal() perhaps for use in common
scripts.

<snip>
    To force a non-zero exit status use:
	status=1
	exit

    Note that:
	exit 1
    won't have the desired effect because of the way the exit trap
    works.


-ritesh

Re: [PATCH v3 1/2] common: Move exit related functions to a common/exit

[Zorro Lang]

On Thu, May 01, 2025 at 08:47:46AM +0530, Ritesh Harjani wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> 
> > 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].
> > While moving the exit related functions, remove _die() and die_now()
> > and replace die_now with _fatal(). It is of no use to keep the
> > unnecessary wrappers.
> >
> > [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> > ---
> >  check           |  2 ++
> >  common/config   | 17 -----------------
> >  common/exit     | 39 +++++++++++++++++++++++++++++++++++++++
> >  common/preamble |  3 +++
> >  common/punch    | 39 +++++++++++++++++----------------------
> >  common/rc       | 28 ----------------------------
> >  6 files changed, 61 insertions(+), 67 deletions(-)
> >  create mode 100644 common/exit
> >
> > diff --git a/check b/check
> > index 9451c350..bd84f213 100755
> > --- a/check
> > +++ b/check
> > @@ -46,6 +46,8 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
> >  
> >  # by default don't output timestamps
> >  timestamp=${TIMESTAMP:=false}
> > +. common/exit
> > +. common/test_names
> 
> So this gets sourced at the beginning of check script here.
> 
> >  
> >  rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
> >  
> <...>
> > diff --git a/common/preamble b/common/preamble
> > index ba029a34..51d03396 100644
> > --- a/common/preamble
> > +++ b/common/preamble
> > @@ -33,6 +33,9 @@ _register_cleanup()
> >  # explicitly as a member of the 'all' group.
> >  _begin_fstest()
> >  {
> > +	. common/exit
> > +	. common/test_names
> > +
> 
> Why do we need to source these files here again? 
> Isn't check script already sourcing both of this in the beginning
> itself?

The _begin_fstest is called at the beginning of each test case (e.g. generic/001).
And "check" run each test cases likes:

  cmd="generic/001"
  ./$cmd

So the imported things (by "check") can't help sub-case running

Thanks,
Zorro

> 
> -ritesh
> 

Re: [PATCH v3 1/2] common: Move exit related functions to a common/exit

[Ritesh Harjani]

Zorro Lang <zlang@redhat.com> writes:

> On Thu, May 01, 2025 at 08:47:46AM +0530, Ritesh Harjani wrote:
>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>> 
>> > 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].
>> > While moving the exit related functions, remove _die() and die_now()
>> > and replace die_now with _fatal(). It is of no use to keep the
>> > unnecessary wrappers.
>> >
>> > [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
>> > Suggested-by: Dave Chinner <david@fromorbit.com>
>> > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> > ---
>> >  check           |  2 ++
>> >  common/config   | 17 -----------------
>> >  common/exit     | 39 +++++++++++++++++++++++++++++++++++++++
>> >  common/preamble |  3 +++
>> >  common/punch    | 39 +++++++++++++++++----------------------
>> >  common/rc       | 28 ----------------------------
>> >  6 files changed, 61 insertions(+), 67 deletions(-)
>> >  create mode 100644 common/exit
>> >
>> > diff --git a/check b/check
>> > index 9451c350..bd84f213 100755
>> > --- a/check
>> > +++ b/check
>> > @@ -46,6 +46,8 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>> >  
>> >  # by default don't output timestamps
>> >  timestamp=${TIMESTAMP:=false}
>> > +. common/exit
>> > +. common/test_names
>> 
>> So this gets sourced at the beginning of check script here.
>> 
>> >  
>> >  rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>> >  
>> <...>
>> > diff --git a/common/preamble b/common/preamble
>> > index ba029a34..51d03396 100644
>> > --- a/common/preamble
>> > +++ b/common/preamble
>> > @@ -33,6 +33,9 @@ _register_cleanup()
>> >  # explicitly as a member of the 'all' group.
>> >  _begin_fstest()
>> >  {
>> > +	. common/exit
>> > +	. common/test_names
>> > +
>> 
>> Why do we need to source these files here again? 
>> Isn't check script already sourcing both of this in the beginning
>> itself?
>
> The _begin_fstest is called at the beginning of each test case (e.g. generic/001).
> And "check" run each test cases likes:
>
>   cmd="generic/001"
>   ./$cmd
>
> So the imported things (by "check") can't help sub-case running

aah right. Each testcase is inoked by "exec ./$seq" and it won't have
the function definitions sourced from the previous shell process. So we
will need to source the necessary files again within the test execution.

-ritesh

Re: [PATCH v3 2/2] check: Replace exit with _fatal and _exit in check

[Nirjhar Roy (IBM)]

On 5/1/25 09:01, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> Some of the "status=<val>;exit" and "exit <val>" were not
>> replaced with _exit <val> and _fatal. Doing it now.
>>
> Indeed a nice cleanup. The changes in this patch looks good to me.
>
> Please feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Thank you.
>
>
> So I guess these couple of series was to cleanup exit routines from
> common bash scripts. Do we plan to update the tests/ as well where
> we call...
>      status=X
>      exit
>
> ...or updating tests/ is not needed since we didn't find any wrong usage of
> "exit X" routines there?

Thank you for pointing this out. The exit command is used in 2 ways in 
the tests:

1. "exit 1"

2. "status=0; exit"

1) works because we set the value of "status" to 1 (failure by default) 
in _begin_fstest() - so even if "exit 1" is not correctly explicitly 
setting the value of "status", it simply works. However, "exit <any 
value != 1>" will not work (although I didn't find any place in tests 
where exit has been used with any other value apart from 0 and 1).

2) This works since we are setting the value of "status" correcting 
before "exit"ing.

But yes, we should ideally replace direct usage of exit with either 
_exit or _fatal (depending on the exit value). I will add this to my 
ToDo list and send a separate patch series with this and the README 
change you have suggested below.

>
>
> Either ways - I think we might need to update the README at some point
> in time which carries this snip. You might need to add that there are
> helper routines like  _exit() and _fatal() perhaps for use in common
> scripts.
>
> <snip>
>      To force a non-zero exit status use:
> 	status=1
> 	exit
>
>      Note that:
> 	exit 1
>      won't have the desired effect because of the way the exit trap
>      works.

I agree. I will send a separate patch with this and the exit call 
replacement of the tests.

--NR

>
>
> -ritesh

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v3 1/2] common: Move exit related functions to a common/exit

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Zorro Lang <zlang@redhat.com> writes:
>
>> On Thu, May 01, 2025 at 08:47:46AM +0530, Ritesh Harjani wrote:
>>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>> 
>>> > 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].
>>> > While moving the exit related functions, remove _die() and die_now()
>>> > and replace die_now with _fatal(). It is of no use to keep the
>>> > unnecessary wrappers.
>>> >
>>> > [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
>>> > Suggested-by: Dave Chinner <david@fromorbit.com>
>>> > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>>> > ---
>>> >  check           |  2 ++
>>> >  common/config   | 17 -----------------
>>> >  common/exit     | 39 +++++++++++++++++++++++++++++++++++++++
>>> >  common/preamble |  3 +++
>>> >  common/punch    | 39 +++++++++++++++++----------------------
>>> >  common/rc       | 28 ----------------------------
>>> >  6 files changed, 61 insertions(+), 67 deletions(-)
>>> >  create mode 100644 common/exit
>>> >
>>> > diff --git a/check b/check
>>> > index 9451c350..bd84f213 100755
>>> > --- a/check
>>> > +++ b/check
>>> > @@ -46,6 +46,8 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>>> >  
>>> >  # by default don't output timestamps
>>> >  timestamp=${TIMESTAMP:=false}
>>> > +. common/exit
>>> > +. common/test_names
>>> 
>>> So this gets sourced at the beginning of check script here.
>>> 
>>> >  
>>> >  rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>>> >  
>>> <...>
>>> > diff --git a/common/preamble b/common/preamble
>>> > index ba029a34..51d03396 100644
>>> > --- a/common/preamble
>>> > +++ b/common/preamble
>>> > @@ -33,6 +33,9 @@ _register_cleanup()
>>> >  # explicitly as a member of the 'all' group.
>>> >  _begin_fstest()
>>> >  {
>>> > +	. common/exit
>>> > +	. common/test_names
>>> > +
>>> 
>>> Why do we need to source these files here again? 
>>> Isn't check script already sourcing both of this in the beginning
>>> itself?
>>
>> The _begin_fstest is called at the beginning of each test case (e.g. generic/001).
>> And "check" run each test cases likes:
>>
>>   cmd="generic/001"
>>   ./$cmd
>>
>> So the imported things (by "check") can't help sub-case running
>
> aah right. Each testcase is inoked by "exec ./$seq" and it won't have

Ok. To be accurate, it is... 

bash -c "<...>; exec ./$seq"

& not just 

exec ./$seq

-ritesh

Re: [PATCH v3 0/2] common: Move exit related functions to common/exit

[Nirjhar Roy (IBM)]

On 4/30/25 18:15, Nirjhar Roy (IBM) wrote:
> This patch series moves all the exit related functions to a separate file -
> common/exit. This will remove the dependency to source non-related files to use
> these exit related functions. Thanks to Dave for suggesting this[1]. The second
> patch replaces exit with _exit in check file - I missed replacing them in [2].
>
> [v2] -> v3
>   Addressed Dave's feedbacks.
>   In patch [1/2]
>    - Removed _die() and die_now() from common/exit
>    - Replaced die_now() with _fatal in common/punch
>    - Removed sourcing of common/exit and common/test_names from common/config
>      and moved them to the beginning of check.
>    - Added sourcing of common/test_names in _begin_fstest() since common/config
>      is no more sourcing common/test_names.
>    - Added a blank line in _begin_fstest() after sourcing common/{exit,test_names}
>   In patch [2/2]
>    - Replaced "_exit 1" with _fatal and "echo <error message>; _exit 1" with
>     _fatal <error message>.
>    - Reverted to "exit \$status" in the trap handler registration in check - just
>      to make it more obvious to the reader that we are capturing $status as the
>      final exit value.

Hi Dave and Zorro,

Any further feedback in this version?

--NR

>
> [v1] https://lore.kernel.org/all/cover.1745390030.git.nirjhar.roy.lists@gmail.com/
> [v2] https://lore.kernel.org/all/cover.1745908976.git.nirjhar.roy.lists@gmail.com/
> [1] https://lore.kernel.org/all/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
> [2] https://lore.kernel.org/all/48dacdf636be19ae8bff66cc3852d27e28030613.1744181682.git.nirjhar.roy.lists@gmail.com/
>
>
> Nirjhar Roy (IBM) (2):
>    common: Move exit related functions to a common/exit
>    check: Replace exit with _fatal and _exit in check
>
>   check           | 54 ++++++++++++++++++-------------------------------
>   common/config   | 17 ----------------
>   common/exit     | 39 +++++++++++++++++++++++++++++++++++
>   common/preamble |  3 +++
>   common/punch    | 39 ++++++++++++++++-------------------
>   common/rc       | 28 -------------------------
>   6 files changed, 79 insertions(+), 101 deletions(-)
>   create mode 100644 common/exit
>
> --
> 2.34.1
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v3 0/2] common: Move exit related functions to common/exit

[Zorro Lang]

On Tue, May 06, 2025 at 02:19:07PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/30/25 18:15, Nirjhar Roy (IBM) wrote:
> > This patch series moves all the exit related functions to a separate file -
> > common/exit. This will remove the dependency to source non-related files to use
> > these exit related functions. Thanks to Dave for suggesting this[1]. The second
> > patch replaces exit with _exit in check file - I missed replacing them in [2].
> > 
> > [v2] -> v3
> >   Addressed Dave's feedbacks.
> >   In patch [1/2]
> >    - Removed _die() and die_now() from common/exit
> >    - Replaced die_now() with _fatal in common/punch
> >    - Removed sourcing of common/exit and common/test_names from common/config
> >      and moved them to the beginning of check.
> >    - Added sourcing of common/test_names in _begin_fstest() since common/config
> >      is no more sourcing common/test_names.
> >    - Added a blank line in _begin_fstest() after sourcing common/{exit,test_names}
> >   In patch [2/2]
> >    - Replaced "_exit 1" with _fatal and "echo <error message>; _exit 1" with
> >     _fatal <error message>.
> >    - Reverted to "exit \$status" in the trap handler registration in check - just
> >      to make it more obvious to the reader that we are capturing $status as the
> >      final exit value.
> 
> Hi Dave and Zorro,
> 
> Any further feedback in this version?

This version is good to me, if there's not more review points from others, I'll merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --NR
> 
> > 
> > [v1] https://lore.kernel.org/all/cover.1745390030.git.nirjhar.roy.lists@gmail.com/
> > [v2] https://lore.kernel.org/all/cover.1745908976.git.nirjhar.roy.lists@gmail.com/
> > [1] https://lore.kernel.org/all/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
> > [2] https://lore.kernel.org/all/48dacdf636be19ae8bff66cc3852d27e28030613.1744181682.git.nirjhar.roy.lists@gmail.com/
> > 
> > 
> > Nirjhar Roy (IBM) (2):
> >    common: Move exit related functions to a common/exit
> >    check: Replace exit with _fatal and _exit in check
> > 
> >   check           | 54 ++++++++++++++++++-------------------------------
> >   common/config   | 17 ----------------
> >   common/exit     | 39 +++++++++++++++++++++++++++++++++++
> >   common/preamble |  3 +++
> >   common/punch    | 39 ++++++++++++++++-------------------
> >   common/rc       | 28 -------------------------
> >   6 files changed, 79 insertions(+), 101 deletions(-)
> >   create mode 100644 common/exit
> > 
> > --
> > 2.34.1
> > 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
> 
>