Tzvetomir,
Thanks for your review, I will address your comments in the next patch.
steve,
Do you have any other comments on this patch?
Thanks,
sameer.
On Tue, Mar 16, 2021 at 5:07 PM Tzvetomir Stoyanov
<tz.stoyanov@xxxxxxxxx> wrote:
>
> Hi Sameer,
> I think we are almost there, I have a few comments on the error
> handling, that should be addressed before merging the patch.
>
> On Mon, Mar 15, 2021 at 7:49 PM Sameeruddin shaik
> <sameeruddin.shaik8@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > changed snprintf to asprintf for string formation
> > handled the lines over 80 characters
> >
> > 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..6b33321 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,93 @@ 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)
> > + continue;
>
> I think we can consider asprintf() failure as a major problem. There
> are two options - break the loop or goto error label.
>
> > + size = write(fd, each_str, write_size);
> > + /* compare written bytes*/
> > + if (size < write_size) {
> > + if (errs) {
> > + e = realloc(e, (j + 1) * (sizeof(char *)));
>
> The realloc return value must be checked. In case of an error - break
> the loop or goto error label.
>
> > + e[j++] = filters[i];
> > + ret -= 1;
> > + }
> > + }
> > + free(each_str);
>
> Set each_str to NULL after the free, This will help with proper
> resources cleanup, in case of an error.
>
> > + }
>
> If you choose the option to break the loop in case of an error, check
> here if filters[i] is not NULL, i.e. if the loop exit with an error.
> If there was an error - clean up all internally allocated resources
> (fd, e, each_str) and return error.
> The other option is to add "error" or "out" label at the end of the
> function where to do this resource cleanups.
>
> > + if (errs) {
> > + e = realloc(e, (j + 1) * (sizeof(char *)));
>
> Check that realloc return value too.
>
> > + e[j] = NULL;
> > + *errs = e;
> > + }
> > + close(fd);
>
> Note: in case of an internal error exit, free e and each_str, if they
> were allocated, and do not modify err.
>
> > + 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.
> Note: in case of an error exit, free e and each_str, if they were allocated
> > + */
> > +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, Sameer!
>
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center
|