Re: [PATCH v7 5/7] perf test: Format log directories for shell tests

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] Temporary directory resource leak when strdup() fails.
- [Low] Implementation of character replacement is overly aggressive and misaligned with the commit intent.
--

commit deaef0503b790e27361344881ded63d280631c12
Author: Jakub Brnak <jbrnak@redhat.com>

perf test: Format log directories for shell tests

This commit creates a temporary directory for shell tests using a template based
on the test suite name. It replaces non-alphanumeric characters with underscores
to avoid issues with whitespace or special characters when logging.

>     Replace the whitespace characters with an underscore to prevent the
>     possible issues caused by the name splitting.

This isn't a bug, but the commit message mentions replacing whitespace
characters, while the implementation replaces all non-alphanumeric characters.
Could the commit message be updated to match the code?

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 306ecfe5e22ff..6cdff3687a828 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -342,16 +342,28 @@ static bool create_logs(struct test_suite *t, int pass)
>  	return false;
>  }
>  
> +static const char PERF_LOG_PREFIX[] = "/tmp/perf_test_";
> +static const char PERF_LOG_SUFFIX[] = ".XXXXXX";
> +
>  static char *setup_shell_logs(const char *name)
>  {
>  	char template[PATH_MAX];
>  	char *temp_dir;
> +	size_t i, len;
>  
> -	if (snprintf(template, PATH_MAX, "/tmp/perf_test_%s.XXXXXX", name) >= PATH_MAX) {
> +	if (snprintf(template, PATH_MAX, "%s%s%s", PERF_LOG_PREFIX, name,
> +		     PERF_LOG_SUFFIX) >= PATH_MAX) {
>  		pr_err("Failed to create log dir template");
>  		return NULL; /* Skip the testsuite */
>  	}
>  
> +	/* Replace non-alphanumeric characters with _ in the name part */
> +	len = strlen(template);
> +	for (i = sizeof(PERF_LOG_PREFIX) - 1; i < len - (sizeof(PERF_LOG_SUFFIX) - 1); i++) {
> +		if (!isalnum((unsigned char)template[i]))
> +			template[i] = '_';
> +	}
> +
>  	temp_dir = mkdtemp(template);
>  	if (temp_dir) {
>  		setenv("PERFSUITE_RUN_DIR", temp_dir, 1);

If strdup(temp_dir) fails right after this and returns NULL, do we leave the
newly created temporary directory on the filesystem? 

It looks like mkdtemp() creates the directory, but there is no cleanup if the
memory allocation fails. Additionally, should we unset the PERFSUITE_RUN_DIR
environment variable on this error path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416111419.385010-1-jbrnak@redhat.com?part=5