Re: [PATCH v1 5/7] perf test: Enhance annotate test coverage and isolate config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Premature script abort and unreachable error handling logic due to the global `set -e` shell option in `test_disassembler`.
- [Low] The cleanup function unnecessarily attempts to delete temporary configuration files that the script never creates.
--

commit 4dda72bcf9505cc21e386908e19f007d3ab5a6ec
Author: Ian Rogers <irogers@google.com>

perf test: Enhance annotate test coverage and isolate config
    
This commit updates the annotate.sh shell test to cover different disassembler
backends utilizing the new command-line option. It also attempts to isolate
the test script from host environment pollution by exporting
PERF_CONFIG=/dev/null at the start of the script.

> diff --git a/tools/perf/tests/shell/annotate.sh b/tools/perf/tests/shell/annotate.sh
> index 689de58e92384..d3bafa7e3db55 100755
> --- a/tools/perf/tests/shell/annotate.sh
> +++ b/tools/perf/tests/shell/annotate.sh

[ ... ]

> @@ -23,6 +25,7 @@ disasm_regex="[0-9]*\.[0-9]* *: *\w*: *\w*"
>  cleanup() {
>    rm -rf "${perfdata}" "${perfout}"
>    rm -rf "${perfdata}".old
> +  rm -f /tmp/__perf_test.config.*
>  
>    trap - EXIT TERM INT
>  }

[Severity: Low]
Is this cleanup necessary? 

Since PERF_CONFIG is exported to /dev/null at the start of the script, it
doesn't appear that any temporary configuration files matching this pattern
are ever created by this test.

[ ... ]

> @@ -106,8 +109,61 @@ test_basic() {
> +test_disassembler() {
> +  disassembler=$1
> +  feature=$2

[ ... ]

> +  perf annotate --no-demangle -i "${perfdata}" --stdio --percent-limit 10 --disassembler "${disassembler}" 2> /dev/null > "${perfout}"
> +  ret=$?
> +
> +  if [ "x${ret}" != "x0" ]
> +  then
> +    echo "annotate with ${disassembler} [Failed: perf annotate error]"
> +    err=1
> +    return 1
> +  fi

[Severity: Medium]
Because this script operates with set -e globally, will a failure of perf
annotate immediately abort the script? 

If the command fails, it seems the shell would terminate before reaching the
ret=$? assignment, making the explicit error handling and logging dead code.

> +
> +  # check if it has the target symbol
> +  if ! grep -q "${testsym}" "${perfout}"
> +  then
> +    echo "annotate with ${disassembler} [Failed: missing target symbol]"
> +    err=1
> +    return 1
> +  fi

[Severity: Medium]
Similarly, since the test_disassembler function is called sequentially
without protection, will returning 1 here trigger a set -e abort at the call
site?

This might abruptly halt the entire test suite and prevent the remaining
disassembler backends from running, rather than continuing to the next test
as the err=1 assignment suggests.

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