| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20131209111936.71a448c5@gandalf.local.home>
Date: Mon, 9 Dec 2013 11:19:36 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
Namhyung Kim <namhyung.kim@....com>
Subject: Re: [PATCH 13/14] tools lib traceevent: Refactor test_filter() to
get rid of die()
On Mon, 9 Dec 2013 14:34:10 +0900
Namhyung Kim <namhyung@...nel.org> wrote:
> The test_filter() function is for testing given filter is matched to a
> given record. However it doesn't handle error cases properly so add a
> new argument error_str to save error info during the test and also
> pass it to internal test functions.
>
> For now, it just save the error but does nothing with it. Maybe it
> can be given by user through pevent_filter_match() later.
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
> tools/lib/traceevent/event-parse.h | 1 +
> tools/lib/traceevent/parse-filter.c | 102 ++++++++++++++++++++++--------------
> 2 files changed, 65 insertions(+), 38 deletions(-)
>
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 6e23f197175f..a1d8b2792e3a 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -836,6 +836,7 @@ struct event_filter {
>
> struct event_filter *pevent_filter_alloc(struct pevent *pevent);
>
> +#define FILTER_ERROR -3
> #define FILTER_NONE -2
> #define FILTER_NOEXIST -1
> #define FILTER_MISS 0
> diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
> index 4d395e8b88bb..8a5b7a74b44e 100644
> --- a/tools/lib/traceevent/parse-filter.c
> +++ b/tools/lib/traceevent/parse-filter.c
> @@ -1698,8 +1698,8 @@ int pevent_filter_event_has_trivial(struct event_filter *filter,
> }
> }
>
> -static int test_filter(struct event_format *event,
> - struct filter_arg *arg, struct pevent_record *record);
> +static int test_filter(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str);
>
> static const char *
> get_comm(struct event_format *event, struct pevent_record *record)
> @@ -1745,15 +1745,17 @@ get_value(struct event_format *event,
> }
>
> static unsigned long long
> -get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record);
> +get_arg_value(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str);
>
> static unsigned long long
> -get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
> +get_exp_value(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> unsigned long long lval, rval;
>
> - lval = get_arg_value(event, arg->exp.left, record);
> - rval = get_arg_value(event, arg->exp.right, record);
> + lval = get_arg_value(event, arg->exp.left, record, error_str);
> + rval = get_arg_value(event, arg->exp.right, record, error_str);
>
> switch (arg->exp.type) {
> case FILTER_EXP_ADD:
> @@ -1788,39 +1790,44 @@ get_exp_value(struct event_format *event, struct filter_arg *arg, struct pevent_
>
> case FILTER_EXP_NOT:
> default:
> - die("error in exp");
> + if (*error_str == NULL)
> + *error_str = "invalid expression type";
Hmm, how do we tell the caller that there was an error? Do they just
check to see if error_str was changed?
> }
> return 0;
> }
>
> static unsigned long long
> -get_arg_value(struct event_format *event, struct filter_arg *arg, struct pevent_record *record)
> +get_arg_value(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> switch (arg->type) {
> case FILTER_ARG_FIELD:
> return get_value(event, arg->field.field, record);
>
> case FILTER_ARG_VALUE:
> - if (arg->value.type != FILTER_NUMBER)
> - die("must have number field!");
> + if (arg->value.type != FILTER_NUMBER) {
> + if (*error_str == NULL)
> + *error_str = "must have number field!";
> + }
> return arg->value.val;
>
> case FILTER_ARG_EXP:
> - return get_exp_value(event, arg, record);
> + return get_exp_value(event, arg, record, error_str);
>
> default:
> - die("oops in filter");
> + if (*error_str == NULL)
> + *error_str = "invalid numeric argument type";
> }
> return 0;
> }
>
> -static int test_num(struct event_format *event,
> - struct filter_arg *arg, struct pevent_record *record)
> +static int test_num(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> unsigned long long lval, rval;
>
> - lval = get_arg_value(event, arg->num.left, record);
> - rval = get_arg_value(event, arg->num.right, record);
> + lval = get_arg_value(event, arg->num.left, record, error_str);
> + rval = get_arg_value(event, arg->num.right, record, error_str);
>
> switch (arg->num.type) {
> case FILTER_CMP_EQ:
> @@ -1842,7 +1849,8 @@ static int test_num(struct event_format *event,
> return lval <= rval;
>
> default:
> - /* ?? */
> + if (*error_str == NULL)
> + *error_str = "invalid numeric comparison type";
> return 0;
> }
> }
> @@ -1889,8 +1897,8 @@ static const char *get_field_str(struct filter_arg *arg, struct pevent_record *r
> return val;
> }
>
> -static int test_str(struct event_format *event,
> - struct filter_arg *arg, struct pevent_record *record)
> +static int test_str(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> const char *val;
>
> @@ -1914,48 +1922,57 @@ static int test_str(struct event_format *event,
> return regexec(&arg->str.reg, val, 0, NULL, 0);
>
> default:
> - /* ?? */
> + if (*error_str == NULL)
> + *error_str = "invalid string comparison type";
> return 0;
> }
> }
>
> -static int test_op(struct event_format *event,
> - struct filter_arg *arg, struct pevent_record *record)
> +static int test_op(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> switch (arg->op.type) {
> case FILTER_OP_AND:
> - return test_filter(event, arg->op.left, record) &&
> - test_filter(event, arg->op.right, record);
> + return test_filter(event, arg->op.left, record, error_str) &&
> + test_filter(event, arg->op.right, record, error_str);
>
> case FILTER_OP_OR:
> - return test_filter(event, arg->op.left, record) ||
> - test_filter(event, arg->op.right, record);
> + return test_filter(event, arg->op.left, record, error_str) ||
> + test_filter(event, arg->op.right, record, error_str);
>
> case FILTER_OP_NOT:
> - return !test_filter(event, arg->op.right, record);
> + return !test_filter(event, arg->op.right, record, error_str);
>
> default:
> - /* ?? */
> + if (*error_str == NULL)
> + *error_str = "invalid operator type";
> return 0;
> }
> }
>
> -static int test_filter(struct event_format *event,
> - struct filter_arg *arg, struct pevent_record *record)
> +static int test_filter(struct event_format *event, struct filter_arg *arg,
> + struct pevent_record *record, char **error_str)
> {
> + if (*error_str) {
> + /*
> + * There was an error, no need to process anymore.
> + */
> + return 0;
> + }
> +
> switch (arg->type) {
> case FILTER_ARG_BOOLEAN:
> /* easy case */
> return arg->boolean.value;
>
> case FILTER_ARG_OP:
> - return test_op(event, arg, record);
> + return test_op(event, arg, record, error_str);
>
> case FILTER_ARG_NUM:
> - return test_num(event, arg, record);
> + return test_num(event, arg, record, error_str);
>
> case FILTER_ARG_STR:
> - return test_str(event, arg, record);
> + return test_str(event, arg, record, error_str);
>
> case FILTER_ARG_EXP:
> case FILTER_ARG_VALUE:
> @@ -1964,11 +1981,11 @@ static int test_filter(struct event_format *event,
> * Expressions, fields and values evaluate
> * to true if they return non zero
> */
> - return !!get_arg_value(event, arg, record);
> + return !!get_arg_value(event, arg, record, error_str);
>
> default:
> - die("oops!");
> - /* ?? */
> + if (*error_str == NULL)
> + *error_str = "invalid argument type";
> return 0;
> }
> }
> @@ -2004,6 +2021,7 @@ int pevent_event_filtered(struct event_filter *filter,
> * 0 - filter found for event and @record does not match
> * -1 - no filter found for @record's event
> * -2 - if no filters exist
> + * -3 - if error occurred during test
> */
> int pevent_filter_match(struct event_filter *filter,
> struct pevent_record *record)
> @@ -2011,6 +2029,8 @@ int pevent_filter_match(struct event_filter *filter,
> struct pevent *pevent = filter->pevent;
> struct filter_type *filter_type;
> int event_id;
> + char *error_str = NULL;
> + int ret;
>
> if (!filter->filters)
> return FILTER_NONE;
> @@ -2022,8 +2042,14 @@ int pevent_filter_match(struct event_filter *filter,
> if (!filter_type)
> return FILTER_NOEXIST;
>
> - return test_filter(filter_type->event, filter_type->filter, record) ?
> - FILTER_MATCH : FILTER_MISS;
> + ret = test_filter(filter_type->event, filter_type->filter, record,
> + &error_str);
> + if (error_str) {
> + /* TODO: maybe we can print it or pass back to user */
Ah, I guess this answers my question :-)
Maybe we can save the error_str in the pevent. Then we can extract it
later. The return of FILTER_ERROR will let the user see what happened.
-- Steve
> + return FILTER_ERROR;
> + }
> +
> + return ret ? FILTER_MATCH : FILTER_MISS;
> }
>
> static char *op_to_str(struct event_filter *filter, struct filter_arg *arg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists