Minor cleanups in common/rc

PATCH Superseded Nirjhar Roy (IBM) 4 participants 17 messages Mar 6, 2025 – Mar 18, 2025

Share & tools

Nirjhar Roy (IBM)

author

2025-03-06 08:18 UTC #0 lore raw

cover Minor cleanups in common/rc

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

Nirjhar Roy (IBM)

author

2025-03-06 08:19 UTC #1 lore raw

PATCH 1/2 generic/749: Remove redundant sourcing of common/rc

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

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

---
 tests/generic/749 | 1 -
 1 file changed, 1 deletion(-)

tests/generic/749 +0 -1

index fc747738..451f283e 100755
@@ -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.

Signature · 1 line

2.34.1

Darrick J. Wong

Reviewed-by on v1 1/2

2025-03-06 17:41 UTC #2 lore raw

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

Quoted 4 lines · Nirjhar Roy (IBM)

common/rc is already sourced before the test starts running in _begin_fstest() preamble.
Signed-off-by Nirjhar Roy (IBM) <[email protected]>

Yeah, that sourceing shouldn't be necessary.

Reviewed-by "Darrick J. Wong" <[email protected]>

--D

Quoted 20 lines

---
 tests/generic/749 | 1 -
 1 file changed, 1 deletion(-)

tests/generic/749 +0 -1

index fc747738..451f283e 100755
@@ -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.

Signature · 1 line

2.34.1

Nirjhar Roy (IBM)

author

2025-03-06 08:19 UTC #3 lore raw

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

In reply to 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) <[email protected]>

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

check +2 -8

index ea92b0d6..d30af1ba 100755
@@ -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.

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
-
 ################################################################################
 # make sure this script returns success
 /bin/true

soak +1 -0

index d5c4229a..5734d854 100755
@@ -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/$$

Signature · 1 line

2.34.1

Darrick J. Wong

2025-03-06 17:46 UTC #4 lore raw

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

Quoted 17 lines · 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) <[email protected]>
---
 check           | 10 ++--------
 common/preamble |  1 +
 common/rc       |  2 --
 soak            |  1 +
 4 files changed, 4 insertions(+), 10 deletions(-)
check +0 -0
index ea92b0d6..d30af1ba 100755

check function run_section() · line 848

Quoted 9 lines

check +0 -6

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

check function run_section() · line 851

Quoted 3 lines

check +0 -1

 	fi
 
-	init_rc

Why remove init_rc here?

Quoted 43 lines

check +2 -1

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

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
-
 ################################################################################
 # make sure this script returns success
 /bin/true

soak +0 -0

index d5c4229a..5734d854 100755

soak line 8

Quoted 4 lines

soak +1 -0

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

Quoted 7 lines

soak +0 -0
 . ./common/filter
 
 tmp=/tmp/$$
Signature · 1 line
2.34.1

Nirjhar Roy (IBM)

author

2025-03-07 05:51 UTC #5 lore raw

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

Quoted 13 lines · Darrick J. Wong

Quoted 12 lines · 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) <[email protected]>
---
  check           | 10 ++--------
  common/preamble |  1 +
  common/rc       |  2 --
  soak            |  1 +
  4 files changed, 4 insertions(+), 10 deletions(-)

Quoted 16 lines

Quoted 5 lines
check +0 -0
index ea92b0d6..d30af1ba 100755
check function run_section() · line 848
Quoted 9 lines
check +0 -6
@@ -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.

Quoted 4 lines

check function run_section() · line 851
Quoted 3 lines
check +0 -1
  	fi
 
-	init_rc
Why remove init_rc here?

Same reason as above.

Quoted 45 lines

check +2 -1

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

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
  	_have_program "$1" || _notrun "$tag required"
  }
 
-init_rc
-
  ################################################################################
  # make sure this script returns success
  /bin/true

soak +0 -0

index d5c4229a..5734d854 100755
@@ -5,6 +5,7 @@

Quoted 5 lines

soak line 8
Quoted 3 lines
soak +1 -0
  # 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/[email protected]/

--NR

Quoted 10 lines

--D
Quoted 7 lines
soak +0 -0
  . ./common/filter
 
  tmp=/tmp/$$
Signature · 1 line
2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Darrick J. Wong

2025-03-07 17:40 UTC #6 lore raw

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

Quoted 31 lines · Nirjhar Roy (IBM)

Quoted 13 lines · Darrick J. Wong
Quoted 12 lines · 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) <[email protected]>
---
  check           | 10 ++--------
  common/preamble |  1 +
  common/rc       |  2 --
  soak            |  1 +
  4 files changed, 4 insertions(+), 10 deletions(-)
Quoted 16 lines
Quoted 5 lines
check +0 -0
index ea92b0d6..d30af1ba 100755
check function run_section() · line 848
Quoted 9 lines
check +0 -6
@@ -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.

Quoted 5 lines

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

Quoted 4 lines

Quoted 3 lines
check function run_section() · line 851
Quoted 2 lines
check +0 -1
  	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

Quoted 67 lines

Quoted 36 lines

check +2 -1

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

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
  	_have_program "$1" || _notrun "$tag required"
  }
-init_rc
-
  ################################################################################
  # make sure this script returns success
  /bin/true

soak +0 -0

index d5c4229a..5734d854 100755

Quoted 8 lines

soak line 7
Quoted 6 lines
soak +1 -0
@@ -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/[email protected]/

--NR

Quoted 9 lines

--D
Quoted 6 lines
soak +0 -0
  . ./common/filter
  tmp=/tmp/$$
Signature · 1 line
2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Nirjhar Roy (IBM)

author

2025-03-12 05:46 UTC #7 lore raw

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

Quoted 42 lines · Darrick J. Wong

Quoted 30 lines · Nirjhar Roy (IBM)
Quoted 13 lines · Darrick J. Wong
Quoted 12 lines · 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) <[email protected]>
---
   check           | 10 ++--------
   common/preamble |  1 +
   common/rc       |  2 --
   soak            |  1 +
   4 files changed, 4 insertions(+), 10 deletions(-)
Quoted 16 lines
Quoted 5 lines
check +0 -0
index ea92b0d6..d30af1ba 100755
check function run_section() · line 848
Quoted 9 lines
check +0 -6
@@ -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.
Quoted 5 lines
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.

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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"

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
check function run_section() · line 851
Quoted 3 lines
check +0 -1
   	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

Quoted 67 lines

--D

Quoted 64 lines

Quoted 35 lines

check +2 -1

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

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
   	_have_program "$1" || _notrun "$tag required"
   }
-init_rc
-
   ################################################################################
   # make sure this script returns success
   /bin/true

soak +0 -0

index d5c4229a..5734d854 100755

Quoted 8 lines

soak line 7
Quoted 6 lines
soak +1 -0
@@ -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/[email protected]/

--NR

Quoted 8 lines

--D
Quoted 6 lines
soak +0 -0
   . ./common/filter
   tmp=/tmp/$$
Signature · 1 line
2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Nirjhar Roy (IBM)

author

2025-03-18 04:57 UTC #8 lore raw

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

Quoted 99 lines · Nirjhar Roy (IBM)

Quoted 42 lines · Darrick J. Wong
Quoted 30 lines · Nirjhar Roy (IBM)
Quoted 13 lines · Darrick J. Wong
Quoted 12 lines · 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) <[email protected]>
---
   check           | 10 ++--------
   common/preamble | 1 +
   common/rc       | 2 --
   soak            | 1 +
   4 files changed, 4 insertions(+), 10 deletions(-)
Quoted 16 lines
Quoted 6 lines
check +0 -0
index ea92b0d6..d30af1ba 100755
@@ -840,16 +840,8 @@ function run_section()
_prepare_test_list
Quoted 8 lines
       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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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

Quoted 8 lines

_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

Quoted 97 lines

--NR
Quoted 3 lines
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"
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
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
Quoted 68 lines
--D
Quoted 65 lines
Quoted 36 lines
Quoted 36 lines
-
seq="check.$$"
check="$RESULT_BASE/check"
check +0 -0
@@ -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.
common/preamble +0 -0
index 0c9ee2e0..c92e55bb 100644
@@ -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
common/rc +0 -0
index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
_have_program "$1" || _notrun "$tag required" }
common/rc +0 -2
-init_rc
-
################################################################################
   # make sure this script returns success
   /bin/true
soak +0 -0
index d5c4229a..5734d854 100755
Quoted 8 lines
Quoted 6 lines
soak +0 -0
@@ -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/[email protected]/
--NR
Quoted 8 lines
--D
Quoted 6 lines
. ./common/filter
tmp=/tmp/$$
Signature · 1 line
2.34.1
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Zorro Lang

2025-03-06 21:14 UTC #9 lore raw

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

In reply to Nirjhar Roy (IBM)

Quoted 17 lines · 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) <[email protected]>
---
 check           | 10 ++--------
 common/preamble |  1 +
 common/rc       |  2 --
 soak            |  1 +
 4 files changed, 4 insertions(+), 10 deletions(-)
check +0 -0
index ea92b0d6..d30af1ba 100755

check function run_section() · line 848

Quoted 9 lines

check +0 -6

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

check function run_section() · line 851

Quoted 3 lines

check +0 -1

 	fi
 
-	init_rc

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

check function run_section() · line 866

Quoted 10 lines

check +2 -1

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

Quoted 33 lines

check +0 -0

 	  _scratch_unmount 2> /dev/null
 	  # call the overridden mkfs - make sure the FS is built
 	  # the same as we'll create it later.

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
 	_have_program "$1" || _notrun "$tag required"
 }
 
-init_rc
-
 ################################################################################
 # make sure this script returns success
 /bin/true

soak +0 -0

index d5c4229a..5734d854 100755

soak line 8

Quoted 4 lines

soak +1 -0

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

Quoted 7 lines

soak +0 -0
 . ./common/filter
 
 tmp=/tmp/$$
Signature · 1 line
2.34.1

Nirjhar Roy (IBM)

author

2025-03-07 05:57 UTC #10 lore raw

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

Quoted 13 lines · Zorro Lang

Quoted 12 lines · 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) <[email protected]>
---
  check           | 10 ++--------
  common/preamble |  1 +
  common/rc       |  2 --
  soak            |  1 +
  4 files changed, 4 insertions(+), 10 deletions(-)

Quoted 15 lines

Quoted 5 lines

check +0 -0

index ea92b0d6..d30af1ba 100755

check function run_section() · line 848

Quoted 9 lines

check +0 -6

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

Quoted 5 lines

check function run_section() · line 851
Quoted 4 lines
check +0 -1
  	fi
 
-	init_rc
Doesn't the "check" need init_rc at here?

Same reason as above.

Quoted 62 lines

check function run_section() · line 866

Quoted 11 lines

check +2 -1

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

Quoted 33 lines

check +0 -0

  	  _scratch_unmount 2> /dev/null
  	  # call the overridden mkfs - make sure the FS is built
  	  # the same as we'll create it later.

common/preamble +1 -0

index 0c9ee2e0..c92e55bb 100644
@@ -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

common/rc +0 -2

index d2de8588..f153ad81 100644
@@ -5754,8 +5754,6 @@ _require_program() {
  	_have_program "$1" || _notrun "$tag required"
  }
 
-init_rc
-
  ################################################################################
  # make sure this script returns success
  /bin/true

soak +0 -0

index d5c4229a..5734d854 100755

soak line 8

Quoted 4 lines

soak +1 -0

@@ -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 <[email protected]>
   Date:   Mon Jan 15 05:01:19 2001 +0000

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

Quoted 5 lines

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

--NR

Quoted 11 lines

Thanks,
Zorro
Quoted 7 lines
soak +0 -0
  . ./common/filter
 
  tmp=/tmp/$$
Signature · 1 line
2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Dave Chinner

2025-03-06 21:30 UTC #11 lore raw

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

In reply to Nirjhar Roy (IBM)

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

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

.....

soak line 9

Quoted 10 lines

soak +1 -0

index d5c4229a..5734d854 100755
@@ -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.

Signature · 48 lines

Dave Chinner
[email protected]


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

From: Dave Chinner <[email protected]>

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

check +13 ~~-11~~

index b4d31d138..b968a134a 100755
@@ -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

common/config +0 ~~-218~~

227 lines not on this page · raw message

common/preamble +1 -0

index 0c9ee2e03..1f40dd5d1 100644
@@ -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

common/rc +217 ~~-17~~

264 lines not on this page · raw message

tests/generic/367 +3 -4

index ed371a02b..1c9c66730 100755
@@ -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"

tests/generic/749 +1 -2

index fc7477380..aac8da20d 100755
@@ -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

Nirjhar Roy (IBM)

author

2025-03-07 08:05 UTC #12 lore raw

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

Quoted 10 lines · Dave Chinner

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

Quoted 6 lines

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.

Quoted 8 lines

.....
Quoted 6 lines
soak +0 -0
index d5c4229a..5734d854 100755
@@ -5,6 +5,7 @@
 

Quoted 9 lines

soak line 9
Quoted 4 lines
soak +1 -0
  # 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

Quoted 3 lines

-Dave.

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Zorro Lang

2025-03-08 07:20 UTC #13 lore raw

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

Quoted 36 lines · Nirjhar Roy (IBM)

Quoted 10 lines · Dave Chinner
Quoted 5 lines · 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) <[email protected]>
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.
Quoted 6 lines
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.
Quoted 16 lines
.....
soak line 8
Quoted 9 lines
soak +1 -0
index d5c4229a..5734d854 100755
@@ -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.

Quoted 2 lines

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

Quoted 12 lines

--NR
Quoted 3 lines
-Dave.
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Zorro Lang

2025-03-10 08:06 UTC #14 lore raw

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

Quoted 40 lines · Zorro Lang

Quoted 36 lines · Nirjhar Roy (IBM)
Quoted 10 lines · Dave Chinner
Quoted 5 lines · 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) <[email protected]>
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.
Quoted 6 lines
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.
Quoted 16 lines
.....
soak line 8
Quoted 9 lines
soak +1 -0
index d5c4229a..5734d854 100755
@@ -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.
Quoted 2 lines
Okay. What do you mean by SGI QE script (sorry, not familiar with this)? Do you mean some kind of CI/automation-test script?

Quoted 6 lines

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

Quoted 16 lines

Thanks,
Zorro
Quoted 12 lines
--NR
Quoted 3 lines
-Dave.
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Nirjhar Roy (IBM)

author

2025-03-12 04:41 UTC #15 lore raw

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

Quoted 43 lines · Zorro Lang

Quoted 34 lines · Zorro Lang
Quoted 33 lines · Nirjhar Roy (IBM)
Quoted 10 lines · Dave Chinner
Quoted 5 lines · 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) <[email protected]>
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.
Quoted 6 lines
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.
Quoted 15 lines
.....
soak line 8
Quoted 9 lines
soak +1 -0
index d5c4229a..5734d854 100755
@@ -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.
Quoted 8 lines
Quoted 2 lines
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 :)

Quoted 8 lines

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

Quoted 14 lines

Quoted 14 lines
Thanks,
Zorro
Quoted 10 lines
--NR
Quoted 2 lines
-Dave.
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Nirjhar Roy (IBM)

author

2025-03-12 04:42 UTC #16 lore raw

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

In reply to Zorro Lang

Quoted 39 lines · Zorro Lang

Quoted 33 lines · Nirjhar Roy (IBM)
Quoted 10 lines · Dave Chinner
Quoted 5 lines · 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) <[email protected]>
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.
Quoted 6 lines
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.
Quoted 15 lines
.....
soak line 8
Quoted 9 lines
soak +1 -0
index d5c4229a..5734d854 100755
@@ -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.
Quoted 2 lines
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

Quoted 6 lines

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

Quoted 11 lines

Quoted 11 lines
--NR
Quoted 2 lines
-Dave.
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore