Re: [PATCH v3 14/14] perf stat: Extend PMU metrics value linter to validate --new outputs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test script directs users to an empty debug directory and leaks temporary files on failure.
--

--- Patch [14]: [PATCH v3 14/14] perf stat: Extend PMU metrics value linter to validate --new outputs ---
commit 04fc0ceb56132c274f7a0d6caa07c7fa1e8373fa
Author: Ian Rogers <irogers@google.com>

perf stat: Extend PMU metrics value linter to validate --new outputs

This commit updates the stat_metrics_values.sh test to run the python metrics
value validation linter twice. It tests both the legacy path and the new API
path using the -new argument to mathematically guarantee valid metric
calculations.

> diff --git a/tools/perf/tests/shell/stat_metrics_values.sh b/tools/perf/tests/shell/stat_metrics_values.sh
> index 30566f0b54279..9823b09589574 100755
> --- a/tools/perf/tests/shell/stat_metrics_values.sh
> +++ b/tools/perf/tests/shell/stat_metrics_values.sh
[ ... ]
> @@ -19,13 +19,29 @@ echo "Output will be stored in: $tmpdir"
>  for cputype in /sys/bus/event_source/devices/cpu_*; do
>  	cputype=$(basename "$cputype")
>  	echo "Testing metrics for: $cputype"
> -	$PYTHON $pythonvalidator -rule $rulefile -output_dir $tmpdir -wl "${workload}" \
> +	mkdir -p "$tmpdir/legacy"
> +	$PYTHON $pythonvalidator -rule $rulefile -output_dir "$tmpdir/legacy" -wl "${workload}" \
>  		-cputype "${cputype}"
>  	ret=$?
> -	rm -rf $tmpdir
>  	if [ $ret -ne 0 ]; then
> -		echo "Metric validation return with errors. Please check metrics reported with errors."
> +		echo "Metric validation return with errors. " \
> +			"Please check metrics reported with errors in: " \
> +			"$tmpdir/legacy"
> +		exit $ret

[Severity: Medium]
Does this error path leak the temporary directory?

Since the cleanup is now at the end of the script instead of inside the loop,
calling exit here will bypass the removal and leave the temporary directory
behind.

Additionally, will this output directory actually contain any error reports?

The error message directs users to check the legacy directory for details, but
because the python validator is run without the -debug flag, it appears this
directory will remain completely empty upon failure.

> +	fi
> +
> +	echo "Testing metrics for: $cputype (New API)"
> +	mkdir -p "$tmpdir/new"
> +	$PYTHON $pythonvalidator -rule $rulefile -output_dir "$tmpdir/new" -wl "${workload}" \
> +		-cputype "${cputype}" -new
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "Metric validation return with errors (New API). " \
> +			"Please check metrics reported with errors in: " \
> +			"$tmpdir/new"
> +		exit $ret

[Severity: Medium]
Similarly, does this exit path also bypass the temporary directory cleanup
at the bottom of the script?

Also like the legacy path above, without passing the -debug flag to the python
validator, it looks like this directory will also be empty when a user attempts
to inspect it for errors.

>  	fi
>  done
> +rm -rf "$tmpdir"
>  exit $ret

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716043223.388233-1-irogers@google.com?part=14