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

* Re: [PATCH v1] libtracefs: An API to set the filtering of functions
  2021-03-16 17:44 [PATCH v1] libtracefs: An API to set the filtering of functions Sameeruddin shaik
@ 2021-03-16 11:37 ` Tzvetomir Stoyanov
  2021-03-16 11:55   ` Sameeruddin Shaik
  0 siblings, 1 reply; 3+ messages in thread
From: Tzvetomir Stoyanov @ 2021-03-16 11:37 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: Steven Rostedt, Linux Trace Devel

Hi Sameer,
I think we are almost there, I have a few comments on the error
handling, that should be addressed before merging the patch.

On Mon, Mar 15, 2021 at 7:49 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>
> ---
> changed snprintf to asprintf for string formation
> handled the lines over 80 characters
>
> 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..6b33321 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,93 @@ 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)
> +                       continue;

I think we can consider asprintf() failure as a major problem. There
are two options - break the loop or goto error label.

> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes*/
> +               if (size < write_size) {
> +                       if (errs) {
> +                               e = realloc(e, (j + 1) * (sizeof(char *)));

The realloc return value must be checked. In case of an error - break
the loop or goto error label.

> +                               e[j++] = filters[i];
> +                               ret -= 1;
> +                       }
> +               }
> +               free(each_str);

Set each_str to NULL after the free, This will help with proper
resources cleanup, in case of an error.

> +       }

If you choose the option to break the loop in case of an error, check
here if filters[i] is not NULL, i.e. if the loop exit with an error.
If there was an error - clean up all internally allocated resources
(fd, e, each_str) and return error.
The other option is to add "error" or "out" label at the end of the
function where to do this resource cleanups.

> +       if (errs) {
> +               e = realloc(e, (j + 1) * (sizeof(char *)));

Check that realloc return value too.

> +               e[j] = NULL;
> +               *errs = e;
> +       }
> +       close(fd);

Note: in case of an internal error exit, free e and each_str, if they
were allocated, and do not modify err.

> +       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.
Note: in case of an error exit, free e and each_str, if they were allocated
> + */
> +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, Sameer!

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

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