Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.

From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Li Zefan <lizefan@huawei.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
Date: Wed, 21 Jan 2015 10:56:13 -0500	[thread overview]
Message-ID: <CA+jJMxtaEF587MyW00YQ_7d7BYmW-QpcgQ8g1KByYWwzMg-_cw@mail.gmail.com> (raw)
In-Reply-To: <20150121141101.GA6835@krava.brq.redhat.com>

On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Jan 21, 2015 at 11:23:54AM +0800, Wang Nan wrote:
>> Some parameters of syscall tracepoints named as 'nr', 'event', etc.
>> When dealing with them, perf convert to ctf meets some problem:
>>
>>  1. If a parameter with name 'nr', it will duplicate syscall's
>>     common field 'nr'. One such syscall is io_submit().
>>
>>  2. If a parameter with name 'event', it is denied to be inserted
>>     because 'event' is a babeltrace keywork. One such syscall is
>>     epoll_ctl.
>
> hum, so this problem 2 is detectable only via bt_ctf_event_class_add_field function?
>
> how big is the blaklist?
>

The blacklist is defined by the CTF specification here [1].

Jérémie

[1] http://git.efficios.com/?p=ctf.git;a=blob;f=common-trace-format-specification.txt;h=abe4fb70fff7f17f6e8242f313fb74bff44cf89a;hb=HEAD#l1477

> SNIP
>
>> +}
>> +
>>  static int add_tracepoint_fields_types(struct ctf_writer *cw,
>>                                      struct format_field *fields,
>>                                      struct bt_ctf_event_class *event_class)
>> @@ -577,6 +609,9 @@ static int add_tracepoint_fields_types(struct ctf_writer *cw,
>>       for (field = fields; field; field = field->next) {
>>               struct bt_ctf_field_type *type;
>>               unsigned long flags = field->flags;
>> +             struct bt_ctf_field_type *f = NULL;
>> +             char *name;
>> +             int dup = 1;
>>
>>               pr2("  field '%s'\n", field->name);
>>
>> @@ -595,14 +630,36 @@ static int add_tracepoint_fields_types(struct ctf_writer *cw,
>>               if (flags & FIELD_IS_ARRAY)
>>                       type = bt_ctf_field_type_array_create(type, field->arraylen);
>>
>> -             ret = bt_ctf_event_class_add_field(event_class, type,
>> -                             field->name);
>> +             /* Check name duplication */
>> +             name = field->name;
>
> could you please put this in separated function like 'get_field_name(..)'
> so we dont polute this function even more
>
>                 name == get_field_name(...)
>                 if (!name)
>                         error path
>
>
> thanks,
> jirka

-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com