Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
- Original Message -
> From: "Jérémie Galarneau"
> To: "Wang Nan"
> Cc: "Jiri Olsa" , "Sebastian Andrzej Siewior"
> , "Li Zefan"
> , linux-kernel@vger.kernel.org, "Mathieu Desnoyers"
>
> Sent: Wednesday, January 21, 2015 10:14:16 PM
> Subject: Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid
> reserved keywords.
>
> On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wrote:
> > On 2015/1/21 23:56, Jérémie Galarneau wrote:
> >> On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa 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).
>
> As far as I know, adding "__attribute__" has been discussed. CC'ing
> Mathieu Desnoyers who may have other extensions in mind.
I've had in mind adding an optional $ prefix to identifiers so they
don't clash with reserved keywords. This would have to go into a
CTF 1.9 though.
Meanwhile, validating that there are no identifier clash in babeltrace
seems like a good idea. Alternatively, prefixing all identifiers with
an underscore eliminates those clashes, and Babeltrace even strip those
underscore before printing, but since underscore is a character that
is allowed within keywords, this can bring interesting clash when
a keyword actually begins with an underscore, so I would like to
replace those by $.
Thoughts ?
Thanks,
Mathieu
>
> 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
> >>
> >>
> >>
> >
> >
>
>
>
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
- Original Message -
From: Jérémie Galarneau jeremie.galarn...@efficios.com
To: Wang Nan wangn...@huawei.com
Cc: Jiri Olsa jo...@redhat.com, Sebastian Andrzej Siewior
bige...@linutronix.de, Li Zefan
lize...@huawei.com, linux-kernel@vger.kernel.org, Mathieu Desnoyers
mathieu.desnoy...@efficios.com
Sent: Wednesday, January 21, 2015 10:14:16 PM
Subject: Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid
reserved keywords.
On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wangn...@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 jo...@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).
As far as I know, adding __attribute__ has been discussed. CC'ing
Mathieu Desnoyers who may have other extensions in mind.
I've had in mind adding an optional $ prefix to identifiers so they
don't clash with reserved keywords. This would have to go into a
CTF 1.9 though.
Meanwhile, validating that there are no identifier clash in babeltrace
seems like a good idea. Alternatively, prefixing all identifiers with
an underscore eliminates those clashes, and Babeltrace even strip those
underscore before printing, but since underscore is a character that
is allowed within keywords, this can bring interesting clash when
a keyword actually begins with an underscore, so I would like to
replace those by $.
Thoughts ?
Thanks,
Mathieu
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
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On 2015/1/22 11:14, Jérémie Galarneau wrote:
> On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wrote:
>> On 2015/1/21 23:56, Jérémie Galarneau wrote:
>>> On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa 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
>>>
>>>
>>>
>>
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wrote:
> On 2015/1/21 23:56, Jérémie Galarneau wrote:
>> On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa 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).
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
>>
>>
>>
>
>
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On 2015/1/21 23:56, Jérémie Galarneau wrote:
> On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa 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?
>
>> 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
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 09:25:45AM -0500, Steven Rostedt wrote: > On Wed, 21 Jan 2015 15:19:38 +0100 > Jiri Olsa wrote: > > > On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: > > > Hi Jiri, > > > > > > I found that only this patch is not enough. When converting such > > > tracepoints, > > > it uses add_tracepoint_fields_values(..., struct format_field *fields > > > ...), > > > and fields->name is still the original one. > > > > > > If 'struct format_field' has a field like 'dup_name' we can make things > > > simpler. > > > However, struct format_field is part of traceevent, not only used by perf. > > > > > > I have no enough time to think on it. Jiri, could you please give me some > > > hints > > > so I can implement another patch tomorrow? > > > > yea, looks like we either need to add 'void *priv' into 'struct > > format_field' > > or if Steven doesn't like it, we'd need to save 'our' field name in some way > > so it's reachable via format_field::name string. > > > > Steven, > > we need to use changed format_field::name to interface babeltrace > > library, because it has restriction that fields within tracepoint > > should have unique names. > > > > Any chance we could introduce 'void *priv' member to format_field::name ? > > Maybe with 'destroy_priv' callback to be called when the field is destroyed. > > > > I can provide patch, just wanted to know first if you're not strictly > > against ;-) > > > > I'm not exactly sure what the issue is. I guess I need to see a patch > to understand better. ok, will do jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, 21 Jan 2015 15:19:38 +0100 Jiri Olsa wrote: > On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: > > Hi Jiri, > > > > I found that only this patch is not enough. When converting such > > tracepoints, > > it uses add_tracepoint_fields_values(..., struct format_field *fields ...), > > and fields->name is still the original one. > > > > If 'struct format_field' has a field like 'dup_name' we can make things > > simpler. > > However, struct format_field is part of traceevent, not only used by perf. > > > > I have no enough time to think on it. Jiri, could you please give me some > > hints > > so I can implement another patch tomorrow? > > yea, looks like we either need to add 'void *priv' into 'struct format_field' > or if Steven doesn't like it, we'd need to save 'our' field name in some way > so it's reachable via format_field::name string. > > Steven, > we need to use changed format_field::name to interface babeltrace > library, because it has restriction that fields within tracepoint > should have unique names. > > Any chance we could introduce 'void *priv' member to format_field::name ? > Maybe with 'destroy_priv' callback to be called when the field is destroyed. > > I can provide patch, just wanted to know first if you're not strictly against > ;-) > I'm not exactly sure what the issue is. I guess I need to see a patch to understand better. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: > Hi Jiri, > > I found that only this patch is not enough. When converting such tracepoints, > it uses add_tracepoint_fields_values(..., struct format_field *fields ...), > and fields->name is still the original one. > > If 'struct format_field' has a field like 'dup_name' we can make things > simpler. > However, struct format_field is part of traceevent, not only used by perf. > > I have no enough time to think on it. Jiri, could you please give me some > hints > so I can implement another patch tomorrow? yea, looks like we either need to add 'void *priv' into 'struct format_field' or if Steven doesn't like it, we'd need to save 'our' field name in some way so it's reachable via format_field::name string. Steven, we need to use changed format_field::name to interface babeltrace library, because it has restriction that fields within tracepoint should have unique names. Any chance we could introduce 'void *priv' member to format_field::name ? Maybe with 'destroy_priv' callback to be called when the field is destroyed. I can provide patch, just wanted to know first if you're not strictly against ;-) thanks, jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
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?
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
Hi Jiri,
I found that only this patch is not enough. When converting such tracepoints,
it uses add_tracepoint_fields_values(..., struct format_field *fields ...),
and fields->name is still the original one.
If 'struct format_field' has a field like 'dup_name' we can make things simpler.
However, struct format_field is part of traceevent, not only used by perf.
I have no enough time to think on it. Jiri, could you please give me some hints
so I can implement another patch tomorrow?
Thank you.
On 2015/1/21 11:23, 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.
>
> This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
> prefix to avoid problem 2.
>
> Signed-off-by: Wang Nan
> ---
> tools/perf/util/data-convert-bt.c | 65
> ---
> 1 file changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/data-convert-bt.c
> b/tools/perf/util/data-convert-bt.c
> index ddecce8..b0a042d 100644
> --- a/tools/perf/util/data-convert-bt.c
> +++ b/tools/perf/util/data-convert-bt.c
> @@ -567,6 +567,38 @@ static int process_sample_event(struct perf_tool *tool,
> return cs ? 0 : -1;
> }
>
> +/* If dup < 0, add a prefix. Else, add _dupl_X suffix. */
> +static char *change_name(char *name, char *orig_name, int dup)
> +{
> + char *new_name = NULL;
> + size_t len;
> +
> + if (!name)
> + name = orig_name;
> +
> + if (dup >= 10)
> + goto out;
> +
> + if (dup < 0)
> + len = strlen(name) + sizeof("_");
> + else
> + len = strlen(orig_name) + sizeof("_dupl_X");
> +
> + new_name = malloc(len);
> + if (!new_name)
> + goto out;
> +
> + if (dup < 0)
> + snprintf(new_name, len, "_%s", name);
> + else
> + snprintf(new_name, len, "%s_dupl_%d", orig_name, dup);
> +
> +out:
> + if (name != orig_name)
> + free(name);
> + return new_name;
> +}
> +
> 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;
> + while (f = bt_ctf_event_class_get_field_by_name(event_class,
> name)) {
> + bt_ctf_field_type_put(f);
> + name = change_name(name, field->name, dup++);
> + if (!name) {
> + pr_err("Failed to create dup name for '%s'\n",
> field->name);
> + return -1;
> + }
> + }
> +
> + ret = bt_ctf_event_class_add_field(event_class, type, name);
> + /* if failed, we may hit a keywork. try again with a '_' prefix
> */
> + if (ret) {
> + name = change_name(name, field->name, -1);
> + if (!name) {
> + pr_err("Failed to alloc name for '_%s'\n",
> field->name);
> + return -1;
> + }
> + ret = bt_ctf_event_class_add_field(event_class, type,
> name);
> + }
> + if (name != field->name)
> + free(name);
>
> if (flags & FIELD_IS_ARRAY)
> bt_ctf_field_type_put(type);
>
> if (ret) {
> - pr_err("Failed to add field '%s\n", field->name);
> + pr_err("Failed to add field '%s': %d\n",
> + field->name, ret);
> return -1;
> }
> }
> @@ -646,7 +703,7 @@ static int add_generic_types(struct ctf_writer *cw,
> struct perf_evsel *evsel,
> do {
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa jo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wangn...@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 jo...@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).
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
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On 2015/1/21 23:56, Jérémie Galarneau wrote:
On Wed, Jan 21, 2015 at 9:11 AM, Jiri Olsa jo...@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?
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On 2015/1/22 11:14, Jérémie Galarneau wrote:
On Wed, Jan 21, 2015 at 8:38 PM, Wang Nan wangn...@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 jo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
Hi Jiri,
I found that only this patch is not enough. When converting such tracepoints,
it uses add_tracepoint_fields_values(..., struct format_field *fields ...),
and fields-name is still the original one.
If 'struct format_field' has a field like 'dup_name' we can make things simpler.
However, struct format_field is part of traceevent, not only used by perf.
I have no enough time to think on it. Jiri, could you please give me some hints
so I can implement another patch tomorrow?
Thank you.
On 2015/1/21 11:23, 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.
This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
prefix to avoid problem 2.
Signed-off-by: Wang Nan wangn...@huawei.com
---
tools/perf/util/data-convert-bt.c | 65
---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/data-convert-bt.c
b/tools/perf/util/data-convert-bt.c
index ddecce8..b0a042d 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -567,6 +567,38 @@ static int process_sample_event(struct perf_tool *tool,
return cs ? 0 : -1;
}
+/* If dup 0, add a prefix. Else, add _dupl_X suffix. */
+static char *change_name(char *name, char *orig_name, int dup)
+{
+ char *new_name = NULL;
+ size_t len;
+
+ if (!name)
+ name = orig_name;
+
+ if (dup = 10)
+ goto out;
+
+ if (dup 0)
+ len = strlen(name) + sizeof(_);
+ else
+ len = strlen(orig_name) + sizeof(_dupl_X);
+
+ new_name = malloc(len);
+ if (!new_name)
+ goto out;
+
+ if (dup 0)
+ snprintf(new_name, len, _%s, name);
+ else
+ snprintf(new_name, len, %s_dupl_%d, orig_name, dup);
+
+out:
+ if (name != orig_name)
+ free(name);
+ return new_name;
+}
+
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;
+ while (f = bt_ctf_event_class_get_field_by_name(event_class,
name)) {
+ bt_ctf_field_type_put(f);
+ name = change_name(name, field-name, dup++);
+ if (!name) {
+ pr_err(Failed to create dup name for '%s'\n,
field-name);
+ return -1;
+ }
+ }
+
+ ret = bt_ctf_event_class_add_field(event_class, type, name);
+ /* if failed, we may hit a keywork. try again with a '_' prefix
*/
+ if (ret) {
+ name = change_name(name, field-name, -1);
+ if (!name) {
+ pr_err(Failed to alloc name for '_%s'\n,
field-name);
+ return -1;
+ }
+ ret = bt_ctf_event_class_add_field(event_class, type,
name);
+ }
+ if (name != field-name)
+ free(name);
if (flags FIELD_IS_ARRAY)
bt_ctf_field_type_put(type);
if (ret) {
- pr_err(Failed to add field '%s\n, field-name);
+ pr_err(Failed to add field '%s': %d\n,
+ field-name, ret);
return -1;
}
}
@@ -646,7 +703,7 @@ static int add_generic_types(struct ctf_writer *cw,
struct perf_evsel *evsel,
do {\
pr2( field '%s'\n, n); \
if
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, 21 Jan 2015 15:19:38 +0100 Jiri Olsa jo...@redhat.com wrote: On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: Hi Jiri, I found that only this patch is not enough. When converting such tracepoints, it uses add_tracepoint_fields_values(..., struct format_field *fields ...), and fields-name is still the original one. If 'struct format_field' has a field like 'dup_name' we can make things simpler. However, struct format_field is part of traceevent, not only used by perf. I have no enough time to think on it. Jiri, could you please give me some hints so I can implement another patch tomorrow? yea, looks like we either need to add 'void *priv' into 'struct format_field' or if Steven doesn't like it, we'd need to save 'our' field name in some way so it's reachable via format_field::name string. Steven, we need to use changed format_field::name to interface babeltrace library, because it has restriction that fields within tracepoint should have unique names. Any chance we could introduce 'void *priv' member to format_field::name ? Maybe with 'destroy_priv' callback to be called when the field is destroyed. I can provide patch, just wanted to know first if you're not strictly against ;-) I'm not exactly sure what the issue is. I guess I need to see a patch to understand better. -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 09:25:45AM -0500, Steven Rostedt wrote: On Wed, 21 Jan 2015 15:19:38 +0100 Jiri Olsa jo...@redhat.com wrote: On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: Hi Jiri, I found that only this patch is not enough. When converting such tracepoints, it uses add_tracepoint_fields_values(..., struct format_field *fields ...), and fields-name is still the original one. If 'struct format_field' has a field like 'dup_name' we can make things simpler. However, struct format_field is part of traceevent, not only used by perf. I have no enough time to think on it. Jiri, could you please give me some hints so I can implement another patch tomorrow? yea, looks like we either need to add 'void *priv' into 'struct format_field' or if Steven doesn't like it, we'd need to save 'our' field name in some way so it's reachable via format_field::name string. Steven, we need to use changed format_field::name to interface babeltrace library, because it has restriction that fields within tracepoint should have unique names. Any chance we could introduce 'void *priv' member to format_field::name ? Maybe with 'destroy_priv' callback to be called when the field is destroyed. I can provide patch, just wanted to know first if you're not strictly against ;-) I'm not exactly sure what the issue is. I guess I need to see a patch to understand better. ok, will do jirka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
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?
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
On Wed, Jan 21, 2015 at 07:16:39PM +0800, Wang Nan wrote: Hi Jiri, I found that only this patch is not enough. When converting such tracepoints, it uses add_tracepoint_fields_values(..., struct format_field *fields ...), and fields-name is still the original one. If 'struct format_field' has a field like 'dup_name' we can make things simpler. However, struct format_field is part of traceevent, not only used by perf. I have no enough time to think on it. Jiri, could you please give me some hints so I can implement another patch tomorrow? yea, looks like we either need to add 'void *priv' into 'struct format_field' or if Steven doesn't like it, we'd need to save 'our' field name in some way so it's reachable via format_field::name string. Steven, we need to use changed format_field::name to interface babeltrace library, because it has restriction that fields within tracepoint should have unique names. Any chance we could introduce 'void *priv' member to format_field::name ? Maybe with 'destroy_priv' callback to be called when the field is destroyed. I can provide patch, just wanted to know first if you're not strictly against ;-) thanks, jirka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
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.
This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
prefix to avoid problem 2.
Signed-off-by: Wang Nan
---
tools/perf/util/data-convert-bt.c | 65 ---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/data-convert-bt.c
b/tools/perf/util/data-convert-bt.c
index ddecce8..b0a042d 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -567,6 +567,38 @@ static int process_sample_event(struct perf_tool *tool,
return cs ? 0 : -1;
}
+/* If dup < 0, add a prefix. Else, add _dupl_X suffix. */
+static char *change_name(char *name, char *orig_name, int dup)
+{
+ char *new_name = NULL;
+ size_t len;
+
+ if (!name)
+ name = orig_name;
+
+ if (dup >= 10)
+ goto out;
+
+ if (dup < 0)
+ len = strlen(name) + sizeof("_");
+ else
+ len = strlen(orig_name) + sizeof("_dupl_X");
+
+ new_name = malloc(len);
+ if (!new_name)
+ goto out;
+
+ if (dup < 0)
+ snprintf(new_name, len, "_%s", name);
+ else
+ snprintf(new_name, len, "%s_dupl_%d", orig_name, dup);
+
+out:
+ if (name != orig_name)
+ free(name);
+ return new_name;
+}
+
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;
+ while (f = bt_ctf_event_class_get_field_by_name(event_class,
name)) {
+ bt_ctf_field_type_put(f);
+ name = change_name(name, field->name, dup++);
+ if (!name) {
+ pr_err("Failed to create dup name for '%s'\n",
field->name);
+ return -1;
+ }
+ }
+
+ ret = bt_ctf_event_class_add_field(event_class, type, name);
+ /* if failed, we may hit a keywork. try again with a '_' prefix
*/
+ if (ret) {
+ name = change_name(name, field->name, -1);
+ if (!name) {
+ pr_err("Failed to alloc name for '_%s'\n",
field->name);
+ return -1;
+ }
+ ret = bt_ctf_event_class_add_field(event_class, type,
name);
+ }
+ if (name != field->name)
+ free(name);
if (flags & FIELD_IS_ARRAY)
bt_ctf_field_type_put(type);
if (ret) {
- pr_err("Failed to add field '%s\n", field->name);
+ pr_err("Failed to add field '%s': %d\n",
+ field->name, ret);
return -1;
}
}
@@ -646,7 +703,7 @@ static int add_generic_types(struct ctf_writer *cw, struct
perf_evsel *evsel,
do {\
pr2(" field '%s'\n", n); \
if (bt_ctf_event_class_add_field(cl, t, n)) { \
- pr_err("Failed to add field '%s;\n", n);\
+ pr_err("Failed to add field '%s';\n", n); \
return -1; \
} \
} while (0)
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More
[PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords.
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.
This patch appends '_dupl_X' suffix to avoid problem 1, prepend a '_'
prefix to avoid problem 2.
Signed-off-by: Wang Nan wangn...@huawei.com
---
tools/perf/util/data-convert-bt.c | 65 ---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/data-convert-bt.c
b/tools/perf/util/data-convert-bt.c
index ddecce8..b0a042d 100644
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -567,6 +567,38 @@ static int process_sample_event(struct perf_tool *tool,
return cs ? 0 : -1;
}
+/* If dup 0, add a prefix. Else, add _dupl_X suffix. */
+static char *change_name(char *name, char *orig_name, int dup)
+{
+ char *new_name = NULL;
+ size_t len;
+
+ if (!name)
+ name = orig_name;
+
+ if (dup = 10)
+ goto out;
+
+ if (dup 0)
+ len = strlen(name) + sizeof(_);
+ else
+ len = strlen(orig_name) + sizeof(_dupl_X);
+
+ new_name = malloc(len);
+ if (!new_name)
+ goto out;
+
+ if (dup 0)
+ snprintf(new_name, len, _%s, name);
+ else
+ snprintf(new_name, len, %s_dupl_%d, orig_name, dup);
+
+out:
+ if (name != orig_name)
+ free(name);
+ return new_name;
+}
+
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;
+ while (f = bt_ctf_event_class_get_field_by_name(event_class,
name)) {
+ bt_ctf_field_type_put(f);
+ name = change_name(name, field-name, dup++);
+ if (!name) {
+ pr_err(Failed to create dup name for '%s'\n,
field-name);
+ return -1;
+ }
+ }
+
+ ret = bt_ctf_event_class_add_field(event_class, type, name);
+ /* if failed, we may hit a keywork. try again with a '_' prefix
*/
+ if (ret) {
+ name = change_name(name, field-name, -1);
+ if (!name) {
+ pr_err(Failed to alloc name for '_%s'\n,
field-name);
+ return -1;
+ }
+ ret = bt_ctf_event_class_add_field(event_class, type,
name);
+ }
+ if (name != field-name)
+ free(name);
if (flags FIELD_IS_ARRAY)
bt_ctf_field_type_put(type);
if (ret) {
- pr_err(Failed to add field '%s\n, field-name);
+ pr_err(Failed to add field '%s': %d\n,
+ field-name, ret);
return -1;
}
}
@@ -646,7 +703,7 @@ static int add_generic_types(struct ctf_writer *cw, struct
perf_evsel *evsel,
do {\
pr2( field '%s'\n, n); \
if (bt_ctf_event_class_add_field(cl, t, n)) { \
- pr_err(Failed to add field '%s;\n, n);\
+ pr_err(Failed to add field '%s';\n, n); \
return -1; \
} \
} while (0)
--
1.8.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at
