On Mon, 9 Dec 2013 14:34:10 +0900 Namhyung Kim <[email protected]> wrote:
> The test_filter() function is for testing given filter is matched to a > given record. However it doesn't handle error cases properly so add a > new argument error_str to save error info during the test and also > pass it to internal test functions. > > For now, it just save the error but does nothing with it. Maybe it > can be given by user through pevent_filter_match() later. > > Signed-off-by: Namhyung Kim <[email protected]> > --- > tools/lib/traceevent/event-parse.h | 1 + > tools/lib/traceevent/parse-filter.c | 102 > ++++++++++++++++++++++-------------- > 2 files changed, 65 insertions(+), 38 deletions(-) > > diff --git a/tools/lib/traceevent/event-parse.h > b/tools/lib/traceevent/event-parse.h > index 6e23f197175f..a1d8b2792e3a 100644 > --- a/tools/lib/traceevent/event-parse.h > +++ b/tools/lib/traceevent/event-parse.h > @@ -836,6 +836,7 @@ struct event_filter { > > struct event_filter *pevent_filter_alloc(struct pevent *pevent); > > +#define FILTER_ERROR -3 > #define FILTER_NONE -2 > #define FILTER_NOEXIST -1 > #define FILTER_MISS 0 > diff --git a/tools/lib/traceevent/parse-filter.c > b/tools/lib/traceevent/parse-filter.c > index 4d395e8b88bb..8a5b7a74b44e 100644 > --- a/tools/lib/traceevent/parse-filter.c > +++ b/tools/lib/traceevent/parse-filter.c > @@ -1698,8 +1698,8 @@ int pevent_filter_event_has_trivial(struct event_filter > *filter, > } > } > > -static int test_filter(struct event_format *event, > - struct filter_arg *arg, struct pevent_record *record); > +static int test_filter(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str); > > static const char * > get_comm(struct event_format *event, struct pevent_record *record) > @@ -1745,15 +1745,17 @@ get_value(struct event_format *event, > } > > static unsigned long long > -get_arg_value(struct event_format *event, struct filter_arg *arg, struct > pevent_record *record); > +get_arg_value(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str); > > static unsigned long long > -get_exp_value(struct event_format *event, struct filter_arg *arg, struct > pevent_record *record) > +get_exp_value(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > unsigned long long lval, rval; > > - lval = get_arg_value(event, arg->exp.left, record); > - rval = get_arg_value(event, arg->exp.right, record); > + lval = get_arg_value(event, arg->exp.left, record, error_str); > + rval = get_arg_value(event, arg->exp.right, record, error_str); > > switch (arg->exp.type) { > case FILTER_EXP_ADD: > @@ -1788,39 +1790,44 @@ get_exp_value(struct event_format *event, struct > filter_arg *arg, struct pevent_ > > case FILTER_EXP_NOT: > default: > - die("error in exp"); > + if (*error_str == NULL) > + *error_str = "invalid expression type"; Hmm, how do we tell the caller that there was an error? Do they just check to see if error_str was changed? > } > return 0; > } > > static unsigned long long > -get_arg_value(struct event_format *event, struct filter_arg *arg, struct > pevent_record *record) > +get_arg_value(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > switch (arg->type) { > case FILTER_ARG_FIELD: > return get_value(event, arg->field.field, record); > > case FILTER_ARG_VALUE: > - if (arg->value.type != FILTER_NUMBER) > - die("must have number field!"); > + if (arg->value.type != FILTER_NUMBER) { > + if (*error_str == NULL) > + *error_str = "must have number field!"; > + } > return arg->value.val; > > case FILTER_ARG_EXP: > - return get_exp_value(event, arg, record); > + return get_exp_value(event, arg, record, error_str); > > default: > - die("oops in filter"); > + if (*error_str == NULL) > + *error_str = "invalid numeric argument type"; > } > return 0; > } > > -static int test_num(struct event_format *event, > - struct filter_arg *arg, struct pevent_record *record) > +static int test_num(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > unsigned long long lval, rval; > > - lval = get_arg_value(event, arg->num.left, record); > - rval = get_arg_value(event, arg->num.right, record); > + lval = get_arg_value(event, arg->num.left, record, error_str); > + rval = get_arg_value(event, arg->num.right, record, error_str); > > switch (arg->num.type) { > case FILTER_CMP_EQ: > @@ -1842,7 +1849,8 @@ static int test_num(struct event_format *event, > return lval <= rval; > > default: > - /* ?? */ > + if (*error_str == NULL) > + *error_str = "invalid numeric comparison type"; > return 0; > } > } > @@ -1889,8 +1897,8 @@ static const char *get_field_str(struct filter_arg > *arg, struct pevent_record *r > return val; > } > > -static int test_str(struct event_format *event, > - struct filter_arg *arg, struct pevent_record *record) > +static int test_str(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > const char *val; > > @@ -1914,48 +1922,57 @@ static int test_str(struct event_format *event, > return regexec(&arg->str.reg, val, 0, NULL, 0); > > default: > - /* ?? */ > + if (*error_str == NULL) > + *error_str = "invalid string comparison type"; > return 0; > } > } > > -static int test_op(struct event_format *event, > - struct filter_arg *arg, struct pevent_record *record) > +static int test_op(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > switch (arg->op.type) { > case FILTER_OP_AND: > - return test_filter(event, arg->op.left, record) && > - test_filter(event, arg->op.right, record); > + return test_filter(event, arg->op.left, record, error_str) && > + test_filter(event, arg->op.right, record, error_str); > > case FILTER_OP_OR: > - return test_filter(event, arg->op.left, record) || > - test_filter(event, arg->op.right, record); > + return test_filter(event, arg->op.left, record, error_str) || > + test_filter(event, arg->op.right, record, error_str); > > case FILTER_OP_NOT: > - return !test_filter(event, arg->op.right, record); > + return !test_filter(event, arg->op.right, record, error_str); > > default: > - /* ?? */ > + if (*error_str == NULL) > + *error_str = "invalid operator type"; > return 0; > } > } > > -static int test_filter(struct event_format *event, > - struct filter_arg *arg, struct pevent_record *record) > +static int test_filter(struct event_format *event, struct filter_arg *arg, > + struct pevent_record *record, char **error_str) > { > + if (*error_str) { > + /* > + * There was an error, no need to process anymore. > + */ > + return 0; > + } > + > switch (arg->type) { > case FILTER_ARG_BOOLEAN: > /* easy case */ > return arg->boolean.value; > > case FILTER_ARG_OP: > - return test_op(event, arg, record); > + return test_op(event, arg, record, error_str); > > case FILTER_ARG_NUM: > - return test_num(event, arg, record); > + return test_num(event, arg, record, error_str); > > case FILTER_ARG_STR: > - return test_str(event, arg, record); > + return test_str(event, arg, record, error_str); > > case FILTER_ARG_EXP: > case FILTER_ARG_VALUE: > @@ -1964,11 +1981,11 @@ static int test_filter(struct event_format *event, > * Expressions, fields and values evaluate > * to true if they return non zero > */ > - return !!get_arg_value(event, arg, record); > + return !!get_arg_value(event, arg, record, error_str); > > default: > - die("oops!"); > - /* ?? */ > + if (*error_str == NULL) > + *error_str = "invalid argument type"; > return 0; > } > } > @@ -2004,6 +2021,7 @@ int pevent_event_filtered(struct event_filter *filter, > * 0 - filter found for event and @record does not match > * -1 - no filter found for @record's event > * -2 - if no filters exist > + * -3 - if error occurred during test > */ > int pevent_filter_match(struct event_filter *filter, > struct pevent_record *record) > @@ -2011,6 +2029,8 @@ int pevent_filter_match(struct event_filter *filter, > struct pevent *pevent = filter->pevent; > struct filter_type *filter_type; > int event_id; > + char *error_str = NULL; > + int ret; > > if (!filter->filters) > return FILTER_NONE; > @@ -2022,8 +2042,14 @@ int pevent_filter_match(struct event_filter *filter, > if (!filter_type) > return FILTER_NOEXIST; > > - return test_filter(filter_type->event, filter_type->filter, record) ? > - FILTER_MATCH : FILTER_MISS; > + ret = test_filter(filter_type->event, filter_type->filter, record, > + &error_str); > + if (error_str) { > + /* TODO: maybe we can print it or pass back to user */ Ah, I guess this answers my question :-) Maybe we can save the error_str in the pevent. Then we can extract it later. The return of FILTER_ERROR will let the user see what happened. -- Steve > + return FILTER_ERROR; > + } > + > + return ret ? FILTER_MATCH : FILTER_MISS; > } > > static char *op_to_str(struct event_filter *filter, struct filter_arg *arg) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
