[PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns.

[PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns.

[Wang Nan]

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

[PATCH 1/2] perf: convert: fix duplicate field names.

[Wang Nan]

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

[PATCH 2/2] perf: convert: fix signess of value.

[Wang Nan]

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

Re: [PATCH 1/2] perf: convert: fix duplicate field names.

[Jiri Olsa]

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

Re: [PATCH 0/2] perf: convert: two patches for converting syscall tracepoitns.

[Jiri Olsa]

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

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

[Wang Nan]

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

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

[Wang Nan]

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

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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

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

[Steven Rostedt]

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

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

[Jiri Olsa]

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

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

[Jérémie Galarneau]

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

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

[Wang Nan]

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

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

[Jérémie Galarneau]

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

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

[Wang Nan]

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

[PATCH RFC 0/2] tools lib traceevent: introduces priv field to struct format_field.

[Wang Nan]

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

[PATCH RFC 1/2] tools lib traceevent: add priv field to truct format_field.

[Wang Nan]

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

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

[Wang Nan]

(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

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

[Jiri Olsa]

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

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

[Wang Nan]

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
> 

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

[Wang Nan]

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

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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

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

[Mathieu Desnoyers]

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

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

[Jérémie Galarneau]

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

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

[Jiri Olsa]

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

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

[Wang Nan]

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
>