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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20180312193124.7d295423e38317fb056e6ccd@kernel.org>
Date:   Mon, 12 Mar 2018 19:31:24 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Al Viro <viro@...IV.linux.org.uk>,
        Tom Zanussi <tom.zanussi@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH 1/3] tracing: Combine enum and arrays into single macro
 in filter code

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

> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
> 
> 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@...nel.org>

Thanks,

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
>  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@...nel.org>