Re: [PATCH v7] 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] The `setup()` callback's error code is silently discarded, causing all setup failures to manifest as Out of Memory (-ENOMEM) errors and aborting the test execution.
--

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

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

This commit adds a setup callback and private data pointer to the test
structures to support dynamically generating test suites. It updates
build_suites() to invoke this setup callback prior to test execution or
listing.

> 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
> @@ -754,10 +754,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]
When a setup callback fails and returns an error code like -EPERM or -EINVAL,
returning NULL here drops the actual error code. Should build_suites() either
propagate the exact error, or perhaps just skip the unsupported suite instead
of aborting the entire test run?

> +		}
>  		num_suites++;
> +	}
>  
>  	result = calloc(num_suites + 1, sizeof(struct test_suite *));
> +	if (!result)
> +		return NULL;

[ ... ]

> @@ -820,6 +829,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;

[Severity: Medium]
Since build_suites() can now return NULL due to a setup callback failing,
does converting the NULL return to -ENOMEM mask the real reason the tests
failed to build? This might make debugging setup failures difficult for users.

[ ... ]

> @@ -852,6 +863,8 @@ int cmd_test(int argc, const char **argv)
>  	rlimit__bump_memlock();
>  
>  	suites = build_suites();
> +	if (!suites)
> +		return -ENOMEM;
>  	ret = __cmd_test(suites, argc, argv, skiplist);
>  	free(suites);

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