[PATCH v2 0/5] Minor cleanups in common/

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

[v1] --> v[2]
 1. Added R.B from Darrick in patch 1 of [v1]
 2. Kept the init_rc call that was deleted in the v1.
 3. Introduced _exit wrapper around exit command. This will help us get correct
    exit codes ("$?") on failures.

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

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

Nirjhar Roy (IBM) (5):
  generic/749: Remove redundant sourcing of common/rc
  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             |   8 +---
 common/btrfs      |   6 +--
 common/ceph       |   2 +-
 common/config     |  15 +++++--
 common/ext4       |   2 +-
 common/populate   |   2 +-
 common/preamble   |   3 +-
 common/punch      |  12 +++---
 common/rc         | 105 ++++++++++++++++++++++------------------------
 common/xfs        |   8 ++--
 tests/generic/749 |   1 -
 11 files changed, 81 insertions(+), 83 deletions(-)

--
2.34.1

[PATCH v2 1/5] 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>
---
 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 v2 2/5] 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>
---
 check | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/check b/check
index 32890470..16bf1586 100755
--- a/check
+++ b/check
@@ -792,12 +792,6 @@ function run_section()
 		_prepare_test_list
 	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
 		_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 v2 3/5] 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>
---
 check           | 2 ++
 common/preamble | 1 +
 common/rc       | 2 --
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/check b/check
index 16bf1586..2d2c82ac 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 v2 4/5] 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>
---
 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 v2 5/5] 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>
---
 common/btrfs    |   6 +--
 common/ceph     |   2 +-
 common/config   |   7 ++--
 common/ext4     |   2 +-
 common/populate |   2 +-
 common/preamble |   2 +-
 common/punch    |  12 +++---
 common/rc       | 103 +++++++++++++++++++++++-------------------------
 common/xfs      |   8 ++--
 9 files changed, 70 insertions(+), 74 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/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..6567b9d1 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";
 		}' |
@@ -225,7 +225,7 @@ _filter_bmap()
 die_now()
 {
 	status=1
-	exit
+	_exit
 }
 
 # test the different corner cases for zeroing a range:
@@ -276,7 +276,7 @@ _test_generic_punch()
 		u)	unwritten_tests=
 		;;
 		?)	echo Invalid flag
-		exit 1
+		_exit 1
 		;;
 		esac
 	done
@@ -552,7 +552,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..02dddc91 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
 	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,7 +4963,7 @@ init_rc()
 			if ! _test_mount
 			then
 				echo "common/rc: could not mount $TEST_DEV on $TEST_DIR"
-				exit 1
+				_exit 1
 			fi
 		fi
 	fi
@@ -4979,7 +4976,7 @@ init_rc()
 		# 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/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 v2 0/5] Minor cleanups in common/

[Dave Chinner]

On Tue, Apr 01, 2025 at 06:43:55AM +0000, Nirjhar Roy (IBM) wrote:
> 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.
> 
> [v1] --> v[2]
>  1. Added R.B from Darrick in patch 1 of [v1]
>  2. Kept the init_rc call that was deleted in the v1.
>  3. Introduced _exit wrapper around exit command. This will help us get correct
>     exit codes ("$?") on failures.
> 
> [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/
> 
> [v1] https://lore.kernel.org/all/cover.1741248214.git.nirjhar.roy.lists@gmail.com/
> 
> Nirjhar Roy (IBM) (5):
>   generic/749: Remove redundant sourcing of common/rc
>   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

Whole series looks fine to me. I've got similar patches in my
current check-parallel stack, as well as changing common/config to
match the "don't run setup code when sourcing the file" behaviour.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/5] generic/749: Remove redundant sourcing of common/rc

[Ritesh Harjani]

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

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

Indeed. The patch looks good to me. 
Feel free to add:

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

>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  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

Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check

[Ritesh Harjani]

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

> 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>
> ---
>  check | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/check b/check
> index 32890470..16bf1586 100755
> --- a/check
> +++ b/check
> @@ -792,12 +792,6 @@ function run_section()
>  		_prepare_test_list
>  	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
>  		_test_unmount 2> /dev/null

Would have been nicer if there was a small comment here like:

  	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
    fi

    init_rc

But either ways, no strong preference for adding comments here.

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


> -		if ! _test_mount
> -		then
> -			echo "check: failed to mount $TEST_DEV on $TEST_DIR"
> -			status=1
> -			exit
> -		fi
>  	fi
>  
>  	init_rc
> -- 
> 2.34.1

Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc

[Ritesh Harjani]

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

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

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


While reviewing this patch, I also noticed couple of related cleanups
which you might be interested in:

1. common/rc sources common/config which executes a function
_canonicalize_devices()

2. tests/generic/367 sources common/config which is not really
required since _begin_fstests() will anyways source common/rc and
common/config will get sourced automatically.

-ritesh

>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  check           | 2 ++
>  common/preamble | 1 +
>  common/rc       | 2 --
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/check b/check
> index 16bf1586..2d2c82ac 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

Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 4/4/25 09:30, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> 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.
> This patch looks good to me. Please feel free to add:
>       Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
> While reviewing this patch, I also noticed couple of related cleanups
> which you might be interested in:
>
> 1. common/rc sources common/config which executes a function
> _canonicalize_devices()
>
> 2. tests/generic/367 sources common/config which is not really
> required since _begin_fstests() will anyways source common/rc and
> common/config will get sourced automatically.

Thank you for the pointer. I can have follow-up patches for such cleanups.

--NR

>
> -ritesh
>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> ---
>>   check           | 2 ++
>>   common/preamble | 1 +
>>   common/rc       | 2 --
>>   3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/check b/check
>> index 16bf1586..2d2c82ac 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

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 4/5] 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.

Nice catch. Feel free to add:

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


>
> The next patch will replace exit with _exit.
>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.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

Re: [PATCH v2 5/5] common: exit --> _exit

[Ritesh Harjani]

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

> 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>
> ---
>  common/btrfs    |   6 +--
>  common/ceph     |   2 +-
>  common/config   |   7 ++--
>  common/ext4     |   2 +-
>  common/populate |   2 +-
>  common/preamble |   2 +-
>  common/punch    |  12 +++---
>  common/rc       | 103 +++++++++++++++++++++++-------------------------
>  common/xfs      |   8 ++--
>  9 files changed, 70 insertions(+), 74 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/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..6567b9d1 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";
>  		}' |
> @@ -225,7 +225,7 @@ _filter_bmap()
>  die_now()
>  {
>  	status=1
> -	exit
> +	_exit

Why not remove status=1 too and just do _exit 1 here too?
Like how we have done at other places?

Rest looks good to me. 

-ritesh

Re: [PATCH v2 0/5] Minor cleanups in common/

[Nirjhar Roy (IBM)]

On 4/2/25 03:07, Dave Chinner wrote:
> On Tue, Apr 01, 2025 at 06:43:55AM +0000, Nirjhar Roy (IBM) wrote:
>> 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.
>>
>> [v1] --> v[2]
>>   1. Added R.B from Darrick in patch 1 of [v1]
>>   2. Kept the init_rc call that was deleted in the v1.
>>   3. Introduced _exit wrapper around exit command. This will help us get correct
>>      exit codes ("$?") on failures.
>>
>> [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/
>>
>> [v1] https://lore.kernel.org/all/cover.1741248214.git.nirjhar.roy.lists@gmail.com/
>>
>> Nirjhar Roy (IBM) (5):
>>    generic/749: Remove redundant sourcing of common/rc
>>    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
> Whole series looks fine to me. I've got similar patches in my
> current check-parallel stack, as well as changing common/config to
> match the "don't run setup code when sourcing the file" behaviour.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thank you for the review.

--NR

>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 5/5] common: exit --> _exit

[Zorro Lang]

On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> 
> > 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>
> > ---
> >  common/btrfs    |   6 +--
> >  common/ceph     |   2 +-
> >  common/config   |   7 ++--
> >  common/ext4     |   2 +-
> >  common/populate |   2 +-
> >  common/preamble |   2 +-
> >  common/punch    |  12 +++---
> >  common/rc       | 103 +++++++++++++++++++++++-------------------------
> >  common/xfs      |   8 ++--
> >  9 files changed, 70 insertions(+), 74 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/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..6567b9d1 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";
> >  		}' |
> > @@ -225,7 +225,7 @@ _filter_bmap()
> >  die_now()
> >  {
> >  	status=1
> > -	exit
> > +	_exit
> 
> Why not remove status=1 too and just do _exit 1 here too?
> Like how we have done at other places?

Yeah, nice catch! As the defination of _exit:

  _exit()
  {
       status="$1"
       exit "$status"
  }

The
  "
  status=1
  exit
  "
should be equal to:
  "
  _exit 1
  "

And "_exit" looks not make sense, due to it gives null to status.

Same problem likes below:


@@ -3776,7 +3773,7 @@ _get_os_name()
                echo 'linux'
        else
                echo Unknown operating system: `uname`
-               exit
+               _exit


The "_exit" without argument looks not make sense.

Thanks,
Zorro

> 
> Rest looks good to me. 
> 
> -ritesh
> 

Re: [PATCH v2 5/5] common: exit --> _exit

[Ritesh Harjani]

Zorro Lang <zlang@redhat.com> writes:

> On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>> 
>> > 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>
<...>
>> > ---
>> > @@ -225,7 +225,7 @@ _filter_bmap()
>> >  die_now()
>> >  {
>> >  	status=1
>> > -	exit
>> > +	_exit
>> 
>> Why not remove status=1 too and just do _exit 1 here too?
>> Like how we have done at other places?
>
> Yeah, nice catch! As the defination of _exit:
>
>   _exit()
>   {
>        status="$1"
>        exit "$status"
>   }
>
> The
>   "
>   status=1
>   exit
>   "
> should be equal to:
>   "
>   _exit 1
>   "
>
> And "_exit" looks not make sense, due to it gives null to status.
>
> Same problem likes below:
>
>
> @@ -3776,7 +3773,7 @@ _get_os_name()
>                 echo 'linux'
>         else
>                 echo Unknown operating system: `uname`
> -               exit
> +               _exit
>
>
> The "_exit" without argument looks not make sense.
>

That's right. _exit called with no argument could make status as null.
To prevent such misuse in future, should we add a warning/echo message
if the no. of arguments passed to _exit() is not 1? 

-ritesh

Re: [PATCH v2 5/5] common: exit --> _exit

[Nirjhar Roy (IBM)]

On 4/7/25 21:49, Zorro Lang wrote:
> On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>
>>> 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>
>>> ---
>>>   common/btrfs    |   6 +--
>>>   common/ceph     |   2 +-
>>>   common/config   |   7 ++--
>>>   common/ext4     |   2 +-
>>>   common/populate |   2 +-
>>>   common/preamble |   2 +-
>>>   common/punch    |  12 +++---
>>>   common/rc       | 103 +++++++++++++++++++++++-------------------------
>>>   common/xfs      |   8 ++--
>>>   9 files changed, 70 insertions(+), 74 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/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..6567b9d1 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";
>>>   		}' |
>>> @@ -225,7 +225,7 @@ _filter_bmap()
>>>   die_now()
>>>   {
>>>   	status=1
>>> -	exit
>>> +	_exit
>> Why not remove status=1 too and just do _exit 1 here too?
>> Like how we have done at other places?
> Yeah, nice catch! As the defination of _exit:
>
>    _exit()
>    {
>         status="$1"
>         exit "$status"
>    }
>
> The
>    "
>    status=1
>    exit
>    "
> should be equal to:
>    "
>    _exit 1
>    "
>
> And "_exit" looks not make sense, due to it gives null to status.
>
> Same problem likes below:
>
>
> @@ -3776,7 +3773,7 @@ _get_os_name()
>                  echo 'linux'
>          else
>                  echo Unknown operating system: `uname`
> -               exit
> +               _exit
>
>
> The "_exit" without argument looks not make sense.

Yes, thank you Ritesh and Zorro. Yes it should have been "_exit 1". I 
missed it while making the changes. I will make the changes in v3 and 
add RBs from Dave[1] and Ritesh.

[1] https://lore.kernel.org/all/Z-xcne3f5Klvuxcq@dread.disaster.area/

>
> Thanks,
> Zorro
>
>> Rest looks good to me.
>>
>> -ritesh
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 5/5] common: exit --> _exit

[Darrick J. Wong]

On Tue, Apr 08, 2025 at 12:16:42AM +0530, Ritesh Harjani wrote:
> Zorro Lang <zlang@redhat.com> writes:

<snip>

> > Yeah, nice catch! As the defination of _exit:
> >
> >   _exit()
> >   {
> >        status="$1"
> >        exit "$status"
> >   }
> >
> > The
> >   "
> >   status=1
> >   exit
> >   "
> > should be equal to:
> >   "
> >   _exit 1
> >   "
> >
> > And "_exit" looks not make sense, due to it gives null to status.
> >
> > Same problem likes below:
> >
> >
> > @@ -3776,7 +3773,7 @@ _get_os_name()
> >                 echo 'linux'
> >         else
> >                 echo Unknown operating system: `uname`
> > -               exit
> > +               _exit
> >
> >
> > The "_exit" without argument looks not make sense.
> >
> 
> That's right. _exit called with no argument could make status as null.
> To prevent such misuse in future, should we add a warning/echo message
> if the no. of arguments passed to _exit() is not 1? 

Why not set status only if the caller provides an argument?

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

perhaps?

--D

> -ritesh

Re: [PATCH v2 5/5] common: exit --> _exit

[Nirjhar Roy (IBM)]

On 4/8/25 00:16, Ritesh Harjani (IBM) wrote:
> Zorro Lang <zlang@redhat.com> writes:
>
>> On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
>>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>>
>>>> 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>
> <...>
>>>> ---
>>>> @@ -225,7 +225,7 @@ _filter_bmap()
>>>>   die_now()
>>>>   {
>>>>   	status=1
>>>> -	exit
>>>> +	_exit
>>> Why not remove status=1 too and just do _exit 1 here too?
>>> Like how we have done at other places?
>> Yeah, nice catch! As the defination of _exit:
>>
>>    _exit()
>>    {
>>         status="$1"
>>         exit "$status"
>>    }
>>
>> The
>>    "
>>    status=1
>>    exit
>>    "
>> should be equal to:
>>    "
>>    _exit 1
>>    "
>>
>> And "_exit" looks not make sense, due to it gives null to status.
>>
>> Same problem likes below:
>>
>>
>> @@ -3776,7 +3773,7 @@ _get_os_name()
>>                  echo 'linux'
>>          else
>>                  echo Unknown operating system: `uname`
>> -               exit
>> +               _exit
>>
>>
>> The "_exit" without argument looks not make sense.
>>
> That's right. _exit called with no argument could make status as null.
Yes, that is correct.
> To prevent such misuse in future, should we add a warning/echo message

Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, 
for cases where the return value is a success, we simply use "_exit". 
Which one do you think adds more value and flexibility to the usage?

--NR

> if the no. of arguments passed to _exit() is not 1?
>
> -ritesh

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 5/5] common: exit --> _exit

[Nirjhar Roy (IBM)]

On 4/8/25 00:42, Darrick J. Wong wrote:
> On Tue, Apr 08, 2025 at 12:16:42AM +0530, Ritesh Harjani wrote:
>> Zorro Lang <zlang@redhat.com> writes:
> <snip>
>
>>> Yeah, nice catch! As the defination of _exit:
>>>
>>>    _exit()
>>>    {
>>>         status="$1"
>>>         exit "$status"
>>>    }
>>>
>>> The
>>>    "
>>>    status=1
>>>    exit
>>>    "
>>> should be equal to:
>>>    "
>>>    _exit 1
>>>    "
>>>
>>> And "_exit" looks not make sense, due to it gives null to status.
>>>
>>> Same problem likes below:
>>>
>>>
>>> @@ -3776,7 +3773,7 @@ _get_os_name()
>>>                  echo 'linux'
>>>          else
>>>                  echo Unknown operating system: `uname`
>>> -               exit
>>> +               _exit
>>>
>>>
>>> The "_exit" without argument looks not make sense.
>>>
>> That's right. _exit called with no argument could make status as null.
>> To prevent such misuse in future, should we add a warning/echo message
>> if the no. of arguments passed to _exit() is not 1?
> Why not set status only if the caller provides an argument?
>
> 	test -n "$1" && status="$1"

And if the caller doesn't provide any, should status hold some default 
value? I have suggested something in [1]

[1] 
https://lore.kernel.org/all/3c1d608d-4ea0-4e24-9abc-95eb226101c2@gmail.com/

--NR

>
> perhaps?
>
> --D
>
>> -ritesh

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check

[Nirjhar Roy (IBM)]

On 4/4/25 09:06, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> 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>
>> ---
>>   check | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/check b/check
>> index 32890470..16bf1586 100755
>> --- a/check
>> +++ b/check
>> @@ -792,12 +792,6 @@ function run_section()
>>   		_prepare_test_list
>>   	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
>>   		_test_unmount 2> /dev/null
> Would have been nicer if there was a small comment here like:
>
>    	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
>      fi
>
>      init_rc
>
> But either ways, no strong preference for adding comments here.

Added in [v3]

[v3] 
https://lore.kernel.org/all/ffefbe485f71206dd2a0a27256d1101d2b0c7a64.1744090313.git.nirjhar.roy.lists@gmail.com/

Thank you for pointing out.

--NR

>
> Feel free to add -
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
>> -		if ! _test_mount
>> -		then
>> -			echo "check: failed to mount $TEST_DEV on $TEST_DIR"
>> -			status=1
>> -			exit
>> -		fi
>>   	fi
>>   
>>   	init_rc
>> -- 
>> 2.34.1

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 4/4/25 09:30, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> 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.
> This patch looks good to me. Please feel free to add:
>       Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
> While reviewing this patch, I also noticed couple of related cleanups
> which you might be interested in:
>
> 1. common/rc sources common/config which executes a function
> _canonicalize_devices()
>
> 2. tests/generic/367 sources common/config which is not really
> required since _begin_fstests() will anyways source common/rc and
> common/config will get sourced automatically.

Addressed 2 in [v3].

[v3] 
https://lore.kernel.org/all/ffefbe485f71206dd2a0a27256d1101d2b0c7a64.1744090313.git.nirjhar.roy.lists@gmail.com/

Thanks.

--NR

>
> -ritesh
>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> ---
>>   check           | 2 ++
>>   common/preamble | 1 +
>>   common/rc       | 2 --
>>   3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/check b/check
>> index 16bf1586..2d2c82ac 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

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check

[Nirjhar Roy (IBM)]

On 4/4/25 09:06, Ritesh Harjani (IBM) wrote:
> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>
>> 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>
>> ---
>>   check | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/check b/check
>> index 32890470..16bf1586 100755
>> --- a/check
>> +++ b/check
>> @@ -792,12 +792,6 @@ function run_section()
>>   		_prepare_test_list
>>   	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
>>   		_test_unmount 2> /dev/null
> Would have been nicer if there was a small comment here like:
>
>    	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
>      fi
>
>      init_rc
>
> But either ways, no strong preference for adding comments here.

Addressed in 
https://lore.kernel.org/all/fa1bfd04d6b592f4d812a90039c643a02d7e1033.1744090313.git.nirjhar.roy.lists@gmail.com/ 
. Please ignore the previous link. I mistakenly gave the link to patch 2/6.

--NR

>
> Feel free to add -
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
>> -		if ! _test_mount
>> -		then
>> -			echo "check: failed to mount $TEST_DEV on $TEST_DIR"
>> -			status=1
>> -			exit
>> -		fi
>>   	fi
>>   
>>   	init_rc
>> -- 
>> 2.34.1

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 5/5] common: exit --> _exit

