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

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

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.
   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.


>
> 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>
> ---
>  Documentation/libtracefs-function-filter.txt | 32 +++++++++++++---
>  include/tracefs-local.h                      |  1 +
>  include/tracefs.h                            |  2 +
>  src/tracefs-instance.c                       |  2 +
>  src/tracefs-tools.c                          | 39 ++++++++++++++++----
>  5 files changed, 64 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/libtracefs-function-filter.txt b/Documentation/libtracefs-function-filter.txt
> index 5b55a72727c8..1ac8a06961bf 100644
> --- a/Documentation/libtracefs-function-filter.txt
> +++ b/Documentation/libtracefs-function-filter.txt
> @@ -52,7 +52,20 @@ The _flags_ parameter may have the following set, or be zero.
>  If _flags_ contains *TRACEFS_FL_RESET*, then it will clear the filters that
>  are currently set before applying the list of filters from _filters_. Otherwise,
>  the list of filters from _filters_ will be added to the current set of filters
> -already enabled.
> +already enabled. This flag is ignored if a previous call to
> +tracefs_function_filter() had the same _instance_ and the
> +*TRACEFS_FL_CONTINUE* flag was set.
> +
> +*TRACEFS_FL_CONTINUE* :
> +If _flags_ contains *TRACEFS_FL_CONTINUE*, then the filters will not take
> +effect after a successful call to tracefs_function_filter(). This allows for
> +multiple calls to tracefs_function_filter() to update the filter function.
> +It can be called multiple times and add more filters. A call without this
> +flag set will commit the changes before returning (if the filters passed in
> +successfully matched). A tracefs_function_filter() call after one that had
> +the *TRACEFS_FL_CONTINUE* flag set for the same instance will ignore the
> +*TRACEFS_FL_RESET* flag, as the reset flag is only applicable for the first
> +filters to be added before committing.
>
>  RETURN VALUE
>  ------------
> @@ -70,13 +83,13 @@ EXAMPLE
>
>  #define INST "dummy"
>
> -const char *filters[] = { "run_init_process", "try_to_run_init_process", "dummy1", NULL };
> +static const char *filters[] = { "run_init_process", "try_to_run_init_process", "dummy1", NULL };
> +static const char *all[] = { "*", NULL };
>
>  int main(int argc, char *argv[])
>  {
>         struct tracefs_instance *inst = tracefs_instance_create(INST);
>         const char **errs = NULL;
> -       int flags = TRACEFS_FL_RESET;
>         int ret;
>         int i = 0;
>
> @@ -84,15 +97,24 @@ int main(int argc, char *argv[])
>                 /* Error creating new trace instance */
>         }
>
> -       ret = tracefs_function_filter(inst, filters, NULL, flags, &errs);
> +       ret = tracefs_function_filter(inst, filters, NULL,
> +                                     TRACEFS_FL_RESET | TRACEF_FL_CONTINUE,
> +                                     &errs);
>
>         if (ret < 0 && errs) {
>                 while (errs[i])
>                         printf("%s\n", errs[i++]);
>         }
> +       free(errs);
> +
> +       ret = tracefs_function_filter(inst, all, "ext4", 0, &errs);
> +       if (ret < 0) {
> +               printf("Failed to set filters for ext4\n");
> +               /* Force the function to commit previous filters */
> +               tracefs_function_filter(inst, NULL, NULL, 0, &errs);
> +       }
>
>         tracefs_instance_destroy(inst);
> -       free(errs);
>         return 0;
>  }
>  --
> diff --git a/include/tracefs-local.h b/include/tracefs-local.h
> index 9c18218cd916..73ec113fdb20 100644
> --- a/include/tracefs-local.h
> +++ b/include/tracefs-local.h
> @@ -18,6 +18,7 @@ struct tracefs_instance {
>         char    *trace_dir;
>         char    *name;
>         int     flags;
> +       int     ftrace_filter_fd;
>  };
>
>  /* Can be overridden */
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 5193d46f41f5..f4775e938f69 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -148,9 +148,11 @@ const char *tracefs_option_name(enum tracefs_option_id id);
>
>  /*
>   * RESET       - Reset on opening filter file (O_TRUNC)
> + * CONTINUE    - Do not close filter file on return.
>   */
>  enum {
>         TRACEFS_FL_RESET        = (1 << 0),
> +       TRACEFS_FL_CONTINUE     = (1 << 1),
>  };
>
>  int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
> diff --git a/src/tracefs-instance.c b/src/tracefs-instance.c
> index a02c839f2079..bf2fabf111ea 100644
> --- a/src/tracefs-instance.c
> +++ b/src/tracefs-instance.c
> @@ -43,6 +43,8 @@ static struct tracefs_instance *instance_alloc(const char *trace_dir, const char
>                         goto error;
>         }
>
> +       instance->ftrace_filter_fd = -1;
> +
>         return instance;
>
>  error:
> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
> index 7e191e207867..862db5caa20e 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -23,6 +23,8 @@
>  #define TRACE_FILTER           "set_ftrace_filter"
>  #define TRACE_FILTER_LIST      "available_filter_functions"
>
> +/* File descriptor for Top level set_ftrace_filter  */
> +static int ftrace_filter_fd = -1;
>  static pthread_mutex_t filter_lock = PTHREAD_MUTEX_INITIALIZER;
>
>  static const char * const options_map[] = {
> @@ -851,6 +853,12 @@ static int write_func_list(int fd, struct func_list *list)
>   *          before applying the @filters. This flag is ignored
>   *          if this function is called again when the previous
>   *          call had TRACEFS_FL_CONTINUE set.
> + *   TRACEFS_FL_CONTINUE - will keep the filter file open on return.
> + *          The filter is updated on closing of the filter file.
> + *          With this flag set, the file is not closed, and more filters
> + *          may be added before they take effect. The last call of this
> + *          function must be called without this flag for the filter
> + *          to take effect.
>   *
>   * returns -x on filter errors (where x is number of failed filter
>   * srtings) and if @errs is not NULL will be an allocated string array
> @@ -869,13 +877,26 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
>         struct func_list *func_list = NULL;
>         char *ftrace_filter_path;
>         bool reset = flags & TRACEFS_FL_RESET;
> +       bool cont = flags & TRACEFS_FL_CONTINUE;
>         int open_flags;
>         int ret = 1;
> -       int fd;
> +       int *fd;
>
>         pthread_mutex_lock(&filter_lock);
> -       if (!filters)
> +       if (instance)
> +               fd = &instance->ftrace_filter_fd;
> +       else
> +               fd = &ftrace_filter_fd;
> +
> +       if (!filters) {
> +               /* OK to call without filters if this is closing the opened file */
> +               if (!cont && *fd >= 0) {
> +                       ret = 0;
> +                       close(*fd);
> +                       *fd = -1;
> +               }
>                 goto out;
> +       }
>
>         func_filters = make_func_filters(filters);
>         if (!func_filters)
> @@ -896,16 +917,20 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
>
>         open_flags = reset ? O_TRUNC : O_APPEND;
>
> -       fd = open(ftrace_filter_path, O_WRONLY | open_flags);
> +       if (*fd < 0)
> +               *fd = open(ftrace_filter_path, O_WRONLY | open_flags);
>         tracefs_put_tracing_file(ftrace_filter_path);
> -       if (fd < 0)
> +       if (*fd < 0)
>                 goto out_free;
>
> -       ret = write_func_list(fd, func_list);
> +       ret = write_func_list(*fd, func_list);
>         if (ret > 0)
> -               ret = controlled_write(fd, func_filters, module, errs);
> +               ret = controlled_write(*fd, func_filters, module, errs);
>
> -       close(fd);
> +       if (!cont) {
> +               close(*fd);
> +               *fd = -1;
> +       }
>
>   out_free:
>         free_func_list(func_list);
> --
> 2.30.1
>
>


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