Re: [PATCH v2 6/6] perf test: Simplify perf kvm record/report tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unwrapped command execution in test script with `set -e` causes premature exit on failure.
--

commit 3ce1271a163998e293f818fbfbc37ed07914f209
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Simplify perf kvm record/report tests

It can now pass command line arguments to perf kvm record --host.
Let's pass 'sleep 1' directly rather than running it in background.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..4ca10388748de 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
> @@ -63,13 +63,7 @@ test_kvm_record_report() {
>  	echo "Recording kvm profile for pid ${qemu_pid}..."
>  	# Use --host to avoid needing guest symbols/mounts for this simple test
>  	# We just want to verify the command runs and produces data
> -	# We run in background and kill it because 'perf kvm record' appends options
> -	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
> -	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
> -	rec_pid=$!
> -	sleep 1
> -	kill -INT "${rec_pid}"
> -	wait "${rec_pid}" || true
> +	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1

[Severity: Medium]
Since this script runs with set -e, will a failure in perf kvm here cause 
the script to exit immediately?

The previous backgrounding implementation handled failures gracefully using
wait "${rec_pid}" || true. If this new foreground command fails (for example 
due to restricted perf_event_paranoid settings), it seems the entire test 
script will abort early and skip the remaining tests instead of accumulating 
errors gracefully. 

Should this be wrapped to catch failures, or append || true to prevent a 
premature script exit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623191439.3621593-1-namhyung@kernel.org?part=6