* [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns. @ 2015-01-20 11:07 Wang Nan 2015-01-20 11:07 ` [PATCH 1/2] perf: convert: fix duplicate field names Wang Nan ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Wang Nan @ 2015-01-20 11:07 UTC (permalink / raw) To: jolsa; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel This 2 patches are based on Jiri Olsa's perf repository: https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/core_ctf_convert Perf data convert to failes when converting perf.data recorded with # perf record -a -e syscalls:* sleep 1 The following 2 patches fix it. Wang Nan (2): perf: convert: fix duplicate field names. perf: convert: fix signess of value. tools/perf/util/data-convert-bt.c | 101 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 96 insertions(+), 5 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2] perf: convert: fix duplicate field names. 2015-01-20 11:07 [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Wang Nan @ 2015-01-20 11:07 ` Wang Nan 2015-01-20 13:06 ` Jiri Olsa 2015-01-20 11:07 ` [PATCH 2/2] perf: convert: fix signess of value Wang Nan 2015-01-20 13:06 ` [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Jiri Olsa 2 siblings, 1 reply; 27+ messages in thread From: Wang Nan @ 2015-01-20 11:07 UTC (permalink / raw) To: jolsa; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel Some parameters of syscall tracepoints named as 'nr', 'event', etc. When dealing with them, perf convert to ctf meets some problems: 1. If a parameter with name 'nr', it duplicate syscall's common field 'nr'. One example syscall is io_submit(). 2. If a parameter with name 'event', it is denied to be inserted because 'event' is a babeltrace keywork. One example syscall is epoll_ctl(). This patch appends '_dupl_X' postfix for those fields to avoid these problems. Signed-off-by: Wang Nan <wangnan0@huawei.com> --- tools/perf/util/data-convert-bt.c | 60 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c index ddecce8..2b87097 100644 --- a/tools/perf/util/data-convert-bt.c +++ b/tools/perf/util/data-convert-bt.c @@ -567,6 +567,27 @@ static int process_sample_event(struct perf_tool *tool, return cs ? 0 : -1; } +static char *get_dupl_name(const char *name, int dup) +{ + int size; + char *str; + + if (dup == 0) + return strdup(name); + + if (dup >= 10) + return NULL; + + /* null byte is considered by sizeof. */ + size = strlen(name) + sizeof("_dupl_X"); + str = malloc(size); + if (!str) + return NULL; + + snprintf(str, size, "%s_dupl_%d", name, dup); + return str; +} + static int add_tracepoint_fields_types(struct ctf_writer *cw, struct format_field *fields, struct bt_ctf_event_class *event_class) @@ -577,6 +598,7 @@ 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; + int dup; pr2(" field '%s'\n", field->name); @@ -595,14 +617,44 @@ 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); + /* + * Babeltrace doesn't allow duplication field name in a structure. + * bt_ctf_event_class_get_field_by_name() can be used to check + * duplication, but babeltrace has some 'reserved_keywords' which + * are also disallowed. 'event' is one of those trouble makers. + * + * So instead of checking duplication, simply tries 10 times. + */ + for (dup = 0; dup < 10; dup ++) { + struct bt_ctf_field_type *f; + char *dupl_name = get_dupl_name(field->name, dup); + + if (!dupl_name) { + pr_err("Failed to alloc memory for dup '%s'\n", + field->name); + return -1; + } + + ret = bt_ctf_event_class_add_field(event_class, type, + dupl_name); + free(dupl_name); + if (ret) + continue; + break; + } + + if (dup >= 10) { + pr_err("Failed to add field '%s': tried 10 times\n", + field->name); + return -1; + } 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 +698,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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names. 2015-01-20 11:07 ` [PATCH 1/2] perf: convert: fix duplicate field names Wang Nan @ 2015-01-20 13:06 ` Jiri Olsa 2015-01-21 3:23 ` [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 0 siblings, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-20 13:06 UTC (permalink / raw) To: Wang Nan; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel On Tue, Jan 20, 2015 at 07:07:08PM +0800, Wang Nan wrote: SNIP > + * > + * So instead of checking duplication, simply tries 10 times. > + */ > + for (dup = 0; dup < 10; dup ++) { > + struct bt_ctf_field_type *f; > + char *dupl_name = get_dupl_name(field->name, dup); > + > + if (!dupl_name) { > + pr_err("Failed to alloc memory for dup '%s'\n", > + field->name); > + return -1; > + } > + > + ret = bt_ctf_event_class_add_field(event_class, type, > + dupl_name); > + free(dupl_name); > + if (ret) > + continue; hum.. so we dont know if we failed because of the name, but we keep trying 10 times anyway.. does not seem nice to me how about using that function you mentioned in the above comment to get the proper name first, like: while(bt_ctf_event_class_get_field_by_name(name)) change_name(name) and then add use it for bt_ctf_event_class_add_field thanks, jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-20 13:06 ` Jiri Olsa @ 2015-01-21 3:23 ` Wang Nan 2015-01-21 11:16 ` Wang Nan 2015-01-21 14:11 ` [PATCH 1/2] " Jiri Olsa 0 siblings, 2 replies; 27+ messages in thread From: Wang Nan @ 2015-01-21 3:23 UTC (permalink / raw) To: jolsa; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel 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 <wangnan0@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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 3:23 ` [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan @ 2015-01-21 11:16 ` Wang Nan 2015-01-21 14:19 ` Jiri Olsa 2015-01-21 14:11 ` [PATCH 1/2] " Jiri Olsa 1 sibling, 1 reply; 27+ messages in thread From: Wang Nan @ 2015-01-21 11:16 UTC (permalink / raw) To: jolsa; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel 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 <wangnan0@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) > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 11:16 ` Wang Nan @ 2015-01-21 14:19 ` Jiri Olsa 2015-01-21 14:25 ` Steven Rostedt 0 siblings, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-21 14:19 UTC (permalink / raw) To: Wang Nan Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel, Steven Rostedt 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 14:19 ` Jiri Olsa @ 2015-01-21 14:25 ` Steven Rostedt 2015-01-21 14:32 ` Jiri Olsa 0 siblings, 1 reply; 27+ messages in thread From: Steven Rostedt @ 2015-01-21 14:25 UTC (permalink / raw) To: Jiri Olsa; +Cc: Wang Nan, jeremie.galarneau, bigeasy, lizefan, linux-kernel On Wed, 21 Jan 2015 15:19:38 +0100 Jiri Olsa <jolsa@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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 14:25 ` Steven Rostedt @ 2015-01-21 14:32 ` Jiri Olsa 2015-01-22 5:35 ` [PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field Wang Nan 0 siblings, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-21 14:32 UTC (permalink / raw) To: Steven Rostedt Cc: Wang Nan, jeremie.galarneau, bigeasy, lizefan, linux-kernel On Wed, Jan 21, 2015 at 09:25:45AM -0500, Steven Rostedt wrote: > On Wed, 21 Jan 2015 15:19:38 +0100 > Jiri Olsa <jolsa@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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field. 2015-01-21 14:32 ` Jiri Olsa @ 2015-01-22 5:35 ` Wang Nan 2015-01-22 5:36 ` [PATCH RFC 1/2] tools lib traceevent: add priv field to truct format_field Wang Nan 2015-01-22 5:36 ` [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 0 siblings, 2 replies; 27+ messages in thread From: Wang Nan @ 2015-01-22 5:35 UTC (permalink / raw) To: jolsa, rostedt; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel Hi Jiri Olsa and Steven Rostedt (and others), Here I post 2 patches. Patch 1 is for Steven Rostedt for his question: https://lkml.org/lkml/2015/1/21/397 (Jiri, I hope you don't mind me posting this patch if you are working on it. I just want to solve the problem.) The second patch is for Jiri. If the first patch is acceptable, we can use it to deal with name conversion problem. Thanks. Wang Nan (2): tools lib traceevent: add priv field to truct format_field. perf: convert: fix duplicate field names and avoid reserved keywords. tools/lib/traceevent/event-parse.c | 2 + tools/lib/traceevent/event-parse.h | 2 + tools/perf/util/data-convert-bt.c | 94 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 94 insertions(+), 4 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH RFC 1/2] tools lib traceevent: add priv field to truct format_field. 2015-01-22 5:35 ` [PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field Wang Nan @ 2015-01-22 5:36 ` Wang Nan 2015-01-22 5:36 ` [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 1 sibling, 0 replies; 27+ messages in thread From: Wang Nan @ 2015-01-22 5:36 UTC (permalink / raw) To: jolsa, rostedt; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel In https://lkml.org/lkml/2015/1/21/383 , Jiri Olsa gives a suggestion about changing lib traceevent to solve a bug of perf-convert-to-ctf, which is related to duplicated field names. I think his suggestion should be something like this patch. Signed-off-by: Wang Nan <wangnan0@huawei.com> --- tools/lib/traceevent/event-parse.c | 2 ++ tools/lib/traceevent/event-parse.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c index cf3a44b..5f76003 100644 --- a/tools/lib/traceevent/event-parse.c +++ b/tools/lib/traceevent/event-parse.c @@ -5909,6 +5909,8 @@ static void free_format_fields(struct format_field *field) free(field->type); free(field->name); free(field); + if (field->destroy_priv) + field->destroy_priv(field); field = next; } } diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h index 7a3873f..928d801 100644 --- a/tools/lib/traceevent/event-parse.h +++ b/tools/lib/traceevent/event-parse.h @@ -190,6 +190,8 @@ struct format_field { unsigned int arraylen; unsigned int elementsize; unsigned long flags; + void *priv; + void (*destroy_priv)(struct format_field *); }; struct format { -- 1.8.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 5:35 ` [PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field Wang Nan 2015-01-22 5:36 ` [PATCH RFC 1/2] tools lib traceevent: add priv field to truct format_field Wang Nan @ 2015-01-22 5:36 ` Wang Nan 2015-01-22 13:27 ` Jiri Olsa 1 sibling, 1 reply; 27+ messages in thread From: Wang Nan @ 2015-01-22 5:36 UTC (permalink / raw) To: jolsa, rostedt; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel (If Steven Rostedt accept the previous patch which introduce a priv field to 'struct format_field', we can use a relative simple method for name conversion. If not , perf must track name conversion by itself.) 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 <wangnan0@huawei.com> --- tools/perf/util/data-convert-bt.c | 94 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 4 deletions(-) diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c index ddecce8..a94f3bd 100644 --- a/tools/perf/util/data-convert-bt.c +++ b/tools/perf/util/data-convert-bt.c @@ -184,6 +184,7 @@ static int add_tracepoint_field_value(struct ctf_writer *cw, unsigned int len; int ret; + name = fmtf->priv ? (const char *)fmtf->priv : fmtf->name; offset = fmtf->offset; len = fmtf->size; if (flags & FIELD_IS_STRING) @@ -567,6 +568,91 @@ 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 void destroy_field_priv(struct format_field *field) +{ + if (!field->priv) + return; + + if (field->priv != field->name) + free(field->priv); + + field->priv = NULL; + field->destroy_priv = NULL; +} + +static int event_class_add_field(struct bt_ctf_event_class *event_class, + struct bt_ctf_field_type *type, + struct format_field *field) +{ + struct bt_ctf_field_type *t = NULL; + char *name; + int dup = 1; + int ret; + + if (field->priv) + return bt_ctf_event_class_add_field(event_class, type, + (char *)field->priv); + + name = field->name; + while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { + bt_ctf_field_type_put(t); + 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); + } + + field->priv = name; + field->destroy_priv = destroy_field_priv; + + return ret; +} + static int add_tracepoint_fields_types(struct ctf_writer *cw, struct format_field *fields, struct bt_ctf_event_class *event_class) @@ -595,14 +681,14 @@ 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); + ret = event_class_add_field(event_class, type, field); 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 +732,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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 5:36 ` [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan @ 2015-01-22 13:27 ` Jiri Olsa 2015-01-23 1:57 ` Wang Nan 0 siblings, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-22 13:27 UTC (permalink / raw) To: Wang Nan; +Cc: rostedt, jeremie.galarneau, bigeasy, lizefan, linux-kernel On Thu, Jan 22, 2015 at 01:36:43PM +0800, Wang Nan wrote: > (If Steven Rostedt accept the previous patch which introduce a priv > field to 'struct format_field', we can use a relative simple method > for name conversion. If not , perf must track name conversion by > itself.) > > 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. I've got compilation error: util/data-convert-bt.c: In function ‘event_class_add_field’: util/data-convert-bt.c:629:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { what's your gcc version? mine's caught that.. [jolsa@krava perf]$ gcc --version gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7) SNIP > > +/* 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"); if we allow for _dupl_10, should we use 'sizeof("_dupl_x")' ^^^ in here? > + > + 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; SNIP > + > + name = field->name; > + while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { > + bt_ctf_field_type_put(t); > + 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); so there's no other way on checking up with the blacklist right? thanks, jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 13:27 ` Jiri Olsa @ 2015-01-23 1:57 ` Wang Nan 2015-01-23 2:53 ` Wang Nan ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Wang Nan @ 2015-01-23 1:57 UTC (permalink / raw) To: Jiri Olsa, jeremie.galarneau; +Cc: rostedt, bigeasy, lizefan, linux-kernel On 2015/1/22 21:27, Jiri Olsa wrote: > On Thu, Jan 22, 2015 at 01:36:43PM +0800, Wang Nan wrote: >> (If Steven Rostedt accept the previous patch which introduce a priv >> field to 'struct format_field', we can use a relative simple method >> for name conversion. If not , perf must track name conversion by >> itself.) >> >> 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. > > I've got compilation error: > > util/data-convert-bt.c: In function ‘event_class_add_field’: > util/data-convert-bt.c:629:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] > while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { > > what's your gcc version? mine's caught that.. > I also curious why you got so many Werror problems I'm not ever seen, until I found a '-w' in my gcc options, which is introduced by your commit 47810c1d429bc690e1f5e9467697538921962171: perf data: Disable Werror convert object. I'll revert that commit in my tree. > [jolsa@krava perf]$ gcc --version > gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7) > > SNIP > >> >> +/* 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"); > > if we allow for _dupl_10, should we use 'sizeof("_dupl_x")' ^^^ in here? > >> + >> + 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; > > SNIP > >> + >> + name = field->name; >> + while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { >> + bt_ctf_field_type_put(t); >> + 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); > > so there's no other way on checking up with the blacklist right? > AFAIK there's no official method to check blacklist right now. Utilizing existing functions to check blacklist is possible. For example, we can create a clock using bt_ctf_clock_create() with the checked name and then free it. However, it is hacky and I think you won't like it. I believe my solution should be acceptable before babeltrace export its validate_identifier() function to users. Jérémie Galarneau, do you have better idea on it? Thanks. > thanks, > jirka > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-23 1:57 ` Wang Nan @ 2015-01-23 2:53 ` Wang Nan 2015-01-23 8:44 ` Jiri Olsa 2015-01-23 8:45 ` Jiri Olsa 2015-01-23 22:40 ` Jérémie Galarneau 2 siblings, 1 reply; 27+ messages in thread From: Wang Nan @ 2015-01-23 2:53 UTC (permalink / raw) To: Jiri Olsa; +Cc: jeremie.galarneau, rostedt, bigeasy, lizefan, linux-kernel On 2015/1/23 9:57, Wang Nan wrote: > On 2015/1/22 21:27, Jiri Olsa wrote: >> On Thu, Jan 22, 2015 at 01:36:43PM +0800, Wang Nan wrote: >>> (If Steven Rostedt accept the previous patch which introduce a priv >>> field to 'struct format_field', we can use a relative simple method >>> for name conversion. If not , perf must track name conversion by >>> itself.) >>> >>> 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. >> >> I've got compilation error: >> >> util/data-convert-bt.c: In function ‘event_class_add_field’: >> util/data-convert-bt.c:629:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] >> while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { >> >> what's your gcc version? mine's caught that.. >> > > I also curious why you got so many Werror problems I'm not ever seen, > until I found a '-w' in my gcc options, which is introduced by your commit > > 47810c1d429bc690e1f5e9467697538921962171: perf data: Disable Werror convert object. > > I'll revert that commit in my tree. > >> [jolsa@krava perf]$ gcc --version >> gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7) >> >> SNIP >> >>> >>> +/* 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"); >> >> if we allow for _dupl_10, should we use 'sizeof("_dupl_x")' ^^^ in here? >> We don't allow _dupl_10. If dup is 10 or larger (see above two if clause), this function will return NULL. >>> + >>> + 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; >> >> SNIP >> >>> + >>> + name = field->name; >>> + while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { >>> + bt_ctf_field_type_put(t); >>> + 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); >> >> so there's no other way on checking up with the blacklist right? >> > > AFAIK there's no official method to check blacklist right now. Utilizing existing > functions to check blacklist is possible. For example, we can create a clock using > bt_ctf_clock_create() with the checked name and then free it. However, it is hacky > and I think you won't like it. > > I believe my solution should be acceptable before babeltrace export its > validate_identifier() function to users. Jérémie Galarneau, do you have better > idea on it? > > Thanks. > >> thanks, >> jirka >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-23 2:53 ` Wang Nan @ 2015-01-23 8:44 ` Jiri Olsa 0 siblings, 0 replies; 27+ messages in thread From: Jiri Olsa @ 2015-01-23 8:44 UTC (permalink / raw) To: Wang Nan; +Cc: jeremie.galarneau, rostedt, bigeasy, lizefan, linux-kernel On Fri, Jan 23, 2015 at 10:53:07AM +0800, Wang Nan wrote: SNIP > >> SNIP > >> > >>> > >>> +/* 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"); > >> > >> if we allow for _dupl_10, should we use 'sizeof("_dupl_x")' ^^^ in here? > >> > > We don't allow _dupl_10. If dup is 10 or larger (see above two if clause), this function will > return NULL. ouch right.. ok ;-) thanks, jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-23 1:57 ` Wang Nan 2015-01-23 2:53 ` Wang Nan @ 2015-01-23 8:45 ` Jiri Olsa 2015-01-23 22:40 ` Jérémie Galarneau 2 siblings, 0 replies; 27+ messages in thread From: Jiri Olsa @ 2015-01-23 8:45 UTC (permalink / raw) To: Wang Nan; +Cc: jeremie.galarneau, rostedt, bigeasy, lizefan, linux-kernel On Fri, Jan 23, 2015 at 09:57:53AM +0800, Wang Nan wrote: > On 2015/1/22 21:27, Jiri Olsa wrote: > > On Thu, Jan 22, 2015 at 01:36:43PM +0800, Wang Nan wrote: > >> (If Steven Rostedt accept the previous patch which introduce a priv > >> field to 'struct format_field', we can use a relative simple method > >> for name conversion. If not , perf must track name conversion by > >> itself.) > >> > >> 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. > > > > I've got compilation error: > > > > util/data-convert-bt.c: In function ‘event_class_add_field’: > > util/data-convert-bt.c:629:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] > > while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { > > > > what's your gcc version? mine's caught that.. > > > > I also curious why you got so many Werror problems I'm not ever seen, > until I found a '-w' in my gcc options, which is introduced by your commit > > 47810c1d429bc690e1f5e9467697538921962171: perf data: Disable Werror convert object. > > I'll revert that commit in my tree. that one was to workaround the regression in babeltrace, I'm now using sane babeltrace HEAD as advertized by Jeremie: "In the meantime, testing against Babeltrace master 3baf0856 should be alright." jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-23 1:57 ` Wang Nan 2015-01-23 2:53 ` Wang Nan 2015-01-23 8:45 ` Jiri Olsa @ 2015-01-23 22:40 ` Jérémie Galarneau 2015-01-24 16:39 ` Jiri Olsa 2 siblings, 1 reply; 27+ messages in thread From: Jérémie Galarneau @ 2015-01-23 22:40 UTC (permalink / raw) To: Wang Nan Cc: Jiri Olsa, Steven Rostedt, Sebastian Andrzej Siewior, Li Zefan, linux-kernel On Thu, Jan 22, 2015 at 8:57 PM, Wang Nan <wangnan0@huawei.com> wrote: > On 2015/1/22 21:27, Jiri Olsa wrote: >> On Thu, Jan 22, 2015 at 01:36:43PM +0800, Wang Nan wrote: >>> (If Steven Rostedt accept the previous patch which introduce a priv >>> field to 'struct format_field', we can use a relative simple method >>> for name conversion. If not , perf must track name conversion by >>> itself.) >>> >>> 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. >> >> I've got compilation error: >> >> util/data-convert-bt.c: In function ‘event_class_add_field’: >> util/data-convert-bt.c:629:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] >> while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { >> >> what's your gcc version? mine's caught that.. >> > > I also curious why you got so many Werror problems I'm not ever seen, > until I found a '-w' in my gcc options, which is introduced by your commit > > 47810c1d429bc690e1f5e9467697538921962171: perf data: Disable Werror convert object. > > I'll revert that commit in my tree. > >> [jolsa@krava perf]$ gcc --version >> gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7) >> >> SNIP >> >>> >>> +/* 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"); >> >> if we allow for _dupl_10, should we use 'sizeof("_dupl_x")' ^^^ in here? >> >>> + >>> + 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; >> >> SNIP >> >>> + >>> + name = field->name; >>> + while (t = bt_ctf_event_class_get_field_by_name(event_class, name)) { >>> + bt_ctf_field_type_put(t); >>> + 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); >> >> so there's no other way on checking up with the blacklist right? >> > > AFAIK there's no official method to check blacklist right now. Utilizing existing > functions to check blacklist is possible. For example, we can create a clock using > bt_ctf_clock_create() with the checked name and then free it. However, it is hacky > and I think you won't like it. The prospect of seeing that code has convinced me to introduce int bt_ctf_validate_identifier(const char *identifier); commit 654c1444b546fd79b209288b93ed4e87d9bb8a2b Author: Jérémie Galarneau <jeremie.galarneau@efficios.com> Date: Fri Jan 23 16:24:52 2015 -0500 Add utility function to validate CTF identifiers Introduces bt_ctf_validate_identifier() which validates a given identifier against the list of CTF reserved keywords. This function may evolve to perform additional validity checks in the future as the CTF specification moves forward. Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> Does that take care of the problem? Jérémie > > I believe my solution should be acceptable before babeltrace export its > validate_identifier() function to users. Jérémie Galarneau, do you have better > idea on it? > > Thanks. > >> thanks, >> jirka >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Jérémie Galarneau EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-23 22:40 ` Jérémie Galarneau @ 2015-01-24 16:39 ` Jiri Olsa 2015-01-26 11:11 ` Wang Nan 0 siblings, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-24 16:39 UTC (permalink / raw) To: Jérémie Galarneau Cc: Wang Nan, Steven Rostedt, Sebastian Andrzej Siewior, Li Zefan, linux-kernel On Fri, Jan 23, 2015 at 05:40:02PM -0500, Jérémie Galarneau wrote: SNIP > > bt_ctf_clock_create() with the checked name and then free it. However, it is hacky > > and I think you won't like it. > > The prospect of seeing that code has convinced me to introduce > int bt_ctf_validate_identifier(const char *identifier); > > commit 654c1444b546fd79b209288b93ed4e87d9bb8a2b > Author: Jérémie Galarneau <jeremie.galarneau@efficios.com> > Date: Fri Jan 23 16:24:52 2015 -0500 > > Add utility function to validate CTF identifiers > > Introduces bt_ctf_validate_identifier() which validates a given > identifier against the list of CTF reserved keywords. > > This function may evolve to perform additional validity checks in > the future as the CTF specification moves forward. > > Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> > > > Does that take care of the problem? seems good to me, thanks jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-24 16:39 ` Jiri Olsa @ 2015-01-26 11:11 ` Wang Nan 0 siblings, 0 replies; 27+ messages in thread From: Wang Nan @ 2015-01-26 11:11 UTC (permalink / raw) To: Jiri Olsa, Jérémie Galarneau Cc: Steven Rostedt, Sebastian Andrzej Siewior, Li Zefan, linux-kernel Hi all, I posted a new series of patches for this problem. I open another thread to avoid disorder this thread. Please see: https://lkml.org/lkml/2015/1/26/198 Thank you. On 2015/1/25 0:39, Jiri Olsa wrote: > On Fri, Jan 23, 2015 at 05:40:02PM -0500, Jérémie Galarneau wrote: > > SNIP > >>> bt_ctf_clock_create() with the checked name and then free it. However, it is hacky >>> and I think you won't like it. >> >> The prospect of seeing that code has convinced me to introduce >> int bt_ctf_validate_identifier(const char *identifier); >> >> commit 654c1444b546fd79b209288b93ed4e87d9bb8a2b >> Author: Jérémie Galarneau <jeremie.galarneau@efficios.com> >> Date: Fri Jan 23 16:24:52 2015 -0500 >> >> Add utility function to validate CTF identifiers >> >> Introduces bt_ctf_validate_identifier() which validates a given >> identifier against the list of CTF reserved keywords. >> >> This function may evolve to perform additional validity checks in >> the future as the CTF specification moves forward. >> >> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com> >> >> >> Does that take care of the problem? > > seems good to me, thanks > > jirka > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 3:23 ` [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 2015-01-21 11:16 ` Wang Nan @ 2015-01-21 14:11 ` Jiri Olsa 2015-01-21 15:56 ` Jérémie Galarneau 1 sibling, 1 reply; 27+ messages in thread From: Jiri Olsa @ 2015-01-21 14:11 UTC (permalink / raw) To: Wang Nan; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 14:11 ` [PATCH 1/2] " Jiri Olsa @ 2015-01-21 15:56 ` Jérémie Galarneau 2015-01-22 1:38 ` Wang Nan 0 siblings, 1 reply; 27+ messages in thread From: Jérémie Galarneau @ 2015-01-21 15:56 UTC (permalink / raw) To: Jiri Olsa; +Cc: Wang Nan, Sebastian Andrzej Siewior, Li Zefan, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-21 15:56 ` Jérémie Galarneau @ 2015-01-22 1:38 ` Wang Nan 2015-01-22 3:14 ` Jérémie Galarneau 0 siblings, 1 reply; 27+ messages in thread From: Wang Nan @ 2015-01-22 1:38 UTC (permalink / raw) To: Jérémie Galarneau, Jiri Olsa Cc: Sebastian Andrzej Siewior, Li Zefan, linux-kernel 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? > >> 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 > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 1:38 ` Wang Nan @ 2015-01-22 3:14 ` Jérémie Galarneau 2015-01-22 3:21 ` Wang Nan 2015-01-23 20:45 ` Mathieu Desnoyers 0 siblings, 2 replies; 27+ messages in thread From: Jérémie Galarneau @ 2015-01-22 3:14 UTC (permalink / raw) To: Wang Nan Cc: Jiri Olsa, Sebastian Andrzej Siewior, Li Zefan, linux-kernel, Mathieu Desnoyers 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). 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 3:14 ` Jérémie Galarneau @ 2015-01-22 3:21 ` Wang Nan 2015-01-23 20:45 ` Mathieu Desnoyers 1 sibling, 0 replies; 27+ messages in thread From: Wang Nan @ 2015-01-22 3:21 UTC (permalink / raw) To: Jérémie Galarneau Cc: Jiri Olsa, Sebastian Andrzej Siewior, Li Zefan, linux-kernel, Mathieu Desnoyers 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 >>> >>> >>> >> >> > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords. 2015-01-22 3:14 ` Jérémie Galarneau 2015-01-22 3:21 ` Wang Nan @ 2015-01-23 20:45 ` Mathieu Desnoyers 1 sibling, 0 replies; 27+ messages in thread From: Mathieu Desnoyers @ 2015-01-23 20:45 UTC (permalink / raw) To: Jérémie Galarneau Cc: Wang Nan, Jiri Olsa, Sebastian Andrzej Siewior, Li Zefan, linux-kernel ----- Original Message ----- > From: "Jérémie Galarneau" <jeremie.galarneau@efficios.com> > To: "Wang Nan" <wangnan0@huawei.com> > Cc: "Jiri Olsa" <jolsa@redhat.com>, "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>, "Li Zefan" > <lizefan@huawei.com>, linux-kernel@vger.kernel.org, "Mathieu Desnoyers" <mathieu.desnoyers@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 <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). > > 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] perf: convert: fix signess of value. 2015-01-20 11:07 [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Wang Nan 2015-01-20 11:07 ` [PATCH 1/2] perf: convert: fix duplicate field names Wang Nan @ 2015-01-20 11:07 ` Wang Nan 2015-01-20 13:06 ` [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Jiri Olsa 2 siblings, 0 replies; 27+ messages in thread From: Wang Nan @ 2015-01-20 11:07 UTC (permalink / raw) To: jolsa; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel When converting int values, perf first extractes it to a ulonglong, then feeds it to babeltrace as a signed value. For negative 32 bit values (for example, return values of failed syscalls), the extracted data should be something like 0xfffffffe (-2). It becomes a large int64 value. Babeltrace denies to insert it with bt_ctf_field_signed_integer_set_value() because it is larger than 0x7fffffff, the largest positive value a signed 32 bit int can be. This patch introduces adjust_signess(), which fills high bits of ulonglong with 1 if the value is negative. Signed-off-by: Wang Nan <wangnan0@huawei.com> --- tools/perf/util/data-convert-bt.c | 41 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c index 2b87097..d43b8a1 100644 --- a/tools/perf/util/data-convert-bt.c +++ b/tools/perf/util/data-convert-bt.c @@ -165,6 +165,44 @@ get_tracepoint_field_type(struct ctf_writer *cw, struct format_field *field) return cw->data.u32; } +static unsigned long long adjust_signess(unsigned long long value_int, int size) +{ + int signbit; + unsigned long long value_mask; + + /* + * value_mask = (1 << (size * 8 - 1)) - 1. + * Directly set value_mask for code readers. + */ + switch (size) { + case 1: + value_mask = 0x7fULL; + break; + case 2: + value_mask = 0x7fffULL; + break; + case 4: + value_mask = 0x7fffffffULL; + break; + case 8: + /* + * For 64 bit value, return it self. There is no need + * to fill high bit. + */ + /* Fall through */ + default: + /* BUG! */ + return value_int; + } + + /* If it is a positive value, don't adjust. */ + if ((value_int & (~0ULL - value_mask)) == 0) + return value_int; + + /* Fill upper part of value_int with 1 to make it a negative long long. */ + return (value_int & value_mask) | ~value_mask; +} + static int add_tracepoint_field_value(struct ctf_writer *cw, struct bt_ctf_event_class *event_class, struct bt_ctf_event *event, @@ -243,7 +281,8 @@ static int add_tracepoint_field_value(struct ctf_writer *cw, field, value_int); else ret = bt_ctf_field_signed_integer_set_value( - field, value_int); + field, adjust_signess(value_int, len)); + if (ret) { pr_err("failed to set file value %s\n", name); goto err_put_field; -- 1.8.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns. 2015-01-20 11:07 [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Wang Nan 2015-01-20 11:07 ` [PATCH 1/2] perf: convert: fix duplicate field names Wang Nan 2015-01-20 11:07 ` [PATCH 2/2] perf: convert: fix signess of value Wang Nan @ 2015-01-20 13:06 ` Jiri Olsa 2 siblings, 0 replies; 27+ messages in thread From: Jiri Olsa @ 2015-01-20 13:06 UTC (permalink / raw) To: Wang Nan; +Cc: jeremie.galarneau, bigeasy, lizefan, linux-kernel On Tue, Jan 20, 2015 at 07:07:07PM +0800, Wang Nan wrote: > This 2 patches are based on Jiri Olsa's perf repository: > > https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/core_ctf_convert > > Perf data convert to failes when converting perf.data recorded with > > # perf record -a -e syscalls:* sleep 1 > > The following 2 patches fix it. > > Wang Nan (2): > perf: convert: fix duplicate field names. > perf: convert: fix signess of value. > > tools/perf/util/data-convert-bt.c | 101 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 96 insertions(+), 5 deletions(-) > > -- > 1.8.4 hi, also I've got following error: BUILD: Doing 'make -j1' parallel build CC util/data-convert-bt.o util/data-convert-bt.c: In function ‘adjust_signess’: util/data-convert-bt.c:170:6: error: unused variable ‘signbit’ [-Werror=unused-variable] int signbit; ^ util/data-convert-bt.c: In function ‘add_tracepoint_fields_types’: util/data-convert-bt.c:668:30: error: unused variable ‘f’ [-Werror=unused-variable] struct bt_ctf_field_type *f; ^ cc1: all warnings being treated as errors make[1]: *** [util/data-convert-bt.o] Error 1 make: *** [all] Error 2 jirka ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-01-26 11:11 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-20 11:07 [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Wang Nan 2015-01-20 11:07 ` [PATCH 1/2] perf: convert: fix duplicate field names Wang Nan 2015-01-20 13:06 ` Jiri Olsa 2015-01-21 3:23 ` [PATCH 1/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 2015-01-21 11:16 ` Wang Nan 2015-01-21 14:19 ` Jiri Olsa 2015-01-21 14:25 ` Steven Rostedt 2015-01-21 14:32 ` Jiri Olsa 2015-01-22 5:35 ` [PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field Wang Nan 2015-01-22 5:36 ` [PATCH RFC 1/2] tools lib traceevent: add priv field to truct format_field Wang Nan 2015-01-22 5:36 ` [PATCH RFC 2/2] perf: convert: fix duplicate field names and avoid reserved keywords Wang Nan 2015-01-22 13:27 ` Jiri Olsa 2015-01-23 1:57 ` Wang Nan 2015-01-23 2:53 ` Wang Nan 2015-01-23 8:44 ` Jiri Olsa 2015-01-23 8:45 ` Jiri Olsa 2015-01-23 22:40 ` Jérémie Galarneau 2015-01-24 16:39 ` Jiri Olsa 2015-01-26 11:11 ` Wang Nan 2015-01-21 14:11 ` [PATCH 1/2] " Jiri Olsa 2015-01-21 15:56 ` Jérémie Galarneau 2015-01-22 1:38 ` Wang Nan 2015-01-22 3:14 ` Jérémie Galarneau 2015-01-22 3:21 ` Wang Nan 2015-01-23 20:45 ` Mathieu Desnoyers 2015-01-20 11:07 ` [PATCH 2/2] perf: convert: fix signess of value Wang Nan 2015-01-20 13:06 ` [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns Jiri Olsa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.