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@xxxxxxxxx> wrote:
>
> Hi Sammer,
>
> On Mon, Feb 8, 2021 at 3:17 AM Sameeruddin Shaik
> <sameeruddin.shaik8@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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
|