Re: [PATCH v2] perf scripts: Add configurable sorting option to powerpc-hcalls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `--sort` argument value is not validated, leading to silently ignored user intent and misleading output for invalid keys.
--

commit 1aa1f92efbcdcc2ec41737225787d40ac02abf40
Author: Shivani Nittor <shivani@linux.ibm.com>

perf scripts: Add configurable sorting option to powerpc-hcalls

The powerpc-hcalls.py script currently prints hypercall statistics in a
fixed sort order based on the number of hcall occurrences. This patch
adds a --sort option to runtime-configure the sort order (count, min, max, avg).

> diff --git a/tools/perf/scripts/python/powerpc-hcalls.py b/tools/perf/scripts/python/powerpc-hcalls.py
> index 8b78dc790adb1..e5b0c1bee65fb 100644
> --- a/tools/perf/scripts/python/powerpc-hcalls.py
> +++ b/tools/perf/scripts/python/powerpc-hcalls.py

[ ... ]

> @@ -158,10 +159,45 @@ def hcall_table_lookup(opcode):
>  
>  print_ptrn = '%-28s%10s%10s%10s%10s'
>  
> +def sort_output(opcode):
> +	stats = output[opcode]
> +
> +	if sort_key == 'min':
> +		return stats['min']
> +	if sort_key == 'max':
> +		return stats['max']
> +	if sort_key == 'avg':
> +		return stats['time'] // stats['cnt']
> +
> +	return stats['cnt']
> +
> +def trace_begin():
> +    global sort_key
> +
> +    i = 1
> +    while i < len(sys.argv):
> +        arg = sys.argv[i]
> +
> +        if arg == '-s' or arg == '--sort':
> +            if i + 1 >= len(sys.argv):
> +                print("Error: -s/--sort requires a sort key argument")
> +                sys.exit(1)
> +            sort_key = sys.argv[i + 1]
> +            i += 2
> +            continue
> +
> +        if arg.startswith('--sort='):
> +            sort_key = arg.split('=', 1)[1]
> +
> +        i += 1
> +
> +    print("SORT KEY =", sort_key)

[Severity: Medium]
Will this result in silently falling back to sorting by count if an invalid
sort key is provided?

If a user runs the script with an invalid sort option like --sort=latency,
trace_begin() will assign it to sort_key and print "SORT KEY = latency".

Then in sort_output(), it will fall through the conditionals and return
stats['cnt'], leaving the user with output sorted by count despite the
printed sort key.

Could we add validation for the sort_key argument to error out if it isn't
one of the expected values (count, min, max, avg)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617144011.17359-1-shivani@linux.ibm.com?part=1