Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions

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

Hi Sammer,

On Mon, Feb 8, 2021 at 3:17 AM Sameeruddin Shaik
<sameeruddin.shaik8@gmail.com> wrote:
>
> Hi steve,
> This is my progress so far on Adding API's to set the filtering of functions https://bugzilla.kernel.org/show_bug.cgi?id=210643. I have few doubts here , based on my assumptions, i have implemented the below API.
> API function:
>
> int tracefs_function_filter(struct tracefs_instance *instance, const char *filter);
>
> please give me some inputs on the below questions
>
> 1.Here filter is only one string or multiple strings followed by space?
It is up to us to decide if the API will accept only one function at
each call, or a list of functions.
I think the logic should be simple, only one function per call.

>
> 2.I have used some internal API's to write to the set_ftrace_filter file, but with those API's
>
> we can be able to write only one string to the file, can i add my own write functionality in this
>
> function?
Yes, please do. You can modify internal functions in order to reuse
functionality. Please, be
sure that all external APIs that use those internal functions are not broken.

>
> Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 8b11d35..29aee66 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -47,6 +47,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 *filter);
>
>  /**
>   * 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 101f389..cb0bc51 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -14,6 +14,7 @@
>  #include "tracefs-local.h"
>
>  #define TRACE_CTRL "tracing_on"
> +#define TRACE_FLTR      "set_ftrace_filter"
>
>  static int trace_on_off(int fd, bool on)
>  {
> @@ -107,3 +108,84 @@ int tracefs_trace_off_fd(int fd)
>   return -1;
>   return trace_on_off(fd, false);
>  }
> +
> +int tracefs_function_filter(struct tracefs_instance *instance, const char *filter)
> +{
> + bool res = 0;;
One extra semicolon.

> + int ret= 0;
> + char *available_filter_functions;
> + char *buf, *saveptr, *str;
> + char *final_filter;
> + struct stat st;
> + size_t len;
> + char *function_name;
> + char *saveptr1, *str1;
> + char *each_filter;
> + char *fun_sub_name;
> + char *tmp = NULL;
Small hint on coding style. Please, arrange the variables by length -
longest first, shortest last.
We are trying to use this coding style rule, at least for the new code.

> +
> + if (!filter)
> + return -1;
> + len = strlen(filter);
> +
> + final_filter = (char *)malloc(sizeof(char) * len);
> +
> + if (!final_filter)
> + goto gracefully_free;
> + memset(final_filter, '\0', len);
> +
> + available_filter_functions = tracefs_instance_get_file(NULL, "available_\
> +filter_functions");
It is better to have it on a single line. This coding style rule could
not be followed
strictly in all cases, sometimes the code is more readable in one line.

> + ret = stat(available_filter_functions, &st);
> + if (ret < 0)
> + goto gracefully_free;
> + /*Reading the available_filter_functions file*/
> + len = str_read_file(available_filter_functions, &buf);
> + if (len <= 0)
> + goto gracefully_free;
> + /* Match each function passed in filter against the available_filter_fucntions*/
I think verifying the requested filter functions using
available_filter_functions could be
omitted. There is such validation in the kernel, so let's use it.
Writing invalid strings into
set_ftrace_filter should return an error, and we can pass that error
to the API caller.
Note that set_ftrace_filter supports wildcards also, you can filter
"sched_*" for example.
That makes the validation more complex.
I would recommend to verify if a const char *filter is a valid string
only and to rely on
the kernel verification for valid function filter.


> + for (str1 = filter; ; str1 = NULL) {
> + each_filter = strtok_r(str1, " ", &saveptr1);
> + if (!each_filter)
> + break;
> + tmp = (char *)malloc(sizeof(char) * len);
> + if (!tmp)
> + goto gracefully_free;
> + if (memcpy(tmp, buf, len) == NULL)
> + goto gracefully_free;
> + for (str = tmp; ; str = NULL) {
> + function_name = strtok_r(str, "\n", &saveptr);
> + if (!function_name)
> + break;
> + /*Matching the fucntion names like chunk_err [brtfs]*/
> + if (strchr(function_name, '[') != NULL) {
> + fun_sub_name = strtok(function_name, " [");
> + if (strcmp(fun_sub_name, each_filter) == 0){
> + strncat(final_filter, each_filter, strlen(each_filter));
> + strcat(final_filter, " ");
> + res = 1;
> + break;
> + }
> + }
> + if (strcmp(function_name, each_filter) == 0) {
> + strncat(final_filter, each_filter, strlen(each_filter));
> + strcat(final_filter, " ");
> + res = 1;
> + break;
> + }
> +
> + }
> + }
> + free(buf);
> + strcat(final_filter, "\n");
> + if (res)
> + ret = tracefs_instance_file_write(instance, TRACE_FLTR, final_filter);
Note that using this API will truncate the file, instead we should
append the new filter.

> + else
> + return -1;
> +
> + gracefully_free:
> + free(available_filter_functions);
> + free(tmp);
> + free(final_filter);
> + return ret;
> +}
>
> Thanks,
>
> sameer.

You can find documentation for ftrace function filtering in kernel tree,
in Documentation/trace/ftrace.rst. Please, look at it and - there are
some filter commands, that set_ftrace_filter supports. This new libtracefs
API should handle them.

Thanks for working on libtracefs :)

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