Re: [PATCH 29/29] perf test: Add truncated perf.data robustness test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unchecked `mktemp` failures lead to unintended file deletions and false positive test results.
- [Low] The use of `dd bs=1` forces an excessive number of 1-byte system calls, causing unnecessary performance degradation.
--

commit 44bbb01cefd526160c464bb6e4d6a81af952c364
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf test: Add truncated perf.data robustness test

This commit adds a shell test to verify that perf report handles truncated
perf.data files gracefully by exiting with an error code rather than
crashing. It records a simple workload and truncates the file at various
offsets, checking the exit code and stderr output for sanitizer violations
or crashes.

> diff --git a/tools/perf/tests/shell/data_validation.sh b/tools/perf/tests/shell/data_validation.sh
> new file mode 100755
> index 0000000000000..36b3ba508e4f2
> --- /dev/null
> +++ b/tools/perf/tests/shell/data_validation.sh
> @@ -0,0 +1,85 @@
> +#!/bin/bash

[ ... ]

> +cleanup() {
> +	rm -f "${perfdata}" "${perfdata}.old" "${truncated}" "${stderrfile}"
> +	trap - EXIT TERM INT
> +}
> +trap 'cleanup; exit 1' TERM INT
> +trap cleanup EXIT
> +
> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +truncated=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +stderrfile=$(mktemp /tmp/__perf_test.perf.data.XXXXX)

[Severity: Medium]
Are we missing checks for mktemp failures here?

If mktemp fails (for instance, due to a full or unwritable /tmp), the
perfdata, truncated, and stderrfile variables will remain empty.

When cleanup executes rm -f "${perfdata}.old", an empty variable causes
it to evaluate to rm -f .old, which destructively removes any file named
.old in the current working directory.

Additionally, if stderrfile is empty, the redirection 2> "${stderrfile}"
later in the script causes a bash syntax error, exiting with code 1.
The script interprets this non-zero exit code as a successful test, masking
the setup failure and falsely reporting a pass.

[ ... ]

> +for cut_at in 8 64 256 $((file_size * 3 / 4)); do
> +	if [ "${cut_at}" -ge "${file_size}" ]; then
> +		continue
> +	fi
> +	dd if="${perfdata}" of="${truncated}" bs=1 count="${cut_at}" 2>/dev/null

[Severity: Low]
Does using bs=1 force an excessive number of system calls?

In the truncation loop, dd is invoked with bs=1 and count=${cut_at}, which
forces dd to perform a 1-byte read() and 1-byte write() system call for
every byte. For example, truncating to 75% of a 100KB file would require
75,000 reads and writes.

While the noploop workload generates a small file, this is highly
inefficient and risks slowing down CI execution on emulators or slower
nodes. Could we use bs="${cut_at}" count=1 or head -c instead, which
would read the entire chunk in one pass?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525010550.1100375-1-acme@kernel.org?part=29