[Zorro Lang]

On Tue, Apr 08, 2025 at 12:43:32AM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/8/25 00:16, Ritesh Harjani (IBM) wrote:
> > Zorro Lang <zlang@redhat.com> writes:
> > 
> > > On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
> > > > "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> > > > 
> > > > > 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>
> > <...>
> > > > > ---
> > > > > @@ -225,7 +225,7 @@ _filter_bmap()
> > > > >   die_now()
> > > > >   {
> > > > >   	status=1
> > > > > -	exit
> > > > > +	_exit
> > > > Why not remove status=1 too and just do _exit 1 here too?
> > > > Like how we have done at other places?
> > > Yeah, nice catch! As the defination of _exit:
> > > 
> > >    _exit()
> > >    {
> > >         status="$1"
> > >         exit "$status"
> > >    }
> > > 
> > > The
> > >    "
> > >    status=1
> > >    exit
> > >    "
> > > should be equal to:
> > >    "
> > >    _exit 1
> > >    "
> > > 
> > > And "_exit" looks not make sense, due to it gives null to status.
> > > 
> > > Same problem likes below:
> > > 
> > > 
> > > @@ -3776,7 +3773,7 @@ _get_os_name()
> > >                  echo 'linux'
> > >          else
> > >                  echo Unknown operating system: `uname`
> > > -               exit
> > > +               _exit
> > > 
> > > 
> > > The "_exit" without argument looks not make sense.
> > > 
> > That's right. _exit called with no argument could make status as null.
> Yes, that is correct.
> > To prevent such misuse in future, should we add a warning/echo message
> 
> Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
                                           ^^^^^^^^^^^^^^
That's good to me, I'm just wondering if the default value should be "1", to
tell us "hey, there's an unknown exit status" :)

Thanks,
Zorro

> cases where the return value is a success, we simply use "_exit". Which one
> do you think adds more value and flexibility to the usage?
> 
> --NR
> 
> > if the no. of arguments passed to _exit() is not 1?
> > 
> > -ritesh
> 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
> 

Re: [PATCH v2 5/5] common: exit --> _exit

[Darrick J. Wong]

On Tue, Apr 08, 2025 at 10:27:48PM +0800, Zorro Lang wrote:
> On Tue, Apr 08, 2025 at 12:43:32AM +0530, Nirjhar Roy (IBM) wrote:
> > 
> > On 4/8/25 00:16, Ritesh Harjani (IBM) wrote:
> > > Zorro Lang <zlang@redhat.com> writes:
> > > 
> > > > On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
> > > > > "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
> > > > > 
> > > > > > 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>
> > > <...>
> > > > > > ---
> > > > > > @@ -225,7 +225,7 @@ _filter_bmap()
> > > > > >   die_now()
> > > > > >   {
> > > > > >   	status=1
> > > > > > -	exit
> > > > > > +	_exit
> > > > > Why not remove status=1 too and just do _exit 1 here too?
> > > > > Like how we have done at other places?
> > > > Yeah, nice catch! As the defination of _exit:
> > > > 
> > > >    _exit()
> > > >    {
> > > >         status="$1"
> > > >         exit "$status"
> > > >    }
> > > > 
> > > > The
> > > >    "
> > > >    status=1
> > > >    exit
> > > >    "
> > > > should be equal to:
> > > >    "
> > > >    _exit 1
> > > >    "
> > > > 
> > > > And "_exit" looks not make sense, due to it gives null to status.
> > > > 
> > > > Same problem likes below:
> > > > 
> > > > 
> > > > @@ -3776,7 +3773,7 @@ _get_os_name()
> > > >                  echo 'linux'
> > > >          else
> > > >                  echo Unknown operating system: `uname`
> > > > -               exit
> > > > +               _exit
> > > > 
> > > > 
> > > > The "_exit" without argument looks not make sense.
> > > > 
> > > That's right. _exit called with no argument could make status as null.
> > Yes, that is correct.
> > > To prevent such misuse in future, should we add a warning/echo message
> > 
> > Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
>                                            ^^^^^^^^^^^^^^
> That's good to me, I'm just wondering if the default value should be "1", to
> tell us "hey, there's an unknown exit status" :)

