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

[PATCH v2 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].

[v1] -> v2
 - Removed redundant sourcing of common/exit from common/{btrfs,ceph,dump,ext4,populate,punch,rc,repair,xfs}. Thanks to Zorro for pointing this out.
 - Moved the sourcing of common/exit in common/preamble from the beginning of the file to inside the function _begin_fstest()
 - Moved the sourcing of common/exit in check script from the patch 1 to patch 2 since patch 2 uses _exit().
 - Replaced exit() with _exit in the trap handler registration in the check script. Thanks to Zorro for pointing this out.

[v1] https://lore.kernel.org/all/cover.1745390030.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 _exit in check

 check           | 44 ++++++++++++++++++-------------------------
 common/config   | 17 +----------------
 common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 common/preamble |  1 +
 common/punch    |  5 -----
 common/rc       | 28 ---------------------------
 6 files changed, 70 insertions(+), 75 deletions(-)
 create mode 100644 common/exit

--
2.34.1

[PATCH v2 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].

[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>
---
 common/config   | 17 +----------------
 common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 common/preamble |  1 +
 common/punch    |  5 -----
 common/rc       | 28 ---------------------------
 5 files changed, 52 insertions(+), 49 deletions(-)
 create mode 100644 common/exit

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/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/preamble b/common/preamble
index ba029a34..9b6b4b26 100644
--- a/common/preamble
+++ b/common/preamble
@@ -33,6 +33,7 @@ _register_cleanup()
 # explicitly as a member of the 'all' group.
 _begin_fstest()
 {
+	. common/exit
 	if [ -n "$seq" ]; then
 		echo "_begin_fstest can only be called once!"
 		_exit 1
diff --git a/common/punch b/common/punch
index 64d665d8..4e8ebcd7 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
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 v2 2/2] check: Replace exit with _exit in check

[Nirjhar Roy (IBM)]

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

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

diff --git a/check b/check
index 9451c350..99d38492 100755
--- a/check
+++ b/check
@@ -47,6 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
 # by default don't output timestamps
 timestamp=${TIMESTAMP:=false}
 
+. common/exit
 rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
 
 SRC_GROUPS="generic"
@@ -121,7 +122,7 @@ examples:
  check -X .exclude -g auto
  check -E ~/.xfstests.exclude
 '
-	    exit 1
+	    _exit 1
 }
 
 get_sub_group_list()
@@ -231,7 +232,7 @@ _prepare_test_list()
 			list=$(get_group_list $group)
 			if [ -z "$list" ]; then
 				echo "Group \"$group\" is empty or not defined?"
-				exit 1
+				_exit 1
 			fi
 
 			for t in $list; do
@@ -316,14 +317,14 @@ while [ $# -gt 0 ]; do
 	-r)
 		if $exact_order; then
 			echo "Cannot specify -r and --exact-order."
-			exit 1
+			_exit 1
 		fi
 		randomize=true
 		;;
 	--exact-order)
 		if $randomize; then
 			echo "Cannnot specify --exact-order and -r."
-			exit 1
+			_exit 1
 		fi
 		exact_order=true
 		;;
@@ -361,7 +362,7 @@ done
 # after processing args, overlay needs FSTYP set before sourcing common/config
 if ! . ./common/rc; then
 	echo "check: failed to source common/rc"
-	exit 1
+	_exit 1
 fi
 
 init_rc
@@ -373,8 +374,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
+		_exit 1
 	fi
 fi
 
@@ -385,8 +385,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
+		_exit 1
 	fi
 fi
 
@@ -404,8 +403,7 @@ if $have_test_arg; then
 	while [ $# -gt 0 ]; do
 		case "$1" in
 		-*)	echo "Arguments before tests, please!"
-			status=1
-			exit $status
+			_exit 1
 			;;
 		*)	# Expand test pattern (e.g. xfs/???, *fs/001)
 			list=$(cd $SRC_DIR; echo $1)
@@ -438,7 +436,7 @@ fi
 if [ `id -u` -ne 0 ]
 then
     echo "check: QA must be run as root"
-    exit 1
+    _exit 1
 fi
 
 _wipe_counters()
@@ -721,9 +719,9 @@ _prepare_test_list
 fstests_start_time="$(date +"%F %T")"
 
 if $OPTIONS_HAVE_SECTIONS; then
-	trap "_summary; exit \$status" 0 1 2 3 15
+	trap "_summary; _exit" 0 1 2 3 15
 else
-	trap "_wrapup; exit \$status" 0 1 2 3 15
+	trap "_wrapup; _exit" 0 1 2 3 15
 fi
 
 function run_section()
@@ -767,8 +765,7 @@ function run_section()
 	mkdir -p $RESULT_BASE
 	if [ ! -d $RESULT_BASE ]; then
 		echo "failed to create results directory $RESULT_BASE"
-		status=1
-		exit
+		_exit 1
 	fi
 
 	if $OPTIONS_HAVE_SECTIONS; then
@@ -784,8 +781,7 @@ function run_section()
 			echo "our local _test_mkfs routine ..."
 			cat $tmp.err
 			echo "check: failed to mkfs \$TEST_DEV using specified options"
-			status=1
-			exit
+			_exit 1
 		fi
 		# Previous FSTYP derived from TEST_DEV could be changed, source
 		# common/rc again with correct FSTYP to get FSTYP specific configs,
@@ -829,8 +825,7 @@ function run_section()
 	      echo "our local _scratch_mkfs routine ..."
 	      cat $tmp.err
 	      echo "check: failed to mkfs \$SCRATCH_DEV using specified options"
-	      status=1
-	      exit
+	      _exit 1
 	  fi
 
 	  # call the overridden mount - make sure the FS mounts with
@@ -840,8 +835,7 @@ function run_section()
 	      echo "our local mount routine ..."
 	      cat $tmp.err
 	      echo "check: failed to mount \$SCRATCH_DEV using specified options"
-	      status=1
-	      exit
+	      _exit 1
 	  else
 	      _scratch_unmount
 	  fi
@@ -1104,12 +1098,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 v2 1/2] common: Move exit related functions to a common/exit

[Nirjhar Roy (IBM)]

On 4/29/25 12:22, 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].

Sorry, I didn't notice this earlier. A similar change[c1] was already 
posted by Dave.

[c1] 
https://lore.kernel.org/all/20250417031208.1852171-4-david@fromorbit.com/

--NR

>
> [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>
> ---
>   common/config   | 17 +----------------
>   common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>   common/preamble |  1 +
>   common/punch    |  5 -----
>   common/rc       | 28 ---------------------------
>   5 files changed, 52 insertions(+), 49 deletions(-)
>   create mode 100644 common/exit
>
> 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/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/preamble b/common/preamble
> index ba029a34..9b6b4b26 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -33,6 +33,7 @@ _register_cleanup()
>   # explicitly as a member of the 'all' group.
>   _begin_fstest()
>   {
> +	. common/exit
>   	if [ -n "$seq" ]; then
>   		echo "_begin_fstest can only be called once!"
>   		_exit 1
> diff --git a/common/punch b/common/punch
> index 64d665d8..4e8ebcd7 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
> 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()
>   {

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

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

[Dave Chinner]

On Tue, Apr 29, 2025 at 06:52:53AM +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@dread.disaster.area/
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  common/config   | 17 +----------------
>  common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  common/preamble |  1 +
>  common/punch    |  5 -----
>  common/rc       | 28 ---------------------------
>  5 files changed, 52 insertions(+), 49 deletions(-)
>  create mode 100644 common/exit

Neither of my replys to v1 made it to the list [1], so I'll have to
repeat what I said here.

I did point out that I had already sent this out here:

https://lore.kernel.org/fstests/20250417031208.1852171-4-david@fromorbit.com/

but now this version is mostly the same (except for things noted
below) so I'm good with this.

> 
> 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

This isn't needed. Include it in check and other high level scripts
(which need to include this, anyway) before including common/config.

> 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
> +}

This should be removed and replaced with _fatal

> +die_now()
> +{
> +	_exit 1
> +}

And this should be removed as well. 

i.e. These two functions are only used by common/punch, so change
them to use _fatal and _exit rather than duplicating the wrappers.

> diff --git a/common/preamble b/common/preamble
> index ba029a34..9b6b4b26 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -33,6 +33,7 @@ _register_cleanup()
>  # explicitly as a member of the 'all' group.
>  _begin_fstest()
>  {
> +	. common/exit
>  	if [ -n "$seq" ]; then
>  		echo "_begin_fstest can only be called once!"
>  		_exit 1

Please leave a blank line between includes and unrelated code.

-Dave.

[1] Thanks Google, for removing mail auth methods without any
warning and not reporting permanent delivery failure on attempts
to send mail an unsupported auth method.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/2] check: Replace exit with _exit in check

[Dave Chinner]

On Tue, Apr 29, 2025 at 06:52:54AM +0000, Nirjhar Roy (IBM) wrote:
> Some of the "status=<val>;exit" and "exit <val>" were not
> replace with _exit <val>. Doing it now.
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  check | 44 ++++++++++++++++++--------------------------
>  1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/check b/check
> index 9451c350..99d38492 100755
> --- a/check
> +++ b/check
> @@ -47,6 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>  # by default don't output timestamps
>  timestamp=${TIMESTAMP:=false}
>  
> +. common/exit
>  rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>  
>  SRC_GROUPS="generic"
> @@ -121,7 +122,7 @@ examples:
>   check -X .exclude -g auto
>   check -E ~/.xfstests.exclude
>  '
> -	    exit 1
> +	    _exit 1
>  }
>  
>  get_sub_group_list()
> @@ -231,7 +232,7 @@ _prepare_test_list()
>  			list=$(get_group_list $group)
>  			if [ -z "$list" ]; then
>  				echo "Group \"$group\" is empty or not defined?"
> -				exit 1
> +				_exit 1
>  			fi
>  
>  			for t in $list; do

This is now:

	_fatal "Group \"$group\" is empty or not defined?"

> @@ -316,14 +317,14 @@ while [ $# -gt 0 ]; do
>  	-r)
>  		if $exact_order; then
>  			echo "Cannot specify -r and --exact-order."
> -			exit 1
> +			_exit 1
>  		fi
>  		randomize=true
>  		;;
>  	--exact-order)
>  		if $randomize; then
>  			echo "Cannnot specify --exact-order and -r."
> -			exit 1
> +			_exit 1
>  		fi
>  		exact_order=true
>  		;;

Same.

> @@ -361,7 +362,7 @@ done
>  # after processing args, overlay needs FSTYP set before sourcing common/config
>  if ! . ./common/rc; then
>  	echo "check: failed to source common/rc"
> -	exit 1
> +	_exit 1
>  fi
>  
>  init_rc

Same.

> @@ -373,8 +374,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
> +		_exit 1
>  	fi
>  fi
>  
> @@ -385,8 +385,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
> +		_exit 1
>  	fi
>  fi
>  
> @@ -404,8 +403,7 @@ if $have_test_arg; then
>  	while [ $# -gt 0 ]; do
>  		case "$1" in
>  		-*)	echo "Arguments before tests, please!"
> -			status=1
> -			exit $status
> +			_exit 1

_fatal

>  			;;
>  		*)	# Expand test pattern (e.g. xfs/???, *fs/001)
>  			list=$(cd $SRC_DIR; echo $1)
> @@ -438,7 +436,7 @@ fi
>  if [ `id -u` -ne 0 ]
>  then
>      echo "check: QA must be run as root"
> -    exit 1
> +    _exit 1
>  fi

Same

>  
>  _wipe_counters()
> @@ -721,9 +719,9 @@ _prepare_test_list
>  fstests_start_time="$(date +"%F %T")"
>  
>  if $OPTIONS_HAVE_SECTIONS; then
> -	trap "_summary; exit \$status" 0 1 2 3 15
> +	trap "_summary; _exit" 0 1 2 3 15
>  else
> -	trap "_wrapup; exit \$status" 0 1 2 3 15
> +	trap "_wrapup; _exit" 0 1 2 3 15
>  fi

Please add a comment explaining that _exit will capture $status
that has been previously set as the exit value.

Realistically, though, I think 'exit $status' is much better here
because it clearly documents that we are capturing $status as the
exit value from the trap rather than having to add a comment to make
it clear that $status is the exit value of the trap...

>  function run_section()
> @@ -767,8 +765,7 @@ function run_section()
>  	mkdir -p $RESULT_BASE
>  	if [ ! -d $RESULT_BASE ]; then
>  		echo "failed to create results directory $RESULT_BASE"
> -		status=1
> -		exit
> +		_exit 1
>  	fi

_fatal

