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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf test: jevents_check.sh never checks in-tree PMU JSON
  tables
- [Low] perf jevents: --check exits 0 on missing dir and rejects
  --check=DIR
--

commit 36d55c52d8c667ed37678b7b12c79a3614fd5c56
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 report unrecognized JSON
object keys in PMU event tables and adds a shell test exercising that option.

> 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
> +
>  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))
> +
>    def dir_path(path: str) -> str:
>      """Validate path is a directory for argparse."""
>      if os.path.isdir(path):

[Severity: Low]
Does _check_tables() exit with status 0 when given a nonexistent or
non-directory path?  Unlike starting_dir below, --check does not validate its
argument with type=dir_path, and Path(root).rglob('*.json') yields an empty
iterator when root does not exist, so a misspelled directory silently passes
with zero files checked.

Also, checking "'--check' in sys.argv" misses the single-token "--check=DIR"
form, which falls through to the main parser and fails with missing positional
arguments.

> 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 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# jevents_check: an unknown key in a table is reported (no_mlock)
> +#
> +# A key that is spelled differently is dropped without a word, so the keys of
> +# the tables are checked against the list jevents.py reads.
> +
> +jevents=$(dirname "$0")/../../pmu-events/jevents.py
> +python=${PYTHON:-python3}
> +
> +# An installed perf has the test but not the generator, and a python that
> +# cannot run the generator is not a failure of the check.
> +[ -r "${jevents}" ] || exit 2
> +command -v "${python}" > /dev/null || exit 2
> +
> +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

[Severity: Medium]
Does jevents_check.sh ever run jevents.py --check on the in-tree JSON tables
under tools/perf/pmu-events/arch?

The commit message and file header state that the table key check runs as a
perf test, and _TABLE_KEYS and _check_tables() include exceptions for unused
keys and metricgroups.json in the tree.  However, this script only tests a
synthetic two-key file in ${tmp}/arch, and nothing in the build or test suite
invokes --check on tools/perf/pmu-events/arch, so neither existing nor future
key typos in the repository's PMU tables are caught by perf test.

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