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

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

On Tue, Feb 9, 2021 at 8:38 AM Sameeruddin Shaik
<sameeruddin.shaik8@gmail.com> wrote:
>
> Tzvetomir,
> Thanks for Your review, will try to modify the code as you suggested
> and will give a new patch.

Hi again Sameer,
I forgot to mention two very important remarks:
 - Please, use "git send-mail" when sending patches.
 - Please, use the kernel script to check the patches for any problems,
   before sending them. You can find it in the kernel tree,
"scripts/checkpatch.pl ... "
 My usual work flow is:
    1. Extract local patches that I want to send, using "git format-patch ..."
    2. Check for errors "scripts/checkpatch.pl ... patch_name". If any - fix,
        apply with "git commit --amend" and goto step 1. You can ignore warnings
        for the line length, decide which of them to address individually.
    3. When everything looks good, use "git send-mail patch_name" to send it.
       Please, add "cc = linux-trace-devel@vger.kernel.org" to your
.gitconfig, and
       ensure "to= ... " is the correct recipient.

> Thanks,
> sameer shaik.
>
>
> On Tue, Feb 9, 2021 at 11:22 AM Tzvetomir Stoyanov
> <tz.stoyanov@gmail.com> wrote:
> >
> > 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



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