Re: [PATCH 1/3] tracing: Combine enum and arrays into single macro in filter code

From: Masami Hiramatsu
Date: Mon Mar 12 2018 - 06:31:32 EST


On Fri, 09 Mar 2018 21:34:43 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> Instead of having a separate enum that is the index into another array, like
> a string array, make a single macro that combines them into a single list,
> and then the two can not get out of sync. This makes it easier to add and
> remove items.
>
> The macro trick is:
>
> #define DOGS \
> C( JACK, "Jack Russell") \
> C( ITALIAN, "Italian Greyhound") \
> C( GERMAN, "German Shepherd")
>
> #undef C
> #define C(a, b) a
>
> enum { DOGS };
>
> #undef C
> #define C(a, b) b
>
> static char dogs[] = { DOGS };

Looks good to me, and nice idea :)

Reviewed-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thanks,

>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> kernel/trace/trace_events_filter.c | 112 ++++++++++++++++---------------------
> 1 file changed, 48 insertions(+), 64 deletions(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index c3c6eee1e4df..a2ef393b3bb2 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -33,22 +33,26 @@
> "# Only events with the given fields will be affected.\n" \
> "# If no events are modified, an error message will be displayed here"
>
> -enum filter_op_ids
> -{
> - OP_OR,
> - OP_AND,
> - OP_GLOB,
> - OP_NE,
> - OP_EQ,
> - OP_LT,
> - OP_LE,
> - OP_GT,
> - OP_GE,
> - OP_BAND,
> - OP_NOT,
> - OP_NONE,
> - OP_OPEN_PAREN,
> -};
> +#define OPS \
> + C( OP_OR, "||", 1 ), \
> + C( OP_AND, "&&", 2 ), \
> + C( OP_GLOB, "~", 4 ), \
> + C( OP_NE, "!=", 4 ), \
> + C( OP_EQ, "==", 4 ), \
> + C( OP_LT, "<", 5 ), \
> + C( OP_LE, "<=", 5 ), \
> + C( OP_GT, ">", 5 ), \
> + C( OP_GE, ">=", 5 ), \
> + C( OP_BAND, "&", 6 ), \
> + C( OP_NOT, "!", 6 ), \
> + C( OP_NONE, "OP_NONE", 0 ), \
> + C( OP_OPEN_PAREN, "(", 0 ), \
> + C( OP_MAX, NULL, 0 )
> +
> +#undef C
> +#define C(a, b, c) a
> +
> +enum filter_op_ids { OPS };
>
> struct filter_op {
> int id;
> @@ -56,56 +60,36 @@ struct filter_op {
> int precedence;
> };
>
> -/* Order must be the same as enum filter_op_ids above */
> -static struct filter_op filter_ops[] = {
> - { OP_OR, "||", 1 },
> - { OP_AND, "&&", 2 },
> - { OP_GLOB, "~", 4 },
> - { OP_NE, "!=", 4 },
> - { OP_EQ, "==", 4 },
> - { OP_LT, "<", 5 },
> - { OP_LE, "<=", 5 },
> - { OP_GT, ">", 5 },
> - { OP_GE, ">=", 5 },
> - { OP_BAND, "&", 6 },
> - { OP_NOT, "!", 6 },
> - { OP_NONE, "OP_NONE", 0 },
> - { OP_OPEN_PAREN, "(", 0 },
> -};
> +#undef C
> +#define C(a, b, c) { a, b, c }
>
> -enum {
> - FILT_ERR_NONE,
> - FILT_ERR_INVALID_OP,
> - FILT_ERR_UNBALANCED_PAREN,
> - FILT_ERR_TOO_MANY_OPERANDS,
> - FILT_ERR_OPERAND_TOO_LONG,
> - FILT_ERR_FIELD_NOT_FOUND,
> - FILT_ERR_ILLEGAL_FIELD_OP,
> - FILT_ERR_ILLEGAL_INTVAL,
> - FILT_ERR_BAD_SUBSYS_FILTER,
> - FILT_ERR_TOO_MANY_PREDS,
> - FILT_ERR_MISSING_FIELD,
> - FILT_ERR_INVALID_FILTER,
> - FILT_ERR_IP_FIELD_ONLY,
> - FILT_ERR_ILLEGAL_NOT_OP,
> -};
> +static struct filter_op filter_ops[] = { OPS };
>
> -static char *err_text[] = {
> - "No error",
> - "Invalid operator",
> - "Unbalanced parens",
> - "Too many operands",
> - "Operand too long",
> - "Field not found",
> - "Illegal operation for field type",
> - "Illegal integer value",
> - "Couldn't find or set field in one of a subsystem's events",
> - "Too many terms in predicate expression",
> - "Missing field name and/or value",
> - "Meaningless filter expression",
> - "Only 'ip' field is supported for function trace",
> - "Illegal use of '!'",
> -};
> +#define ERRORS \
> + C( NONE, "No error"), \
> + C( INVALID_OP, "Invalid operator"), \
> + C( UNBALANCED_PAREN, "Unbalanced parens"), \
> + C( TOO_MANY_OPERANDS, "Too many operands"), \
> + C( OPERAND_TOO_LONG, "Operand too long"), \
> + C( FIELD_NOT_FOUND, "Field not found"), \
> + C( ILLEGAL_FIELD_OP, "Illegal operation for field type"), \
> + C( ILLEGAL_INTVAL, "Illegal integer value"), \
> + C( BAD_SUBSYS_FILTER, "Couldn't find or set field in one of a subsystem's events"), \
> + C( TOO_MANY_PREDS, "Too many terms in predicate expression"), \
> + C( MISSING_FIELD, "Missing field name and/or value"), \
> + C( INVALID_FILTER, "Meaningless filter expression"), \
> + C( IP_FIELD_ONLY, "Only 'ip' field is supported for function trace"), \
> + C( ILLEGAL_NOT_OP, "Illegal use of '!'"),
> +
> +#undef C
> +#define C(a, b) FILT_ERR_##a
> +
> +enum { ERRORS };
> +
> +#undef C
> +#define C(a, b) b
> +
> +static char *err_text[] = { ERRORS };
>
> struct opstack_op {
> enum filter_op_ids op;
> --
> 2.15.1
>
>


--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>