[PATCH 1/2] perf-tools: Fix tracepoint event type recording

[PATCH 1/2] perf-tools: Fix tracepoint event type recording

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

Tracepoint event registering was done by store_event_type() which took
the full event name (sys + ':' + event) as argument.

However commit f006d25a15216a483cec71e886786874f66f9452 broke this
by only passing a subset of this full name, that is the substring
following the colon.

The consequence of this is that no more tracepoint event type was
valid, hence making the trace info section empty.

This patch fixes this by merging store_event_type() into
parse_single_tracepoint_event(), so a tracepoint type event is
registered when parsed.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 tools/perf/util/header.c       |   15 ++++++++++-----
 tools/perf/util/header.h       |    2 +-
 tools/perf/util/parse-events.c |   38 ++++++--------------------------------
 3 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 989fa2d..35b707c 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
 static int event_count;
 static struct perf_trace_event_type *events;
 
-int perf_header__push_event(u64 id, const char *name)
+int perf_header__push_event(u64 id, const char *name, size_t len)
 {
-	if (strlen(name) > MAX_EVENT_NAME)
+	if (len > MAX_EVENT_NAME - 1) {
 		pr_warning("Event %s will be truncated\n", name);
+		len = MAX_EVENT_NAME - 1;
+	}
 
 	if (!events) {
 		events = malloc(sizeof(struct perf_trace_event_type));
@@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
 	}
 	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
 	events[event_count].event_id = id;
-	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
+	strncpy(events[event_count].name, name, len);
+	events[event_count].name[len] = '\0';
 	event_count++;
 	return 0;
 }
