[PATCH v8 02/12] Introduce new filtering architecture

Dmitry V. Levin ldv at altlinux.org
Tue Aug 22 17:33:01 UTC 2017


On Tue, Aug 22, 2017 at 01:01:43PM +0700, Nikolay Marchuk wrote:
> This change introduces new filtering architecture primitives: filter,
> filter_action and bool_expression. Filtering is now done after decoding of
> syscall and tcp->qual_flg stores filtering results.
> 
> * basic_actions.c: New file.
> * basic_filters.c (number_isset): Return bool.
> (is_number_in_set): Likewise.
> (add_number_to_set): Make static.
> (qualify_syscall_number): Rename to parse_syscall_number.
> (qualify_syscall_regex): Rename to parse_syscall_regex.
> (qualify_syscall_class): Rename to parse_syscall_class.
> (qualify_syscall_name): Rename to parse_syscall_name.
> (qualify_syscall): Rename to parse_syscall, use renamed functions.
> (qualify_syscall_tokens): Rename to parse_syscall_set, make static,
> remove "name" argument (assume "system call"), remove 'handle_inversion'
> label, remove additional clearing of the sets.
> (qualify_tokens): Rename to parse_set, remove 'handle_inversion' label,
> remove additional clearing of the set.
> (parse_syscall_filter, parse_fd_filter, parse_path_filter,
> run_syscall_filter, run_fd_filter, run_path_filter,
> free_syscall_filter, free_fd_filter, free_path_filter): New functions.
> * defs.h (struct inject_opts): Add init flag.
> (QUAL_READ, QUAL_WRITE): Change description.
> (traced, dump_read, dump_write): Add macros for checking QUAL_TRACE,
> QUAL_READ, QUAL_WRITE.
> (read_set, write_set): Remove global set variables.
> (qualify, qual_flags): Remove old declarations ...
> (filter_syscall, parse_qualify_filter, filtering_parsing_finish):
>  ... and add new declarations.
> * filter.c: New file.
> * filter.h: Change declarations.
> * filter_action.c: New file.
> * filter_expression.c: Likewise.
> * filter_qualify.c (read_set, write_set, abbrev_set, inject_set, raw_set,
> trace_set, verbose_set): Remove set variables.
> (parse_inject_expression): Remove function.
> (parse_inject_common_args): New function.
> (qualify_read): Rename to parse_read.
> (qualify_write): Rename to parse_write.
> (qualify_signals): Use parse_set function. Add clearing of the set.
> (qualify_trace): Rename to parse_trace.
> (qualify_abbrev): Rename to parse_abbrev.
> (qualify_verbose): Rename to parse_verbose.
> (qualify_raw): Rename to parse_raw.
> (qualify_inject_common): Rename to parse_inject_common_qualify, use
> new filters.
> (qualify_fault): Rename to parse_fault.
> (qualify_inject): Rename to parse_inject.
> (qualify): Rename to parse_qualify_filter.
> (qual_flags): Remove function.
> * Makefile.am (strace_SOURCES): Add new files.
> * pathtrace.c (storepath): Duplicate paths.
> * strace.c (init): Use new filtering for -e option.
> (trace_syscall): Add filtering after syscall decoding.
> (droptcb): Don't free inject_vec for empty personalities.
> * syscall.c (decode_socket_subcall): Remove qual_flags from decoder.
> (decode_ipc_subcall): Likewise.
> (decode_mips_subcall): Likewise.
> (get_scno): Likewise.
> (inject_vec, tamper_with_syscall_entering): Remove inject_vec support code.
> (syscall_entering_trace): Use traced macro
> (dumpio): Check new macros instead of global sets.
> ---
>  Makefile.am         |   4 +
>  basic_actions.c     | 130 ++++++++++++++++++++++++
>  basic_filters.c     | 169 ++++++++++++++++++++++---------
>  defs.h              |  15 +--
>  filter.c            | 144 +++++++++++++++++++++++++++
>  filter.h            |  37 +++++--
>  filter_action.c     | 240 ++++++++++++++++++++++++++++++++++++++++++++
>  filter_expression.c | 281 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  filter_qualify.c    | 242 ++++++++++++++++++++------------------------
>  pathtrace.c         |   2 +-
>  strace.c            |  18 ++--
>  syscall.c           |  24 +----
>  xmalloc.c           |   3 +
>  13 files changed, 1090 insertions(+), 219 deletions(-)
>  create mode 100644 basic_actions.c
>  create mode 100644 filter.c
>  create mode 100644 filter_action.c
>  create mode 100644 filter_expression.c

First of all, this patch is too large, much larger than I can review.

If you do pure renaming (btw, why do you want to rename all these
functions?), please do not mix it with other changes.

