[PATCH] Introduce new filtering architecture

[Nikolay Marchuk]

On 19.06.2017 11:08, Eugene Syromiatnikov wrote:
>> +		if (!strncmp(name, str, len)) {
> Not sure whether strncmp is better than strcmp here, since this function
> is not called with user-supplied str at all.
> 
> Or, do you expect that these will work with user-supplied strings in the
> future?
> 
> BTW, personally, I don't think that this needs to be string-keyed.
> Something as simple as
> 
>     enum filter_type_name {
>             FILTER_TYPE_syscall,
>             FILTER_TYPE_fd,
>     };
> 
>     #define FILTER_TYPE(name) \
>             [FILTER_TYPE_ ## name] = { \
>                     .parse_filter = parse_ ## name ## _filter, \
>                     .run_filter = run_ ## name ## _filter, \
>                     ._free_priv_data = free_ ## name ## _filter, \
>             }
> 
>     static const struct filter_type {
>             void* (*parse_filter)(const char *, const char *const);
>             bool (*run_filter)(struct tcb *, void *);
>             void (*_free_priv_data)(void *);
>     } filter_types[] = {
>             FILTER_TYPE(syscall),
>             FILTER_TYPE(fd),
>     };
> 
>     struct filter *
>     add_filter_to_array(struct filter **filters, unsigned int *nfilters,
>                         enum filter_type_name name)
>     {
>             *filters = xreallocarray(*filters, ++(*nfilters),
>                                      sizeof(struct filter));
>             struct filter *filter = &((*filters)[*nfilters - 1]);
>             filter->type = filters[name];
>             return filter;
>     }
> 
>     [...]
> 
>     #define create_filter(a, t) actual_create_filter(a, FILTER_TYPE_ ## t)
> 
>     [...]
> 
>     create_filter(action, fd);
> 
> should also suffice.
I am going to use string-keyed filters and actions for new filtering
language
parsing and enum is not sufficient.

>> +
>> +DECL_FILTER_ACTION(trace);
>> +DECL_FILTER_ACTION(inject);
>> +DECL_FILTER_ACTION(read);
>> +DECL_FILTER_ACTION(write);
>> +DECL_FILTER_ACTION(raw);
>> +DECL_FILTER_ACTION(abbrev);
>> +DECL_FILTER_ACTION(verbose);
>> +
>> +#undef DECL_FILTER_ACTION
> And the same confusion about declarations and definitions.
These declarations are necessary for extensibility of filtering mechanism.
New action can be declared in new file, but after adding a declaration
and a filter_action_type entry it can be used for filtering.

>> +	FILTER_ACTION_TYPE(read,	1,	null,	is_traced),
>> +	FILTER_ACTION_TYPE(write,	1,	null,	is_traced),
> This is actually an interesting question: do we want I/O being dumped for
> the calls which we do not trace? Current architecture doesn't allow that,
> but is there a possibility that there are circumstances where it may make
> sense?
This patch's goal is to make new implementation of old behavior.

>> +static void
>> +run_filter_action(struct tcb *tcp, struct filter_action *filt_action)
>> +{
>> +	if (filt_action->type->prefilter
>> +	    && !filt_action->type->prefilter(tcp))
> I'm not quite sure how it is guaranteed that the flag would be set (like,
> where the "trace filter runs first" order is implied).
Fixed with sorting actions by priority.

> Overall the most significant issue I see with the approach is the need
> to run all the filters even they are not needed for the evaluation of
> the boolean expression. Since I do not see the current action-to-filter
> relation as many-to-one, I see little advantage in running all
> the filters in advance and not during the expression evaluation (current
> implementation already can benefit of it, as it adds only the last
> filter to expression, effectively replacing previous one, so only the
> last filter has to be executed).
I fixed computing of all filters for current implementation. After full
implementation of new mechanism I will add optimizations of filter
expression and only necessary filters will be checked.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: OpenPGP digital signature
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170619/86c39c50/attachment.bin>