Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

2 views
Skip to first unread message

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
Make it return -2 to distinguish malloc allocation failure.

Signed-off-by: Namhyung Kim <namh...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
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 <namh...@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)

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
Signed-off-by: Namhyung Kim <namh...@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;

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
The realloc() should check return value and not to overwrite previous
pointer in case of error.

Signed-off-by: Namhyung Kim <namh...@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);

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
Signed-off-by: Namhyung Kim <namh...@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;

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
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 <namh...@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)

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
Change the function signature to return error code and not call die()
anymore.

Signed-off-by: Namhyung Kim <namh...@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;
}

/**

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
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(-)

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
Also check return value and handle it.

Signed-off-by: Namhyung Kim <namh...@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;

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
Signed-off-by: Namhyung Kim <namh...@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;

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
Signed-off-by: Namhyung Kim <namh...@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);

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
It returns NULL when allocation fails so the users should check the
return value from now on.

Signed-off-by: Namhyung Kim <namh...@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);

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
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 <namh...@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;

Namhyung Kim

unread,
Dec 9, 2013, 12:40:01 AM12/9/13
to
Signed-off-by: Namhyung Kim <namh...@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 */

Namhyung Kim

unread,
Dec 9, 2013, 12:40:02 AM12/9/13
to
Make it return error value since its only caller find_event() now can
handle allocation error properly.

Signed-off-by: Namhyung Kim <namh...@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;

Ilia Mirkin

unread,
Dec 9, 2013, 1:30:02 AM12/9/13
to
That should probably be

if (!op->str.buffer)

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

Namhyung Kim

unread,
Dec 9, 2013, 2:00:02 AM12/9/13
to
Hi Ilia,
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

Jiri Olsa

unread,
Dec 9, 2013, 5:50:02 AM12/9/13
to
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

Jiri Olsa

unread,
Dec 9, 2013, 5:50:02 AM12/9/13
to
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

Jiri Olsa

unread,
Dec 9, 2013, 6:10:02 AM12/9/13
to
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 <namh...@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

Steven Rostedt

unread,
Dec 9, 2013, 11:10:02 AM12/9/13
to
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

Steven Rostedt

unread,
Dec 9, 2013, 11:20:02 AM12/9/13
to
Hmm, how do we tell the caller that there was an error? Do they just
check to see if error_str was changed?
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)

--

Steven Rostedt

unread,
Dec 9, 2013, 11:30:02 AM12/9/13
to
On Mon, 9 Dec 2013 14:34:11 +0900
Namhyung Kim <namh...@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);

Steven Rostedt

unread,
Dec 9, 2013, 11:30:03 AM12/9/13
to
On Mon, 9 Dec 2013 14:33:57 +0900
Namhyung Kim <namh...@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 <ros...@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(-)
>

--

Steven Rostedt

unread,
Dec 9, 2013, 11:30:03 AM12/9/13
to
On Mon, 9 Dec 2013 12:03:50 +0100
Jiri Olsa <jo...@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 <namh...@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

Steven Rostedt

unread,
Dec 9, 2013, 11:40:02 AM12/9/13
to
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

Steven Rostedt

unread,
Dec 9, 2013, 11:50:01 AM12/9/13
to
On Mon, 9 Dec 2013 11:47:18 +0100
Jiri Olsa <jo...@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

Arnaldo Carvalho de Melo

unread,
Dec 9, 2013, 1:40:02 PM12/9/13
to
Can *error_str point to either malloc'ed or constant strings? Who
releases the allocated memory?

- Arnaldo

Arnaldo Carvalho de Melo

unread,
Dec 9, 2013, 1:50:01 PM12/9/13
to
Em Mon, Dec 09, 2013 at 11:24:49AM -0500, Steven Rostedt escreveu:
> On Mon, 9 Dec 2013 14:33:57 +0900 Namhyung Kim <namh...@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 <ros...@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

Steven Rostedt

unread,
Dec 9, 2013, 2:10:01 PM12/9/13
to
On Mon, 9 Dec 2013 15:30:09 -0300
Arnaldo Carvalho de Melo <ac...@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

Arnaldo Carvalho de Melo

unread,
Dec 9, 2013, 2:20:02 PM12/9/13
to
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 <ac...@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

Steven Rostedt

unread,
Dec 9, 2013, 2:30:02 PM12/9/13
to
On Mon, 9 Dec 2013 16:14:39 -0300
Arnaldo Carvalho de Melo <ac...@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 <ac...@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

Namhyung Kim

unread,
Dec 9, 2013, 6:50:02 PM12/9/13
to
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 <namh...@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

Namhyung Kim

unread,
Dec 9, 2013, 7:40:02 PM12/9/13
to
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 <namh...@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 <ros...@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

Namhyung Kim

unread,
Dec 9, 2013, 7:40:02 PM12/9/13
to
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

Namhyung Kim

unread,
Dec 9, 2013, 7:50:02 PM12/9/13
to
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 <namh...@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

Namhyung Kim

unread,
Dec 9, 2013, 8:30:03 PM12/9/13
to
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

Namhyung Kim

unread,
Dec 9, 2013, 8:50:02 PM12/9/13
to
On Mon, 9 Dec 2013 11:19:36 -0500, Steven Rostedt wrote:
> On Mon, 9 Dec 2013 14:34:10 +0900
> Namhyung Kim <namh...@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]
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

Namhyung Kim

unread,
Dec 9, 2013, 9:00:02 PM12/9/13
to
On Mon, 9 Dec 2013 11:24:10 -0500, Steven Rostedt wrote:
> On Mon, 9 Dec 2013 14:34:11 +0900
> Namhyung Kim <namh...@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

Namhyung Kim

unread,
Dec 9, 2013, 9:10:02 PM12/9/13
to
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

Steven Rostedt

unread,
Dec 9, 2013, 9:10:02 PM12/9/13
to
On Tue, 10 Dec 2013 10:21:48 +0900
Namhyung Kim <namh...@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

Steven Rostedt

unread,
Dec 9, 2013, 9:20:01 PM12/9/13
to
On Tue, 10 Dec 2013 11:03:44 +0900
Namhyung Kim <namh...@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

Namhyung Kim

unread,
Dec 10, 2013, 12:10:01 AM12/10/13
to
On Mon, 9 Dec 2013 21:14:10 -0500, Steven Rostedt wrote:
> On Tue, 10 Dec 2013 11:03:44 +0900
> Namhyung Kim <namh...@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

Namhyung Kim

unread,
Dec 10, 2013, 12:40:03 AM12/10/13
to
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 <namh...@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

Namhyung Kim

unread,
Dec 10, 2013, 7:50:01 PM12/10/13
to
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 <namh...@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.

Steven Rostedt

unread,
Dec 10, 2013, 9:00:03 PM12/10/13
to
On Wed, 11 Dec 2013 09:40:35 +0900
Namhyung Kim <namh...@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

Namhyung Kim

unread,
Dec 10, 2013, 9:30:01 PM12/10/13
to
On Tue, 10 Dec 2013 20:55:26 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2013 09:40:35 +0900
> Namhyung Kim <namh...@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

tip-bot for Namhyung Kim

unread,
Dec 11, 2013, 6:10:02 AM12/11/13
to
Commit-ID: 4f24416331e9a507e953e90d4534e9a9802cbc12
Gitweb: http://git.kernel.org/tip/4f24416331e9a507e953e90d4534e9a9802cbc12
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:00 +0900
Committer: Arnaldo Carvalho de Melo <ac...@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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-4-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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);

tip-bot for Namhyung Kim

unread,
Dec 11, 2013, 6:10:03 AM12/11/13
to
Commit-ID: 7ef2e813476273ac9c9138f002d8f4cb28e5adad
Gitweb: http://git.kernel.org/tip/7ef2e813476273ac9c9138f002d8f4cb28e5adad
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:09 +0900
Committer: Arnaldo Carvalho de Melo <ac...@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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-13-g...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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;
}

/**

tip-bot for Namhyung Kim

unread,
Dec 11, 2013, 6:10:03 AM12/11/13
to
Commit-ID: 2036fcd1c7ce455424c11bdb1c8a2ac906430e2f
Gitweb: http://git.kernel.org/tip/2036fcd1c7ce455424c11bdb1c8a2ac906430e2f
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:05 +0900
Committer: Arnaldo Carvalho de Melo <ac...@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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-9-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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 */

tip-bot for Namhyung Kim

unread,
Dec 11, 2013, 6:10:05 AM12/11/13
to
Commit-ID: 28942c87e5e907f591d77547203e86ad1089b499
Gitweb: http://git.kernel.org/tip/28942c87e5e907f591d77547203e86ad1089b499
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:08 +0900
Committer: Arnaldo Carvalho de Melo <ac...@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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-12-g...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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;

tip-bot for Namhyung Kim

unread,
Dec 11, 2013, 6:20:02 AM12/11/13
to
Commit-ID: 234520d3fbe43ef72268c4959f85ae326459378c
Gitweb: http://git.kernel.org/tip/234520d3fbe43ef72268c4959f85ae326459378c
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Mon, 9 Dec 2013 14:34:04 +0900
Committer: Arnaldo Carvalho de Melo <ac...@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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-8-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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;

Steven Rostedt

unread,
Dec 11, 2013, 8:20:02 PM12/11/13
to
On Wed, 11 Dec 2013 11:29:21 +0900
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

Namhyung Kim

unread,
Dec 12, 2013, 2:40:01 AM12/12/13
to
The realloc() should check return value and not to overwrite previous
pointer in case of error.

Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Signed-off-by: Namhyung Kim <namh...@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

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
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(-)

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
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 <namh...@kernel.org>
---
tools/lib/traceevent/parse-filter.c | 58 +++++++++++++++++++++++--------------
1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9303c55128db..32ab4396653c 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1361,8 +1361,10 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
if (ret >= 0 && pevent->test_filters) {
char *test;
test = pevent_filter_make_string(filter, event->event->id);
- printf(" '%s: %s'\n", event->event->name, test);
- free(test);
+ if (test) {
+ printf(" '%s: %s'\n", event->event->name, test);
+ free(test);
+ }
}
}

@@ -2097,7 +2099,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
@@ -2120,7 +2124,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;
@@ -2138,7 +2144,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
@@ -2146,8 +2154,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:
@@ -2163,9 +2172,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;
}
@@ -2220,12 +2229,14 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
op = "^";
break;
default:
- die("oops in exp");
+ op = "[ERROR IN EXPRESSION TYPE]";
+ 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);
@@ -2271,9 +2282,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:
@@ -2311,10 +2322,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:
@@ -2330,7 +2342,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
@@ -2369,7 +2383,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)

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
Make it return pevent_errno to distinguish malloc allocation failure.
Since it'll be returned to user later, add more error code.

Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 4 +++-
tools/lib/traceevent/parse-filter.c | 27 +++++++++++++++++++--------
2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 6e23f197175f..abdfd3c606ed 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -356,7 +356,9 @@ enum pevent_flag {
_PE(READ_FORMAT_FAILED, "failed to read event format"), \
_PE(READ_PRINT_FAILED, "failed to read event print fmt"), \
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
- _PE(INVALID_ARG_TYPE, "invalid argument type")
+ _PE(INVALID_ARG_TYPE, "invalid argument type"), \
+ _PE(INVALID_EVENT_NAME, "invalid event name"), \
+ _PE(EVENT_NOT_FOUND, "No event found")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 246ee81e1f93..a0ab040e8f71 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -287,7 +287,7 @@ static int event_match(struct event_format *event,
!regexec(ereg, event->name, 0, NULL, 0);
}

-static int
+static enum pevent_errno
find_event(struct pevent *pevent, struct event_list **events,
char *sys_name, char *event_name)
{
@@ -306,23 +306,31 @@ 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 PEVENT_ERRNO__MEM_ALLOC_FAILED;
+
sprintf(reg, "^%s$", event_name);

ret = regcomp(&ereg, reg, REG_ICASE|REG_NOSUB);
free(reg);

if (ret)
- return -1;
+ return PEVENT_ERRNO__INVALID_EVENT_NAME;

if (sys_name) {
- reg = malloc_or_die(strlen(sys_name) + 3);
+ reg = malloc(strlen(sys_name) + 3);
+ if (reg == NULL) {
+ regfree(&ereg);
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
+ }
+
sprintf(reg, "^%s$", sys_name);
ret = regcomp(&sreg, reg, REG_ICASE|REG_NOSUB);
free(reg);
if (ret) {
regfree(&ereg);
- return -1;
+ return PEVENT_ERRNO__INVALID_EVENT_NAME;
}
}

@@ -342,9 +350,9 @@ find_event(struct pevent *pevent, struct event_list **events,
regfree(&sreg);

if (!match)
- return -1;
+ return PEVENT_ERRNO__EVENT_NOT_FOUND;
if (fail)
- return -2;
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;

return 0;
}
@@ -1312,7 +1320,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 == PEVENT_ERRNO__MEM_ALLOC_FAILED)
+ 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);

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
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 err to save error info during the test and also pass it
to internal test functions.

The return value of pevent_filter_match() also converted to
pevent_errno to indicate an exact error case.

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

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 089964e56ed4..3ad784f5f647 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -357,6 +357,8 @@ enum pevent_flag {
_PE(READ_PRINT_FAILED, "failed to read event print fmt"), \
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
_PE(INVALID_ARG_TYPE, "invalid argument type"), \
+ _PE(INVALID_EXP_TYPE, "invalid expression type"), \
+ _PE(INVALID_OP_TYPE, "invalid operator type"), \
_PE(INVALID_EVENT_NAME, "invalid event name"), \
_PE(EVENT_NOT_FOUND, "no event found"), \
_PE(SYNTAX_ERROR, "syntax error"), \
@@ -373,12 +375,16 @@ enum pevent_flag {
_PE(INVALID_PAREN, "open parenthesis cannot come here"), \
_PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
_PE(UNKNOWN_TOKEN, "unknown token"), \
- _PE(FILTER_NOT_FOUND, "no filter found")
+ _PE(FILTER_NOT_FOUND, "no filter found"), \
+ _PE(NOT_A_NUMBER, "must have number field"), \
+ _PE(NO_FILTER, "no filters exists"), \
+ _PE(FILTER_MISS, "record does not match to filter")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
enum pevent_errno {
PEVENT_ERRNO__SUCCESS = 0,
+ PEVENT_ERRNO__FILTER_MATCH = PEVENT_ERRNO__SUCCESS,

/*
* Choose an arbitrary negative big number not to clash with standard
@@ -853,10 +859,11 @@ struct event_filter {

struct event_filter *pevent_filter_alloc(struct pevent *pevent);

-#define FILTER_NONE -2
-#define FILTER_NOEXIST -1
-#define FILTER_MISS 0
-#define FILTER_MATCH 1
+/* for backward compatibility */
+#define FILTER_NONE PEVENT_ERRNO__FILTER_NOT_FOUND
+#define FILTER_NOEXIST PEVENT_ERRNO__NO_FILTER
+#define FILTER_MISS PEVENT_ERRNO__FILTER_MISS
+#define FILTER_MATCH PEVENT_ERRNO__FILTER_MATCH

enum filter_trivial_type {
FILTER_TRIVIAL_FALSE,
@@ -868,8 +875,8 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
const char *filter_str);


-int pevent_filter_match(struct event_filter *filter,
- struct pevent_record *record);
+enum pevent_errno pevent_filter_match(struct event_filter *filter,
+ struct pevent_record *record);

int pevent_event_filtered(struct event_filter *filter,
int event_id);
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 78440d73e0ad..9303c55128db 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1678,8 +1678,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, enum pevent_errno *err);

static const char *
get_comm(struct event_format *event, struct pevent_record *record)
@@ -1725,15 +1725,24 @@ 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, enum pevent_errno *err);

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, enum pevent_errno *err)
{
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, err);
+ rval = get_arg_value(event, arg->exp.right, record, err);
+
+ if (*err) {
+ /*
+ * There was an error, no need to process anymore.
+ */
+ return 0;
+ }

switch (arg->exp.type) {
case FILTER_EXP_ADD:
@@ -1768,39 +1777,51 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_

case FILTER_EXP_NOT:
default:
- die("error in exp");
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_EXP_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, enum pevent_errno *err)
{
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 (!*err)
+ *err = PEVENT_ERRNO__NOT_A_NUMBER;
+ }
return arg->value.val;

case FILTER_ARG_EXP:
- return get_exp_value(event, arg, record);
+ return get_exp_value(event, arg, record, err);

default:
- die("oops in filter");
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_ARG_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, enum pevent_errno *err)
{
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, err);
+ rval = get_arg_value(event, arg->num.right, record, err);
+
+ if (*err) {
+ /*
+ * There was an error, no need to process anymore.
+ */
+ return 0;
+ }

switch (arg->num.type) {
case FILTER_CMP_EQ:
@@ -1822,7 +1843,8 @@ static int test_num(struct event_format *event,
return lval <= rval;

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__ILLEGAL_INTEGER_CMP;
return 0;
}
}
@@ -1869,8 +1891,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, enum pevent_errno *err)
{
const char *val;

@@ -1894,48 +1916,57 @@ static int test_str(struct event_format *event,
return regexec(&arg->str.reg, val, 0, NULL, 0);

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__ILLEGAL_STRING_CMP;
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, enum pevent_errno *err)
{
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, err) &&
+ test_filter(event, arg->op.right, record, err);

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, err) ||
+ test_filter(event, arg->op.right, record, err);

case FILTER_OP_NOT:
- return !test_filter(event, arg->op.right, record);
+ return !test_filter(event, arg->op.right, record, err);

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_OP_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, enum pevent_errno *err)
{
+ if (*err) {
+ /*
+ * 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, err);

case FILTER_ARG_NUM:
- return test_num(event, arg, record);
+ return test_num(event, arg, record, err);

case FILTER_ARG_STR:
- return test_str(event, arg, record);
+ return test_str(event, arg, record, err);

case FILTER_ARG_EXP:
case FILTER_ARG_VALUE:
@@ -1944,11 +1975,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, err);

default:
- die("oops!");
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_ARG_TYPE;
return 0;
}
}
@@ -1961,8 +1992,7 @@ static int test_filter(struct event_format *event,
* Returns 1 if filter found for @event_id
* otherwise 0;
*/
-int pevent_event_filtered(struct event_filter *filter,
- int event_id)
+int pevent_event_filtered(struct event_filter *filter, int event_id)
{
struct filter_type *filter_type;

@@ -1979,31 +2009,36 @@ int pevent_event_filtered(struct event_filter *filter,
* @filter: filter struct with filter information
* @record: the record to test against the filter
*
- * Returns:
- * 1 - filter found for event and @record matches
- * 0 - filter found for event and @record does not match
- * -1 - no filter found for @record's event
- * -2 - if no filters exist
+ * Returns: match result or error code (prefixed with PEVENT_ERRNO__)
+ * FILTER_MATCH - filter found for event and @record matches
+ * FILTER_MISS - filter found for event and @record does not match
+ * FILTER_NOT_FOUND - no filter found for @record's event
+ * NO_FILTER - if no filters exist
+ * otherwise - error occurred during test
*/
-int pevent_filter_match(struct event_filter *filter,
- struct pevent_record *record)
+enum pevent_errno pevent_filter_match(struct event_filter *filter,
+ struct pevent_record *record)
{
struct pevent *pevent = filter->pevent;
struct filter_type *filter_type;
int event_id;
+ int ret;
+ enum pevent_errno err = 0;

if (!filter->filters)
- return FILTER_NONE;
+ return PEVENT_ERRNO__NO_FILTER;

event_id = pevent_data_type(pevent, record);

filter_type = find_filter_type(filter, event_id);
-
if (!filter_type)
- return FILTER_NOEXIST;
+ return PEVENT_ERRNO__FILTER_NOT_FOUND;
+
+ ret = test_filter(filter_type->event, filter_type->filter, record, &err);
+ if (err)
+ return err;

- return test_filter(filter_type->event, filter_type->filter, record) ?
- FILTER_MATCH : FILTER_MISS;
+ return ret ? PEVENT_ERRNO__FILTER_MATCH : PEVENT_ERRNO__FILTER_MISS;
}

static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Signed-off-by: Namhyung Kim <namh...@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 ab9cefe320b4..246ee81e1f93 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 */
@@ -1123,6 +1127,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;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
Signed-off-by: Namhyung Kim <namh...@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 ab402fb2dcf7..d4b0bac80dc8 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 = NULL;
+ return;
+ }
+ }

if (len) {
strcpy(error, input);

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
From: Namhyung Kim <namhyu...@lge.com>

So that it can return a proper pevent_errno value.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 6 +++-
tools/lib/traceevent/parse-filter.c | 64 +++++++++++++++++++++----------------
2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 57b66aed8122..da942d59cc3a 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -368,7 +368,11 @@ enum pevent_flag {
_PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
_PE(REPARENT_FAILED, "failed to reparent filter OP"), \
_PE(BAD_FILTER_ARG, "bad arg in filter tree"), \
- _PE(UNEXPECTED_TYPE, "unexpected type (not a value)")
+ _PE(UNEXPECTED_TYPE, "unexpected type (not a value)"), \
+ _PE(ILLEGAL_TOKEN, "illegal token"), \
+ _PE(INVALID_PAREN, "open parenthesis cannot come here"), \
+ _PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
+ _PE(UNKNOWN_TOKEN, "unknown token")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 8d71208f0131..5aa5012a17ee 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -937,9 +937,10 @@ static int test_arg(struct filter_arg *parent, struct filter_arg *arg,
}

/* Remove any unknown event fields */
-static struct filter_arg *collapse_tree(struct filter_arg *arg, char **error_str)
+static int collapse_tree(struct filter_arg *arg,
+ struct filter_arg **arg_collapsed, char **error_str)
{
- enum filter_vals ret;
+ int ret;

ret = test_arg(arg, arg, error_str);
switch (ret) {
@@ -955,6 +956,7 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg, char **error_str
arg->boolean.value = ret == FILTER_VAL_TRUE;
} else {
show_error(error_str, "Failed to allocate filter arg");
+ ret = PEVENT_ERRNO__MEM_ALLOC_FAILED;
}
break;

@@ -965,10 +967,11 @@ static struct filter_arg *collapse_tree(struct filter_arg *arg, char **error_str
break;
}

- return arg;
+ *arg_collapsed = arg;
+ return ret;
}

-static int
+static enum pevent_errno
process_filter(struct event_format *event, struct filter_arg **parg,
char **error_str, int not)
{
@@ -982,7 +985,7 @@ process_filter(struct event_format *event, struct filter_arg **parg,
enum filter_op_type btype;
enum filter_exp_type etype;
enum filter_cmp_type ctype;
- int ret;
+ enum pevent_errno ret;

*parg = NULL;

@@ -1007,20 +1010,20 @@ process_filter(struct event_format *event, struct filter_arg **parg,
if (not) {
arg = NULL;
if (current_op)
- goto fail_print;
+ goto fail_syntax;
free(token);
*parg = current_exp;
return 0;
}
} else
- goto fail_print;
+ goto fail_syntax;
arg = NULL;
break;

case EVENT_DELIM:
if (*token == ',') {
- show_error(error_str,
- "Illegal token ','");
+ show_error(error_str, "Illegal token ','");
+ ret = PEVENT_ERRNO__ILLEGAL_TOKEN;
goto fail;
}

@@ -1028,19 +1031,23 @@ process_filter(struct event_format *event, struct filter_arg **parg,
if (left_item) {
show_error(error_str,
"Open paren can not come after item");
+ ret = PEVENT_ERRNO__INVALID_PAREN;
goto fail;
}
if (current_exp) {
show_error(error_str,
"Open paren can not come after expression");
+ ret = PEVENT_ERRNO__INVALID_PAREN;
goto fail;
}

ret = process_filter(event, &arg, error_str, 0);
- if (ret != 1) {
- if (ret == 0)
+ if (ret != PEVENT_ERRNO__UNBALANCED_PAREN) {
+ if (ret == 0) {
show_error(error_str,
"Unbalanced number of '('");
+ ret = PEVENT_ERRNO__UNBALANCED_PAREN;
+ }
goto fail;
}
ret = 0;
@@ -1048,7 +1055,7 @@ process_filter(struct event_format *event, struct filter_arg **parg,
/* A not wants just one expression */
if (not) {
if (current_op)
- goto fail_print;
+ goto fail_syntax;
*parg = arg;
return 0;
}
@@ -1063,19 +1070,19 @@ process_filter(struct event_format *event, struct filter_arg **parg,

} else { /* ')' */
if (!current_op && !current_exp)
- goto fail_print;
+ goto fail_syntax;

/* Make sure everything is finished at this level */
if (current_exp && !check_op_done(current_exp))
- goto fail_print;
+ goto fail_syntax;
if (current_op && !check_op_done(current_op))
- goto fail_print;
+ goto fail_syntax;

if (current_op)
*parg = current_op;
else
*parg = current_exp;
- return 1;
+ return PEVENT_ERRNO__UNBALANCED_PAREN;
}
break;

@@ -1087,21 +1094,22 @@ process_filter(struct event_format *event, struct filter_arg **parg,
case OP_BOOL:
/* Logic ops need a left expression */
if (!current_exp && !current_op)
- goto fail_print;
+ goto fail_syntax;
/* fall through */
case OP_NOT:
/* logic only processes ops and exp */
if (left_item)
- goto fail_print;
+ goto fail_syntax;
break;
case OP_EXP:
case OP_CMP:
if (!left_item)
- goto fail_print;
+ goto fail_syntax;
break;
case OP_NONE:
show_error(error_str,
"Unknown op token %s", token);
+ ret = PEVENT_ERRNO__UNKNOWN_TOKEN;
goto fail;
}

@@ -1152,7 +1160,7 @@ process_filter(struct event_format *event, struct filter_arg **parg,
ret = add_left(arg, left_item);
if (ret < 0) {
arg = NULL;
- goto fail_print;
+ goto fail_syntax;
}
current_exp = arg;
break;
@@ -1161,25 +1169,25 @@ process_filter(struct event_format *event, struct filter_arg **parg,
}
arg = NULL;
if (ret < 0)
- goto fail_print;
+ goto fail_syntax;
break;
case EVENT_NONE:
break;
case EVENT_ERROR:
goto fail_alloc;
default:
- goto fail_print;
+ goto fail_syntax;
}
} while (type != EVENT_NONE);

if (!current_op && !current_exp)
- goto fail_print;
+ goto fail_syntax;

if (!current_op)
current_op = current_exp;

- current_op = collapse_tree(current_op, error_str);
- if (current_op == NULL)
+ ret = collapse_tree(current_op, parg, error_str);
+ if (ret < 0)
goto fail;

*parg = current_op;
@@ -1188,15 +1196,17 @@ process_filter(struct event_format *event, struct filter_arg **parg,

fail_alloc:
show_error(error_str, "failed to allocate filter arg");
+ ret = PEVENT_ERRNO__MEM_ALLOC_FAILED;
goto fail;
- fail_print:
+ fail_syntax:
show_error(error_str, "Syntax error");
+ ret = PEVENT_ERRNO__SYNTAX_ERROR;
fail:
free_arg(current_op);
free_arg(current_exp);
free_arg(arg);
free(token);
- return -1;
+ return ret;
}

static int

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
Refactor it to return appropriate pevent_errno value.

Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 8 +++++++-
tools/lib/traceevent/parse-filter.c | 34 +++++++++++++++++++---------------
2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index abdfd3c606ed..89e4dfd40db6 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -358,7 +358,13 @@ enum pevent_flag {
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
_PE(INVALID_ARG_TYPE, "invalid argument type"), \
_PE(INVALID_EVENT_NAME, "invalid event name"), \
- _PE(EVENT_NOT_FOUND, "No event found")
+ _PE(EVENT_NOT_FOUND, "no event found"), \
+ _PE(SYNTAX_ERROR, "syntax error"), \
+ _PE(ILLEGAL_RVALUE, "illegal rvalue"), \
+ _PE(ILLEGAL_LVALUE, "illegal lvalue for string comparison"), \
+ _PE(INVALID_REGEX, "regex did not compute"), \
+ _PE(ILLEGAL_STRING_CMP, "illegal comparison for string"), \
+ _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index a0ab040e8f71..c08ce594cabe 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -473,8 +473,8 @@ create_arg_cmp(enum filter_exp_type etype)
return arg;
}

-static int add_right(struct filter_arg *op, struct filter_arg *arg,
- char **error_str)
+static enum pevent_errno
+add_right(struct filter_arg *op, struct filter_arg *arg, char **error_str)
{
struct filter_arg *left;
char *str;
@@ -505,9 +505,8 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
case FILTER_ARG_FIELD:
break;
default:
- show_error(error_str,
- "Illegal rvalue");
- return -1;
+ show_error(error_str, "Illegal rvalue");
+ return PEVENT_ERRNO__ILLEGAL_RVALUE;
}

/*
@@ -554,7 +553,7 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
if (left->type != FILTER_ARG_FIELD) {
show_error(error_str,
"Illegal lvalue for string comparison");
- return -1;
+ return PEVENT_ERRNO__ILLEGAL_LVALUE;
}

/* Make sure this is a valid string compare */
@@ -573,25 +572,31 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
show_error(error_str,
"RegEx '%s' did not compute",
str);
- return -1;
+ return PEVENT_ERRNO__INVALID_REGEX;
}
break;
default:
show_error(error_str,
"Illegal comparison for string");
- return -1;
+ return PEVENT_ERRNO__ILLEGAL_STRING_CMP;
}

op->type = FILTER_ARG_STR;
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 PEVENT_ERRNO__MEM_ALLOC_FAILED;
+ }
/*
* 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 PEVENT_ERRNO__MEM_ALLOC_FAILED;
+ }
/* Null terminate this buffer */
op->str.buffer[op->str.field->size] = 0;

@@ -609,7 +614,7 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
case FILTER_CMP_NOT_REGEX:
show_error(error_str,
"Op not allowed with integers");
- return -1;
+ return PEVENT_ERRNO__ILLEGAL_INTEGER_CMP;

default:
break;
@@ -629,9 +634,8 @@ static int add_right(struct filter_arg *op, struct filter_arg *arg,
return 0;

out_fail:
- show_error(error_str,
- "Syntax error");
- return -1;
+ show_error(error_str, "Syntax error");
+ return PEVENT_ERRNO__SYNTAX_ERROR;
}

static struct filter_arg *

Namhyung Kim

unread,
Dec 12, 2013, 2:40:02 AM12/12/13
to
To do that, make the function returns the error code. Also pass
error_str so that it can set proper error message when error occurred.

Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 5 +-
tools/lib/traceevent/parse-filter.c | 94 +++++++++++++++++++++++--------------
2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 89e4dfd40db6..5e4392d8e2d4 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -364,7 +364,10 @@ enum pevent_flag {
_PE(ILLEGAL_LVALUE, "illegal lvalue for string comparison"), \
_PE(INVALID_REGEX, "regex did not compute"), \
_PE(ILLEGAL_STRING_CMP, "illegal comparison for string"), \
- _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer")
+ _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer"), \
+ _PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
+ _PE(REPARENT_FAILED, "failed to reparent filter OP"), \
+ _PE(BAD_FILTER_ARG, "bad arg in filter tree")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 774c3e4c1d9f..9b05892566e0 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -784,15 +784,18 @@ enum filter_vals {
FILTER_VAL_TRUE,
};

-void reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
- struct filter_arg *arg)
+static enum pevent_errno
+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 PEVENT_ERRNO__REPARENT_NOT_OP;
+ }

/* Get the sibling */
if (old_child->op.right == arg) {
@@ -801,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 PEVENT_ERRNO__REPARENT_FAILED;
+ }

/* Detach arg from old_child */
*ptr = NULL;
@@ -813,23 +818,29 @@ void reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
*parent = *arg;
/* Free arg without recussion */
free(arg);
- return;
+ return 0;
}

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 PEVENT_ERRNO__REPARENT_FAILED;
+ }
+
*ptr = arg;

free_arg(old_child);
+ return 0;
}

-enum filter_vals test_arg(struct filter_arg *parent, struct filter_arg *arg)
+/* Returns either filter_vals (success) or pevent_errno (failfure) */
+static int test_arg(struct filter_arg *parent, struct filter_arg *arg,
+ char **error_str)
{
- enum filter_vals lval, rval;
+ int lval, rval;

switch (arg->type) {

@@ -844,63 +855,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);
+
+ default:
+ return lval;
}
}

- rval = test_arg(arg, arg->op.right);
+ rval = test_arg(arg, arg->op.right, error_str);
switch (rval) {
case FILTER_VAL_NORM:
+ default:
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)
@@ -908,26 +924,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 PEVENT_ERRNO__BAD_FILTER_ARG;
}
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:
@@ -936,7 +953,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;
+
+ default:
+ /* test_arg() already set the error_str */
+ free_arg(arg);
+ arg = NULL;
+ break;
}

return arg;
@@ -1152,9 +1178,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;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:03 AM12/12/13
to
From: Namhyung Kim <namhyu...@lge.com>

Refactor the pevent_filter_add_filter_str() to return a proper error
code and get rid of the third error_str argument.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 8 ++--
tools/lib/traceevent/parse-filter.c | 78 +++++++++++--------------------------
2 files changed, 27 insertions(+), 59 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index da942d59cc3a..089964e56ed4 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -372,7 +372,8 @@ enum pevent_flag {
_PE(ILLEGAL_TOKEN, "illegal token"), \
_PE(INVALID_PAREN, "open parenthesis cannot come here"), \
_PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
- _PE(UNKNOWN_TOKEN, "unknown token")
+ _PE(UNKNOWN_TOKEN, "unknown token"), \
+ _PE(FILTER_NOT_FOUND, "no filter found")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
@@ -863,9 +864,8 @@ enum filter_trivial_type {
FILTER_TRIVIAL_BOTH,
};

-int pevent_filter_add_filter_str(struct event_filter *filter,
- const char *filter_str,
- char **error_str);
+enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
+ const char *filter_str);


int pevent_filter_match(struct event_filter *filter,
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 5aa5012a17ee..78440d73e0ad 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1209,7 +1209,7 @@ process_filter(struct event_format *event, struct filter_arg **parg,
return ret;
}

-static int
+static enum pevent_errno
process_event(struct event_format *event, const char *filter_str,
struct filter_arg **parg, char **error_str)
{
@@ -1218,21 +1218,15 @@ process_event(struct event_format *event, const char *filter_str,
pevent_buffer_init(filter_str, strlen(filter_str));

ret = process_filter(event, parg, error_str, 0);
- if (ret == 1) {
- show_error(error_str,
- "Unbalanced number of ')'");
- return -1;
- }
if (ret < 0)
return ret;

/* 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;
- }
+ if (*parg == NULL)
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
+
(*parg)->type = FILTER_ARG_BOOLEAN;
(*parg)->boolean.value = FILTER_FALSE;
}
@@ -1240,13 +1234,13 @@ process_event(struct event_format *event, const char *filter_str,
return 0;
}

-static int filter_event(struct event_filter *filter,
- struct event_format *event,
- const char *filter_str, char **error_str)
+static enum pevent_errno
+filter_event(struct event_filter *filter, struct event_format *event,
+ const char *filter_str, char **error_str)
{
struct filter_type *filter_type;
struct filter_arg *arg;
- int ret;
+ enum pevent_errno ret;

if (filter_str) {
ret = process_event(event, filter_str, &arg, error_str);
@@ -1256,20 +1250,16 @@ 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;
- }
+ if (arg == NULL)
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
+
arg->type = FILTER_ARG_BOOLEAN;
arg->boolean.value = FILTER_TRUE;
}

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 == NULL)
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;

if (filter_type->filter)
free_arg(filter_type->filter);
@@ -1282,18 +1272,12 @@ static int filter_event(struct event_filter *filter,
* pevent_filter_add_filter_str - add a new filter
* @filter: the event filter to add to
* @filter_str: the filter string that contains the filter
- * @error_str: string containing reason for failed filter
- *
- * Returns 0 if the filter was successfully added
- * -1 if there was an error.
*
- * On error, if @error_str points to a string pointer,
- * it is set to the reason that the filter failed.
- * This string must be freed with "free".
+ * Returns 0 if the filter was successfully added or a
+ * negative error code.
*/
-int pevent_filter_add_filter_str(struct event_filter *filter,
- const char *filter_str,
- char **error_str)
+enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
+ const char *filter_str)
{
struct pevent *pevent = filter->pevent;
struct event_list *event;
@@ -1304,23 +1288,20 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
char *event_name = NULL;
char *sys_name = NULL;
char *sp;
- int rtn = 0;
+ enum pevent_errno rtn = 0; /* PEVENT_ERRNO__SUCCESS */
int len;
int ret;
+ char *error_str = NULL;

/* clear buffer to reset show error */
pevent_buffer_init("", 0);

- if (error_str)
- *error_str = NULL;
-
filter_start = strchr(filter_str, ':');
if (filter_start)
len = filter_start - filter_str;
else
len = strlen(filter_str);

-
do {
next_event = strchr(filter_str, ',');
if (next_event &&
@@ -1333,10 +1314,9 @@ int pevent_filter_add_filter_str(struct event_filter *filter,

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;
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}
memcpy(this_event, filter_str, len);
this_event[len] = 0;
@@ -1350,30 +1330,18 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
event_name = strtok_r(NULL, "/", &sp);

if (!sys_name) {
- show_error(error_str, "No filter found");
/* This can only happen when events is NULL, but still */
free_events(events);
free(this_event);
- return -1;
+ return PEVENT_ERRNO__FILTER_NOT_FOUND;
}

/* Find this event */
ret = find_event(pevent, &events, strim(sys_name), strim(event_name));
if (ret < 0) {
- if (ret == PEVENT_ERRNO__MEM_ALLOC_FAILED)
- 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);
- else
- show_error(error_str,
- "No event found under '%s'",
- sys_name);
free_events(events);
free(this_event);
- return -1;
+ return ret;
}
free(this_event);
} while (filter_str);
@@ -1385,7 +1353,7 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
/* filter starts here */
for (event = events; event; event = event->next) {
ret = filter_event(filter, event->event, filter_start,
- error_str);
+ &error_str);
/* Failures are returned if a parse error happened */
if (ret < 0)
rtn = ret;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:03 AM12/12/13
to
Also check return value and handle it.

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

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 767de4f1e8ee..ab9cefe320b4 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)
@@ -369,6 +364,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) {

@@ -422,6 +421,9 @@ create_arg_op(enum filter_op_type btype)
struct filter_arg *arg;

arg = allocate_arg();
+ if (!arg)
+ return NULL;
+
arg->type = FILTER_ARG_OP;
arg->op.type = btype;

@@ -434,6 +436,9 @@ create_arg_exp(enum filter_exp_type etype)
struct filter_arg *arg;

arg = allocate_arg();
+ if (!arg)
+ return NULL;
+
arg->type = FILTER_ARG_EXP;
arg->op.type = etype;

@@ -446,6 +451,9 @@ create_arg_cmp(enum filter_exp_type etype)
struct filter_arg *arg;

arg = allocate_arg();
+ if (!arg)
+ return NULL;
+
/* Use NUM and change if necessary */
arg->type = FILTER_ARG_NUM;
arg->op.type = etype;
@@ -909,8 +917,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;
@@ -1057,6 +1067,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
@@ -1067,6 +1079,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)
@@ -1086,6 +1100,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);
@@ -1119,11 +1135,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:
@@ -1154,6 +1175,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;
}
@@ -1177,6 +1202,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;
}
@@ -1418,6 +1447,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;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:03 AM12/12/13
to
From: Namhyung Kim <namhyu...@lge.com>

So that it can propagate error properly.

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

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index c08ce594cabe..774c3e4c1d9f 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -648,7 +648,7 @@ rotate_op_right(struct filter_arg *a, struct filter_arg *b)
return arg;
}

-static int add_left(struct filter_arg *op, struct filter_arg *arg)
+static enum pevent_errno add_left(struct filter_arg *op, struct filter_arg *arg)
{
switch (op->type) {
case FILTER_ARG_EXP:
@@ -667,11 +667,11 @@ static int add_left(struct filter_arg *op, struct filter_arg *arg)
/* left arg of compares must be a field */
if (arg->type != FILTER_ARG_FIELD &&
arg->type != FILTER_ARG_BOOLEAN)
- return -1;
+ return PEVENT_ERRNO__INVALID_ARG_TYPE;
op->num.left = arg;
break;
default:
- return -1;
+ return PEVENT_ERRNO__INVALID_ARG_TYPE;
}
return 0;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:03 AM12/12/13
to
From: Namhyung Kim <namhyu...@lge.com>

So that it can return a proper pevent_errno value.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.h | 3 ++-
tools/lib/traceevent/parse-filter.c | 20 ++++++++++----------
2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 5e4392d8e2d4..57b66aed8122 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -367,7 +367,8 @@ enum pevent_flag {
_PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer"), \
_PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
_PE(REPARENT_FAILED, "failed to reparent filter OP"), \
- _PE(BAD_FILTER_ARG, "bad arg in filter tree")
+ _PE(BAD_FILTER_ARG, "bad arg in filter tree"), \
+ _PE(UNEXPECTED_TYPE, "unexpected type (not a value)")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9b05892566e0..8d71208f0131 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -368,9 +368,9 @@ static void free_events(struct event_list *events)
}
}

-static struct filter_arg *
+static enum pevent_errno
create_arg_item(struct event_format *event, const char *token,
- enum event_type type, char **error_str)
+ enum event_type type, struct filter_arg **parg, char **error_str)
{
struct format_field *field;
struct filter_arg *arg;
@@ -378,7 +378,7 @@ create_arg_item(struct event_format *event, const char *token,
arg = allocate_arg();
if (arg == NULL) {
show_error(error_str, "failed to allocate filter arg");
- return NULL;
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}

switch (type) {
@@ -392,7 +392,7 @@ create_arg_item(struct event_format *event, const char *token,
if (!arg->value.str) {
free_arg(arg);
show_error(error_str, "failed to allocate string filter arg");
- return NULL;
+ return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}
break;
case EVENT_ITEM:
@@ -420,11 +420,11 @@ create_arg_item(struct event_format *event, const char *token,
break;
default:
free_arg(arg);
- show_error(error_str, "expected a value but found %s",
- token);
- return NULL;
+ show_error(error_str, "expected a value but found %s", token);
+ return PEVENT_ERRNO__UNEXPECTED_TYPE;
}
- return arg;
+ *parg = arg;
+ return 0;
}

static struct filter_arg *
@@ -993,8 +993,8 @@ process_filter(struct event_format *event, struct filter_arg **parg,
case EVENT_SQUOTE:
case EVENT_DQUOTE:
case EVENT_ITEM:
- arg = create_arg_item(event, token, type, error_str);
- if (!arg)
+ ret = create_arg_item(event, token, type, &arg, error_str);
+ if (ret < 0)
goto fail;
if (!left_item)
left_item = arg;

Namhyung Kim

unread,
Dec 12, 2013, 2:40:03 AM12/12/13
to
From: Namhyung Kim <namhyu...@lge.com>

The pevent_filter_strerror() function is for receiving actual error
message from pevent_errno value. To do that, add a static buffer to
event_filter for saving internal error message

If a failed function saved other information in the static buffer
returns the information, otherwise returns generic error message.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
---
tools/lib/traceevent/event-parse.c | 17 +------
tools/lib/traceevent/event-parse.h | 7 ++-
tools/lib/traceevent/parse-filter.c | 98 ++++++++++++++++++++-----------------
3 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 22566c271275..2ce565a73dd5 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -5230,22 +5230,7 @@ int pevent_strerror(struct pevent *pevent __maybe_unused,

idx = errnum - __PEVENT_ERRNO__START - 1;
msg = pevent_error_str[idx];
-
- switch (errnum) {
- case PEVENT_ERRNO__MEM_ALLOC_FAILED:
- case PEVENT_ERRNO__PARSE_EVENT_FAILED:
- case PEVENT_ERRNO__READ_ID_FAILED:
- case PEVENT_ERRNO__READ_FORMAT_FAILED:
- case PEVENT_ERRNO__READ_PRINT_FAILED:
- case PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED:
- case PEVENT_ERRNO__INVALID_ARG_TYPE:
- snprintf(buf, buflen, "%s", msg);
- break;
-
- default:
- /* cannot reach here */
- break;
- }
+ snprintf(buf, buflen, "%s", msg);

return 0;
}
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 3ad784f5f647..cf5db9013f2c 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -851,10 +851,13 @@ struct filter_type {
struct filter_arg *filter;
};

+#define PEVENT_FILTER_ERROR_BUFSZ 1024
+
struct event_filter {
struct pevent *pevent;
int filters;
struct filter_type *event_filters;
+ char error_buffer[PEVENT_FILTER_ERROR_BUFSZ];
};

struct event_filter *pevent_filter_alloc(struct pevent *pevent);
@@ -874,10 +877,12 @@ enum filter_trivial_type {
enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
const char *filter_str);

-
enum pevent_errno pevent_filter_match(struct event_filter *filter,
struct pevent_record *record);

+int pevent_filter_strerror(struct event_filter *filter, enum pevent_errno err,
+ char *buf, size_t buflen);
+
int pevent_event_filtered(struct event_filter *filter,
int event_id);

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 32ab4396653c..c28b1a912a0c 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -38,55 +38,31 @@ struct event_list {
struct event_format *event;
};

-#define MAX_ERR_STR_SIZE 256
-
-static void show_error(char **error_str, const char *fmt, ...)
+static void show_error(char *error_buf, const char *fmt, ...)
{
unsigned long long index;
const char *input;
- char *error;
va_list ap;
int len;
int i;

- if (!error_str)
- return;
-
input = pevent_get_input_buf();
index = pevent_get_input_buf_ptr();
len = input ? strlen(input) : 0;

- 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 = NULL;
- return;
- }
- }
-
if (len) {
- strcpy(error, input);
- error[len] = '\n';
+ strcpy(error_buf, input);
+ error_buf[len] = '\n';
for (i = 1; i < len && i < index; i++)
- error[len+i] = ' ';
- error[len + i] = '^';
- error[len + i + 1] = '\n';
+ error_buf[len+i] = ' ';
+ error_buf[len + i] = '^';
+ error_buf[len + i + 1] = '\n';
len += i+2;
}

va_start(ap, fmt);
- vsnprintf(error + len, MAX_ERR_STR_SIZE, fmt, ap);
+ vsnprintf(error_buf + len, PEVENT_FILTER_ERROR_BUFSZ - len, fmt, ap);
va_end(ap);
-
- *error_str = error;
}

static void free_token(char *token)
@@ -370,7 +346,7 @@ static void free_events(struct event_list *events)

static enum pevent_errno
create_arg_item(struct event_format *event, const char *token,
- enum event_type type, struct filter_arg **parg, char **error_str)
+ enum event_type type, struct filter_arg **parg, char *error_str)
{
struct format_field *field;
struct filter_arg *arg;
@@ -474,7 +450,7 @@ create_arg_cmp(enum filter_exp_type etype)
}

static enum pevent_errno
-add_right(struct filter_arg *op, struct filter_arg *arg, char **error_str)
+add_right(struct filter_arg *op, struct filter_arg *arg, char *error_str)
{
struct filter_arg *left;
char *str;
@@ -786,7 +762,7 @@ enum filter_vals {

static enum pevent_errno
reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,
- struct filter_arg *arg, char **error_str)
+ struct filter_arg *arg, char *error_str)
{
struct filter_arg *other_child;
struct filter_arg **ptr;
@@ -838,7 +814,7 @@ reparent_op_arg(struct filter_arg *parent, struct filter_arg *old_child,

/* Returns either filter_vals (success) or pevent_errno (failfure) */
static int test_arg(struct filter_arg *parent, struct filter_arg *arg,
- char **error_str)
+ char *error_str)
{
int lval, rval;

@@ -938,7 +914,7 @@ static int test_arg(struct filter_arg *parent, struct filter_arg *arg,

/* Remove any unknown event fields */
static int collapse_tree(struct filter_arg *arg,
- struct filter_arg **arg_collapsed, char **error_str)
+ struct filter_arg **arg_collapsed, char *error_str)
{
int ret;

@@ -973,7 +949,7 @@ static int collapse_tree(struct filter_arg *arg,

static enum pevent_errno
process_filter(struct event_format *event, struct filter_arg **parg,
- char **error_str, int not)
+ char *error_str, int not)
{
enum event_type type;
char *token = NULL;
@@ -1211,7 +1187,7 @@ process_filter(struct event_format *event, struct filter_arg **parg,

static enum pevent_errno
process_event(struct event_format *event, const char *filter_str,
- struct filter_arg **parg, char **error_str)
+ struct filter_arg **parg, char *error_str)
{
int ret;

@@ -1236,7 +1212,7 @@ process_event(struct event_format *event, const char *filter_str,

static enum pevent_errno
filter_event(struct event_filter *filter, struct event_format *event,
- const char *filter_str, char **error_str)
+ const char *filter_str, char *error_str)
{
struct filter_type *filter_type;
struct filter_arg *arg;
@@ -1268,13 +1244,21 @@ filter_event(struct event_filter *filter, struct event_format *event,
return 0;
}

+static void filter_init_error_buf(struct event_filter *filter)
+{
+ /* clear buffer to reset show error */
+ pevent_buffer_init("", 0);
+ filter->error_buffer[0] = '\0';
+}
+
/**
* pevent_filter_add_filter_str - add a new filter
* @filter: the event filter to add to
* @filter_str: the filter string that contains the filter
*
* Returns 0 if the filter was successfully added or a
- * negative error code.
+ * negative error code. Use pevent_filter_strerror() to see
+ * actual error message in case of error.
*/
enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
const char *filter_str)
@@ -1291,10 +1275,8 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
enum pevent_errno rtn = 0; /* PEVENT_ERRNO__SUCCESS */
int len;
int ret;
- char *error_str = NULL;

- /* clear buffer to reset show error */
- pevent_buffer_init("", 0);
+ filter_init_error_buf(filter);

filter_start = strchr(filter_str, ':');
if (filter_start)
@@ -1353,7 +1335,7 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
/* filter starts here */
for (event = events; event; event = event->next) {
ret = filter_event(filter, event->event, filter_start,
- &error_str);
+ filter->error_buffer);
/* Failures are returned if a parse error happened */
if (ret < 0)
rtn = ret;
@@ -1382,6 +1364,32 @@ static void free_filter_type(struct filter_type *filter_type)
}

/**
+ * pevent_filter_strerror - fill error message in a buffer
+ * @filter: the event filter contains error
+ * @err: the error code
+ * @buf: the buffer to be filled in
+ * @buflen: the size of the buffer
+ *
+ * Returns 0 if message was filled successfully, -1 if error
+ */
+int pevent_filter_strerror(struct event_filter *filter, enum pevent_errno err,
+ char *buf, size_t buflen)
+{
+ if (err <= __PEVENT_ERRNO__START || err >= __PEVENT_ERRNO__END)
+ return -1;
+
+ if (strlen(filter->error_buffer) > 0) {
+ size_t len = snprintf(buf, buflen, "%s", filter->error_buffer);
+
+ if (len > buflen)
+ return -1;
+ return 0;
+ }
+
+ return pevent_strerror(filter->pevent, err, buf, buflen);
+}
+
+/**
* pevent_filter_remove_event - remove a filter for an event
* @filter: the event filter to remove from
* @event_id: the event to remove a filter for
@@ -2027,6 +2035,8 @@ enum pevent_errno pevent_filter_match(struct event_filter *filter,
int ret;
enum pevent_errno err = 0;

+ filter_init_error_buf(filter);
+
if (!filter->filters)
return PEVENT_ERRNO__NO_FILTER;

Arnaldo Carvalho de Melo

unread,
Dec 12, 2013, 1:50:02 PM12/12/13
to
Why note:

str = NULL;
if (asprintf(&str, "(%s) %s (%s)", left, op, right);
break;

instead?

Ditto for other places here, also there is a typo in the subject line.

I've applied all the previous patches, thanks!

- Arnaldo

Namhyung Kim

unread,
Dec 12, 2013, 7:20:02 PM12/12/13
to
Hi Arnaldo,
Thank you very much!

Here is the updated patch:


From d775295fb53930b1e3d946e145e9a6172773d4f5 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namh...@kernel.org>
Date: Sat, 7 Dec 2013 15:13:35 +0000
Subject: [PATCH v2.1 13/14] tools lib traceevent: Get rid of die() in some string
conversion functions

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 <namh...@kernel.org>
---
tools/lib/traceevent/parse-filter.c | 59 +++++++++++++++----------------------
1 file changed, 24 insertions(+), 35 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9303c55128db..72fe16562546 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1361,8 +1361,10 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
if (ret >= 0 && pevent->test_filters) {
char *test;
test = pevent_filter_make_string(filter, event->event->id);
- printf(" '%s: %s'\n", event->event->name, test);
- free(test);
+ if (test) {
+ printf(" '%s: %s'\n", event->event->name, test);
+ free(test);
+ }
}
}

@@ -2050,7 +2052,6 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
int left_val = -1;
int right_val = -1;
int val;
- int len;

switch (arg->op.type) {
case FILTER_OP_AND:
@@ -2097,7 +2098,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
@@ -2119,10 +2122,7 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
break;
}

- len = strlen(left) + strlen(right) + strlen(op) + 10;
- str = malloc_or_die(len);
- snprintf(str, len, "(%s) %s (%s)",
- left, op, right);
+ asprintf(&str, "(%s) %s (%s)", left, op, right);
break;

case FILTER_OP_NOT:
@@ -2138,16 +2138,16 @@ 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
strcpy(str, "TRUE");
break;
}
- len = strlen(right) + strlen(op) + 3;
- str = malloc_or_die(len);
- snprintf(str, len, "%s(%s)", op, right);
+ asprintf(&str, "%s(%s)", op, right);
break;

default:
@@ -2161,11 +2161,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
{
- char *str;
-
- str = malloc_or_die(30);
+ char *str = NULL;

- snprintf(str, 30, "%lld", arg->value.val);
+ asprintf(&str, "%lld", arg->value.val);

return str;
}
@@ -2181,7 +2179,6 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
char *rstr;
char *op;
char *str = NULL;
- int len;

lstr = arg_to_str(filter, arg->exp.left);
rstr = arg_to_str(filter, arg->exp.right);
@@ -2220,12 +2217,11 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
op = "^";
break;
default:
- die("oops in exp");
+ op = "[ERROR IN EXPRESSION TYPE]";
+ break;
}

- len = strlen(op) + strlen(lstr) + strlen(rstr) + 4;
- str = malloc_or_die(len);
- snprintf(str, len, "%s %s %s", lstr, op, rstr);
+ asprintf(&str, "%s %s %s", lstr, op, rstr);
out:
free(lstr);
free(rstr);
@@ -2239,7 +2235,6 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
char *rstr;
char *str = NULL;
char *op = NULL;
- int len;

lstr = arg_to_str(filter, arg->num.left);
rstr = arg_to_str(filter, arg->num.right);
@@ -2270,10 +2265,7 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "<=";

- len = strlen(lstr) + strlen(op) + strlen(rstr) + 4;
- str = malloc_or_die(len);
- sprintf(str, "%s %s %s", lstr, op, rstr);
-
+ asprintf(&str, "%s %s %s", lstr, op, rstr);
break;

default:
@@ -2291,7 +2283,6 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
{
char *str = NULL;
char *op = NULL;
- int len;

switch (arg->str.type) {
case FILTER_CMP_MATCH:
@@ -2309,12 +2300,8 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "!~";

- 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);
+ asprintf(&str, "%s %s \"%s\"",
+ arg->str.field->name, op, arg->str.val);
break;

default:
@@ -2330,7 +2317,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
@@ -2369,7 +2358,7 @@ static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)

Arnaldo Carvalho de Melo

unread,
Dec 13, 2013, 10:00:02 AM12/13/13
to
Thanks! There are some more questions below:
The malloc here can be combined with the strcpy, and the else clause
gets immediately after:

if (asprintf(&str, "%s", "TRUE") < 0)

> @@ -2119,10 +2122,7 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
> break;
> }
>
> - len = strlen(left) + strlen(right) + strlen(op) + 10;
> - str = malloc_or_die(len);
> - snprintf(str, len, "(%s) %s (%s)",
> - left, op, right);
> + asprintf(&str, "(%s) %s (%s)", left, op, right);
> break;

I was unsure if str was NULL, it is already, at decl site, good :)

All the rest is ok, so its just the malloc + strcpy that remains to be
converted, do you want me to do it?

- Arnaldo

Namhyung Kim

unread,
Dec 15, 2013, 11:50:02 PM12/15/13
to
Hi Arnaldo,

On Fri, 13 Dec 2013 11:52:04 -0300, Arnaldo Carvalho de Melo wrote:
>> - str = malloc_or_die(6);
>> + str = malloc(6);
>> + if (str == NULL)
>> + break;
>> if (val)
>> strcpy(str, "TRUE");
>> else
>
> The malloc here can be combined with the strcpy, and the else clause
> gets immediately after:
>
> if (asprintf(&str, "%s", "TRUE") < 0)
>
>> @@ -2119,10 +2122,7 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
>> break;
>> }
>>
>> - len = strlen(left) + strlen(right) + strlen(op) + 10;
>> - str = malloc_or_die(len);
>> - snprintf(str, len, "(%s) %s (%s)",
>> - left, op, right);
>> + asprintf(&str, "(%s) %s (%s)", left, op, right);
>> break;
>
> I was unsure if str was NULL, it is already, at decl site, good :)
>
> All the rest is ok, so its just the malloc + strcpy that remains to be
> converted, do you want me to do it?

Hmm.. did you mean like this?

str = NULL;
if (val)
asprintf(&str, "TRUE");
else
asprintf(&str, "FALSE");
return str;

Thanks,
Namhyung

Arnaldo Carvalho de Melo

unread,
Dec 16, 2013, 7:50:04 AM12/16/13
to
Em Mon, Dec 16, 2013 at 01:49:11PM +0900, Namhyung Kim escreveu:
> On Fri, 13 Dec 2013 11:52:04 -0300, Arnaldo Carvalho de Melo wrote:
> > All the rest is ok, so its just the malloc + strcpy that remains to be
> > converted, do you want me to do it?

> Hmm.. did you mean like this?

> str = NULL;
> if (val)
> asprintf(&str, "TRUE");
> else
> asprintf(&str, "FALSE");
> return str;

More compact:

if (asprintf(&str, "%s", val ? "TRUE" : "FALSE") < 0)
// error handling path

At that point str already is set to NULL.

- Arnaldo

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:01 AM12/16/13
to
Commit-ID: c8ea690dd0d1385a766d68c51832497181e013b8
Gitweb: http://git.kernel.org/tip/c8ea690dd0d1385a766d68c51832497181e013b8
Author: Namhyung Kim <namhyu...@lge.com>
AuthorDate: Thu, 12 Dec 2013 16:36:12 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

tools lib traceevent: Refactor create_arg_item()

So that it can return a proper pevent_errno value.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-10-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 3 ++-
tools/lib/traceevent/parse-filter.c | 20 ++++++++++----------
2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 5e4392d..57b66ae 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -367,7 +367,8 @@ enum pevent_flag {
_PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer"), \
_PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
_PE(REPARENT_FAILED, "failed to reparent filter OP"), \
- _PE(BAD_FILTER_ARG, "bad arg in filter tree")
+ _PE(BAD_FILTER_ARG, "bad arg in filter tree"), \
+ _PE(UNEXPECTED_TYPE, "unexpected type (not a value)")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9b05892..8d71208 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:01 AM12/16/13
to
Commit-ID: 91dfa49bdd8ef9600d850ef68ec892eb70824e3d
Gitweb: http://git.kernel.org/tip/91dfa49bdd8ef9600d850ef68ec892eb70824e3d
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:07 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:21 -0300

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

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-5-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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 ab9cefe..246ee81 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 */
@@ -1123,6 +1127,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;
}
--

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:01 AM12/16/13
to
Commit-ID: 02d62d6d17b9b718be2878477cdcae95df0d5b4e
Gitweb: http://git.kernel.org/tip/02d62d6d17b9b718be2878477cdcae95df0d5b4e
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:09 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

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

Refactor it to return appropriate pevent_errno value.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-7-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 8 +++++++-
tools/lib/traceevent/parse-filter.c | 34 +++++++++++++++++++---------------
2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index abdfd3c..89e4dfd 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -358,7 +358,13 @@ enum pevent_flag {
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
_PE(INVALID_ARG_TYPE, "invalid argument type"), \
_PE(INVALID_EVENT_NAME, "invalid event name"), \
- _PE(EVENT_NOT_FOUND, "No event found")
+ _PE(EVENT_NOT_FOUND, "no event found"), \
+ _PE(SYNTAX_ERROR, "syntax error"), \
+ _PE(ILLEGAL_RVALUE, "illegal rvalue"), \
+ _PE(ILLEGAL_LVALUE, "illegal lvalue for string comparison"), \
+ _PE(INVALID_REGEX, "regex did not compute"), \
+ _PE(ILLEGAL_STRING_CMP, "illegal comparison for string"), \
+ _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index a0ab040..c08ce59 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:02 AM12/16/13
to
Commit-ID: 69c770a690422c6cdc4ea52d9edbba7c20cd1aff
Gitweb: http://git.kernel.org/tip/69c770a690422c6cdc4ea52d9edbba7c20cd1aff
Author: Namhyung Kim <namhyu...@lge.com>
AuthorDate: Thu, 12 Dec 2013 16:36:14 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

tools lib traceevent: Make pevent_filter_add_filter_str() return pevent_errno

Refactor the pevent_filter_add_filter_str() to return a proper error
code and get rid of the third error_str argument.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-12-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 8 ++--
tools/lib/traceevent/parse-filter.c | 78 +++++++++++--------------------------
2 files changed, 27 insertions(+), 59 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index da942d5..089964e 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -372,7 +372,8 @@ enum pevent_flag {
_PE(ILLEGAL_TOKEN, "illegal token"), \
_PE(INVALID_PAREN, "open parenthesis cannot come here"), \
_PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
- _PE(UNKNOWN_TOKEN, "unknown token")
+ _PE(UNKNOWN_TOKEN, "unknown token"), \
+ _PE(FILTER_NOT_FOUND, "no filter found")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
@@ -863,9 +864,8 @@ enum filter_trivial_type {
FILTER_TRIVIAL_BOTH,
};

-int pevent_filter_add_filter_str(struct event_filter *filter,
- const char *filter_str,
- char **error_str);
+enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
+ const char *filter_str);


int pevent_filter_match(struct event_filter *filter,
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 5aa5012..78440d7 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:02 AM12/16/13
to
Commit-ID: 605b8fda958a578e0a50ed1df3cac5a12f1fe8dc
Gitweb: http://git.kernel.org/tip/605b8fda958a578e0a50ed1df3cac5a12f1fe8dc
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:08 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

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

Make it return pevent_errno to distinguish malloc allocation failure.
Since it'll be returned to user later, add more error code.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-6-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 4 +++-
tools/lib/traceevent/parse-filter.c | 27 +++++++++++++++++++--------
2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 6e23f19..abdfd3c 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -356,7 +356,9 @@ enum pevent_flag {
_PE(READ_FORMAT_FAILED, "failed to read event format"), \
_PE(READ_PRINT_FAILED, "failed to read event print fmt"), \
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
- _PE(INVALID_ARG_TYPE, "invalid argument type")
+ _PE(INVALID_ARG_TYPE, "invalid argument type"), \
+ _PE(INVALID_EVENT_NAME, "invalid event name"), \
+ _PE(EVENT_NOT_FOUND, "No event found")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 246ee81..a0ab040 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:02 AM12/16/13
to
Commit-ID: 9451a2fd78c785445afe0f6966b2043c3ee187ca
Gitweb: http://git.kernel.org/tip/9451a2fd78c785445afe0f6966b2043c3ee187ca
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:04 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:21 -0300

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

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-2-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
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 ab402fb..d4b0bac 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 = NULL;
+ return;
+ }
+ }

if (len) {
strcpy(error, input);
--

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:03 AM12/16/13
to
Commit-ID: ef3072cd1d5c2ea229f7abf8d6475e0c200eeb71
Gitweb: http://git.kernel.org/tip/ef3072cd1d5c2ea229f7abf8d6475e0c200eeb71
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:05 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:21 -0300

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

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

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-3-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
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 d4b0bac..767de4f 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);

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:03 AM12/16/13
to
Commit-ID: 7bb73553e2490ac6667387ee723e0faa61e9d999
Gitweb: http://git.kernel.org/tip/7bb73553e2490ac6667387ee723e0faa61e9d999
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:11 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

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

To do that, 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 <namh...@kernel.org>
Reviewed-by: Steven Rostedt <ros...@goodmis.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-9-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 5 +-
tools/lib/traceevent/parse-filter.c | 94 +++++++++++++++++++++++--------------
2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 89e4dfd..5e4392d 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -364,7 +364,10 @@ enum pevent_flag {
_PE(ILLEGAL_LVALUE, "illegal lvalue for string comparison"), \
_PE(INVALID_REGEX, "regex did not compute"), \
_PE(ILLEGAL_STRING_CMP, "illegal comparison for string"), \
- _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer")
+ _PE(ILLEGAL_INTEGER_CMP,"illegal comparison for integer"), \
+ _PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
+ _PE(REPARENT_FAILED, "failed to reparent filter OP"), \
+ _PE(BAD_FILTER_ARG, "bad arg in filter tree")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 774c3e4..9b05892 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:03 AM12/16/13
to
Commit-ID: 42d6194d133cbaf12f34cbdc4111bd8f7dc0ed2a
Gitweb: http://git.kernel.org/tip/42d6194d133cbaf12f34cbdc4111bd8f7dc0ed2a
Author: Namhyung Kim <namhyu...@lge.com>
AuthorDate: Thu, 12 Dec 2013 16:36:13 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

tools lib traceevent: Refactor process_filter()

So that it can return a proper pevent_errno value.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-11-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 6 +++-
tools/lib/traceevent/parse-filter.c | 64 +++++++++++++++++++++----------------
2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 57b66ae..da942d5 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -368,7 +368,11 @@ enum pevent_flag {
_PE(REPARENT_NOT_OP, "cannot reparent other than OP"), \
_PE(REPARENT_FAILED, "failed to reparent filter OP"), \
_PE(BAD_FILTER_ARG, "bad arg in filter tree"), \
- _PE(UNEXPECTED_TYPE, "unexpected type (not a value)")
+ _PE(UNEXPECTED_TYPE, "unexpected type (not a value)"), \
+ _PE(ILLEGAL_TOKEN, "illegal token"), \
+ _PE(INVALID_PAREN, "open parenthesis cannot come here"), \
+ _PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
+ _PE(UNKNOWN_TOKEN, "unknown token")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 8d71208..5aa5012 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:30:04 AM12/16/13
to
Commit-ID: 2e4eb10d7e59df71ab649343b3f1bff9259da15d
Gitweb: http://git.kernel.org/tip/2e4eb10d7e59df71ab649343b3f1bff9259da15d
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:06 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:21 -0300

tools lib traceevent: Get rid of malloc_or_die() allocate_arg()

Also check return value and handle it.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-4-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/parse-filter.c | 48 ++++++++++++++++++++++++++++++-------
1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 767de4f..ab9cefe 100644

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:40:02 AM12/16/13
to
Commit-ID: 41e12e580a7b0c151199f927193548b84d3e874c
Gitweb: http://git.kernel.org/tip/41e12e580a7b0c151199f927193548b84d3e874c
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Thu, 12 Dec 2013 16:36:15 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

tools lib traceevent: Refactor pevent_filter_match() to get rid of die()

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 err to save error info during the test and also pass it to
internal test functions.

The return value of pevent_filter_match() also converted to pevent_errno
to indicate an exact error case.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-13-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.h | 21 ++++--
tools/lib/traceevent/parse-filter.c | 135 +++++++++++++++++++++++-------------
2 files changed, 99 insertions(+), 57 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 089964e..3ad784f 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -357,6 +357,8 @@ enum pevent_flag {
_PE(READ_PRINT_FAILED, "failed to read event print fmt"), \
_PE(OLD_FTRACE_ARG_FAILED,"failed to allocate field name for ftrace"),\
_PE(INVALID_ARG_TYPE, "invalid argument type"), \
+ _PE(INVALID_EXP_TYPE, "invalid expression type"), \
+ _PE(INVALID_OP_TYPE, "invalid operator type"), \
_PE(INVALID_EVENT_NAME, "invalid event name"), \
_PE(EVENT_NOT_FOUND, "no event found"), \
_PE(SYNTAX_ERROR, "syntax error"), \
@@ -373,12 +375,16 @@ enum pevent_flag {
_PE(INVALID_PAREN, "open parenthesis cannot come here"), \
_PE(UNBALANCED_PAREN, "unbalanced number of parenthesis"), \
_PE(UNKNOWN_TOKEN, "unknown token"), \
- _PE(FILTER_NOT_FOUND, "no filter found")
+ _PE(FILTER_NOT_FOUND, "no filter found"), \
+ _PE(NOT_A_NUMBER, "must have number field"), \
+ _PE(NO_FILTER, "no filters exists"), \
+ _PE(FILTER_MISS, "record does not match to filter")

#undef _PE
#define _PE(__code, __str) PEVENT_ERRNO__ ## __code
enum pevent_errno {
PEVENT_ERRNO__SUCCESS = 0,
+ PEVENT_ERRNO__FILTER_MATCH = PEVENT_ERRNO__SUCCESS,

/*
* Choose an arbitrary negative big number not to clash with standard
@@ -853,10 +859,11 @@ struct event_filter {

struct event_filter *pevent_filter_alloc(struct pevent *pevent);

-#define FILTER_NONE -2
-#define FILTER_NOEXIST -1
-#define FILTER_MISS 0
-#define FILTER_MATCH 1
+/* for backward compatibility */
+#define FILTER_NONE PEVENT_ERRNO__FILTER_NOT_FOUND
+#define FILTER_NOEXIST PEVENT_ERRNO__NO_FILTER
+#define FILTER_MISS PEVENT_ERRNO__FILTER_MISS
+#define FILTER_MATCH PEVENT_ERRNO__FILTER_MATCH

enum filter_trivial_type {
FILTER_TRIVIAL_FALSE,
@@ -868,8 +875,8 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
const char *filter_str);


-int pevent_filter_match(struct event_filter *filter,
- struct pevent_record *record);
+enum pevent_errno pevent_filter_match(struct event_filter *filter,
+ struct pevent_record *record);

int pevent_event_filtered(struct event_filter *filter,
int event_id);
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 78440d7..9303c55 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1678,8 +1678,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, enum pevent_errno *err);

static const char *
get_comm(struct event_format *event, struct pevent_record *record)
@@ -1725,15 +1725,24 @@ 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, enum pevent_errno *err);

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, enum pevent_errno *err)
{
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, err);
+ rval = get_arg_value(event, arg->exp.right, record, err);
+
+ if (*err) {
+ /*
+ * There was an error, no need to process anymore.
+ */
+ return 0;
+ }

switch (arg->exp.type) {
case FILTER_EXP_ADD:
@@ -1768,39 +1777,51 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_

case FILTER_EXP_NOT:
default:
- die("error in exp");
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_EXP_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, enum pevent_errno *err)
{
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 (!*err)
+ *err = PEVENT_ERRNO__NOT_A_NUMBER;
+ }
return arg->value.val;

case FILTER_ARG_EXP:
- return get_exp_value(event, arg, record);
+ return get_exp_value(event, arg, record, err);

default:
- die("oops in filter");
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_ARG_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, enum pevent_errno *err)
{
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, err);
+ rval = get_arg_value(event, arg->num.right, record, err);
+
+ if (*err) {
+ /*
+ * There was an error, no need to process anymore.
+ */
+ return 0;
+ }

switch (arg->num.type) {
case FILTER_CMP_EQ:
@@ -1822,7 +1843,8 @@ static int test_num(struct event_format *event,
return lval <= rval;

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__ILLEGAL_INTEGER_CMP;
return 0;
}
}
@@ -1869,8 +1891,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, enum pevent_errno *err)
{
const char *val;

@@ -1894,48 +1916,57 @@ static int test_str(struct event_format *event,
return regexec(&arg->str.reg, val, 0, NULL, 0);

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__ILLEGAL_STRING_CMP;
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, enum pevent_errno *err)
{
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, err) &&
+ test_filter(event, arg->op.right, record, err);

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, err) ||
+ test_filter(event, arg->op.right, record, err);

case FILTER_OP_NOT:
- return !test_filter(event, arg->op.right, record);
+ return !test_filter(event, arg->op.right, record, err);

default:
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_OP_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, enum pevent_errno *err)
{
+ if (*err) {
+ /*
+ * 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, err);

case FILTER_ARG_NUM:
- return test_num(event, arg, record);
+ return test_num(event, arg, record, err);

case FILTER_ARG_STR:
- return test_str(event, arg, record);
+ return test_str(event, arg, record, err);

case FILTER_ARG_EXP:
case FILTER_ARG_VALUE:
@@ -1944,11 +1975,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, err);

default:
- die("oops!");
- /* ?? */
+ if (!*err)
+ *err = PEVENT_ERRNO__INVALID_ARG_TYPE;
return 0;
}
}
@@ -1961,8 +1992,7 @@ static int test_filter(struct event_format *event,
* Returns 1 if filter found for @event_id
* otherwise 0;
*/
-int pevent_event_filtered(struct event_filter *filter,
- int event_id)
+int pevent_event_filtered(struct event_filter *filter, int event_id)
{
struct filter_type *filter_type;

@@ -1979,31 +2009,36 @@ int pevent_event_filtered(struct event_filter *filter,
* @filter: filter struct with filter information
* @record: the record to test against the filter
*
- * Returns:
- * 1 - filter found for event and @record matches
- * 0 - filter found for event and @record does not match
- * -1 - no filter found for @record's event
- * -2 - if no filters exist
+ * Returns: match result or error code (prefixed with PEVENT_ERRNO__)
+ * FILTER_MATCH - filter found for event and @record matches
+ * FILTER_MISS - filter found for event and @record does not match
+ * FILTER_NOT_FOUND - no filter found for @record's event
+ * NO_FILTER - if no filters exist
+ * otherwise - error occurred during test
*/
-int pevent_filter_match(struct event_filter *filter,
- struct pevent_record *record)
+enum pevent_errno pevent_filter_match(struct event_filter *filter,
+ struct pevent_record *record)
{
struct pevent *pevent = filter->pevent;
struct filter_type *filter_type;
int event_id;
+ int ret;
+ enum pevent_errno err = 0;

if (!filter->filters)
- return FILTER_NONE;
+ return PEVENT_ERRNO__NO_FILTER;

event_id = pevent_data_type(pevent, record);

filter_type = find_filter_type(filter, event_id);
-
if (!filter_type)
- return FILTER_NOEXIST;
+ return PEVENT_ERRNO__FILTER_NOT_FOUND;
+
+ ret = test_filter(filter_type->event, filter_type->filter, record, &err);
+ if (err)
+ return err;

- return test_filter(filter_type->event, filter_type->filter, record) ?
- FILTER_MATCH : FILTER_MISS;
+ return ret ? PEVENT_ERRNO__FILTER_MATCH : PEVENT_ERRNO__FILTER_MISS;
}

static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

tip-bot for Namhyung Kim

unread,
Dec 16, 2013, 10:40:02 AM12/16/13
to
Commit-ID: ff533fc058975579dffbb62a731f63911ae714be
Gitweb: http://git.kernel.org/tip/ff533fc058975579dffbb62a731f63911ae714be
Author: Namhyung Kim <namhyu...@lge.com>
AuthorDate: Thu, 12 Dec 2013 16:36:10 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:22 -0300

tools lib traceevent: Make add_left() return pevent_errno

So that it can propagate error properly.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-8-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/parse-filter.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index c08ce59..774c3e4 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -648,7 +648,7 @@ rotate_op_right(struct filter_arg *a, struct filter_arg *b)
return arg;
}

-static int add_left(struct filter_arg *op, struct filter_arg *arg)
+static enum pevent_errno add_left(struct filter_arg *op, struct filter_arg *arg)
{
switch (op->type) {
case FILTER_ARG_EXP:
@@ -667,11 +667,11 @@ static int add_left(struct filter_arg *op, struct filter_arg *arg)
/* left arg of compares must be a field */
if (arg->type != FILTER_ARG_FIELD &&
arg->type != FILTER_ARG_BOOLEAN)
- return -1;
+ return PEVENT_ERRNO__INVALID_ARG_TYPE;
op->num.left = arg;
break;
default:
- return -1;
+ return PEVENT_ERRNO__INVALID_ARG_TYPE;
}
return 0;
}
--

Namhyung Kim

unread,
Dec 16, 2013, 7:10:02 PM12/16/13
to
Hi Arnaldo,

On Mon, 16 Dec 2013 09:40:51 -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 16, 2013 at 01:49:11PM +0900, Namhyung Kim escreveu:
>> On Fri, 13 Dec 2013 11:52:04 -0300, Arnaldo Carvalho de Melo wrote:
>> > All the rest is ok, so its just the malloc + strcpy that remains to be
>> > converted, do you want me to do it?
>
>> Hmm.. did you mean like this?
>
>> str = NULL;
>> if (val)
>> asprintf(&str, "TRUE");
>> else
>> asprintf(&str, "FALSE");
>> return str;
>
> More compact:
>
> if (asprintf(&str, "%s", val ? "TRUE" : "FALSE") < 0)
> // error handling path
>
> At that point str already is set to NULL.

Okay, this is a new one:


From 56f95f2341161ff2a9155d36605a74623a4ce663 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namh...@kernel.org>
Date: Sat, 7 Dec 2013 15:13:35 +0000
Subject: [PATCH v2.2 13/14] tools lib traceevent: Get rid of die() in some string
conversion functions

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 <namh...@kernel.org>
---
tools/lib/traceevent/parse-filter.c | 67 +++++++++++--------------------------
1 file changed, 19 insertions(+), 48 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9303c55128db..e2842b926759 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1361,8 +1361,10 @@ enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
if (ret >= 0 && pevent->test_filters) {
char *test;
test = pevent_filter_make_string(filter, event->event->id);
- printf(" '%s: %s'\n", event->event->name, test);
- free(test);
+ if (test) {
+ printf(" '%s: %s'\n", event->event->name, test);
+ free(test);
+ }
}
}

@@ -2050,7 +2052,6 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
int left_val = -1;
int right_val = -1;
int val;
- int len;

switch (arg->op.type) {
case FILTER_OP_AND:
@@ -2097,11 +2098,7 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
default:
break;
}
- str = malloc_or_die(6);
- if (val)
- strcpy(str, "TRUE");
- else
- strcpy(str, "FALSE");
+ asprintf(&str, val ? "TRUE" : "FALSE");
break;
}
}
@@ -2119,10 +2116,7 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
break;
}

- len = strlen(left) + strlen(right) + strlen(op) + 10;
- str = malloc_or_die(len);
- snprintf(str, len, "(%s) %s (%s)",
- left, op, right);
+ asprintf(&str, "(%s) %s (%s)", left, op, right);
break;

case FILTER_OP_NOT:
@@ -2138,16 +2132,10 @@ 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);
- if (right_val)
- strcpy(str, "FALSE");
- else
- strcpy(str, "TRUE");
+ asprintf(&str, right_val ? "FALSE" : "TRUE");
break;
}
- len = strlen(right) + strlen(op) + 3;
- str = malloc_or_die(len);
- snprintf(str, len, "%s(%s)", op, right);
+ asprintf(&str, "%s(%s)", op, right);
break;

default:
@@ -2161,11 +2149,9 @@ static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)

static char *val_to_str(struct event_filter *filter, struct filter_arg *arg)
{
- char *str;
-
- str = malloc_or_die(30);
+ char *str = NULL;

- snprintf(str, 30, "%lld", arg->value.val);
+ asprintf(&str, "%lld", arg->value.val);

return str;
}
@@ -2181,7 +2167,6 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
char *rstr;
char *op;
char *str = NULL;
- int len;

lstr = arg_to_str(filter, arg->exp.left);
rstr = arg_to_str(filter, arg->exp.right);
@@ -2220,12 +2205,11 @@ static char *exp_to_str(struct event_filter *filter, struct filter_arg *arg)
op = "^";
break;
default:
- die("oops in exp");
+ op = "[ERROR IN EXPRESSION TYPE]";
+ break;
}

- len = strlen(op) + strlen(lstr) + strlen(rstr) + 4;
- str = malloc_or_die(len);
- snprintf(str, len, "%s %s %s", lstr, op, rstr);
+ asprintf(&str, "%s %s %s", lstr, op, rstr);
out:
free(lstr);
free(rstr);
@@ -2239,7 +2223,6 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
char *rstr;
char *str = NULL;
char *op = NULL;
- int len;

lstr = arg_to_str(filter, arg->num.left);
rstr = arg_to_str(filter, arg->num.right);
@@ -2270,10 +2253,7 @@ static char *num_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "<=";

- len = strlen(lstr) + strlen(op) + strlen(rstr) + 4;
- str = malloc_or_die(len);
- sprintf(str, "%s %s %s", lstr, op, rstr);
-
+ asprintf(&str, "%s %s %s", lstr, op, rstr);
break;

default:
@@ -2291,7 +2271,6 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
{
char *str = NULL;
char *op = NULL;
- int len;

switch (arg->str.type) {
case FILTER_CMP_MATCH:
@@ -2309,12 +2288,8 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)
if (!op)
op = "!~";

- 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);
+ asprintf(&str, "%s %s \"%s\"",
+ arg->str.field->name, op, arg->str.val);
break;

default:
@@ -2326,15 +2301,11 @@ static char *str_to_str(struct event_filter *filter, struct filter_arg *arg)

static char *arg_to_str(struct event_filter *filter, struct filter_arg *arg)
{
- char *str;
+ char *str = NULL;

switch (arg->type) {
case FILTER_ARG_BOOLEAN:
- str = malloc_or_die(6);
- if (arg->boolean.value)
- strcpy(str, "TRUE");
- else
- strcpy(str, "FALSE");
+ asprintf(&str, arg->boolean.value ? "TRUE" : "FALSE");
return str;

case FILTER_ARG_OP:
@@ -2369,7 +2340,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

Arnaldo Carvalho de Melo

unread,
Dec 17, 2013, 3:10:02 PM12/17/13
to
Em Tue, Dec 17, 2013 at 09:02:36AM +0900, Namhyung Kim escreveu:
> On Mon, 16 Dec 2013 09:40:51 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Dec 16, 2013 at 01:49:11PM +0900, Namhyung Kim escreveu:
> >> On Fri, 13 Dec 2013 11:52:04 -0300, Arnaldo Carvalho de Melo wrote:
> >> > All the rest is ok, so its just the malloc + strcpy that remains to be
> >> > converted, do you want me to do it?

> >> Hmm.. did you mean like this?

> >> str = NULL;
> >> if (val)
> >> asprintf(&str, "TRUE");
> >> else
> >> asprintf(&str, "FALSE");
> >> return str;

> > More compact:

> > if (asprintf(&str, "%s", val ? "TRUE" : "FALSE") < 0)
> > // error handling path

> > At that point str already is set to NULL.

> Okay, this is a new one:

Thanks, it all seems now, but just prior to applying this I noticed:

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

It handles NULL in what way? This comment:

> @@ -2369,7 +2340,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)

Made me a bit unconfortable, so if it handles NULL as a filter not
found, how will it figure out what happened?

/me looks at the callers...

From just a quick look I couldn't see cases where NULL could cause
segfaults, but saw some cases where allocation errors would not be
notified in any way to the user :-\

Anyway, applying this patch, those are other kinds of problems, i.e. further
fallout from converting from the previous panic()-at-alloc-failure approach.

- Arnaldo

Namhyung Kim

unread,
Dec 17, 2013, 11:10:02 PM12/17/13
to
Hi Arnaldo,
Right. I just wanted to keep the existing interface as long as possible.

>
> Anyway, applying this patch, those are other kinds of problems, i.e. further
> fallout from converting from the previous panic()-at-alloc-failure approach.

Thanks! But there's one more patch (14/14) left from the series.
Please also consider merging it too. :)

Thanks,
Namhyung

tip-bot for Namhyung Kim

unread,
Dec 18, 2013, 5:40:04 AM12/18/13
to
Commit-ID: f23b24f1bf90b56cfaeb2a1c9b77c46efe8916a6
Gitweb: http://git.kernel.org/tip/f23b24f1bf90b56cfaeb2a1c9b77c46efe8916a6
Author: Namhyung Kim <namh...@kernel.org>
AuthorDate: Tue, 17 Dec 2013 09:02:36 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Tue, 17 Dec 2013 16:51:49 -0300

tools lib traceevent: Get rid of die() in some string conversion functions

Those functions 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 <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Namhyung Kim <namhyu...@lge.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/878uvkg...@sejong.aot.lge.com
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/parse-filter.c | 67 +++++++++++--------------------------
1 file changed, 19 insertions(+), 48 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 9303c55..e2842b9 100644

tip-bot for Namhyung Kim

unread,
Jan 12, 2014, 1:40:04 PM1/12/14
to
Commit-ID: bf19b82e7cf033319525a9eab12216b59c41c519
Gitweb: http://git.kernel.org/tip/bf19b82e7cf033319525a9eab12216b59c41c519
Author: Namhyung Kim <namhyu...@lge.com>
AuthorDate: Thu, 12 Dec 2013 16:36:17 +0900
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitDate: Wed, 18 Dec 2013 14:47:58 -0300

tools lib traceevent: Introduce pevent_filter_strerror()

The pevent_filter_strerror() function is for receiving actual error
message from pevent_errno value. To do that, add a static buffer to
event_filter for saving internal error message

If a failed function saved other information in the static buffer
returns the information, otherwise returns generic error message.

Signed-off-by: Namhyung Kim <namh...@kernel.org>
Cc: Frederic Weisbecker <fwei...@gmail.com>
Cc: Ingo Molnar <mi...@kernel.org>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Steven Rostedt <ros...@goodmis.org>
Link: http://lkml.kernel.org/r/1386833777-3790-15-gi...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/traceevent/event-parse.c | 17 +------
tools/lib/traceevent/event-parse.h | 7 ++-
tools/lib/traceevent/parse-filter.c | 98 ++++++++++++++++++++-----------------
3 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 22566c2..2ce565a 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -5230,22 +5230,7 @@ int pevent_strerror(struct pevent *pevent __maybe_unused,

idx = errnum - __PEVENT_ERRNO__START - 1;
msg = pevent_error_str[idx];
-
- switch (errnum) {
- case PEVENT_ERRNO__MEM_ALLOC_FAILED:
- case PEVENT_ERRNO__PARSE_EVENT_FAILED:
- case PEVENT_ERRNO__READ_ID_FAILED:
- case PEVENT_ERRNO__READ_FORMAT_FAILED:
- case PEVENT_ERRNO__READ_PRINT_FAILED:
- case PEVENT_ERRNO__OLD_FTRACE_ARG_FAILED:
- case PEVENT_ERRNO__INVALID_ARG_TYPE:
- snprintf(buf, buflen, "%s", msg);
- break;
-
- default:
- /* cannot reach here */
- break;
- }
+ snprintf(buf, buflen, "%s", msg);

return 0;
}
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 3ad784f..cf5db90 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -851,10 +851,13 @@ struct filter_type {
struct filter_arg *filter;
};

+#define PEVENT_FILTER_ERROR_BUFSZ 1024
+
struct event_filter {
struct pevent *pevent;
int filters;
struct filter_type *event_filters;
+ char error_buffer[PEVENT_FILTER_ERROR_BUFSZ];
};

struct event_filter *pevent_filter_alloc(struct pevent *pevent);
@@ -874,10 +877,12 @@ enum filter_trivial_type {
enum pevent_errno pevent_filter_add_filter_str(struct event_filter *filter,
const char *filter_str);

-
enum pevent_errno pevent_filter_match(struct event_filter *filter,
struct pevent_record *record);

+int pevent_filter_strerror(struct event_filter *filter, enum pevent_errno err,
+ char *buf, size_t buflen);
+
int pevent_event_filtered(struct event_filter *filter,
int event_id);

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index e2842b9..b502344 100644
0 new messages