common: Move exit related functions to common/exit

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/[email protected]/

Suggested-by Dave Chinner <[email protected]>

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

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

common/config +1 -16

index eada3971..6a60d144 100644
@@ -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"
 

common/exit new file +50 -0

new file mode 100644
index 00000000..ad7e7498
@@ -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
+}
+

common/preamble +1 -0

index ba029a34..9b6b4b26 100644
@@ -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

common/punch +0 -5

index 64d665d8..4e8ebcd7 100644
@@ -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

common/rc +0 -28

index 9bed6dad..fac9b6da 100644
@@ -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()
  {

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/[email protected]/

Suggested-by Dave Chinner <[email protected]>

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

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

common/exit new file +12 -0

new file mode 100644
index 00000000..ad7e7498
@@ -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()

Quoted 8 lines · 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/[email protected]/

Suggested-by Dave Chinner <[email protected]>

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

Quoted 8 lines

---
  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/[email protected]/

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

common/config line 42

Quoted 11 lines

common/config +1 -1

index eada3971..6a60d144 100644
@@ -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.

Quoted 22 lines

common/exit new file +15 -0

new file mode 100644
index 00000000..ad7e7498
@@ -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

common/exit line 22

Quoted 7 lines

common/exit new file +7 -0

+}
+
+_die()
+{
+        echo $@
+        _exit 1
+}

This should be removed and replaced with _fatal

common/exit line 27

Quoted 5 lines

common/exit new file +4 -0

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

common/preamble _register_cleanup() · line 39

Quoted 13 lines

common/preamble +1 -0

index ba029a34..9b6b4b26 100644
@@ -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.

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

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

---
 check | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

check +2 -1

index 9451c350..99d38492 100755
@@ -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()

check +1 -1

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

check +2 -4

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

check +2 -4

 	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

Quoted 37 lines · 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) <[email protected]>

---
  check | 44 ++++++++++++++++++--------------------------
  1 file changed, 18 insertions(+), 26 deletions(-)

check +3 -2

index 9451c350..99d38492 100755
@@ -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
 

check _prepare_test_list() · line 238

Quoted 1 line

check +0 -0

  			for t in $list; do

This is now:

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

Quoted 11 lines

check +1 -1

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

check while [ $# -gt 0 ]; do · line 330

Quoted 7 lines

check +1 -1

  		if $randomize; then
  			echo "Cannnot specify --exact-order and -r."
-			exit 1
+			_exit 1
  		fi
  		exact_order=true
  		;;

Same.

check done · line 368

Quoted 10 lines

check +1 -1

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

Quoted 21 lines

check +2 -4

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

check if $have_test_arg; then · line 406

Quoted 7 lines

check +1 -2

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

check fi · line 440

Quoted 11 lines

check +1 -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

Same

check _prepare_test_list · line 725

Quoted 13 lines

check +2 -2

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

check function run_section() · line 769

Quoted 10 lines

check +1 -2

  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

Quoted 21 lines

check +2 -4

  	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
 

check function run_section() · line 841

Quoted 11 lines

check +1 -2

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