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

[Qu Wenruo <wqu@suse.com>]

On 2021/7/20 上午9:16, Darrick J. Wong wrote:
> 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?

That's a great advice.

As my mindset is still in the pre-preamble age...

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

That would be super awesome, although it means the hook user should also 
be extra cautious not to crash the hook itself...

Thanks,
Qu

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