Re: [PATCH v9 1/1] perf jevents: Check the keys of the JSON tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] perf test: jevents_check.sh never checks in-tree PMU JSON
  tables
- [Low] perf jevents: main() rejects --check=DIR due to exact sys.argv
  check
- [Low] perf jevents: _check_tables() silently passes nonexistent
  directories
--

commit 45e79c2a6c1a4e8e393708b7a23890578df64421
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf jevents: Check the keys of the JSON tables

This commit adds a --check option to jevents.py to validate JSON event
table keys against an allowlist and adds a shell test for it.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..5daf92527573e 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> +def _check_tables(root: str) -> int:
> +  """Report the keys in the tables under root that a table may not carry."""
> +  bad = 0
> +  for path in sorted(Path(root).rglob('*.json')):
> +    if path.name.endswith('metricgroups.json'):
> +      # A dictionary keyed by metric group name, not a list of events.
> +      continue
> +    for obj in json.loads(path.read_text(encoding='utf-8')):
> +      if extra := set(obj) - _TABLE_KEYS:
> +        print(f'{path}: {sorted(extra)}')
> +        bad += 1
> +  return 1 if bad else 0

[Severity: Low]
Does _check_tables() silently succeed when given a nonexistent directory?

Although the commit message notes that a missing directory is reported
instead of passed, --check does not use type=dir_path and
Path(root).rglob('*.json') yields an empty iterator when root does not
exist, so _check_tables() returns 0 without checking any files.

>  def main() -> None:
>    global _args
>  
> +  # --check takes the directory to check and needs nothing else, so it is read
> +  # before the parser below asks for the three positional arguments.
> +  if '--check' in sys.argv:
> +    ap = argparse.ArgumentParser()
> +    ap.add_argument('--check', metavar='DIR', required=True,
> +                    help='report the keys of the tables under DIR and exit')
> +    sys.exit(_check_tables(ap.parse_args().check))

[Severity: Low]
Does the "'--check' in sys.argv" guard reject "--check=DIR"?

List membership checks for an exact match against "--check", so passing
"--check=DIR" as a single argument skips this branch and falls through to
the main ArgumentParser, which fails because the positional arguments are
missing.

> diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
> new file mode 100755
> index 0000000000000..62ce70ef5d022
> --- /dev/null
> +++ b/tools/perf/tests/shell/jevents_check.sh
> @@ -0,0 +1,29 @@
[ ... ]
> +tmp=$(mktemp -d)
> +trap 'rm -rf "${tmp}"' EXIT
> +tables="${tmp}/arch/x86/model"
> +mkdir -p "${tables}"
> +
> +# Known keys pass.
> +printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
> +"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null || exit 1
> +
> +# A key the generator does not read is reported.
> +printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
> +"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null && exit 1
> +
> +exit 0

[Severity: Medium]
Should jevents_check.sh also run --check against the in-tree tables under
tools/perf/pmu-events/arch?

As written, the test only checks a synthetic file in a temporary directory
and neither the build nor perf test ever runs --check on the repository's
JSON tables, so unknown keys in tools/perf/pmu-events/arch go undetected.
Also, discarding stdout and stderr on the negative check means any crash or
import failure in jevents.py will cause that check to pass.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112222526.462146-1-takano32@gmail.com?part=1