* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions [not found] <CAK7tX=angRdxNdg2yNnss-NuBm+i-Y3unyT6BaeY0Quj-QB+UA@mail.gmail.com> @ 2021-02-09 5:52 ` Tzvetomir Stoyanov 2021-02-09 6:38 ` Sameeruddin Shaik 2021-02-09 16:58 ` Steven Rostedt 0 siblings, 2 replies; 7+ messages in thread From: Tzvetomir Stoyanov @ 2021-02-09 5:52 UTC (permalink / raw) To: Sameeruddin Shaik; +Cc: Steven Rostedt, Linux Trace Devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-09 5:52 ` [PATCH][WIP]libtracefs:New API to enable the filtering of functions Tzvetomir Stoyanov @ 2021-02-09 6:38 ` Sameeruddin Shaik 2021-02-09 14:41 ` Tzvetomir Stoyanov 2021-02-09 16:58 ` Steven Rostedt 1 sibling, 1 reply; 7+ messages in thread From: Sameeruddin Shaik @ 2021-02-09 6:38 UTC (permalink / raw) To: Tzvetomir Stoyanov; +Cc: Steven Rostedt, Linux Trace Devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-09 6:38 ` Sameeruddin Shaik @ 2021-02-09 14:41 ` Tzvetomir Stoyanov 0 siblings, 0 replies; 7+ messages in thread From: Tzvetomir Stoyanov @ 2021-02-09 14:41 UTC (permalink / raw) To: Sameeruddin Shaik; +Cc: Steven Rostedt, Linux Trace Devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-09 5:52 ` [PATCH][WIP]libtracefs:New API to enable the filtering of functions Tzvetomir Stoyanov 2021-02-09 6:38 ` Sameeruddin Shaik @ 2021-02-09 16:58 ` Steven Rostedt 2021-02-11 3:03 ` Sameeruddin Shaik 1 sibling, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2021-02-09 16:58 UTC (permalink / raw) To: Tzvetomir Stoyanov; +Cc: Sameeruddin Shaik, Linux Trace Devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-09 16:58 ` Steven Rostedt @ 2021-02-11 3:03 ` Sameeruddin Shaik 2021-02-10 15:38 ` Steven Rostedt 0 siblings, 1 reply; 7+ messages in thread From: Sameeruddin Shaik @ 2021-02-11 3:03 UTC (permalink / raw) To: Steven Rostedt; +Cc: Tzvetomir Stoyanov (VMware), Linux Trace Devel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-11 3:03 ` Sameeruddin Shaik @ 2021-02-10 15:38 ` Steven Rostedt 2021-02-12 3:09 ` Sameeruddin Shaik 0 siblings, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2021-02-10 15:38 UTC (permalink / raw) To: Sameeruddin Shaik; +Cc: Tzvetomir Stoyanov (VMware), Linux Trace Devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][WIP]libtracefs:New API to enable the filtering of functions 2021-02-10 15:38 ` Steven Rostedt @ 2021-02-12 3:09 ` Sameeruddin Shaik 0 siblings, 0 replies; 7+ messages in thread From: Sameeruddin Shaik @ 2021-02-12 3:09 UTC (permalink / raw) To: Steven Rostedt; +Cc: Tzvetomir Stoyanov (VMware), Linux Trace Devel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-11 3:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAK7tX=angRdxNdg2yNnss-NuBm+i-Y3unyT6BaeY0Quj-QB+UA@mail.gmail.com> 2021-02-09 5:52 ` [PATCH][WIP]libtracefs:New API to enable the filtering of functions Tzvetomir Stoyanov 2021-02-09 6:38 ` Sameeruddin Shaik 2021-02-09 14:41 ` Tzvetomir Stoyanov 2021-02-09 16:58 ` Steven Rostedt 2021-02-11 3:03 ` Sameeruddin Shaik 2021-02-10 15:38 ` Steven Rostedt 2021-02-12 3:09 ` Sameeruddin Shaik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).