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

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Jun 23, 2026 at 07:09:14AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The synchronous `perf kvm record` command is not wrapped in an error-checking conditional, causing the script to unexpectedly abort via `set -e` upon failure.
> --
> 
> commit a940fab65237cd8e4c37035fe66af932fcd3f955
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf test: Simplify perf kvm record/report tests
> 
> This commit simplifies the perf kvm tests by passing sleep directly as a
> command line argument to perf kvm record, rather than running it in the
> background and manually killing it.
> 
> > 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]
> If this command fails, will the script abort immediately due to set -e being
> enabled globally?
> 
> The previous implementation guarded against non-zero exit codes by using
> wait "${rec_pid}" || true. Without an error-checking conditional or an
> appended || true, a failure during this synchronous execution could cause the
> script to instantly terminate.
> 
> Would this bypass the test's intended logic of recording the failure (err=1)
> and accidentally skip subsequent tests like test_kvm_buildid_list?

I think it's ok to fail when the record command failed.

Thanks,
Namhyung

> 
> >  
> >  	echo "Reporting kvm profile..."
> >  	# Check for some standard output from report
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=4