[PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Namhyung Kim]

Hello,

This patchset tries to remove all die() calls in event filter parsing
code.  The only remaining bits are in trace-seq.c which implement
print functions and I want to hear what's the best way we can handle
the error case during the print.

I also put this patches on libtraceevent/die-removal-v1 branch in my tree

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git


Any comments are welcome, thanks
Namhyung


Namhyung Kim (14):
  tools lib traceevent: Get rid of malloc_or_die() in show_error()
  tools lib traceevent: Get rid of die in add_filter_type()
  tools lib traceevent: Get rid of malloc_or_die() in
    pevent_filter_alloc()
  tools lib traceevent: Get rid of malloc_or_die() allocate_arg()
  tools lib traceevent: Get rid of malloc_or_die() in read_token()
  tools lib traceevent: Get rid of malloc_or_die() in find_event()
  tools lib traceevent: Get rid of malloc_or_die() in add_event()
  tools lib traceevent: Get rid of die() in create_arg_item()
  tools lib traceevent: Get rid of die() in add_right()
  tools lib traceevent: Get rid of die() in reparent_op_arg()
  tools lib traceevent: Get rid of malloc_or_die() in
    pevent_filter_add_filter_str()
  tools lib traceevent: Get rid of die() in
    pevent_filter_clear_trivial()
  tools lib traceevent: Refactor test_filter() to get rid of die()
  tools lib traceevent: Get rid of die() in some string conversion
    funcitons

 tools/lib/traceevent/event-parse.h  |   3 +-
 tools/lib/traceevent/parse-filter.c | 432 +++++++++++++++++++++++++-----------
 2 files changed, 303 insertions(+), 132 deletions(-)

-- 
1.7.11.7

[PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 2500e75583fc..0fc905c230ad 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -56,7 +56,21 @@ static void show_error(char **error_str, const char *fmt, ...)
 	index = pevent_get_input_buf_ptr();
 	len = input ? strlen(input) : 0;
 
-	error = malloc_or_die(MAX_ERR_STR_SIZE + (len*2) + 3);
+	error = malloc(MAX_ERR_STR_SIZE + (len*2) + 3);
+	if (error == NULL) {
+		/*
+		 * Maybe it's due to len is too long.
+		 * Retry without the input buffer part.
+		 */
+		len = 0;
+
+		error = malloc(MAX_ERR_STR_SIZE);
+		if (error == NULL) {
+			/* no memory */
+			*error_str = "failed to allocate memory";
+			return;
+		}
+	}
 
 	if (len) {
 		strcpy(error, input);
-- 
1.7.11.7

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Arnaldo Carvalho de Melo]

Em Mon, Dec 09, 2013 at 02:33:58PM +0900, Namhyung Kim escreveu:
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/parse-filter.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 2500e75583fc..0fc905c230ad 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -56,7 +56,21 @@ static void show_error(char **error_str, const char *fmt, ...)
>  	index = pevent_get_input_buf_ptr();
>  	len = input ? strlen(input) : 0;
>  
> -	error = malloc_or_die(MAX_ERR_STR_SIZE + (len*2) + 3);
> +	error = malloc(MAX_ERR_STR_SIZE + (len*2) + 3);
> +	if (error == NULL) {
> +		/*
> +		 * Maybe it's due to len is too long.
> +		 * Retry without the input buffer part.
> +		 */
> +		len = 0;
> +
> +		error = malloc(MAX_ERR_STR_SIZE);
> +		if (error == NULL) {
> +			/* no memory */
> +			*error_str = "failed to allocate memory";
> +			return;

Can *error_str point to either malloc'ed or constant strings? Who
releases the allocated memory?

- Arnaldo

> +		}
> +	}
>  
>  	if (len) {
>  		strcpy(error, input);
> -- 
> 1.7.11.7

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Steven Rostedt]

On Mon, 9 Dec 2013 15:30:09 -0300
Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:


> > +		error = malloc(MAX_ERR_STR_SIZE);
> > +		if (error == NULL) {
> > +			/* no memory */
> > +			*error_str = "failed to allocate memory";
> > +			return;
> 
> Can *error_str point to either malloc'ed or constant strings? Who
> releases the allocated memory?
> 

Good question. Perhaps we should have a flag that states if the string
is allocated or not. Or better yet, since the only reason it would be
pointing to a static string is if the string for error_str itself
failed to allocate. Then we could use a string within pevent for it:

static char *pevent_failed_error_alloc = "failed to allocate memory";

Then in the freeing of error str:

void pevent_free_error_str(error_str)
{
	if (error_str != pevent_failed_error_alloc)
		free(error_str);
}

-- Steve

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Arnaldo Carvalho de Melo]

Em Mon, Dec 09, 2013 at 02:03:42PM -0500, Steven Rostedt escreveu:
> On Mon, 9 Dec 2013 15:30:09 -0300
> Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> 
> 
> > > +		error = malloc(MAX_ERR_STR_SIZE);
> > > +		if (error == NULL) {
> > > +			/* no memory */
> > > +			*error_str = "failed to allocate memory";
> > > +			return;
> > 
> > Can *error_str point to either malloc'ed or constant strings? Who
> > releases the allocated memory?
> > 
> 
> Good question. Perhaps we should have a flag that states if the string
> is allocated or not. Or better yet, since the only reason it would be
> pointing to a static string is if the string for error_str itself
> failed to allocate. Then we could use a string within pevent for it:
> 
> static char *pevent_failed_error_alloc = "failed to allocate memory";
> 
> Then in the freeing of error str:
> 
> void pevent_free_error_str(error_str)
> {
> 	if (error_str != pevent_failed_error_alloc)
> 		free(error_str);
> }

That is a possibility, yes, then any other routine that works in such a
way could check against this string, but what is wrong with returning a
value to that function and checking against < 0?

- Arnaldo

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Steven Rostedt]

On Mon, 9 Dec 2013 16:14:39 -0300
Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Mon, Dec 09, 2013 at 02:03:42PM -0500, Steven Rostedt escreveu:
> > On Mon, 9 Dec 2013 15:30:09 -0300
> > Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > 
> > 
> > > > +		error = malloc(MAX_ERR_STR_SIZE);
> > > > +		if (error == NULL) {
> > > > +			/* no memory */
> > > > +			*error_str = "failed to allocate memory";
> > > > +			return;
> > > 
> > > Can *error_str point to either malloc'ed or constant strings? Who
> > > releases the allocated memory?
> > > 
> > 
> > Good question. Perhaps we should have a flag that states if the string
> > is allocated or not. Or better yet, since the only reason it would be
> > pointing to a static string is if the string for error_str itself
> > failed to allocate. Then we could use a string within pevent for it:
> > 
> > static char *pevent_failed_error_alloc = "failed to allocate memory";
> > 
> > Then in the freeing of error str:
> > 
> > void pevent_free_error_str(error_str)
> > {
> > 	if (error_str != pevent_failed_error_alloc)
> > 		free(error_str);
> > }
> 
> That is a possibility, yes, then any other routine that works in such a
> way could check against this string, but what is wrong with returning a
> value to that function and checking against < 0?

Then everyone has to check if show_error() failed. Then report a bug if
it did. Egad, then we need to check if that error function failed, and
then that one and that one and that one :-)

-- Steve

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

On Mon, 9 Dec 2013 14:23:50 -0500, Steven Rostedt wrote:
> On Mon, 9 Dec 2013 16:14:39 -0300
> Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
>
>> Em Mon, Dec 09, 2013 at 02:03:42PM -0500, Steven Rostedt escreveu:
>> > On Mon, 9 Dec 2013 15:30:09 -0300
>> > Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
>> > 
>> > 
>> > > > +		error = malloc(MAX_ERR_STR_SIZE);
>> > > > +		if (error == NULL) {
>> > > > +			/* no memory */
>> > > > +			*error_str = "failed to allocate memory";
>> > > > +			return;
>> > > 
>> > > Can *error_str point to either malloc'ed or constant strings? Who
>> > > releases the allocated memory?
>> > > 
>> > 
>> > Good question. Perhaps we should have a flag that states if the string
>> > is allocated or not. Or better yet, since the only reason it would be
>> > pointing to a static string is if the string for error_str itself
>> > failed to allocate. Then we could use a string within pevent for it:
>> > 
>> > static char *pevent_failed_error_alloc = "failed to allocate memory";
>> > 
>> > Then in the freeing of error str:
>> > 
>> > void pevent_free_error_str(error_str)
>> > {
>> > 	if (error_str != pevent_failed_error_alloc)
>> > 		free(error_str);
>> > }
>> 
>> That is a possibility, yes, then any other routine that works in such a
>> way could check against this string, but what is wrong with returning a
>> value to that function and checking against < 0?
>
> Then everyone has to check if show_error() failed. Then report a bug if
> it did. Egad, then we need to check if that error function failed, and
> then that one and that one and that one :-)

What about returning error code rather than string?  This way we won't
worry about the allocation of the error string itself.

But the downside of it is loosing a positional info of the error.
Hmm.. what about using a static buffer in pevent for it then?

Thanks,
Namhyung

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Steven Rostedt]

On Tue, 10 Dec 2013 11:03:44 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> What about returning error code rather than string?  This way we won't
> worry about the allocation of the error string itself.
> 
> But the downside of it is loosing a positional info of the error.
> Hmm.. what about using a static buffer in pevent for it then?

A static buffer may be the solution. Never need to worry about
allocating it on error, as it will already be allocated. And we can add
APIs to print it nicely.

Perhaps call it

	pevent->filter_error_buffer

?

-- Steve

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

On Mon, 9 Dec 2013 21:14:10 -0500, Steven Rostedt wrote:
> On Tue, 10 Dec 2013 11:03:44 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> What about returning error code rather than string?  This way we won't
>> worry about the allocation of the error string itself.
>> 
>> But the downside of it is loosing a positional info of the error.
>> Hmm.. what about using a static buffer in pevent for it then?
>
> A static buffer may be the solution. Never need to worry about
> allocating it on error, as it will already be allocated. And we can add
> APIs to print it nicely.
>
> Perhaps call it
>
> 	pevent->filter_error_buffer
>
> ?

Okay, I'll go with this.

Thanks,
Namhyung

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

On Tue, 10 Dec 2013 14:01:44 +0900, Namhyung Kim wrote:
> On Mon, 9 Dec 2013 21:14:10 -0500, Steven Rostedt wrote:
>> On Tue, 10 Dec 2013 11:03:44 +0900
>> Namhyung Kim <namhyung@kernel.org> wrote:
>>
>>> What about returning error code rather than string?  This way we won't
>>> worry about the allocation of the error string itself.
>>> 
>>> But the downside of it is loosing a positional info of the error.
>>> Hmm.. what about using a static buffer in pevent for it then?
>>
>> A static buffer may be the solution. Never need to worry about
>> allocating it on error, as it will already be allocated. And we can add
>> APIs to print it nicely.
>>
>> Perhaps call it
>>
>> 	pevent->filter_error_buffer
>>
>> ?

Hmm.. thinking about it twice, if it's only for filter functions
wouldn't it be better moving it to event_filter rather than pevent?

	filter->error_buffer

Thanks,
Namhyung

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

On Tue, 10 Dec 2013 14:30:18 +0900, Namhyung Kim wrote:
> On Tue, 10 Dec 2013 14:01:44 +0900, Namhyung Kim wrote:
>> On Mon, 9 Dec 2013 21:14:10 -0500, Steven Rostedt wrote:
>>> On Tue, 10 Dec 2013 11:03:44 +0900
>>> Namhyung Kim <namhyung@kernel.org> wrote:
>>>
>>>> What about returning error code rather than string?  This way we won't
>>>> worry about the allocation of the error string itself.
>>>> 
>>>> But the downside of it is loosing a positional info of the error.
>>>> Hmm.. what about using a static buffer in pevent for it then?
>>>
>>> A static buffer may be the solution. Never need to worry about
>>> allocating it on error, as it will already be allocated. And we can add
>>> APIs to print it nicely.
>>>
>>> Perhaps call it
>>>
>>> 	pevent->filter_error_buffer
>>>
>>> ?
>
> Hmm.. thinking about it twice, if it's only for filter functions
> wouldn't it be better moving it to event_filter rather than pevent?
>
> 	filter->error_buffer
>

One more thinking, if we agree on converting to return error code, does
pevent_filter_add_filter_str() also need to be changed not to receive
the third "error_str" argument?

And should we extend the error code to include the return value of
pevent_filter_match() too?  If not, it seems we need to pass another
argument to receive the actual error code in case of FILTER_ERROR.

I'm saying these here since they might require interface/signature
change so will affect existing users like trace-cmd.

Thanks,
Namhyung

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Steven Rostedt]

On Wed, 11 Dec 2013 09:40:35 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> >>> Perhaps call it
> >>>
> >>> 	pevent->filter_error_buffer
> >>>
> >>> ?
> >
> > Hmm.. thinking about it twice, if it's only for filter functions
> > wouldn't it be better moving it to event_filter rather than pevent?
> >
> > 	filter->error_buffer

Agreed.

> >
> 
> One more thinking, if we agree on converting to return error code, does
> pevent_filter_add_filter_str() also need to be changed not to receive
> the third "error_str" argument?

Yes, if we add the filter->error_buffer, we can remove it here.

> 
> And should we extend the error code to include the return value of
> pevent_filter_match() too?  If not, it seems we need to pass another
> argument to receive the actual error code in case of FILTER_ERROR.

I'm a bit confused on this. Perhaps it's something you added in your
patches. If what returns FILTER_ERROR?

> 
> I'm saying these here since they might require interface/signature
> change so will affect existing users like trace-cmd.

I'm OK if they change now. I'll have trace-cmd and other users adapt.
As each currently has their own copy. I've been updating trace-cmd with
what's in tools for a while now, and plan to continue doing that until
we have something that seems good for a public library.

-- Steve

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Namhyung Kim]

On Tue, 10 Dec 2013 20:55:26 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2013 09:40:35 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>> And should we extend the error code to include the return value of
>> pevent_filter_match() too?  If not, it seems we need to pass another
>> argument to receive the actual error code in case of FILTER_ERROR.
>
> I'm a bit confused on this. Perhaps it's something you added in your
> patches. If what returns FILTER_ERROR?

Well, I mean there are some cases which return FILTER_ERROR.  With my
patch, test_filter() can failed with the error_str set like following:

 - invalid expression type
 - must have number field
 - invalid numeric argument type
 - invalid numeric comparison type
 - invalid string comparison type
 - invalid operator type
 - invalid argument type

To distinguish them, we either need to extend return value or another
argument.  But the current return value of pevent_filter_match() was
defined as FILTER_{MATCH,MISS,NOEXIST,NONE,ERROR}.

And also I want all user APIs share same return value/type as
pevent_errno so that user can pass it our strerror function to see the
error message.

So to use return value, we need to extend the error code to include all
possible error cases above as well as normal cases (MATCH, MISS, ...).

>
>> 
>> I'm saying these here since they might require interface/signature
>> change so will affect existing users like trace-cmd.
>
> I'm OK if they change now. I'll have trace-cmd and other users adapt.
> As each currently has their own copy. I've been updating trace-cmd with
> what's in tools for a while now, and plan to continue doing that until
> we have something that seems good for a public library.

Okay, I'll cook the patch soon!

Thanks,
Namhyung

Re: [PATCH 01/14] tools lib traceevent: Get rid of malloc_or_die() in show_error()

[Steven Rostedt]

On Wed, 11 Dec 2013 11:29:21 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Tue, 10 Dec 2013 20:55:26 -0500, Steven Rostedt wrote:
> > On Wed, 11 Dec 2013 09:40:35 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> >> And should we extend the error code to include the return value of
> >> pevent_filter_match() too?  If not, it seems we need to pass another
> >> argument to receive the actual error code in case of FILTER_ERROR.
> >
> > I'm a bit confused on this. Perhaps it's something you added in your
> > patches. If what returns FILTER_ERROR?
> 
> Well, I mean there are some cases which return FILTER_ERROR.  With my
> patch, test_filter() can failed with the error_str set like following:
> 
>  - invalid expression type
>  - must have number field
>  - invalid numeric argument type
>  - invalid numeric comparison type
>  - invalid string comparison type
>  - invalid operator type
>  - invalid argument type
> 
> To distinguish them, we either need to extend return value or another
> argument.  But the current return value of pevent_filter_match() was
> defined as FILTER_{MATCH,MISS,NOEXIST,NONE,ERROR}.
> 
> And also I want all user APIs share same return value/type as
> pevent_errno so that user can pass it our strerror function to see the
> error message.
> 
> So to use return value, we need to extend the error code to include all
> possible error cases above as well as normal cases (MATCH, MISS, ...).

Sure, lets add them to the list of pevent errnos.


> 
> >
> >> 
> >> I'm saying these here since they might require interface/signature
> >> change so will affect existing users like trace-cmd.
> >
> > I'm OK if they change now. I'll have trace-cmd and other users adapt.
> > As each currently has their own copy. I've been updating trace-cmd with
> > what's in tools for a while now, and plan to continue doing that until
> > we have something that seems good for a public library.
> 
> Okay, I'll cook the patch soon!
> 

My only concern with the libtraceevent API is that it still maintains
all the features that it currently has.

-- Steve

[PATCH 02/14] tools lib traceevent: Get rid of die in add_filter_type()

[Namhyung Kim]

The realloc() should check return value and not to overwrite previous
pointer in case of error.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 0fc905c230ad..d9c239933992 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -161,11 +161,13 @@ add_filter_type(struct event_filter *filter, int id)
 	if (filter_type)
 		return filter_type;
 
-	filter->event_filters =	realloc(filter->event_filters,
-					sizeof(*filter->event_filters) *
-					(filter->filters + 1));
-	if (!filter->event_filters)
-		die("Could not allocate filter");
+	filter_type = realloc(filter->event_filters,
+			      sizeof(*filter->event_filters) *
+			      (filter->filters + 1));
+	if (!filter_type)
+		return NULL;
+
+	filter->event_filters = filter_type;
 
 	for (i = 0; i < filter->filters; i++) {
 		if (filter->event_filters[i].event_id > id)
@@ -1164,6 +1166,12 @@ static int filter_event(struct event_filter *filter,
 	}
 
 	filter_type = add_filter_type(filter, event->id);
+	if (filter_type == NULL) {
+		show_error(error_str, "failed to add a new filter: %s",
+			   filter_str ? filter_str : "true");
+		return -1;
+	}
+
 	if (filter_type->filter)
 		free_arg(filter_type->filter);
 	filter_type->filter = arg;
@@ -1395,6 +1403,9 @@ static int copy_filter_type(struct event_filter *filter,
 			arg->boolean.value = 0;
 
 		filter_type = add_filter_type(filter, event->id);
+		if (filter_type == NULL)
+			return -1;
+
 		filter_type->filter = arg;
 
 		free(str);
-- 
1.7.11.7

Re: [PATCH 02/14] tools lib traceevent: Get rid of die in add_filter_type()

[Jiri Olsa]

On Mon, Dec 09, 2013 at 02:33:59PM +0900, Namhyung Kim wrote:
> The realloc() should check return value and not to overwrite previous
> pointer in case of error.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/parse-filter.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 0fc905c230ad..d9c239933992 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -161,11 +161,13 @@ add_filter_type(struct event_filter *filter, int id)
>  	if (filter_type)
>  		return filter_type;
>  
> -	filter->event_filters =	realloc(filter->event_filters,
> -					sizeof(*filter->event_filters) *
> -					(filter->filters + 1));
> -	if (!filter->event_filters)
> -		die("Could not allocate filter");
> +	filter_type = realloc(filter->event_filters,
> +			      sizeof(*filter->event_filters) *
> +			      (filter->filters + 1));
> +	if (!filter_type)
> +		return NULL;
> +
> +	filter->event_filters = filter_type;
>  
>  	for (i = 0; i < filter->filters; i++) {
>  		if (filter->event_filters[i].event_id > id)
> @@ -1164,6 +1166,12 @@ static int filter_event(struct event_filter *filter,
>  	}
>  
>  	filter_type = add_filter_type(filter, event->id);
> +	if (filter_type == NULL) {
> +		show_error(error_str, "failed to add a new filter: %s",
> +			   filter_str ? filter_str : "true");
> +		return -1;

so your key for using show_error in case of error is if it's
used already in the error path in the function.. right?

jirka

> +	}
> +
>  	if (filter_type->filter)
>  		free_arg(filter_type->filter);
>  	filter_type->filter = arg;
> @@ -1395,6 +1403,9 @@ static int copy_filter_type(struct event_filter *filter,
>  			arg->boolean.value = 0;
>  
>  		filter_type = add_filter_type(filter, event->id);
> +		if (filter_type == NULL)
> +			return -1;
> +
>  		filter_type->filter = arg;
>  
>  		free(str);
> -- 
> 1.7.11.7
> 

Re: [PATCH 02/14] tools lib traceevent: Get rid of die in add_filter_type()

[Namhyung Kim]

On Mon, 9 Dec 2013 11:44:37 +0100, Jiri Olsa wrote:
> On Mon, Dec 09, 2013 at 02:33:59PM +0900, Namhyung Kim wrote:
>> The realloc() should check return value and not to overwrite previous
>> pointer in case of error.
>> 
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/lib/traceevent/parse-filter.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>> 
>> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
>> index 0fc905c230ad..d9c239933992 100644
>> --- a/tools/lib/traceevent/parse-filter.c
>> +++ b/tools/lib/traceevent/parse-filter.c
>> @@ -161,11 +161,13 @@ add_filter_type(struct event_filter *filter, int id)
>>  	if (filter_type)
>>  		return filter_type;
>>  
>> -	filter->event_filters =	realloc(filter->event_filters,
>> -					sizeof(*filter->event_filters) *
>> -					(filter->filters + 1));
>> -	if (!filter->event_filters)
>> -		die("Could not allocate filter");
>> +	filter_type = realloc(filter->event_filters,
>> +			      sizeof(*filter->event_filters) *
>> +			      (filter->filters + 1));
>> +	if (!filter_type)
>> +		return NULL;
>> +
>> +	filter->event_filters = filter_type;
>>  
>>  	for (i = 0; i < filter->filters; i++) {
>>  		if (filter->event_filters[i].event_id > id)
>> @@ -1164,6 +1166,12 @@ static int filter_event(struct event_filter *filter,
>>  	}
>>  
>>  	filter_type = add_filter_type(filter, event->id);
>> +	if (filter_type == NULL) {
>> +		show_error(error_str, "failed to add a new filter: %s",
>> +			   filter_str ? filter_str : "true");
>> +		return -1;
>
> so your key for using show_error in case of error is if it's
> used already in the error path in the function.. right?

Right.  In fact, it might look better if copy_filter_type() or
pevent_filter_copy() pass error_str too.  But the pevent_filter_
copy() looks like an user API so I was not sure I can change the
signature.  Also user can see its return value anyway and it's
relatively simple function so might not care about the detail..

Thanks,
Namhyung

>
>> +	}
>> +
>>  	if (filter_type->filter)
>>  		free_arg(filter_type->filter);
>>  	filter_type->filter = arg;
>> @@ -1395,6 +1403,9 @@ static int copy_filter_type(struct event_filter *filter,
>>  			arg->boolean.value = 0;
>>  
>>  		filter_type = add_filter_type(filter, event->id);
>> +		if (filter_type == NULL)
>> +			return -1;
>> +
>>  		filter_type->filter = arg;
>>  
>>  		free(str);
>> -- 
>> 1.7.11.7
>> 

[PATCH 03/14] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()

[Namhyung Kim]

It returns NULL when allocation fails so the users should check the
return value from now on.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index d9c239933992..21d13a4f9a5f 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -198,7 +198,10 @@ struct event_filter *pevent_filter_alloc(struct pevent *pevent)
 {
 	struct event_filter *filter;
 
-	filter = malloc_or_die(sizeof(*filter));
+	filter = malloc(sizeof(*filter));
+	if (filter == NULL)
+		return NULL;
+
 	memset(filter, 0, sizeof(*filter));
 	filter->pevent = pevent;
 	pevent_ref(pevent);
-- 
1.7.11.7

[tip:perf/core] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()

[tip-bot for Namhyung Kim]

Commit-ID:  4f24416331e9a507e953e90d4534e9a9802cbc12
Gitweb:     http://git.kernel.org/tip/4f24416331e9a507e953e90d4534e9a9802cbc12
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:00 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:39:35 -0300

tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()

It returns NULL when allocation fails so the users should check the
return value from now on.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 2500e75..b3a61d4 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -182,7 +182,10 @@ struct event_filter *pevent_filter_alloc(struct pevent *pevent)
 {
 	struct event_filter *filter;
 
-	filter = malloc_or_die(sizeof(*filter));
+	filter = malloc(sizeof(*filter));
+	if (filter == NULL)
+		return NULL;
+
 	memset(filter, 0, sizeof(*filter));
 	filter->pevent = pevent;
 	pevent_ref(pevent);

[PATCH 04/14] tools lib traceevent: Get rid of malloc_or_die() allocate_arg()

[Namhyung Kim]

Also check return value and handle it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 59 +++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 21d13a4f9a5f..35fac1fa376b 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -211,12 +211,7 @@ struct event_filter *pevent_filter_alloc(struct pevent *pevent)
 
 static struct filter_arg *allocate_arg(void)
 {
-	struct filter_arg *arg;
-
-	arg = malloc_or_die(sizeof(*arg));
-	memset(arg, 0, sizeof(*arg));
-
-	return arg;
+	return calloc(1, sizeof(struct filter_arg));
 }
 
 static void free_arg(struct filter_arg *arg)
@@ -359,6 +354,10 @@ create_arg_item(struct event_format *event, const char *token,
 	struct filter_arg *arg;
 
 	arg = allocate_arg();
+	if (arg == NULL) {
+		show_error(error_str, "failed to allocate filter arg");
+		return NULL;
+	}
 
 	switch (type) {
 
@@ -409,8 +408,10 @@ create_arg_op(enum filter_op_type btype)
 	struct filter_arg *arg;
 
 	arg = allocate_arg();
-	arg->type = FILTER_ARG_OP;
-	arg->op.type = btype;
+	if (arg) {
+		arg->type = FILTER_ARG_OP;
+		arg->op.type = btype;
+	}
 
 	return arg;
 }
@@ -421,8 +422,10 @@ create_arg_exp(enum filter_exp_type etype)
 	struct filter_arg *arg;
 
 	arg = allocate_arg();
-	arg->type = FILTER_ARG_EXP;
-	arg->op.type = etype;
+	if (arg) {
+		arg->type = FILTER_ARG_EXP;
+		arg->op.type = etype;
+	}
 
 	return arg;
 }
@@ -433,9 +436,11 @@ create_arg_cmp(enum filter_exp_type etype)
 	struct filter_arg *arg;
 
 	arg = allocate_arg();
-	/* Use NUM and change if necessary */
-	arg->type = FILTER_ARG_NUM;
-	arg->op.type = etype;
+	if (arg) {
+		/* Use NUM and change if necessary */
+		arg->type = FILTER_ARG_NUM;
+		arg->op.type = etype;
+	}
 
 	return arg;
 }
@@ -896,8 +901,10 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg)
 	case FILTER_VAL_FALSE:
 		free_arg(arg);
 		arg = allocate_arg();
-		arg->type = FILTER_ARG_BOOLEAN;
-		arg->boolean.value = ret == FILTER_VAL_TRUE;
+		if (arg) {
+			arg->type = FILTER_ARG_BOOLEAN;
+			arg->boolean.value = ret == FILTER_VAL_TRUE;
+		}
 	}
 
 	return arg;
@@ -1044,6 +1051,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 			switch (op_type) {
 			case OP_BOOL:
 				arg = create_arg_op(btype);
+				if (arg == NULL)
+					goto fail_alloc;
 				if (current_op)
 					ret = add_left(arg, current_op);
 				else
@@ -1054,6 +1063,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 
 			case OP_NOT:
 				arg = create_arg_op(btype);
+				if (arg == NULL)
+					goto fail_alloc;
 				if (current_op)
 					ret = add_right(current_op, arg, error_str);
 				if (ret < 0)
@@ -1073,6 +1084,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 					arg = create_arg_exp(etype);
 				else
 					arg = create_arg_cmp(ctype);
+				if (arg == NULL)
+					goto fail_alloc;
 
 				if (current_op)
 					ret = add_right(current_op, arg, error_str);
@@ -1106,11 +1119,16 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 		current_op = current_exp;
 
 	current_op = collapse_tree(current_op);
+	if (current_op == NULL)
+		goto fail_alloc;
 
 	*parg = current_op;
 
 	return 0;
 
+ fail_alloc:
+	show_error(error_str, "failed to allocate filter arg");
+	goto fail;
  fail_print:
 	show_error(error_str, "Syntax error");
  fail:
@@ -1141,6 +1159,10 @@ process_event(struct event_format *event, const char *filter_str,
 	/* If parg is NULL, then make it into FALSE */
 	if (!*parg) {
 		*parg = allocate_arg();
+		if (*parg == NULL) {
+			show_error(error_str, "failed to allocate filter arg");
+			return -1;
+		}
 		(*parg)->type = FILTER_ARG_BOOLEAN;
 		(*parg)->boolean.value = FILTER_FALSE;
 	}
@@ -1164,6 +1186,10 @@ static int filter_event(struct event_filter *filter,
 	} else {
 		/* just add a TRUE arg */
 		arg = allocate_arg();
+		if (arg == NULL) {
+			show_error(error_str, "failed to allocate filter arg");
+			return -1;
+		}
 		arg->type = FILTER_ARG_BOOLEAN;
 		arg->boolean.value = FILTER_TRUE;
 	}
@@ -1399,6 +1425,9 @@ static int copy_filter_type(struct event_filter *filter,
 	if (strcmp(str, "TRUE") == 0 || strcmp(str, "FALSE") == 0) {
 		/* Add trivial event */
 		arg = allocate_arg();
+		if (arg == NULL)
+			return -1;
+
 		arg->type = FILTER_ARG_BOOLEAN;
 		if (strcmp(str, "TRUE") == 0)
 			arg->boolean.value = 1;
-- 
1.7.11.7

Re: [PATCH 04/14] tools lib traceevent: Get rid of malloc_or_die() allocate_arg()

[Steven Rostedt]

On Mon,  9 Dec 2013 14:34:01 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> @@ -409,8 +408,10 @@ create_arg_op(enum filter_op_type btype)
>  	struct filter_arg *arg;
>  
>  	arg = allocate_arg();
> -	arg->type = FILTER_ARG_OP;
> -	arg->op.type = btype;
> +	if (arg) {
> +		arg->type = FILTER_ARG_OP;
> +		arg->op.type = btype;
> +	}
>  
>  	return arg;
>  }
> @@ -421,8 +422,10 @@ create_arg_exp(enum filter_exp_type etype)
>  	struct filter_arg *arg;
>  
>  	arg = allocate_arg();
> -	arg->type = FILTER_ARG_EXP;
> -	arg->op.type = etype;
> +	if (arg) {
> +		arg->type = FILTER_ARG_EXP;
> +		arg->op.type = etype;
> +	}
>  
>  	return arg;
>  }
> @@ -433,9 +436,11 @@ create_arg_cmp(enum filter_exp_type etype)
>  	struct filter_arg *arg;
>  
>  	arg = allocate_arg();
> -	/* Use NUM and change if necessary */
> -	arg->type = FILTER_ARG_NUM;
> -	arg->op.type = etype;
> +	if (arg) {
> +		/* Use NUM and change if necessary */
> +		arg->type = FILTER_ARG_NUM;
> +		arg->op.type = etype;
> +	}
>  
>  	return arg;
>  }
> @@ -896,8 +901,10 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg)
>  	case FILTER_VAL_FALSE:
>  		free_arg(arg);
>  		arg = allocate_arg();
> -		arg->type = FILTER_ARG_BOOLEAN;
> -		arg->boolean.value = ret == FILTER_VAL_TRUE;
> +		if (arg) {
> +			arg->type = FILTER_ARG_BOOLEAN;
> +			arg->boolean.value = ret == FILTER_VAL_TRUE;
> +		}

Just a nit, but I wonder if all these would look nicer if we just did:

	arg = allocate_arg();
	if (!arg)
		return NULL;
	[...]

Instead of doing the work within an if statement.

Also, I prefer if (!arg) over if (arg == NULL), but I'm not going to
fight over that ;-)

-- Steve

>  	}
>  
>  	return arg;
> @@ -1044,6 +1051,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
>  			switch (op_type) {
>  			case OP_BOOL:
>  				arg = create_arg_op(btype);
> +				if (arg == NULL)
> +					goto fail_alloc;
>  				if (current_op)
>  					ret = add_left(arg, current_op);
>  				else
> @@ -1054,6 +1063,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
>  
>  			case OP_NOT:
>  				arg = create_arg_op(btype);
> +				if (arg == NULL)
> +					goto fail_alloc;
>  				if (current_op)
>  					ret = add_right(current_op, arg, error_str);
>  				if (ret < 0)
> @@ -1073,6 +1084,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
>  					arg = create_arg_exp(etype);
>  				else
>  					arg = create_arg_cmp(ctype);
> +				if (arg == NULL)
> +					goto fail_alloc;
>  
>  				if (current_op)
>  					ret = add_right(current_op, arg, error_str);
> @@ -1106,11 +1119,16 @@ process_filter(struct event_format *event, struct filter_arg **parg,
>  		current_op = current_exp;
>  
>  	current_op = collapse_tree(current_op);
> +	if (current_op == NULL)
> +		goto fail_alloc;
>  
>  	*parg = current_op;
>  
>  	return 0;
>  
> + fail_alloc:
> +	show_error(error_str, "failed to allocate filter arg");
> +	goto fail;
>   fail_print:
>  	show_error(error_str, "Syntax error");
>   fail:
> @@ -1141,6 +1159,10 @@ process_event(struct event_format *event, const char *filter_str,
>  	/* If parg is NULL, then make it into FALSE */
>  	if (!*parg) {
>  		*parg = allocate_arg();
> +		if (*parg == NULL) {
> +			show_error(error_str, "failed to allocate filter arg");
> +			return -1;
> +		}
>  		(*parg)->type = FILTER_ARG_BOOLEAN;
>  		(*parg)->boolean.value = FILTER_FALSE;
>  	}
> @@ -1164,6 +1186,10 @@ static int filter_event(struct event_filter *filter,
>  	} else {
>  		/* just add a TRUE arg */
>  		arg = allocate_arg();
> +		if (arg == NULL) {
> +			show_error(error_str, "failed to allocate filter arg");
> +			return -1;
> +		}
>  		arg->type = FILTER_ARG_BOOLEAN;
>  		arg->boolean.value = FILTER_TRUE;
>  	}
> @@ -1399,6 +1425,9 @@ static int copy_filter_type(struct event_filter *filter,
>  	if (strcmp(str, "TRUE") == 0 || strcmp(str, "FALSE") == 0) {
>  		/* Add trivial event */
>  		arg = allocate_arg();
> +		if (arg == NULL)
> +			return -1;
> +
>  		arg->type = FILTER_ARG_BOOLEAN;
>  		if (strcmp(str, "TRUE") == 0)
>  			arg->boolean.value = 1;

Re: [PATCH 04/14] tools lib traceevent: Get rid of malloc_or_die() allocate_arg()

[Namhyung Kim]

On Mon, 9 Dec 2013 11:05:19 -0500, Steven Rostedt wrote:
> On Mon,  9 Dec 2013 14:34:01 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> @@ -409,8 +408,10 @@ create_arg_op(enum filter_op_type btype)
>>  	struct filter_arg *arg;
>>  
>>  	arg = allocate_arg();
>> -	arg->type = FILTER_ARG_OP;
>> -	arg->op.type = btype;
>> +	if (arg) {
>> +		arg->type = FILTER_ARG_OP;
>> +		arg->op.type = btype;
>> +	}
>>  
>>  	return arg;
>>  }
>> @@ -421,8 +422,10 @@ create_arg_exp(enum filter_exp_type etype)
>>  	struct filter_arg *arg;
>>  
>>  	arg = allocate_arg();
>> -	arg->type = FILTER_ARG_EXP;
>> -	arg->op.type = etype;
>> +	if (arg) {
>> +		arg->type = FILTER_ARG_EXP;
>> +		arg->op.type = etype;
>> +	}
>>  
>>  	return arg;
>>  }
>> @@ -433,9 +436,11 @@ create_arg_cmp(enum filter_exp_type etype)
>>  	struct filter_arg *arg;
>>  
>>  	arg = allocate_arg();
>> -	/* Use NUM and change if necessary */
>> -	arg->type = FILTER_ARG_NUM;
>> -	arg->op.type = etype;
>> +	if (arg) {
>> +		/* Use NUM and change if necessary */
>> +		arg->type = FILTER_ARG_NUM;
>> +		arg->op.type = etype;
>> +	}
>>  
>>  	return arg;
>>  }
>> @@ -896,8 +901,10 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg)
>>  	case FILTER_VAL_FALSE:
>>  		free_arg(arg);
>>  		arg = allocate_arg();
>> -		arg->type = FILTER_ARG_BOOLEAN;
>> -		arg->boolean.value = ret == FILTER_VAL_TRUE;
>> +		if (arg) {
>> +			arg->type = FILTER_ARG_BOOLEAN;
>> +			arg->boolean.value = ret == FILTER_VAL_TRUE;
>> +		}
>
> Just a nit, but I wonder if all these would look nicer if we just did:
>
> 	arg = allocate_arg();
> 	if (!arg)
> 		return NULL;
> 	[...]
>
> Instead of doing the work within an if statement.

Sure, no problem.

>
> Also, I prefer if (!arg) over if (arg == NULL), but I'm not going to
> fight over that ;-)

Yeah, it's about preference.  I can do it your way from now on if you
want while as you can see it's more error-prone - but no, I didn't do it
intentionally because of that. ;-)

Thanks,
Namhyung

Re: [PATCH 04/14] tools lib traceevent: Get rid of malloc_or_die() allocate_arg()

[Steven Rostedt]

On Tue, 10 Dec 2013 10:21:48 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

 
> >
> > Also, I prefer if (!arg) over if (arg == NULL), but I'm not going to
> > fight over that ;-)
> 
> Yeah, it's about preference.  I can do it your way from now on if you
> want while as you can see it's more error-prone - but no, I didn't do it
> intentionally because of that. ;-)

I'm not really that picky. When I code, I use the (!arg) version. I
haven't had an inverse bug due to it in a long time. But I've fixed
others that have done it, so I understand the rational for it.

What I refuse to accept is the "if (NULL == arg)" notation. That just
makes it hard to read, and compilers are smart enough today to detect
the "if (arg = NULL)" bugs.

-- Steve

[PATCH 05/14] tools lib traceevent: Get rid of malloc_or_die() in read_token()

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 35fac1fa376b..e9d17bfcdffd 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -109,7 +109,11 @@ static enum event_type read_token(char **tok)
 	    (strcmp(token, "=") == 0 || strcmp(token, "!") == 0) &&
 	    pevent_peek_char() == '~') {
 		/* append it */
-		*tok = malloc_or_die(3);
+		*tok = malloc(3);
+		if (*tok == NULL) {
+			free_token(token);
+			return EVENT_ERROR;
+		}
 		sprintf(*tok, "%c%c", *token, '~');
 		free_token(token);
 		/* Now remove the '~' from the buffer */
@@ -1107,6 +1111,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 			break;
 		case EVENT_NONE:
 			break;
+		case EVENT_ERROR:
+			goto fail_alloc;
 		default:
 			goto fail_print;
 		}
-- 
1.7.11.7

[PATCH 06/14] tools lib traceevent: Get rid of malloc_or_die() in find_event()

[Namhyung Kim]

Make it return -2 to distinguish malloc allocation failure.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index e9d17bfcdffd..06e5af9f8fc4 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -301,7 +301,10 @@ find_event(struct pevent *pevent, struct event_list **events,
 		sys_name = NULL;
 	}
 
-	reg = malloc_or_die(strlen(event_name) + 3);
+	reg = malloc(strlen(event_name) + 3);
+	if (reg == NULL)
+		return -2;
+
 	sprintf(reg, "^%s$", event_name);
 
 	ret = regcomp(&ereg, reg, REG_ICASE|REG_NOSUB);
@@ -311,7 +314,12 @@ find_event(struct pevent *pevent, struct event_list **events,
 		return -1;
 
 	if (sys_name) {
-		reg = malloc_or_die(strlen(sys_name) + 3);
+		reg = malloc(strlen(sys_name) + 3);
+		if (reg == NULL) {
+			regfree(&ereg);
+			return -2;
+		}
+
 		sprintf(reg, "^%s$", sys_name);
 		ret = regcomp(&sreg, reg, REG_ICASE|REG_NOSUB);
 		free(reg);
@@ -1290,7 +1298,10 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
 		/* Find this event */
 		ret = find_event(pevent, &events, strim(sys_name), strim(event_name));
 		if (ret < 0) {
-			if (event_name)
+			if (ret == -2)
+				show_error(error_str,
+					   "Memory allocation failure");
+			else if (event_name)
 				show_error(error_str,
 					   "No event found under '%s.%s'",
 					   sys_name, event_name);
-- 
1.7.11.7

Re: [PATCH 06/14] tools lib traceevent: Get rid of malloc_or_die() in find_event()

[Jiri Olsa]

On Mon, Dec 09, 2013 at 02:34:03PM +0900, Namhyung Kim wrote:
> Make it return -2 to distinguish malloc allocation failure.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/parse-filter.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index e9d17bfcdffd..06e5af9f8fc4 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -301,7 +301,10 @@ find_event(struct pevent *pevent, struct event_list **events,
>  		sys_name = NULL;
>  	}
>  
> -	reg = malloc_or_die(strlen(event_name) + 3);
> +	reg = malloc(strlen(event_name) + 3);
> +	if (reg == NULL)
> +		return -2;
> +

I guess we dont need error defines or enums when this is just
static function, but at least please add some comment (description)
of return values like in pevent_filter_match function

jirka

Re: [PATCH 06/14] tools lib traceevent: Get rid of malloc_or_die() in find_event()

[Steven Rostedt]

On Mon, 9 Dec 2013 12:03:50 +0100
Jiri Olsa <jolsa@redhat.com> wrote:

> On Mon, Dec 09, 2013 at 02:34:03PM +0900, Namhyung Kim wrote:
> > Make it return -2 to distinguish malloc allocation failure.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/lib/traceevent/parse-filter.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> > index e9d17bfcdffd..06e5af9f8fc4 100644
> > --- a/tools/lib/traceevent/parse-filter.c
> > +++ b/tools/lib/traceevent/parse-filter.c
> > @@ -301,7 +301,10 @@ find_event(struct pevent *pevent, struct event_list **events,
> >  		sys_name = NULL;
> >  	}
> >  
> > -	reg = malloc_or_die(strlen(event_name) + 3);
> > +	reg = malloc(strlen(event_name) + 3);
> > +	if (reg == NULL)
> > +		return -2;
> > +
> 
> I guess we dont need error defines or enums when this is just
> static function, but at least please add some comment (description)
> of return values like in pevent_filter_match function

Even for static functions, we should have an enum too.

-- Steve

Re: [PATCH 06/14] tools lib traceevent: Get rid of malloc_or_die() in find_event()

[Namhyung Kim]

On Mon, 9 Dec 2013 12:03:50 +0100, Jiri Olsa wrote:
> On Mon, Dec 09, 2013 at 02:34:03PM +0900, Namhyung Kim wrote:
>> Make it return -2 to distinguish malloc allocation failure.
>> 
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/lib/traceevent/parse-filter.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
>> index e9d17bfcdffd..06e5af9f8fc4 100644
>> --- a/tools/lib/traceevent/parse-filter.c
>> +++ b/tools/lib/traceevent/parse-filter.c
>> @@ -301,7 +301,10 @@ find_event(struct pevent *pevent, struct event_list **events,
>>  		sys_name = NULL;
>>  	}
>>  
>> -	reg = malloc_or_die(strlen(event_name) + 3);
>> +	reg = malloc(strlen(event_name) + 3);
>> +	if (reg == NULL)
>> +		return -2;
>> +
>
> I guess we dont need error defines or enums when this is just
> static function, but at least please add some comment (description)
> of return values like in pevent_filter_match function

Well, probably we can pass the error_str to find_event().

For comments, I think we only add a formal comment on the external APIs.
So it'll look like this:

/* 
 * Returns 0 if succeeded, -1 if not matched or invalid regex,
 * -2 if memory allocation failed.
 */

What do you think?

Thanks,
Namhyung

[PATCH 07/14] tools lib traceevent: Get rid of malloc_or_die() in add_event()

[Namhyung Kim]

Make it return error value since its only caller find_event() now can
handle allocation error properly.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 06e5af9f8fc4..faa10824b87d 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -260,15 +260,19 @@ static void free_arg(struct filter_arg *arg)
 	free(arg);
 }
 
-static void add_event(struct event_list **events,
+static int add_event(struct event_list **events,
 		      struct event_format *event)
 {
 	struct event_list *list;
 
-	list = malloc_or_die(sizeof(*list));
+	list = malloc(sizeof(*list));
+	if (list == NULL)
+		return -1;
+
 	list->next = *events;
 	*events = list;
 	list->event = event;
+	return 0;
 }
 
 static int event_match(struct event_format *event,
@@ -291,6 +295,7 @@ find_event(struct pevent *pevent, struct event_list **events,
 	regex_t ereg;
 	regex_t sreg;
 	int match = 0;
+	int fail = 0;
 	char *reg;
 	int ret;
 	int i;
@@ -333,7 +338,10 @@ find_event(struct pevent *pevent, struct event_list **events,
 		event = pevent->events[i];
 		if (event_match(event, sys_name ? &sreg : NULL, &ereg)) {
 			match = 1;
-			add_event(events, event);
+			if (add_event(events, event) < 0) {
+				fail = 1;
+				break;
+			}
 		}
 	}
 
@@ -343,6 +351,8 @@ find_event(struct pevent *pevent, struct event_list **events,
 
 	if (!match)
 		return -1;
+	if (fail)
+		return -2;
 
 	return 0;
 }
-- 
1.7.11.7

[tip:perf/core] tools lib traceevent: Get rid of malloc_or_die() in add_event()

[tip-bot for Namhyung Kim]

Commit-ID:  234520d3fbe43ef72268c4959f85ae326459378c
Gitweb:     http://git.kernel.org/tip/234520d3fbe43ef72268c4959f85ae326459378c
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:04 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:39:40 -0300

tools lib traceevent: Get rid of malloc_or_die() in add_event()

Make it return error value since its only caller find_event() now can
handle allocation error properly.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index b3a61d4..2b73abf 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -245,15 +245,19 @@ static void free_arg(struct filter_arg *arg)
 	free(arg);
 }
 
-static void add_event(struct event_list **events,
+static int add_event(struct event_list **events,
 		      struct event_format *event)
 {
 	struct event_list *list;
 
-	list = malloc_or_die(sizeof(*list));
+	list = malloc(sizeof(*list));
+	if (list == NULL)
+		return -1;
+
 	list->next = *events;
 	*events = list;
 	list->event = event;
+	return 0;
 }
 
 static int event_match(struct event_format *event,
@@ -276,6 +280,7 @@ find_event(struct pevent *pevent, struct event_list **events,
 	regex_t ereg;
 	regex_t sreg;
 	int match = 0;
+	int fail = 0;
 	char *reg;
 	int ret;
 	int i;
@@ -310,7 +315,10 @@ find_event(struct pevent *pevent, struct event_list **events,
 		event = pevent->events[i];
 		if (event_match(event, sys_name ? &sreg : NULL, &ereg)) {
 			match = 1;
-			add_event(events, event);
+			if (add_event(events, event) < 0) {
+				fail = 1;
+				break;
+			}
 		}
 	}
 
@@ -320,6 +328,8 @@ find_event(struct pevent *pevent, struct event_list **events,
 
 	if (!match)
 		return -1;
+	if (fail)
+		return -2;
 
 	return 0;
 }

[PATCH 08/14] tools lib traceevent: Get rid of die() in create_arg_item()

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index faa10824b87d..5efe66a682bd 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -389,8 +389,11 @@ create_arg_item(struct event_format *event, const char *token,
 		arg->value.type =
 			type == EVENT_DQUOTE ? FILTER_STRING : FILTER_CHAR;
 		arg->value.str = strdup(token);
-		if (!arg->value.str)
-			die("malloc string");
+		if (!arg->value.str) {
+			free_arg(arg);
+			show_error(error_str, "failed to allocate string filter arg");
+			return NULL;
+		}
 		break;
 	case EVENT_ITEM:
 		/* if it is a number, then convert it */
-- 
1.7.11.7

[tip:perf/core] tools lib traceevent: Get rid of die() in create_arg_item()

[tip-bot for Namhyung Kim]

Commit-ID:  2036fcd1c7ce455424c11bdb1c8a2ac906430e2f
Gitweb:     http://git.kernel.org/tip/2036fcd1c7ce455424c11bdb1c8a2ac906430e2f
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:05 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:39:46 -0300

tools lib traceevent: Get rid of die() in create_arg_item()

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-9-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 2b73abf..53e48eb 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -362,8 +362,11 @@ create_arg_item(struct event_format *event, const char *token,
 		arg->value.type =
 			type == EVENT_DQUOTE ? FILTER_STRING : FILTER_CHAR;
 		arg->value.str = strdup(token);
-		if (!arg->value.str)
-			die("malloc string");
+		if (!arg->value.str) {
+			free_arg(arg);
+			show_error(error_str, "failed to allocate string filter arg");
+			return NULL;
+		}
 		break;
 	case EVENT_ITEM:
 		/* if it is a number, then convert it */

[PATCH 09/14] tools lib traceevent: Get rid of die() in add_right()

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 5efe66a682bd..a1ad609a860f 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -583,12 +583,18 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
 			op->str.type = op_type;
 			op->str.field = left->field.field;
 			op->str.val = strdup(str);
-			if (!op->str.val)
-				die("malloc string");
+			if (!op->str.val) {
+				show_error(error_str, "Failed to allocate string filter");
+				return -1;
+			}
 			/*
 			 * Need a buffer to copy data for tests
 			 */
-			op->str.buffer = malloc_or_die(op->str.field->size + 1);
+			op->str.buffer = malloc(op->str.field->size + 1);
+			if (op->str.buffer) {
+				show_error(error_str, "Failed to allocate string filter");
+				return -1;
+			}
 			/* Null terminate this buffer */
 			op->str.buffer[op->str.field->size] = 0;
 
-- 
1.7.11.7

Re: [PATCH 09/14] tools lib traceevent: Get rid of die() in add_right()

[Ilia Mirkin]

On Mon, Dec 9, 2013 at 12:34 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/parse-filter.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 5efe66a682bd..a1ad609a860f 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -583,12 +583,18 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
>                         op->str.type = op_type;
>                         op->str.field = left->field.field;
>                         op->str.val = strdup(str);
> -                       if (!op->str.val)
> -                               die("malloc string");
> +                       if (!op->str.val) {
> +                               show_error(error_str, "Failed to allocate string filter");
> +                               return -1;
> +                       }
>                         /*
>                          * Need a buffer to copy data for tests
>                          */
> -                       op->str.buffer = malloc_or_die(op->str.field->size + 1);
> +                       op->str.buffer = malloc(op->str.field->size + 1);
> +                       if (op->str.buffer) {

That should probably be

if (!op->str.buffer)

Also, should you free op->str.val? Perhaps the surrounding code takes
care of that.

> +                               show_error(error_str, "Failed to allocate string filter");
> +                               return -1;
> +                       }
>                         /* Null terminate this buffer */
>                         op->str.buffer[op->str.field->size] = 0;
>
> --
> 1.7.11.7
>
> --
> 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 09/14] tools lib traceevent: Get rid of die() in add_right()

[Namhyung Kim]

Hi Ilia,

On Mon, 9 Dec 2013 01:28:26 -0500, Ilia Mirkin wrote:
> On Mon, Dec 9, 2013 at 12:34 AM, Namhyung Kim <namhyung@kernel.org> wrote:
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/lib/traceevent/parse-filter.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
>> index 5efe66a682bd..a1ad609a860f 100644
>> --- a/tools/lib/traceevent/parse-filter.c
>> +++ b/tools/lib/traceevent/parse-filter.c
>> @@ -583,12 +583,18 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
>>                         op->str.type = op_type;
>>                         op->str.field = left->field.field;
>>                         op->str.val = strdup(str);
>> -                       if (!op->str.val)
>> -                               die("malloc string");
>> +                       if (!op->str.val) {
>> +                               show_error(error_str, "Failed to allocate string filter");
>> +                               return -1;
>> +                       }
>>                         /*
>>                          * Need a buffer to copy data for tests
>>                          */
>> -                       op->str.buffer = malloc_or_die(op->str.field->size + 1);
>> +                       op->str.buffer = malloc(op->str.field->size + 1);
>> +                       if (op->str.buffer) {
>
> That should probably be
>
> if (!op->str.buffer)

Argh.. you're right!

>
> Also, should you free op->str.val? Perhaps the surrounding code takes
> care of that.

Yeah, it'll be handled by the caller - process_filter().

Thanks for the quick review!
Namhyung

>
>> +                               show_error(error_str, "Failed to allocate string filter");
>> +                               return -1;
>> +                       }
>>                         /* Null terminate this buffer */
>>                         op->str.buffer[op->str.field->size] = 0;
>>
>> --
>> 1.7.11.7
>>
>> --
>> 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 09/14] tools lib traceevent: Get rid of die() in add_right()

[Steven Rostedt]

On Mon, 09 Dec 2013 15:59:26 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Ilia,
> 
> On Mon, 9 Dec 2013 01:28:26 -0500, Ilia Mirkin wrote:
> > On Mon, Dec 9, 2013 at 12:34 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> >> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >> ---
> >>  tools/lib/traceevent/parse-filter.c | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> >> index 5efe66a682bd..a1ad609a860f 100644
> >> --- a/tools/lib/traceevent/parse-filter.c
> >> +++ b/tools/lib/traceevent/parse-filter.c
> >> @@ -583,12 +583,18 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
> >>                         op->str.type = op_type;
> >>                         op->str.field = left->field.field;
> >>                         op->str.val = strdup(str);
> >> -                       if (!op->str.val)
> >> -                               die("malloc string");
> >> +                       if (!op->str.val) {
> >> +                               show_error(error_str, "Failed to allocate string filter");
> >> +                               return -1;
> >> +                       }
> >>                         /*
> >>                          * Need a buffer to copy data for tests
> >>                          */
> >> -                       op->str.buffer = malloc_or_die(op->str.field->size + 1);
> >> +                       op->str.buffer = malloc(op->str.field->size + 1);
> >> +                       if (op->str.buffer) {
> >
> > That should probably be
> >
> > if (!op->str.buffer)
> 
> Argh.. you're right!

I was thinking that it was better to do it Namhyung's way, with:

	if (op->str.buffer == NULL)

than my preferred way of:

	if (!op->str.buffer)

because I thought this mistake is more prevalent with my way. But It's
good to know that this bug happens regardless of which way you
prefer ;-)

-- Steve

Re: [PATCH 09/14] tools lib traceevent: Get rid of die() in add_right()

[Namhyung Kim]

Hi Steve,

On Mon, 9 Dec 2013 11:32:56 -0500, Steven Rostedt wrote:
> On Mon, 09 Dec 2013 15:59:26 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> Hi Ilia,
>> 
>> On Mon, 9 Dec 2013 01:28:26 -0500, Ilia Mirkin wrote:
>> > That should probably be
>> >
>> > if (!op->str.buffer)
>> 
>> Argh.. you're right!
>
> I was thinking that it was better to do it Namhyung's way, with:
>
> 	if (op->str.buffer == NULL)
>
> than my preferred way of:
>
> 	if (!op->str.buffer)
>
> because I thought this mistake is more prevalent with my way. But It's
> good to know that this bug happens regardless of which way you
> prefer ;-)

Well, while I prefer explicit "buf == NULL" style, I was using "!buf"
style at that time since the previous line of "!op->str.val" already
used that style so that's why the typo came in.  Just FYI. :)

Thanks,
Namhyung

[PATCH 10/14] tools lib traceevent: Get rid of die() in reparent_op_arg()

[Namhyung Kim]

To do that, add FILTER_VAL_ERROR to enum filter_vals and make the
function returns the error code.  Also pass error_str so that it can
set proper error message when error occurred.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 91 +++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 33 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index a1ad609a860f..dabae52bbbcb 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -781,17 +781,21 @@ enum filter_vals {
 	FILTER_VAL_NORM,
 	FILTER_VAL_FALSE,
 	FILTER_VAL_TRUE,
+	FILTER_VAL_ERROR,
 };
 
-void reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
-		  struct filter_arg *arg)
+enum filter_vals
+reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
+		struct filter_arg *arg, char **error_str)
 {
 	struct filter_arg *other_child;
 	struct filter_arg **ptr;
 
 	if (parent->type != FILTER_ARG_OP &&
-	    arg->type != FILTER_ARG_OP)
-		die("can not reparent other than OP");
+	    arg->type != FILTER_ARG_OP) {
+		show_error(error_str, "can not reparent other than OP");
+		return FILTER_VAL_ERROR;
+	}
 
 	/* Get the sibling */
 	if (old_child->op.right == arg) {
@@ -800,8 +804,10 @@ void reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
 	} else if (old_child->op.left == arg) {
 		ptr = &old_child->op.left;
 		other_child = old_child->op.right;
-	} else
-		die("Error in reparent op, find other child");
+	} else {
+		show_error(error_str, "Error in reparent op, find other child");
+		return FILTER_VAL_ERROR;
+	}
 
 	/* Detach arg from old_child */
 	*ptr = NULL;
@@ -812,21 +818,25 @@ void reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
 		*parent = *arg;
 		/* Free arg without recussion */
 		free(arg);
-		return;
+		return FILTER_VAL_NORM;
 	}
 
 	if (parent->op.right == old_child)
 		ptr = &parent->op.right;
 	else if (parent->op.left == old_child)
 		ptr = &parent->op.left;
-	else
-		die("Error in reparent op");
+	else {
+		show_error(error_str, "Error in reparent op");
+		return FILTER_VAL_ERROR;
+	}
 	*ptr = arg;
 
 	free_arg(old_child);
+	return FILTER_VAL_NORM;
 }
 
-enum filter_vals test_arg(struct filter_arg *parent, struct filter_arg *arg)
+enum filter_vals test_arg(struct filter_arg *parent, struct filter_arg *arg,
+			  char **error_str)
 {
 	enum filter_vals lval, rval;
 
@@ -843,63 +853,68 @@ enum filter_vals test_arg(struct filter_arg *parent, struct filter_arg *arg)
 		return FILTER_VAL_NORM;
 
 	case FILTER_ARG_EXP:
-		lval = test_arg(arg, arg->exp.left);
+		lval = test_arg(arg, arg->exp.left, error_str);
 		if (lval != FILTER_VAL_NORM)
 			return lval;
-		rval = test_arg(arg, arg->exp.right);
+		rval = test_arg(arg, arg->exp.right, error_str);
 		if (rval != FILTER_VAL_NORM)
 			return rval;
 		return FILTER_VAL_NORM;
 
 	case FILTER_ARG_NUM:
-		lval = test_arg(arg, arg->num.left);
+		lval = test_arg(arg, arg->num.left, error_str);
 		if (lval != FILTER_VAL_NORM)
 			return lval;
-		rval = test_arg(arg, arg->num.right);
+		rval = test_arg(arg, arg->num.right, error_str);
 		if (rval != FILTER_VAL_NORM)
 			return rval;
 		return FILTER_VAL_NORM;
 
 	case FILTER_ARG_OP:
 		if (arg->op.type != FILTER_OP_NOT) {
-			lval = test_arg(arg, arg->op.left);
+			lval = test_arg(arg, arg->op.left, error_str);
 			switch (lval) {
 			case FILTER_VAL_NORM:
 				break;
 			case FILTER_VAL_TRUE:
 				if (arg->op.type == FILTER_OP_OR)
 					return FILTER_VAL_TRUE;
-				rval = test_arg(arg, arg->op.right);
+				rval = test_arg(arg, arg->op.right, error_str);
 				if (rval != FILTER_VAL_NORM)
 					return rval;
 
-				reparent_op_arg(parent, arg, arg->op.right);
-				return FILTER_VAL_NORM;
+				return reparent_op_arg(parent, arg, arg->op.right,
+						       error_str);
 
 			case FILTER_VAL_FALSE:
 				if (arg->op.type == FILTER_OP_AND)
 					return FILTER_VAL_FALSE;
-				rval = test_arg(arg, arg->op.right);
+				rval = test_arg(arg, arg->op.right, error_str);
 				if (rval != FILTER_VAL_NORM)
 					return rval;
 
-				reparent_op_arg(parent, arg, arg->op.right);
-				return FILTER_VAL_NORM;
+				return reparent_op_arg(parent, arg, arg->op.right,
+						       error_str);
+
+			case FILTER_VAL_ERROR:
+				return FILTER_VAL_ERROR;
 			}
 		}
 
-		rval = test_arg(arg, arg->op.right);
+		rval = test_arg(arg, arg->op.right, error_str);
 		switch (rval) {
 		case FILTER_VAL_NORM:
+		case FILTER_VAL_ERROR:
 			break;
+
 		case FILTER_VAL_TRUE:
 			if (arg->op.type == FILTER_OP_OR)
 				return FILTER_VAL_TRUE;
 			if (arg->op.type == FILTER_OP_NOT)
 				return FILTER_VAL_FALSE;
 
-			reparent_op_arg(parent, arg, arg->op.left);
-			return FILTER_VAL_NORM;
+			return reparent_op_arg(parent, arg, arg->op.left,
+					       error_str);
 
 		case FILTER_VAL_FALSE:
 			if (arg->op.type == FILTER_OP_AND)
@@ -907,26 +922,27 @@ enum filter_vals test_arg(struct filter_arg *parent, struct filter_arg *arg)
 			if (arg->op.type == FILTER_OP_NOT)
 				return FILTER_VAL_TRUE;
 
-			reparent_op_arg(parent, arg, arg->op.left);
-			return FILTER_VAL_NORM;
+			return reparent_op_arg(parent, arg, arg->op.left,
+					       error_str);
 		}
 
-		return FILTER_VAL_NORM;
+		return rval;
 	default:
-		die("bad arg in filter tree");
+		show_error(error_str, "bad arg in filter tree");
+		return FILTER_VAL_ERROR;
 	}
 	return FILTER_VAL_NORM;
 }
 
 /* Remove any unknown event fields */
-static struct filter_arg *collapse_tree(struct filter_arg *arg)
+static struct filter_arg *collapse_tree(struct filter_arg *arg, char **error_str)
 {
 	enum filter_vals ret;
 
-	ret = test_arg(arg, arg);
+	ret = test_arg(arg, arg, error_str);
 	switch (ret) {
 	case FILTER_VAL_NORM:
-		return arg;
+		break;
 
 	case FILTER_VAL_TRUE:
 	case FILTER_VAL_FALSE:
@@ -935,7 +951,16 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg)
 		if (arg) {
 			arg->type = FILTER_ARG_BOOLEAN;
 			arg->boolean.value = ret == FILTER_VAL_TRUE;
+		} else {
+			show_error(error_str, "Failed to allocate filter arg");
 		}
+		break;
+
+	case FILTER_VAL_ERROR:
+		/* test_arg() already set the error_str */
+		free_arg(arg);
+		arg = NULL;
+		break;
 	}
 
 	return arg;
@@ -1151,9 +1176,9 @@ process_filter(struct event_format *event, struct filter_arg **parg,
 	if (!current_op)
 		current_op = current_exp;
 
-	current_op = collapse_tree(current_op);
+	current_op = collapse_tree(current_op, error_str);
 	if (current_op == NULL)
-		goto fail_alloc;
+		goto fail;
 
 	*parg = current_op;
 
-- 
1.7.11.7

[PATCH 11/14] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index dabae52bbbcb..d8613308c08d 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1319,7 +1319,13 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
 		else
 			len = strlen(filter_str);
 
-		this_event = malloc_or_die(len + 1);
+		this_event = malloc(len + 1);
+		if (this_event == NULL) {
+			show_error(error_str, "Memory allocation failure");
+			/* This can only happen when events is NULL, but still */
+			free_events(events);
+			return -1;
+		}
 		memcpy(this_event, filter_str, len);
 		this_event[len] = 0;
 
-- 
1.7.11.7

[tip:perf/core] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()

[tip-bot for Namhyung Kim]

Commit-ID:  28942c87e5e907f591d77547203e86ad1089b499
Gitweb:     http://git.kernel.org/tip/28942c87e5e907f591d77547203e86ad1089b499
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:08 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:39:53 -0300

tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-12-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 53e48eb..a4d5bb2 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1226,7 +1226,13 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
 		else
 			len = strlen(filter_str);
 
-		this_event = malloc_or_die(len + 1);
+		this_event = malloc(len + 1);
+		if (this_event == NULL) {
+			show_error(error_str, "Memory allocation failure");
+			/* This can only happen when events is NULL, but still */
+			free_events(events);
+			return -1;
+		}
 		memcpy(this_event, filter_str, len);
 		this_event[len] = 0;
 

[PATCH 12/14] tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()

[Namhyung Kim]

Change the function signature to return error code and not call die()
anymore.

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

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 620c27a72960..6e23f197175f 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -860,7 +860,7 @@ int pevent_event_filtered(struct event_filter *filter,
 
 void pevent_filter_reset(struct event_filter *filter);
 
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type);
 
 void pevent_filter_free(struct event_filter *filter);
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index d8613308c08d..4d395e8b88bb 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1606,8 +1606,10 @@ int pevent_update_trivial(struct event_filter *dest, struct event_filter *source
  * @type: remove only true, false, or both
  *
  * Removes filters that only contain a TRUE or FALES boolean arg.
+ *
+ * Returns 0 on success and -1 if there was a problem.
  */
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type)
 {
 	struct filter_type *filter_type;
@@ -1616,13 +1618,15 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 	int i;
 
 	if (!filter->filters)
-		return;
+		return 0;
 
 	/*
 	 * Two steps, first get all ids with trivial filters.
 	 *  then remove those ids.
 	 */
 	for (i = 0; i < filter->filters; i++) {
+		int *new_ids;
+
 		filter_type = &filter->event_filters[i];
 		if (filter_type->filter->type != FILTER_ARG_BOOLEAN)
 			continue;
@@ -1637,19 +1641,24 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 			break;
 		}
 
-		ids = realloc(ids, sizeof(*ids) * (count + 1));
-		if (!ids)
-			die("Can't allocate ids");
+		new_ids = realloc(ids, sizeof(*ids) * (count + 1));
+		if (!new_ids) {
+			free(ids);
+			return -1;
+		}
+
+		ids = new_ids;
 		ids[count++] = filter_type->event_id;
 	}
 
 	if (!count)
-		return;
+		return 0;
 
 	for (i = 0; i < count; i++)
 		pevent_filter_remove_event(filter, ids[i]);
 
 	free(ids);
+	return 0;
 }
 
 /**
-- 
1.7.11.7

[tip:perf/core] tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()

[tip-bot for Namhyung Kim]

Commit-ID:  7ef2e813476273ac9c9138f002d8f4cb28e5adad
Gitweb:     http://git.kernel.org/tip/7ef2e813476273ac9c9138f002d8f4cb28e5adad
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:09 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:39:57 -0300

tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()

Change the function signature to return error code and not call die()
anymore.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-13-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/event-parse.h  |  2 +-
 tools/lib/traceevent/parse-filter.c | 21 +++++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 620c27a..6e23f19 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -860,7 +860,7 @@ int pevent_event_filtered(struct event_filter *filter,
 
 void pevent_filter_reset(struct event_filter *filter);
 
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type);
 
 void pevent_filter_free(struct event_filter *filter);
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index a4d5bb2..ab402fb 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1504,8 +1504,10 @@ int pevent_update_trivial(struct event_filter *dest, struct event_filter *source
  * @type: remove only true, false, or both
  *
  * Removes filters that only contain a TRUE or FALES boolean arg.
+ *
+ * Returns 0 on success and -1 if there was a problem.
  */
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type)
 {
 	struct filter_type *filter_type;
@@ -1514,13 +1516,15 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 	int i;
 
 	if (!filter->filters)
-		return;
+		return 0;
 
 	/*
 	 * Two steps, first get all ids with trivial filters.
 	 *  then remove those ids.
 	 */
 	for (i = 0; i < filter->filters; i++) {
+		int *new_ids;
+
 		filter_type = &filter->event_filters[i];
 		if (filter_type->filter->type != FILTER_ARG_BOOLEAN)
 			continue;
@@ -1535,19 +1539,24 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 			break;
 		}
 
-		ids = realloc(ids, sizeof(*ids) * (count + 1));
-		if (!ids)
-			die("Can't allocate ids");
+		new_ids = realloc(ids, sizeof(*ids) * (count + 1));
+		if (!new_ids) {
+			free(ids);
+			return -1;
+		}
+
+		ids = new_ids;
 		ids[count++] = filter_type->event_id;
 	}
 
 	if (!count)
