[PATCH v3 0/6] Minor cleanups in common/

[PATCH v3 0/6] Minor cleanups in common/

[Nirjhar Roy (IBM)]

This patch series removes some unnecessary sourcing of common/rc
and decouples the call to init_rc() from the sourcing of common/rc.
This is proposed in [1] and [2]. It also removes direct usage of exit command
with a _exit wrapper. The individual patches have the details.

[v2] --> [v3]
 1. Added R.Bs from Dave[3] and Ritesh in all the patches of [v2]
 2. Replaced status=1;exit with "_exit 1"  in some of the functions I missed in [v2]
 3. Added some comments in the check script(suggested by Ritesh)
 4. Added a new patch (patch 2) that removes redundant sourcing of common/config in generic/367
 5. As of now, I didn't change the definition of _exit() function.

[1] https://lore.kernel.org/all/20250206155251.GA21787@frogsfrogsfrogs/
[2] https://lore.kernel.org/all/20250210142322.tptpphdntglsz4eq@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/
[3] https://lore.kernel.org/all/Z-xcne3f5Klvuxcq@dread.disaster.area/

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

Nirjhar Roy (IBM) (6):
  generic/749: Remove redundant sourcing of common/rc
  generic/367: Remove redundant sourcing if common/config
  check: Remove redundant _test_mount in check
  check,common{rc,preamble}: Decouple init_rc() call from sourcing
    common/rc
  common/config: Introduce _exit wrapper around exit command
  common: exit --> _exit

 check             |  10 ++---
 common/btrfs      |   6 +--
 common/ceph       |   2 +-
 common/config     |  15 +++++--
 common/dump       |  11 +++--
 common/ext4       |   2 +-
 common/populate   |   2 +-
 common/preamble   |   3 +-
 common/punch      |  13 +++---
 common/rc         | 107 ++++++++++++++++++++++------------------------
 common/repair     |   4 +-
 common/xfs        |   8 ++--
 tests/generic/367 |   1 -
 tests/generic/749 |   1 -
 14 files changed, 91 insertions(+), 94 deletions(-)

--
2.34.1

[PATCH v3 1/6] generic/749: Remove redundant sourcing of common/rc

[Nirjhar Roy (IBM)]

common/rc is already sourced before the test starts running
in _begin_fstest() preamble.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 tests/generic/749 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/generic/749 b/tests/generic/749
index fc747738..451f283e 100755
--- a/tests/generic/749
+++ b/tests/generic/749
@@ -15,7 +15,6 @@
 # boundary and ensures we get a SIGBUS if we write to data beyond the system
 # page size even if the block size is greater than the system page size.
 . ./common/preamble
-. ./common/rc
 _begin_fstest auto quick prealloc
 
 # Import common functions.
-- 
2.34.1

[PATCH v3 2/6] generic/367: Remove redundant sourcing if common/config

[Nirjhar Roy (IBM)]

common/config will be source by _begin_fstest

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 tests/generic/367 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/generic/367 b/tests/generic/367
index ed371a02..567db557 100755
--- a/tests/generic/367
+++ b/tests/generic/367
@@ -11,7 +11,6 @@
 # check if the extsize value and the xflag bit actually got reflected after
 # setting/re-setting the extsize value.
 
-. ./common/config
 . ./common/filter
 . ./common/preamble
 
-- 
2.34.1

[PATCH v3 3/6] check: Remove redundant _test_mount in check

[Nirjhar Roy (IBM)]

init_rc already does a _test_mount. Hence removing the additional
_test_mount call when OLD_TEST_FS_MOUNT_OPTS != TEST_FS_MOUNT_OPTS.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 check | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/check b/check
index 32890470..3d621210 100755
--- a/check
+++ b/check
@@ -791,13 +791,9 @@ function run_section()
 		. common/rc
 		_prepare_test_list
 	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
+		# Unmount TEST_DEV to apply the updated mount options.
+		# It will be mounted again by init_rc(), called shortly after.
 		_test_unmount 2> /dev/null
-		if ! _test_mount
-		then
-			echo "check: failed to mount $TEST_DEV on $TEST_DIR"
-			status=1
-			exit
-		fi
 	fi
 
 	init_rc
-- 
2.34.1

[PATCH v3 4/6] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

Silently executing scripts during sourcing common/rc isn't good practice
and also causes unnecessary script execution. Decouple init_rc() call
and call init_rc() explicitly where required.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 check           | 2 ++
 common/preamble | 1 +
 common/rc       | 2 --
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/check b/check
index 3d621210..9451c350 100755
--- a/check
+++ b/check
@@ -364,6 +364,8 @@ if ! . ./common/rc; then
 	exit 1
 fi
 
+init_rc
+
 # If the test config specified a soak test duration, see if there are any
 # unit suffixes that need converting to an integer seconds count.
 if [ -n "$SOAK_DURATION" ]; then
diff --git a/common/preamble b/common/preamble
index 0c9ee2e0..c92e55bb 100644
--- a/common/preamble
+++ b/common/preamble
@@ -50,6 +50,7 @@ _begin_fstest()
 	_register_cleanup _cleanup
 
 	. ./common/rc
+	init_rc
 
 	# remove previous $seqres.full before test
 	rm -f $seqres.full $seqres.hints
diff --git a/common/rc b/common/rc
index 16d627e1..038c22f6 100644
--- a/common/rc
+++ b/common/rc
@@ -5817,8 +5817,6 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
-
 ################################################################################
 # make sure this script returns success
 /bin/true
-- 
2.34.1

[PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Nirjhar Roy (IBM)]

We should always set the value of status correctly when we are exiting.
Else, "$?" might not give us the correct value.
If we see the following trap
handler registration in the check script:

if $OPTIONS_HAVE_SECTIONS; then
     trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
else
     trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
fi

So, "exit 1" will exit the check script without setting the correct
return value. I ran with the following local.config file:

