Re: [PATCH RFC] fstests: allow running custom hooks

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote:
> This patch will allow fstests to run custom hooks before and after each
> test case.
> 
> These hooks will need to follow requirements:
> 
> - Both hook files needs to be executable
>   Or they will just be ignored
> 
> - Stderr and stdout will be redirected to "$seqres.full"
>   With extra separator to distinguish the hook output with real
>   test output
> 
>   Thus if any of the hook is specified, all tests will generate
>   "$seqres.full" which may increase the disk usage for results.
> 
> - Error in hooks script will be ignored completely
> 
> - Environment variable "$HOOK_TEMP" will be exported for both hooks
>   And the variable will be ensured not to change for both hooks.
> 
>   Thus it's possible to store temporary values between the two hooks,
>   like pid.
> 
> - Start hook has only one parameter passed in
>   $1 is "$seq" from "check" script. The content will the path of current
>   test case. E.g "tests/btrfs/001"
> 
> - End hook has two parameters passed in
>   $1 is the same as start hook.
>   $2 is the return value of the test case.
>   NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak
>   check.
> 
> For more info, please refer to "README.hooks".
> 
> Also update .gitignore to ignore "hooks/start.hook" and "hooks/end.hook"
> so that one won't accidentally submit the debug hook.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Instead of previous attempt to manually utilize sysfs interface of
> ftrace, this time just add some hooks to allow one to do whatever they
> want.
> 
> RFC for how everything should be passed to hooks.
> Currently it's using a mixed method, $seq/$sts is passed as paramaters,
> while HOOK_TMP is passed as environmental variable.
> 
> Not sure what's the recommended way for hooks.
> ---
>  .gitignore      |  4 +++
>  README.hooks    | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
>  check           |  5 ++++
>  common/preamble |  4 ---
>  common/rc       | 43 +++++++++++++++++++++++++++++
>  5 files changed, 124 insertions(+), 4 deletions(-)
>  create mode 100644 README.hooks
> 
> diff --git a/.gitignore b/.gitignore
> index 2d72b064..99905ff9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -201,3 +201,7 @@ tags
>  # cscope files
>  cscope.*
>  ncscope.*
> +
> +# hook scripts
> +/hooks/start.hook
> +/hooks/end.hook
> diff --git a/README.hooks b/README.hooks
> new file mode 100644
> index 00000000..be92a7d7
> --- /dev/null
> +++ b/README.hooks
> @@ -0,0 +1,72 @@
> +To run extra commands before and after each test case, there is the
> +'hooks/start.hook' and 'hooks/end.hook' files for such usage.
> +
> +Some notes for those two hooks:
> +
> +- Both hook files needs to be executable
> +  Or they will just be ignored
> +
> +- Stderr and stdout will be redirected to "$seqres.full"
> +  With extra separator to distinguish the hook output with real
> +  test output
> +
> +  Thus if any of the hook is specified, all tests will generate
> +  "$seqres.full" which may increase the disk usage for results.
> +
> +- Error in hooks script will be ignored completely
> +
> +- Environment variable "$HOOK_TEMP" will be exported for both hooks
> +  And the variable will be ensured not to change for both hooks.
> +
> +  Thus it's possible to store temporary values between the two hooks,
> +  like pid.
> +
> +- Start hook has only one parameter passed in
> +  $1 is "$seq" from "check" script. The content will the path of current
> +  test case. E.g "tests/btrfs/001"
> +
> +- End hook has two parameters passed in
> +  $1 is the same as start hook.
> +  $2 is the return value of the test case.
> +  NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak
> +  check.
> +
> +The very basic test hooks would look like:
> +
> +hooks/start.hook:
> +  #!/bin/bash
> +  seq=$1
> +  echo "HOOK_TMP=$HOOK_TMP"
> +  echo "seq=$seq"
> +
> +hooks/end.hook:
> +  #!/bin/bash
> +  seq=$1
> +  sts=$2
> +  echo "HOOK_TMP=$HOOK_TMP"
> +  echo "seq=$seq"
> +  echo "sts=$sts"
> +
> +Then run test btrfs/001 and btrfs/002, their "$seqres.full" would look like:
> +
> +result/btrfs/001.full:
> +  === Running start hook ===
> +  HOOK_TMP=/tmp/78973.hook
> +  seq=tests/btrfs/001
> +  === Start hook finished ===
> +  === Running end hook ===
> +  HOOK_TMP=/tmp/78973.hook
> +  seq=tests/btrfs/001
> +  sts=0
> +  === End hook finished ===
> +
> +result/btrfs/002.full:
> +  === Running start hook ===
> +  HOOK_TMP=/tmp/78973.hook
> +  seq=tests/btrfs/002
> +  === Start hook finished ===
> +  === Running end hook ===
> +  HOOK_TMP=/tmp/78973.hook
> +  seq=tests/btrfs/002
> +  sts=0
> +  === End hook finished ===
> diff --git a/check b/check
> index bb7e030c..f24906f5 100755
> --- a/check
> +++ b/check
> @@ -846,6 +846,10 @@ function run_section()
>  		# to be reported for each test
>  		(echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1
>  
> +		# Remove previous $seqres.full before start hook
> +		rm -f $seqres.full
> +
> +		_run_start_hook

Seeing as we now have standardized preamble and cleanup in every single
test, why don't you run this from _begin_fstest and modify
_register_cleanup to run _run_end_hook from the trap handler?

This way the start hooks run from within each test and therefore can
modify the test's environment (as opposed to ./check's environment; the
two are /not/ the same thing!) and/or _notrun the test like Ted wants.

(Granted I wonder why not use an exclude list, because I bet my 3.10
kernel has patches that yours doesn't, so kernel versions aren't all
that meaningful...)

--D

>  		if [ "$DUMP_OUTPUT" = true ]; then
>  			_run_seq 2>&1 | tee $tmp.out
>  			# Because $? would get tee's return code
> @@ -854,6 +858,7 @@ function run_section()
>  			_run_seq >$tmp.out 2>&1
>  			sts=$?
>  		fi
> +		_run_end_hook
>  
>  		if [ -f core ]; then
>  			_dump_err_cont "[dumped core]"
> diff --git a/common/preamble b/common/preamble
> index 66b0ed05..41a12060 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -56,8 +56,4 @@ _begin_fstest()
>  	_register_cleanup _cleanup
>  
>  	. ./common/rc
> -
> -	# remove previous $seqres.full before test
> -	rm -f $seqres.full
> -
>  }
> diff --git a/common/rc b/common/rc
> index d4b1f21f..ec434aa5 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4612,6 +4612,49 @@ _require_names_are_bytes() {
>          esac
>  }
>  
> +_run_start_hook()
> +{
> +	if [[ ! -d "hooks" ]]; then
> +		return
> +	fi
> +
> +	if [[ ! -x "hooks/start.hook" ]]; then
> +		return
> +	fi
> +
> +	# Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp",
> +	# so we won't overwrite any existing $tmp.* files
> +	export HOOK_TMP=$tmp.hook
> +
> +	echo "=== Running start hook ===" >> $seqres.full
> +	# $1 is alwasy $seq
> +	./hooks/start.hook $seq >> $seqres.full 2>&1
> +	echo "=== Start hook finished ===" >> $seqres.full
> +	unset HOOK_TMP
> +}
> +
> +_run_end_hook()
> +{
> +	if [[ ! -d "hooks" ]]; then
> +		return
> +	fi
> +
> +	if [[ ! -x "hooks/end.hook" ]]; then
> +		return
> +	fi
> +
> +	# Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp",
> +	# so we won't overwrite any existing $tmp.* files
> +	export HOOK_TMP=$tmp.hook
> +
> +	echo "=== Running end hook ===" >> "$seqres.full"
> +	# $1 is alwasy $seq
> +	# $2 is alwasy $sts
> +	./hooks/end.hook $seq $sts >> "$seqres.full" 2>&1
> +	echo "=== End hook finished ===" >> "$seqres.full"
> +	unset HOOK_TMP
> +}
> +
>  init_rc
>  
>  ################################################################################
> -- 
> 2.31.1
>