Re: [PATCH v3 8/9] perf hists browser: Add option for runtimeswitching perf data file

[Feng Tang]

Hi Namhyung,

On Thu, Sep 27, 2012 at 01:02:05PM +0900, Namhyung Kim wrote:
> Hi Feng,
> 
> On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
> > 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@xxxxxxxxxx> 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:
> 
> Well, how about adding a flag like "input_name_alloced" and checking it
> before new allocation?  This way we can avoid needless strdup when
> runtime switching is not used.

Right, myself have checked that only the runtime switch will malloc a
buffer for the new "input_name", IOW "input_name" in other case can't be
called with free(). So one flag only inside hists.c is ok to cover this,
like the following code.

btw, thanks for your patch about the parse-events()! will try it out.

- Feng

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6048820..c29335a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,13 @@ static inline void free_popup_options(char **options, int n)
 	}
 }
 
+/*
+ * Only runtime switching of perf data file will make "input_name" point
+ * to a malloced buffer. So add "is_input_name_malloced" flag to decide
+ * whether we need to call free() for current "input_name" during the switch.
+ */
+static bool is_input_name_malloced = false;
+
 static int switch_data_file(void)
 {
 	char *pwd, *options[256], *abs_path[256], *tmp;
@@ -1186,8 +1193,10 @@ close_file_and_continue:
 		if (choice < nr_options && choice >= 0) {
 			tmp = strdup(abs_path[choice]);
 			if (tmp) {
+				if (is_input_name_malloced)
+					free((void *)input_name);
 				input_name = tmp;
+				is_input_name_malloced = true;
 				ret = 0;
 			}
 		}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/