Re: [PATCH 13/13 v2] libtracefs: Add CONTINUE to tracefs_function_filter()

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Tue, Mar 30, 2021 at 5:52 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 30 Mar 2021 17:29:57 +0300
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > On Tue, Mar 30, 2021 at 3:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > >
> > > Add the TRACEFS_FL_CONTINUE flag to the tracefs_function_filter() API that
> > > will allow it to return without closing the set_ftrace_filter file. When
> > > the set_ftrace_filter file is closed, all the changes to it take place,
> > > but not before hand. In the case that multiple modules need to be set in
> > > one activation, the tracefs_function_filter() would need to be called
> > > multiple times without closing the file descriptor.
> > >
> > > Note, the next call to tracefs_function_filter() after it was called with
> > > the CONTINUE flag set, the RESET flag is ignored, as the RESET flag only
> > > takes effect on opening the file. The next call to
> > > tracefs_function_filter() after it was called with the CONTINUE flag (on
> > > the same instance) does not reopen the file, and thus will not reset the
> > > file.
> >
> > I think it is better to not maintain a state in the API context. Let's
> > make it simple and allow the user to take care of the calling order
> > and flags.
> >    If RESET is set - close the file (if it is opened already) and open
> > it with the TRUNC flag.
>
> In the use cases I've played with, it was simple to do:
>
>         for (i = 0; filters[i]; i++)
>                 tracefs_function_filter(instance, filters[i], module,
>                                 TRACEFS_FL_RESET | TRACEFS_FL_CONTINUE);
>

If the user takes care of the state, instead of the library, this use
case could look like:

   tracefs_function_filter(instance, NULL, NULL, TRACEFS_FL_RESET |
TRACEFS_FL_CONTINUE);
/* or add a wrapper tracefs_function_filter_reset() */

   for (i = 0; filters[i]; i++)
                 tracefs_function_filter(instance, filters[i], module,
TRACEFS_FL_CONTINUE);

   tracefs_function_filter(instance, NULL, NULL, 0);
/* or add a wrapper tracefs_function_filter_commit() */


> And not worry about keeping track of the RESET flag. This is the reason I
> did it this way. Otherwise I would need to be:
>
>         int reset = TRACEFS_FL_RESET;
>         for (i = 0; filters[i]; i++) {
>                 tracefs_function_filter(instance, filters[i], module,
>                                 reset | TRACEFS_FL_CONTINUE);
>                 reset = 0;
>         }
>
> Or something else. But I'm looking to simplify the most common case.
>
> And closing the file can cause issues. If we were to go this way, I would
> make it that if the file is open and RESET is set, it would return an error
> EBUSY.
>
> But no, closing the file just because RESET is set is dangerous.
>
> >    If CONTINUE is set - do not close the file.
> > Also, allow calling the API with no filters and any combination of
> > flags, just to have resting and committing currently written filters.
> > For example:
> >  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_RESET, NULL); //
> > Close the file (if it is opened already), open it with the TRUNC flag
> > and close it.
> >  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_CONTINUE, NULL);
> > //  Open the file with APPEND (if it is not opened already) and do not
> > close it.
> >  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_RESET |
> > TRACEFS_FL_CONTINUE, NULL);  // Close the file (if it is opened
> > already), open it with the TRUNC flag and do not close it.
>
> Yes, I had already planned on adding a patch to allow NULL filter when
> these flags are set. I just didn't get there yet.
>
> -- Steve
>
> >
> >
> > >
> > > If the file is opened, calling tracefs_function_filter() with no filters
> > > and the continue flag not set, will simply close the set_ftrace_filter
> > > file.
> > >
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > ---



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center