lkml.org 
[lkml]   [2011]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] perf-tools: Fix tracepoint event type recording
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


\
 
 \ /
  Last update: 2011-01-26 22:59    [W:0.052 / U:8.092 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site