Re: [PATCH 10/58] objtool: Keep failing test workdirs by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Tests that fail due to unexpected termination (e.g., shell errors, signals) or undeclared skips have their working directories incorrectly deleted despite KEEP=failed, while the runner falsely claims they were kept.
- [Medium] When a test is executed standalone (not via run-tests.sh) and fails, KEEP=failed silently leaks the temporary directory without printing its path.
--

commit 14c7ac654e8638035a6ea9f8c26ee25df0293be4
Author: Joe Lawrence <joe.lawrence@redhat.com>

objtool: Keep failing test workdirs by default

Change the default behavior of the klp test runner to save failing test
working directories instead of deleting them on failure. Provide a KEEP
Makefile variable that can optionally retain 'all' or 'none' of those
directories.

> diff --git a/tools/objtool/tests/lib.sh b/tools/objtool/tests/lib.sh
> index 4d8401ee8b412..6c607a0435023 100644
> --- a/tools/objtool/tests/lib.sh
> +++ b/tools/objtool/tests/lib.sh
[ ... ]
> @@ -186,12 +187,14 @@ cleanup()
>  {
>  	[ -n "$workdir" ] || return 0
>  
> -	if [ -n "${KLP_TEST_KEEP:-}" ]; then
> -		[ -n "${KLP_TEST_WORKDIR:-}" ] || echo "# kept $workdir"
> -		return 0
> -	fi
> -
> -	rm -rf "$workdir"

[Severity: Medium]
When a test is executed standalone (not via run-tests.sh) and fails, does this
change silently leak the temporary directory without printing its path? 

Before this patch, standalone test failures using KEEP=failed informed the
user of the generated /tmp directory path. Without the print statement,
developers might not be aware of the leaked directory location.

> +	case "${KLP_TEST_KEEP:-failed}" in
> +	all)	return 0 ;;
> +	none)	rm -rf "$workdir" ;;
> +	failed|*)
> +		[ -n "${KLP_TEST_FAILED:-}" ] && return 0
> +		rm -rf "$workdir"

[Severity: Medium]
If a test script exits unexpectedly without setting KLP_TEST_FAILED=1
(e.g., syntax error, shell error, or signal), will this incorrectly delete
the working directory despite KEEP=failed being set?

> +		;;
> +	esac
>  }
>  
>  # setup [exported symbol...]
> diff --git a/tools/objtool/tests/run-tests.sh b/tools/objtool/tests/run-tests.sh
> index e6f1ac1b5d0da..e7bd25097ea58 100755
> --- a/tools/objtool/tests/run-tests.sh
> +++ b/tools/objtool/tests/run-tests.sh
[ ... ]
> @@ -147,16 +162,16 @@ for t in "${tests[@]}"; do
>  		rest="$rest${rest:+$'\n'}was: $result"
>  		result="not ok - $(basename "$t" .sh): undeclared skip"
>  		result="$result (use gcc_only/clang_only or require_input_*)"
> -		fail=$((fail + 1)) ;;
> +		fail=$((fail + 1)); failed_dirs+=( "$rundir/${t%.sh}" ) ;;
>  	"not ok"*"# TODO"*)	xfail=$((xfail + 1)) ;;
> -	"ok"*"# TODO"*)		xpass=$((xpass + 1)) ;;
> -	"not ok"*)		fail=$((fail + 1)) ;;
> +	"ok"*"# TODO"*)		xpass=$((xpass + 1)); failed_dirs+=( "$rundir/${t%.sh}" ) ;;
> +	"not ok"*)		fail=$((fail + 1)); failed_dirs+=( "$rundir/${t%.sh}" ) ;;
>  	"ok"*)			pass=$((pass + 1)) ;;
>  	*)
>  		# No result line at all: the test died before reporting.
>  		rest="$rest${rest:+$'\n'}exited $rc without a result line"
>  		result="not ok - $(basename "$t" .sh): no TAP result"
> -		fail=$((fail + 1)) ;;
> +		fail=$((fail + 1)); failed_dirs+=( "$rundir/${t%.sh}" ) ;;
>  	esac
>  
>  	echo "$result"
[ ... ]
> @@ -167,15 +182,35 @@ done
>  echo "# pass:$pass fail:$fail static-skip:$static_skip" \
>       "probe-skip:$probe_skip xfail:$xfail xpass:$xpass"
[ ... ]
> +failed)
> +	if [ "${#failed_dirs[@]}" -gt 0 ]; then
> +		echo "# keep=failed: ${#failed_dirs[@]} failing test(s) kept under $rundir:"
> +		for d in "${failed_dirs[@]}"; do
> +			echo "#   ${d#"$rundir"/}/"
> +		done

[Severity: Medium]
Will this instruct the user to inspect directories that have already been
wiped?

When a test crashes or skips without setting KLP_TEST_FAILED, the main loop in
run-tests.sh correctly detects this as a failure and adds it to failed_dirs.
However, cleanup() in lib.sh will have already deleted the directory because
KLP_TEST_FAILED is empty, leading to confusing instructions for the user.

> +		echo "# inspect: diff.log  readelf -S out.o"
> +		echo "# one test: $PWD/run-tests.sh <name>"
> +		echo "# cleanup: rm -rf $rundir"
> +	elif ! rmdir "$rundir" 2>/dev/null; then

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911185031.1534046-1-song@kernel.org?part=10