Minor cleanups in common/

common/config will be source by _begin_fstest

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

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

tests/generic/367 +0 -1

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

Signature · 1 line

2.34.1

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
 common/config | 8 ++++++++
 1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

common/config +0 -0

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

Signature · 1 line

2.34.1

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
 common/config | 8 ++++++++
 1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

Quoted 49 lines · Nirjhar Roy (IBM)

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
  common/config | 8 ++++++++
  1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 100

Quoted 5 lines

common/config +2 -0

@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
 
  export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
 
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.

...as it might not set the value of "$status" correctly, which is used as an exit code in the trap handler routine set up by the check script.

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 106

Quoted 6 lines

common/config +6 -0

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

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

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

We should update _exit() with...

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

-ritesh

Quoted 5 lines

common/config +0 -0

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

Signature · 1 line

2.34.1

Quoted 70 lines · Ritesh Harjani (IBM)

Quoted 49 lines · Nirjhar Roy (IBM)

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
  common/config | 8 ++++++++
  1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 98

Quoted 3 lines

common/config +2 -0

@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
  export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.

...as it might not set the value of "$status" correctly, which is used as an exit code in the trap handler routine set up by the check script.

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 106

Quoted 6 lines

common/config +6 -0

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

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

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

Quoted 3 lines

We should update _exit() with...

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

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

--NR

Quoted 9 lines

-ritesh

Quoted 5 lines

common/config +0 -0

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

Signature · 1 line

2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Quoted 71 lines · Nirjhar Roy (IBM)

Quoted 70 lines · Ritesh Harjani (IBM)

Quoted 49 lines · Nirjhar Roy (IBM)

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
   common/config | 8 ++++++++
   1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 98

Quoted 3 lines

common/config +2 -0

@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
   export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.

...as it might not set the value of "$status" correctly, which is used as an exit code in the trap handler routine set up by the check script.

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 106

Quoted 6 lines

common/config +6 -0

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

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

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

Quoted 5 lines

Quoted 3 lines

We should update _exit() with...

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

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

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

trap 'echo cleanup' INT QUIT TERM EXIT
exit 5

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

--D

Quoted 15 lines

--NR

Quoted 8 lines

-ritesh

Quoted 5 lines

common/config +0 -0

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

Signature · 1 line

2.34.1

Signature · 3 lines

Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Quoted 49 lines · Nirjhar Roy (IBM)

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

if $OPTIONS_HAVE_SECTIONS; then

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

fi

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

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

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

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

The next patch will replace exit with _exit.

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

Reviewed-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Dave Chinner <[email protected]>

---
  common/config | 8 ++++++++
  1 file changed, 8 insertions(+)

common/config +0 -0

index 79bec87f..eb6af35a 100644

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 105

Quoted 10 lines

common/config +7 -0

@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
 
  export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
 
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.
+_exit()
+{
+	status="$1"
+	exit "$status"
+}

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

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

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

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

-Dave.

Quoted 28 lines · Dave Chinner

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 103

Quoted 13 lines · Nirjhar Roy (IBM)

common/config +7 -0

index 79bec87f..eb6af35a 100644
@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
  export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.
+_exit()
+{
+	status="$1"
+	exit "$status"
+}

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

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

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

Quoted 6 lines

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

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

common/config will be source by _begin_fstest

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

---

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

tests/generic/367 +0 -1

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

Signature · 1 line

2.34.1

Quoted 35 lines · Nirjhar Roy (IBM)

Quoted 34 lines · Dave Chinner

common/config export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes} · line 103

Quoted 13 lines · Nirjhar Roy (IBM)

common/config +7 -0

index 79bec87f..eb6af35a 100644
@@ -96,6 +96,14 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
   export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
+# This functions sets the exit code to status and then exits. Don't use
+# exit directly, as it might not set the value of "status" correctly.
+_exit()
+{
+	status="$1"
+	exit "$status"
+}

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

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

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

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

Quoted 3 lines

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

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

-Dave.