[PATCH v1 0/2] Minor cleanups in common/rc

[PATCH v1 0/2] Minor cleanups in common/rc

[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.
The individual patches have the details. This is proposed in [1] and [2].

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

Nirjhar Roy (IBM) (2):
  generic/749: Remove redundant sourcing of common/rc
  check,common/{preamble,rc},soak: Decoupling init_rc() call from
    sourcing common/rc

 check             | 10 ++--------
 common/preamble   |  1 +
 common/rc         |  2 --
 soak              |  1 +
 tests/generic/749 |  1 -
 5 files changed, 4 insertions(+), 11 deletions(-)

--
2.34.1

[PATCH v1 1/2] 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>
---
 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 v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

Silently executing scripts during sourcing common/rc doesn't look good
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           | 10 ++--------
 common/preamble |  1 +
 common/rc       |  2 --
 soak            |  1 +
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/check b/check
index ea92b0d6..d30af1ba 100755
--- a/check
+++ b/check
@@ -840,16 +840,8 @@ 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
-
 	seq="check.$$"
 	check="$RESULT_BASE/check"
 
@@ -870,6 +862,8 @@ function run_section()
 	needwrap=true
 
 	if [ ! -z "$SCRATCH_DEV" ]; then
+		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
+		[ $? -le 1 ] || exit 1
 	  _scratch_unmount 2> /dev/null
 	  # call the overridden mkfs - make sure the FS is built
 	  # the same as we'll create it later.
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 d2de8588..f153ad81 100644
--- a/common/rc
+++ b/common/rc
@@ -5754,8 +5754,6 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
-
 ################################################################################
 # make sure this script returns success
 /bin/true
diff --git a/soak b/soak
index d5c4229a..5734d854 100755
--- a/soak
+++ b/soak
@@ -5,6 +5,7 @@
 
 # get standard environment, filters and checks
 . ./common/rc
+# ToDo: Do we need an init_rc() here? How is soak used?
 . ./common/filter
 
 tmp=/tmp/$$
-- 
2.34.1

Re: [PATCH v1 1/2] generic/749: Remove redundant sourcing of common/rc

[Darrick J. Wong]

On Thu, Mar 06, 2025 at 08:17:40AM +0000, Nirjhar Roy (IBM) wrote:
> 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>

Yeah, that sourceing shouldn't be necessary.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  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 v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Darrick J. Wong]

On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> Silently executing scripts during sourcing common/rc doesn't look good
> 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           | 10 ++--------
>  common/preamble |  1 +
>  common/rc       |  2 --
>  soak            |  1 +
>  4 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/check b/check
> index ea92b0d6..d30af1ba 100755
> --- a/check
> +++ b/check
> @@ -840,16 +840,8 @@ 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

Unrelated change?  I was expecting a mechanical ". ./common/rc" =>
". ./common/rc ; init_rc" change in this patch.

>  	fi
>  
> -	init_rc

Why remove init_rc here?

> -
>  	seq="check.$$"
>  	check="$RESULT_BASE/check"
>  
> @@ -870,6 +862,8 @@ function run_section()
>  	needwrap=true
>  
>  	if [ ! -z "$SCRATCH_DEV" ]; then
> +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
> +		[ $? -le 1 ] || exit 1
>  	  _scratch_unmount 2> /dev/null
>  	  # call the overridden mkfs - make sure the FS is built
>  	  # the same as we'll create it later.
> 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 d2de8588..f153ad81 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5754,8 +5754,6 @@ _require_program() {
>  	_have_program "$1" || _notrun "$tag required"
>  }
>  
> -init_rc
> -
>  ################################################################################
>  # make sure this script returns success
>  /bin/true
> diff --git a/soak b/soak
> index d5c4229a..5734d854 100755
> --- a/soak
> +++ b/soak
> @@ -5,6 +5,7 @@
>  
>  # get standard environment, filters and checks
>  . ./common/rc
> +# ToDo: Do we need an init_rc() here? How is soak used?

I have no idea what soak does and have never used it, but I think for
continuity's sake you should call init_rc here.

--D

>  . ./common/filter
>  
>  tmp=/tmp/$$
> -- 
> 2.34.1
> 
> 

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Zorro Lang]

On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> Silently executing scripts during sourcing common/rc doesn't look good
> 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           | 10 ++--------
>  common/preamble |  1 +
>  common/rc       |  2 --
>  soak            |  1 +
>  4 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/check b/check
> index ea92b0d6..d30af1ba 100755
> --- a/check
> +++ b/check
> @@ -840,16 +840,8 @@ 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

Why remove these lines?

>  	fi
>  
> -	init_rc

Doesn't the "check" need init_rc at here?

> -
>  	seq="check.$$"
>  	check="$RESULT_BASE/check"
>  
> @@ -870,6 +862,8 @@ function run_section()
>  	needwrap=true
>  
>  	if [ ! -z "$SCRATCH_DEV" ]; then
> +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
> +		[ $? -le 1 ] || exit 1
         ^^^^^^^
         Different indent with below code.

This looks like part of init_rc. If you don't remove above init_rc, can this
change be saved? 

>  	  _scratch_unmount 2> /dev/null
>  	  # call the overridden mkfs - make sure the FS is built
>  	  # the same as we'll create it later.
> 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 d2de8588..f153ad81 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5754,8 +5754,6 @@ _require_program() {
>  	_have_program "$1" || _notrun "$tag required"
>  }
>  
> -init_rc
> -
>  ################################################################################
>  # make sure this script returns success
>  /bin/true
> diff --git a/soak b/soak
> index d5c4229a..5734d854 100755
> --- a/soak
> +++ b/soak
> @@ -5,6 +5,7 @@
>  
>  # get standard environment, filters and checks
>  . ./common/rc
> +# ToDo: Do we need an init_rc() here? How is soak used?

I never noticed we have this file... this file was create by:

  commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247
  Author: Nathan Scott <nathans@sgi.com>
  Date:   Mon Jan 15 05:01:19 2001 +0000

      cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001

I can't understand the relationship of this commit with this file. Does
anyone learn about the history of it.

I tried to "grep" the whole fstests, looks like nothing uses this file.
Maybe we should remove it?

Thanks,
Zorro

>  . ./common/filter
>  
>  tmp=/tmp/$$
> -- 
> 2.34.1
> 
> 

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Dave Chinner]

On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> Silently executing scripts during sourcing common/rc doesn't look good
> 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>

FWIW, I've just done somethign similar for check-parallel. I need to
decouple common/config from common/rc and not run any code from
either common/config or common/rc.

I've included the patch below (it won't apply because there's all
sorts of refactoring for test list and config-section parsing in the
series before it), but it should give you an idea of how I think we
should be separating one-off initialisation environment varaibles,
common code inclusion and the repeated initialisation of section
specific parameters....

.....
> diff --git a/soak b/soak
> index d5c4229a..5734d854 100755
> --- a/soak
> +++ b/soak
> @@ -5,6 +5,7 @@
>  
>  # get standard environment, filters and checks
>  . ./common/rc
> +# ToDo: Do we need an init_rc() here? How is soak used?
>  . ./common/filter

I've also go a patch series that removes all these old 2000-era SGI
QE scripts that have not been used by anyone for the last 15
years. I did that to get rid of the technical debt that these
scripts have gathered over years of neglect. They aren't used, we
shouldn't even attempt to maintain them anymore.

-Dave.

-- 
Dave Chinner
david@fromorbit.com


fstests: separate sourcing common/rc and common/config from initialisation

From: Dave Chinner <dchinner@redhat.com>

The sourcing of common/rc causes code to be run. init_rc
is executed from common/rc, and it includes common/config which
also runs a couple of initialisation functions.

This is messy, because re-sourcing those files also does an awful
lot more setup work than running those a couple of init functions.

common/config only needs to be included once - everything that
scripts then depend on should be exported by it, and hence it should
only be included once from check/check-parallel to set up all the
environmental parameters for the entire run.

common/rc also only needs to be included once per context, but it
does not need to directly include common config nor does it need to
run init_rc in each individual test context.

Seperate out this mess. Include common/config directly where needed
and only use it to set up the environment. Move all the code that is
in common/config to common/rc so that common/config is not needed
for any purpose other than setting up the initial environment.
Move the initialisation functions to the scripts that include
common/config.

Config file and config section parsing can be run directly from check
and/or check-parallel; this is not needed for every context that
needs to know how what XFS_MKFS_PROG is set to...

Similarly, include common/rc only once, and only call init_rc or
_source_specific_fs() from the contexts that actually need that code
to be run.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 check             |  24 +++---
 common/config     | 218 --------------------------------------------------
 common/preamble   |   1 +
 common/rc         | 234 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 tests/generic/367 |   7 +-
 tests/generic/749 |   3 +-
 6 files changed, 235 insertions(+), 252 deletions(-)

diff --git a/check b/check
index b4d31d138..b968a134a 100755
--- a/check
+++ b/check
@@ -43,10 +43,12 @@ timestamp=${TIMESTAMP:=false}
 
 rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
 
-# We need to include the test list processing first as argument parsing
-# requires test list parsing and setup.
+. ./common/config
+. ./common/config-sections
+. ./common/rc
 . ./common/test_list
 
+
 usage()
 {
     echo "Usage: $0 [options] [testlist]"'
@@ -183,11 +185,15 @@ while [ $# -gt 0 ]; do
 	shift
 done
 
-# we need common/rc, that also sources common/config. We need to source it
-# after processing args, overlay needs FSTYP set before sourcing common/config
-if ! . ./common/rc; then
-	echo "check: failed to source common/rc"
-	exit 1
+# now we have done argument parsing, overlay has FSTYP set and we can now
+# start processing the config files and setting up devices.
+_config_section_setup
+_canonicalize_devices
+init_rc
+
+if [ ! -z "$REPORT_LIST" ]; then
+	. ./common/report
+	_assert_report_list
 fi
 
 # If the test config specified a soak test duration, see if there are any
@@ -601,10 +607,6 @@ function run_section()
 			status=1
 			exit
 		fi
-		# Previous FSTYP derived from TEST_DEV could be changed, source
-		# common/rc again with correct FSTYP to get FSTYP specific configs,
-		# e.g. common/xfs
-		. common/rc
 		_tl_prepare_test_list
 	elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
 		_test_unmount 2> /dev/null
diff --git a/common/config b/common/config
index 571e52a58..03970bf54 100644
--- a/common/config
+++ b/common/config
@@ -40,7 +40,6 @@
 #
 
 . common/test_names
-. common/config-sections
 
 # all tests should use a common language setting to prevent golden
 # output mismatches.
@@ -348,220 +347,3 @@ if [ -x /usr/sbin/selinuxenabled ] && /usr/sbin/selinuxenabled; then
 	export SELINUX_MOUNT_OPTIONS
 fi
 
-_common_mount_opts()
-{
-	case $FSTYP in
-	9p)
-		echo $PLAN9_MOUNT_OPTIONS
-		;;
-	fuse)
-		echo $FUSE_MOUNT_OPTIONS
-		;;
-	xfs)
-		echo $XFS_MOUNT_OPTIONS
-		;;
-	udf)
-		echo $UDF_MOUNT_OPTIONS
-		;;
-	nfs)
-		echo $NFS_MOUNT_OPTIONS
-		;;
-	afs)
-		echo $AFS_MOUNT_OPTIONS
-		;;
-	cifs)
-		echo $CIFS_MOUNT_OPTIONS
-		;;
-	ceph)
-		echo $CEPHFS_MOUNT_OPTIONS
-		;;
-	glusterfs)
-		echo $GLUSTERFS_MOUNT_OPTIONS
-		;;
-	overlay)
-		echo $OVERLAY_MOUNT_OPTIONS
-		;;
-	ext2|ext3|ext4)
-		# acls & xattrs aren't turned on by default on ext$FOO
-		echo "-o acl,user_xattr $EXT_MOUNT_OPTIONS"
-		;;
-	f2fs)
-		echo "-o acl,user_xattr $F2FS_MOUNT_OPTIONS"
-		;;
-	reiserfs)
-		# acls & xattrs aren't turned on by default on reiserfs
-		echo "-o acl,user_xattr $REISERFS_MOUNT_OPTIONS"
-		;;
-       reiser4)
-		# acls & xattrs aren't supported by reiser4
-		echo $REISER4_MOUNT_OPTIONS
-		;;
-	gfs2)
-		# acls aren't turned on by default on gfs2
-		echo "-o acl $GFS2_MOUNT_OPTIONS"
-		;;
-	tmpfs)
-		# We need to specify the size at mount, use 1G by default
-		echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
-		;;
-	ubifs)
-		echo $UBIFS_MOUNT_OPTIONS
-		;;
-	*)
-		;;
-	esac
-}
-
-_mount_opts()
-{
-	export MOUNT_OPTIONS=$(_common_mount_opts)
-}
-
-_test_mount_opts()
-{
-	export TEST_FS_MOUNT_OPTS=$(_common_mount_opts)
-}
-
-_mkfs_opts()
-{
-	case $FSTYP in
-	xfs)
-		export MKFS_OPTIONS=$XFS_MKFS_OPTIONS
-		;;
-	udf)
-		[ ! -z "$udf_fsize" ] && \
-			UDF_MKFS_OPTIONS="$UDF_MKFS_OPTIONS -s $udf_fsize"
-		export MKFS_OPTIONS=$UDF_MKFS_OPTIONS
-		;;
-	nfs)
-		export MKFS_OPTIONS=$NFS_MKFS_OPTIONS
-		;;
-	afs)
-		export MKFS_OPTIONS=$AFS_MKFS_OPTIONS
-		;;
-	cifs)
-		export MKFS_OPTIONS=$CIFS_MKFS_OPTIONS
-		;;
-	ceph)
-		export MKFS_OPTIONS=$CEPHFS_MKFS_OPTIONS
-		;;
-	reiserfs)
-		export MKFS_OPTIONS="$REISERFS_MKFS_OPTIONS -q"
-		;;
-       reiser4)
-		export MKFS_OPTIONS=$REISER4_MKFS_OPTIONS
-		;;
-	gfs2)
-		export MKFS_OPTIONS="$GFS2_MKFS_OPTIONS -O -p lock_nolock"
-		;;
-	jfs)
-		export MKFS_OPTIONS="$JFS_MKFS_OPTIONS -q"
-		;;
-	f2fs)
-		export MKFS_OPTIONS="$F2FS_MKFS_OPTIONS"
-		;;
-	btrfs)
-		export MKFS_OPTIONS="$BTRFS_MKFS_OPTIONS"
-		;;
-	bcachefs)
-		export MKFS_OPTIONS=$BCACHEFS_MKFS_OPTIONS
-		;;
-	*)
-		;;
-	esac
-}
-
-_fsck_opts()
-{
-	case $FSTYP in
-	ext2|ext3|ext4)
-		export FSCK_OPTIONS="-nf"
-		;;
-	reiser*)
-		export FSCK_OPTIONS="--yes"
-		;;
-	f2fs)
-		export FSCK_OPTIONS=""
-		;;
-	*)
-		export FSCK_OPTIONS="-n"
-		;;
-	esac
-}
-
-# check necessary running dependences then source sepcific fs helpers
-_source_specific_fs()
-{
-	local fs=$1
-
-	if [ -z "$fs" ];then
-		fs=$FSTYP
-	fi
-
-	case "$fs" in
-	xfs)
-		[ "$XFS_LOGPRINT_PROG" = "" ] && _fatal "xfs_logprint not found"
-		[ "$XFS_REPAIR_PROG" = "" ] && _fatal "xfs_repair not found"
-		[ "$XFS_DB_PROG" = "" ] && _fatal "xfs_db not found"
-		[ "$MKFS_XFS_PROG" = "" ] && _fatal "mkfs_xfs not found"
-		[ "$XFS_INFO_PROG" = "" ] && _fatal "xfs_info not found"
-
-		. ./common/xfs
-		;;
-	udf)
-		[ "$MKFS_UDF_PROG" = "" ] && _fatal "mkfs_udf/mkudffs not found"
-		;;
-	btrfs)
-		[ "$MKFS_BTRFS_PROG" = "" ] && _fatal "mkfs.btrfs not found"
-
-		. ./common/btrfs
-		;;
-	ext4)
-		[ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
-		. ./common/ext4
-		;;
-	ext2|ext3)
-		. ./common/ext4
-		;;
-	f2fs)
-		[ "$MKFS_F2FS_PROG" = "" ] && _fatal "mkfs.f2fs not found"
-		;;
-	nfs)
-		. ./common/nfs
-		;;
-	afs)
-		;;
-	cifs)
-		;;
-	9p)
-		;;
-	fuse)
-		;;
-	ceph)
-		. ./common/ceph
-		;;
-	glusterfs)
-		;;
-	overlay)
-		. ./common/overlay
-		;;
-	reiser4)
-		[ "$MKFS_REISER4_PROG" = "" ] && _fatal "mkfs.reiser4 not found"
-		;;
-	pvfs2)
-		;;
-	ubifs)
-		[ "$UBIUPDATEVOL_PROG" = "" ] && _fatal "ubiupdatevol not found"
-		. ./common/ubifs
-		;;
-	esac
-}
-
-_config_section_setup
-_canonicalize_devices
-# mkfs.xfs checks for TEST_DEV before permitting < 300M filesystems. TEST_DIR
-# and QA_CHECK_FS are also checked by mkfs.xfs, but already exported elsewhere.
-export TEST_DEV
-
-# make sure this script returns success
-/bin/true
diff --git a/common/preamble b/common/preamble
index 0c9ee2e03..1f40dd5d1 100644
--- a/common/preamble
+++ b/common/preamble
@@ -50,6 +50,7 @@ _begin_fstest()
 	_register_cleanup _cleanup
 
 	. ./common/rc
+	_source_specific_fs $FSTYP
 
 	# remove previous $seqres.full before test
 	rm -f $seqres.full $seqres.hints
diff --git a/common/rc b/common/rc
index 056112714..01f8cba2e 100644
--- a/common/rc
+++ b/common/rc
@@ -2,10 +2,11 @@
 # SPDX-License-Identifier: GPL-2.0+
 # Copyright (c) 2000-2006 Silicon Graphics, Inc.  All Rights Reserved.
 
-. common/config
-
 BC="$(type -P bc)" || BC=
 
+# make sure we have a standard umask
+umask 022
+
 # Don't use sync(1) directly if at all possible. In most cases we only need to
 # sync the fs under test, so we use syncfs if it is supported to prevent
 # disturbance of other tests that may be running concurrently.
@@ -250,17 +251,6 @@ _log_err()
     echo "(see $seqres.full for details)"
 }
 
-# make sure we have a standard umask
-umask 022
-
-# check for correct setup and source the $FSTYP specific functions now
-_source_specific_fs $FSTYP
-
-if [ ! -z "$REPORT_LIST" ]; then
-	. ./common/report
-	_assert_report_list
-fi
-
 _get_filesize()
 {
     stat -c %s "$1"
@@ -4895,6 +4885,8 @@ init_rc()
 		exit 1
 	fi
 
+	_source_specific_fs $FSTYP
+
 	# if $TEST_DEV is not mounted, mount it now as XFS
 	if [ -z "`_fs_type $TEST_DEV`" ]
 	then
@@ -4934,6 +4926,11 @@ init_rc()
 	# it is supported.
 	$XFS_IO_PROG -i -c quit 2>/dev/null && \
 		export XFS_IO_PROG="$XFS_IO_PROG -i"
+
+	# mkfs.xfs checks for TEST_DEV before permitting < 300M filesystems.
+	# TEST_DIR and QA_CHECK_FS are also checked by mkfs.xfs, but already
+	# exported elsewhere.
+	export TEST_DEV
 }
 
 # get real device path name by following link
@@ -5757,8 +5754,211 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
+_common_mount_opts()
+{
+	case $FSTYP in
+	9p)
+		echo $PLAN9_MOUNT_OPTIONS
+		;;
+	fuse)
+		echo $FUSE_MOUNT_OPTIONS
+		;;
+	xfs)
+		echo $XFS_MOUNT_OPTIONS
+		;;
+	udf)
+		echo $UDF_MOUNT_OPTIONS
+		;;
+	nfs)
+		echo $NFS_MOUNT_OPTIONS
+		;;
+	afs)
+		echo $AFS_MOUNT_OPTIONS
+		;;
+	cifs)
+		echo $CIFS_MOUNT_OPTIONS
+		;;
+	ceph)
+		echo $CEPHFS_MOUNT_OPTIONS
+		;;
+	glusterfs)
+		echo $GLUSTERFS_MOUNT_OPTIONS
+		;;
+	overlay)
+		echo $OVERLAY_MOUNT_OPTIONS
+		;;
+	ext2|ext3|ext4)
+		# acls & xattrs aren't turned on by default on ext$FOO
+		echo "-o acl,user_xattr $EXT_MOUNT_OPTIONS"
+		;;
+	f2fs)
+		echo "-o acl,user_xattr $F2FS_MOUNT_OPTIONS"
+		;;
+	reiserfs)
+		# acls & xattrs aren't turned on by default on reiserfs
+		echo "-o acl,user_xattr $REISERFS_MOUNT_OPTIONS"
+		;;
+       reiser4)
+		# acls & xattrs aren't supported by reiser4
+		echo $REISER4_MOUNT_OPTIONS
+		;;
+	gfs2)
+		# acls aren't turned on by default on gfs2
+		echo "-o acl $GFS2_MOUNT_OPTIONS"
+		;;
+	tmpfs)
+		# We need to specify the size at mount, use 1G by default
+		echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
+		;;
+	ubifs)
+		echo $UBIFS_MOUNT_OPTIONS
+		;;
+	*)
+		;;
+	esac
+}
 
-################################################################################
-# make sure this script returns success
-/bin/true
+_mount_opts()
+{
+	export MOUNT_OPTIONS=$(_common_mount_opts)
+}
+
+_test_mount_opts()
+{
+	export TEST_FS_MOUNT_OPTS=$(_common_mount_opts)
+}
+
+_mkfs_opts()
+{
+	case $FSTYP in
+	xfs)
+		export MKFS_OPTIONS=$XFS_MKFS_OPTIONS
+		;;
+	udf)
+		[ ! -z "$udf_fsize" ] && \
+			UDF_MKFS_OPTIONS="$UDF_MKFS_OPTIONS -s $udf_fsize"
+		export MKFS_OPTIONS=$UDF_MKFS_OPTIONS
+		;;
+	nfs)
+		export MKFS_OPTIONS=$NFS_MKFS_OPTIONS
+		;;
+	afs)
+		export MKFS_OPTIONS=$AFS_MKFS_OPTIONS
+		;;
+	cifs)
+		export MKFS_OPTIONS=$CIFS_MKFS_OPTIONS
+		;;
+	ceph)
+		export MKFS_OPTIONS=$CEPHFS_MKFS_OPTIONS
+		;;
+	reiserfs)
+		export MKFS_OPTIONS="$REISERFS_MKFS_OPTIONS -q"
+		;;
+       reiser4)
+		export MKFS_OPTIONS=$REISER4_MKFS_OPTIONS
+		;;
+	gfs2)
+		export MKFS_OPTIONS="$GFS2_MKFS_OPTIONS -O -p lock_nolock"
+		;;
+	jfs)
+		export MKFS_OPTIONS="$JFS_MKFS_OPTIONS -q"
+		;;
+	f2fs)
+		export MKFS_OPTIONS="$F2FS_MKFS_OPTIONS"
+		;;
+	btrfs)
+		export MKFS_OPTIONS="$BTRFS_MKFS_OPTIONS"
+		;;
+	bcachefs)
+		export MKFS_OPTIONS=$BCACHEFS_MKFS_OPTIONS
+		;;
+	*)
+		;;
+	esac
+}
+
+_fsck_opts()
+{
+	case $FSTYP in
+	ext2|ext3|ext4)
+		export FSCK_OPTIONS="-nf"
+		;;
+	reiser*)
+		export FSCK_OPTIONS="--yes"
+		;;
+	f2fs)
+		export FSCK_OPTIONS=""
+		;;
+	*)
+		export FSCK_OPTIONS="-n"
+		;;
+	esac
+}
+
+# check necessary running dependences then source sepcific fs helpers
+_source_specific_fs()
+{
+	local fs=$1
+
+	if [ -z "$fs" ];then
+		fs=$FSTYP
+	fi
+
+	case "$fs" in
+	xfs)
+		[ "$XFS_LOGPRINT_PROG" = "" ] && _fatal "xfs_logprint not found"
+		[ "$XFS_REPAIR_PROG" = "" ] && _fatal "xfs_repair not found"
+		[ "$XFS_DB_PROG" = "" ] && _fatal "xfs_db not found"
+		[ "$MKFS_XFS_PROG" = "" ] && _fatal "mkfs_xfs not found"
+		[ "$XFS_INFO_PROG" = "" ] && _fatal "xfs_info not found"
+
+		. ./common/xfs
+		;;
+	udf)
+		[ "$MKFS_UDF_PROG" = "" ] && _fatal "mkfs_udf/mkudffs not found"
+		;;
+	btrfs)
+		[ "$MKFS_BTRFS_PROG" = "" ] && _fatal "mkfs.btrfs not found"
+
+		. ./common/btrfs
+		;;
+	ext4)
+		[ "$MKFS_EXT4_PROG" = "" ] && _fatal "mkfs.ext4 not found"
+		. ./common/ext4
+		;;
+	ext2|ext3)
+		. ./common/ext4
+		;;
+	f2fs)
+		[ "$MKFS_F2FS_PROG" = "" ] && _fatal "mkfs.f2fs not found"
+		;;
+	nfs)
+		. ./common/nfs
+		;;
+	afs)
+		;;
+	cifs)
+		;;
+	9p)
+		;;
+	fuse)
+		;;
+	ceph)
+		. ./common/ceph
+		;;
+	glusterfs)
+		;;
+	overlay)
+		. ./common/overlay
+		;;
+	reiser4)
+		[ "$MKFS_REISER4_PROG" = "" ] && _fatal "mkfs.reiser4 not found"
+		;;
+	pvfs2)
+		;;
+	ubifs)
+		[ "$UBIUPDATEVOL_PROG" = "" ] && _fatal "ubiupdatevol not found"
+		. ./common/ubifs
+		;;
+	esac
+}
diff --git a/tests/generic/367 b/tests/generic/367
index ed371a02b..1c9c66730 100755
--- a/tests/generic/367
+++ b/tests/generic/367
@@ -10,13 +10,12 @@
 # i.e, with any file with allocated extents or delayed allocation. We also
 # 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
-
 _begin_fstest ioctl quick
 
+. ./common/filter
+
 [ "$FSTYP" = "xfs" ] && _fixed_by_kernel_commit 2a492ff66673 \
 	"xfs: Check for delayed allocations before setting extsize"
 
diff --git a/tests/generic/749 b/tests/generic/749
index fc7477380..aac8da20d 100755
--- a/tests/generic/749
+++ b/tests/generic/749
@@ -14,11 +14,10 @@
 # page, you should get a SIGBUS. This test verifies we zero-fill to page
 # 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.
 . ./common/filter
 
 _require_scratch_nocheck

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/6/25 23:16, Darrick J. Wong wrote:
> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>> Silently executing scripts during sourcing common/rc doesn't look good
>> 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           | 10 ++--------
>>   common/preamble |  1 +
>>   common/rc       |  2 --
>>   soak            |  1 +
>>   4 files changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/check b/check
>> index ea92b0d6..d30af1ba 100755
>> --- a/check
>> +++ b/check
>> @@ -840,16 +840,8 @@ 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
> Unrelated change?  I was expecting a mechanical ". ./common/rc" =>
> ". ./common/rc ; init_rc" change in this patch.
This patch adds an init_rc() call to _begin_fstests() in common/preamble 
and hence the above _test_mount() will be executed during that call. So 
this _test_mount isn't necessary here, right? _test_mount() will be 
executed (as a part of init_rc() call) before every test run. Please let 
me know if my understanding isn't correct.
>>   	fi
>>   
>> -	init_rc
> Why remove init_rc here?
Same reason as above.
>
>> -
>>   	seq="check.$$"
>>   	check="$RESULT_BASE/check"
>>   
>> @@ -870,6 +862,8 @@ function run_section()
>>   	needwrap=true
>>   
>>   	if [ ! -z "$SCRATCH_DEV" ]; then
>> +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
>> +		[ $? -le 1 ] || exit 1
>>   	  _scratch_unmount 2> /dev/null
>>   	  # call the overridden mkfs - make sure the FS is built
>>   	  # the same as we'll create it later.
>> 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 d2de8588..f153ad81 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -5754,8 +5754,6 @@ _require_program() {
>>   	_have_program "$1" || _notrun "$tag required"
>>   }
>>   
>> -init_rc
>> -
>>   ################################################################################
>>   # make sure this script returns success
>>   /bin/true
>> diff --git a/soak b/soak
>> index d5c4229a..5734d854 100755
>> --- a/soak
>> +++ b/soak
>> @@ -5,6 +5,7 @@
>>   
>>   # get standard environment, filters and checks
>>   . ./common/rc
>> +# ToDo: Do we need an init_rc() here? How is soak used?
> I have no idea what soak does and have never used it, but I think for
> continuity's sake you should call init_rc here.

Okay. I think Dave has suggested removing this file[1]. This doesn't 
seem to used anymore.

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

--NR

>
> --D
>
>>   . ./common/filter
>>   
>>   tmp=/tmp/$$
>> -- 
>> 2.34.1
>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/7/25 02:43, Zorro Lang wrote:
> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>> Silently executing scripts during sourcing common/rc doesn't look good
>> 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           | 10 ++--------
>>   common/preamble |  1 +
>>   common/rc       |  2 --
>>   soak            |  1 +
>>   4 files changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/check b/check
>> index ea92b0d6..d30af1ba 100755
>> --- a/check
>> +++ b/check
>> @@ -840,16 +840,8 @@ 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
> Why remove these lines?

Darrick has asked the same question [1]. Basically I have already added 
init_rc() call to _begin_fstests() which will do the _test_mount() so we 
don't need the above lines, right?


[1] 
https://lore.kernel.org/all/716e0d26-7728-42bb-981d-aae89ef50d7f@gmail.com/

>
>>   	fi
>>   
>> -	init_rc
> Doesn't the "check" need init_rc at here?
Same reason as above.
>
>> -
>>   	seq="check.$$"
>>   	check="$RESULT_BASE/check"
>>   
>> @@ -870,6 +862,8 @@ function run_section()
>>   	needwrap=true
>>   
>>   	if [ ! -z "$SCRATCH_DEV" ]; then
>> +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
>> +		[ $? -le 1 ] || exit 1
>           ^^^^^^^
>           Different indent with below code.
>
> This looks like part of init_rc. If you don't remove above init_rc, can this
> change be saved?
>
>>   	  _scratch_unmount 2> /dev/null
>>   	  # call the overridden mkfs - make sure the FS is built
>>   	  # the same as we'll create it later.
>> 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 d2de8588..f153ad81 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -5754,8 +5754,6 @@ _require_program() {
>>   	_have_program "$1" || _notrun "$tag required"
>>   }
>>   
>> -init_rc
>> -
>>   ################################################################################
>>   # make sure this script returns success
>>   /bin/true
>> diff --git a/soak b/soak
>> index d5c4229a..5734d854 100755
>> --- a/soak
>> +++ b/soak
>> @@ -5,6 +5,7 @@
>>   
>>   # get standard environment, filters and checks
>>   . ./common/rc
>> +# ToDo: Do we need an init_rc() here? How is soak used?
> I never noticed we have this file... this file was create by:
>
>    commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247
>    Author: Nathan Scott <nathans@sgi.com>
>    Date:   Mon Jan 15 05:01:19 2001 +0000
>
>        cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001
>
> I can't understand the relationship of this commit with this file. Does
> anyone learn about the history of it.
>
> I tried to "grep" the whole fstests, looks like nothing uses this file.
> Maybe we should remove it?

Okay. I can see Dave suggesting something similar and has also given a 
sample patch where he is planning to do the same[2].

[2] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/

--NR


>
> Thanks,
> Zorro
>
>>   . ./common/filter
>>   
>>   tmp=/tmp/$$
>> -- 
>> 2.34.1
>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/7/25 03:00, Dave Chinner wrote:
> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>> Silently executing scripts during sourcing common/rc doesn't look good
>> 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>
> FWIW, I've just done somethign similar for check-parallel. I need to
> decouple common/config from common/rc and not run any code from
> either common/config or common/rc.
>
> I've included the patch below (it won't apply because there's all
> sorts of refactoring for test list and config-section parsing in the
> series before it), but it should give you an idea of how I think we
> should be separating one-off initialisation environment varaibles,
> common code inclusion and the repeated initialisation of section
> specific parameters....
Thank you so much. I can a look at this.
>
> .....
>> diff --git a/soak b/soak
>> index d5c4229a..5734d854 100755
>> --- a/soak
>> +++ b/soak
>> @@ -5,6 +5,7 @@
>>   
>>   # get standard environment, filters and checks
>>   . ./common/rc
>> +# ToDo: Do we need an init_rc() here? How is soak used?
>>   . ./common/filter
> I've also go a patch series that removes all these old 2000-era SGI
> QE scripts that have not been used by anyone for the last 15
> years. I did that to get rid of the technical debt that these
> scripts have gathered over years of neglect. They aren't used, we
> shouldn't even attempt to maintain them anymore.

Okay. What do you mean by SGI QE script (sorry, not familiar with this)? 
Do you mean some kind of CI/automation-test script?

--NR

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

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Darrick J. Wong]

On Fri, Mar 07, 2025 at 11:21:15AM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 3/6/25 23:16, Darrick J. Wong wrote:
> > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> > > Silently executing scripts during sourcing common/rc doesn't look good
> > > 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           | 10 ++--------
> > >   common/preamble |  1 +
> > >   common/rc       |  2 --
> > >   soak            |  1 +
> > >   4 files changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/check b/check
> > > index ea92b0d6..d30af1ba 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -840,16 +840,8 @@ 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
> > Unrelated change?  I was expecting a mechanical ". ./common/rc" =>
> > ". ./common/rc ; init_rc" change in this patch.
> This patch adds an init_rc() call to _begin_fstests() in common/preamble and
> hence the above _test_mount() will be executed during that call. So this
> _test_mount isn't necessary here, right? _test_mount() will be executed (as
> a part of init_rc() call) before every test run. Please let me know if my
> understanding isn't correct.

It's true that in terms of getting the test filesystem mounted, the
_test_mount here and in init_rc are redundant.  But look at what happens
on error here -- we print "check: failed to mount..." to signal that the
new section's TEST_FS_MOUNT_OPTS are not valid, and exit the ./check
process.

By deferring the mount to the init_rc in _preamble, that means that
we'll run the whole section with bad mount options, most likely
resulting in every test spewing "common/rc: could not mount..." and
appearing to fail.

I think.  I'm not sure what "status=1; exit" does as compared to
"exit 1"; AFAICT the former actually results in an exit code of 0
because the (otherwise pointless) assignment succeeds.

Granted, the init_rc that you remove below would also catch that case
and exit ./check

> > >   	fi
> > > -	init_rc
> > Why remove init_rc here?
> Same reason as above.

But that's an additional change in behavior.  If there's no reason for
calling init_rc() from run_section() then that should be a separate
patch with a separate justification.

--D

> > 
> > > -
> > >   	seq="check.$$"
> > >   	check="$RESULT_BASE/check"
> > > @@ -870,6 +862,8 @@ function run_section()
> > >   	needwrap=true
> > >   	if [ ! -z "$SCRATCH_DEV" ]; then
> > > +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
> > > +		[ $? -le 1 ] || exit 1
> > >   	  _scratch_unmount 2> /dev/null
> > >   	  # call the overridden mkfs - make sure the FS is built
> > >   	  # the same as we'll create it later.
> > > 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 d2de8588..f153ad81 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -5754,8 +5754,6 @@ _require_program() {
> > >   	_have_program "$1" || _notrun "$tag required"
> > >   }
> > > -init_rc
> > > -
> > >   ################################################################################
> > >   # make sure this script returns success
> > >   /bin/true
> > > diff --git a/soak b/soak
> > > index d5c4229a..5734d854 100755
> > > --- a/soak
> > > +++ b/soak
> > > @@ -5,6 +5,7 @@
> > >   # get standard environment, filters and checks
> > >   . ./common/rc
> > > +# ToDo: Do we need an init_rc() here? How is soak used?
> > I have no idea what soak does and have never used it, but I think for
> > continuity's sake you should call init_rc here.
> 
> Okay. I think Dave has suggested removing this file[1]. This doesn't seem to
> used anymore.
> 
> [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/
> 
> --NR
> 
> > 
> > --D
> > 
> > >   . ./common/filter
> > >   tmp=/tmp/$$
> > > -- 
> > > 2.34.1
> > > 
> > > 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
> 
> 

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Zorro Lang]

On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 3/7/25 03:00, Dave Chinner wrote:
> > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> > > Silently executing scripts during sourcing common/rc doesn't look good
> > > 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>
> > FWIW, I've just done somethign similar for check-parallel. I need to
> > decouple common/config from common/rc and not run any code from
> > either common/config or common/rc.
> > 
> > I've included the patch below (it won't apply because there's all
> > sorts of refactoring for test list and config-section parsing in the
> > series before it), but it should give you an idea of how I think we
> > should be separating one-off initialisation environment varaibles,
> > common code inclusion and the repeated initialisation of section
> > specific parameters....
> Thank you so much. I can a look at this.
> > 
> > .....
> > > diff --git a/soak b/soak
> > > index d5c4229a..5734d854 100755
> > > --- a/soak
> > > +++ b/soak
> > > @@ -5,6 +5,7 @@
> > >   # get standard environment, filters and checks
> > >   . ./common/rc
> > > +# ToDo: Do we need an init_rc() here? How is soak used?
> > >   . ./common/filter
> > I've also go a patch series that removes all these old 2000-era SGI
> > QE scripts that have not been used by anyone for the last 15
> > years. I did that to get rid of the technical debt that these
> > scripts have gathered over years of neglect. They aren't used, we
> > shouldn't even attempt to maintain them anymore.
> 
> Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do
> you mean some kind of CI/automation-test script?

SGI is Silicon Graphics International Corp. :
https://en.wikipedia.org/wiki/Silicon_Graphics_International

xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX)
of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert
of all these things, and knows lots of past details :)

Thanks,
Zorro

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

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Zorro Lang]

On Sat, Mar 08, 2025 at 03:20:34PM +0800, Zorro Lang wrote:
> On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote:
> > 
> > On 3/7/25 03:00, Dave Chinner wrote:
> > > On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
> > > > Silently executing scripts during sourcing common/rc doesn't look good
> > > > 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>
> > > FWIW, I've just done somethign similar for check-parallel. I need to
> > > decouple common/config from common/rc and not run any code from
> > > either common/config or common/rc.
> > > 
> > > I've included the patch below (it won't apply because there's all
> > > sorts of refactoring for test list and config-section parsing in the
> > > series before it), but it should give you an idea of how I think we
> > > should be separating one-off initialisation environment varaibles,
> > > common code inclusion and the repeated initialisation of section
> > > specific parameters....
> > Thank you so much. I can a look at this.
> > > 
> > > .....
> > > > diff --git a/soak b/soak
> > > > index d5c4229a..5734d854 100755
> > > > --- a/soak
> > > > +++ b/soak
> > > > @@ -5,6 +5,7 @@
> > > >   # get standard environment, filters and checks
> > > >   . ./common/rc
> > > > +# ToDo: Do we need an init_rc() here? How is soak used?
> > > >   . ./common/filter
> > > I've also go a patch series that removes all these old 2000-era SGI
> > > QE scripts that have not been used by anyone for the last 15
> > > years. I did that to get rid of the technical debt that these
> > > scripts have gathered over years of neglect. They aren't used, we
> > > shouldn't even attempt to maintain them anymore.
> > 
> > Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do
> > you mean some kind of CI/automation-test script?
> 
> SGI is Silicon Graphics International Corp. :
> https://en.wikipedia.org/wiki/Silicon_Graphics_International
> 
> xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX)
> of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert
> of all these things, and knows lots of past details :)

Hi Nirjhar,

I've merged Dave's "[PATCH 0/5] fstests: remove old SGI QE scripts" into
patches-in-queue branch. You can base on that to write your V2, to avoid
dealing with the "soak" file.

Thanks,
Zorro

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

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/10/25 13:36, Zorro Lang wrote:
> On Sat, Mar 08, 2025 at 03:20:34PM +0800, Zorro Lang wrote:
>> On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote:
>>> On 3/7/25 03:00, Dave Chinner wrote:
>>>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>>>>> Silently executing scripts during sourcing common/rc doesn't look good
>>>>> 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>
>>>> FWIW, I've just done somethign similar for check-parallel. I need to
>>>> decouple common/config from common/rc and not run any code from
>>>> either common/config or common/rc.
>>>>
>>>> I've included the patch below (it won't apply because there's all
>>>> sorts of refactoring for test list and config-section parsing in the
>>>> series before it), but it should give you an idea of how I think we
>>>> should be separating one-off initialisation environment varaibles,
>>>> common code inclusion and the repeated initialisation of section
>>>> specific parameters....
>>> Thank you so much. I can a look at this.
>>>> .....
>>>>> diff --git a/soak b/soak
>>>>> index d5c4229a..5734d854 100755
>>>>> --- a/soak
>>>>> +++ b/soak
>>>>> @@ -5,6 +5,7 @@
>>>>>    # get standard environment, filters and checks
>>>>>    . ./common/rc
>>>>> +# ToDo: Do we need an init_rc() here? How is soak used?
>>>>>    . ./common/filter
>>>> I've also go a patch series that removes all these old 2000-era SGI
>>>> QE scripts that have not been used by anyone for the last 15
>>>> years. I did that to get rid of the technical debt that these
>>>> scripts have gathered over years of neglect. They aren't used, we
>>>> shouldn't even attempt to maintain them anymore.
>>> Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do
>>> you mean some kind of CI/automation-test script?
>> SGI is Silicon Graphics International Corp. :
>> https://en.wikipedia.org/wiki/Silicon_Graphics_International
>>
>> xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX)
>> of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert
>> of all these things, and knows lots of past details :)
> Hi Nirjhar,
>
> I've merged Dave's "[PATCH 0/5] fstests: remove old SGI QE scripts" into
> patches-in-queue branch. You can base on that to write your V2, to avoid
> dealing with the "soak" file.
>
> Thanks,
> Zorro

Okay, thank you for the pointer. I will send the v2 after rebasing.

--NR

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

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/8/25 12:50, Zorro Lang wrote:
> On Fri, Mar 07, 2025 at 01:35:02PM +0530, Nirjhar Roy (IBM) wrote:
>> On 3/7/25 03:00, Dave Chinner wrote:
>>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>>>> Silently executing scripts during sourcing common/rc doesn't look good
>>>> 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>
>>> FWIW, I've just done somethign similar for check-parallel. I need to
>>> decouple common/config from common/rc and not run any code from
>>> either common/config or common/rc.
>>>
>>> I've included the patch below (it won't apply because there's all
>>> sorts of refactoring for test list and config-section parsing in the
>>> series before it), but it should give you an idea of how I think we
>>> should be separating one-off initialisation environment varaibles,
>>> common code inclusion and the repeated initialisation of section
>>> specific parameters....
>> Thank you so much. I can a look at this.
>>> .....
>>>> diff --git a/soak b/soak
>>>> index d5c4229a..5734d854 100755
>>>> --- a/soak
>>>> +++ b/soak
>>>> @@ -5,6 +5,7 @@
>>>>    # get standard environment, filters and checks
>>>>    . ./common/rc
>>>> +# ToDo: Do we need an init_rc() here? How is soak used?
>>>>    . ./common/filter
>>> I've also go a patch series that removes all these old 2000-era SGI
>>> QE scripts that have not been used by anyone for the last 15
>>> years. I did that to get rid of the technical debt that these
>>> scripts have gathered over years of neglect. They aren't used, we
>>> shouldn't even attempt to maintain them anymore.
>> Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do
>> you mean some kind of CI/automation-test script?
> SGI is Silicon Graphics International Corp. :
> https://en.wikipedia.org/wiki/Silicon_Graphics_International
>
> xfstests was created to test xfs on IRIX (https://en.wikipedia.org/wiki/IRIX)
> of SGI. Dave Chinner worked in SGI company long time ago, so he's the expert
> of all these things, and knows lots of past details :)
>
> Thanks,
> Zorro

Okay, got it. Thank you.

--NR

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

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/7/25 23:10, Darrick J. Wong wrote:
> On Fri, Mar 07, 2025 at 11:21:15AM +0530, Nirjhar Roy (IBM) wrote:
>> On 3/6/25 23:16, Darrick J. Wong wrote:
>>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>>>> Silently executing scripts during sourcing common/rc doesn't look good
>>>> 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           | 10 ++--------
>>>>    common/preamble |  1 +
>>>>    common/rc       |  2 --
>>>>    soak            |  1 +
>>>>    4 files changed, 4 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/check b/check
>>>> index ea92b0d6..d30af1ba 100755
>>>> --- a/check
>>>> +++ b/check
>>>> @@ -840,16 +840,8 @@ 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
>>> Unrelated change?  I was expecting a mechanical ". ./common/rc" =>
>>> ". ./common/rc ; init_rc" change in this patch.
>> This patch adds an init_rc() call to _begin_fstests() in common/preamble and
>> hence the above _test_mount() will be executed during that call. So this
>> _test_mount isn't necessary here, right? _test_mount() will be executed (as
>> a part of init_rc() call) before every test run. Please let me know if my
>> understanding isn't correct.
> It's true that in terms of getting the test filesystem mounted, the
> _test_mount here and in init_rc are redundant.  But look at what happens
> on error here -- we print "check: failed to mount..." to signal that the
> new section's TEST_FS_MOUNT_OPTS are not valid, and exit the ./check
> process.
>
> By deferring the mount to the init_rc in _preamble, that means that
> we'll run the whole section with bad mount options, most likely
> resulting in every test spewing "common/rc: could not mount..." and
> appearing to fail.
Aah, right. The exit should be at the check level if some parameter is 
not correct in a section. I will make the change in v2.
>
> I think.  I'm not sure what "status=1; exit" does as compared to
> "exit 1"; AFAICT the former actually results in an exit code of 0
> because the (otherwise pointless) assignment succeeds.

I think "status=0; exit" has a reason. 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".

However, when I executed 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
TEST_FS_MOUNT_OPTS="-o invalidss"

This caused the "elif [ "$OLD_TEST_FS_MOUNT_OPTS" != 
"$TEST_FS_MOUNT_OPTS" ]; then" to be executed. Now, when I checked the 
value of "echo $?", it showed 1. IMO, this is the correct behavior, and 
we should always use "status=<value>; exit" and NOT "exit 1" directly. 
Even if 1 section fails,   "./check <test-list>" command should return a 
non-zero value. Can you please let me know if my understanding is 
correct? If yes, maybe we can have a function like

_set_status_and_exit()
{

     status="$1"
     exit
}

and replace all the "status <value>; exit" and "exit <value>" with 
"_set_status_and_exit <value>"

--NR


>
> Granted, the init_rc that you remove below would also catch that case
> and exit ./check
Yes. init_rc can catch that case with an additional difference that it 
will attempt another mount "retrying test device mount with external set"
>
>>>>    	fi
>>>> -	init_rc
>>> Why remove init_rc here?
>> Same reason as above.
> But that's an additional change in behavior.  If there's no reason for
> calling init_rc() from run_section() then that should be a separate
> patch with a separate justification.

Since the check for _test_mount should be at the check script level and 
not at the _begin_fstest(), maybe we should

1. Keep the init_rc call here

2. Remove the _test_mount above (the one with "elif [ 
"$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then" ) and have a 
separate patch for it with proper justification.

3. NOT have any init_rc call in _begin_fstest(), since the _test_mount 
related checks would already been done by the time _begin_fstests() gets 
executed.

The above changes will also not change any existing behavior. Can you 
please let me know your thoughts and I can send a V2 accordingly?

--NR

>
> --D
>
>>>> -
>>>>    	seq="check.$$"
>>>>    	check="$RESULT_BASE/check"
>>>> @@ -870,6 +862,8 @@ function run_section()
>>>>    	needwrap=true
>>>>    	if [ ! -z "$SCRATCH_DEV" ]; then
>>>> +		_check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT
>>>> +		[ $? -le 1 ] || exit 1
>>>>    	  _scratch_unmount 2> /dev/null
>>>>    	  # call the overridden mkfs - make sure the FS is built
>>>>    	  # the same as we'll create it later.
>>>> 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 d2de8588..f153ad81 100644
>>>> --- a/common/rc
>>>> +++ b/common/rc
>>>> @@ -5754,8 +5754,6 @@ _require_program() {
>>>>    	_have_program "$1" || _notrun "$tag required"
>>>>    }
>>>> -init_rc
>>>> -
>>>>    ################################################################################
>>>>    # make sure this script returns success
>>>>    /bin/true
>>>> diff --git a/soak b/soak
>>>> index d5c4229a..5734d854 100755
>>>> --- a/soak
>>>> +++ b/soak
>>>> @@ -5,6 +5,7 @@
>>>>    # get standard environment, filters and checks
>>>>    . ./common/rc
>>>> +# ToDo: Do we need an init_rc() here? How is soak used?
>>> I have no idea what soak does and have never used it, but I think for
>>> continuity's sake you should call init_rc here.
>> Okay. I think Dave has suggested removing this file[1]. This doesn't seem to
>> used anymore.
>>
>> [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/
>>
>> --NR
>>
>>> --D
>>>
>>>>    . ./common/filter
>>>>    tmp=/tmp/$$
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>> -- 
>> Nirjhar Roy
>> Linux Kernel Developer
>> IBM, Bangalore
>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling init_rc() call from sourcing common/rc

[Nirjhar Roy (IBM)]

On 3/12/25 11:16, Nirjhar Roy (IBM) wrote:
>
> On 3/7/25 23:10, Darrick J. Wong wrote:
>> On Fri, Mar 07, 2025 at 11:21:15AM +0530, Nirjhar Roy (IBM) wrote:
>>> On 3/6/25 23:16, Darrick J. Wong wrote:
>>>> On Thu, Mar 06, 2025 at 08:17:41AM +0000, Nirjhar Roy (IBM) wrote:
>>>>> Silently executing scripts during sourcing common/rc doesn't look 
>>>>> good
>>>>> 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           | 10 ++--------
>>>>>    common/preamble |  1 +
>>>>>    common/rc       |  2 --
>>>>>    soak            |  1 +
>>>>>    4 files changed, 4 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/check b/check
>>>>> index ea92b0d6..d30af1ba 100755
>>>>> --- a/check
>>>>> +++ b/check
>>>>> @@ -840,16 +840,8 @@ 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
>>>> Unrelated change?  I was expecting a mechanical ". ./common/rc" =>
>>>> ". ./common/rc ; init_rc" change in this patch.
>>> This patch adds an init_rc() call to _begin_fstests() in 
>>> common/preamble and
>>> hence the above _test_mount() will be executed during that call. So 
>>> this
>>> _test_mount isn't necessary here, right? _test_mount() will be 
>>> executed (as
>>> a part of init_rc() call) before every test run. Please let me know 
>>> if my
>>> understanding isn't correct.
>> It's true that in terms of getting the test filesystem mounted, the
>> _test_mount here and in init_rc are redundant.  But look at what happens
>> on error here -- we print "check: failed to mount..." to signal that the
>> new section's TEST_FS_MOUNT_OPTS are not valid, and exit the ./check
>> process.
>>
>> By deferring the mount to the init_rc in _preamble, that means that
>> we'll run the whole section with bad mount options, most likely
>> resulting in every test spewing "common/rc: could not mount..." and
>> appearing to fail.
> Aah, right. The exit should be at the check level if some parameter is 
> not correct in a section. I will make the change in v2.
>>
>> I think.  I'm not sure what "status=1; exit" does as compared to
>> "exit 1"; AFAICT the former actually results in an exit code of 0
>> because the (otherwise pointless) assignment succeeds.
>
> I think "status=0; exit" has a reason. 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".
>
> However, when I executed 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
> TEST_FS_MOUNT_OPTS="-o invalidss"
>
> This caused the "elif [ "$OLD_TEST_FS_MOUNT_OPTS" != 
> "$TEST_FS_MOUNT_OPTS" ]; then" to be executed. Now, when I checked the 
> value of "echo $?", it showed 1. IMO, this is the correct behavior, 
> and we should always use "status=<value>; exit" and NOT "exit 1" 
> directly. Even if 1 section fails,   "./check <test-list>" command 
> should return a non-zero value. Can you please let me know if my 
> understanding is correct? If yes, maybe we can have a function like
>
> _set_status_and_exit()
> {
>
>     status="$1"
>     exit
> }
>
> and replace all the "status <value>; exit" and "exit <value>" with 
> "_set_status_and_exit <value>"

Hi Darrick, any feedback on the above?

--NR

>
> --NR
>
>
>>
>> Granted, the init_rc that you remove below would also catch that case
>> and exit ./check
> Yes. init_rc can catch that case with an additional difference that it 
> will attempt another mount "retrying test device mount with external set"
>>
>>>>>        fi
>>>>> -    init_rc
>>>> Why remove init_rc here?
>>> Same reason as above.
>> But that's an additional change in behavior.  If there's no reason for
>> calling init_rc() from run_section() then that should be a separate
>> patch with a separate justification.
>
> Since the check for _test_mount should be at the check script level 
> and not at the _begin_fstest(), maybe we should
>
> 1. Keep the init_rc call here
>
> 2. Remove the _test_mount above (the one with "elif [ 
> "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then" ) and have 
> a separate patch for it with proper justification.
>
> 3. NOT have any init_rc call in _begin_fstest(), since the _test_mount 
> related checks would already been done by the time _begin_fstests() 
> gets executed.
>
> The above changes will also not change any existing behavior. Can you 
> please let me know your thoughts and I can send a V2 accordingly?
>
> --NR
>
>>
>> --D
>>
>>>>> -
>>>>>        seq="check.$$"
>>>>>        check="$RESULT_BASE/check"
>>>>> @@ -870,6 +862,8 @@ function run_section()
>>>>>        needwrap=true
>>>>>        if [ ! -z "$SCRATCH_DEV" ]; then
>>>>> +        _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT 
>>>>> $SCRATCH_MNT
>>>>> +        [ $? -le 1 ] || exit 1
>>>>>          _scratch_unmount 2> /dev/null
>>>>>          # call the overridden mkfs - make sure the FS is built
>>>>>          # the same as we'll create it later.
>>>>> 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 d2de8588..f153ad81 100644
>>>>> --- a/common/rc
>>>>> +++ b/common/rc
>>>>> @@ -5754,8 +5754,6 @@ _require_program() {
>>>>>        _have_program "$1" || _notrun "$tag required"
>>>>>    }
>>>>> -init_rc
>>>>> -
>>>>> ################################################################################
>>>>>    # make sure this script returns success
>>>>>    /bin/true
>>>>> diff --git a/soak b/soak
>>>>> index d5c4229a..5734d854 100755
>>>>> --- a/soak
>>>>> +++ b/soak
>>>>> @@ -5,6 +5,7 @@
>>>>>    # get standard environment, filters and checks
>>>>>    . ./common/rc
>>>>> +# ToDo: Do we need an init_rc() here? How is soak used?
>>>> I have no idea what soak does and have never used it, but I think for
>>>> continuity's sake you should call init_rc here.
>>> Okay. I think Dave has suggested removing this file[1]. This doesn't 
>>> seem to
>>> used anymore.
>>>
>>> [1] https://lore.kernel.org/all/Z8oT_tBYG-a79CjA@dread.disaster.area/
>>>
>>> --NR
>>>
>>>> --D
>>>>
>>>>>    . ./common/filter
>>>>>    tmp=/tmp/$$
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>>
>>> -- 
>>> Nirjhar Roy
>>> Linux Kernel Developer
>>> IBM, Bangalore
>>>
>>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore