Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 22 Mar 2021 21:27:55 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This adds a new API tracefs_function_filter() as described in:
> 
>  https://bugzilla.kernel.org/show_bug.cgi?id=210643
> 
> It will use regular expressions against available_filter_functions (or even
> the kernel glob expression) to enable the function by the index method if it
> is supported. If it is not supported, it will go back to the writing of the
> filter strings directly into the set_ftrace_filter file.
> 
> 

Playing with the interface some more, I feel it's not quite adequate.

The returning the negative number of filters that failed, isn't very
useful. Having the errs array that points to those filters gives us that
information.

Also, because of the way that file works in the kernel, we need to be able
to call this function several times without closing the file. That's
because the actions take place when the file is closed, *not* when a write
is made. Having the interface return errors without closing the file would
allow the user to fix the failed filters and try again.

I plan on committing this patch series, so any new changes will be done on
top of them. But here's what I think needs to be done to make the interface
more usable.

 - Create a tracefs_function_filter_commit(instance) API that will close
   the file and commit the changes.

 - Have the tracefs_function_filter() open the set_ftrace_filter file if it
   is not already opened. This will allow for the function to be called
   multiple times. The file descriptor will be part of the instance
   descriptor or a global variable for the top level instance. The opening
   of the files will need to be protected by a pthread mutex.
   Note, the "reset" parameter is only applicable if the file is not
   already opened.

 - On success, return 0 and tracefs_function_filter_commit() must be called.

 - If the file descriptor is opened and an error happens, return 1, and the
   tracefs_function_filter_commit() still needs to be called, but the user
   can call tracefs_function_filter() again to try to fix the problem.

 - If an error is found before the file descriptor is opened, then
   tracefs_function_filter_commit() does not need to be called.

That is:

	ret = tracefs_function_filter(...);
	if (ret >= 0)
		tracefs_function_filter_commit();

 - Now for errs, it will be allocated if a problem happened on a filter,
   and may be set if the return value is non zero (1 or -1). The user can
   use it to know if the problem happened on a filter as well as find out
   which filter had the problem.

 - We can have a tracefs_read_function_filter() that returns an array of
   strings which ends with a NULL pointer (and needs to be freed with
   tracefs_list_free(), and have this:

   save_filters = tracefs_read_function_filter(instance);

   ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
   if (ret > 0 && errs) {
	/* Modified but failed */
	int i, j;
	for (i = 0; filters[i]; i++)
		;
	for (j = 0; errs[i]; j++)
		;
	if (i == j)
		/* all filters failed! Put back the original */
		tracefs_function_filter(instance, save_filters, NULL, false, NULL);
   }
   if (ret >= 0)
	tracefs_function_filter_commit(instance);

Another reason to have it not close the file is because we only support
passing in one module at a time. If the user wants to enable functions in
two modules, they would need to call this twice, and we want them to be
able to do so and have both changes take affect at the same time.

Thoughts?

-- Steve