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

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

[Tzvetomir Stoyanov]

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

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

[Sameeruddin Shaik]

Tzvetomir,
Thanks for Your review, will try to modify the code as you suggested
and will give a new patch.
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

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

[Tzvetomir Stoyanov]

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

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

[Steven Rostedt]

On Tue, 9 Feb 2021 07:52:20 +0200
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.

I was thinking about this some more. If we have one function per call, it
would require opening up the set_ftrace_filter file each time, which I
think is unnecessary. This also brings up how we update the filter. If you
open the file with O_TRUNC set, it will clear the existing filter,
otherwise it appends to it.

Perhaps instead of passing a "const char *filter", we should pass an array,
"const char * const *filters", that is a n array of const char *
functions, and the last one being NULL.

We should also add a reset boolean value. Which if true, it opens the file
with O_TRUNC causing the file to be reset before we add the new values.

int tracefs_function_filter(struct tracefs_instance *instance,
		const char * const *filter, bool reset);

> >
> > 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"

I'd rather have the above be "TRACE_FILTER", making things too cryptic adds
more unnecessary confusion. The TRACE_CTRL is cryptic enough ;-)

> >
> >  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.

Yes, we call it upside down x-mas tree style. it looks like an upside down
x-mas tree and is much easier to read. Although, its best if you try to
keep the same types bunched together.

> 
> > +
> > + if (!filter)
> > + return -1;
> > + len = strlen(filter);
> > +
> > + final_filter = (char *)malloc(sizeof(char) * len);

Unnecessary typecast. Also, sizeof(char) is always one.

	final_filter = malloc(len);

is prefectly fine.

> > +
> > + if (!final_filter)
> > + goto gracefully_free;
> > + memset(final_filter, '\0', len);

If you are going to zero it out, its best to use calloc instead of malloc:

	final_filter = calloc(1, len);

Which will zero out the memory that you allocate for you.

> > +
> > + 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;

At the beginning of the loop, you should use regex to define the filters
(thus, people can use regex or the glob kernel parsing).

See: man 3 regex.

Create one regex_t for each filter passed in. Then you can go down and run
regexec() against the available filter names to find the matches.

Also since 5.1 (so we need to test this out to see if it errors or not),
you can pass in numbers to set_ftrace_filter that will set the
corresponding record in available_filter_functions (starting with 1).

That is:

 # cd /sys/kernel/tracing
 # head available_filter_functions
__traceiter_initcall_level
__traceiter_initcall_start
__traceiter_initcall_finish
trace_initcall_finish_cb
initcall_blacklisted
do_one_initcall
do_one_initcall
match_dev_by_label
match_dev_by_uuid
rootfs_init_fs_context

 # echo 5 > set_ftrace_filter
 # cat set_ftrace_filter
initcall_blacklisted

Where "initcall_blacklisted" is the fifth element in
available_filter_functions. It's much quicker to pass in numbers than it is
names, as it removes the searches.

Thus, if you can find all the names in available filter functions and
create an array of their index (remember it starts with 1 not zero), and
pass in those numbers, it will be much quicker!

But if a number fails (errors on write), then you have to default back to
the names (as kernels before 5.1 don't have this feature).

Who knew such a "simple" API would be so complex ;-)

Thanks!

-- Steve

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

[Sameeruddin Shaik]

tzvetomir,
sure will do as you said.
since its a WIP patch, i sent it without any validations.

steve,
-_________________________________________________
> > +
> > + 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;

At the beginning of the loop, you should use regex to define the filters
(thus, people can use regex or the glob kernel parsing).

See: man 3 regex.

Create one regex_t for each filter passed in. Then you can go down and run
regexec() against the available filter names to find the matches.

Also since 5.1 (so we need to test this out to see if it errors or not),
you can pass in numbers to set_ftrace_filter that will set the
corresponding record in available_filter_functions (starting with 1)
_________________________________________________________

By this you mean to say like, we have to do the filter functions
validation in the API?
or
shall we continue with tzvetomir idea (I mean depending on the kernel
validation)?

Thanks,
sameer.


On Tue, Feb 9, 2021 at 10:28 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 9 Feb 2021 07:52:20 +0200
> 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.
>
> I was thinking about this some more. If we have one function per call, it
> would require opening up the set_ftrace_filter file each time, which I
> think is unnecessary. This also brings up how we update the filter. If you
> open the file with O_TRUNC set, it will clear the existing filter,
> otherwise it appends to it.
>
> Perhaps instead of passing a "const char *filter", we should pass an array,
> "const char * const *filters", that is a n array of const char *
> functions, and the last one being NULL.
>
> We should also add a reset boolean value. Which if true, it opens the file
> with O_TRUNC causing the file to be reset before we add the new values.
>
> int tracefs_function_filter(struct tracefs_instance *instance,
>                 const char * const *filter, bool reset);
>
> > >
> > > 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"
>
> I'd rather have the above be "TRACE_FILTER", making things too cryptic adds
> more unnecessary confusion. The TRACE_CTRL is cryptic enough ;-)
>
> > >
> > >  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.
>
> Yes, we call it upside down x-mas tree style. it looks like an upside down
> x-mas tree and is much easier to read. Although, its best if you try to
> keep the same types bunched together.
>
> >
> > > +
> > > + if (!filter)
> > > + return -1;
> > > + len = strlen(filter);
> > > +
> > > + final_filter = (char *)malloc(sizeof(char) * len);
>
> Unnecessary typecast. Also, sizeof(char) is always one.
>
>         final_filter = malloc(len);
>
> is prefectly fine.
>
> > > +
> > > + if (!final_filter)
> > > + goto gracefully_free;
> > > + memset(final_filter, '\0', len);
>
> If you are going to zero it out, its best to use calloc instead of malloc:
>
>         final_filter = calloc(1, len);
>
> Which will zero out the memory that you allocate for you.
>
> > > +
> > > + 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;
>
> At the beginning of the loop, you should use regex to define the filters
> (thus, people can use regex or the glob kernel parsing).
>
> See: man 3 regex.
>
> Create one regex_t for each filter passed in. Then you can go down and run
> regexec() against the available filter names to find the matches.
>
> Also since 5.1 (so we need to test this out to see if it errors or not),
> you can pass in numbers to set_ftrace_filter that will set the
> corresponding record in available_filter_functions (starting with 1).
>
> That is:
>
>  # cd /sys/kernel/tracing
>  # head available_filter_functions
> __traceiter_initcall_level
> __traceiter_initcall_start
> __traceiter_initcall_finish
> trace_initcall_finish_cb
> initcall_blacklisted
> do_one_initcall
> do_one_initcall
> match_dev_by_label
> match_dev_by_uuid
> rootfs_init_fs_context
>
>  # echo 5 > set_ftrace_filter
>  # cat set_ftrace_filter
> initcall_blacklisted
>
> Where "initcall_blacklisted" is the fifth element in
> available_filter_functions. It's much quicker to pass in numbers than it is
> names, as it removes the searches.
>
> Thus, if you can find all the names in available filter functions and
> create an array of their index (remember it starts with 1 not zero), and
> pass in those numbers, it will be much quicker!
>
> But if a number fails (errors on write), then you have to default back to
> the names (as kernels before 5.1 don't have this feature).
>
> Who knew such a "simple" API would be so complex ;-)
>
> Thanks!
>
> -- Steve
>

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

[Steven Rostedt]

On Thu, 11 Feb 2021 08:33:50 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> By this you mean to say like, we have to do the filter functions
> validation in the API?
> or
> shall we continue with tzvetomir idea (I mean depending on the kernel
> validation)?

I mean write to the file, and if it errors, then do our own operations on
it. This would need to be documented in the man page for this function.


For example:

   filter = "sched*"

would write into the file, which the kernel should turn into a selection of
all functions that start with "sched".

But if you had:

   filter = "sched_feat_[^w].*"

Which would set "sched_feat_open" and "sched_feat_show" but not
"sched_feat_write". Since it is a regex and not a glob expression, it will
fail the filter.

 fd = open("set_ftrace_filter", O_WRONLY);
 write(fd, "sched_feat_[^w].*", 7);

Will return an error of EINVAL. When that happens, you should run this
through the regex exec, and then use the result to find all the matches.

Note, it should be implied that if the regex does not start with "^" and
end with "$", then that should be added too.

  "sched_feat_[^w].*" turns into "^sched_feat_[^w].*$"

before processing it into the regex.

You may be asking, what if someone wants to add something that is a regex,
but also acceptable as a glob?

Let's say you want "_*acpi_device_uevent_modalias", where you want:

 "__acpi_device_uevent_modalias" and "acpi_device_uevent_modalias"

But the glob would only select "__acpi_device_uevent_modalias"

To force the regex, the user should just add "^" to the beginning of the
string.

Thus "_*acpi_device_uevent_modalias" wont work as expected, but
"^_*acpi_device_uevent_modalias" will.

Make sense?

-- Steve

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

[Sameeruddin Shaik]

Yeah steve understood,
Will try to implement the functionality in phases.

On Wed, Feb 10, 2021 at 9:08 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 11 Feb 2021 08:33:50 +0530
> Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > By this you mean to say like, we have to do the filter functions
> > validation in the API?
> > or
> > shall we continue with tzvetomir idea (I mean depending on the kernel
> > validation)?
>
> I mean write to the file, and if it errors, then do our own operations on
> it. This would need to be documented in the man page for this function.
>
>
> For example:
>
>    filter = "sched*"
>
> would write into the file, which the kernel should turn into a selection of
> all functions that start with "sched".
>
> But if you had:
>
>    filter = "sched_feat_[^w].*"
>
> Which would set "sched_feat_open" and "sched_feat_show" but not
> "sched_feat_write". Since it is a regex and not a glob expression, it will
> fail the filter.
>
>  fd = open("set_ftrace_filter", O_WRONLY);
>  write(fd, "sched_feat_[^w].*", 7);
>
> Will return an error of EINVAL. When that happens, you should run this
> through the regex exec, and then use the result to find all the matches.
>
> Note, it should be implied that if the regex does not start with "^" and
> end with "$", then that should be added too.
>
>   "sched_feat_[^w].*" turns into "^sched_feat_[^w].*$"
>
> before processing it into the regex.
>
> You may be asking, what if someone wants to add something that is a regex,
> but also acceptable as a glob?
>
> Let's say you want "_*acpi_device_uevent_modalias", where you want:
>
>  "__acpi_device_uevent_modalias" and "acpi_device_uevent_modalias"
>
> But the glob would only select "__acpi_device_uevent_modalias"
>
> To force the regex, the user should just add "^" to the beginning of the
> string.
>
> Thus "_*acpi_device_uevent_modalias" wont work as expected, but
> "^_*acpi_device_uevent_modalias" will.
>
> Make sense?
>
> -- Steve
>