[PATCH v7 fix 5/8] Implement new filtering language parsing

Eugene Syromiatnikov esyr at redhat.com
Mon Aug 14 01:59:24 UTC 2017


On Sat, Aug 12, 2017 at 03:23:24PM +0700, Nikolay Marchuk wrote:
> * basic_filters.c (parse_set, parse_syscall_set): Use set inversion only in
> qualify mode.
> * filter.h (parse_filter_action, parse_qualify_action,
> parse_filter_expression): Add new declarations.
> * filter_action.c (parse_filter_action): Add new parsing function.
> (filtering_parse_finish): Use filtering_parse instead of
> parse_qualify_filter.
> * filter_expression.c (parse_filter_expression): Implement parsing of filter
> expression.
> (add_variable_token, add_operator_token, parse_operator, push_operator,
> is_higher_priority, is_allowed_in_name): Add helper functions.
> (is_space_ascii): Add new definition.
> * filter_parse.c: New file.
> * filter_qualify.c (parse_read, parse_write, qualify_signals, parse_trace,
> parse_abbrev, parse_verbose, parse_raw, parse_inject_common, parse_fault,
> parse_inject): Use new parsing API.
> * strace.c (init): Use filtering_parse instead of parse_qualify_filter.
> * Makefile.am (strace_SOURCES): Add filter_parse.c.
> * defs.h (filtering_parse): Add new definition.
> ---
>  Makefile.am                         |   1 +
>  basic_filters.c                     |  34 +++---
>  defs.h                              |   2 +-
>  filter.h                            |   7 ++
>  filter_action.c                     |  14 ++-
>  filter_expression.c                 | 220 +++++++++++++++++++++++++++++++++++
>  filter_parse.c                      | 223 ++++++++++++++++++++++++++++++++++++
>  filter_qualify.c                    |  99 +++++++++-------
>  strace.c                            |   6 +-
>  tests/filtering_syscall-syntax.test |  17 +--
>  10 files changed, 546 insertions(+), 77 deletions(-)
>  create mode 100644 filter_parse.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 84df39a8..ae10dc5f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -136,6 +136,7 @@ strace_SOURCES =	\
>  	file_ioctl.c	\
>  	filter_action.c	\
>  	filter_expression.c \
> +	filter_parse.c	\
>  	filter_qualify.c \
>  	filter.c	\
>  	filter.h	\
> diff --git a/basic_filters.c b/basic_filters.c
> index ada7b1a5..f12c9046 100644
> --- a/basic_filters.c
> +++ b/basic_filters.c
> @@ -254,15 +254,17 @@ parse_syscall_set(const char *const str, struct number_set *const set)
>  	unsigned int p;
>  	const char *s = str;
>  
> -	/*
> -	 * Each leading ! character means inversion
> -	 * of the remaining specification.
> -	 */
> -	while (*s == '!') {
> -		for (p = 0; p < SUPPORTED_PERSONALITIES; ++p) {
> -			set[p].not = !set[p].not;
> +	if (parsing_qualify_mode) {
It's better to pass such parameters via arguments and not via global
variable.

> +		/*
> +		 * Each leading ! character means inversion
> +		 * of the remaining specification.
> +		 */
> +		while (*s == '!') {
> +			for (p = 0; p < SUPPORTED_PERSONALITIES; ++p) {
> +				set[p].not = !set[p].not;
> +			}
> +			++s;
>  		}
> -		++s;
>  	}
>  
>  	if (strcmp(s, "none") == 0) {
> @@ -345,13 +347,15 @@ parse_set(const char *const str, struct number_set *const set,
>  {
>  	const char *s = str;
>  
> -	/*
> -	 * Each leading ! character means inversion
> -	 * of the remaining specification.
> -	 */
> -	while (*s == '!') {
> -		set->not = !set->not;
> -		++s;
> +	if (parsing_qualify_mode) {
Ditto here.

> +		/*
> +		 * Each leading ! character means inversion
> +		 * of the remaining specification.
> +		 */
> +		while (*s == '!') {
> +			set->not = !set->not;
> +			++s;
> +		}
>  	}
>  
>  	if (strcmp(s, "none") == 0) {
> diff --git a/defs.h b/defs.h
> index 69cd765e..13893f64 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -670,7 +670,7 @@ extern struct number_set signal_set;
>  extern bool is_number_in_set(unsigned int number, const struct number_set *);
>  extern void filtering_parsing_finish(void);
>  extern void filter_syscall(struct tcb *);
> -extern void parse_qualify_filter(const char *);
> +extern void filtering_parse(const char *);
>  
>  #define DECL_IOCTL(name)						\
>  extern int								\
> diff --git a/filter.h b/filter.h
> index 7509b1e5..3861f8b8 100644
> --- a/filter.h
> +++ b/filter.h
> @@ -35,6 +35,8 @@ struct filter_action;
>  
>  struct bool_expression;
>  
> +extern bool parsing_qualify_mode;
> +
>  typedef int (*string_to_uint_func)(const char *);
>  void parse_set(const char *const, struct number_set *const,
>  	       string_to_uint_func, const char *const);
> @@ -56,6 +58,7 @@ void set_filters_qualify_mode(struct filter **, unsigned int *nfilters);
>  /* filter action api */
>  struct filter *create_filter(struct filter_action *, const char *name);
>  struct filter_action *find_or_add_action(const char *);
> +void parse_filter_action(const char *, const char *, const char *);
>  void *get_filter_action_priv_data(struct filter_action *);
>  void set_filter_action_priv_data(struct filter_action *, void *);
>  void set_qualify_mode(struct filter_action *);
> @@ -64,5 +67,9 @@ void set_qualify_mode(struct filter_action *);
>  struct bool_expression *create_expression();
>  bool run_expression(struct bool_expression *, bool *, unsigned int);
>  void set_expression_qualify_mode(struct bool_expression *);
> +void parse_filter_expression(struct bool_expression *, const char *,
> +			     struct filter_action *, unsigned int);
> +
> +void parse_qualify_action(const char *, const char *, const char *);
>  
>  #endif /* !STRACE_FILTER_H */
> diff --git a/filter_action.c b/filter_action.c
> index be4af4d3..776ffded 100644
> --- a/filter_action.c
> +++ b/filter_action.c
> @@ -127,7 +127,7 @@ filtering_parsing_finish(void)
>  
>  	/* Init trace action if pathtracing is enabled */
>  	if (tracing_paths && (default_flags & QUAL_TRACE))
> -		parse_qualify_filter("trace=all");
> +		filtering_parse("trace=all");
>  
>  	/* Sort actions by priority */
>  	if (nfilter_actions == 0)
> @@ -193,6 +193,18 @@ find_or_add_action(const char *name)
>  	return add_action(type);
>  }
>  
> +void
> +parse_filter_action(const char *action_name, const char *expr, const char *args)
> +{
> +	struct filter_action *action = find_or_add_action(action_name);
> +
> +	parse_filter_expression(action->expr, expr, action, action->nfilters);
> +	if (args && action->type->parse_args == &parse_null)
> +		error_msg("%s action takes no arguments, ignored argument '%s'",
> +			  action->type->name, args);
> +	action->_priv_data = action->type->parse_args(args);
> +}
> +
>  static void
>  run_filter_action(struct tcb *tcp, struct filter_action *action)
>  {
> diff --git a/filter_expression.c b/filter_expression.c
> index 05f8903e..c991bb2b 100644
> --- a/filter_expression.c
> +++ b/filter_expression.c
> @@ -27,6 +27,16 @@
>  
>  #include "defs.h"
>  #include <stdarg.h>
> +#include "filter.h"
> +
> +extern bool is_space_ascii(char);
> +
> +static bool
> +is_allowed_in_name(char c)
> +{
> +	return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
> +	       || (c >= '0' && c <= '9') || (c == '_');
> +}
>  
>  struct expression_token {
>  	enum token_type {
> @@ -42,6 +52,8 @@ struct expression_token {
>  		} operator_id;
>  	} data;
>  };
> +/* Pseudo-operator for parsing */
> +#define OP_PARENTHESIS 3
>  
>  struct bool_expression {
>  	unsigned int ntokens;
> @@ -68,6 +80,25 @@ reallocate_expression(struct bool_expression *const expr,
>  	expr->ntokens = new_ntokens;
>  }
>  
> +static void
> +add_variable_token(struct bool_expression *expr, unsigned int id)
> +{
> +	struct expression_token token;
> +	token.type = TOK_VARIABLE;
> +	token.data.variable_id = id;
> +	reallocate_expression(expr, expr->ntokens + 1);
> +	expr->tokens[expr->ntokens - 1] = token;
> +}
> +
> +static void
> +add_operator_token(struct bool_expression *expr, int op) {
> +	struct expression_token token;
> +	token.type = TOK_OPERATOR;
> +	token.data.operator_id = op;
> +	reallocate_expression(expr, expr->ntokens + 1);
> +	expr->tokens[expr->ntokens - 1] = token;
> +}
> +
>  void
>  set_expression_qualify_mode(struct bool_expression *expr)
>  {
> @@ -233,3 +264,192 @@ run_expression(struct bool_expression *expr, bool *variables,
>  					    variables, variables_num);
>  	return stack[0];
>  }
> +
> +/*
> + * Parse operator and add operator length to str and pos.
> + * Return -1 if no operator found.
> + */
> +static int
> +parse_operator(char **str, unsigned int *pos)
> +{
> +#define _OP(s, op) { s, sizeof(s) - 1, op }
> +	struct {
> +		const char *str;
> +		int len;
> +		enum operator_type op;
> +	} ops[] = {
> +		_OP("!",	OP_NOT),
> +		_OP("not",	OP_NOT),
> +		_OP("&&",	OP_AND),
> +		_OP("and",	OP_AND),
> +		_OP("||",	OP_OR),
> +		_OP("or",	OP_OR),
> +	};
> +#undef _OP
> +	char *p = *str;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ops); i++) {
> +		if (!strncmp(p, ops[i].str, ops[i].len) &&
> +		    (!is_allowed_in_name(ops[i].str[0]) ||
> +		    !is_allowed_in_name(p[ops[i].len]))) {
> +			*str += ops[i].len - 1;
> +			*pos += ops[i].len - 1;
> +			return ops[i].op;
> +		}
> +	}
> +	return -1;
> +}
> +
> +static char *
> +unescape_argument(char **str)
> +{
> +	char *p;
> +	char *p_new;
> +	bool escaped = false;
> +	unsigned int size = 1;
> +	char *new_str = xcalloc(strlen(*str) + 1, 1);
> +
> +	for (p = *str, p_new = new_str; *p; ++p) {
> +		if (!escaped) {
> +			if (*p == '\\') {
> +				escaped = true;
> +				continue;
> +			} else if (is_space_ascii(*p) || *p == ')' || *p == '|'
> +			           || *p == '&') {
> +				break;
> +			}
> +		}
> +		escaped = false;
> +		*(p_new++) = *p;
> +		size++;
> +	}
> +	*str = p - 1;
> +	return xreallocarray(new_str, size, 1);;
Excess semicolon.

> +}
> +
> +static void
> +push_operator(int *stack, unsigned int *stack_size, int op)
> +{
> +	if (*stack_size == MAX_STACK_SIZE)
> +		error_msg_and_die("stack overflow");
> +	stack[*stack_size] = op;
> +	(*stack_size)++;
> +}
> +
> +static bool
> +is_higher_priority(int op_a, int op_b)
> +{
> +	bool op_priority[] = {
> +		[OP_NOT] = 2,
> +		[OP_AND] = 1,
> +		[OP_OR]  = 0
Curiously, previous version of the patch had comma here.

> +	};
> +	return op_priority[op_a] > op_priority[op_b];
> +}
> +
> +void
> +parse_filter_expression(struct bool_expression *expr, const char *str,
> +			struct filter_action *action, unsigned int start_id)
> +{
> +	enum {
> +		WAIT_FILTER,
> +		FILTER_NAME,
> +		FILTER_ARG,
> +		WAIT_OPERATOR,
> +	} state = WAIT_FILTER;
> +	unsigned int variable_id = start_id;
> +	/* Current stack stack_size */
> +	unsigned int st_size = 0;
> +	int stack[MAX_STACK_SIZE];
> +	char *buf = xstrdup(str);
> +	struct filter *cur_filter = NULL;
> +	char *filter_name = NULL;
> +	char *filter_arg = NULL;
> +	int op;
> +	char *p;
> +	unsigned int pos = 0;
> +
> +	for (p = buf; *p; ++p, ++pos) {
> +		switch (state) {
> +		case WAIT_FILTER:
> +			if (*p == '(') {
> +				push_operator(stack, &st_size, OP_PARENTHESIS);
> +			} else if ((op = parse_operator(&p, &pos)) >= 0) {
> +				if (op == OP_NOT) {
> +					push_operator(stack, &st_size, op);
> +				} else {
> +					error_msg_and_die("invalid operator "
> +							  "at '%s':%u",
> +							  str, pos);
> +				}
> +			} else if (!is_space_ascii(*p)) {
> +				filter_name = p;
> +				state = FILTER_NAME;
> +			}
> +			break;
> +
> +		case FILTER_NAME:
> +			if (is_space_ascii(*p)) {
> +				*p = '\0';
> +				cur_filter = create_filter(action, filter_name);
> +				filter_arg = NULL;
> +				state = FILTER_ARG;
> +			}
> +			break;
> +
> +		case FILTER_ARG:
> +			if (!filter_arg && is_space_ascii(*p))
> +				break;
> +			filter_arg = unescape_argument(&p);
> +			parse_filter(cur_filter, filter_arg);
> +			free(filter_arg);
> +			add_variable_token(expr, variable_id++);
> +			state = WAIT_OPERATOR;
> +			break;
> +
> +		case WAIT_OPERATOR:
> +			if (is_space_ascii(*p))
> +				break;
> +			if (*p == ')') {
> +				while ((st_size > 0) &&
> +				       (stack[st_size - 1] != OP_PARENTHESIS)) {
> +						op = stack[--st_size];
> +						add_operator_token(expr, op);
> +					}
> +				if (st_size == 0)
> +					error_msg_and_die("unexpected ')' at "
> +							"'%s':%u", str, pos);
> +				--st_size;
> +				break;
> +			}
> +			op = parse_operator(&p, &pos);
> +			if (op < 0 || op == OP_NOT)
> +				error_msg_and_die("invalid operator at '%s':%u",
> +						  str, pos);
> +
> +			/* Pop operators with higher priority. */
> +			while ((st_size > 0) &&
> +			       (stack[st_size - 1] != OP_PARENTHESIS) &&
> +			       is_higher_priority(stack[st_size - 1], op))
> +				add_operator_token(expr, stack[--st_size]);
> +
> +			push_operator(stack, &st_size, op);
> +			state = WAIT_FILTER;
> +			break;
> +		}
> +	}
> +
> +	free(buf);
> +	if (state != WAIT_OPERATOR)
> +		error_msg_and_die("unfinished filter expression '%s'", str);
> +
> +	while (st_size > 0) {
> +		if (stack[--st_size] == OP_PARENTHESIS)
> +			error_msg_and_die("missing ')' in expression '%s'",
> +					  str);
> +		add_operator_token(expr, stack[st_size]);
> +	}
> +	if (start_id > 0)
> +		add_operator_token(expr, OP_OR);
> +}
> diff --git a/filter_parse.c b/filter_parse.c
> new file mode 100644
> index 00000000..dd69806f
> --- /dev/null
> +++ b/filter_parse.c
> @@ -0,0 +1,223 @@
> +/*
> + * Copyright (c) 2017 Nikolay Marchuk <marchuk.nikolay.a at gmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "defs.h"
> +#include "filter.h"
> +
> +bool
> +is_space_ascii(char c)
> +{
> +	return (c == ' ') || (c == '\t') || (c == '\n') || (c == '\r') ||
> +	       (c == '\v') || (c == '\f');
> +}
> +
> +/*
> + * Split expression into action name, filter expression or qualify set
> + * and action arguments.
> + */
> +void
> +filtering_parse(const char *str)
> +{
> +	enum parsing_states {
> +		F_EMPTY,
> +		F_BEGIN,
> +		F_QUAL_SET,
> +		F_FILT_EXPR,
> +		F_QUAL_ARGS,
> +		F_FILT_ARGS,
> +		F_END,
> +	} state = F_EMPTY;
> +	const char *begin = NULL;
> +	const char *action_name = NULL;
> +	const char *main_part = NULL;
> +	const char *args = NULL;
> +	bool escaped = false;
> +	int parentheses_count = 0;
> +	/* Used to store position of last terminating parenthesis. */
> +	char *expression_end = NULL;
> +	/* Used to provide diagnostics. */
> +	unsigned int pos = 0;
> +	char *buf = xstrdup(str);
> +	char *p;
> +
> +	for (p = buf; *p; ++p, ++pos) {
> +		switch (state) {
> +		case F_EMPTY:
> +			switch (*p) {
> +			/* trace(), action name omitted */
> +			case '(':
> +				parentheses_count++;
> +				action_name = "trace";
> +				main_part = p + 1;
Btw, the fact that this change hadn't affected tests is remarkable.

> +				state = F_FILT_EXPR;
> +				break;
> +			case '=':
> +				error_msg_and_die("invalid filter action ''");
> +			default:
> +				if (!is_space_ascii(*p)) {
> +					begin = p;
> +					state = F_BEGIN;
> +				}
> +			}
> +			if (state != F_BEGIN)
> +				break;
> +			/* else fall through to check for qualify set */
> +
> +		case F_BEGIN:
> +			switch (*p) {
> +			/* action(...) */
> +			case '(':
> +				parentheses_count++;
> +				/* expression without action with leading ! */
> +				if (*begin == '!')
> +					break;
> +				action_name = begin;
> +				*p = '\0';
> +				main_part = p + 1;
> +				state = F_FILT_EXPR;
> +				break;
> +			/* action=... */
> +			case '=':
> +				action_name = begin;
> +				*p = '\0';
> +				main_part = p + 1;
> +				state = F_QUAL_SET;
> +				break;
> +			/* qualify set without action. */
> +			case ',':
> +			case '?':
> +			case '/':
> +			case '%':
> +			case '-':
> +				action_name = "trace";
> +				main_part = begin;
> +				state = F_QUAL_SET;
> +				break;
> +			default:
> +				/* new expression without action. */
> +				if (is_space_ascii(*p)) {
> +					action_name = "trace";
> +					main_part = begin;
> +					state = F_FILT_EXPR;
> +				}
> +			}
> +			break;
> +
> +		case F_QUAL_SET:
> +			if (*p == ':') {
> +				*p = '\0';
> +				args = p + 1;
> +				state = F_QUAL_ARGS;
> +			}
> +			break;
> +
> +		case F_FILT_EXPR:
> +			if (!escaped) {
> +				switch (*p) {
> +				case ';':
> +					if (parentheses_count != 1)
> +						error_msg_and_die("invalid "
> +								  "arguments "
> +								  "position "
> +								  "'%s':%u",
> +								  str, pos);
> +					*p = '\0';
> +					args = p + 1;
> +					state = F_FILT_ARGS;
> +					break;
> +				case '(':
> +					parentheses_count++;
> +					break;
> +				case ')':
> +					if (parentheses_count <= 0)
> +						error_msg_and_die("unexpected "
> +								  "')' at "
> +								  "'%s':%u",
> +								  str, pos);
> +					parentheses_count--;
> +					expression_end = p;
> +					break;
> +				case '\\':
> +					escaped = true;
> +					break;
> +				}
> +			} else
> +				escaped = false;
> +			break;
> +
> +		case F_QUAL_ARGS:
> +			break;
> +		case F_FILT_ARGS:
> +			if (!escaped) {
> +				switch (*p) {
> +				case ')':
> +					if (parentheses_count <= 0)
> +						error_msg_and_die("unexpected "
> +								  "')' at "
> +								  "'%s':%u",
> +								  str, pos);
> +					parentheses_count--;
> +					expression_end = p;
> +					state = F_END;
> +					break;
> +				case '\\':
> +					escaped = true;
> +					break;
> +				}
> +			} else
> +				escaped = false;
> +			break;
> +		case F_END:
> +			if (!is_space_ascii(*p))
> +				error_msg_and_die("illegal character '%c' in "
> +						  "'%s':%u", *p, str, pos);
> +		}
> +	}
> +
> +	switch (state) {
> +	case F_EMPTY:
> +		error_msg_and_die("invalid filter expression '%s'", str);
> +	case F_BEGIN:
> +		action_name = "trace";
> +		main_part = begin;
> +		/* fall through */
> +	case F_QUAL_SET:
> +	case F_QUAL_ARGS:
> +		parse_qualify_action(action_name, main_part, args);
> +		break;
> +	case F_FILT_EXPR:
> +	case F_FILT_ARGS:
> +	case F_END:
> +		if (parentheses_count != 0)
> +			error_msg_and_die("missing ')' in '%s'", str);
> +		if (expression_end)
> +			*expression_end = '\0';
> +		parse_filter_action(action_name, main_part, args);
> +		break;
> +	}
> +	free(buf);
> +}
> diff --git a/filter_qualify.c b/filter_qualify.c
> index 3765f656..51f1a2ba 100644
> --- a/filter_qualify.c
> +++ b/filter_qualify.c
> @@ -181,27 +181,31 @@ parse_inject_common_args(char *str, struct inject_opts *const opts,
>  }
>  
>  static void
> -parse_read(const char *const str)
> +parse_read(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("read");
>  	struct filter *filter = create_filter(action, "fd");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("read action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -parse_write(const char *const str)
> +parse_write(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("write");
>  	struct filter *filter = create_filter(action, "fd");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("write action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -qualify_signals(const char *const str)
> +qualify_signals(const char *const main_part, const char *const args)
>  {
>  	/* Clear the set. */
>  	if (signal_set.nslots)
> @@ -209,89 +213,97 @@ qualify_signals(const char *const str)
>  		       sizeof(*signal_set.vec) * signal_set.nslots);
>  	signal_set.not = false;
>  
> -	parse_set(str, &signal_set, sigstr_to_uint, "signal");
> +	parse_set(main_part, &signal_set, sigstr_to_uint, "signal");
> +	if (args)
> +		error_msg("signal action takes no arguments: '%s'", args);
>  }
>  
>  static void
> -parse_trace(const char *const str)
> +parse_trace(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("trace");
>  	struct filter *filter = create_filter(action, "syscall");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("trace action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -parse_abbrev(const char *const str)
> +parse_abbrev(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("abbrev");
>  	struct filter *filter = create_filter(action, "syscall");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("abbrev action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -parse_verbose(const char *const str)
> +parse_verbose(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("verbose");
>  	struct filter *filter = create_filter(action, "syscall");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("verbose action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -parse_raw(const char *const str)
> +parse_raw(const char *const main_part, const char *const args)
>  {
>  	struct filter_action *action = find_or_add_action("raw");
>  	struct filter *filter = create_filter(action, "syscall");
>  
> -	parse_filter(filter, str);
> +	parse_filter(filter, main_part);
> +	if (args)
> +		error_msg("raw action takes no arguments: '%s'", args);
>  	set_qualify_mode(action);
>  }
>  
>  static void
> -parse_inject_common_qualify(const char *const str, const bool fault_tokens_only,
> -			    const char *const description)
> +parse_inject_common_qualify(const char *const main_part, const char *const args,
> +		    const bool fault_tokens_only, const char *const description)
>  {
>  	struct inject_opts *opts = xmalloc(sizeof(struct inject_opts));
> -	char *buf = xstrdup(str);
> +	char *buf = args ? xstrdup(args) : NULL;
>  	struct filter_action *action;
>  	struct filter *filter;
> -	char *args = strchr(buf, ':');
> -
> -	if (args)
> -		*(args++) = '\0';
>  
>  	action = find_or_add_action(fault_tokens_only ? "fault" : "inject");
>  	filter = create_filter(action, "syscall");
> -	parse_filter(filter, buf);
> +	parse_filter(filter, main_part);
>  	set_qualify_mode(action);
> -	parse_inject_common_args(args, opts, ":", fault_tokens_only);
> -	if (!opts->init)
> +	parse_inject_common_args(buf, opts, ":", fault_tokens_only);
> +	if (!opts->init) {
Unexpected addition of curly braces.

>  		error_msg_and_die("invalid %s '%s'", description,
>  				  args ? args : "");
> -	free(buf);
> +	}
> +	if (buf)
> +		free(buf);
>  	set_filter_action_priv_data(action, opts);
>  }
>  
>  static void
> -parse_fault(const char *const str)
> +parse_fault(const char *const main_part, const char *const args)
>  {
> -	parse_inject_common_qualify(str, true, "fault argument");
> +	parse_inject_common_qualify(main_part, args, true, "fault argument");
>  }
>  
>  static void
> -parse_inject(const char *const str)
> +parse_inject(const char *const main_part, const char *const args)
>  {
> -	parse_inject_common_qualify(str, false, "inject argument");
> +	parse_inject_common_qualify(main_part, args, false, "inject argument");
>  }
>  
>  static const struct qual_options {
>  	const char *name;
> -	void (*qualify)(const char *);
> +	void (*qualify)(const char *, const char *);
>  } qual_options[] = {
>  	{ "trace",	parse_trace	},
>  	{ "t",		parse_trace	},
> @@ -314,23 +326,26 @@ static const struct qual_options {
>  	{ "inject",	parse_inject	},
>  };
>  
> +bool parsing_qualify_mode = false;
> +
>  void
> -parse_qualify_filter(const char *str)
> +parse_qualify_action(const char *action_name, const char *main_part,
> +		     const char *args)
>  {
> -	const struct qual_options *opt = qual_options;
> +	const struct qual_options *opt = NULL;
>  	unsigned int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(qual_options); ++i) {
> -		const char *name = qual_options[i].name;
> -		const size_t len = strlen(name);
> -		const char *val = str_strip_prefix_len(str, name, len);
> -
> -		if (val == str || *val != '=')
> -			continue;
> -		str = val + 1;
> -		opt = &qual_options[i];
> -		break;
> +		if (!strcmp(action_name, qual_options[i].name)) {
> +			opt = &qual_options[i];
> +			break;
> +		}
>  	}
>  
> -	opt->qualify(str);
> +	if (!opt)
> +		error_msg_and_die("invalid filter action '%s'", action_name);
> +	/* Use qualify mode of set parsing. */
> +	parsing_qualify_mode = true;
> +	opt->qualify(main_part, args);
> +	parsing_qualify_mode = false;
>  }
> diff --git a/strace.c b/strace.c
> index 6162d3b3..79c5397e 100644
> --- a/strace.c
> +++ b/strace.c
> @@ -1580,7 +1580,7 @@ init(int argc, char *argv[])
>  #if DEFAULT_QUAL_FLAGS != (QUAL_TRACE | QUAL_ABBREV | QUAL_VERBOSE)
>  # error Bug in DEFAULT_QUAL_FLAGS
>  #endif
> -	parse_qualify_filter("signal=all");
> +	filtering_parse("signal=all");
>  	while ((c = getopt(argc, argv,
>  		"+b:cCdfFhiqrtTvVwxyz"
>  #ifdef USE_LIBUNWIND
> @@ -1647,7 +1647,7 @@ init(int argc, char *argv[])
>  			show_fd_path++;
>  			break;
>  		case 'v':
> -			parse_qualify_filter("abbrev=none");
> +			filtering_parse("abbrev=none");
>  			break;
>  		case 'V':
>  			print_version();
> @@ -1662,7 +1662,7 @@ init(int argc, char *argv[])
>  				error_opt_arg(c, optarg);
>  			break;
>  		case 'e':
> -			parse_qualify_filter(optarg);
> +			filtering_parse(optarg);
>  			break;
>  		case 'o':
>  			outfname = optarg;
> diff --git a/tests/filtering_syscall-syntax.test b/tests/filtering_syscall-syntax.test
> index 2a4125ae..3f8db02b 100755
> --- a/tests/filtering_syscall-syntax.test
> +++ b/tests/filtering_syscall-syntax.test
> @@ -54,25 +54,11 @@ for arg in '' , ,, ,,, \! \!, \
>  	check_syscall "$arg" "$arg"
>  done
>  
> -# Temporary workaround
>  for arg in : :: ::: \
>  	   ; do
> -	check_e "invalid system call '$arg'" -e trace="$arg"
> -	check_e "invalid system call '$arg'" -e abbrev="$arg"
> -	check_e "invalid system call '$arg'" -e verbose="$arg"
> -	check_e "invalid system call '$arg'" -e raw="$arg"
> -	check_e "invalid system call ''" -e inject="$arg"
> -	check_e "invalid system call ''" -e fault="$arg"
> +	check_syscall "" "$arg"
>  done
>  
> -# Temporary workaround for '!:' check
> -check_e "invalid system call ':'" -e trace=\!:
> -check_e "invalid system call ':'" -e abbrev=\!:
> -check_e "invalid system call ':'" -e verbose=\!:
> -check_e "invalid system call ':'" -e raw=\!:
> -check_e "invalid system call '!'" -e inject=\!:
> -check_e "invalid system call '!'" -e fault=\!:
> -
>  # invalid syscall with inversion
>  for arg in -1 -2 -3 -4 -5 \
>  	   invalid_syscall_name \
> @@ -99,6 +85,7 @@ for arg in file,nonsense \
>  done
>  
>  # special cases
> +check_syscall "!"	\!:
>  check_syscall "!"	1,\!
>  check_syscall "!open"	1,\!open
>  check_syscall "none"	1,none
> -- 
> 2.11.0




More information about the Strace-devel mailing list