Re: [PATCH v6 44/59] perf sctop: Port sctop to use python module

sashiko-bot
2026-04-25T18:08:58Z

Sashiko AI review found 5 potential issue(s):
- [High] Filtering by command name (`for_comm`) silently drops all events in live mode.
- [Medium] Terminal is cleared during fast offline processing, hiding all but the final interval.
- [Medium] Error messages printed in the main exception block are immediately erased from the terminal.
- [Medium] The `interval` argument lacks validation against zero or negative values, leading to infinite loops or terminal spam.
- [Low] Misleading usage message instructing execution via `perf script`.
--

commit 1d0230890ca491ddc08c37ac65574ba53c7bac25
Author: Ian Rogers <irogers@google.com>

perf sctop: Port sctop to use python module

This commit ports sctop.py from tools/perf/scripts/python/ to
tools/perf/python/ using a class-based structure. It also adds live mode
support using the LiveSession helper with a fallback strategy for
tracepoint names.

> diff --git a/tools/perf/python/sctop.py b/tools/perf/python/sctop.py
> new file mode 100755
> index 000000000000..b94f66a8307d
> --- /dev/null
> +++ b/tools/perf/python/sctop.py
> @@ -0,0 +1,186 @@
[ ... ]
> + comm = "Unknown"
> + if hasattr(self, 'session') and self.session:
> + try:
> + proc = self.session.find_thread(sample.sample_pid)
> + if proc:
> + comm = proc.comm()
> + except (TypeError, AttributeError):
> + pass
> + else:
> + comm = getattr(sample, "comm", "Unknown")

In live mode, analyzer.session is None, so this falls back to
getattr(sample, "comm", "Unknown"). Since LiveSession reads raw RECORD_SAMPLE
events without maintaining a thread map, will sample.comm always evaluate to
"Unknown"?

> +
> + if name in ("evsel(raw_syscalls:sys_enter)", "evsel(syscalls:sys_enter)"):
> + if self.for_comm is not None and comm != self.for_comm:
> + return

If sample.comm is always "Unknown" in live mode, will filtering by command
name silently drop all events and present an empty screen?

[ ... ]
> + if self.last_print_time is None:
> + self.last_print_time = sample.time
> + elif sample.time - self.last_print_time >= interval_ns:
> + self.print_current_totals()
> + self.last_print_time = sample.time
> +
> + def print_current_totals(self):
> + """Print current syscall totals."""
> + # Clear terminal
> + print("\x1b[2J\x1b[H", end="")

In offline mode, events are processed as fast as they can be read. Does
clearing the terminal here cause the screen to flash and hide all but the
final interval's data from the user?

[ ... ]
> + for_comm = None
> + default_interval = 3
> + interval = default_interval
> +
> + if len(args.args) > 2:
> + print("Usage: perf script -s sctop.py [comm] [interval]")

This isn't a bug, but since this has been ported to a standalone Python
script, is this usage message misleading?

> + sys.exit(1)
> +
> + if len(args.args) > 1:
> + for_comm = args.args[0]
> + try:
> + interval = int(args.args[1])
> + except ValueError:
> + print(f"Invalid interval: {args.args[1]}")
> + sys.exit(1)
> + elif len(args.args) > 0:
> + try:
> + interval = int(args.args[0])

Is it possible for the interval to be zero or negative here?

If 0 or a negative number is provided, in live mode
self.stop_event.wait(self.interval) will return immediately. Will this cause
the background thread to spin in a tight loop and max out a CPU core?

> + except ValueError:
> + for_comm = args.args[0]
> + interval = default_interval

[ ... ]

> + live_session.run()
> + except KeyboardInterrupt:
> + pass
> + except IOError as e:
> + print(f"Error: {e}")
> + finally:
> + if args.input:
> + analyzer.print_current_totals()
> + else:
> + analyzer.stop()

If an IOError occurs, the error message is printed. However, the finally block
either calls analyzer.print_current_totals() (in offline mode) or unblocks the
background thread which calls it (in live mode).

Since print_current_totals() unconditionally clears the screen with
"\x1b[2J\x1b[H", will this immediately erase the error message and hide the
failure reason from the user?

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