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

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

Hi Sameer,

On Fri, Mar 5, 2021 at 1:21 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>
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..2e5d3e3 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -50,6 +50,7 @@ int tracefs_trace_on(struct tracefs_instance *instance);
>  int tracefs_trace_off(struct tracefs_instance *instance);
>  int tracefs_trace_on_fd(int fd);
>  int tracefs_trace_off_fd(int fd);
> +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, const char *module, bool reset, const char **errs);
>
>  /**
>   * tracefs_trace_on_get_fd - Get a file descriptor of "tracing_on" in given instance
> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
> index e2dfc7b..8311191 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,90 @@ 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;
> +       int write_size;
> +       char *each_str;
> +       int ret = 0;
> +       int j = 0;
> +       int size;
> +       int slen;
> +       int fd;
> +       int i;
> +
> +       fd = open(filter_path, O_WRONLY | flags);
> +       if (fd < 0)
> +               return -1;

The error return code should be 1, or the caller of this internal
function should translate this "-1" to the error code according
to the API description.

> +
> +       for (i = 0; filters[i]; i++) {
> +               slen = strlen(filters[i]);
> +               if (!slen)
> +                       continue;
> +
> +               if (module)
> +                       /* Adding 5 extra bytes for ":mod:"*/
> +                       slen += strlen(module) + 5;
> +
> +               /* Adding 2 extra byte for the space and '\0' at the end*/
> +               slen += 2;
> +               each_str = calloc(1, slen);
> +               if (!each_str)
> +                       return -1;

Same here.

> +               if (module)
> +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> +               else
> +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> +
> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes and also compare the written bytes with difference of added 5 bytes to string for ":mod:"*/
> +               if ((size < write_size) && (size < (write_size - 5))) {

The second comparison is not needed, the size must be equal to
write_size in case of success.

> +                       errs[j++] = filters[i];

The errs must be (re)allocated here, to hold additional pointer
to string. The user does not know in advance the number of
failed filters and it is not expected to provide an already
allocated array. So it should be a triple pointer.

It is OK to assign directly failed input string to the error array,
but this should be written in the API description, i.e. the user
should not free the strings from the @errs array as it is a subset
of the @filters array, or something like that. But note, the user
still must free the @errs itself.

> +                       ret -= 1;
> +               }
> +               free(each_str);
> +       }
> +       errs[j] = NULL;
> +       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: An Array of failed function names ending with a NULL pointer
> + *
> + * 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 @filter. Otherwise, the functions set by @filter
> + * will be appended to the filter file
> + *
> + * The @errs is an array of strings, where each string is a failed function
> + * name
> + *
> + * returns -x (where x is number of failed filter srtings or it can be
> + * 1 for general errors), or 0 if there are no errors.
> + */
> +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;

This should be initialised to the error return code, to ensure that
every "goto out" will return error.

> +
> +       if (!filters)
> +               return -1;

Should return 1, according to the API description.

> +
> +       ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> +       if (!ftrace_filter_path)
> +               goto out;

"ret" is 0 here, so success will be returned, which is not the desired
behaviour.
"ftrace_filter_path" is NULL and the tracefs_put_tracing_file()
will be called with NULL, but this is OK, should not be a problem.

> +
> +       ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
> + out:
> +       tracefs_put_tracing_file(ftrace_filter_path);
> +       return ret;
> +}
> --
> 2.7.4
>

It is a good starting point, thanks for sending this patch Sameer!

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