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
--
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/