[PATCH v2 0/4] Add ftrace config and refactor several parts

[PATCH v2 0/4] Add ftrace config and refactor several parts

[Taeung Song]

Hi, :)

Lately I sent several patches.
But they need to be modified so I send v2 packing them.

v2:
 - Check the return of perf_config() and
 warn the user with config error message (Arnaldo)
 - Change commit messages (Jiri)
 - Add only one macro instead of two macros (Jiri) 

Taeung Song (4):
  perf tools: Create for_each_event macro for tracepoints iteration
  perf ftrace: Add ftrace.tracer config option
  perf tools: Check NULL after zalloc() and Use zfree() instead of
    free() in parse-events.c
  perf tools: Increase index if perf_evsel__new_idx() succeeded

 tools/perf/builtin-ftrace.c        | 25 +++++++++++++++++++++++++
 tools/perf/util/parse-events.c     | 23 ++++++++++++-----------
 tools/perf/util/trace-event-info.c | 38 ++++++++++++++++++--------------------
 3 files changed, 55 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] perf tools: Create for_each_event macro for tracepoints iteration

[Taeung Song]

Such as for_each_subsystem and for_each_event in util/parse-events.c,
add new macros 'for_each_event' for easy iteration over the tracepoints
in order to be more compact and readable.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/trace-event-info.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index ceb0e27..fb6d95d 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -170,6 +170,12 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
 	return false;
 }
 
+#define for_each_event(dir, dent, tps)				\
+	while ((dent = readdir(dir)))				\
+		if (dent->d_type == DT_DIR &&			\
+		    (strcmp(dent->d_name, ".")) &&		\
+		    (strcmp(dent->d_name, "..")))		\
+
 static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
 	struct dirent *dent;