I think status=1 usually means failure...

/usr/include/stdlib.h:92:#define        EXIT_FAILURE    1       /* Failing exit status.  */
/usr/include/stdlib.h:93:#define        EXIT_SUCCESS    0       /* Successful exit status.  */

--D

> Thanks,
> Zorro
> 
> > cases where the return value is a success, we simply use "_exit". Which one
> > do you think adds more value and flexibility to the usage?
> > 
> > --NR
> > 
> > > if the no. of arguments passed to _exit() is not 1?
> > > 
> > > -ritesh
> > 
> > -- 
> > Nirjhar Roy
> > Linux Kernel Developer
> > IBM, Bangalore
> > 
> 
> 

Re: [PATCH v2 5/5] common: exit --> _exit

[Nirjhar Roy (IBM)]

On 4/8/25 20:03, Darrick J. Wong wrote:
> On Tue, Apr 08, 2025 at 10:27:48PM +0800, Zorro Lang wrote:
>> On Tue, Apr 08, 2025 at 12:43:32AM +0530, Nirjhar Roy (IBM) wrote:
>>> On 4/8/25 00:16, Ritesh Harjani (IBM) wrote:
>>>> Zorro Lang <zlang@redhat.com> writes:
>>>>
>>>>> On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
>>>>>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>>>>>
>>>>>>> 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>
>>>> <...>
>>>>>>> ---
>>>>>>> @@ -225,7 +225,7 @@ _filter_bmap()
>>>>>>>    die_now()
>>>>>>>    {
>>>>>>>    	status=1
>>>>>>> -	exit
>>>>>>> +	_exit
>>>>>> Why not remove status=1 too and just do _exit 1 here too?
>>>>>> Like how we have done at other places?
>>>>> Yeah, nice catch! As the defination of _exit:
>>>>>
>>>>>     _exit()
>>>>>     {
>>>>>          status="$1"
>>>>>          exit "$status"
>>>>>     }
>>>>>
>>>>> The
>>>>>     "
>>>>>     status=1
>>>>>     exit
>>>>>     "
>>>>> should be equal to:
>>>>>     "
>>>>>     _exit 1
>>>>>     "
>>>>>
>>>>> And "_exit" looks not make sense, due to it gives null to status.
>>>>>
>>>>> Same problem likes below:
>>>>>
>>>>>
>>>>> @@ -3776,7 +3773,7 @@ _get_os_name()
>>>>>                   echo 'linux'
>>>>>           else
>>>>>                   echo Unknown operating system: `uname`
>>>>> -               exit
>>>>> +               _exit
>>>>>
>>>>>
>>>>> The "_exit" without argument looks not make sense.
>>>>>
>>>> That's right. _exit called with no argument could make status as null.
>>> Yes, that is correct.
>>>> To prevent such misuse in future, should we add a warning/echo message
>>> Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
>>                                             ^^^^^^^^^^^^^^
>> That's good to me, I'm just wondering if the default value should be "1", to
>> tell us "hey, there's an unknown exit status" :)
> I think status=1 usually means failure...
>
> /usr/include/stdlib.h:92:#define        EXIT_FAILURE    1       /* Failing exit status.  */
> /usr/include/stdlib.h:93:#define        EXIT_SUCCESS    0       /* Successful exit status.  */

Yeah, right. I like Darrick's suggestion to explicitly set the value of 
status in _exit() only if it is passed (test -n "$1" && status="$1"). 
This will preserve any intentional pre-set value of status. I have sent 
a [v3] for this series but haven't modified the definition of _exit. I 
will wait for further comments on [v3] and make this change (along with 
the other changes that will be suggested).

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

--NR

>
> --D
>
>> Thanks,
>> Zorro
>>
>>> cases where the return value is a success, we simply use "_exit". Which one
>>> do you think adds more value and flexibility to the usage?
>>>
>>> --NR
>>>
>>>> if the no. of arguments passed to _exit() is not 1?
>>>>
>>>> -ritesh
>>> -- 
>>> Nirjhar Roy
>>> Linux Kernel Developer
>>> IBM, Bangalore
>>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore