lkml.org 
[lkml]   [2012]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> > On Tue, 25 Sep 2012 11:11:21 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> > > Ditto. Plus it might leak previous input_name.
> >
> > Nice catch, will check the return value of "strdup".
> >
> > For input_name mem leak, in some cases the input_name can't be called
> > with free(), like those got from parse "-i" option. In case the old
> > input_name is got from malloc through strdup, I think it's not a big
> > issue given that buffer will be freed any way when the application exit.
>
> I think this is a matter of discipline, leaking is bad, kernel or
> userspace, why special case it?
>

I see, then we need make sure all "input_name" point to a malloced buffer,
here is a initial debug patch will only touch the annotate/report/script
cmds, pls review and more suggestions are welcomed:

----
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index b3d60ae..5165660 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -288,6 +288,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)

argc = parse_options(argc, argv, options, annotate_usage, 0);

+ try_dup_input_name();
+
if (annotate.use_stdio)
use_browser = 0;
else if (annotate.use_tui)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b4d216b..bec614c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -668,6 +668,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
input_name = "perf.data";
}

+ try_dup_input_name();
+
repeat:
session = perf_session__new(input_name, O_RDONLY,
report.force, false, &report.tool);
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 30e880c..f13df66 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1477,6 +1477,8 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
if (!script_name)
setup_pager();

+ try_dup_input_name();
+
session = perf_session__new(input_name, O_RDONLY, 0, false,
&perf_script);
if (session == NULL)
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 2a09de0..d6a097e 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -64,6 +64,20 @@ struct pager_config {
int val;
};

+/*
+ * "input_name" may point to a memory regsion got or not got from
+ * malloc(), this fun will enforce it to point to a malloced region,
+ * this is to avoid memory leak when runtime siwtching perf data file.
+ */
+void try_dup_input_name(void)
+{
+ if (input_name) {
+ input_name = strdup(input_name);
+ if (!input_name)
+ printf("Warning: Fail to duplicate input_name\n");
+ }
+}
+
static int pager_command_config(const char *var, const char *value, void *data)
{
struct pager_config *c = data;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 42da932..1159213 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,6 +207,7 @@ extern bool perf_host, perf_guest;
extern const char perf_version_string[];

void pthread__unblock_sigwinch(void);
+void try_dup_input_name(void);

#include "util/target.h"

Thanks,
Feng



\
 
 \ /
  Last update: 2012-09-26 10:41    [W:0.140 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site