@@ -1126,8 +1129,10 @@ int event__synthesize_event_types(event__handler_t process,
 int event__process_event_type(event_t *self,
 			      struct perf_session *session __unused)
 {
-	if (perf_header__push_event(self->event_type.event_type.event_id,
-				    self->event_type.event_type.name) < 0)
+	struct perf_trace_event_type *event_type = &self->event_type.event_type;
+
+	if (perf_header__push_event(event_type->event_id,
+				    event_type->name, strlen(event_type->name)) < 0)
 		return -ENOMEM;
 
 	return 0;
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 33f16be..0603a02 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -72,7 +72,7 @@ int perf_header__write_pipe(int fd);
 int perf_header__add_attr(struct perf_header *self,
 			  struct perf_header_attr *attr);
 
-int perf_header__push_event(u64 id, const char *name);
+int perf_header__push_event(u64 id, const char *name, size_t name_len);
 char *perf_header__find_event(u64 id);
 
 struct perf_header_attr *perf_header_attr__new(struct perf_event_attr *attr);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5cb6f4b..a58407e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -417,6 +417,7 @@ parse_single_tracepoint_event(char *sys_name,
 	char id_buf[4];
 	u64 id;
 	int fd;
+	size_t len;
 
 	snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
 		 sys_name, evt_name);
@@ -434,7 +435,6 @@ parse_single_tracepoint_event(char *sys_name,
 	id = atoll(id_buf);
 	attr->config = id;
 	attr->type = PERF_TYPE_TRACEPOINT;
-	*strp += strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
 
 	attr->sample_type |= PERF_SAMPLE_RAW;
 	attr->sample_type |= PERF_SAMPLE_TIME;
@@ -442,7 +442,11 @@ parse_single_tracepoint_event(char *sys_name,
 
 	attr->sample_period = 1;
 
+	len = strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
+	if (perf_header__push_event(id, *strp, len) < 0)
+		return EVT_FAILED;
 
+	*strp += len;
 	return EVT_HANDLED;
 }
 
@@ -490,32 +494,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {
@@ -558,13 +536,9 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		*strp += strlen(sys_name) + evt_length;
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
-	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
+	} else
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
-	}
 }
 
 static enum event_result
-- 
1.7.3.2

Re: [PATCH 1/2] perf-tools: Fix tracepoint event type recording

[Arnaldo Carvalho de Melo]

Em Mon, Jan 17, 2011 at 07:13:11PM +0100, Franck Bui-Huu escreveu:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> Tracepoint event registering was done by store_event_type() which took
> the full event name (sys + ':' + event) as argument.
> 
> However commit f006d25a15216a483cec71e886786874f66f9452 broke this
> by only passing a subset of this full name, that is the substring
> following the colon.
> 
> The consequence of this is that no more tracepoint event type was
> valid, hence making the trace info section empty.
> 
> This patch fixes this by merging store_event_type() into
> parse_single_tracepoint_event(), so a tracepoint type event is
> registered when parsed.

Please try not to do multiple, unrelated changes in a single patch, see
below:
 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  tools/perf/util/header.c       |   15 ++++++++++-----
>  tools/perf/util/header.h       |    2 +-
>  tools/perf/util/parse-events.c |   38 ++++++--------------------------------
>  3 files changed, 17 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 989fa2d..35b707c 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
>  static int event_count;
>  static struct perf_trace_event_type *events;
>  
> -int perf_header__push_event(u64 id, const char *name)
> +int perf_header__push_event(u64 id, const char *name, size_t len)
>  {
> -	if (strlen(name) > MAX_EVENT_NAME)
> +	if (len > MAX_EVENT_NAME - 1) {
>  		pr_warning("Event %s will be truncated\n", name);
> +		len = MAX_EVENT_NAME - 1;
> +	}
>  
>  	if (!events) {
>  		events = malloc(sizeof(struct perf_trace_event_type));
> @@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
>  	}
>  	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
>  	events[event_count].event_id = id;
> -	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
> +	strncpy(events[event_count].name, name, len);
> +	events[event_count].name[len] = '\0';

Is this change related to what you describe in the changelog comment? It
is a cleanup, can be done as a follow up patch, for perf/core.

The problem you describe deserves perf/urgent treatment, please redo
this patch with just the essential bits for that fix.

>  	event_count++;
>  	return 0;
>  }
> @@ -1126,8 +1129,10 @@ int event__synthesize_event_types(event__handler_t process,
>  int event__process_event_type(event_t *self,
>  			      struct perf_session *session __unused)
>  {
> -	if (perf_header__push_event(self->event_type.event_type.event_id,
> -				    self->event_type.event_type.name) < 0)
> +	struct perf_trace_event_type *event_type = &self->event_type.event_type;
> +
> +	if (perf_header__push_event(event_type->event_id,
> +				    event_type->name, strlen(event_type->name)) < 0)
>  		return -ENOMEM;

Ditto.
 
>  	return 0;
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 33f16be..0603a02 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -72,7 +72,7 @@ int perf_header__write_pipe(int fd);
>  int perf_header__add_attr(struct perf_header *self,
>  			  struct perf_header_attr *attr);
>  
> -int perf_header__push_event(u64 id, const char *name);
> +int perf_header__push_event(u64 id, const char *name, size_t name_len);
>  char *perf_header__find_event(u64 id);
>  
>  struct perf_header_attr *perf_header_attr__new(struct perf_event_attr *attr);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 5cb6f4b..a58407e 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -417,6 +417,7 @@ parse_single_tracepoint_event(char *sys_name,
>  	char id_buf[4];
>  	u64 id;
>  	int fd;
> +	size_t len;
>  
>  	snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
>  		 sys_name, evt_name);
> @@ -434,7 +435,6 @@ parse_single_tracepoint_event(char *sys_name,
>  	id = atoll(id_buf);
>  	attr->config = id;
>  	attr->type = PERF_TYPE_TRACEPOINT;
> -	*strp += strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
>  
>  	attr->sample_type |= PERF_SAMPLE_RAW;
>  	attr->sample_type |= PERF_SAMPLE_TIME;
> @@ -442,7 +442,11 @@ parse_single_tracepoint_event(char *sys_name,
>  
>  	attr->sample_period = 1;
>  
> +	len = strlen(sys_name) + evt_length + 1; /* + 1 for the ':' */
> +	if (perf_header__push_event(id, *strp, len) < 0)
> +		return EVT_FAILED;

Since you're changing the point where perf_header__push_event() is
called, consider doing it _after_ parse_events() finishes, by traversing
the evsel_list, that way, as a bonus, later, in perf/core we can kill
some more global variables:

static int event_count;
static struct perf_trace_event_type *events;

These variables should be in 'struct perf_header', but anyway, this is
for later, I'm digressing, please just try to do it after
parse_events(), traversing the evsel_list and getting the tracepoint
name in string form using event_name(evsel) (that also uses an static
variagle, argh, another fix to do).

Doing it that way and excluding the strnlen cleanup, the patch should be
much smaller.

We also untie event parsing from header handling, that are only together
in record, not in, say, top.
  
- Arnaldo

Re: [PATCH 1/2] perf-tools: Fix tracepoint event type recording

[Arnaldo Carvalho de Melo]

Em Mon, Jan 17, 2011 at 05:50:34PM -0200, Arnaldo Carvalho de Melo escreveu:
> > +	if (perf_header__push_event(id, *strp, len) < 0)
> > +		return EVT_FAILED;
> 
> Since you're changing the point where perf_header__push_event() is
> called, consider doing it _after_ parse_events() finishes, by traversing
> the evsel_list, that way, as a bonus, later, in perf/core we can kill
> some more global variables:
> 
> static int event_count;
> static struct perf_trace_event_type *events;
> 
> These variables should be in 'struct perf_header', but anyway, this is
> for later, I'm digressing, please just try to do it after
> parse_events(), traversing the evsel_list and getting the tracepoint
> name in string form using event_name(evsel) (that also uses an static
> variagle, argh, another fix to do).
> 
> Doing it that way and excluding the strnlen cleanup, the patch should be
> much smaller.
> 
> We also untie event parsing from header handling, that are only together
> in record, not in, say, top.

So here is the minimal patch, tested with:

[root@felicio l]# perf record -a -e sched:* sleep 2
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.180 MB perf.data (~95232 samples) ]
[root@felicio l]# hexdump -s 0x6e8 -C perf.data -n 512
000006e8  39 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |9.......sched:sc|
000006f8  68 65 64 5f 6b 74 68 72  65 61 64 5f 73 74 6f 70  |hed_kthread_stop|
00000708  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000728  00 00 00 00 00 00 00 00  38 00 00 00 00 00 00 00  |........8.......|
00000738  73 63 68 65 64 3a 73 63  68 65 64 5f 6b 74 68 72  |sched:sched_kthr|
00000748  65 61 64 5f 73 74 6f 70  5f 72 65 74 00 00 00 00  |ead_stop_ret....|
00000758  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000778  37 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |7.......sched:sc|
00000788  68 65 64 5f 77 61 6b 65  75 70 00 00 00 00 00 00  |hed_wakeup......|
00000798  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
000007b8  00 00 00 00 00 00 00 00  36 00 00 00 00 00 00 00  |........6.......|
000007c8  73 63 68 65 64 3a 73 63  68 65 64 5f 77 61 6b 65  |sched:sched_wake|
000007d8  75 70 5f 6e 65 77 00 00  00 00 00 00 00 00 00 00  |up_new..........|
000007e8  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000808  35 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |5.......sched:sc|
00000818  68 65 64 5f 73 77 69 74  63 68 00 00 00 00 00 00  |hed_switch......|
00000828  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000848  00 00 00 00 00 00 00 00  34 00 00 00 00 00 00 00  |........4.......|
00000858  73 63 68 65 64 3a 73 63  68 65 64 5f 6d 69 67 72  |sched:sched_migr|
00000868  61 74 65 5f 74 61 73 6b  00 00 00 00 00 00 00 00  |ate_task........|
00000878  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000898  33 00 00 00 00 00 00 00  73 63 68 65 64 3a 73 63  |3.......sched:sc|
000008a8  68 65 64 5f 70 72 6f 63  65 73 73 5f 66 72 65 65  |hed_process_free|
000008b8  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
000008d8  00 00 00 00 00 00 00 00  32 00 00 00 00 00 00 00  |........2.......|
000008e8
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_kthread_stop/id 
57
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_kthread_stop_ret/id 
56
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/id 
55
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup_new/id 
54
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_wakeup_switch/id 
cat: /sys/kernel/debug/tracing/events/sched/sched_wakeup_switch/id: No such file or directory
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_switch/id 
53
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_migrate_task/id 
52
[root@felicio l]# cat /sys/kernel/debug/tracing/events/sched/sched_process_free/id 
51
[root@felicio l]# 

See the 0x39, 0x38, 0x37, 0x36, just before the tracepoint names? :-)

It trows all this perf_header stuff out of the parsing routines and moves it to
record, where it belongs. No need to re-open the /sys/ file again to get something
we already have in perf_evsel->attr.config and that was parsed, opening the /sys
file in parse_single_tracepoint_event.

Please let me know if you see any hole in it.

- Arnaldo

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index df6064a..fcd29e8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -936,6 +936,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	list_for_each_entry(pos, &evsel_list, node) {
 		if (perf_evsel__alloc_fd(pos, cpus->nr, threads->nr) < 0)
 			goto out_free_fd;
+		if (perf_header__push_event(pos->attr.config, event_name(pos)))
+			goto out_free_fd;
 	}
 	event_array = malloc((sizeof(struct pollfd) * MAX_NR_CPUS *
 			      MAX_COUNTERS * threads->nr));
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1f4cfe5..bc2732e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -490,32 +490,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {
@@ -559,9 +533,6 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
 	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
 	}

[tip:perf/urgent] perf tools: Fix tracepoint id to string perf.data header table

[tip-bot for Arnaldo Carvalho de Melo]

Commit-ID:  ad7f4e3f7b966ac09c8f98dbc5024813a1685775
Gitweb:     http://git.kernel.org/tip/ad7f4e3f7b966ac09c8f98dbc5024813a1685775
Author:     Arnaldo Carvalho de Melo <acme@redhat.com>
AuthorDate: Mon, 17 Jan 2011 18:28:13 -0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 17 Jan 2011 18:28:13 -0200

perf tools: Fix tracepoint id to string perf.data header table

It was broken by f006d25 that passed just the event name, not the complete
sys:event that it expected to open the /sys/.../sys/sys:event/id file to get
the id.

Fix it by moving it to after parse_events in cmd_record, as at that point
we can just traverse the evsel_list and use evsel->attr.config +
event_name(evsel) instead of re-opening the /id file.

Reported-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
Cc: Franck Bui-Huu <vagabon.xyz@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Han Pingtian <phan@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <20110117202801.GG2085@ghostprotocols.net>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c    |    2 ++
 tools/perf/util/parse-events.c |   29 -----------------------------
 2 files changed, 2 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index df6064a..fcd29e8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -936,6 +936,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	list_for_each_entry(pos, &evsel_list, node) {
 		if (perf_evsel__alloc_fd(pos, cpus->nr, threads->nr) < 0)
 			goto out_free_fd;
+		if (perf_header__push_event(pos->attr.config, event_name(pos)))
+			goto out_free_fd;
 	}
 	event_array = malloc((sizeof(struct pollfd) * MAX_NR_CPUS *
 			      MAX_COUNTERS * threads->nr));
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1f4cfe5..bc2732e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -490,32 +490,6 @@ parse_multiple_tracepoint_event(char *sys_name, const char *evt_exp,
 	return EVT_HANDLED_ALL;
 }
 
-static int store_event_type(const char *orgname)
-{
-	char filename[PATH_MAX], *c;
-	FILE *file;
-	int id, n;
-
-	sprintf(filename, "%s/", debugfs_path);
-	strncat(filename, orgname, strlen(orgname));
-	strcat(filename, "/id");
-
-	c = strchr(filename, ':');
-	if (c)
-		*c = '/';
-
-	file = fopen(filename, "r");
-	if (!file)
-		return 0;
-	n = fscanf(file, "%i", &id);
-	fclose(file);
-	if (n < 1) {
-		pr_err("cannot store event ID\n");
-		return -EINVAL;
-	}
-	return perf_header__push_event(id, orgname);
-}
-
 static enum event_result parse_tracepoint_event(const char **strp,
 				    struct perf_event_attr *attr)
 {
@@ -559,9 +533,6 @@ static enum event_result parse_tracepoint_event(const char **strp,
 		return parse_multiple_tracepoint_event(sys_name, evt_name,
 						       flags);
 	} else {
-		if (store_event_type(evt_name) < 0)
-			return EVT_FAILED;
-
 		return parse_single_tracepoint_event(sys_name, evt_name,
 						     evt_length, attr, strp);
 	}

Re: [PATCH 1/2] perf-tools: Fix tracepoint event type recording

[Franck Bui-Huu]

Hello,

Sorry for my (very) late answer...

On 01/17/2011 08:50 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 17, 2011 at 07:13:11PM +0100, Franck Bui-Huu escreveu:
[...]

> 
> Please try not to do multiple, unrelated changes in a single patch, see
> below:

Well, they're not unrelated because of the place I choose to put
perf_header__push_event().

But I agree with you, placing it after parse_events() is much better.

>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 989fa2d..35b707c 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -104,10 +104,12 @@ int perf_header__add_attr(struct perf_header *self,
>>  static int event_count;
>>  static struct perf_trace_event_type *events;
>>  
>> -int perf_header__push_event(u64 id, const char *name)
>> +int perf_header__push_event(u64 id, const char *name, size_t len)
>>  {
>> -	if (strlen(name) > MAX_EVENT_NAME)
>> +	if (len > MAX_EVENT_NAME - 1) {
>>  		pr_warning("Event %s will be truncated\n", name);
>> +		len = MAX_EVENT_NAME - 1;
>> +	}
>>  
>>  	if (!events) {
>>  		events = malloc(sizeof(struct perf_trace_event_type));
>> @@ -123,7 +125,8 @@ int perf_header__push_event(u64 id, const char *name)
>>  	}
>>  	memset(&events[event_count], 0, sizeof(struct perf_trace_event_type));
>>  	events[event_count].event_id = id;
>> -	strncpy(events[event_count].name, name, MAX_EVENT_NAME - 1);
>> +	strncpy(events[event_count].name, name, len);
>> +	events[event_count].name[len] = '\0';
> 
> Is this change related to what you describe in the changelog comment? It
> is a cleanup, can be done as a follow up patch, for perf/core.

It's not really unrelated if you place perf_header__push_event() where I
did.

BTW it's not a cleanup too, it's rather a fix since the original code is
broken if the name is truncated since the null byte is missing in this case.
[...]

> Since you're changing the point where perf_header__push_event() is
> called, consider doing it _after_ parse_events() finishes, by traversing
> the evsel_list, that way, as a bonus, later, in perf/core we can kill
> some more global variables:
> 
> static int event_count;
> static struct perf_trace_event_type *events;
> 
> These variables should be in 'struct perf_header', but anyway, this is
> for later, I'm digressing, please just try to do it after
> parse_events(), traversing the evsel_list and getting the tracepoint
> name in string form using event_name(evsel) (that also uses an static
> variagle, argh, another fix to do).
> 
> Doing it that way and excluding the strnlen cleanup, the patch should be
> much smaller.
> 
> We also untie event parsing from header handling, that are only together
> in record, not in, say, top.

Agreed.

		Franck