diff mbox series

[2/8] fstests: _cleanup overrides are messy

Message ID 20220524073411.1943480-3-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series fstests: _cleanup() overrides are a mess | expand

Commit Message

Dave Chinner May 24, 2022, 7:34 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

Most _cleanup() function overrides look like:

_cleanup()
{
	# do something test specific
	cd /
	rm -rf $tmp.*
}

But they often get the last two lines either wrong or omit them.
These are the lines the common/preamble::_cleanup() define.

The problem here is that we are just overriding the generic _cleanup
function by redeclaring it after calling _begin_fstest. What we
should be doing is registering a new local cleanup function that
calls the generic cleanup function when we have finished the local
cleanup. i.e.:

_local_cleanup()
{
	# do something test specific

	_cleanup
}

Make _register_cleanup() function to cancel the existing
cleanup trap and register the new trap function so that local
cleanups can be done cleanly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 common/preamble | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Amir Goldstein May 24, 2022, 4:16 p.m. UTC | #1 
On Tue, May 24, 2022 at 6:39 PM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Most _cleanup() function overrides look like:
>
> _cleanup()
> {
>         # do something test specific
>         cd /
>         rm -rf $tmp.*
> }
>
> But they often get the last two lines either wrong or omit them.
> These are the lines the common/preamble::_cleanup() define.
>
> The problem here is that we are just overriding the generic _cleanup
> function by redeclaring it after calling _begin_fstest. What we
> should be doing is registering a new local cleanup function that
> calls the generic cleanup function when we have finished the local
> cleanup. i.e.:
>
> _local_cleanup()
> {
>         # do something test specific
>
>         _cleanup
> }
>
> Make _register_cleanup() function to cancel the existing
> cleanup trap and register the new trap function so that local
> cleanups can be done cleanly.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.
diff mbox series

Patch

diff --git a/common/preamble b/common/preamble
index e60cd949..7aa55dc6 100644
--- a/common/preamble
+++ b/common/preamble
@@ -4,7 +4,9 @@ 
 
 # Boilerplate fstests functionality
 
-# Standard cleanup function.  Individual tests can override this.
+# Standard cleanup function. If individual tests register their own cleanup
+# function, they need to call this from within their own cleanup function once
+# the test has finished cleaning up it's own state.
 _cleanup()
 {
 	cd /
@@ -19,6 +21,9 @@  _register_cleanup()
 	local cleanup="$1"
 	shift
 
+	# clear out existing traps first
+	trap - EXIT HUP INT QUIT TERM $*
+
 	test -n "$cleanup" && cleanup="${cleanup}; "
 	trap "${cleanup}exit \$status" EXIT HUP INT QUIT TERM $*
 }