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

[Wang Nan <wangnan0@huawei.com>]

On 2015/1/22 11:14, Jérémie Galarneau wrote:
> On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan <wangnan0@huawei.com> wrote:
>> On 2015/1/21 23:56, Jérémie Galarneau wrote:
>>> 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
>>
>> Is there any possibility that the someone expand the list?
>>
> 
> Good question. There is discussion around a v1.9 version of the CTF
> spec going on at the moment (which should not affect the Babeltrace
> API).
> 

Since the blacklist is expanding, do you think babeltrace API should provide a mean to
notify users the reason about the failure, such as returning meanful error code instead
of -1, or exporting validate_identifier() to users?

> As far as I know, adding "__attribute__" has been discussed. CC'ing
> Mathieu Desnoyer who may have other extensions in mind.
> 
> Jérémie
> 
>>>
>>>> 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
>>>
>>>
>>>
>>
>>
> 
> 
>