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.
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
