Messages in this thread | | | Subject | Re: [PATCH] perf config: fix caching and memory leak in perf_home_perfconfig | From | Riccardo Mancini <> | Date | Fri, 20 Aug 2021 21:58:16 +0200 |
| |
Hi,
On Fri, 2021-08-20 at 11:13 -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Aug 20, 2021 at 03:08:17PM +0200, Riccardo Mancini escreveu: > > Acaict, perf_home_perfconfig is supposed to cache the result of > > home_perfconfig, which returns the default location of perfconfig for > > the user, given the HOME environment variable. > > However, the current implementation calls home_perfconfig every time > > perf_home_perfconfig is called (so no caching is actually performed), > > replacing the previous pointer, thus also causing a memory leak. > > > > This patch adds a check of whether either config or failed is set and, > > in that case, directly returns config without calling home_perfconfig at > > each invocation. > > > > Cc: Jiri Olsa <jolsa@kernel.org> > > Fixes: f5f03e19ce14fc31 ("perf config: Add perf_home_perfconfig function") > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com> > > --- > > tools/perf/util/config.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > > index 63d472b336de21d4..6ab670cdf512507e 100644 > > --- a/tools/perf/util/config.c > > +++ b/tools/perf/util/config.c > > @@ -581,6 +581,9 @@ const char *perf_home_perfconfig(void) > > static const char *config; > > static bool failed; > > > > + if (config || failed) > > + return config; > > + > > config = failed ? NULL : home_perfconfig(); > > humm, why keep the above failed test then?
Sorry, I forgot about that, it should be removed.
> > > if (!config) > > failed = true; > > I.e. please check this: > > > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > index 63d472b336de21d4..4fb5e90d7a57ae48 100644 > --- a/tools/perf/util/config.c > +++ b/tools/perf/util/config.c > @@ -581,7 +581,10 @@ const char *perf_home_perfconfig(void) > static const char *config; > static bool failed; > > - config = failed ? NULL : home_perfconfig(); > + if (failed || config) > + return config; > + > + config = home_perfconfig(); > if (!config) > failed = true; >
Looks good to me. Shall I resend it fixed?
Thanks, Riccardo
| |