Re: [PATCH v4 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 intentionally bypasses temporary directory cleanup on failure to aid debugging, but points the user to an empty directory because it fails to pass the `-debug` flag to the Python validator.
--

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

This patch updates the stat_metrics_values.sh script to run the metrics
value validation linter twice: once for the legacy path, and once under
the --new print flag. It also adds early exits on validation failures
so developers can inspect the output directory.

> 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]
Will this leave the user with an empty directory to inspect?

The script intentionally exits early on failure to bypass the temporary
directory cleanup, directing the user to check "$tmpdir/legacy".

However, looking at Validator.create_report() in perf_metric_validation.py,
the Python script only generates output files if the debug flag is provided:

tools/perf/tests/shell/lib/perf_metric_validation.py:Validator.create_report() {
    ...
    if self.debug:
        allres = [{"Workload": self.workloads[i], ... }]
        self.json_dump(allres, self.datafname)
    ...
}

Since -debug is not passed to the Python script in this patch, won't this
result in an empty directory being leaked in /tmp while leaving the
developer with no output data to inspect?

> +	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]
This appears to have the same issue as above. The script directs the user to 
inspect "$tmpdir/new", but without passing -debug to the pythonvalidator, won't
this directory also be empty?

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

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