Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 5 Mar 2021 14:20:15 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > +               if (module)
> > +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> > +               else
> > +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> > +
> > +               size = write(fd, each_str, write_size);
> > +               /* compare written bytes and also compare the written bytes with difference of added 5 bytes to string for ":mod:"*/
> > +               if ((size < write_size) && (size < (write_size - 5))) {  
> 
> The second comparison is not needed, the size must be equal to
> write_size in case of success.
> 
> > +                       errs[j++] = filters[i];  
> 
> The errs must be (re)allocated here, to hold additional pointer
> to string. The user does not know in advance the number of
> failed filters and it is not expected to provide an already
> allocated array. So it should be a triple pointer.
> 
> It is OK to assign directly failed input string to the error array,
> but this should be written in the API description, i.e. the user
> should not free the strings from the @errs array as it is a subset
> of the @filters array, or something like that. But note, the user
> still must free the @errs itself.

A check to see if "errs" is NULL is needed too. If the user doesn't care
about the errors, they can pass in NULL to errs, in which case, they should
be ignored.

> 
> > +                       ret -= 1;
> > +               }
> > +               free(each_str);
> > +       }
> > +       errs[j] = NULL;
> > +       close(fd);
> > +       return ret;
> > +}
> > +
> > +/**
> > + * tracefs_function_filter - write to set_ftrace_filter file to trace particular functions
> > + * @instance: ftrace instance, can be NULL for top tracing instance
> > + * @filters: An array of function names ending with a NULL pointer
> > + * @module: Module to be traced
> > + * @reset: set to true to reset the file before applying the filter
> > + * @errs: An Array of failed function names ending with a NULL pointer

Thus the description for @errs should be:

 @errs: A pointer to an array of const strings that will be allocated on
        a negative return of this function, pointing to the filters that
        failed. May be NULL, in which case this field will be ignored.

> > + *
> > + * The @filters is an array of strings, where each string will be used to set
> > + * a function or functions to be traced.
> > + *
> > + * If @reset is true, then all functions in the filter are cleared before
> > + * adding functions from @filter. Otherwise, the functions set by @filter
> > + * will be appended to the filter file
> > + *
> > + * The @errs is an array of strings, where each string is a failed function
> > + * name
> > + *
> > + * returns -x (where x is number of failed filter srtings or it can be
> > + * 1 for general errors), or 0 if there are no errors.
> > + */
> > +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, const char *module, bool reset, const char **errs)

Note, @filters should be of type: const char * const * filters,  as not
only is filters pointing to constant strings, the array itself will not be
modified. And errs, should be of type: const char ***errs. Where it will be
allocated.

Since a triple pointer is difficult to manage in the code, you could have:

	const char **e = NULL;

	
		if (errs) {
			e = realloc(sizeof(*e), j + 1);
			e[j++] = filters[i];
		}

Then at the end:

	if (errs)
		*errs = e;


> > +{
> > +       char *ftrace_filter_path;
> > +       int ret = 0;  
> 
> This should be initialised to the error return code, to ensure that
> every "goto out" will return error.
> 
> > +
> > +       if (!filters)
> > +               return -1;  
> 
> Should return 1, according to the API description.
> 
> > +
> > +       ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> > +       if (!ftrace_filter_path)
> > +               goto out;  
> 
> "ret" is 0 here, so success will be returned, which is not the desired
> behaviour.
> "ftrace_filter_path" is NULL and the tracefs_put_tracing_file()
> will be called with NULL, but this is OK, should not be a problem.

It shouldn't be a problem, but it is also fine to return directly here.

	if (!ftrace_filter_path)
		return 1;

Thanks!

-- Steve

> 
> > +
> > +       ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
> > + out:
> > +       tracefs_put_tracing_file(ftrace_filter_path);
> > +       return ret;
> > +}
> > --
> > 2.7.4
> >  
> 
> It is a good starting point, thanks for sending this patch Sameer!
>