Re: [PATCH] perf tools: fix bug in usage of the basename() function
* Stephane Eranian wrote: > >> + 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. Yeah, see my previous mail, I think having dso->short_name as 'const' is a mistake, as there are really just two main usecases for methods that operate on 'struct dso': - life time affecting (setup/free) methods which need access to all fields, which don't want dso->short_name as a const (as evidenced by the cast). - actual usage methods that get a 'const struct dso' anyway, so they don't need dso->short_name as a const. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: fix bug in usage of the basename() function
On Thu, Dec 5, 2013 at 9:59 AM, Ingo Molnar wrote: > > * Stephane Eranian 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 >> Signed-off-by: Stephane Eranian >> --- > > 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 > > Thanks, > > Ingo Reposting ASAP. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: fix bug in usage of the basename() function
* Stephane Eranian 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 > Signed-off-by: Stephane Eranian > --- 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 Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/