Re: [PATCH 04/15] perf test: Support dynamic test suites with setup callback and private data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Error codes from a test suite's `setup` callback are discarded and inappropriately masked as `-ENOMEM`.
--

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

perf test: Support dynamic test suites with setup callback and private data

This commit adds a void pointer and a setup callback to test suites to allow
for dynamic generation of test cases. The setup callback is invoked during
suite initialization to ensure cases are available before listing or running.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -765,10 +765,19 @@ static struct test_suite **build_suites(void)
>  	for (size_t i = 0, j = 0; i < ARRAY_SIZE(suites); i++, j = 0)	\
>  		while ((suite = suites[i][j++]) != NULL)
>  
> -	for_each_suite(t)
> +	for_each_suite(t) {
> +		if (t->setup) {
> +			int ret = t->setup(t);
> +
> +			if (ret < 0)
> +				return NULL;

[Severity: Medium]
Is it intentional to discard the specific error code returned by t->setup()?

When t->setup() returns a negative error code (like -EINVAL), build_suites() 
returns NULL, which drops the original error context.

> +		}
>  		num_suites++;
> +	}
>  
>  	result = calloc(num_suites + 1, sizeof(struct test_suite *));
[ ... ]
> @@ -831,6 +840,8 @@ int cmd_test(int argc, const char **argv)
>  	argc = parse_options_subcommand(argc, argv, test_options, test_subcommands, test_usage, 0);
>  	if (argc >= 1 && !strcmp(argv[0], "list")) {
>  		suites = build_suites();
> +		if (!suites)
> +			return -ENOMEM;

Because build_suites() returns NULL on any setup error, the callers in
cmd_test() unconditionally return -ENOMEM. Would it be better for
build_suites() to return the actual error code (perhaps via an
ERR_PTR) so that tools report the correct failure reason instead
of always failing with out-of-memory?

>  		ret = perf_test__list(stdout, suites, argc - 1, argv + 1);
>  		free(suites);
>  		return ret;

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