Re: [PATCH 02/15] perf debuginfo: Set DEBUGINFOD_URLS from /etc/debuginfod when unset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf debuginfo: bypass of /etc/debuginfod fallback for record/buildid-cache
--

commit abf1731695e2130d2a1cbc46b0ed7ae8527966ac
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf debuginfo: Set DEBUGINFOD_URLS from /etc/debuginfod when unset

This commit allows perf to read DEBUGINFOD_URLS from the .urls files in
/etc/debuginfod during early setup if the environment variable is not already
configured. This aims to resolve debuginfod failures in environments like
cron jobs, systemd services, or CI pipelines where distro setup scripts
might not populate the environment.

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
[ ... ]
>  void perf_debuginfod_setup(struct perf_debuginfod *di)
>  {
[ ... ]
> -	if (!di->set)
> -		setenv("DEBUGINFOD_URLS", "", 1);
> -	else if (di->urls && strcmp(di->urls, "system"))
> -		setenv("DEBUGINFOD_URLS", di->urls, 1);
> +	if (!di->set) {
> +		debuginfod__set_urls_env("");
> +	} else if (di->urls == NULL || !strcmp(di->urls, "system")) {
> +		const char *urls = getenv(DEBUGINFOD_URLS_ENV);
> +
> +		if (urls != NULL)
> +			debuginfod__set_urls_env(urls);
> +	} else {
> +		debuginfod__set_urls_env(di->urls);
> +	}
>  
> -	pr_debug("DEBUGINFOD_URLS=%s\n", getenv("DEBUGINFOD_URLS"));
> +	debuginfod__urls_set_by_tool = true;

[Severity: Medium]
When perf tools request debuginfod support with the default "system" URLs,
and the environment variable is unset (urls == NULL), does unconditionally
setting debuginfod__urls_set_by_tool to true here unintentionally lock out
the fallback mechanism?

[ ... ]
> +#ifdef HAVE_DEBUGINFOD_SUPPORT
> +/*
> + * The debuginfod client checks its local cache only as part of the
[ ... ]
> +void debuginfod__setup_urls_env(void)
> +{
> +	const char *env_urls;
> +	char *urls = NULL;
> +	DIR *dir;
> +	struct dirent *dent;
> +
> +	/*
> +	 * Tools that set DEBUGINFOD_URLS themselves already made their choice,
> +	 * leave it alone.
> +	 */
> +	if (debuginfod__urls_set_by_tool)
> +		return;

[Severity: Medium]
Because debuginfod__urls_set_by_tool is flagged to true earlier in
perf_debuginfod_setup(), will tools like perf record and perf buildid-cache
that run with --debuginfod exit early here?

It looks like this sequence causes these tools to entirely bypass the new
/etc/debuginfod fallback logic when DEBUGINFOD_URLS is unset, which seems
to defeat the purpose of the patch for these subcommands.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917155528.62607-1-acme@kernel.org?part=2