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

* Re: [PATCH v2] libtracefs: An API to set the filtering of functions
  2021-03-18 15:30 [PATCH v2] libtracefs: An API to set the filtering of functions Sameeruddin shaik
@ 2021-03-18  4:23 ` Tzvetomir Stoyanov
  2021-03-19 16:50   ` sameeruddin shaik
  0 siblings, 1 reply; 3+ messages in thread
From: Tzvetomir Stoyanov @ 2021-03-18  4:23 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: Steven Rostedt, Linux Trace Devel

Hi Sameer,
I have only one comment on this version:

On Wed, Mar 17, 2021 at 6:02 PM Sameeruddin shaik
<sameeruddin.shaik8@gmail.com> wrote:
>
> This new API will write the function filters into the
> set_ftrace_filter file.
>
> tracefs_function_filter()
>
> https://bugzilla.kernel.org/show_bug.cgi?id=210643
>
> Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
> ---
> Error handling is addressed as per the comments
> if filters[i] is not NULL, then only we are going inside for loop, at the asprintf i think it won't be necessary to check the filters[i] is NULL or not again, when it throws error, if i am wrong please correct me.
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..c1f07b0 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o
>  int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id);
>  int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id);
>  const char *tracefs_option_name(enum tracefs_option_id id);
> -
> +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
> +                           const char *module, bool reset, const char ***errs);
>  #endif /* _TRACE_FS_H */
> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
> index e2dfc7b..ca6077b 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -18,6 +18,7 @@
>  #include "tracefs-local.h"
>
>  #define TRACE_CTRL     "tracing_on"
> +#define TRACE_FILTER   "set_ftrace_filter"
>
>  static const char * const options_map[] = {
>         "unknown",
> @@ -387,3 +388,108 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt
>         if (options && id > TRACEFS_OPTION_INVALID)
>                 options->mask &= ~(1ULL << (id - 1));
>  }
> +
> +static int controlled_write(const char *filter_path, const char **filters,
> +                           const char *module, bool reset, const char ***errs)
> +{
> +       int flags = reset ? O_TRUNC : O_APPEND;
> +       const char **e = NULL;
> +       char *each_str = NULL;
> +       int write_size = 0;
> +       int size = 0;
> +       int fd = -1;
> +       int ret = 0;
> +       int j = 0;
> +       int i;
> +
> +       fd = open(filter_path, O_WRONLY | flags);
> +       if (fd < 0)
> +               return 1;
> +
> +       for (i = 0; filters[i]; i++) {
> +               if (module)
> +                       write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module);
> +               else
> +                       write_size = asprintf(&each_str, "%s ", filters[i]);
> +               if (write_size < 0) {
> +                       ret = 1;
> +                       goto error;
> +               }
> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes*/
> +               if (size < write_size) {
> +                       if (errs) {
> +                               e = realloc(e, (j + 1) * (sizeof(char *)));
> +                               if (!e) {

The commonly used pattern when working with realloc is to use a
temporary pointer for the return value. The problem when using the
same pointer is that in case of a realloc() error, it will return NULL
which will overwrite our original pointer and we will lose it. The old
memory is still valid, realloc() does not free it in case of an error,
but we cannot access it as the old pointer is overwritten with that
NULL. So, I would suggest something like that:
                              e_new = realloc(e, (j + 1) * (sizeof(char *)));
                              if (!e_new) {
                                       free(e);
                                       ret = 1;
                                       goto error;
                               } else
                                     e = e_new;

> +                                       ret = 1;
> +                                       goto error;
> +                               }
> +                               e[j++] = filters[i];
> +                               ret -= 1;
> +                       }
> +               }
> +               free(each_str);
> +               each_str = NULL;
> +       }
> +       if (errs) {
> +               e = realloc(e, (j + 1) * (sizeof(char *)));
> +               if (!e) {
> +                       free(e);

The same comment as above. Here "e" is already NULL. Calling
free(NULL) will not crash, but will not free the old memory.

> +                       ret = 1;
> +                       goto error;
> +               }
> +               e[j] = NULL;
> +               *errs = e;
> +       }
> + error:
> +       if (each_str)
> +               free(each_str);
> +       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: A pointer to array of constant strings that will be allocated
> + * on 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 @filters. Otherwise, the functions set
> + * by @filters will be appended to the filter file
> + *
> + * 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
> + * pointing to the strings in @filters that failed and must be freed
> + * with free().
> + *
> + * returns 1 on general errors not realted to setting the filter.
> + * @errs is not set even if supplied.
> + *
> + * return 0 on success and @errs is not set.
> + */
> +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
> +                           const char *module, bool reset, const char ***errs)
> +{
> +       char *ftrace_filter_path;
> +       int ret = 0;
> +
> +       if (!filters)
> +               return 1;
> +
> +       ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> +       if (!ftrace_filter_path)
> +               return 1;
> +
> +       ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
> +       tracefs_put_tracing_file(ftrace_filter_path);
> +       return ret;
> +}
> --
> 2.7.4
>

Thanks for sending this version, Sameer!
I think we can merge it,  when you fix that realloc() error checking.
I consider this patch as a good foundation for the API, but we should
work on some optimizations on top of it, in separate patches. What
else should be added, when this patch is merged:
 1. A unit test of the API
 2. Man page of the API
 3. Optimizations: new kernels support writing indexes instead of
strings into the "set_ftrace_filter" file. This is faster, as there is
no string comparison in the kernel context. The function index can be
retrieved from "available_filter_functions" files - the first function
is with index 0, the next is 1 ... and so forth. So, the possible
optimization of the API is to check - if the kernel supports indexes,
use it, or fail back to the legacy logic with strings.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread