[RFC PATCHSET 0/3] tools lib traceevent: Generic error handling for pevent

[RFC PATCHSET 0/3] tools lib traceevent: Generic error handling for pevent

[Namhyung Kim]

Hi,

This is a small patch series for preparation of the die removal.
I added some error code to be used for pevent_parse_event() and
_strerror function much like the perf target does.

I don't like the name 'pevent' but since it's a part of the API
so I used it for the name.

This can eliminate a few of die() and do_warning() but will lose
some details on the error message - i.e. event name. I couldn't
think of a way that doesn't lose such info after returning from
the function. But I guess we might get the info from the context.

Patch 01 is independent and can be applied separately.

Any comments are welcome.

Thanks,
Namhyung


Namhyung Kim (3):
  tools lib traceevent: Do not link broken field arg for an old ftrace event
  tools lib traceevent: Introduce pevent_errno
  tools lib traceevent: Introduce pevent_strerror

 tools/lib/traceevent/event-parse.c |  104 ++++++++++++++++++++++++++++--------
 tools/lib/traceevent/event-parse.h |   28 +++++++++-
 2 files changed, 107 insertions(+), 25 deletions(-)

-- 
1.7.10.2

[PATCH 1/3] tools lib traceevent: Do not link broken field arg for an old ftrace event

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Defer linking a newly allocated arg to print_fmt.args until
all of its field is setup so that later access to ->field.name
cannot be NULL.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/event-parse.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index a6b5bcdc5580..7fcfdf9e2716 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -4665,20 +4665,20 @@ int pevent_parse_event(struct pevent *pevent,
 		struct print_arg *arg, **list;
 
 		/* old ftrace had no args */
-
 		list = &event->print_fmt.args;
 		for (field = event->format.fields; field; field = field->next) {
 			arg = alloc_arg();
-			*list = arg;
-			list = &arg->next;
 			arg->type = PRINT_FIELD;
 			arg->field.name = strdup(field->name);
 			if (!arg->field.name) {
 				do_warning("failed to allocate field name");
 				event->flags |= EVENT_FL_FAILED;
+				free_arg(arg);
 				return -1;
 			}
 			arg->field.field = field;
+			*list = arg;
+			list = &arg->next;
 		}
 		return 0;
 	}
-- 
1.7.10.2

[PATCH 2/3] tools lib traceevent: Introduce pevent_errno

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Define and use error numbers for pevent_parse_event()
and get rid of die() and do_warning() calls. If the
function returns non-zero value, the caller can check
the return code and do appropriate things.

I chose the error numbers to be negative not to clash
with standard errno, and as usual, 0 for success.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/event-parse.c |   50 +++++++++++++++++++++---------------
 tools/lib/traceevent/event-parse.h |   26 +++++++++++++++++--
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 7fcfdf9e2716..866bdca4825d 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -4597,9 +4597,8 @@ static int find_event_handle(struct pevent *pevent, struct event_format *event)
  *
  * /sys/kernel/debug/tracing/events/.../.../format
  */
-int pevent_parse_event(struct pevent *pevent,
-		       const char *buf, unsigned long size,
-		       const char *sys)
+enum pevent_errno pevent_parse_event(struct pevent *pevent, const char *buf,
+				     unsigned long size, const char *sys)
 {
 	struct event_format *event;
 	int ret;
@@ -4608,17 +4607,16 @@ int pevent_parse_event(struct pevent *pevent,
 
 	event = alloc_event();
 	if (!event)
-		return -ENOMEM;
+		return PEVENT_ERRNO__MEM_ALLOC_FAILED;
 
 	event->name = event_read_name();
 	if (!event->name) {
 		/* Bad event? */
-		free(event);
-		return -1;
+		ret = PEVENT_ERRNO__MEM_ALLOC_FAILED;
+		goto event_alloc_failed;
 	}
 
 	if (strcmp(sys, "ftrace") == 0) {
-
 		event->flags |= EVENT_FL_ISFTRACE;
 
 		if (strcmp(event->name, "bprint") == 0)
@@ -4626,20 +4624,28 @@ int pevent_parse_event(struct pevent *pevent,
 	}
 		
 	event->id = event_read_id();
-	if (event->id < 0)
-		die("failed to read event id");
+	if (event->id < 0) {
+		ret = PEVENT_ERRNO__READ_ID_FAILED;
+		/*
+		 * This isn't an allocation error actually.
+		 * But as the ID is critical, just bail out.
+		 */
+		goto event_alloc_failed;
+	}
 
 	event->system = strdup(sys);
-	if (!event->system)
-		die("failed to allocate system");
+	if (!event->system) {
+		ret = PEVENT_ERRNO__MEM_ALLOC_FAILED;
+		goto event_alloc_failed;
+	}
 
 	/* Add pevent to event so that it can be referenced */
 	event->pevent = pevent;
 
 	ret = event_read_format(event);
 	if (ret < 0) {
-		do_warning("failed to read event format for %s", event->name);
-		goto event_failed;
+		ret = PEVENT_ERRNO__READ_FORMAT_FAILED;
+		goto event_parse_failed;
 	}
 
 	/*
@@ -4651,10 +4657,9 @@ int pevent_parse_event(struct pevent *pevent,
 
 	ret = event_read_print(event);
 	if (ret < 0) {
-		do_warning("failed to read event print fmt for %s",
-			   event->name);
 		show_warning = 1;
-		goto event_failed;
+		ret = PEVENT_ERRNO__READ_PRINT_FAILED;
+		goto event_parse_failed;
 	}
 	show_warning = 1;
 
@@ -4671,10 +4676,9 @@ int pevent_parse_event(struct pevent *pevent,
 			arg->type = PRINT_FIELD;
 			arg->field.name = strdup(field->name);
 			if (!arg->field.name) {
-				do_warning("failed to allocate field name");
 				event->flags |= EVENT_FL_FAILED;
 				free_arg(arg);
-				return -1;
+				return PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED;
 			}
 			arg->field.field = field;
 			*list = arg;
@@ -4689,11 +4693,17 @@ int pevent_parse_event(struct pevent *pevent,
 
 	return 0;
 
- event_failed:
+ event_parse_failed:
 	event->flags |= EVENT_FL_FAILED;
 	/* still add it even if it failed */
 	add_event(pevent, event);
-	return -1;
+	return ret;
+
+ event_alloc_failed:
+	free(event->system);
+	free(event->name);
+	free(event);
+	return ret;
 }
 
 int get_field_val(struct trace_seq *s, struct format_field *field,
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index ac997bc7b592..24b7649d530b 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -338,6 +338,28 @@ enum pevent_flag {
 	PEVENT_NSEC_OUTPUT		= 1,	/* output in NSECS */
 };
 
+enum pevent_errno {
+	PEVENT_ERRNO__SUCCESS			= 0,
+
+	/*
+	 * Choose an arbitrary negative big number not to clash with standard
+	 * errno since SUS requires the errno has distinct positive values.
+	 * See 'Issue 6' in the link below.
+	 *
+	 * http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
+	 */
+	__PEVENT_ERRNO__START			= -100000,
+
+	PEVENT_ERRNO__MEM_ALLOC_FAILED		= __PEVENT_ERRNO__START,
+	PEVENT_ERRNO__PARSE_EVENT_FAILED,
+	PEVENT_ERRNO__READ_ID_FAILED,
+	PEVENT_ERRNO__READ_FORMAT_FAILED,
+	PEVENT_ERRNO__READ_PRINT_FAILED,
+	PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED,
+
+	__PEVENT_ERRNO__END,
+};
+
 struct cmdline;
 struct cmdline_list;
 struct func_map;
@@ -502,8 +524,8 @@ void pevent_print_event(struct pevent *pevent, struct trace_seq *s,
 int pevent_parse_header_page(struct pevent *pevent, char *buf, unsigned long size,
 			     int long_size);
 
-int pevent_parse_event(struct pevent *pevent, const char *buf,
-		       unsigned long size, const char *sys);
+enum pevent_errno pevent_parse_event(struct pevent *pevent, const char *buf,
+				     unsigned long size, const char *sys);
 
 void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
 			   const char *name, struct pevent_record *record,
-- 
1.7.10.2

[PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The pevent_strerror() sets @buf to a string that describes the
(libtraceevent-specific) error condition that is passed via @errnum.

This is similar to strerror_r() and does same thing if @errnum has a
standard errno value.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/event-parse.c |   48 ++++++++++++++++++++++++++++++++++++
 tools/lib/traceevent/event-parse.h |    2 ++
 2 files changed, 50 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 866bdca4825d..3ed2d9362e69 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -4706,6 +4706,54 @@ enum pevent_errno pevent_parse_event(struct pevent *pevent, const char *buf,
 	return ret;
 }
 
+/*
+ * This must have a same ordering as the enum pevent_errno.
+ */
+static const char * const pevent_error_str[] = {
+	"failed to allocate memory",
+	"failed to parse event",
+	"failed to read event id",
+	"failed to read event format",
+	"failed to read event print fmt",
+	"failed to allocate field name for ftrace",
+};
+
+int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
+		    char *buf, size_t buflen)
+{
+	int idx;
+	const char *msg;
+
+	if (errnum >= 0) {
+		strerror_r(errnum, buf, buflen);
+		return 0;
+	}
+
+	if (errnum <  __PEVENT_ERRNO__START ||
+	    errnum >= __PEVENT_ERRNO__END)
+		return -1;
+
+	idx = errnum - __PEVENT_ERRNO__START;
+	msg = pevent_error_str[idx];
+
+	switch (errnum) {
+	case PEVENT_ERRNO__MEM_ALLOC_FAILED:
+	case PEVENT_ERRNO__PARSE_EVENT_FAILED:
+	case PEVENT_ERRNO__READ_ID_FAILED:
+	case PEVENT_ERRNO__READ_FORMAT_FAILED:
+	case PEVENT_ERRNO__READ_PRINT_FAILED:
+	case PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED:
+		snprintf(buf, buflen, "%s", msg);
+		break;
+
+	default:
+		/* cannot reach here */
+		break;
+	}
+
+	return 0;
+}
+
 int get_field_val(struct trace_seq *s, struct format_field *field,
 		  const char *name, struct pevent_record *record,
 		  unsigned long long *val, int err)
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 24b7649d530b..7e90aba20850 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -576,6 +576,8 @@ int pevent_data_pid(struct pevent *pevent, struct pevent_record *rec);
 const char *pevent_data_comm_from_pid(struct pevent *pevent, int pid);
 void pevent_event_info(struct trace_seq *s, struct event_format *event,
 		       struct pevent_record *record);
+int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
+		    char *buf, size_t buflen);
 
 struct event_format **pevent_list_events(struct pevent *pevent, enum event_sort_type);
 struct format_field **pevent_event_common_fields(struct event_format *event);
-- 
1.7.10.2

Re: [RFC PATCHSET 0/3] tools lib traceevent: Generic error handling for pevent

[Steven Rostedt]

On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:

> I don't like the name 'pevent' but since it's a part of the API
> so I used it for the name.
> 

Have a better suggestion? It's still a rather new API. We can still
change trace-cmd and power-top if we can come up with a better naming
convention.

Speak now or forever hold your piece ;-) (or is that peace?)

-- Steve

Re: [PATCH 1/3] tools lib traceevent: Do not link broken field arg for an old ftrace event

[Steven Rostedt]

On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Defer linking a newly allocated arg to print_fmt.args until
> all of its field is setup so that later access to ->field.name
> cannot be NULL.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve


> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/event-parse.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c

Re: [PATCH 2/3] tools lib traceevent: Introduce pevent_errno

[Steven Rostedt]

On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Define and use error numbers for pevent_parse_event()
> and get rid of die() and do_warning() calls. If the
> function returns non-zero value, the caller can check
> the return code and do appropriate things.
> 
> I chose the error numbers to be negative not to clash
> with standard errno, and as usual, 0 for success.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

- Steve

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/event-parse.c |   50 +++++++++++++++++++++---------------
>  tools/lib/traceevent/event-parse.h |   26 +++++++++++++++++--
>  2 files changed, 54 insertions(+), 22 deletions(-)
> 

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> The pevent_strerror() sets @buf to a string that describes the
> (libtraceevent-specific) error condition that is passed via @errnum.
> 
> This is similar to strerror_r() and does same thing if @errnum has a
> standard errno value.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/event-parse.c |   48 ++++++++++++++++++++++++++++++++++++
>  tools/lib/traceevent/event-parse.h |    2 ++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index 866bdca4825d..3ed2d9362e69 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -4706,6 +4706,54 @@ enum pevent_errno pevent_parse_event(struct pevent *pevent, const char *buf,
>  	return ret;
>  }
>  
> +/*
> + * This must have a same ordering as the enum pevent_errno.
> + */
> +static const char * const pevent_error_str[] = {
> +	"failed to allocate memory",
> +	"failed to parse event",
> +	"failed to read event id",
> +	"failed to read event format",
> +	"failed to read event print fmt",
> +	"failed to allocate field name for ftrace",
> +};
> +
> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
> +		    char *buf, size_t buflen)

Hmm, actually I wonder if we should put the error into the pevent
structure. Then we wouldn't even need to waste time to pass the data
through.

That is, you can simply do:

	ret = pevent_foo();
	if (ret < 0) {
		pevent_strerr(pevent, buf, buflen);
		printf("%s\n", buf);
	}

Perhaps even include a pevent_perror(), to just do:

	if (ret < 0) {
		pevent_perror(pevent);
		return ret;
	}

-- Steve

> +{
> +	int idx;
> +	const char *msg;
> +
> +	if (errnum >= 0) {
> +		strerror_r(errnum, buf, buflen);
> +		return 0;
> +	}
> +
> +	if (errnum <  __PEVENT_ERRNO__START ||
> +	    errnum >= __PEVENT_ERRNO__END)
> +		return -1;
> +
> +	idx = errnum - __PEVENT_ERRNO__START;
> +	msg = pevent_error_str[idx];
> +
> +	switch (errnum) {
> +	case PEVENT_ERRNO__MEM_ALLOC_FAILED:
> +	case PEVENT_ERRNO__PARSE_EVENT_FAILED:
> +	case PEVENT_ERRNO__READ_ID_FAILED:
> +	case PEVENT_ERRNO__READ_FORMAT_FAILED:
> +	case PEVENT_ERRNO__READ_PRINT_FAILED:
> +	case PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED:
> +		snprintf(buf, buflen, "%s", msg);
> +		break;
> +
> +	default:
> +		/* cannot reach here */
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  int get_field_val(struct trace_seq *s, struct format_field *field,
>  		  const char *name, struct pevent_record *record,
>  		  unsigned long long *val, int err)
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 24b7649d530b..7e90aba20850 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -576,6 +576,8 @@ int pevent_data_pid(struct pevent *pevent, struct pevent_record *rec);
>  const char *pevent_data_comm_from_pid(struct pevent *pevent, int pid);
>  void pevent_event_info(struct trace_seq *s, struct event_format *event,
>  		       struct pevent_record *record);
> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
> +		    char *buf, size_t buflen);
>  
>  struct event_format **pevent_list_events(struct pevent *pevent, enum event_sort_type);
>  struct format_field **pevent_event_common_fields(struct event_format *event);

Re: [RFC PATCHSET 0/3] tools lib traceevent: Generic error handling for pevent

[Namhyung Kim]

Hi,

On Tue, 12 Jun 2012 13:49:46 -0400, Steven Rostedt wrote:
> On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
>
>> I don't like the name 'pevent' but since it's a part of the API
>> so I used it for the name.
>> 
>
> Have a better suggestion? It's still a rather new API. We can still
> change trace-cmd and power-top if we can come up with a better naming
> convention.
>
> Speak now or forever hold your piece ;-) (or is that peace?)
>

I don't think I have a good naming sense. :/

As the library name changed, 'tevent' might look like little bit better,
but I'm not sure it's worth changing. How about 'libte', 'lte', or
'lkte' for linux (kernel) trace event? For the last case, 'telk' (trace
event for linux kernel) might be a better name to pronounce.

Any better suggestions?

Thanks,
Namhyung

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

Hi,

On Tue, 12 Jun 2012 14:01:54 -0400, Steven Rostedt wrote:
> On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
>> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
>> +		    char *buf, size_t buflen)
>
> Hmm, actually I wonder if we should put the error into the pevent
> structure. Then we wouldn't even need to waste time to pass the data
> through.
>
> That is, you can simply do:
>
> 	ret = pevent_foo();
> 	if (ret < 0) {
> 		pevent_strerr(pevent, buf, buflen);
> 		printf("%s\n", buf);
> 	}
>
> Perhaps even include a pevent_perror(), to just do:
>
> 	if (ret < 0) {
> 		pevent_perror(pevent);
> 		return ret;
> 	}
>

I thought something like this, but worried about the thread-safety. What
about if more than one thread call pevent functions for a same pevent
concurrently?  Should we make the pevent->errno TLS?

Thanks,
Namhyung

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Wed, 2012-06-13 at 12:02 +0900, Namhyung Kim wrote:
> Hi,
> 
> On Tue, 12 Jun 2012 14:01:54 -0400, Steven Rostedt wrote:
> > On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
> >> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
> >> +		    char *buf, size_t buflen)
> >
> > Hmm, actually I wonder if we should put the error into the pevent
> > structure. Then we wouldn't even need to waste time to pass the data
> > through.
> >
> > That is, you can simply do:
> >
> > 	ret = pevent_foo();
> > 	if (ret < 0) {
> > 		pevent_strerr(pevent, buf, buflen);
> > 		printf("%s\n", buf);
> > 	}
> >
> > Perhaps even include a pevent_perror(), to just do:
> >
> > 	if (ret < 0) {
> > 		pevent_perror(pevent);
> > 		return ret;
> > 	}
> >
> 
> I thought something like this, but worried about the thread-safety. What
> about if more than one thread call pevent functions for a same pevent
> concurrently?  Should we make the pevent->errno TLS?

I hate threads :-)

Anyway, there's a couple of things that can be done:

1) have two functions. A simple 'pevent_stderr()' that does the above,
and perhaps a pevent_get_error() that can be passed the return value of
a previous command, and give you the error string for it. This is thread
safe for programs that require it. The ret is always returned.

2) Make it TLS, although honestly I've never made dynamic variables
that, and didn't even realize that you could.

I'm liking the explicit 'make this thread safe' method (#1). As it may
be for a gui, a separate thread may be used to print out the error
messages, and making it thread unique will make that difficult.

If you need the code to be thread safe, have all errors do:

	ret = pevent_foo();
	if (ret < 0) {
		pevent_strerr_val(ret, buf, buflen);


For programs that do not need to be thread safe, then:

	ret = pevent_foo();
	if (ret < 0) {
		pevent_strerr(pevent, buf, buflen);


-- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

Hi, Steve

On Thu, 14 Jun 2012 23:27:05 -0400, Steven Rostedt wrote:
> On Wed, 2012-06-13 at 12:02 +0900, Namhyung Kim wrote:
>> On Tue, 12 Jun 2012 14:01:54 -0400, Steven Rostedt wrote:
>> > On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
>> >> +int pevent_strerror(struct pevent *pevent, enum pevent_errno errnum,
>> >> +		    char *buf, size_t buflen)
>> >
>> > Hmm, actually I wonder if we should put the error into the pevent
>> > structure. Then we wouldn't even need to waste time to pass the data
>> > through.
>> >
>> > That is, you can simply do:
>> >
>> > 	ret = pevent_foo();
>> > 	if (ret < 0) {
>> > 		pevent_strerr(pevent, buf, buflen);
>> > 		printf("%s\n", buf);
>> > 	}
>> >
>> > Perhaps even include a pevent_perror(), to just do:
>> >
>> > 	if (ret < 0) {
>> > 		pevent_perror(pevent);
>> > 		return ret;
>> > 	}
>> >
>> 
>> I thought something like this, but worried about the thread-safety. What
>> about if more than one thread call pevent functions for a same pevent
>> concurrently?  Should we make the pevent->errno TLS?
>
> I hate threads :-)
>
> Anyway, there's a couple of things that can be done:
>
> 1) have two functions. A simple 'pevent_stderr()' that does the above,
> and perhaps a pevent_get_error() that can be passed the return value of
> a previous command, and give you the error string for it. This is thread
> safe for programs that require it. The ret is always returned.
>