>  	if $OPTIONS_HAVE_SECTIONS; then
> @@ -784,8 +781,7 @@ function run_section()
>  			echo "our local _test_mkfs routine ..."
>  			cat $tmp.err
>  			echo "check: failed to mkfs \$TEST_DEV using specified options"
> -			status=1
> -			exit
> +			_exit 1
>  		fi
>  		# Previous FSTYP derived from TEST_DEV could be changed, source
>  		# common/rc again with correct FSTYP to get FSTYP specific configs,
> @@ -829,8 +825,7 @@ function run_section()
>  	      echo "our local _scratch_mkfs routine ..."
>  	      cat $tmp.err
>  	      echo "check: failed to mkfs \$SCRATCH_DEV using specified options"
> -	      status=1
> -	      exit
> +	      _exit 1
>  	  fi
>  
>  	  # call the overridden mount - make sure the FS mounts with
> @@ -840,8 +835,7 @@ function run_section()
>  	      echo "our local mount routine ..."
>  	      cat $tmp.err
>  	      echo "check: failed to mount \$SCRATCH_DEV using specified options"
> -	      status=1
> -	      exit
> +	      _exit 1
>  	  else
>  	      _scratch_unmount
>  	  fi

Same for all these.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Nirjhar Roy (IBM)]

On 4/30/25 05:21, Dave Chinner wrote:
> On Tue, Apr 29, 2025 at 06:52:53AM +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@dread.disaster.area/
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> ---
>>   common/config   | 17 +----------------
>>   common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   common/preamble |  1 +
>>   common/punch    |  5 -----
>>   common/rc       | 28 ---------------------------
>>   5 files changed, 52 insertions(+), 49 deletions(-)
>>   create mode 100644 common/exit
> Neither of my replys to v1 made it to the list [1], so I'll have to
> repeat what I said here.

I am really sorry. I didn't get any of your feedback in [v1]. I will 
address them here.

[v1] 
https://lore.kernel.org/all/cover.1745390030.git.nirjhar.roy.lists@gmail.com/

>
> I did point out that I had already sent this out here:
>
> https://lore.kernel.org/fstests/20250417031208.1852171-4-david@fromorbit.com/
>
> but now this version is mostly the same (except for things noted
> below) so I'm good with this.
>
>> 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
> This isn't needed. Include it in check and other high level scripts
> (which need to include this, anyway) before including common/config.
Yeah, right.
>
>> 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
>> +}
> This should be removed and replaced with _fatal
Okay.
>
>> +die_now()
>> +{
>> +	_exit 1
>> +}
> And this should be removed as well.
>
> i.e. These two functions are only used by common/punch, so change
> them to use _fatal and _exit rather than duplicating the wrappers.
Okay.
>
>> diff --git a/common/preamble b/common/preamble
>> index ba029a34..9b6b4b26 100644
>> --- a/common/preamble
>> +++ b/common/preamble
>> @@ -33,6 +33,7 @@ _register_cleanup()
>>   # explicitly as a member of the 'all' group.
>>   _begin_fstest()
>>   {
>> +	. common/exit
>>   	if [ -n "$seq" ]; then
>>   		echo "_begin_fstest can only be called once!"
>>   		_exit 1
> Please leave a blank line between includes and unrelated code.

Sure.

--NR

>
> -Dave.
>
> [1] Thanks Google, for removing mail auth methods without any
> warning and not reporting permanent delivery failure on attempts
> to send mail an unsupported auth method.
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 2/2] check: Replace exit with _exit in check

[Nirjhar Roy (IBM)]

On 4/30/25 05:29, Dave Chinner wrote:
> On Tue, Apr 29, 2025 at 06:52:54AM +0000, Nirjhar Roy (IBM) wrote:
>> Some of the "status=<val>;exit" and "exit <val>" were not
>> replace with _exit <val>. Doing it now.
>>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> ---
>>   check | 44 ++++++++++++++++++--------------------------
>>   1 file changed, 18 insertions(+), 26 deletions(-)
>>
>> diff --git a/check b/check
>> index 9451c350..99d38492 100755
>> --- a/check
>> +++ b/check
>> @@ -47,6 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>>   # by default don't output timestamps
>>   timestamp=${TIMESTAMP:=false}
>>   
>> +. common/exit
>>   rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>>   
>>   SRC_GROUPS="generic"
>> @@ -121,7 +122,7 @@ examples:
>>    check -X .exclude -g auto
>>    check -E ~/.xfstests.exclude
>>   '
>> -	    exit 1
>> +	    _exit 1
>>   }
>>   
>>   get_sub_group_list()
>> @@ -231,7 +232,7 @@ _prepare_test_list()
>>   			list=$(get_group_list $group)
>>   			if [ -z "$list" ]; then
>>   				echo "Group \"$group\" is empty or not defined?"
>> -				exit 1
>> +				_exit 1
>>   			fi
>>   
>>   			for t in $list; do
> This is now:
>
> 	_fatal "Group \"$group\" is empty or not defined?"
Noted.
>
>> @@ -316,14 +317,14 @@ while [ $# -gt 0 ]; do
>>   	-r)
>>   		if $exact_order; then
>>   			echo "Cannot specify -r and --exact-order."
>> -			exit 1
>> +			_exit 1
>>   		fi
>>   		randomize=true
>>   		;;
>>   	--exact-order)
>>   		if $randomize; then
>>   			echo "Cannnot specify --exact-order and -r."
>> -			exit 1
>> +			_exit 1
>>   		fi
>>   		exact_order=true
>>   		;;
> Same.
Noted.
>
>> @@ -361,7 +362,7 @@ done
>>   # after processing args, overlay needs FSTYP set before sourcing common/config
>>   if ! . ./common/rc; then
>>   	echo "check: failed to source common/rc"
>> -	exit 1
>> +	_exit 1
>>   fi
>>   
>>   init_rc
> Same.