-		return;
+		return 0;
 
 	for (i = 0; i < count; i++)
 		pevent_filter_remove_event(filter, ids[i]);
 
 	free(ids);
+	return 0;
 }
 
 /**

[PATCH 13/14] tools lib traceevent: Refactor test_filter() to get rid of die()

[Namhyung Kim]

The test_filter() function is for testing given filter is matched to a
given record.  However it doesn't handle error cases properly so add a
new argument error_str to save error info during the test and also
pass it to internal test functions.

For now, it just save the error but does nothing with it.  Maybe it
can be given by user through pevent_filter_match() later.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/event-parse.h  |   1 +
 tools/lib/traceevent/parse-filter.c | 102 ++++++++++++++++++++++--------------
 2 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 6e23f197175f..a1d8b2792e3a 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -836,6 +836,7 @@ struct event_filter {
 
 struct event_filter *pevent_filter_alloc(struct pevent *pevent);
 
+#define FILTER_ERROR		-3
 #define FILTER_NONE		-2
 #define FILTER_NOEXIST		-1
 #define FILTER_MISS		0
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 4d395e8b88bb..8a5b7a74b44e 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1698,8 +1698,8 @@ int pevent_filter_event_has_trivial(struct event_filter *filter,
 	}
 }
 
-static int test_filter(struct event_format *event,
-		       struct filter_arg *arg, struct pevent_record *record);
+static int test_filter(struct event_format *event, struct filter_arg *arg,
+		       struct pevent_record *record, char **error_str);
 
 static const char *
 get_comm(struct event_format *event, struct pevent_record *record)
@@ -1745,15 +1745,17 @@ get_value(struct event_format *event,
 }
 
 static unsigned long long
-get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record);
+get_arg_value(struct event_format *event, struct filter_arg *arg,
+	      struct pevent_record *record, char **error_str);
 
 static unsigned long long
-get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
+get_exp_value(struct event_format *event, struct filter_arg *arg,
+	      struct pevent_record *record, char **error_str)
 {
 	unsigned long long lval, rval;
 
-	lval = get_arg_value(event, arg->exp.left, record);
-	rval = get_arg_value(event, arg->exp.right, record);
+	lval = get_arg_value(event, arg->exp.left, record, error_str);
+	rval = get_arg_value(event, arg->exp.right, record, error_str);
 
 	switch (arg->exp.type) {
 	case FILTER_EXP_ADD:
@@ -1788,39 +1790,44 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_
 
 	case FILTER_EXP_NOT:
 	default:
-		die("error in exp");
+		if (*error_str == NULL)
+			*error_str = "invalid expression type";
 	}
 	return 0;
 }
 
 static unsigned long long
-get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
+get_arg_value(struct event_format *event, struct filter_arg *arg,
+	      struct pevent_record *record, char **error_str)
 {
 	switch (arg->type) {
 	case FILTER_ARG_FIELD:
 		return get_value(event, arg->field.field, record);
 
 	case FILTER_ARG_VALUE:
-		if (arg->value.type != FILTER_NUMBER)
-			die("must have number field!");
+		if (arg->value.type != FILTER_NUMBER) {
+			if (*error_str == NULL)
+				*error_str = "must have number field!";
+		}
 		return arg->value.val;
 
 	case FILTER_ARG_EXP:
-		return get_exp_value(event, arg, record);
+		return get_exp_value(event, arg, record, error_str);
 
 	default:
-		die("oops in filter");
+		if (*error_str == NULL)
+			*error_str = "invalid numeric argument type";
 	}
 	return 0;
 }
 
-static int test_num(struct event_format *event,
-		    struct filter_arg *arg, struct pevent_record *record)
+static int test_num(struct event_format *event, struct filter_arg *arg,
+		    struct pevent_record *record, char **error_str)
 {
 	unsigned long long lval, rval;
 
-	lval = get_arg_value(event, arg->num.left, record);
-	rval = get_arg_value(event, arg->num.right, record);
+	lval = get_arg_value(event, arg->num.left, record, error_str);
+	rval = get_arg_value(event, arg->num.right, record, error_str);
 
 	switch (arg->num.type) {
 	case FILTER_CMP_EQ:
@@ -1842,7 +1849,8 @@ static int test_num(struct event_format *event,
 		return lval <= rval;
 
 	default:
-		/* ?? */
+		if (*error_str == NULL)
+			*error_str = "invalid numeric comparison type";
 		return 0;
 	}
 }
@@ -1889,8 +1897,8 @@ static const char *get_field_str(struct filter_arg *arg, struct pevent_record *r
 	return val;
 }
 
-static int test_str(struct event_format *event,
-		    struct filter_arg *arg, struct pevent_record *record)
+static int test_str(struct event_format *event, struct filter_arg *arg,
+		    struct pevent_record *record, char **error_str)
 {
 	const char *val;
 
@@ -1914,48 +1922,57 @@ static int test_str(struct event_format *event,
 		return regexec(&arg->str.reg, val, 0, NULL, 0);
 
 	default:
-		/* ?? */
+		if (*error_str == NULL)
+			*error_str = "invalid string comparison type";
 		return 0;
 	}
 }
 
-static int test_op(struct event_format *event,
-		   struct filter_arg *arg, struct pevent_record *record)
+static int test_op(struct event_format *event, struct filter_arg *arg,
+		   struct pevent_record *record, char **error_str)
 {
 	switch (arg->op.type) {
 	case FILTER_OP_AND:
-		return test_filter(event, arg->op.left, record) &&
-			test_filter(event, arg->op.right, record);
+		return test_filter(event, arg->op.left, record, error_str) &&
+			test_filter(event, arg->op.right, record, error_str);
 
 	case FILTER_OP_OR:
-		return test_filter(event, arg->op.left, record) ||
-			test_filter(event, arg->op.right, record);
+		return test_filter(event, arg->op.left, record, error_str) ||
+			test_filter(event, arg->op.right, record, error_str);
 
 	case FILTER_OP_NOT:
-		return !test_filter(event, arg->op.right, record);
+		return !test_filter(event, arg->op.right, record, error_str);
 
 	default:
-		/* ?? */
+		if (*error_str == NULL)
+			*error_str = "invalid operator type";
 		return 0;
 	}
 }
 
-static int test_filter(struct event_format *event,
-		       struct filter_arg *arg, struct pevent_record *record)
+static int test_filter(struct event_format *event, struct filter_arg *arg,
+		       struct pevent_record *record, char **error_str)
 {
+	if (*error_str) {
+		/*
+		 * There was an error, no need to process anymore.
+		 */
+		return 0;
+	}
+
 	switch (arg->type) {
 	case FILTER_ARG_BOOLEAN:
 		/* easy case */
 		return arg->boolean.value;
 
 	case FILTER_ARG_OP:
-		return test_op(event, arg, record);
+		return test_op(event, arg, record, error_str);
 
 	case FILTER_ARG_NUM:
-		return test_num(event, arg, record);
+		return test_num(event, arg, record, error_str);
 
 	case FILTER_ARG_STR:
-		return test_str(event, arg, record);
+		return test_str(event, arg, record, error_str);
 
 	case FILTER_ARG_EXP:
 	case FILTER_ARG_VALUE:
@@ -1964,11 +1981,11 @@ static int test_filter(struct event_format *event,
 		 * Expressions, fields and values evaluate
 		 * to true if they return non zero
 		 */
-		return !!get_arg_value(event, arg, record);
+		return !!get_arg_value(event, arg, record, error_str);
 
 	default:
-		die("oops!");
-		/* ?? */
+		if (*error_str == NULL)
+			*error_str = "invalid argument type";
 		return 0;
 	}
 }
@@ -2004,6 +2021,7 @@ int pevent_event_filtered(struct event_filter *filter,
  *  0 - filter found for event and @record does not match
  * -1 - no filter found for @record's event
  * -2 - if no filters exist
+ * -3 - if error occurred during test
  */
 int pevent_filter_match(struct event_filter *filter,
 			struct pevent_record *record)
@@ -2011,6 +2029,8 @@ int pevent_filter_match(struct event_filter *filter,
 	struct pevent *pevent = filter->pevent;
 	struct filter_type *filter_type;
 	int event_id;
+	char *error_str = NULL;
+	int ret;
 
 	if (!filter->filters)
 		return FILTER_NONE;
@@ -2022,8 +2042,14 @@ int pevent_filter_match(struct event_filter *filter,
 	if (!filter_type)
 		return FILTER_NOEXIST;
 
-	return test_filter(filter_type->event, filter_type->filter, record) ?
-		FILTER_MATCH : FILTER_MISS;
+	ret = test_filter(filter_type->event, filter_type->filter, record,
+			  &error_str);
+	if (error_str) {
+		/* TODO: maybe we can print it or pass back to user */
+		return FILTER_ERROR;
+	}
+
+	return ret ? FILTER_MATCH : FILTER_MISS;
 }
 
 static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
-- 
1.7.11.7

Re: [PATCH 13/14] tools lib traceevent: Refactor test_filter() to get rid of die()

[Steven Rostedt]

On Mon,  9 Dec 2013 14:34:10 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> The test_filter() function is for testing given filter is matched to a
> given record.  However it doesn't handle error cases properly so add a
> new argument error_str to save error info during the test and also
> pass it to internal test functions.
> 
> For now, it just save the error but does nothing with it.  Maybe it
> can be given by user through pevent_filter_match() later.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/lib/traceevent/event-parse.h  |   1 +
>  tools/lib/traceevent/parse-filter.c | 102 ++++++++++++++++++++++--------------
>  2 files changed, 65 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 6e23f197175f..a1d8b2792e3a 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -836,6 +836,7 @@ struct event_filter {
>  
>  struct event_filter *pevent_filter_alloc(struct pevent *pevent);
>  
> +#define FILTER_ERROR		-3
>  #define FILTER_NONE		-2
>  #define FILTER_NOEXIST		-1
>  #define FILTER_MISS		0
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 4d395e8b88bb..8a5b7a74b44e 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -1698,8 +1698,8 @@ int pevent_filter_event_has_trivial(struct event_filter *filter,
>  	}
>  }
>  
> -static int test_filter(struct event_format *event,
> -		       struct filter_arg *arg, struct pevent_record *record);
> +static int test_filter(struct event_format *event, struct filter_arg *arg,
> +		       struct pevent_record *record, char **error_str);
>  
>  static const char *
>  get_comm(struct event_format *event, struct pevent_record *record)
> @@ -1745,15 +1745,17 @@ get_value(struct event_format *event,
>  }
>  
>  static unsigned long long
> -get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record);
> +get_arg_value(struct event_format *event, struct filter_arg *arg,
> +	      struct pevent_record *record, char **error_str);
>  
>  static unsigned long long
> -get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
> +get_exp_value(struct event_format *event, struct filter_arg *arg,
> +	      struct pevent_record *record, char **error_str)
>  {
>  	unsigned long long lval, rval;
>  
> -	lval = get_arg_value(event, arg->exp.left, record);
> -	rval = get_arg_value(event, arg->exp.right, record);
> +	lval = get_arg_value(event, arg->exp.left, record, error_str);
> +	rval = get_arg_value(event, arg->exp.right, record, error_str);
>  
>  	switch (arg->exp.type) {
>  	case FILTER_EXP_ADD:
> @@ -1788,39 +1790,44 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_
>  
>  	case FILTER_EXP_NOT:
>  	default:
> -		die("error in exp");
> +		if (*error_str == NULL)
> +			*error_str = "invalid expression type";

Hmm, how do we tell the caller that there was an error? Do they just
check to see if error_str was changed?

>  	}
>  	return 0;
>  }
>  
>  static unsigned long long
> -get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
> +get_arg_value(struct event_format *event, struct filter_arg *arg,
> +	      struct pevent_record *record, char **error_str)
>  {
>  	switch (arg->type) {
>  	case FILTER_ARG_FIELD:
>  		return get_value(event, arg->field.field, record);
>  
>  	case FILTER_ARG_VALUE:
> -		if (arg->value.type != FILTER_NUMBER)
> -			die("must have number field!");
> +		if (arg->value.type != FILTER_NUMBER) {
> +			if (*error_str == NULL)
> +				*error_str = "must have number field!";
> +		}
>  		return arg->value.val;
>  
>  	case FILTER_ARG_EXP:
> -		return get_exp_value(event, arg, record);
> +		return get_exp_value(event, arg, record, error_str);
>  
>  	default:
> -		die("oops in filter");
> +		if (*error_str == NULL)
> +			*error_str = "invalid numeric argument type";
>  	}
>  	return 0;
>  }
>  
> -static int test_num(struct event_format *event,
> -		    struct filter_arg *arg, struct pevent_record *record)
> +static int test_num(struct event_format *event, struct filter_arg *arg,
> +		    struct pevent_record *record, char **error_str)
>  {
>  	unsigned long long lval, rval;
>  
> -	lval = get_arg_value(event, arg->num.left, record);
> -	rval = get_arg_value(event, arg->num.right, record);
> +	lval = get_arg_value(event, arg->num.left, record, error_str);
> +	rval = get_arg_value(event, arg->num.right, record, error_str);
>  
>  	switch (arg->num.type) {
>  	case FILTER_CMP_EQ:
> @@ -1842,7 +1849,8 @@ static int test_num(struct event_format *event,
>  		return lval <= rval;
>  
>  	default:
> -		/* ?? */
> +		if (*error_str == NULL)
> +			*error_str = "invalid numeric comparison type";
>  		return 0;
>  	}
>  }
> @@ -1889,8 +1897,8 @@ static const char *get_field_str(struct filter_arg *arg, struct pevent_record *r
>  	return val;
>  }
>  
> -static int test_str(struct event_format *event,
> -		    struct filter_arg *arg, struct pevent_record *record)
> +static int test_str(struct event_format *event, struct filter_arg *arg,
> +		    struct pevent_record *record, char **error_str)
>  {
>  	const char *val;
>  
> @@ -1914,48 +1922,57 @@ static int test_str(struct event_format *event,
>  		return regexec(&arg->str.reg, val, 0, NULL, 0);
>  
>  	default:
> -		/* ?? */
> +		if (*error_str == NULL)
> +			*error_str = "invalid string comparison type";
>  		return 0;
>  	}
>  }
>  
> -static int test_op(struct event_format *event,
> -		   struct filter_arg *arg, struct pevent_record *record)
> +static int test_op(struct event_format *event, struct filter_arg *arg,
> +		   struct pevent_record *record, char **error_str)
>  {
>  	switch (arg->op.type) {
>  	case FILTER_OP_AND:
> -		return test_filter(event, arg->op.left, record) &&
> -			test_filter(event, arg->op.right, record);
> +		return test_filter(event, arg->op.left, record, error_str) &&
> +			test_filter(event, arg->op.right, record, error_str);
>  
>  	case FILTER_OP_OR:
> -		return test_filter(event, arg->op.left, record) ||
> -			test_filter(event, arg->op.right, record);
> +		return test_filter(event, arg->op.left, record, error_str) ||
> +			test_filter(event, arg->op.right, record, error_str);
>  
>  	case FILTER_OP_NOT:
> -		return !test_filter(event, arg->op.right, record);
> +		return !test_filter(event, arg->op.right, record, error_str);
>  
>  	default:
> -		/* ?? */
> +		if (*error_str == NULL)
> +			*error_str = "invalid operator type";
>  		return 0;
>  	}
>  }
>  
> -static int test_filter(struct event_format *event,
> -		       struct filter_arg *arg, struct pevent_record *record)
> +static int test_filter(struct event_format *event, struct filter_arg *arg,
> +		       struct pevent_record *record, char **error_str)
>  {
> +	if (*error_str) {
> +		/*
> +		 * There was an error, no need to process anymore.
> +		 */
> +		return 0;
> +	}
> +
>  	switch (arg->type) {
>  	case FILTER_ARG_BOOLEAN:
>  		/* easy case */
>  		return arg->boolean.value;
>  
>  	case FILTER_ARG_OP:
> -		return test_op(event, arg, record);
> +		return test_op(event, arg, record, error_str);
>  
>  	case FILTER_ARG_NUM:
> -		return test_num(event, arg, record);
> +		return test_num(event, arg, record, error_str);
>  
>  	case FILTER_ARG_STR:
> -		return test_str(event, arg, record);
> +		return test_str(event, arg, record, error_str);
>  
>  	case FILTER_ARG_EXP:
>  	case FILTER_ARG_VALUE:
> @@ -1964,11 +1981,11 @@ static int test_filter(struct event_format *event,
>  		 * Expressions, fields and values evaluate
>  		 * to true if they return non zero
>  		 */
> -		return !!get_arg_value(event, arg, record);
> +		return !!get_arg_value(event, arg, record, error_str);
>  
>  	default:
> -		die("oops!");
> -		/* ?? */
> +		if (*error_str == NULL)
> +			*error_str = "invalid argument type";
>  		return 0;
>  	}
>  }
> @@ -2004,6 +2021,7 @@ int pevent_event_filtered(struct event_filter *filter,
>   *  0 - filter found for event and @record does not match
>   * -1 - no filter found for @record's event
>   * -2 - if no filters exist
> + * -3 - if error occurred during test
>   */
>  int pevent_filter_match(struct event_filter *filter,
>  			struct pevent_record *record)
> @@ -2011,6 +2029,8 @@ int pevent_filter_match(struct event_filter *filter,
>  	struct pevent *pevent = filter->pevent;
>  	struct filter_type *filter_type;
>  	int event_id;
> +	char *error_str = NULL;
> +	int ret;
>  
>  	if (!filter->filters)
>  		return FILTER_NONE;
> @@ -2022,8 +2042,14 @@ int pevent_filter_match(struct event_filter *filter,
>  	if (!filter_type)
>  		return FILTER_NOEXIST;
>  
> -	return test_filter(filter_type->event, filter_type->filter, record) ?
> -		FILTER_MATCH : FILTER_MISS;
> +	ret = test_filter(filter_type->event, filter_type->filter, record,
> +			  &error_str);
> +	if (error_str) {
> +		/* TODO: maybe we can print it or pass back to user */

Ah, I guess this answers my question :-)

Maybe we can save the error_str in the pevent. Then we can extract it
later. The return of FILTER_ERROR will let the user see what happened.

-- Steve

> +		return FILTER_ERROR;
> +	}
> +
> +	return ret ? FILTER_MATCH : FILTER_MISS;
>  }
>  
>  static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

Re: [PATCH 13/14] tools lib traceevent: Refactor test_filter() to get rid of die()

[Namhyung Kim]

On Mon, 9 Dec 2013 11:19:36 -0500, Steven Rostedt wrote:
> On Mon,  9 Dec 2013 14:34:10 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>> @@ -1788,39 +1790,44 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_
>>  
>>  	case FILTER_EXP_NOT:
>>  	default:
>> -		die("error in exp");
>> +		if (*error_str == NULL)
>> +			*error_str = "invalid expression type";
>
> Hmm, how do we tell the caller that there was an error? Do they just
> check to see if error_str was changed?
>
>>  	}
>>  	return 0;
>>  }
[SNIP]
>> @@ -2004,6 +2021,7 @@ int pevent_event_filtered(struct event_filter *filter,
>>   *  0 - filter found for event and @record does not match
>>   * -1 - no filter found for @record's event
>>   * -2 - if no filters exist
>> + * -3 - if error occurred during test
>>   */
>>  int pevent_filter_match(struct event_filter *filter,
>>  			struct pevent_record *record)
>> @@ -2011,6 +2029,8 @@ int pevent_filter_match(struct event_filter *filter,
>>  	struct pevent *pevent = filter->pevent;
>>  	struct filter_type *filter_type;
>>  	int event_id;
>> +	char *error_str = NULL;
>> +	int ret;
>>  
>>  	if (!filter->filters)
>>  		return FILTER_NONE;
>> @@ -2022,8 +2042,14 @@ int pevent_filter_match(struct event_filter *filter,
>>  	if (!filter_type)
>>  		return FILTER_NOEXIST;
>>  
>> -	return test_filter(filter_type->event, filter_type->filter, record) ?
>> -		FILTER_MATCH : FILTER_MISS;
>> +	ret = test_filter(filter_type->event, filter_type->filter, record,
>> +			  &error_str);
>> +	if (error_str) {
>> +		/* TODO: maybe we can print it or pass back to user */
>
> Ah, I guess this answers my question :-)

Right.  I was also considering what's the best way to handle error..

>
> Maybe we can save the error_str in the pevent. Then we can extract it
> later. The return of FILTER_ERROR will let the user see what happened.

Yes, but then we need the extraction and free-ing APIs too.

Or else, we can do similar to pevent_errno/strerror - returns a specific
error code and print it with user-supplied buffer.  If so we need to
think about whether consolidating it with existing pevent API or making
it a separate filter-specific API IMHO.

Thanks,
Namhyung

>
>> +		return FILTER_ERROR;
>> +	}
>> +
>> +	return ret ? FILTER_MATCH : FILTER_MISS;
>>  }
>>  
>>  static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

[PATCH 14/14] tools lib traceevent: Get rid of die() in some string conversion funcitons

[Namhyung Kim]

Those functions are for stringify filter arguments.  As caller of
those functions handles NULL string properly, it seems that it's
enough to return NULL rather than calling die().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 51 ++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 8a5b7a74b44e..ff95da94eee2 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -2108,7 +2108,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
 				default:
 					break;
 				}
-				str = malloc_or_die(6);
+				str = malloc(6);
+				if (str == NULL)
+					break;
 				if (val)
 					strcpy(str, "TRUE");
 				else
@@ -2131,7 +2133,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
 		}
 
 		len = strlen(left) + strlen(right) + strlen(op) + 10;
-		str = malloc_or_die(len);
+		str = malloc(len);
+		if (str == NULL)
+			break;
 		snprintf(str, len, "(%s) %s (%s)",
 			 left, op, right);
 		break;
@@ -2149,7 +2153,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
 			right_val = 0;
 		if (right_val >= 0) {
 			/* just return the opposite */
-			str = malloc_or_die(6);
+			str = malloc(6);
+			if (str == NULL)
+				break;
 			if (right_val)
 				strcpy(str, "FALSE");
 			else
@@ -2157,8 +2163,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
 			break;
 		}
 		len = strlen(right) + strlen(op) + 3;
-		str = malloc_or_die(len);
-		snprintf(str, len, "%s(%s)", op, right);
+		str = malloc(len);
+		if (str)
+			snprintf(str, len, "%s(%s)", op, right);
 		break;
 
 	default:
@@ -2174,9 +2181,9 @@ static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
 {
 	char *str;
 
-	str = malloc_or_die(30);
-
-	snprintf(str, 30, "%lld", arg->value.val);
+	str = malloc(30);
+	if (str)
+		snprintf(str, 30, "%lld", arg->value.val);
 
 	return str;
 }
@@ -2231,12 +2238,13 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
 		op = "^";
 		break;
 	default:
-		die("oops in exp");
+		break;
 	}
 
 	len = strlen(op) + strlen(lstr) + strlen(rstr) + 4;
-	str = malloc_or_die(len);
-	snprintf(str, len, "%s %s %s", lstr, op, rstr);
+	str = malloc(len);
+	if (str)
+		snprintf(str, len, "%s %s %s", lstr, op, rstr);
 out:
 	free(lstr);
 	free(rstr);
@@ -2282,9 +2290,9 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
 			op = "<=";
 
 		len = strlen(lstr) + strlen(op) + strlen(rstr) + 4;
-		str = malloc_or_die(len);
-		sprintf(str, "%s %s %s", lstr, op, rstr);
-
+		str = malloc(len);
+		if (str)
+			sprintf(str, "%s %s %s", lstr, op, rstr);
 		break;
 
 	default:
@@ -2322,10 +2330,11 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
 
 		len = strlen(arg->str.field->name) + strlen(op) +
 			strlen(arg->str.val) + 6;
-		str = malloc_or_die(len);
-		snprintf(str, len, "%s %s \"%s\"",
-			 arg->str.field->name,
-			 op, arg->str.val);
+		str = malloc(len);
+		if (str) {
+			snprintf(str, len, "%s %s \"%s\"",
+				 arg->str.field->name, op, arg->str.val);
+		}
 		break;
 
 	default:
@@ -2341,7 +2350,9 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
 
 	switch (arg->type) {
 	case FILTER_ARG_BOOLEAN:
-		str = malloc_or_die(6);
+		str = malloc(6);
+		if (str == NULL)
+			return NULL;
 		if (arg->boolean.value)
 			strcpy(str, "TRUE");
 		else
@@ -2380,7 +2391,7 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
  *
  * Returns a string that displays the filter contents.
  *  This string must be freed with free(str).
- *  NULL is returned if no filter is found.
+ *  NULL is returned if no filter is found or allocation failed.
  */
 char *
 pevent_filter_make_string(struct event_filter *filter, int event_id)
-- 
1.7.11.7

Re: [PATCH 14/14] tools lib traceevent: Get rid of die() in some string conversion funcitons

[Steven Rostedt]

On Mon,  9 Dec 2013 14:34:11 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> @@ -2231,12 +2238,13 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
>  		op = "^";
>  		break;
>  	default:
> -		die("oops in exp");
> +		break;
>  	}

This looks like we silently ignored the warning. Perhaps we should have:

	default:
		op = "[ERROR IN EXPRESSION TYPE]";
		break;


-- Steve

>  
>  	len = strlen(op) + strlen(lstr) + strlen(rstr) + 4;
> -	str = malloc_or_die(len);
> -	snprintf(str, len, "%s %s %s", lstr, op, rstr);
> +	str = malloc(len);
> +	if (str)
> +		snprintf(str, len, "%s %s %s", lstr, op, rstr);
>  out:
>  	free(lstr);
>  	free(rstr);

Re: [PATCH 14/14] tools lib traceevent: Get rid of die() in some string conversion funcitons

[Namhyung Kim]

On Mon, 9 Dec 2013 11:24:10 -0500, Steven Rostedt wrote:
> On Mon,  9 Dec 2013 14:34:11 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> @@ -2231,12 +2238,13 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
>>  		op = "^";
>>  		break;
>>  	default:
>> -		die("oops in exp");
>> +		break;
>>  	}
>
> This looks like we silently ignored the warning. Perhaps we should have:
>
> 	default:
> 		op = "[ERROR IN EXPRESSION TYPE]";
> 		break;

