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

[Sameeruddin Shaik <sameeruddin.shaik8@gmail.com>]

Hi steve,
I have doubt

On Tue, Mar 23, 2021 at 6:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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);

are we going to use the tracefs_read_function_filter() function to
read the set_ftrace_file for getting written filters?
>
>    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

Thanks,
sameer.