Re: [PATCH] perf tools: fix bug in usage of the basename() function

[Ingo Molnar <mingo@kernel.org>]

* Stephane Eranian <eranian@google.com> wrote:

> 
> The basename() implementation varies a lot between systems.
> The Linux man page says: "basename may modify the content of the path,
> so it may be desirable to pass a copy when calling the function".
> On some other systems, the returned address may come from an internal
> buffer which can be reused in subsequent calls, thus the results should
> also be copied.
> 
> The dso__set_basename() function was not doing this causing problems
> on some systems with wrong library names being shown by perf report,
> such as on Android systems.
> 
> This patch fixes the problem.
> Thanks to Ben Cheng for tracking down the problem.
> 
> Patch relative to tip.git at commit 631d5ea.
> 
> Reported-by: Ben Cheng <bccheng@google.com>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---

Just three nits:

>  tools/perf/util/dso.c |   29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index af4c687c..d186ace 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -404,7 +404,34 @@ void dso__set_short_name(struct dso *dso, const char *name)
>  
>  static void dso__set_basename(struct dso *dso)
>  {
> -	dso__set_short_name(dso, basename(dso->long_name));
> +	char *lname, *base;
> +
> +	/*
> +	 * basename may modify path buffer, so we must pass
> +	 * a copy.

s/basename may modify path buffer
 /basename() may modify the path buffer

> +	 */
> +	lname = strdup(dso->long_name);
> +	if (!lname)
> +		return;
> +
> +	/*
> +	 * basename may return pointer to internal
> +	 * storage which is reused in subsequent calls
> +	 * so copy the result
> +	 */

s/basename may return pointer
 /basename() may return a pointer

Makes for easier reading.

(Also please use consistent periods - the first comment has a period, 
the second one doesn't. ':' works well too:)


> +	base = strdup(basename(lname));
> +
> +	free(lname);
> +
> +	if (!base)
> +		return;
> +
> +	if (dso->sname_alloc)
> +		free((char *)dso->short_name);

That cast is probably not needed.

> +	else
> +		dso->sname_alloc = 1;
> +
> +	dso__set_short_name(dso, base);
>  }
>  
>  int dso__name_len(const struct dso *dso)

Otherwise:

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo