* [PATCH v2] libtracefs: An API to set the filtering of functions @ 2021-03-18 15:30 Sameeruddin shaik 2021-03-18 4:23 ` Tzvetomir Stoyanov 0 siblings, 1 reply; 3+ messages in thread From: Sameeruddin shaik @ 2021-03-18 15:30 UTC (permalink / raw) To: rostedt; +Cc: linux-trace-devel, Sameeruddin shaik This new API will write the function filters into the set_ftrace_filter file. tracefs_function_filter() https://bugzilla.kernel.org/show_bug.cgi?id=210643 Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com> --- Error handling is addressed as per the comments if filters[i] is not NULL, then only we are going inside for loop, at the asprintf i think it won't be necessary to check the filters[i] is NULL or not again, when it throws error, if i am wrong please correct me. diff --git a/include/tracefs.h b/include/tracefs.h index f3eec62..c1f07b0 100644 --- a/include/tracefs.h +++ b/include/tracefs.h @@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); const char *tracefs_option_name(enum tracefs_option_id id); - +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, + const char *module, bool reset, const char ***errs); #endif /* _TRACE_FS_H */ diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c index e2dfc7b..ca6077b 100644 --- a/src/tracefs-tools.c +++ b/src/tracefs-tools.c @@ -18,6 +18,7 @@ #include "tracefs-local.h" #define TRACE_CTRL "tracing_on" +#define TRACE_FILTER "set_ftrace_filter" static const char * const options_map[] = { "unknown", @@ -387,3 +388,108 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt if (options && id > TRACEFS_OPTION_INVALID) options->mask &= ~(1ULL << (id - 1)); } + +static int controlled_write(const char *filter_path, const char **filters, + const char *module, bool reset, const char ***errs) +{ + int flags = reset ? O_TRUNC : O_APPEND; + const char **e = NULL; + char *each_str = NULL; + int write_size = 0; + int size = 0; + int fd = -1; + int ret = 0; + int j = 0; + int i; + + fd = open(filter_path, O_WRONLY | flags); + if (fd < 0) + return 1; + + for (i = 0; filters[i]; i++) { + if (module) + write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module); + else + write_size = asprintf(&each_str, "%s ", filters[i]); + if (write_size < 0) { + ret = 1; + goto error; + } + size = write(fd, each_str, write_size); + /* compare written bytes*/ + if (size < write_size) { + if (errs) { + e = realloc(e, (j + 1) * (sizeof(char *))); + if (!e) { + ret = 1; + goto error; + } + e[j++] = filters[i]; + ret -= 1; + } + } + free(each_str); + each_str = NULL; + } + if (errs) { + e = realloc(e, (j + 1) * (sizeof(char *))); + if (!e) { + free(e); + ret = 1; + goto error; + } + e[j] = NULL; + *errs = e; + } + error: + if (each_str) + free(each_str); + close(fd); + return ret; +} + +/** + * tracefs_function_filter - write to set_ftrace_filter file to trace + * particular functions + * @instance: ftrace instance, can be NULL for top tracing instance + * @filters: An array of function names ending with a NULL pointer + * @module: Module to be traced + * @reset: set to true to reset the file before applying the filter + * @errs: A pointer to array of constant strings that will be allocated + * on negative return of this function, pointing to the filters that + * failed.May be NULL, in which case this field will be ignored. + * + * The @filters is an array of strings, where each string will be used + * to set a function or functions to be traced. + * + * If @reset is true, then all functions in the filter are cleared + * before adding functions from @filters. Otherwise, the functions set + * by @filters will be appended to the filter file + * + * returns -x on filter errors (where x is number of failed filter + * srtings) and if @errs is not NULL will be an allocated string array + * pointing to the strings in @filters that failed and must be freed + * with free(). + * + * returns 1 on general errors not realted to setting the filter. + * @errs is not set even if supplied. + * + * return 0 on success and @errs is not set. + */ +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, + const char *module, bool reset, const char ***errs) +{ + char *ftrace_filter_path; + int ret = 0; + + if (!filters) + return 1; + + ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER); + if (!ftrace_filter_path) + return 1; + + ret = controlled_write(ftrace_filter_path, filters, module, reset, errs); + tracefs_put_tracing_file(ftrace_filter_path); + return ret; +} -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libtracefs: An API to set the filtering of functions 2021-03-18 15:30 [PATCH v2] libtracefs: An API to set the filtering of functions Sameeruddin shaik @ 2021-03-18 4:23 ` Tzvetomir Stoyanov 2021-03-19 16:50 ` sameeruddin shaik 0 siblings, 1 reply; 3+ messages in thread From: Tzvetomir Stoyanov @ 2021-03-18 4:23 UTC (permalink / raw) To: Sameeruddin shaik; +Cc: Steven Rostedt, Linux Trace Devel Hi Sameer, I have only one comment on this version: On Wed, Mar 17, 2021 at 6:02 PM Sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote: > > This new API will write the function filters into the > set_ftrace_filter file. > > tracefs_function_filter() > > https://bugzilla.kernel.org/show_bug.cgi?id=210643 > > Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com> > --- > Error handling is addressed as per the comments > if filters[i] is not NULL, then only we are going inside for loop, at the asprintf i think it won't be necessary to check the filters[i] is NULL or not again, when it throws error, if i am wrong please correct me. > > diff --git a/include/tracefs.h b/include/tracefs.h > index f3eec62..c1f07b0 100644 > --- a/include/tracefs.h > +++ b/include/tracefs.h > @@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o > int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); > int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); > const char *tracefs_option_name(enum tracefs_option_id id); > - > +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, > + const char *module, bool reset, const char ***errs); > #endif /* _TRACE_FS_H */ > diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c > index e2dfc7b..ca6077b 100644 > --- a/src/tracefs-tools.c > +++ b/src/tracefs-tools.c > @@ -18,6 +18,7 @@ > #include "tracefs-local.h" > > #define TRACE_CTRL "tracing_on" > +#define TRACE_FILTER "set_ftrace_filter" > > static const char * const options_map[] = { > "unknown", > @@ -387,3 +388,108 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt > if (options && id > TRACEFS_OPTION_INVALID) > options->mask &= ~(1ULL << (id - 1)); > } > + > +static int controlled_write(const char *filter_path, const char **filters, > + const char *module, bool reset, const char ***errs) > +{ > + int flags = reset ? O_TRUNC : O_APPEND; > + const char **e = NULL; > + char *each_str = NULL; > + int write_size = 0; > + int size = 0; > + int fd = -1; > + int ret = 0; > + int j = 0; > + int i; > + > + fd = open(filter_path, O_WRONLY | flags); > + if (fd < 0) > + return 1; > + > + for (i = 0; filters[i]; i++) { > + if (module) > + write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module); > + else > + write_size = asprintf(&each_str, "%s ", filters[i]); > + if (write_size < 0) { > + ret = 1; > + goto error; > + } > + size = write(fd, each_str, write_size); > + /* compare written bytes*/ > + if (size < write_size) { > + if (errs) { > + e = realloc(e, (j + 1) * (sizeof(char *))); > + if (!e) { The commonly used pattern when working with realloc is to use a temporary pointer for the return value. The problem when using the same pointer is that in case of a realloc() error, it will return NULL which will overwrite our original pointer and we will lose it. The old memory is still valid, realloc() does not free it in case of an error, but we cannot access it as the old pointer is overwritten with that NULL. So, I would suggest something like that: e_new = realloc(e, (j + 1) * (sizeof(char *))); if (!e_new) { free(e); ret = 1; goto error; } else e = e_new; > + ret = 1; > + goto error; > + } > + e[j++] = filters[i]; > + ret -= 1; > + } > + } > + free(each_str); > + each_str = NULL; > + } > + if (errs) { > + e = realloc(e, (j + 1) * (sizeof(char *))); > + if (!e) { > + free(e); The same comment as above. Here "e" is already NULL. Calling free(NULL) will not crash, but will not free the old memory. > + ret = 1; > + goto error; > + } > + e[j] = NULL; > + *errs = e; > + } > + error: > + if (each_str) > + free(each_str); > + close(fd); > + return ret; > +} > + > +/** > + * tracefs_function_filter - write to set_ftrace_filter file to trace > + * particular functions > + * @instance: ftrace instance, can be NULL for top tracing instance > + * @filters: An array of function names ending with a NULL pointer > + * @module: Module to be traced > + * @reset: set to true to reset the file before applying the filter > + * @errs: A pointer to array of constant strings that will be allocated > + * on negative return of this function, pointing to the filters that > + * failed.May be NULL, in which case this field will be ignored. > + * > + * The @filters is an array of strings, where each string will be used > + * to set a function or functions to be traced. > + * > + * If @reset is true, then all functions in the filter are cleared > + * before adding functions from @filters. Otherwise, the functions set > + * by @filters will be appended to the filter file > + * > + * returns -x on filter errors (where x is number of failed filter > + * srtings) and if @errs is not NULL will be an allocated string array > + * pointing to the strings in @filters that failed and must be freed > + * with free(). > + * > + * returns 1 on general errors not realted to setting the filter. > + * @errs is not set even if supplied. > + * > + * return 0 on success and @errs is not set. > + */ > +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, > + const char *module, bool reset, const char ***errs) > +{ > + char *ftrace_filter_path; > + int ret = 0; > + > + if (!filters) > + return 1; > + > + ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER); > + if (!ftrace_filter_path) > + return 1; > + > + ret = controlled_write(ftrace_filter_path, filters, module, reset, errs); > + tracefs_put_tracing_file(ftrace_filter_path); > + return ret; > +} > -- > 2.7.4 > Thanks for sending this version, Sameer! I think we can merge it, when you fix that realloc() error checking. I consider this patch as a good foundation for the API, but we should work on some optimizations on top of it, in separate patches. What else should be added, when this patch is merged: 1. A unit test of the API 2. Man page of the API 3. Optimizations: new kernels support writing indexes instead of strings into the "set_ftrace_filter" file. This is faster, as there is no string comparison in the kernel context. The function index can be retrieved from "available_filter_functions" files - the first function is with index 0, the next is 1 ... and so forth. So, the possible optimization of the API is to check - if the kernel supports indexes, use it, or fail back to the legacy logic with strings. -- Tzvetomir (Ceco) Stoyanov VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libtracefs: An API to set the filtering of functions 2021-03-18 4:23 ` Tzvetomir Stoyanov @ 2021-03-19 16:50 ` sameeruddin shaik 0 siblings, 0 replies; 3+ messages in thread From: sameeruddin shaik @ 2021-03-19 16:50 UTC (permalink / raw) To: Tzvetomir Stoyanov; +Cc: Steven Rostedt, Linux Trace Devel Hi Tzvetomir Stoyanov, Thanks for your support and reviews. It's been a pleasure working with you guys, I am so grateful for your time, effort and reviews. I will try to continue my support. Thanks, sameer On 18/03/21 9:53 am, Tzvetomir Stoyanov wrote: > Hi Sameer, > I have only one comment on this version: > > On Wed, Mar 17, 2021 at 6:02 PM Sameeruddin shaik > <sameeruddin.shaik8@gmail.com> wrote: >> This new API will write the function filters into the >> set_ftrace_filter file. >> >> tracefs_function_filter() >> >> https://bugzilla.kernel.org/show_bug.cgi?id=210643 >> >> Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com> >> --- >> Error handling is addressed as per the comments >> if filters[i] is not NULL, then only we are going inside for loop, at the asprintf i think it won't be necessary to check the filters[i] is NULL or not again, when it throws error, if i am wrong please correct me. >> >> diff --git a/include/tracefs.h b/include/tracefs.h >> index f3eec62..c1f07b0 100644 >> --- a/include/tracefs.h >> +++ b/include/tracefs.h >> @@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o >> int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id); >> int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id); >> const char *tracefs_option_name(enum tracefs_option_id id); >> - >> +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, >> + const char *module, bool reset, const char ***errs); >> #endif /* _TRACE_FS_H */ >> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c >> index e2dfc7b..ca6077b 100644 >> --- a/src/tracefs-tools.c >> +++ b/src/tracefs-tools.c >> @@ -18,6 +18,7 @@ >> #include "tracefs-local.h" >> >> #define TRACE_CTRL "tracing_on" >> +#define TRACE_FILTER "set_ftrace_filter" >> >> static const char * const options_map[] = { >> "unknown", >> @@ -387,3 +388,108 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt >> if (options && id > TRACEFS_OPTION_INVALID) >> options->mask &= ~(1ULL << (id - 1)); >> } >> + >> +static int controlled_write(const char *filter_path, const char **filters, >> + const char *module, bool reset, const char ***errs) >> +{ >> + int flags = reset ? O_TRUNC : O_APPEND; >> + const char **e = NULL; >> + char *each_str = NULL; >> + int write_size = 0; >> + int size = 0; >> + int fd = -1; >> + int ret = 0; >> + int j = 0; >> + int i; >> + >> + fd = open(filter_path, O_WRONLY | flags); >> + if (fd < 0) >> + return 1; >> + >> + for (i = 0; filters[i]; i++) { >> + if (module) >> + write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module); >> + else >> + write_size = asprintf(&each_str, "%s ", filters[i]); >> + if (write_size < 0) { >> + ret = 1; >> + goto error; >> + } >> + size = write(fd, each_str, write_size); >> + /* compare written bytes*/ >> + if (size < write_size) { >> + if (errs) { >> + e = realloc(e, (j + 1) * (sizeof(char *))); >> + if (!e) { > The commonly used pattern when working with realloc is to use a > temporary pointer for the return value. The problem when using the > same pointer is that in case of a realloc() error, it will return NULL > which will overwrite our original pointer and we will lose it. The old > memory is still valid, realloc() does not free it in case of an error, > but we cannot access it as the old pointer is overwritten with that > NULL. So, I would suggest something like that: > e_new = realloc(e, (j + 1) * (sizeof(char *))); > if (!e_new) { > free(e); > ret = 1; > goto error; > } else > e = e_new; > > >> + ret = 1; >> + goto error; >> + } >> + e[j++] = filters[i]; >> + ret -= 1; >> + } >> + } >> + free(each_str); >> + each_str = NULL; >> + } >> + if (errs) { >> + e = realloc(e, (j + 1) * (sizeof(char *))); >> + if (!e) { >> + free(e); > The same comment as above. Here "e" is already NULL. Calling > free(NULL) will not crash, but will not free the old memory. > >> + ret = 1; >> + goto error; >> + } >> + e[j] = NULL; >> + *errs = e; >> + } >> + error: >> + if (each_str) >> + free(each_str); >> + close(fd); >> + return ret; >> +} >> + >> +/** >> + * tracefs_function_filter - write to set_ftrace_filter file to trace >> + * particular functions >> + * @instance: ftrace instance, can be NULL for top tracing instance >> + * @filters: An array of function names ending with a NULL pointer >> + * @module: Module to be traced >> + * @reset: set to true to reset the file before applying the filter >> + * @errs: A pointer to array of constant strings that will be allocated >> + * on negative return of this function, pointing to the filters that >> + * failed.May be NULL, in which case this field will be ignored. >> + * >> + * The @filters is an array of strings, where each string will be used >> + * to set a function or functions to be traced. >> + * >> + * If @reset is true, then all functions in the filter are cleared >> + * before adding functions from @filters. Otherwise, the functions set >> + * by @filters will be appended to the filter file >> + * >> + * returns -x on filter errors (where x is number of failed filter >> + * srtings) and if @errs is not NULL will be an allocated string array >> + * pointing to the strings in @filters that failed and must be freed >> + * with free(). >> + * >> + * returns 1 on general errors not realted to setting the filter. >> + * @errs is not set even if supplied. >> + * >> + * return 0 on success and @errs is not set. >> + */ >> +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, >> + const char *module, bool reset, const char ***errs) >> +{ >> + char *ftrace_filter_path; >> + int ret = 0; >> + >> + if (!filters) >> + return 1; >> + >> + ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER); >> + if (!ftrace_filter_path) >> + return 1; >> + >> + ret = controlled_write(ftrace_filter_path, filters, module, reset, errs); >> + tracefs_put_tracing_file(ftrace_filter_path); >> + return ret; >> +} >> -- >> 2.7.4 >> > Thanks for sending this version, Sameer! > I think we can merge it, when you fix that realloc() error checking. > I consider this patch as a good foundation for the API, but we should > work on some optimizations on top of it, in separate patches. What > else should be added, when this patch is merged: > 1. A unit test of the API > 2. Man page of the API > 3. Optimizations: new kernels support writing indexes instead of > strings into the "set_ftrace_filter" file. This is faster, as there is > no string comparison in the kernel context. The function index can be > retrieved from "available_filter_functions" files - the first function > is with index 0, the next is 1 ... and so forth. So, the possible > optimization of the API is to check - if the kernel supports indexes, > use it, or fail back to the legacy logic with strings. > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-18 16:50 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-18 15:30 [PATCH v2] libtracefs: An API to set the filtering of functions Sameeruddin shaik 2021-03-18 4:23 ` Tzvetomir Stoyanov 2021-03-19 16:50 ` sameeruddin shaik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.