Noted.

>
>> @@ -373,8 +374,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
>> +		_exit 1
>>   	fi
>>   fi
>>   
>> @@ -385,8 +385,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
>> +		_exit 1
>>   	fi
>>   fi
>>   
>> @@ -404,8 +403,7 @@ if $have_test_arg; then
>>   	while [ $# -gt 0 ]; do
>>   		case "$1" in
>>   		-*)	echo "Arguments before tests, please!"
>> -			status=1
>> -			exit $status
>> +			_exit 1
> _fatal
Noted.
>
>>   			;;
>>   		*)	# Expand test pattern (e.g. xfs/???, *fs/001)
>>   			list=$(cd $SRC_DIR; echo $1)
>> @@ -438,7 +436,7 @@ fi
>>   if [ `id -u` -ne 0 ]
>>   then
>>       echo "check: QA must be run as root"
>> -    exit 1
>> +    _exit 1
>>   fi
> Same
Noted.
>
>>   
>>   _wipe_counters()
>> @@ -721,9 +719,9 @@ _prepare_test_list
>>   fstests_start_time="$(date +"%F %T")"
>>   
>>   if $OPTIONS_HAVE_SECTIONS; then
>> -	trap "_summary; exit \$status" 0 1 2 3 15
>> +	trap "_summary; _exit" 0 1 2 3 15
>>   else
>> -	trap "_wrapup; exit \$status" 0 1 2 3 15
>> +	trap "_wrapup; _exit" 0 1 2 3 15
>>   fi
> Please add a comment explaining that _exit will capture $status
> that has been previously set as the exit value.
>
> Realistically, though, I think 'exit $status' is much better here
> because it clearly documents that we are capturing $status as the
> exit value from the trap rather than having to add a comment to make
> it clear that $status is the exit value of the trap...

Yeah, I will use "exit \$status" and mention in comments as to why we 
aren't using _exit(), although we can.

>
>>   function run_section()
>> @@ -767,8 +765,7 @@ function run_section()
>>   	mkdir -p $RESULT_BASE
>>   	if [ ! -d $RESULT_BASE ]; then
>>   		echo "failed to create results directory $RESULT_BASE"
>> -		status=1
>> -		exit
>> +		_exit 1
>>   	fi
> _fatal
Noted.
>
>>   	if $OPTIONS_HAVE_SECTIONS; then
>> @@ -784,8 +781,7 @@ function run_section()
>>   			echo "our local _test_mkfs routine ..."
>>   			cat $tmp.err
>>   			echo "check: failed to mkfs \$TEST_DEV using specified options"
>> -			status=1
>> -			exit
>> +			_exit 1
>>   		fi
>>   		# Previous FSTYP derived from TEST_DEV could be changed, source
>>   		# common/rc again with correct FSTYP to get FSTYP specific configs,
>> @@ -829,8 +825,7 @@ function run_section()
>>   	      echo "our local _scratch_mkfs routine ..."
>>   	      cat $tmp.err
>>   	      echo "check: failed to mkfs \$SCRATCH_DEV using specified options"
>> -	      status=1
>> -	      exit
>> +	      _exit 1
>>   	  fi
>>   
>>   	  # call the overridden mount - make sure the FS mounts with
>> @@ -840,8 +835,7 @@ function run_section()
>>   	      echo "our local mount routine ..."
>>   	      cat $tmp.err
>>   	      echo "check: failed to mount \$SCRATCH_DEV using specified options"
>> -	      status=1
>> -	      exit
>> +	      _exit 1
>>   	  else
>>   	      _scratch_unmount
>>   	  fi
> Same for all these.
Noted.
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore