Re: [PATCH v1 04/14] 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 2 potential issue(s) to consider:
- [High] Missing NULL check for calloc in build_suites() leads to a crash on allocation failure.
- [Medium] A single test suite's setup() failure aborts the entire test framework and falsely reports -ENOMEM.
--

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

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

This commit adds a setup callback to test_suite and a private data
pointer to test_case to allow for dynamic test case generation. The
build_suites function is updated to invoke the setup callback for each
suite if it is present. Callers of build_suites are also updated to
return -ENOMEM on failure.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index e88a5f72faed3..a350db071674a 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -764,8 +764,15 @@ 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;

If a single test suite's setup() callback returns an error code like
-EPERM or -ENOENT, does it cause build_suites() to abort entirely by
returning a NULL pointer?

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

Since the callers in cmd_test() were updated to check if build_suites()
returns a NULL pointer to handle allocation failures, does the calloc()
here also need a check to prevent a NULL pointer dereference? If calloc()
fails, result[n++] = t will be executed unconditionally on the next pass.

[ ... ]
> @@ -830,6 +837,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 a NULL pointer when any test suite fails
setup(), cmd_test() will map this to -ENOMEM and abort. Could this
regression cause a localized failure in one dynamic suite to prevent all
other static test suites from running or being listed, while also masking
the actual error code from the user?

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