On Thu, Dec 5, 2013 at 9:59 AM, Ingo Molnar <[email protected]> wrote: > > * Stephane Eranian <[email protected]> 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 <[email protected]> >> Signed-off-by: Stephane Eranian <[email protected]> >> --- > > 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:) > Fixed that.
> >> + base = strdup(basename(lname)); >> + >> + free(lname); >> + >> + if (!base) >> + return; >> + >> + if (dso->sname_alloc) >> + free((char *)dso->short_name); > > That cast is probably not needed. > It is with my compiler. It prints out a warning otherwise. >> + else >> + dso->sname_alloc = 1; >> + >> + dso__set_short_name(dso, base); >> } >> >> int dso__name_len(const struct dso *dso) > > Otherwise: > > Acked-by: Ingo Molnar <[email protected]> > > Thanks, > > Ingo Reposting ASAP. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