Ah, looks better. :)

Thanks,
Namhyung

Re: [PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Jiri Olsa]

On Mon, Dec 09, 2013 at 02:33:57PM +0900, Namhyung Kim wrote:
> Hello,

SNIP

> 
>  tools/lib/traceevent/event-parse.h  |   3 +-
>  tools/lib/traceevent/parse-filter.c | 432 +++++++++++++++++++++++++-----------

I guess we have plans to actualy use parse-filter.c right? ;-)

AFAICS currently there's no user and could (should?) be omited
from compilation

jirka

>  2 files changed, 303 insertions(+), 132 deletions(-)
> 
> -- 
> 1.7.11.7
> 

Re: [PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Steven Rostedt]

On Mon, 9 Dec 2013 11:47:18 +0100
Jiri Olsa <jolsa@redhat.com> wrote:

> On Mon, Dec 09, 2013 at 02:33:57PM +0900, Namhyung Kim wrote:
> > Hello,
> 
> SNIP
> 
> > 
> >  tools/lib/traceevent/event-parse.h  |   3 +-
> >  tools/lib/traceevent/parse-filter.c | 432 +++++++++++++++++++++++++-----------
> 
> I guess we have plans to actualy use parse-filter.c right? ;-)
> 
> AFAICS currently there's no user and could (should?) be omited
> from compilation
> 

There's no user in tools/ but there's users outside of tools. If this
is going to be a user space library, then I wouldn't hold it to the
same level that the main kernel has. That is, if there's no internal
users of an interface, that we do not support it.

This library did come from trace-cmd, and trace-cmd does use these
interfaces. I would like to have a single repository for the library,
but that wont happen if we don't support all the interfaces.

-- Steve

Re: [PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Steven Rostedt]

On Mon,  9 Dec 2013 14:33:57 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hello,
> 
> This patchset tries to remove all die() calls in event filter parsing
> code.  The only remaining bits are in trace-seq.c which implement
> print functions and I want to hear what's the best way we can handle
> the error case during the print.
> 
> I also put this patches on libtraceevent/die-removal-v1 branch in my tree
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> 
> Any comments are welcome, thanks
> Namhyung
> 

Other than what I commented on, the rest can have my:

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

-- Steve

> 
> Namhyung Kim (14):
>   tools lib traceevent: Get rid of malloc_or_die() in show_error()
>   tools lib traceevent: Get rid of die in add_filter_type()
>   tools lib traceevent: Get rid of malloc_or_die() in
>     pevent_filter_alloc()
>   tools lib traceevent: Get rid of malloc_or_die() allocate_arg()
>   tools lib traceevent: Get rid of malloc_or_die() in read_token()
>   tools lib traceevent: Get rid of malloc_or_die() in find_event()
>   tools lib traceevent: Get rid of malloc_or_die() in add_event()
>   tools lib traceevent: Get rid of die() in create_arg_item()
>   tools lib traceevent: Get rid of die() in add_right()
>   tools lib traceevent: Get rid of die() in reparent_op_arg()
>   tools lib traceevent: Get rid of malloc_or_die() in
>     pevent_filter_add_filter_str()
>   tools lib traceevent: Get rid of die() in
>     pevent_filter_clear_trivial()
>   tools lib traceevent: Refactor test_filter() to get rid of die()
>   tools lib traceevent: Get rid of die() in some string conversion
>     funcitons
> 
>  tools/lib/traceevent/event-parse.h  |   3 +-
>  tools/lib/traceevent/parse-filter.c | 432 +++++++++++++++++++++++++-----------
>  2 files changed, 303 insertions(+), 132 deletions(-)
> 

Re: [PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Arnaldo Carvalho de Melo]

Em Mon, Dec 09, 2013 at 11:24:49AM -0500, Steven Rostedt escreveu:
> On Mon,  9 Dec 2013 14:33:57 +0900 Namhyung Kim <namhyung@kernel.org> wrote:
> > This patchset tries to remove all die() calls in event filter parsing
> > code.  The only remaining bits are in trace-seq.c which implement
> > print functions and I want to hear what's the best way we can handle
> > the error case during the print.

> Other than what I commented on, the rest can have my:
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

Added this to all the ones I applied, Namhyung, please address the other
concerns and check which ones were not applied so that you can have them
in your next request.

Thanks a lot!

- Arnaldo

Re: [PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c

[Namhyung Kim]

Hi Arnaldo,

On Mon, 9 Dec 2013 15:41:19 -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 09, 2013 at 11:24:49AM -0500, Steven Rostedt escreveu:
>> On Mon,  9 Dec 2013 14:33:57 +0900 Namhyung Kim <namhyung@kernel.org> wrote:
>> > This patchset tries to remove all die() calls in event filter parsing
>> > code.  The only remaining bits are in trace-seq.c which implement
>> > print functions and I want to hear what's the best way we can handle
>> > the error case during the print.
>
>> Other than what I commented on, the rest can have my:
>> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
>
> Added this to all the ones I applied, Namhyung, please address the other
> concerns and check which ones were not applied so that you can have them
> in your next request.

Okay, will do!

Thanks,
Namhyung

[PATCHSET 00/14] tools lib traceevent: Get rid of *die() calls from parse-filter.c (v2)

[Namhyung Kim]

Hello,

This patchset tries to remove all die() calls in event filter parsing
code.  I changed two main functions of pevent_filter_add_filter_str()
and pevent_filter_match() to return a proper error code (pevent_errno).

The actual error message might be saved in a static buffer in pevent_
filter and it can be accessed by new pevent_filter_strerror() function.
The old pevent_strerror() still works for them too.

The only remaining bits are in trace-seq.c which implement print
functions and I want to hear what's the best way we can handle the
error case during the print.

I also put this patches on libtraceevent/die-removal-v2 branch in my tree

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git


Any comments are welcome, thanks
Namhyung


Namhyung Kim (14):
  tools lib traceevent: Get rid of malloc_or_die() in show_error()
  tools lib traceevent: Get rid of die in add_filter_type()
  tools lib traceevent: Get rid of malloc_or_die() allocate_arg()
  tools lib traceevent: Get rid of malloc_or_die() in read_token()
  tools lib traceevent: Get rid of malloc_or_die() in find_event()
  tools lib traceevent: Get rid of die() in add_right()
  tools lib traceevent: Make add_left() return pevent_errno
  tools lib traceevent: Get rid of die() in reparent_op_arg()
  tools lib traceevent: Refactor create_arg_item()
  tools lib traceevent: Refactor process_filter()
  tools lib traceevent: Make pevent_filter_add_filter_str() return
    pevent_errno
  tools lib traceevent: Refactor pevent_filter_match() to get rid of
    die()
  tools lib traceevent: Get rid of die() in some string conversion
    funcitons
  tools lib traceevent: Introduce pevent_filter_strerror()

 tools/lib/traceevent/event-parse.c  |  17 +-
 tools/lib/traceevent/event-parse.h  |  48 ++-
 tools/lib/traceevent/parse-filter.c | 615 ++++++++++++++++++++++--------------
 3 files changed, 417 insertions(+), 263 deletions(-)

-- 
1.7.11.7

[PATCH 02/14] tools lib traceevent: Get rid of die in add_filter_type()

[Namhyung Kim]

The realloc() should check return value and not to overwrite previous
pointer in case of error.

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/traceevent/parse-filter.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index d4b0bac80dc8..767de4f1e8ee 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -161,11 +161,13 @@ add_filter_type(struct event_filter *filter, int id)
 	if (filter_type)
 		return filter_type;
 
-	filter->event_filters =	realloc(filter->event_filters,
-					sizeof(*filter->event_filters) *
-					(filter->filters + 1));
-	if (!filter->event_filters)
-		die("Could not allocate filter");
+	filter_type = realloc(filter->event_filters,
+			      sizeof(*filter->event_filters) *
+			      (filter->filters + 1));
+	if (!filter_type)
+		return NULL;
+
+	filter->event_filters = filter_type;
 
 	for (i = 0; i < filter->filters; i++) {
 		if (filter->event_filters[i].event_id > id)
@@ -1180,6 +1182,12 @@ static int filter_event(struct event_filter *filter,
 	}
 
 	filter_type = add_filter_type(filter, event->id);
+	if (filter_type == NULL) {
+		show_error(error_str, "failed to add a new filter: %s",
+			   filter_str ? filter_str : "true");
+		return -1;
+	}
+
 	if (filter_type->filter)
 		free_arg(filter_type->filter);
 	filter_type->filter = arg;
@@ -1417,6 +1425,9 @@ static int copy_filter_type(struct event_filter *filter,
 			arg->boolean.value = 0;
 
 		filter_type = add_filter_type(filter, event->id);
+		if (filter_type == NULL)
+			return -1;
+
 		filter_type->filter = arg;
 
 		free(str);
-- 
1.7.11.7