I'm not sure I understood you. Are you saying the pevent_get_error
should look like:

       char *pevent_get_error(int errcode, ...)

? So, what's the difference to my original pevent_strerror ?


> 2) Make it TLS, although honestly I've never made dynamic variables
> that, and didn't even realize that you could.
>

Right, it should be TSD (thread-specific data) for dynamic ones,
I guess.


> I'm liking the explicit 'make this thread safe' method (#1). As it may
> be for a gui, a separate thread may be used to print out the error
> messages, and making it thread unique will make that difficult.
>

Yeah, I like the #1 too.


> If you need the code to be thread safe, have all errors do:
>
> 	ret = pevent_foo();
> 	if (ret < 0) {
> 		pevent_strerr_val(ret, buf, buflen);
>
>
> For programs that do not need to be thread safe, then:
>
> 	ret = pevent_foo();
> 	if (ret < 0) {
> 		pevent_strerr(pevent, buf, buflen);
>

Do we really need these two? I think having a single API is just
enough, IMHO.

Thanks,
Namhyung

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Fri, 2012-06-15 at 18:04 +0900, Namhyung Kim wrote:

> 
> > If you need the code to be thread safe, have all errors do:
> >
> > 	ret = pevent_foo();
> > 	if (ret < 0) {
> > 		pevent_strerr_val(ret, buf, buflen);
> >
> >
> > For programs that do not need to be thread safe, then:
> >
> > 	ret = pevent_foo();
> > 	if (ret < 0) {
> > 		pevent_strerr(pevent, buf, buflen);
> >
> 
> Do we really need these two? I think having a single API is just
> enough, IMHO.

Hmm, maybe not. I guess I was confused about the need to pass the pevent
into the function. I think we only need the return val. Or is there
other data in the future that you envision will require needing pevent
passed in?

-- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:

> +/*
> + * This must have a same ordering as the enum pevent_errno.
> + */
> +static const char * const pevent_error_str[] = {
> +	"failed to allocate memory",
> +	"failed to parse event",
> +	"failed to read event id",
> +	"failed to read event format",
> +	"failed to read event print fmt",
> +	"failed to allocate field name for ftrace",
> +};
> +

Here's a little macro trick to keep the strings and enums always in
sync:

#define __PEVENT_ERRNO_CODES \
 _C(PEVENT_ERRNO__MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
 _C(PEVENT_ERRNO__PARSE_EVENT_FAILED,	"failed to parse event"),	\
 _C(PEVENT_ERRNO__READ_ID_FAILED,	"failed to read event id"),	\
 _C(PEVENT_ERRNO__READ_FORMAT_FAILED,	"failed to read event format"),	\
 _C(PEVENT_ERRNO__READ_PRINT_FAILED,	"failed to read event print fmt"),\
 _C(PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),

#undef _C
#define _C(a,b) b
static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };

#undef _C
#define _C(a, b) a

enum pevent_errno {
	__PEVENT_ERRNO__BEFORE_START		= -100000 - 1,
	__PEVENT_ERRNO_CODES
	__PEVENT_ERRNO__END,
};

#define __PEVENT_ERRNO__START (__PEVENT_ERRNO__BEFORE_START + 1)

Just saying ;-)

-- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

Steven Rostedt <rostedt@goodmis.org> writes:
> On Fri, 2012-06-15 at 18:04 +0900, Namhyung Kim wrote:
>> > If you need the code to be thread safe, have all errors do:
>> >
>> > 	ret = pevent_foo();
>> > 	if (ret < 0) {
>> > 		pevent_strerr_val(ret, buf, buflen);
>> >
>> >
>> > For programs that do not need to be thread safe, then:
>> >
>> > 	ret = pevent_foo();
>> > 	if (ret < 0) {
>> > 		pevent_strerr(pevent, buf, buflen);
>> >
>> 
>> Do we really need these two? I think having a single API is just
>> enough, IMHO.
>
> Hmm, maybe not. I guess I was confused about the need to pass the pevent
> into the function. I think we only need the return val. Or is there
> other data in the future that you envision will require needing pevent
> passed in?
>

I was thinking about a way to provide additional information in case of
a parse error by saving them into the pevent. But it turned out to make
the API non-thread-safe. :/ I couldn't find a good way yet.

So, for now, passing the return value only would be enough. But I think
passing pevent into a function name started by pevent_ looks natural. :)

Thanks,
Namhyung

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Sat, 2012-06-16 at 07:18 +0900, Namhyung Kim wrote:

> So, for now, passing the return value only would be enough. But I think
> passing pevent into a function name started by pevent_ looks natural. :)

Yeah, I agree, and who knows, maybe in the future it will be useful.

Perhaps, we should add a note that says pevent can be NULL. Just in case
you just want to convert a return value and you do not have an available
pevent at hand.

-- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
>
>> +/*
>> + * This must have a same ordering as the enum pevent_errno.
>> + */
>> +static const char * const pevent_error_str[] = {
>> +	"failed to allocate memory",
>> +	"failed to parse event",
>> +	"failed to read event id",
>> +	"failed to read event format",
>> +	"failed to read event print fmt",
>> +	"failed to allocate field name for ftrace",
>> +};
>> +
>
> Here's a little macro trick to keep the strings and enums always in
> sync:
>
> #define __PEVENT_ERRNO_CODES \
>  _C(PEVENT_ERRNO__MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
>  _C(PEVENT_ERRNO__PARSE_EVENT_FAILED,	"failed to parse event"),	\
>  _C(PEVENT_ERRNO__READ_ID_FAILED,	"failed to read event id"),	\
>  _C(PEVENT_ERRNO__READ_FORMAT_FAILED,	"failed to read event format"),	\
>  _C(PEVENT_ERRNO__READ_PRINT_FAILED,	"failed to read event print fmt"),\
>  _C(PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),
>
> #undef _C
> #define _C(a,b) b
> static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };
>

Cool. Maybe even shorter:

#define __PEVENT_ERRNO_CODES \
 _C(MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
 _C(PARSE_EVENT_FAILED,	"failed to parse event"),	\
 _C(READ_ID_FAILED,	"failed to read event id"),	\
 _C(READ_FORMAT_FAILED,	"failed to read event format"),	\
 _C(READ_PRINT_FAILED,	"failed to read event print fmt"), \
 _C(OLD_FTRACE_ARG_FAILED, "failed to allocate field name for ftrace"),

#undef _C
#define _C(a,b) PEVENT_ERRNO__ ## a
static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };

But it make them less grep-able?

Thanks,
Namhyung


> #undef _C
> #define _C(a, b) a
>
> enum pevent_errno {
> 	__PEVENT_ERRNO__BEFORE_START		= -100000 - 1,
> 	__PEVENT_ERRNO_CODES
> 	__PEVENT_ERRNO__END,
> };
>
> #define __PEVENT_ERRNO__START (__PEVENT_ERRNO__BEFORE_START + 1)
>
> Just saying ;-)
>
> -- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Sat, 2012-06-16 at 07:25 +0900, Namhyung Kim wrote:
> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > On Tue, 2012-06-12 at 16:42 +0900, Namhyung Kim wrote:
> >
> >> +/*
> >> + * This must have a same ordering as the enum pevent_errno.
> >> + */
> >> +static const char * const pevent_error_str[] = {
> >> +	"failed to allocate memory",
> >> +	"failed to parse event",
> >> +	"failed to read event id",
> >> +	"failed to read event format",
> >> +	"failed to read event print fmt",
> >> +	"failed to allocate field name for ftrace",
> >> +};
> >> +
> >
> > Here's a little macro trick to keep the strings and enums always in
> > sync:
> >
> > #define __PEVENT_ERRNO_CODES \
> >  _C(PEVENT_ERRNO__MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
> >  _C(PEVENT_ERRNO__PARSE_EVENT_FAILED,	"failed to parse event"),	\
> >  _C(PEVENT_ERRNO__READ_ID_FAILED,	"failed to read event id"),	\
> >  _C(PEVENT_ERRNO__READ_FORMAT_FAILED,	"failed to read event format"),	\
> >  _C(PEVENT_ERRNO__READ_PRINT_FAILED,	"failed to read event print fmt"),\
> >  _C(PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),
> >
> > #undef _C
> > #define _C(a,b) b
> > static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };
> >
> 
> Cool. Maybe even shorter:
> 
> #define __PEVENT_ERRNO_CODES \
>  _C(MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
>  _C(PARSE_EVENT_FAILED,	"failed to parse event"),	\
>  _C(READ_ID_FAILED,	"failed to read event id"),	\
>  _C(READ_FORMAT_FAILED,	"failed to read event format"),	\
>  _C(READ_PRINT_FAILED,	"failed to read event print fmt"), \
>  _C(OLD_FTRACE_ARG_FAILED, "failed to allocate field name for ftrace"),
> 
> #undef _C
> #define _C(a,b) PEVENT_ERRNO__ ## a

I think you wanted b on this one. ;-)

> static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };
> 
> But it make them less grep-able?

Yeah, that is a problem, although we could perhaps teach ctags and etags
how to find them.

-- Steve

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Namhyung Kim]

Steven Rostedt <rostedt@goodmis.org> writes:
> On Sat, 2012-06-16 at 07:25 +0900, Namhyung Kim wrote:
>> Steven Rostedt <rostedt@goodmis.org> writes:
>> > Here's a little macro trick to keep the strings and enums always in
>> > sync:
>> >
>> > #define __PEVENT_ERRNO_CODES \
>> >  _C(PEVENT_ERRNO__MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
>> >  _C(PEVENT_ERRNO__PARSE_EVENT_FAILED,	"failed to parse event"),	\
>> >  _C(PEVENT_ERRNO__READ_ID_FAILED,	"failed to read event id"),	\
>> >  _C(PEVENT_ERRNO__READ_FORMAT_FAILED,	"failed to read event format"),	\
>> >  _C(PEVENT_ERRNO__READ_PRINT_FAILED,	"failed to read event print fmt"),\
>> >  _C(PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),
>> >
>> > #undef _C
>> > #define _C(a,b) b
>> > static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };
>> >
>> 
>> Cool. Maybe even shorter:
>> 
>> #define __PEVENT_ERRNO_CODES \
>>  _C(MEM_ALLOC_FAILED,	"failed to allocate memory"),	\
>>  _C(PARSE_EVENT_FAILED,	"failed to parse event"),	\
>>  _C(READ_ID_FAILED,	"failed to read event id"),	\
>>  _C(READ_FORMAT_FAILED,	"failed to read event format"),	\
>>  _C(READ_PRINT_FAILED,	"failed to read event print fmt"), \
>>  _C(OLD_FTRACE_ARG_FAILED, "failed to allocate field name for ftrace"),
>> 
>> #undef _C
>> #define _C(a,b) PEVENT_ERRNO__ ## a
>
> I think you wanted b on this one. ;-)
>

Oops, right. :)


>> static const char * const pevent_error_str[] = { __PEVENT_ERROR_CODES };
>> 
>> But it make them less grep-able?
>
> Yeah, that is a problem, although we could perhaps teach ctags and etags
> how to find them.
>

Ok, then I'll go with yours.

Thanks,
Namhyung

Re: [PATCH 3/3] tools lib traceevent: Introduce pevent_strerror

[Steven Rostedt]

On Sat, 2012-06-16 at 07:51 +0900, Namhyung Kim wrote:

> > Yeah, that is a problem, although we could perhaps teach ctags and etags
> > how to find them.
> >
> 
> Ok, then I'll go with yours.
> 

I think we can keep yours. I know [ce]tags can be taught to handle that
as well.

Do a search for 'sys_read' in the kernel. It brings you to:

SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)

Which means that it can be taught.

-- Steve