[xfs_4k_valid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/test
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch

[xfs_4k_invalid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/invalid_dir
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch

This caused the init_rc() to catch the case of invalid _test_mount
options. Although the check script correctly failed during the execution
of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
returned 0. This is because init_rc exits with "exit 1" without
correctly setting the value of "status". IMO, the correct behavior
should have been that "$?" should have been non-zero.

The next patch will replace exit with _exit.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 common/config | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/config b/common/config
index 79bec87f..eb6af35a 100644
--- a/common/config
+++ b/common/config
@@ -96,6 +96,14 @@ 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.
+_exit()
+{
+	status="$1"
+	exit "$status"
+}
+
 # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
 set_mkfs_prog_path_with_opts()
 {
-- 
2.34.1

[PATCH v3 6/6] common: exit --> _exit

[Nirjhar Roy (IBM)]

Replace exit <return-val> with _exit <return-val> which
is introduced in the previous patch.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 common/btrfs    |   6 +--
 common/ceph     |   2 +-
 common/config   |   7 ++--
 common/dump     |  11 +++--
 common/ext4     |   2 +-
 common/populate |   2 +-
 common/preamble |   2 +-
 common/punch    |  13 +++---
 common/rc       | 105 +++++++++++++++++++++++-------------------------
 common/repair   |   4 +-
 common/xfs      |   8 ++--
 11 files changed, 78 insertions(+), 84 deletions(-)

diff --git a/common/btrfs b/common/btrfs
index a3b9c12f..3725632c 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -80,7 +80,7 @@ _require_btrfs_mkfs_feature()
 {
 	if [ -z $1 ]; then
 		echo "Missing feature name argument for _require_btrfs_mkfs_feature"
-		exit 1
+		_exit 1
 	fi
 	feat=$1
 	$MKFS_BTRFS_PROG -O list-all 2>&1 | \
@@ -104,7 +104,7 @@ _require_btrfs_fs_feature()
 {
 	if [ -z $1 ]; then
 		echo "Missing feature name argument for _require_btrfs_fs_feature"
-		exit 1
+		_exit 1
 	fi
 	feat=$1
 	modprobe btrfs > /dev/null 2>&1
@@ -214,7 +214,7 @@ _check_btrfs_filesystem()
 	if [ $ok -eq 0 ]; then
 		status=1
 		if [ "$iam" != "check" ]; then
-			exit 1
+			_exit 1
 		fi
 		return 1
 	fi
diff --git a/common/ceph b/common/ceph
index d6f24df1..df7a6814 100644
--- a/common/ceph
+++ b/common/ceph
@@ -14,7 +14,7 @@ _ceph_create_file_layout()
 
 	if [ -e $fname ]; then
 		echo "File $fname already exists."
-		exit 1
+		_exit 1
 	fi
 	touch $fname
 	$SETFATTR_PROG -n ceph.file.layout \
diff --git a/common/config b/common/config
index eb6af35a..4c5435b7 100644
--- a/common/config
+++ b/common/config
@@ -123,8 +123,7 @@ set_mkfs_prog_path_with_opts()
 _fatal()
 {
     echo "$*"
-    status=1
-    exit 1
+    _exit 1
 }
 
 export MKFS_PROG="$(type -P mkfs)"
@@ -868,7 +867,7 @@ get_next_config() {
 		echo "Warning: need to define parameters for host $HOST"
 		echo "       or set variables:"
 		echo "       $MC"
-		exit 1
+		_exit 1
 	fi
 
 	_check_device TEST_DEV required $TEST_DEV
@@ -879,7 +878,7 @@ get_next_config() {
 	if [ ! -z "$SCRATCH_DEV_POOL" ]; then
 		if [ ! -z "$SCRATCH_DEV" ]; then
 			echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) should be unset when \$SCRATCH_DEV_POOL ($SCRATCH_DEV_POOL) is set"
-			exit 1
+			_exit 1
 		fi
 		SCRATCH_DEV=`echo $SCRATCH_DEV_POOL | awk '{print $1}'`
 		export SCRATCH_DEV
diff --git a/common/dump b/common/dump
index 6dcd6250..09859006 100644
--- a/common/dump
+++ b/common/dump
@@ -280,8 +280,7 @@ _create_dumpdir_stress_num()
     rm -rf $dump_dir
     if ! mkdir $dump_dir; then
         echo "    failed to mkdir $dump_dir"
-        status=1
-        exit
+        _exit 1
     fi
 
     # Remove fsstress commands that aren't supported on all xfs configs so that
@@ -480,7 +479,7 @@ _do_create_dumpdir_fill()
 		else
 		    $verbose && echo
 		    echo "Error: cannot mkdir \"$dir\""
-		    exit 1
+		    _exit 1
 		fi
 	    fi
 	else
@@ -496,7 +495,7 @@ _do_create_dumpdir_fill()
 		    else
 			$verbose && echo
 			echo "Error: cannot mkdir \"$dir\""
-			exit 1
+			_exit 1
 		    fi
 		fi
 	    fi
@@ -507,7 +506,7 @@ _do_create_dumpdir_fill()
 	    else
 		$verbose && echo
 		echo "Error: cannot create \"$file\""
-		exit 1
+		_exit 1
 	    fi
 	fi
 	if [ -n "$owner" -a -n "$group" ]; then
@@ -649,7 +648,7 @@ _do_create_dump_symlinks()
 		else
 		    $verbose && echo
 		    echo "Error: cannot mkdir \"$dir\""
-		    exit 1
+		    _exit 1
 		fi
 	    fi
 	fi
diff --git a/common/ext4 b/common/ext4
index e1b336d3..f88fa532 100644
--- a/common/ext4
+++ b/common/ext4
@@ -182,7 +182,7 @@ _require_scratch_ext4_feature()
 {
     if [ -z "$1" ]; then
         echo "Usage: _require_scratch_ext4_feature feature"
-        exit 1
+        _exit 1
     fi
     $MKFS_EXT4_PROG -F $MKFS_OPTIONS -O "$1" \
 		    $SCRATCH_DEV 512m >/dev/null 2>&1 \
diff --git a/common/populate b/common/populate
index 7352f598..50dc75d3 100644
--- a/common/populate
+++ b/common/populate
@@ -1003,7 +1003,7 @@ _fill_fs()
 
 	if [ $# -ne 4 ]; then
 		echo "Usage: _fill_fs filesize dir blocksize switch_user"
-		exit 1
+		_exit 1
 	fi
 
 	if [ $switch_user -eq 0 ]; then
diff --git a/common/preamble b/common/preamble
index c92e55bb..ba029a34 100644
--- a/common/preamble
+++ b/common/preamble
@@ -35,7 +35,7 @@ _begin_fstest()
 {
 	if [ -n "$seq" ]; then
 		echo "_begin_fstest can only be called once!"
-		exit 1
+		_exit 1
 	fi
 
 	seq=`basename $0`
diff --git a/common/punch b/common/punch
index 43ccab69..64d665d8 100644
--- a/common/punch
+++ b/common/punch
@@ -172,16 +172,16 @@ _filter_fiemap_flags()
 	$AWK_PROG -e "$awk_script" | _coalesce_extents
 }
 
-# Filters fiemap output to only print the 
+# Filters fiemap output to only print the
 # file offset column and whether or not
 # it is an extent or a hole
 _filter_hole_fiemap()
 {
 	$AWK_PROG '
 		$3 ~ /hole/ {
-			print $1, $2, $3; 
+			print $1, $2, $3;
 			next;
-		}   
+		}
 		$5 ~ /0x[[:xdigit:]]+/ {
 			print $1, $2, "extent";
 		}' |
@@ -224,8 +224,7 @@ _filter_bmap()
 
 die_now()
 {
-	status=1
-	exit
+	_exit 1
 }
 
 # test the different corner cases for zeroing a range:
@@ -276,7 +275,7 @@ _test_generic_punch()
 		u)	unwritten_tests=
 		;;
 		?)	echo Invalid flag
-		exit 1
+		_exit 1
 		;;
 		esac
 	done
@@ -552,7 +551,7 @@ _test_block_boundaries()
 		d)	sync_cmd=
 		;;
 		?)	echo Invalid flag
-		exit 1
+		_exit 1
 		;;
 		esac
 	done
diff --git a/common/rc b/common/rc
index 038c22f6..3b21eb27 100644
--- a/common/rc
+++ b/common/rc
@@ -909,8 +909,7 @@ _mkfs_dev()
 	# output stored mkfs output
 	cat $tmp.mkfserr >&2
 	cat $tmp.mkfsstd
-	status=1
-	exit 1
+	_exit 1
     fi
     rm -f $tmp.mkfserr $tmp.mkfsstd
 }
@@ -1575,7 +1574,7 @@ _get_pids_by_name()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _get_pids_by_name process-name" 1>&2
-	exit 1
+	_exit 1
     fi
 
     # Algorithm ... all ps(1) variants have a time of the form MM:SS or
@@ -1609,7 +1608,7 @@ _df_device()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _df_device device" 1>&2
-	exit 1
+	_exit 1
     fi
 
     # Note that we use "==" here so awk doesn't try to interpret an NFS over
@@ -1641,7 +1640,7 @@ _df_dir()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _df_dir device" 1>&2
-	exit 1
+	_exit 1
     fi
 
     $DF_PROG $1 2>/dev/null | $AWK_PROG -v what=$1 '
@@ -1667,7 +1666,7 @@ _used()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _used device" 1>&2
-	exit 1
+	_exit 1
     fi
 
     _df_device $1 | $AWK_PROG '{ sub("%", "") ; print $6 }'
@@ -1680,7 +1679,7 @@ _fs_type()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _fs_type device" 1>&2
-	exit 1
+	_exit 1
     fi
 
     #
@@ -1705,7 +1704,7 @@ _fs_options()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _fs_options device" 1>&2
-	exit 1
+	_exit 1
     fi
 
     $AWK_PROG -v dev=$1 '
@@ -1720,7 +1719,7 @@ _is_block_dev()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _is_block_dev dev" 1>&2
-	exit 1
+	_exit 1
     fi
 
     local dev=$1
@@ -1739,7 +1738,7 @@ _is_char_dev()
 {
 	if [ $# -ne 1 ]; then
 		echo "Usage: _is_char_dev dev" 1>&2
-		exit 1
+		_exit 1
 	fi
 
 	local dev=$1
@@ -1772,7 +1771,7 @@ _do()
 	echo -n "$note... "
     else
 	echo "Usage: _do [note] cmd" 1>&2
-	status=1; exit
+	_exit 1
     fi
 
     (eval "echo '---' \"$cmd\"") >>$seqres.full
@@ -1793,7 +1792,7 @@ _do()
     then
 	[ $# -ne 2 ] && echo
 	eval "echo \"$cmd\" failed \(returned $ret\): see $seqres.full"
-	status=1; exit
+	_exit 1
     fi
 
     return $ret
@@ -1809,8 +1808,7 @@ _notrun()
     rm -f ${RESULT_DIR}/require_test*
     rm -f ${RESULT_DIR}/require_scratch*
 
-    status=0
-    exit
+    _exit 0
 }
 
 # just plain bail out
@@ -1819,8 +1817,7 @@ _fail()
 {
     echo "$*" | tee -a $seqres.full
     echo "(see $seqres.full for details)"
-    status=1
-    exit 1
+    _exit 1
 }
 
 #
@@ -2049,14 +2046,14 @@ _require_scratch_nocheck()
 
     _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
     local err=$?
-    [ $err -le 1 ] || exit 1
+    [ $err -le 1 ] || _exit 1
     if [ $err -eq 0 ]
     then
         # if it's mounted, unmount it
         if ! _scratch_unmount
         then
             echo "failed to unmount $SCRATCH_DEV"
-            exit 1
+            _exit 1
         fi
     fi
     rm -f ${RESULT_DIR}/require_scratch "$RESULT_DIR/.skip_orebuild" "$RESULT_DIR/.skip_rebuild"
@@ -2273,13 +2270,13 @@ _require_test()
 
     _check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR
     local err=$?
-    [ $err -le 1 ] || exit 1
+    [ $err -le 1 ] || _exit 1
     if [ $err -ne 0 ]
     then
 	if ! _test_mount
 	then
 		echo "!!! failed to mount $TEST_DEV on $TEST_DIR"
-		exit 1
+		_exit 1
 	fi
     fi
     touch ${RESULT_DIR}/require_test
@@ -2391,7 +2388,7 @@ _require_block_device()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _require_block_device <dev>" 1>&2
-		exit 1
+		_exit 1
 	fi
 	if [ "`_is_block_dev "$1"`" == "" ]; then
 		_notrun "require $1 to be valid block disk"
@@ -2404,7 +2401,7 @@ _require_local_device()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _require_local_device <dev>" 1>&2
-		exit 1
+		_exit 1
 	fi
 	if [ "`_is_block_dev "$1"`" != "" ]; then
 		return 0
@@ -2512,7 +2509,7 @@ _zone_type()
 	local target=$1
 	if [ -z $target ]; then
 		echo "Usage: _zone_type <device>"
-		exit 1
+		_exit 1
 	fi
 	local sdev=`_short_dev $target`
 
@@ -2528,7 +2525,7 @@ _require_zoned_device()
 	local target=$1
 	if [ -z $target ]; then
 		echo "Usage: _require_zoned_device <device>"
-		exit 1
+		_exit 1
 	fi
 
 	local type=`_zone_type ${target}`
@@ -2668,7 +2665,7 @@ _run_aiodio()
     if [ -z "$1" ]
     then
         echo "usage: _run_aiodio command_name" 2>&1
-        status=1; exit 1
+        _exit 1
     fi
 
     _require_aiodio $1
@@ -2880,7 +2877,7 @@ _require_xfs_io_command()
 	if [ -z "$1" ]
 	then
 		echo "Usage: _require_xfs_io_command command [switch]" 1>&2
-		exit 1
+		_exit 1
 	fi
 	local command=$1
 	shift
@@ -3364,7 +3361,7 @@ _is_dev_mounted()
 
 	if [ $# -lt 1 ]; then
 		echo "Usage: _is_dev_mounted <device> [fstype]" 1>&2
-		exit 1
+		_exit 1
 	fi
 
 	findmnt -rncv -S $dev -t $fstype -o TARGET | head -1
@@ -3378,7 +3375,7 @@ _is_dir_mountpoint()
 
 	if [ $# -lt 1 ]; then
 		echo "Uasge: _is_dir_mountpoint <dir> [fstype]" 1>&2
-		exit 1
+		_exit 1
 	fi
 
 	findmnt -rncv -t $fstype -o TARGET $dir | head -1
@@ -3391,7 +3388,7 @@ _remount()
     if [ $# -ne 2 ]
     then
 	echo "Usage: _remount device ro/rw" 1>&2
-	exit 1
+	_exit 1
     fi
     local device=$1
     local mode=$2
@@ -3399,7 +3396,7 @@ _remount()
     if ! mount -o remount,$mode $device
     then
         echo "_remount: failed to remount filesystem on $device as $mode"
-        exit 1
+        _exit 1
     fi
 }
 
@@ -3417,7 +3414,7 @@ _umount_or_remount_ro()
     if [ $# -ne 1 ]
     then
 	echo "Usage: _umount_or_remount_ro <device>" 1>&2
-	exit 1
+	_exit 1
     fi
 
     local device=$1
@@ -3435,7 +3432,7 @@ _mount_or_remount_rw()
 {
 	if [ $# -ne 3 ]; then
 		echo "Usage: _mount_or_remount_rw <opts> <dev> <mnt>" 1>&2
-		exit 1
+		_exit 1
 	fi
 	local mount_opts=$1
 	local device=$2
@@ -3516,7 +3513,7 @@ _check_generic_filesystem()
     if [ $ok -eq 0 ]; then
 	status=1
 	if [ "$iam" != "check" ]; then
-		exit 1
+		_exit 1
 	fi
 	return 1
     fi
@@ -3582,7 +3579,7 @@ _check_udf_filesystem()
     if [ $# -ne 1 -a $# -ne 2 ]
     then
 	echo "Usage: _check_udf_filesystem device [last_block]" 1>&2
-	exit 1
+	_exit 1
     fi
 
     if [ ! -x $here/src/udf_test ]
@@ -3776,7 +3773,7 @@ _get_os_name()
 		echo 'linux'
 	else
 		echo Unknown operating system: `uname`
-		exit
+		_exit 1
 	fi
 }
 
@@ -3837,7 +3834,7 @@ _link_out_file()
 _die()
 {
         echo $@
-        exit 1
+        _exit 1
 }
 
 # convert urandom incompressible data to compressible text data
@@ -3994,7 +3991,7 @@ _require_scratch_dev_pool()
 		if _mount | grep -q $i; then
 			if ! _unmount $i; then
 		            echo "failed to unmount $i - aborting"
-		            exit 1
+		            _exit 1
 		        fi
 		fi
 		# To help better debug when something fails, we remove
@@ -4403,7 +4400,7 @@ _require_batched_discard()
 {
 	if [ $# -ne 1 ]; then
 		echo "Usage: _require_batched_discard mnt_point" 1>&2
-		exit 1
+		_exit 1
 	fi
 	_require_fstrim
 
@@ -4630,7 +4627,7 @@ _require_chattr()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _require_chattr <attr>"
-		exit 1
+		_exit 1
 	fi
 	local attribute=$1
 
@@ -4649,7 +4646,7 @@ _get_total_inode()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_total_inode <mnt>"
-		exit 1
+		_exit 1
 	fi
 	local nr_inode;
 	nr_inode=`$DF_PROG -i $1 | tail -1 | awk '{print $3}'`
@@ -4660,7 +4657,7 @@ _get_used_inode()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_used_inode <mnt>"
-		exit 1
+		_exit 1
 	fi
 	local nr_inode;
 	nr_inode=`$DF_PROG -i $1 | tail -1 | awk '{print $4}'`
@@ -4671,7 +4668,7 @@ _get_used_inode_percent()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_used_inode_percent <mnt>"
-		exit 1
+		_exit 1
 	fi
 	local pct_inode;
 	pct_inode=`$DF_PROG -i $1 | tail -1 | awk '{ print $6 }' | \
@@ -4683,7 +4680,7 @@ _get_free_inode()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_free_inode <mnt>"
-		exit 1
+		_exit 1
 	fi
 	local nr_inode;
 	nr_inode=`$DF_PROG -i $1 | tail -1 | awk '{print $5}'`
@@ -4696,7 +4693,7 @@ _get_available_space()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_available_space <mnt>"
-		exit 1
+		_exit 1
 	fi
 	$DF_PROG -B 1 $1 | tail -n1 | awk '{ print $5 }'
 }
@@ -4707,7 +4704,7 @@ _get_total_space()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _get_total_space <mnt>"
-		exit 1
+		_exit 1
 	fi
 	$DF_PROG -B 1 $1 | tail -n1 | awk '{ print $3 }'
 }
@@ -4952,7 +4949,7 @@ init_rc()
 	if [ "$TEST_DEV" = ""  ]
 	then
 		echo "common/rc: Error: \$TEST_DEV is not set"
-		exit 1
+		_exit 1
 	fi
 
 	# if $TEST_DEV is not mounted, mount it now as XFS
@@ -4966,20 +4963,20 @@ init_rc()
 			if ! _test_mount
 			then
 				echo "common/rc: could not mount $TEST_DEV on $TEST_DIR"
-				exit 1
+				_exit 1
 			fi
 		fi
 	fi
 
 	# Sanity check that TEST partition is not mounted at another mount point
 	# or as another fs type
-	_check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR $FSTYP || exit 1
+	_check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR $FSTYP || _exit 1
 	if [ -n "$SCRATCH_DEV" ]; then
 		# Sanity check that SCRATCH partition is not mounted at another
 		# mount point, because it is about to be unmounted and formatted.
 		# Another fs type for scratch is fine (bye bye old fs type).
 		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
-		[ $? -le 1 ] || exit 1
+		[ $? -le 1 ] || _exit 1
 	fi
 
 	# Figure out if we need to add -F ("foreign", deprecated) option to xfs_io
@@ -5029,7 +5026,7 @@ _get_file_block_size()
 {
 	if [ -z $1 ] || [ ! -d $1 ]; then
 		echo "Missing mount point argument for _get_file_block_size"
-		exit 1
+		_exit 1
 	fi
 
 	case "$FSTYP" in
@@ -5076,7 +5073,7 @@ _get_block_size()
 {
 	if [ -z $1 ] || [ ! -d $1 ]; then
 		echo "Missing mount point argument for _get_block_size"
-		exit 1
+		_exit 1
 	fi
 	stat -f -c %S $1
 }
@@ -5146,14 +5143,14 @@ _run_hugepage_fsx() {
 	fi
 	cat $tmp.hugepage_fsx
 	rm -f $tmp.hugepage_fsx
-	test $res -ne 0 && exit 1
+	test $res -ne 0 && _exit 1
 	return 0
 }
 
 # run fsx or exit the test
 run_fsx()
 {
-	_run_fsx "$@" || exit 1
+	_run_fsx "$@" || _exit 1
 }
 
 _require_statx()
@@ -5318,7 +5315,7 @@ _get_max_file_size()
 {
 	if [ -z $1 ] || [ ! -d $1 ]; then
 		echo "Missing mount point argument for _get_max_file_size"
-		exit 1
+		_exit 1
 	fi
 
 	local mnt=$1
diff --git a/common/repair b/common/repair
index a79f9b2b..fd206f8e 100644
--- a/common/repair
+++ b/common/repair
@@ -16,7 +16,7 @@ _zero_position()
 		}'`
 	if [ -z "$offset" -o -z "$length" ]; then
 		echo "cannot calculate offset ($offset) or length ($length)"
-		exit
+		_exit 1
 	fi
 	length=`expr $length / 512`
 	$here/src/devzero -v $value -b 1 -n $length -o $offset $SCRATCH_DEV \
@@ -113,7 +113,7 @@ _filter_dd()
 }
 
 # do some controlled corrupting & ensure repair recovers us
-# 
+#
 _check_repair()
 {
 	value=$1
diff --git a/common/xfs b/common/xfs
index 81d568d3..96c15f3c 100644
--- a/common/xfs
+++ b/common/xfs
@@ -553,7 +553,7 @@ _require_xfs_db_command()
 {
 	if [ $# -ne 1 ]; then
 		echo "Usage: _require_xfs_db_command command" 1>&2
-		exit 1
+		_exit 1
 	fi
 	command=$1
 
@@ -789,7 +789,7 @@ _check_xfs_filesystem()
 
 	if [ $# -ne 3 ]; then
 		echo "Usage: _check_xfs_filesystem device <logdev>|none <rtdev>|none" 1>&2
-		exit 1
+		_exit 1
 	fi
 
 	extra_mount_options=""
@@ -1014,7 +1014,7 @@ _check_xfs_filesystem()
 	if [ $ok -eq 0 ]; then
 		status=1
 		if [ "$iam" != "check" ]; then
-			exit 1
+			_exit 1
 		fi
 		return 1
 	fi
@@ -1379,7 +1379,7 @@ _require_xfs_spaceman_command()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _require_xfs_spaceman_command command [switch]" 1>&2
-		exit 1
+		_exit 1
 	fi
 	local command=$1
 	shift
-- 
2.34.1

Re: [PATCH v3 2/6] generic/367: Remove redundant sourcing if common/config

[Ritesh Harjani]

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

> common/config will be source by _begin_fstest
>

Looks good to me. Please feel free to add:

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  tests/generic/367 | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tests/generic/367 b/tests/generic/367
> index ed371a02..567db557 100755
> --- a/tests/generic/367
> +++ b/tests/generic/367
> @@ -11,7 +11,6 @@
>  # check if the extsize value and the xflag bit actually got reflected after
>  # setting/re-setting the extsize value.
>  
> -. ./common/config
>  . ./common/filter
>  . ./common/preamble
>  
> -- 
> 2.34.1

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Ritesh Harjani]

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

> We should always set the value of status correctly when we are exiting.
> Else, "$?" might not give us the correct value.
> If we see the following trap
> handler registration in the check script:
>
> if $OPTIONS_HAVE_SECTIONS; then
>      trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
> else
>      trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
> fi
>
> So, "exit 1" will exit the check script without setting the correct
> return value. I ran with the following local.config file:
>
> [xfs_4k_valid]
> FSTYP=xfs
> TEST_DEV=/dev/loop0
> TEST_DIR=/mnt1/test
> SCRATCH_DEV=/dev/loop1
> SCRATCH_MNT=/mnt1/scratch
>
> [xfs_4k_invalid]
> FSTYP=xfs
> TEST_DEV=/dev/loop0
> TEST_DIR=/mnt1/invalid_dir
> SCRATCH_DEV=/dev/loop1
> SCRATCH_MNT=/mnt1/scratch
>
> This caused the init_rc() to catch the case of invalid _test_mount
> options. Although the check script correctly failed during the execution
> of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
> returned 0. This is because init_rc exits with "exit 1" without
> correctly setting the value of "status". IMO, the correct behavior
> should have been that "$?" should have been non-zero.
>
> The next patch will replace exit with _exit.
>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
>  common/config | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/common/config b/common/config
> index 79bec87f..eb6af35a 100644
> --- a/common/config
> +++ b/common/config
> @@ -96,6 +96,14 @@ 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.

...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()
> +{
> +	status="$1"
> +	exit "$status"
> +}
> +

I agree with Darrick’s suggestion here. It’s safer to update status only
when an argument is passed - otherwise, it’s easy to trip over this.

Let’s also avoid defaulting status to 0 inside _exit(). That way, if the
caller forgets to pass an argument but has explicitly set status
earlier, we preserve the intended value.

We should update _exit() with... 

test -n "$1" && status="$1"

-ritesh


>  # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
>  set_mkfs_prog_path_with_opts()
>  {
> -- 
> 2.34.1

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Dave Chinner]

On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote:
> We should always set the value of status correctly when we are exiting.
> Else, "$?" might not give us the correct value.
> If we see the following trap
> handler registration in the check script:
> 
> if $OPTIONS_HAVE_SECTIONS; then
>      trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
> else
>      trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
> fi
> 
> So, "exit 1" will exit the check script without setting the correct
> return value. I ran with the following local.config file:
> 
> [xfs_4k_valid]
> FSTYP=xfs
> TEST_DEV=/dev/loop0
> TEST_DIR=/mnt1/test
> SCRATCH_DEV=/dev/loop1
> SCRATCH_MNT=/mnt1/scratch
> 
> [xfs_4k_invalid]
> FSTYP=xfs
> TEST_DEV=/dev/loop0
> TEST_DIR=/mnt1/invalid_dir
> SCRATCH_DEV=/dev/loop1
> SCRATCH_MNT=/mnt1/scratch
> 
> This caused the init_rc() to catch the case of invalid _test_mount
> options. Although the check script correctly failed during the execution
> of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
> returned 0. This is because init_rc exits with "exit 1" without
> correctly setting the value of "status". IMO, the correct behavior
> should have been that "$?" should have been non-zero.
> 
> The next patch will replace exit with _exit.
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
>  common/config | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/common/config b/common/config
> index 79bec87f..eb6af35a 100644
> --- a/common/config
> +++ b/common/config
> @@ -96,6 +96,14 @@ 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.
> +_exit()
> +{
> +	status="$1"
> +	exit "$status"
> +}

The only issue with putting this helper in common/config is that
calling _exit() requires sourcing common/config from the shell
context that calls it.

This means every test must source common/config and re-execute the
environment setup, even though we already have all the environment
set up because it was exported from check when it sourced
common/config.

We have the same problem with _fatal() - it is defined in
common/config instead of an independent common file. If we put such
functions in their own common file, they can be sourced
without dependencies on any other common file being included first.

e.g. we create common/exit and place all the functions that
terminate tests in it - _fatal, _notrun, _exit, etc and source that
file once per shell context before we source common/config,
common/rc, etc. This means we can source and call those termination
functions from any context without having to worry about
dependencies...

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Nirjhar Roy (IBM)]

On 4/8/25 14:43, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> We should always set the value of status correctly when we are exiting.
>> Else, "$?" might not give us the correct value.
>> If we see the following trap
>> handler registration in the check script:
>>
>> if $OPTIONS_HAVE_SECTIONS; then
>>       trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
>> else
>>       trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
>> fi
>>
>> So, "exit 1" will exit the check script without setting the correct
>> return value. I ran with the following local.config file:
>>
>> [xfs_4k_valid]
>> FSTYP=xfs
>> TEST_DEV=/dev/loop0
>> TEST_DIR=/mnt1/test
>> SCRATCH_DEV=/dev/loop1
>> SCRATCH_MNT=/mnt1/scratch
>>
>> [xfs_4k_invalid]
>> FSTYP=xfs
>> TEST_DEV=/dev/loop0
>> TEST_DIR=/mnt1/invalid_dir
>> SCRATCH_DEV=/dev/loop1
>> SCRATCH_MNT=/mnt1/scratch
>>
>> This caused the init_rc() to catch the case of invalid _test_mount
>> options. Although the check script correctly failed during the execution
>> of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
>> returned 0. This is because init_rc exits with "exit 1" without
>> correctly setting the value of "status". IMO, the correct behavior
>> should have been that "$?" should have been non-zero.
>>
>> The next patch will replace exit with _exit.
>>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>> ---
>>   common/config | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/common/config b/common/config
>> index 79bec87f..eb6af35a 100644
>> --- a/common/config
>> +++ b/common/config
>> @@ -96,6 +96,14 @@ 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.
> ...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()
>> +{
>> +	status="$1"
>> +	exit "$status"
>> +}
>> +
> I agree with Darrick’s suggestion here. It’s safer to update status only
> when an argument is passed - otherwise, it’s easy to trip over this.
>
> Let’s also avoid defaulting status to 0 inside _exit(). That way, if the
> caller forgets to pass an argument but has explicitly set status
> earlier, we preserve the intended value.
>
> We should update _exit() with...
>
> test -n "$1" && status="$1"

Okay, so in that case if someone does "status=<value>;_exit", we should 
end up with the "<value>" instead of something else, right?

--NR

>
> -ritesh
>
>
>>   # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
>>   set_mkfs_prog_path_with_opts()
>>   {
>> -- 
>> 2.34.1

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Darrick J. Wong]

On Tue, Apr 08, 2025 at 09:45:53PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/8/25 14:43, Ritesh Harjani (IBM) wrote:
> > "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> > 
> > > We should always set the value of status correctly when we are exiting.
> > > Else, "$?" might not give us the correct value.
> > > If we see the following trap
> > > handler registration in the check script:
> > > 
> > > if $OPTIONS_HAVE_SECTIONS; then
> > >       trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
> > > else
> > >       trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
> > > fi
> > > 
> > > So, "exit 1" will exit the check script without setting the correct
> > > return value. I ran with the following local.config file:
> > > 
> > > [xfs_4k_valid]
> > > FSTYP=xfs
> > > TEST_DEV=/dev/loop0
> > > TEST_DIR=/mnt1/test
> > > SCRATCH_DEV=/dev/loop1
> > > SCRATCH_MNT=/mnt1/scratch
> > > 
> > > [xfs_4k_invalid]
> > > FSTYP=xfs
> > > TEST_DEV=/dev/loop0
> > > TEST_DIR=/mnt1/invalid_dir
> > > SCRATCH_DEV=/dev/loop1
> > > SCRATCH_MNT=/mnt1/scratch
> > > 
> > > This caused the init_rc() to catch the case of invalid _test_mount
> > > options. Although the check script correctly failed during the execution
> > > of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
> > > returned 0. This is because init_rc exits with "exit 1" without
> > > correctly setting the value of "status". IMO, the correct behavior
> > > should have been that "$?" should have been non-zero.
> > > 
> > > The next patch will replace exit with _exit.
> > > 
> > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> > > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >   common/config | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/common/config b/common/config
> > > index 79bec87f..eb6af35a 100644
> > > --- a/common/config
> > > +++ b/common/config
> > > @@ -96,6 +96,14 @@ 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.
> > ...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()
> > > +{
> > > +	status="$1"
> > > +	exit "$status"
> > > +}
> > > +
> > I agree with Darrick’s suggestion here. It’s safer to update status only
> > when an argument is passed - otherwise, it’s easy to trip over this.
> > 
> > Let’s also avoid defaulting status to 0 inside _exit(). That way, if the
> > caller forgets to pass an argument but has explicitly set status
> > earlier, we preserve the intended value.
> > 
> > We should update _exit() with...
> > 
> > test -n "$1" && status="$1"
> 
> Okay, so in that case if someone does "status=<value>;_exit", we should end
> up with the "<value>" instead of something else, right?

Right.  I think.  AFAICT the following simple program actually does
return 5 despite the cleanup:

trap 'echo cleanup' INT QUIT TERM EXIT
exit 5

But since fstests set a variable named "status" and then "exit $status"
from cleanup, I think it doesn't matter how status gets set as long as
it /does/ get set somewhere.

--D

> --NR
> 
> > 
> > -ritesh
> > 
> > 
> > >   # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
> > >   set_mkfs_prog_path_with_opts()
> > >   {
> > > -- 
> > > 2.34.1
> 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
> 

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Nirjhar Roy (IBM)]

On 4/8/25 22:02, Darrick J. Wong wrote:
> On Tue, Apr 08, 2025 at 09:45:53PM +0530, Nirjhar Roy (IBM) wrote:
>> On 4/8/25 14:43, Ritesh Harjani (IBM) wrote:
>>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>>
>>>> We should always set the value of status correctly when we are exiting.
>>>> Else, "$?" might not give us the correct value.
>>>> If we see the following trap
>>>> handler registration in the check script:
>>>>
>>>> if $OPTIONS_HAVE_SECTIONS; then
>>>>        trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
>>>> else
>>>>        trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
>>>> fi
>>>>
>>>> So, "exit 1" will exit the check script without setting the correct
>>>> return value. I ran with the following local.config file:
>>>>
>>>> [xfs_4k_valid]
>>>> FSTYP=xfs
>>>> TEST_DEV=/dev/loop0
>>>> TEST_DIR=/mnt1/test
>>>> SCRATCH_DEV=/dev/loop1
>>>> SCRATCH_MNT=/mnt1/scratch
>>>>
>>>> [xfs_4k_invalid]
>>>> FSTYP=xfs
>>>> TEST_DEV=/dev/loop0
>>>> TEST_DIR=/mnt1/invalid_dir
>>>> SCRATCH_DEV=/dev/loop1
>>>> SCRATCH_MNT=/mnt1/scratch
>>>>
>>>> This caused the init_rc() to catch the case of invalid _test_mount
>>>> options. Although the check script correctly failed during the execution
>>>> of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
>>>> returned 0. This is because init_rc exits with "exit 1" without
>>>> correctly setting the value of "status". IMO, the correct behavior
>>>> should have been that "$?" should have been non-zero.
>>>>
>>>> The next patch will replace exit with _exit.
>>>>
>>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>>>> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>>>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>>>> ---
>>>>    common/config | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/common/config b/common/config
>>>> index 79bec87f..eb6af35a 100644
>>>> --- a/common/config
>>>> +++ b/common/config
>>>> @@ -96,6 +96,14 @@ 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.
>>> ...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()
>>>> +{
>>>> +	status="$1"
>>>> +	exit "$status"
>>>> +}
>>>> +
>>> I agree with Darrick’s suggestion here. It’s safer to update status only
>>> when an argument is passed - otherwise, it’s easy to trip over this.
>>>
>>> Let’s also avoid defaulting status to 0 inside _exit(). That way, if the
>>> caller forgets to pass an argument but has explicitly set status
>>> earlier, we preserve the intended value.
>>>
>>> We should update _exit() with...
>>>
>>> test -n "$1" && status="$1"
>> Okay, so in that case if someone does "status=<value>;_exit", we should end
>> up with the "<value>" instead of something else, right?
> Right.  I think.  AFAICT the following simple program actually does
> return 5 despite the cleanup:
>
> trap 'echo cleanup' INT QUIT TERM EXIT
> exit 5
>
> But since fstests set a variable named "status" and then "exit $status"
> from cleanup, I think it doesn't matter how status gets set as long as
> it /does/ get set somewhere.

Right, I have got the idea. I will make the changes in next revision.

--NR

>
> --D
>
>> --NR
>>
>>> -ritesh
>>>
>>>
>>>>    # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
>>>>    set_mkfs_prog_path_with_opts()
>>>>    {
>>>> -- 
>>>> 2.34.1
>> -- 
>> Nirjhar Roy
>> Linux Kernel Developer
>> IBM, Bangalore
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Nirjhar Roy (IBM)]

On 4/8/25 17:05, Dave Chinner wrote:
> On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote:
>> We should always set the value of status correctly when we are exiting.
>> Else, "$?" might not give us the correct value.
>> If we see the following trap
>> handler registration in the check script:
>>
>> if $OPTIONS_HAVE_SECTIONS; then
>>       trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
>> else
>>       trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
>> fi
>>
>> So, "exit 1" will exit the check script without setting the correct
>> return value. I ran with the following local.config file:
>>
>> [xfs_4k_valid]
>> FSTYP=xfs
>> TEST_DEV=/dev/loop0
>> TEST_DIR=/mnt1/test
>> SCRATCH_DEV=/dev/loop1
>> SCRATCH_MNT=/mnt1/scratch
>>
>> [xfs_4k_invalid]
>> FSTYP=xfs
>> TEST_DEV=/dev/loop0
>> TEST_DIR=/mnt1/invalid_dir
>> SCRATCH_DEV=/dev/loop1
>> SCRATCH_MNT=/mnt1/scratch
>>
>> This caused the init_rc() to catch the case of invalid _test_mount
>> options. Although the check script correctly failed during the execution
>> of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?"
>> returned 0. This is because init_rc exits with "exit 1" without
>> correctly setting the value of "status". IMO, the correct behavior
>> should have been that "$?" should have been non-zero.
>>
>> The next patch will replace exit with _exit.
>>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>> ---
>>   common/config | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/common/config b/common/config
>> index 79bec87f..eb6af35a 100644
>> --- a/common/config
>> +++ b/common/config
>> @@ -96,6 +96,14 @@ 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.
>> +_exit()
>> +{
>> +	status="$1"
>> +	exit "$status"
>> +}
> The only issue with putting this helper in common/config is that
> calling _exit() requires sourcing common/config from the shell
> context that calls it.
>
> This means every test must source common/config and re-execute the
> environment setup, even though we already have all the environment
> set up because it was exported from check when it sourced
> common/config.
>
> We have the same problem with _fatal() - it is defined in
> common/config instead of an independent common file. If we put such
> functions in their own common file, they can be sourced
> without dependencies on any other common file being included first.
>
> e.g. we create common/exit and place all the functions that
> terminate tests in it - _fatal, _notrun, _exit, etc and source that
> file once per shell context before we source common/config,
> common/rc, etc. This means we can source and call those termination
> functions from any context without having to worry about
> dependencies...

Yes, I agree to the above. Do you want this refactoring to be done as a 
part of this patch series in the further revisions, or can this be sent 
as a separate series?

--NR

>
> -Dave.
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Dave Chinner]

On Tue, Apr 08, 2025 at 10:13:54PM +0530, Nirjhar Roy (IBM) wrote:
> On 4/8/25 17:05, Dave Chinner wrote:
> > On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote:
> > > diff --git a/common/config b/common/config
> > > index 79bec87f..eb6af35a 100644
> > > --- a/common/config
> > > +++ b/common/config
> > > @@ -96,6 +96,14 @@ 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.
> > > +_exit()
> > > +{
> > > +	status="$1"
> > > +	exit "$status"
> > > +}
> > The only issue with putting this helper in common/config is that
> > calling _exit() requires sourcing common/config from the shell
> > context that calls it.
> > 
> > This means every test must source common/config and re-execute the
> > environment setup, even though we already have all the environment
> > set up because it was exported from check when it sourced
> > common/config.
> > 
> > We have the same problem with _fatal() - it is defined in
> > common/config instead of an independent common file. If we put such
> > functions in their own common file, they can be sourced
> > without dependencies on any other common file being included first.
> > 
> > e.g. we create common/exit and place all the functions that
> > terminate tests in it - _fatal, _notrun, _exit, etc and source that
> > file once per shell context before we source common/config,
> > common/rc, etc. This means we can source and call those termination
> > functions from any context without having to worry about
> > dependencies...
> 
> Yes, I agree to the above. Do you want this refactoring to be done as a part
> of this patch series in the further revisions, or can this be sent as a
> separate series?

Seperate series is fine. You're not making the dependency mess any
worse than it already is with this change...

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

Re: [PATCH v3 2/6] generic/367: Remove redundant sourcing if common/config

[Zorro Lang]

On Tue, Apr 08, 2025 at 05:37:18AM +0000, Nirjhar Roy (IBM) wrote:
> common/config will be source by _begin_fstest
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---

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

>  tests/generic/367 | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/generic/367 b/tests/generic/367
> index ed371a02..567db557 100755
> --- a/tests/generic/367
> +++ b/tests/generic/367
> @@ -11,7 +11,6 @@
>  # check if the extsize value and the xflag bit actually got reflected after
>  # setting/re-setting the extsize value.
>  
> -. ./common/config
>  . ./common/filter
>  . ./common/preamble
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 5/6] common/config: Introduce _exit wrapper around exit command

[Nirjhar Roy (IBM)]

On 4/9/25 06:14, Dave Chinner wrote:
> On Tue, Apr 08, 2025 at 10:13:54PM +0530, Nirjhar Roy (IBM) wrote:
>> On 4/8/25 17:05, Dave Chinner wrote:
>>> On Tue, Apr 08, 2025 at 05:37:21AM +0000, Nirjhar Roy (IBM) wrote:
>>>> diff --git a/common/config b/common/config
>>>> index 79bec87f..eb6af35a 100644
>>>> --- a/common/config
>>>> +++ b/common/config
>>>> @@ -96,6 +96,14 @@ 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.
>>>> +_exit()
>>>> +{
>>>> +	status="$1"
>>>> +	exit "$status"
>>>> +}
>>> The only issue with putting this helper in common/config is that
>>> calling _exit() requires sourcing common/config from the shell
>>> context that calls it.
>>>
>>> This means every test must source common/config and re-execute the
>>> environment setup, even though we already have all the environment
>>> set up because it was exported from check when it sourced
>>> common/config.
>>>
>>> We have the same problem with _fatal() - it is defined in
>>> common/config instead of an independent common file. If we put such
>>> functions in their own common file, they can be sourced
>>> without dependencies on any other common file being included first.
>>>
>>> e.g. we create common/exit and place all the functions that
>>> terminate tests in it - _fatal, _notrun, _exit, etc and source that
>>> file once per shell context before we source common/config,
>>> common/rc, etc. This means we can source and call those termination
>>> functions from any context without having to worry about
>>> dependencies...
>> Yes, I agree to the above. Do you want this refactoring to be done as a part
>> of this patch series in the further revisions, or can this be sent as a
>> separate series?
> Seperate series is fine. You're not making the dependency mess any
> worse than it already is with this change...

Okay, got it. I will add this list of ToDos. Thank you so much for this 
suggestion.

--NR

>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore