Message-ID: <Z8oT_tBYG-a79CjA@dread.disaster.area>
Date: Fri, 7 Mar 2025 08:30:38 +1100
From: Dave Chinner <david@...morbit.com>
To: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
	linux-xfs@...r.kernel.org, ritesh.list@...il.com,
	ojaswin@...ux.ibm.com, djwong@...nel.org, zlang@...nel.org
Subject: Re: [PATCH v1 2/2] check,common/{preamble,rc},soak: Decoupling
 init_rc() call from sourcing common/rc

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@...il.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@...morbit.com


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

From: Dave Chinner <dchinner@...hat.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@...hat.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

Powered by blists - more mailing lists