Re: [PATCH v5 38/58] perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

sashiko-bot
2026-04-24T17:07:13Z

Sashiko AI review found 4 potential issue(s):
- [High] Chaining `.comm()` directly on `self.session.process(pid)` is unsafe and prone to crashing due to an unhandled `TypeError`.
- [Medium] Filtering by PID 0 is ignored due to a falsy condition check.
- [Medium] Meaningful code and commit message mismatch regarding nested statistics aggregation.
- [Low] Unhandled events are aggregated but silently discarded without being displayed.
--

commit 76816da881d00075e4983b987c755a57bd2cf9b9
Author: Ian Rogers <irogers@google.com>

perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

This commit ports the failed-syscalls-by-pid script to the perf Python
module API. It updates syscall name resolution, error string resolution,
and adds support for command-line filtering by comm or pid.

> - Used standard collections.defaultdict for nested statistics
> aggregation.

Is this description accurate? The implementation uses a flat dictionary
keyed by a 4-tuple rather than nested aggregation.

> diff --git a/tools/perf/python/failed-syscalls-by-pid.py b/tools/perf/python/failed-syscalls-by-pid.py
> new file mode 100755
> index 00000000000000..1a8c9bb7e90c48
> --- /dev/null
> +++ b/tools/perf/python/failed-syscalls-by-pid.py

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process a single sample event."""
> + event_name = str(sample.evsel)
> + if "sys_exit" not in event_name:
> + return
> +
> + pid = sample.sample_pid
> + if hasattr(self, 'session') and self.session:
> + comm = self.session.process(pid).comm()

Could session.process(pid) raise a TypeError if the PID cannot be resolved
in the thread map? If so, this might crash the script when it encounters
an unresolved PID instead of safely falling back to "Unknown".

> + else:
> + comm = "Unknown"
> +
> + if self.for_comm and comm != self.for_comm:
> + return
> + if self.for_pid and pid != self.for_pid:

Will this bypass the filter if the user intends to filter by PID 0 (the swapper
task), since self.for_pid evaluates to False? Would it be better to explicitly
check if self.for_pid is not None?

> + return
> +
> + ret = getattr(sample, "ret", 0)
> + if ret < 0:
> + syscall_id = getattr(sample, "id", -1)
> + if syscall_id == -1:
> + syscall_id = getattr(sample, "sys_id", -1)
> +
> + if syscall_id != -1:
> + self.syscalls[(comm, pid, syscall_id, ret)] += 1
> + else:
> + self.unhandled[event_name] += 1

The script populates self.unhandled here, but it doesn't appear to be used
in print_summary() or anywhere else. Was the intention to display these
unhandled events in the summary?

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