@@ -186,12 +192,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 		return -errno;
 	}
 
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
+	for_each_event(dir, dent, tps) {
+		if (!name_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;
@@ -210,12 +214,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 	}
 
 	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
+	for_each_event(dir, dent, tps) {
+		if (!name_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;
@@ -290,13 +292,11 @@ static int record_event_files(struct tracepoint_path *tps)
 		goto out;
 	}
 
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
+	for_each_event(dir, dent, tps) {
+		if (!strcmp(dent->d_name, "ftrace") ||
 		    !system_in_tp_list(dent->d_name, tps))
 			continue;
+
 		count++;
 	}
 
@@ -307,13 +307,11 @@ static int record_event_files(struct tracepoint_path *tps)
 	}
 
 	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
+	for_each_event(dir, dent, tps) {
+		if (!strcmp(dent->d_name, "ftrace") ||
 		    !system_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&sys, "%s/%s", path, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;
-- 
2.7.4

Re: [PATCH v2 1/4] perf tools: Create for_each_event macro for tracepoints iteration

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 08:38:28PM +0900, Taeung Song escreveu:
> Such as for_each_subsystem and for_each_event in util/parse-events.c,
> add new macros 'for_each_event' for easy iteration over the tracepoints
> in order to be more compact and readable.

Looks ok, applied, but look below for some minor suggestions.

- Arnaldo
 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/trace-event-info.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index ceb0e27..fb6d95d 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -170,6 +170,12 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
>  	return false;
>  }
>  
> +#define for_each_event(dir, dent, tps)				\
> +	while ((dent = readdir(dir)))				\
> +		if (dent->d_type == DT_DIR &&			\
> +		    (strcmp(dent->d_name, ".")) &&		\
> +		    (strcmp(dent->d_name, "..")))		\
> +
>  static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>  {
>  	struct dirent *dent;
> @@ -186,12 +192,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>  		return -errno;
>  	}
>  
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    !name_in_tp_list(dent->d_name, tps))
> +	for_each_event(dir, dent, tps) {
> +		if (!name_in_tp_list(dent->d_name, tps))
>  			continue;
> +
>  		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
>  			err = -ENOMEM;
>  			goto out;
> @@ -210,12 +214,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>  	}
>  
>  	rewinddir(dir);
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    !name_in_tp_list(dent->d_name, tps))
> +	for_each_event(dir, dent, tps) {
> +		if (!name_in_tp_list(dent->d_name, tps))
>  			continue;
> +
>  		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
>  			err = -ENOMEM;
>  			goto out;
> @@ -290,13 +292,11 @@ static int record_event_files(struct tracepoint_path *tps)
>  		goto out;
>  	}
>  
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    strcmp(dent->d_name, "ftrace") == 0 ||
> +	for_each_event(dir, dent, tps) {
> +		if (!strcmp(dent->d_name, "ftrace") ||

the existing style was == 0, you switched it to !, equivalent, but
gratuitous, keeping the existing style would make reviewing slightly
faster, as the pattern wouldn't have changed.

>  		    !system_in_tp_list(dent->d_name, tps))
>  			continue;
> +
>  		count++;
>  	}
>  
> @@ -307,13 +307,11 @@ static int record_event_files(struct tracepoint_path *tps)
>  	}
>  
>  	rewinddir(dir);
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    strcmp(dent->d_name, "ftrace") == 0 ||
> +	for_each_event(dir, dent, tps) {
> +		if (!strcmp(dent->d_name, "ftrace") ||

Ditto.

>  		    !system_in_tp_list(dent->d_name, tps))
>  			continue;
> +
>  		if (asprintf(&sys, "%s/%s", path, dent->d_name) < 0) {
>  			err = -ENOMEM;
>  			goto out;
> -- 
> 2.7.4

Re: [PATCH v2 1/4] perf tools: Create for_each_event macro for tracepoints iteration

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 09:21:16AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jan 31, 2017 at 08:38:28PM +0900, Taeung Song escreveu:
> > -	while ((dent = readdir(dir))) {
> > -		if (dent->d_type != DT_DIR ||
> > -		    strcmp(dent->d_name, ".") == 0 ||
> > -		    strcmp(dent->d_name, "..") == 0 ||
> > -		    strcmp(dent->d_name, "ftrace") == 0 ||
> > +	for_each_event(dir, dent, tps) {
> > +		if (!strcmp(dent->d_name, "ftrace") ||
> >  		    !system_in_tp_list(dent->d_name, tps))
> >  			continue;

> the existing style was == 0, you switched it to !, equivalent, but
> gratuitous, keeping the existing style would make reviewing slightly
> faster, as the pattern wouldn't have changed.
 
Here it is:

-       while ((dent = readdir(dir))) {
-               if (dent->d_type != DT_DIR ||
-                   strcmp(dent->d_name, ".") == 0 ||
-                   strcmp(dent->d_name, "..") == 0 ||
-                   strcmp(dent->d_name, "ftrace") == 0 ||
+       for_each_event(dir, dent, tps) {
+               if (strcmp(dent->d_name, "ftrace") == 0 ||
                    !system_in_tp_list(dent->d_name, tps))
                        continue;

Re: [PATCH v2 1/4] perf tools: Create for_each_event macro for tracepoints iteration

[Steven Rostedt]

On Tue, 31 Jan 2017 09:23:48 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Tue, Jan 31, 2017 at 09:21:16AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Jan 31, 2017 at 08:38:28PM +0900, Taeung Song escreveu:  
> > > -	while ((dent = readdir(dir))) {
> > > -		if (dent->d_type != DT_DIR ||
> > > -		    strcmp(dent->d_name, ".") == 0 ||
> > > -		    strcmp(dent->d_name, "..") == 0 ||
> > > -		    strcmp(dent->d_name, "ftrace") == 0 ||
> > > +	for_each_event(dir, dent, tps) {
> > > +		if (!strcmp(dent->d_name, "ftrace") ||
> > >  		    !system_in_tp_list(dent->d_name, tps))
> > >  			continue;  
> 
> > the existing style was == 0, you switched it to !, equivalent, but
> > gratuitous, keeping the existing style would make reviewing slightly
> > faster, as the pattern wouldn't have changed.  
>  
> Here it is:
> 
> -       while ((dent = readdir(dir))) {
> -               if (dent->d_type != DT_DIR ||
> -                   strcmp(dent->d_name, ".") == 0 ||
> -                   strcmp(dent->d_name, "..") == 0 ||
> -                   strcmp(dent->d_name, "ftrace") == 0 ||
> +       for_each_event(dir, dent, tps) {
> +               if (strcmp(dent->d_name, "ftrace") == 0 ||
>                     !system_in_tp_list(dent->d_name, tps))
>                         continue;

Thanks, because I always screw up the !strcmp(). Thus I find the "== 0"
to me is easier to process "matches" and "!= 0" is "doesn't match".

-- Steve

Re: [PATCH v2 1/4] perf tools: Create for_each_event macro for tracepoints iteration

[Taeung Song]

Hi, Arnaldo :)

On 01/31/2017 09:21 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 31, 2017 at 08:38:28PM +0900, Taeung Song escreveu:
>> Such as for_each_subsystem and for_each_event in util/parse-events.c,
>> add new macros 'for_each_event' for easy iteration over the tracepoints
>> in order to be more compact and readable.
>
> Looks ok, applied, but look below for some minor suggestions.
>
> - Arnaldo
>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/util/trace-event-info.c | 38 ++++++++++++++++++--------------------
>>  1 file changed, 18 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
>> index ceb0e27..fb6d95d 100644
>> --- a/tools/perf/util/trace-event-info.c
>> +++ b/tools/perf/util/trace-event-info.c
>> @@ -170,6 +170,12 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
>>  	return false;
>>  }
>>
>> +#define for_each_event(dir, dent, tps)				\
>> +	while ((dent = readdir(dir)))				\
>> +		if (dent->d_type == DT_DIR &&			\
>> +		    (strcmp(dent->d_name, ".")) &&		\
>> +		    (strcmp(dent->d_name, "..")))		\
>> +
>>  static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>>  {
>>  	struct dirent *dent;
>> @@ -186,12 +192,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>>  		return -errno;
>>  	}
>>
>> -	while ((dent = readdir(dir))) {
>> -		if (dent->d_type != DT_DIR ||
>> -		    strcmp(dent->d_name, ".") == 0 ||
>> -		    strcmp(dent->d_name, "..") == 0 ||
>> -		    !name_in_tp_list(dent->d_name, tps))
>> +	for_each_event(dir, dent, tps) {
>> +		if (!name_in_tp_list(dent->d_name, tps))
>>  			continue;
>> +
>>  		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
>>  			err = -ENOMEM;
>>  			goto out;
>> @@ -210,12 +214,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
>>  	}
>>
>>  	rewinddir(dir);
>> -	while ((dent = readdir(dir))) {
>> -		if (dent->d_type != DT_DIR ||
>> -		    strcmp(dent->d_name, ".") == 0 ||
>> -		    strcmp(dent->d_name, "..") == 0 ||
>> -		    !name_in_tp_list(dent->d_name, tps))
>> +	for_each_event(dir, dent, tps) {
>> +		if (!name_in_tp_list(dent->d_name, tps))
>>  			continue;
>> +
>>  		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
>>  			err = -ENOMEM;
>>  			goto out;
>> @@ -290,13 +292,11 @@ static int record_event_files(struct tracepoint_path *tps)
>>  		goto out;
>>  	}
>>
>> -	while ((dent = readdir(dir))) {
>> -		if (dent->d_type != DT_DIR ||
>> -		    strcmp(dent->d_name, ".") == 0 ||
>> -		    strcmp(dent->d_name, "..") == 0 ||
>> -		    strcmp(dent->d_name, "ftrace") == 0 ||
>> +	for_each_event(dir, dent, tps) {
>> +		if (!strcmp(dent->d_name, "ftrace") ||
>
> the existing style was == 0, you switched it to !, equivalent, but
> gratuitous, keeping the existing style would make reviewing slightly
> faster, as the pattern wouldn't have changed.
>

I understood !!
In the future, I'll think this when making some patches.

Thanks,
Taeung

[tip:perf/core] perf tools: Create for_each_event macro for tracepoints iteration

[tip-bot for Taeung Song]

Commit-ID:  43d41deb71fe1850264e5dd8109211683954ea14
Gitweb:     http://git.kernel.org/tip/43d41deb71fe1850264e5dd8109211683954ea14
Author:     Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Tue, 31 Jan 2017 20:38:28 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 31 Jan 2017 16:20:08 -0300

perf tools: Create for_each_event macro for tracepoints iteration

Similar to for_each_subsystem and for_each_event in util/parse-events.c,
add new macro 'for_each_event' for easy iteration over the tracepoints
in order to be more compact and readable.

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1485862711-20216-2-git-send-email-treeze.taeung@gmail.com
[ Slight change to keep existing style for checking strcmp() return ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/trace-event-info.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index ceb0e27..e7d60d0 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -170,6 +170,12 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
 	return false;
 }
 
+#define for_each_event(dir, dent, tps)				\
+	while ((dent = readdir(dir)))				\
+		if (dent->d_type == DT_DIR &&			\
+		    (strcmp(dent->d_name, ".")) &&		\
+		    (strcmp(dent->d_name, "..")))		\
+
 static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
 	struct dirent *dent;
@@ -186,12 +192,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 		return -errno;
 	}
 
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
+	for_each_event(dir, dent, tps) {
+		if (!name_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;
@@ -210,12 +214,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 	}
 
 	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
+	for_each_event(dir, dent, tps) {
+		if (!name_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&format, "%s/%s/format", sys, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;
@@ -290,13 +292,11 @@ static int record_event_files(struct tracepoint_path *tps)
 		goto out;
 	}
 
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
+	for_each_event(dir, dent, tps) {
+		if (strcmp(dent->d_name, "ftrace") == 0 ||
 		    !system_in_tp_list(dent->d_name, tps))
 			continue;
+
 		count++;
 	}
 
@@ -307,13 +307,11 @@ static int record_event_files(struct tracepoint_path *tps)
 	}
 
 	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
+	for_each_event(dir, dent, tps) {
+		if (strcmp(dent->d_name, "ftrace") == 0 ||
 		    !system_in_tp_list(dent->d_name, tps))
 			continue;
+
 		if (asprintf(&sys, "%s/%s", path, dent->d_name) < 0) {
 			err = -ENOMEM;
 			goto out;

[PATCH v2 2/4] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

    # cat ~/.perfconfig
    [ftrace]
        tracer = function

    # perf ftrace usleep 123456 | head -10
      <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
      <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
      <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
      <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-ftrace.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 414444d..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"
 
 
 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,26 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
 	return done ? 0 : -1;
 }
 
+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+	struct perf_ftrace *ftrace = cb;
+
+	if (!strcmp(var, "ftrace.tracer")) {
+		if (!strcmp(value, "function_graph"))
+			ftrace->tracer = DEFAULT_TRACER;
+		else if (!strcmp(value, "function"))
+			ftrace->tracer = "function";
+		else {
+			pr_err("Please select function_graph(default)"
+			       "or function to use tracer.\n");
+			return -1;
+		}
+		return 0;
+	}
+
+	return 0;
+}
+
 int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret;
@@ -218,6 +239,10 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_END()
 	};
 
+	ret = perf_config(perf_ftrace_config, &ftrace);
+	if (ret < 0)
+		return -1;
+
 	argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc)
-- 
2.7.4

Re: [PATCH v2 2/4] perf ftrace: Add ftrace.tracer config option

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 08:38:29PM +0900, Taeung Song escreveu:
> Currently perf ftrace command will select 'function_graph' or 'function'.
> So add ftrace.tracer config option to select tracer

The above is confusing, I changed it to:

    Currently 'perf ftrace' command allows selecting 'function_graph' or
    'function', defaulting to 'function_graph'.
    
    Add the ftrace.tracer config option to select the default tracer:
 
>     # cat ~/.perfconfig
>     [ftrace]
>         tracer = function
> 
>     # perf ftrace usleep 123456 | head -10
>       <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
>       <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
>       <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
>       <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock
> 
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-ftrace.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 414444d..00e228f 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -17,6 +17,7 @@
>  #include "evlist.h"
>  #include "target.h"
>  #include "thread_map.h"
> +#include "util/config.h"
>  
>  
>  #define DEFAULT_TRACER  "function_graph"
> @@ -198,6 +199,26 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
>  	return done ? 0 : -1;
>  }
>  
> +static int perf_ftrace_config(const char *var, const char *value, void *cb)
> +{
> +	struct perf_ftrace *ftrace = cb;
> +
> +	if (!strcmp(var, "ftrace.tracer")) {
> +		if (!strcmp(value, "function_graph"))
> +			ftrace->tracer = DEFAULT_TRACER;
> +		else if (!strcmp(value, "function"))
> +			ftrace->tracer = "function";
> +		else {
> +			pr_err("Please select function_graph(default)"
> +			       "or function to use tracer.\n");
> +			return -1;
> +		}

If you use {} in the else case, use it in the other branch, also I
simplified it to be:

+	if (!strcmp(var, "ftrace.tracer")) {
+		if (!strcmp(value, "function_graph") ||
+		    !strcmp(value, "function"))
+			ftrace->tracer = value;
+		else {
+			pr_err("Please select function_graph(default)"
+			       "or function to use tracer.\n");
+			return -1;
+		}

As we know item->value will not go away (its a perf_config_item,
allocated and kept in that perf_config_set structure).

Also it doesn't make sense comparing against "function_graph" to set it
to DEFAULT_TRACER, as if we decide to change the DEFAULT_TRACER this
simply breaks.

Also you forgot to test by setting an invalid tracer, I did and noticed
that you forgot to add a space between "(default)" and "or ", please be
more careful with testing.

Also you are silently ignoring any unknown variable in this section, so
if someone has this:

cat ~/.perfconfig

[ftrace]

	trace = function


I.e. forgets the 'r', it will keep using the current default,
"function_graph" till the user checks the config and adds that missing
'r'...

I haven't changed this because I think this is so common that we should
instead have a different error return for those config callbacks that
will tell perf_config() to say something like:

  "unknown variable %s.%s\n", section_name, variable_name

- Arnaldo


> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
>  int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int ret;
> @@ -218,6 +239,10 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_END()
>  	};
>  
> +	ret = perf_config(perf_ftrace_config, &ftrace);
> +	if (ret < 0)
> +		return -1;
> +
>  	argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
>  	if (!argc)
> -- 
> 2.7.4

Re: [PATCH v2 2/4] perf ftrace: Add ftrace.tracer config option

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 09:46:28AM -0300, Arnaldo Carvalho de Melo escreveu:
> Also you are silently ignoring any unknown variable in this section, so
> if someone has this:
> 
> cat ~/.perfconfig
> 
> [ftrace]
> 
> 	trace = function
> 
> 
> I.e. forgets the 'r', it will keep using the current default,
> "function_graph" till the user checks the config and adds that missing
> 'r'...
> 
> I haven't changed this because I think this is so common that we should
> instead have a different error return for those config callbacks that
> will tell perf_config() to say something like:
> 
>   "unknown variable %s.%s\n", section_name, variable_name

So I read a bit more the config code and ended up with this:

static int perf_ftrace_config(const char *var, const char *value, void *cb)
{
        struct perf_ftrace *ftrace = cb;

        if (prefixcmp(var, "ftrace."))
                return 0;

        if (strcmp(var, "ftrace.tracer"))
                return -1;

        if (!strcmp(value, "function_graph") ||
            !strcmp(value, "function")) {
                ftrace->tracer = value;
                return 0;
        }

	pr_err("Please select \"function_graph\" (default) or \"function\"\n");
        return -1;
}

I.e. it will ignore variables for other sections and will stop at an unknown
variable or value for ftrace.tracer:

[root@jouet ~]# cat ~/.perfconfig 
[ftrace]
	trace = function
[root@jouet ~]# perf ftrace usleep 1
Error: wrong config key-value pair ftrace.trace=function
[root@jouet ~]# cat ~/.perfconfig 
[ftrace]
	tracer = functin
[root@jouet ~]# perf ftrace usleep 1
Please select "function_graph" (default) or "function"
Error: wrong config key-value pair ftrace.tracer=functin
[root@jouet ~]# cat ~/.perfconfig 
[ftrace]
	tracer = function
[root@jouet ~]# perf ftrace usleep 1 | head -5
          <idle>-0     [000] d...  3855.820847: switch_mm_irqs_off <-__schedule
           <...>-18550 [000] d...  3855.820849: finish_task_switch <-__schedule
           <...>-18550 [000] d...  3855.820851: smp_irq_work_interrupt <-irq_work_interrupt
           <...>-18550 [000] d...  3855.820851: irq_enter <-smp_irq_work_interrupt
           <...>-18550 [000] d...  3855.820851: rcu_irq_enter <-irq_enter
[root@jouet ~]#

- Arnaldo

Re: [PATCH v2 2/4] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

On 01/31/2017 10:15 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 31, 2017 at 09:46:28AM -0300, Arnaldo Carvalho de Melo escreveu:
>> Also you are silently ignoring any unknown variable in this section, so
>> if someone has this:
>>
>> cat ~/.perfconfig
>>
>> [ftrace]
>>
>> 	trace = function
>>
>>
>> I.e. forgets the 'r', it will keep using the current default,
>> "function_graph" till the user checks the config and adds that missing
>> 'r'...
>>
>> I haven't changed this because I think this is so common that we should
>> instead have a different error return for those config callbacks that
>> will tell perf_config() to say something like:
>>
>>   "unknown variable %s.%s\n", section_name, variable_name
>
> So I read a bit more the config code and ended up with this:
>
> static int perf_ftrace_config(const char *var, const char *value, void *cb)
> {
>         struct perf_ftrace *ftrace = cb;
>
>         if (prefixcmp(var, "ftrace."))
>                 return 0;
>
>         if (strcmp(var, "ftrace.tracer"))
>                 return -1;
>
>         if (!strcmp(value, "function_graph") ||
>             !strcmp(value, "function")) {
>                 ftrace->tracer = value;
>                 return 0;
>         }
>
> 	pr_err("Please select \"function_graph\" (default) or \"function\"\n");
>         return -1;
> }
>
> I.e. it will ignore variables for other sections and will stop at an unknown
> variable or value for ftrace.tracer:
>
> [root@jouet ~]# cat ~/.perfconfig
> [ftrace]
> 	trace = function
> [root@jouet ~]# perf ftrace usleep 1
> Error: wrong config key-value pair ftrace.trace=function
> [root@jouet ~]# cat ~/.perfconfig
> [ftrace]
> 	tracer = functin
> [root@jouet ~]# perf ftrace usleep 1
> Please select "function_graph" (default) or "function"
> Error: wrong config key-value pair ftrace.tracer=functin
> [root@jouet ~]# cat ~/.perfconfig
> [ftrace]
> 	tracer = function
> [root@jouet ~]# perf ftrace usleep 1 | head -5
>           <idle>-0     [000] d...  3855.820847: switch_mm_irqs_off <-__schedule
>            <...>-18550 [000] d...  3855.820849: finish_task_switch <-__schedule
>            <...>-18550 [000] d...  3855.820851: smp_irq_work_interrupt <-irq_work_interrupt
>            <...>-18550 [000] d...  3855.820851: irq_enter <-smp_irq_work_interrupt
>            <...>-18550 [000] d...  3855.820851: rcu_irq_enter <-irq_enter
> [root@jouet ~]#
>

This is great!! I agonized about error message for
each other cases as above! But I think this is more precise than my patch!

Thanks,
Taeung

[tip:perf/core] perf ftrace: Add ftrace.tracer config option

[tip-bot for Taeung Song]

Commit-ID:  b05d1093987a78695766b71a2d723aa65b5c25c5
Gitweb:     http://git.kernel.org/tip/b05d1093987a78695766b71a2d723aa65b5c25c5
Author:     Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Tue, 31 Jan 2017 20:38:29 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 31 Jan 2017 16:20:09 -0300

perf ftrace: Add ftrace.tracer config option

Currently 'perf ftrace' command allows selecting 'function_graph' or
'function', defaulting to 'function_graph'.

Add ftrace.tracer config option to select the default tracer:

    # cat ~/.perfconfig
    [ftrace]
        tracer = function

    # perf ftrace usleep 123456 | head -10
      <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
      <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
      <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
      <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock

Committer notes:

Retesting it with invalid variables, invalid values for ftrace.tracer,
and a valid one:

  # cat ~/.perfconfig
  [ftrace]
        trace = function
  # perf ftrace usleep 1
  Error: wrong config key-value pair ftrace.trace=function
  # cat ~/.perfconfig
  [ftrace]
        tracer = functin
  # perf ftrace usleep 1
  Please select "function_graph" (default) or "function"
  Error: wrong config key-value pair ftrace.tracer=functin
  # cat ~/.perfconfig
  [ftrace]
        tracer = function
  # perf ftrace usleep 1 | head -5
          <idle>-0     [000] d...  3855.820847: switch_mm_irqs_off <-__schedule
           <...>-18550 [000] d...  3855.820849: finish_task_switch <-__schedule
           <...>-18550 [000] d...  3855.820851: smp_irq_work_interrupt <-irq_work_interrupt
           <...>-18550 [000] d...  3855.820851: irq_enter <-smp_irq_work_interrupt
           <...>-18550 [000] d...  3855.820851: rcu_irq_enter <-irq_enter
  #

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1485862711-20216-3-git-send-email-treeze.taeung@gmail.com
[ Added missign space in error message, changed the logic to make it more compact and less error prone ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-ftrace.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 414444d..c3e6436 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"
 
 
 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,26 @@ out:
 	return done ? 0 : -1;
 }
 
+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+	struct perf_ftrace *ftrace = cb;
+
+	if (prefixcmp(var, "ftrace."))
+		return 0;
+
+	if (strcmp(var, "ftrace.tracer"))
+		return -1;
+
+	if (!strcmp(value, "function_graph") ||
+	    !strcmp(value, "function")) {
+		ftrace->tracer = value;
+		return 0;
+	}
+
+	pr_err("Please select \"function_graph\" (default) or \"function\"\n");
+	return -1;
+}
+
 int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret;
@@ -218,6 +239,10 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_END()
 	};
 
+	ret = perf_config(perf_ftrace_config, &ftrace);
+	if (ret < 0)
+		return -1;
+
 	argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc)

[PATCH v2 3/4] perf tools: Check NULL after zalloc() and Use zfree() instead of free() in parse-events.c

[Taeung Song]

Currently there are several parts not checking NULL
after allocating with zalloc() or asigning NULL value
to a pointer variable after doing free().

So I fill in code checking NULL and
use zfree() instead of free().

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/parse-events.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3c876b8..87a3e5a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -211,6 +211,8 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
 				closedir(evt_dir);
 				closedir(sys_dir);
 				path = zalloc(sizeof(*path));
+				if (!path)
+					return NULL;
 				path->system = malloc(MAX_EVENT_LENGTH);
 				if (!path->system) {
 					free(path);
@@ -252,8 +254,7 @@ struct tracepoint_path *tracepoint_name_to_path(const char *name)
 	if (path->system == NULL || path->name == NULL) {
 		zfree(&path->system);
 		zfree(&path->name);
-		free(path);
-		path = NULL;
+		zfree(&path);
 	}
 
 	return path;
@@ -1477,10 +1478,9 @@ static void perf_pmu__parse_cleanup(void)
 
 		for (i = 0; i < perf_pmu_events_list_num; i++) {
 			p = perf_pmu_events_list + i;
-			free(p->symbol);
+			zfree(&p->symbol);
 		}
-		free(perf_pmu_events_list);
-		perf_pmu_events_list = NULL;
+		zfree(&perf_pmu_events_list);
 		perf_pmu_events_list_num = 0;
 	}
 }
@@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name)
 	r = bsearch(&p, perf_pmu_events_list,
 			(size_t) perf_pmu_events_list_num,
 			sizeof(struct perf_pmu_event_symbol), comp_pmu);
-	free(p.symbol);
+	zfree(&p.symbol);
 	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
 }
 
@@ -1710,8 +1710,8 @@ static void parse_events_print_error(struct parse_events_error *err,
 		fprintf(stderr, "%*s\\___ %s\n", idx + 1, "", err->str);
 		if (err->help)
 			fprintf(stderr, "\n%s\n", err->help);
-		free(err->str);
-		free(err->help);
+		zfree(&err->str);
+		zfree(&err->help);
 	}
 
 	fprintf(stderr, "Run 'perf list' for a list of valid events\n");
@@ -2406,7 +2406,7 @@ void parse_events_terms__purge(struct list_head *terms)
 
 	list_for_each_entry_safe(term, h, terms, list) {
 		if (term->array.nr_ranges)
-			free(term->array.ranges);
+			zfree(&term->array.ranges);
 		list_del_init(&term->list);
 		free(term);
 	}
@@ -2422,7 +2422,7 @@ void parse_events_terms__delete(struct list_head *terms)
 
 void parse_events__clear_array(struct parse_events_array *a)
 {
-	free(a->ranges);
+	zfree(&a->ranges);
 }
 
 void parse_events_evlist_error(struct parse_events_evlist *data,
-- 
2.7.4

Re: [PATCH v2 3/4] perf tools: Check NULL after zalloc() and Use zfree() instead of free() in parse-events.c

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 08:38:30PM +0900, Taeung Song escreveu:
> Currently there are several parts not checking NULL
> after allocating with zalloc() or asigning NULL value
> to a pointer variable after doing free().
> 
> So I fill in code checking NULL and
> use zfree() instead of free().

You are doing more than one thing on this patch, don't do that.

Please split it for the "no functional changes" parts, i.e. things like

-		free(path);
-		path = NULL;
+		zfree(&path);

>From the ones that _change_ logic, like the first hunk:

 				path = zalloc(sizeof(*path));
+				if (!path)
+					return NULL;
 				path->system = malloc(MAX_EVENT_LENGTH);
 				if (!path->system) {
 					free(path);

This one as well changes logic:

@@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name)
 	r = bsearch(&p, perf_pmu_events_list,
 			(size_t) perf_pmu_events_list_num,
 			sizeof(struct perf_pmu_event_symbol), comp_pmu);
-	free(p.symbol);
+	zfree(&p.symbol);
 	return r ? r->type : PMU_EVENT_SYMBOL_ERR;

 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/parse-events.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 3c876b8..87a3e5a 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -211,6 +211,8 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
>  				closedir(evt_dir);
>  				closedir(sys_dir);
>  				path = zalloc(sizeof(*path));
> +				if (!path)
> +					return NULL;
>  				path->system = malloc(MAX_EVENT_LENGTH);
>  				if (!path->system) {
>  					free(path);
> @@ -252,8 +254,7 @@ struct tracepoint_path *tracepoint_name_to_path(const char *name)
>  	if (path->system == NULL || path->name == NULL) {
>  		zfree(&path->system);
>  		zfree(&path->name);
> -		free(path);
> -		path = NULL;
> +		zfree(&path);
>  	}
>  
>  	return path;
> @@ -1477,10 +1478,9 @@ static void perf_pmu__parse_cleanup(void)
>  
>  		for (i = 0; i < perf_pmu_events_list_num; i++) {
>  			p = perf_pmu_events_list + i;
> -			free(p->symbol);
> +			zfree(&p->symbol);
>  		}
> -		free(perf_pmu_events_list);
> -		perf_pmu_events_list = NULL;
> +		zfree(&perf_pmu_events_list);
>  		perf_pmu_events_list_num = 0;
>  	}
>  }
> @@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name)
>  	r = bsearch(&p, perf_pmu_events_list,
>  			(size_t) perf_pmu_events_list_num,
>  			sizeof(struct perf_pmu_event_symbol), comp_pmu);
> -	free(p.symbol);
> +	zfree(&p.symbol);
>  	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
>  }
>  
> @@ -1710,8 +1710,8 @@ static void parse_events_print_error(struct parse_events_error *err,
>  		fprintf(stderr, "%*s\\___ %s\n", idx + 1, "", err->str);
>  		if (err->help)
>  			fprintf(stderr, "\n%s\n", err->help);
> -		free(err->str);
> -		free(err->help);
> +		zfree(&err->str);
> +		zfree(&err->help);
>  	}
>  
>  	fprintf(stderr, "Run 'perf list' for a list of valid events\n");
> @@ -2406,7 +2406,7 @@ void parse_events_terms__purge(struct list_head *terms)
>  
>  	list_for_each_entry_safe(term, h, terms, list) {
>  		if (term->array.nr_ranges)
> -			free(term->array.ranges);
> +			zfree(&term->array.ranges);
>  		list_del_init(&term->list);
>  		free(term);
>  	}
> @@ -2422,7 +2422,7 @@ void parse_events_terms__delete(struct list_head *terms)
>  
>  void parse_events__clear_array(struct parse_events_array *a)
>  {
> -	free(a->ranges);
> +	zfree(&a->ranges);
>  }
>  
>  void parse_events_evlist_error(struct parse_events_evlist *data,
> -- 
> 2.7.4

Re: [PATCH v2 3/4] perf tools: Check NULL after zalloc() and Use zfree() instead of free() in parse-events.c

[Taeung Song]

Sorry I'm late..
I got it!
I'll send v3 with changed patches !!

Thanks,
Taeung

On 01/31/2017 10:23 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 31, 2017 at 08:38:30PM +0900, Taeung Song escreveu:
>> Currently there are several parts not checking NULL
>> after allocating with zalloc() or asigning NULL value
>> to a pointer variable after doing free().
>>
>> So I fill in code checking NULL and
>> use zfree() instead of free().
>
> You are doing more than one thing on this patch, don't do that.
>
> Please split it for the "no functional changes" parts, i.e. things like
>
> -		free(path);
> -		path = NULL;
> +		zfree(&path);
>
> From the ones that _change_ logic, like the first hunk:
>
>  				path = zalloc(sizeof(*path));
> +				if (!path)
> +					return NULL;
>  				path->system = malloc(MAX_EVENT_LENGTH);
>  				if (!path->system) {
>  					free(path);
>
> This one as well changes logic:
>
> @@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name)
>  	r = bsearch(&p, perf_pmu_events_list,
>  			(size_t) perf_pmu_events_list_num,
>  			sizeof(struct perf_pmu_event_symbol), comp_pmu);
> -	free(p.symbol);
> +	zfree(&p.symbol);
>  	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
>
>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/util/parse-events.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 3c876b8..87a3e5a 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -211,6 +211,8 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
>>  				closedir(evt_dir);
>>  				closedir(sys_dir);
>>  				path = zalloc(sizeof(*path));
>> +				if (!path)
>> +					return NULL;
>>  				path->system = malloc(MAX_EVENT_LENGTH);
>>  				if (!path->system) {
>>  					free(path);
>> @@ -252,8 +254,7 @@ struct tracepoint_path *tracepoint_name_to_path(const char *name)
>>  	if (path->system == NULL || path->name == NULL) {
>>  		zfree(&path->system);
>>  		zfree(&path->name);
>> -		free(path);
>> -		path = NULL;
>> +		zfree(&path);
>>  	}
>>
>>  	return path;
>> @@ -1477,10 +1478,9 @@ static void perf_pmu__parse_cleanup(void)
>>
>>  		for (i = 0; i < perf_pmu_events_list_num; i++) {
>>  			p = perf_pmu_events_list + i;
>> -			free(p->symbol);
>> +			zfree(&p->symbol);
>>  		}
>> -		free(perf_pmu_events_list);
>> -		perf_pmu_events_list = NULL;
>> +		zfree(&perf_pmu_events_list);
>>  		perf_pmu_events_list_num = 0;
>>  	}
>>  }
>> @@ -1563,7 +1563,7 @@ perf_pmu__parse_check(const char *name)
>>  	r = bsearch(&p, perf_pmu_events_list,
>>  			(size_t) perf_pmu_events_list_num,
>>  			sizeof(struct perf_pmu_event_symbol), comp_pmu);
>> -	free(p.symbol);
>> +	zfree(&p.symbol);
>>  	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
>>  }
>>
>> @@ -1710,8 +1710,8 @@ static void parse_events_print_error(struct parse_events_error *err,
>>  		fprintf(stderr, "%*s\\___ %s\n", idx + 1, "", err->str);
>>  		if (err->help)
>>  			fprintf(stderr, "\n%s\n", err->help);
>> -		free(err->str);
>> -		free(err->help);
>> +		zfree(&err->str);
>> +		zfree(&err->help);
>>  	}
>>
>>  	fprintf(stderr, "Run 'perf list' for a list of valid events\n");
>> @@ -2406,7 +2406,7 @@ void parse_events_terms__purge(struct list_head *terms)
>>
>>  	list_for_each_entry_safe(term, h, terms, list) {
>>  		if (term->array.nr_ranges)
>> -			free(term->array.ranges);
>> +			zfree(&term->array.ranges);
>>  		list_del_init(&term->list);
>>  		free(term);
>>  	}
>> @@ -2422,7 +2422,7 @@ void parse_events_terms__delete(struct list_head *terms)
>>
>>  void parse_events__clear_array(struct parse_events_array *a)
>>  {
>> -	free(a->ranges);
>> +	zfree(&a->ranges);
>>  }
>>
>>  void parse_events_evlist_error(struct parse_events_evlist *data,
>> --
>> 2.7.4

[PATCH v2 4/4] perf tools: Increase index if perf_evsel__new_idx() succeeded

[Taeung Song]

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/parse-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 87a3e5a..ac47c12 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -311,10 +311,11 @@ __add_event(struct list_head *list, int *idx,
 
 	event_attr_init(attr);
 
-	evsel = perf_evsel__new_idx(attr, (*idx)++);
+	evsel = perf_evsel__new_idx(attr, *idx);
 	if (!evsel)
 		return NULL;
 
+	(*idx)++;
 	evsel->cpus     = cpu_map__get(cpus);
 	evsel->own_cpus = cpu_map__get(cpus);
 
-- 
2.7.4

Re: [PATCH v2 4/4] perf tools: Increase index if perf_evsel__new_idx() succeeded

[Arnaldo Carvalho de Melo]

Em Tue, Jan 31, 2017 at 08:38:31PM +0900, Taeung Song escreveu:
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>

The title of this patch is misleading, ambiguous. It leads one to have
doubt if we were not incrementing that index before and now we are,
after successfully calling perf_evsel__new_idx().

It should have been:

  "perf tools: Only increase index if perf_evsel__new_idx() succeeds"

- Arnaldo

> ---
>  tools/perf/util/parse-events.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 87a3e5a..ac47c12 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -311,10 +311,11 @@ __add_event(struct list_head *list, int *idx,
>  
>  	event_attr_init(attr);
>  
> -	evsel = perf_evsel__new_idx(attr, (*idx)++);
> +	evsel = perf_evsel__new_idx(attr, *idx);
>  	if (!evsel)
>  		return NULL;
>  
> +	(*idx)++;
>  	evsel->cpus     = cpu_map__get(cpus);
>  	evsel->own_cpus = cpu_map__get(cpus);
>  
> -- 
> 2.7.4

Re: [PATCH v2 4/4] perf tools: Increase index if perf_evsel__new_idx() succeeded

[Taeung Song]

On 01/31/2017 10:25 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 31, 2017 at 08:38:31PM +0900, Taeung Song escreveu:
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>
> The title of this patch is misleading, ambiguous. It leads one to have
> doubt if we were not incrementing that index before and now we are,
> after successfully calling perf_evsel__new_idx().
>
> It should have been:
>
>   "perf tools: Only increase index if perf_evsel__new_idx() succeeds"
>

Understood!
I'll change the title!

Thanks,
Taeung