> diff --git a/basic_filters.c b/basic_filters.c
> index 1dbdeac8..c6b5497e 100644
> --- a/basic_filters.c
> +++ b/basic_filters.c
> @@ -48,7 +48,8 @@ number_setbit(const unsigned int i, number_slot_t *const vec)
>  static bool
>  number_isset(const unsigned int i, const number_slot_t *const vec)
>  {
> -	return vec[i / BITS_PER_SLOT] & ((number_slot_t) 1 << (i % BITS_PER_SLOT));
> +	return !!((vec[i / BITS_PER_SLOT]
> +		  & ((number_slot_t) 1 << (i % BITS_PER_SLOT))));

I don't understand why do you need this ...

>  }
> 
>  static void
> @@ -62,7 +63,7 @@ reallocate_number_set(struct number_set *const set, const unsigned int new_nslot
>  	set->nslots = new_nslots;
>  }
> 
> -void
> +static void
>  add_number_to_set(const unsigned int number, struct number_set *const set)
>  {
>  	reallocate_number_set(set, number / BITS_PER_SLOT + 1);
> @@ -72,12 +73,12 @@ add_number_to_set(const unsigned int number, struct number_set *const set)
>  bool
>  is_number_in_set(const unsigned int number, const struct number_set *const set)
>  {
> -	return ((number / BITS_PER_SLOT < set->nslots)
> -		&& number_isset(number, set->vec)) ^ set->not;
> +	return !!(((number / BITS_PER_SLOT < set->nslots)
> +		&& number_isset(number, set->vec)) ^ set->not);

and this.

>  }
> 
>  static bool
> -qualify_syscall_number(const char *s, struct number_set *set)
> +parse_syscall_number(const char *s, struct number_set *set)
>  {
>  	int n = string_to_uint(s);
>  	if (n < 0)
> @@ -108,7 +109,7 @@ regerror_msg_and_die(int errcode, const regex_t *preg,
>  }
> 
>  static bool
> -qualify_syscall_regex(const char *s, struct number_set *set)
> +parse_syscall_regex(const char *s, struct number_set *set)
>  {
>  	regex_t preg;
>  	int rc;
> @@ -180,7 +181,7 @@ lookup_class(const char *s)
>  }
> 
>  static bool
> -qualify_syscall_class(const char *s, struct number_set *set)
> +parse_syscall_class(const char *s, struct number_set *set)
>  {
>  	const unsigned int n = lookup_class(s);
>  	if (!n)
> @@ -203,7 +204,7 @@ qualify_syscall_class(const char *s, struct number_set *set)
>  }
> 
>  static bool
> -qualify_syscall_name(const char *s, struct number_set *set)
> +parse_syscall_name(const char *s, struct number_set *set)
>  {
>  	unsigned int p;
>  	bool found = false;
> @@ -225,7 +226,7 @@ qualify_syscall_name(const char *s, struct number_set *set)
>  }
> 
>  static bool
> -qualify_syscall(const char *token, struct number_set *set)
> +parse_syscall(const char *token, struct number_set *set)
>  {
>  	bool ignore_fail = false;
> 
> @@ -234,11 +235,11 @@ qualify_syscall(const char *token, struct number_set *set)
>  		ignore_fail = true;
>  	}
>  	if (*token >= '0' && *token <= '9')
> -		return qualify_syscall_number(token, set) || ignore_fail;
> +		return parse_syscall_number(token, set) || ignore_fail;
>  	if (*token == '/')
> -		return qualify_syscall_regex(token + 1, set) || ignore_fail;
> -	return qualify_syscall_class(token, set)
> -	       || qualify_syscall_name(token, set)
> +		return parse_syscall_regex(token + 1, set) || ignore_fail;
> +	return parse_syscall_class(token, set)
> +	       || parse_syscall_name(token, set)
>  	       || ignore_fail;
>  }
> 

These changes look like pure renames.  Please provide a justification
for this change and please do not mix renames with other changes.

> diff --git a/filter_qualify.c b/filter_qualify.c
> index 4283e769..ff54720a 100644
> --- a/filter_qualify.c
> +++ b/filter_qualify.c
> @@ -38,16 +38,8 @@ struct number_set {
>  	bool not;
>  };
> 
> -struct number_set read_set;
> -struct number_set write_set;
>  struct number_set signal_set;
> 
> -static struct number_set abbrev_set[SUPPORTED_PERSONALITIES];
> -static struct number_set inject_set[SUPPORTED_PERSONALITIES];
> -static struct number_set raw_set[SUPPORTED_PERSONALITIES];
> -static struct number_set trace_set[SUPPORTED_PERSONALITIES];
> -static struct number_set verbose_set[SUPPORTED_PERSONALITIES];

Note that both basic_filters.c and filter_qualify.c define the same
structure number_set.  This is absolutely unacceptable and I'm sorry
I missed it while reviewing b75c5b14, otherwise it wouldn't appear
in the first place.

I think I'll move all these number_set functions to a separate file
and create a proper API header.

> +		/* Clear qual_flg to differ valid syscall from printargs */

s/differ/distinguish/

> diff --git a/xmalloc.c b/xmalloc.c
> index 45ff57b1..b77f6910 100644
> --- a/xmalloc.c
> +++ b/xmalloc.c
> @@ -91,6 +91,9 @@ xreallocarray(void *ptr, size_t nmemb, size_t size)
>  char *
>  xstrdup(const char *str)
>  {
> +	if (!str)
> +		return NULL;
> +
>  	char *p = strdup(str);
> 
>  	if (!p)

Please do not mix this kind of changes with other stuff.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170822/6e801064/attachment.bin>


More information about the Strace-devel mailing list