Re: [PATCH] libtracefs: An API to set the filtering of functions

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Tue,  2 Mar 2021 22:45:10 +0530
Sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote:

> This new API will write the function filters into the
> set_ftrace_filter file, it will write only string as of now, it can't
> handle kernel glob or regular expressions.

The limitation of no glob or regular expressions is fine. We can add that
in future patches.

> 
> tracefs_function_filter()
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=210643
> 
> Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..b5259f9 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -50,6 +50,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 * const *filters, const char *module, bool reset);
>  
>  /**
>   * 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 e2dfc7b..b8d8c99 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,85 @@ 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 * const *filters, const char *module, bool reset)
> +{
> +	int write_size;
> +	char *each_str;
> +	int size = 0;
> +	int slen;
> +	int fd;
> +	int i;
> +
> +

Only need one empty line between declaration and code.

> +	if (!filters)
> +		return -1;
> +	if (reset)
> +		fd = open(filter_path, O_WRONLY | O_TRUNC);
> +	else
> +		fd = open(filter_path, O_WRONLY | O_APPEND);

It's usually more robust to have a system call parameter set by an if
statement and only call the system call once.

	int flags;

	flags = reset ? O_TRUNC : O_APPEND;

Although O_APPEND isn't really needed here (zero would work), but it's fine
to have.

	fd = open(filter_path, O_WRONLY | flags);


> +	if (fd < 0)
> +		return -1;

I would add a blank line between the check and for loop, to space out the
logic a bit better.

	fd = open(filter_path, O_WRONLY | flags);
	if (fd < 0)
		return -1;

	for (i = 0; filters[i]; i++)

No space between setting the fd and checking it. It gives a visual on how
code is related.

> +	for (i = 0; filters[i] != NULL ; i++) {

No need for the " != NULL"


> +		slen = 0;
> +		slen = strlen(*(filters + i));

No need for slen = 0 as you set it immediately afterward, also, it is more
appropriate to use:

		slen = strlen(filters[i]);


> +		if (slen < 0)

The above can't happen, strlen always returns a positive integer (or zero).
Now checking for zero would make sense.

		if (!slen)
> +			continue;


> +
> +		if (module)
> +			slen += strlen(module) + 5;

What's the "+ 5" for? Should be commented.

> +		/* Adding 2 extra byte for the space and '\0' at the end*/
> +		slen += 2;
> +		each_str = calloc(1, slen);
> +		if (!each_str)
> +			return -1;
> +		if (module)
> +			write_size = snprintf(each_str, slen, "%s:mod:%s ", *(filters + i), module);
> +		else
> +			write_size = snprintf(each_str, slen, "%s ", *(filters + i));
> +		if (write_size < (slen - 1)) {

This should never happen. If it does, then there's something wrong with
this code, not the input.


> +			free(each_str);
> +			continue;
> +		}
> +		size += write(fd, each_str, write_size);

Need to check errors here, and we need a way to report that an error
happened for a string. Perhaps the API also needs to have an error message
pointer that is passed in? Or a bitmask that lets the user know which
filter failed?

Tzvetomir, have any ideas on how to report back to the caller which filter
failed? Could just send back an array of strings of the filters that failed.

	int tracefs_function_filter(struct tracefs_instance *instance,
				    const char * const * filters,
				    const char * module, bool reset,
				    const char * const ** errs);

Where if errs points to a pointer, that pointer gets updated with an array.

	const char **filters = { "func1", "func2", func3" };
	const char **errs;
	ret;

	ret = tracefs_function_filter(NULL, filters, NULL, &errs);

If "func1" and "func3" fail, then ret is < 0 (-2 for number of failures?)
and errs would be:

	errs = { &filter[0], &filters[2], NULL };

	and the users would need to free it as well.

	free(errs);

if ret is zero, then errs would not be touched, and the caller does not
need to do anything with it.

Note, if the user doesn't care about errors, errs could be NULL, in which
case the calling function would obviously not set it.



> +		free(each_str);
> +	}
> +	close(fd);
> +	return size;
> +}
> +
> +/**
> + * tracefs_function_filter - write to set_ftrace_filter file to trace particular functions
> + * @instance: ftrace instance, can be NULL for top tracing instance
> + * @filter: An array of function names ending with a NULL pointer
> + * @module: Module Name to be traced
> + * @reset: set to true to reset the file before applying the filter
> + *
> + * The @filter is an array of strings, where each string will be use 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 @filter. Otherwise, the functions set by @filter
> + * will be appended to the filter
> + *
> + * Returns the number of bytes written into the filter file or -1 if
> + * there is any error in writing to filter file
> + */
> +int tracefs_function_filter(struct tracefs_instance *instance, const char * const *filters, const char *module, bool reset)
> +{
> +	char *ftrace_filter_path;
> +	int ret;
> +
> +	if (!filters)
> +		return -1;

You can add a empty line here.

> +	ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> +
> +	if (!ftrace_filter_path)
> +		goto gracefully_free;

It should just be "goto out".

> +
> +	ret = controlled_write(ftrace_filter_path, filters, module, reset);
> +
> + gracefully_free:
> +	tracefs_put_tracing_file(ftrace_filter_path);
> +	return ret;

Note, you need to initialize ret to -1, otherwise it is undefined if you
do the "goto out".

-- Steve

> +}

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Wed, 3 Mar 2021 06:46:26 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> what if we store the indices of the failed filters in an integer array
> and return them back?

There's not much difference if we return an array of pointers to the
filters or an array of integers to the index. I was just thinking about
how I would use the interface. When having a working interface, we
should write a few robust programs to see how easy it is to use, and
that will help in making the API appropriate. This needs to be done
*before* we accept it. This particular API is going to be widely used,
and it needs to be simple and robust.

> let's return the number of bytes written, also we will calculate the
> complete filters length and return it, if there is difference,
> we will loop into the integer array and print the erroneous filters

Not sure how that is helpful. How would you use the number of bytes
written?

> 
> Let's fix the number of parameters to this function:)

Not sure what you mean by that.


Here's how I envision this interface.

	char **errs;
	char *filters[] = {
		"sched*", "spin_*", NULL
	};
	int ret;


	ret = tracefs_function_filter(NULL, filters, NULL, &errs);
	if (ret < 0) {
		int i;

		printf("Failed to apply: ");
		for (i = 0; errs[i]; i++) {
			if (i)
				printf(", ");
			printf("'%s'", errs[i]);
		}
		printf("\n";
		exit(ret);
	}

-- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Tzvetomir Stoyanov]

On Mon, Mar 1, 2021 at 10:40 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue,  2 Mar 2021 22:45:10 +0530
> Sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > This new API will write the function filters into the
> > set_ftrace_filter file, it will write only string as of now, it can't
> > handle kernel glob or regular expressions.
>
> The limitation of no glob or regular expressions is fine. We can add that
> in future patches.
>
> >
> > tracefs_function_filter()
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=210643
> >
> > Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
> >
[ ... ]
>
> > +                     free(each_str);
> > +                     continue;
> > +             }
> > +             size += write(fd, each_str, write_size);
>
> Need to check errors here, and we need a way to report that an error
> happened for a string. Perhaps the API also needs to have an error message
> pointer that is passed in? Or a bitmask that lets the user know which
> filter failed?
>
> Tzvetomir, have any ideas on how to report back to the caller which filter
> failed? Could just send back an array of strings of the filters that failed.

It is very useful to have a way to report back the failed filters.
Using an array
of strings will work for this API, but I was thinking somehow to leverage the
error_log file by the ftrace itself. Currently it does not report any
error, just
returns EINVAL. In more complex filters it would be useful to log
more detailed description of the problem in the error_log file.

[ ... ]


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

> Let's fix the number of parameters to this function:)

>> Not sure what you mean by that.
Actually i meant this ""int tracefs_function_filter(struct
tracefs_instance *instance,
                                    const char * const * filters,
                                    const char * module, bool reset,
                                    const char * const ** errs);""
For every patch, a parameter is increasing in this API.

> let's return the number of bytes written, also we will calculate the
> complete filters length and return it, if there is difference,
> we will loop into the integer array and print the erroneous filters

>>Not sure how that is helpful. How would you use the number of bytes
>>written?

We will return the number of bytes written and we also store the total
length of strings
in filters array, in one pointer variable, we will check the
difference between bytes written
and the total length of the strings, if there is difference we will
print failed filters otherwise
we will not print anything.

>It is very useful to have a way to report back the failed filters.
>Using an array
>of strings will work for this API, but I was thinking somehow to leverage the
>error_log file by the ftrace itself. Currently it does not report any
>error, just
>returns EINVAL. In more complex filters it would be useful to log
>more detailed description of the problem in the error_log file.

This error_log is also a good idea.

If possible let's have a live discussion on this API,so that we can
discuss the corner cases in the design
more efficiently and we can close it ASAP.

Thanks,
sameer shaik.

Thanks,
sameer shaik


On Tue, Mar 2, 2021 at 9:52 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> On Mon, Mar 1, 2021 at 10:40 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue,  2 Mar 2021 22:45:10 +0530
> > Sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote:
> >
> > > This new API will write the function filters into the
> > > set_ftrace_filter file, it will write only string as of now, it can't
> > > handle kernel glob or regular expressions.
> >
> > The limitation of no glob or regular expressions is fine. We can add that
> > in future patches.
> >
> > >
> > > tracefs_function_filter()
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=210643
> > >
> > > Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
> > >
> [ ... ]
> >
> > > +                     free(each_str);
> > > +                     continue;
> > > +             }
> > > +             size += write(fd, each_str, write_size);
> >
> > Need to check errors here, and we need a way to report that an error
> > happened for a string. Perhaps the API also needs to have an error message
> > pointer that is passed in? Or a bitmask that lets the user know which
> > filter failed?
> >
> > Tzvetomir, have any ideas on how to report back to the caller which filter
> > failed? Could just send back an array of strings of the filters that failed.
>
> It is very useful to have a way to report back the failed filters.
> Using an array
> of strings will work for this API, but I was thinking somehow to leverage the
> error_log file by the ftrace itself. Currently it does not report any
> error, just
> returns EINVAL. In more complex filters it would be useful to log
> more detailed description of the problem in the error_log file.
>
> [ ... ]
>
>
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Tue, 2 Mar 2021 10:44:14 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> > Let's fix the number of parameters to this function:)  
> 
> >> Not sure what you mean by that.  
> Actually i meant this ""int tracefs_function_filter(struct
> tracefs_instance *instance,
>                                     const char * const * filters,
>                                     const char * module, bool reset,
>                                     const char * const ** errs);""
> For every patch, a parameter is increasing in this API.

And that's a normal approach to developing APIs. I've been doing this
for over 25 years, there's nothing special about this case. In
discussion on APIs, parameters grow to handle new cases. That's par for
the course. There's only 5 parameters, not too many.

> 
> > let's return the number of bytes written, also we will calculate the
> > complete filters length and return it, if there is difference,
> > we will loop into the integer array and print the erroneous filters  
> 
> >>Not sure how that is helpful. How would you use the number of bytes
> >>written?  
> 
> We will return the number of bytes written and we also store the total
> length of strings
> in filters array, in one pointer variable, we will check the
> difference between bytes written
> and the total length of the strings, if there is difference we will
> print failed filters otherwise
> we will not print anything.

Please show an example of a use case that you would use this with?

I gave you an example of how I intend on using it, and the user of this
interface should not care about number of bytes written. And the
interface should not be printing any error messages, it is a library,
error messages are for applications to produce. The interface must give
the application enough information to be able to produce it.


> 
> >It is very useful to have a way to report back the failed filters.
> >Using an array
> >of strings will work for this API, but I was thinking somehow to leverage the
> >error_log file by the ftrace itself. Currently it does not report any
> >error, just
> >returns EINVAL. In more complex filters it would be useful to log
> >more detailed description of the problem in the error_log file.  
> 
> This error_log is also a good idea.

It's actually not very useful for this interface. The only error that
it would give you is that it could not find any functions that match a
filter.

When we create other interfaces that do more than just setting
functions (like setting triggers) then we can return to looking at the
error log. But since the kernel does not produce anything in the error
log at the moment, it must be updated. And kernels take about a year or
two after a change to get into distributions. That means, this library
can't rely on it being there, and still needs a way to inform the
application of errors.

> 
> If possible let's have a live discussion on this API,so that we can
> discuss the corner cases in the design
> more efficiently and we can close it ASAP.
> 

I have a very good idea of what I want from this interface, which is
why I created the bugzilla about it. If you want to implement it
different than my idea, please show code examples of your use cases.

-- Steve

[PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin shaik]

This new API will write the function filters into the
set_ftrace_filter file, it will write only string as of now, it can't
handle kernel glob or regular expressions.

tracefs_function_filter()

https://bugzilla.kernel.org/show_bug.cgi?id=210643

Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>

diff --git a/include/tracefs.h b/include/tracefs.h
index f3eec62..b5259f9 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -50,6 +50,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 * const *filters, const char *module, bool reset);
 
 /**
  * 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 e2dfc7b..b8d8c99 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,85 @@ 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 * const *filters, const char *module, bool reset)
+{
+	int write_size;
+	char *each_str;
+	int size = 0;
+	int slen;
+	int fd;
+	int i;
+
+
+	if (!filters)
+		return -1;
+	if (reset)
+		fd = open(filter_path, O_WRONLY | O_TRUNC);
+	else
+		fd = open(filter_path, O_WRONLY | O_APPEND);
+	if (fd < 0)
+		return -1;
+	for (i = 0; filters[i] != NULL ; i++) {
+		slen = 0;
+		slen = strlen(*(filters + i));
+		if (slen < 0)
+			continue;
+
+		if (module)
+			slen += strlen(module) + 5;
+		/* Adding 2 extra byte for the space and '\0' at the end*/
+		slen += 2;
+		each_str = calloc(1, slen);
+		if (!each_str)
+			return -1;
+		if (module)
+			write_size = snprintf(each_str, slen, "%s:mod:%s ", *(filters + i), module);
+		else
+			write_size = snprintf(each_str, slen, "%s ", *(filters + i));
+		if (write_size < (slen - 1)) {
+			free(each_str);
+			continue;
+		}
+		size += write(fd, each_str, write_size);
+		free(each_str);
+	}
+	close(fd);
+	return size;
+}
+
+/**
+ * tracefs_function_filter - write to set_ftrace_filter file to trace particular functions
+ * @instance: ftrace instance, can be NULL for top tracing instance
+ * @filter: An array of function names ending with a NULL pointer
+ * @module: Module Name to be traced
+ * @reset: set to true to reset the file before applying the filter
+ *
+ * The @filter is an array of strings, where each string will be use 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 @filter. Otherwise, the functions set by @filter
+ * will be appended to the filter
+ *
+ * Returns the number of bytes written into the filter file or -1 if
+ * there is any error in writing to filter file
+ */
+int tracefs_function_filter(struct tracefs_instance *instance, const char * const *filters, const char *module, bool reset)
+{
+	char *ftrace_filter_path;
+	int ret;
+
+	if (!filters)
+		return -1;
+	ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
+
+	if (!ftrace_filter_path)
+		goto gracefully_free;
+
+	ret = controlled_write(ftrace_filter_path, filters, module, reset);
+
+ gracefully_free:
+	tracefs_put_tracing_file(ftrace_filter_path);
+	return ret;
+}
-- 
2.7.4

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

what if we store the indices of the failed filters in an integer array
and return them back?
let's return the number of bytes written, also we will calculate the
complete filters length and return it, if there is difference,
we will loop into the integer array and print the erroneous filters

Let's fix the number of parameters to this function:)
Thanks,
sameer.



On Mon, Mar 1, 2021 at 11:47 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue,  2 Mar 2021 22:45:10 +0530
> Sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > This new API will write the function filters into the
> > set_ftrace_filter file, it will write only string as of now, it can't
> > handle kernel glob or regular expressions.
>
> The limitation of no glob or regular expressions is fine. We can add that
> in future patches.
>
> >
> > tracefs_function_filter()
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=210643
> >
> > Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
> >
> > diff --git a/include/tracefs.h b/include/tracefs.h
> > index f3eec62..b5259f9 100644
> > --- a/include/tracefs.h
> > +++ b/include/tracefs.h
> > @@ -50,6 +50,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 * const *filters, const char *module, bool reset);
> >
> >  /**
> >   * 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 e2dfc7b..b8d8c99 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,85 @@ 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 * const *filters, const char *module, bool reset)
> > +{
> > +     int write_size;
> > +     char *each_str;
> > +     int size = 0;
> > +     int slen;
> > +     int fd;
> > +     int i;
> > +
> > +
>
> Only need one empty line between declaration and code.
>
> > +     if (!filters)
> > +             return -1;
> > +     if (reset)
> > +             fd = open(filter_path, O_WRONLY | O_TRUNC);
> > +     else
> > +             fd = open(filter_path, O_WRONLY | O_APPEND);
>
> It's usually more robust to have a system call parameter set by an if
> statement and only call the system call once.
>
>         int flags;
>
>         flags = reset ? O_TRUNC : O_APPEND;
>
> Although O_APPEND isn't really needed here (zero would work), but it's fine
> to have.
>
>         fd = open(filter_path, O_WRONLY | flags);
>
>
> > +     if (fd < 0)
> > +             return -1;
>
> I would add a blank line between the check and for loop, to space out the
> logic a bit better.
>
>         fd = open(filter_path, O_WRONLY | flags);
>         if (fd < 0)
>                 return -1;
>
>         for (i = 0; filters[i]; i++)
>
> No space between setting the fd and checking it. It gives a visual on how
> code is related.
>
> > +     for (i = 0; filters[i] != NULL ; i++) {
>
> No need for the " != NULL"
>
>
> > +             slen = 0;
> > +             slen = strlen(*(filters + i));
>
> No need for slen = 0 as you set it immediately afterward, also, it is more
> appropriate to use:
>
>                 slen = strlen(filters[i]);
>
>
> > +             if (slen < 0)
>
> The above can't happen, strlen always returns a positive integer (or zero).
> Now checking for zero would make sense.
>
>                 if (!slen)
> > +                     continue;
>
>
> > +
> > +             if (module)
> > +                     slen += strlen(module) + 5;
>
> What's the "+ 5" for? Should be commented.
>
> > +             /* Adding 2 extra byte for the space and '\0' at the end*/
> > +             slen += 2;
> > +             each_str = calloc(1, slen);
> > +             if (!each_str)
> > +                     return -1;
> > +             if (module)
> > +                     write_size = snprintf(each_str, slen, "%s:mod:%s ", *(filters + i), module);
> > +             else
> > +                     write_size = snprintf(each_str, slen, "%s ", *(filters + i));
> > +             if (write_size < (slen - 1)) {
>
> This should never happen. If it does, then there's something wrong with
> this code, not the input.
>
>
> > +                     free(each_str);
> > +                     continue;
> > +             }
> > +             size += write(fd, each_str, write_size);
>
> Need to check errors here, and we need a way to report that an error
> happened for a string. Perhaps the API also needs to have an error message
> pointer that is passed in? Or a bitmask that lets the user know which
> filter failed?
>
> Tzvetomir, have any ideas on how to report back to the caller which filter
> failed? Could just send back an array of strings of the filters that failed.
>
>         int tracefs_function_filter(struct tracefs_instance *instance,
>                                     const char * const * filters,
>                                     const char * module, bool reset,
>                                     const char * const ** errs);
>
> Where if errs points to a pointer, that pointer gets updated with an array.
>
>         const char **filters = { "func1", "func2", func3" };
>         const char **errs;
>         ret;
>
>         ret = tracefs_function_filter(NULL, filters, NULL, &errs);
>
> If "func1" and "func3" fail, then ret is < 0 (-2 for number of failures?)
> and errs would be:
>
>         errs = { &filter[0], &filters[2], NULL };
>
>         and the users would need to free it as well.
>
>         free(errs);
>
> if ret is zero, then errs would not be touched, and the caller does not
> need to do anything with it.
>
> Note, if the user doesn't care about errors, errs could be NULL, in which
> case the calling function would obviously not set it.
>
>
>
> > +             free(each_str);
> > +     }
> > +     close(fd);
> > +     return size;
> > +}
> > +
> > +/**
> > + * tracefs_function_filter - write to set_ftrace_filter file to trace particular functions
> > + * @instance: ftrace instance, can be NULL for top tracing instance
> > + * @filter: An array of function names ending with a NULL pointer
> > + * @module: Module Name to be traced
> > + * @reset: set to true to reset the file before applying the filter
> > + *
> > + * The @filter is an array of strings, where each string will be use 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 @filter. Otherwise, the functions set by @filter
> > + * will be appended to the filter
> > + *
> > + * Returns the number of bytes written into the filter file or -1 if
> > + * there is any error in writing to filter file
> > + */
> > +int tracefs_function_filter(struct tracefs_instance *instance, const char * const *filters, const char *module, bool reset)
> > +{
> > +     char *ftrace_filter_path;
> > +     int ret;
> > +
> > +     if (!filters)
> > +             return -1;
>
> You can add a empty line here.
>
> > +     ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> > +
> > +     if (!ftrace_filter_path)
> > +             goto gracefully_free;
>
> It should just be "goto out".
>
> > +
> > +     ret = controlled_write(ftrace_filter_path, filters, module, reset);
> > +
> > + gracefully_free:
> > +     tracefs_put_tracing_file(ftrace_filter_path);
> > +     return ret;
>
> Note, you need to initialize ret to -1, otherwise it is undefined if you
> do the "goto out".
>
> -- Steve
>
> > +}
>

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Tzvetomir Stoyanov]

On Thu, Mar 4, 2021 at 9:24 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 3 Mar 2021 06:46:26 +0530
> Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > what if we store the indices of the failed filters in an integer array
> > and return them back?
>
> There's not much difference if we return an array of pointers to the
> filters or an array of integers to the index. I was just thinking about
> how I would use the interface. When having a working interface, we
> should write a few robust programs to see how easy it is to use, and
> that will help in making the API appropriate. This needs to be done
> *before* we accept it. This particular API is going to be widely used,
> and it needs to be simple and robust.

One remark, not directly related to this discussion. When the implementation
of the API is ready, there should be a unit test (in a separate patch) - as for
any of the other APIs. Usually these are the first use cases that I write for
the new APIs.
Sameer, please look at utest directory where the unit tests are, each API has
a unit test there. We use the cunit framework, ask if there are questions about
it. This should be the next step, when the final version of the
implementation is
ready.
Thanks!

>
> > let's return the number of bytes written, also we will calculate the
> > complete filters length and return it, if there is difference,
> > we will loop into the integer array and print the erroneous filters
>
> Not sure how that is helpful. How would you use the number of bytes
> written?
>
> >
> > Let's fix the number of parameters to this function:)
>
> Not sure what you mean by that.
>
>
> Here's how I envision this interface.
>
>         char **errs;
>         char *filters[] = {
>                 "sched*", "spin_*", NULL
>         };
>         int ret;
>
>
>         ret = tracefs_function_filter(NULL, filters, NULL, &errs);
>         if (ret < 0) {
>                 int i;
>
>                 printf("Failed to apply: ");
>                 for (i = 0; errs[i]; i++) {
>                         if (i)
>                                 printf(", ");
>                         printf("'%s'", errs[i]);
>                 }
>                 printf("\n";
>                 exit(ret);
>         }
>
> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

sure tzvetomir.

On Thu, Mar 4, 2021 at 2:29 PM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> On Thu, Mar 4, 2021 at 9:24 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed, 3 Mar 2021 06:46:26 +0530
> > Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:
> >
> > > what if we store the indices of the failed filters in an integer array
> > > and return them back?
> >
> > There's not much difference if we return an array of pointers to the
> > filters or an array of integers to the index. I was just thinking about
> > how I would use the interface. When having a working interface, we
> > should write a few robust programs to see how easy it is to use, and
> > that will help in making the API appropriate. This needs to be done
> > *before* we accept it. This particular API is going to be widely used,
> > and it needs to be simple and robust.
>
> One remark, not directly related to this discussion. When the implementation
> of the API is ready, there should be a unit test (in a separate patch) - as for
> any of the other APIs. Usually these are the first use cases that I write for
> the new APIs.
> Sameer, please look at utest directory where the unit tests are, each API has
> a unit test there. We use the cunit framework, ask if there are questions about
> it. This should be the next step, when the final version of the
> implementation is
> ready.
> Thanks!
>
> >
> > > let's return the number of bytes written, also we will calculate the
> > > complete filters length and return it, if there is difference,
> > > we will loop into the integer array and print the erroneous filters
> >
> > Not sure how that is helpful. How would you use the number of bytes
> > written?
> >
> > >
> > > Let's fix the number of parameters to this function:)
> >
> > Not sure what you mean by that.
> >
> >
> > Here's how I envision this interface.
> >
> >         char **errs;
> >         char *filters[] = {
> >                 "sched*", "spin_*", NULL
> >         };
> >         int ret;
> >
> >
> >         ret = tracefs_function_filter(NULL, filters, NULL, &errs);
> >         if (ret < 0) {
> >                 int i;
> >
> >                 printf("Failed to apply: ");
> >                 for (i = 0; errs[i]; i++) {
> >                         if (i)
> >                                 printf(", ");
> >                         printf("'%s'", errs[i]);
> >                 }
> >                 printf("\n";
> >                 exit(ret);
> >         }
> >
> > -- Steve
>
>
>
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

This is my test code, I tried handling the triple pointers in the library,
but when it comes to user program, at first "errs" has NULL at the
end(checked it using the gdb), but when we try to dereference and
print, gradually position holding NULL in "errs" is getting assinged
to some other location, and i am getting the segmentation fault.

>Since a triple pointer is difficult to manage in the code, you could have:
>
>        const char **e = NULL;
>
>
>               if (errs) {
>                        e = realloc(sizeof(*e), j + 1);
>                        e[j++] = filters[i];
>                }
>
>Then at the end:
>
>        if (errs)
>                *errs = e;

i didn't got the clear picture of what, the above code snippet is doing.
particularly the below line

>e = realloc(sizeof(*e), j + 1);

From man page i got

void *realloc(void *ptr, size_t size);

could you please brief me with an example?

Subject: [PATCH] test: Test code
diff --git a/test.c b/test.c
index d38fc92..3e90711 100644
--- a/test.c
+++ b/test.c
@@ -1,7 +1,39 @@
 #include <tracefs.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+
+
+#define INST "dummy"
+
+const char *filters[] = {"kvm_pmu_reset", "kvm_pmu_init",
"dir_item_err", "tzvetomir", "sameer", "steve", NULL};

 int main()
 {
-    tracefs_tracing_dir();
+    struct tracefs_instance *instance = NULL;
+    const char ***errs;
+    int ret;
+    int fd;
+    int n;
+    int i = 0;
+
+    errs = malloc(sizeof(char *));
+    printf("%s\n",tracefs_tracing_dir());
+    instance = tracefs_instance_create(INST);
+    ret = tracefs_function_filter(instance, filters, "kvm", 1, errs);
+
+    if(ret < -1)
+        if (errs)
+            while (errs[i])
+                printf("%s\n", *errs[i++]);
+
+    getchar();
+    tracefs_instance_destroy(instance);
+    tracefs_instance_free(instance);
+    free(errs);
     return 0;
 }

On Tue, Mar 9, 2021 at 9:51 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>
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..a2249d0 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -50,6 +50,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 **filters, const char *module, bool reset, const char ***errs);
>
>  /**
>   * 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 e2dfc7b..4e168df 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,96 @@ 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;
> +       int write_size;
> +       char *each_str;
> +       int ret = 0;
> +       int j = 0;
> +       int size;
> +       int slen;
> +       int fd;
> +       int i;
> +
> +       fd = open(filter_path, O_WRONLY | flags);
> +       if (fd < 0)
> +               return 1;
> +
> +       for (i = 0; filters[i]; i++) {
> +               slen = strlen(filters[i]);
> +               if (!slen)
> +                       continue;
> +
> +               if (module)
> +                       /* Adding 5 extra bytes for ":mod:"*/
> +                       slen += strlen(module) + 5;
> +
> +               /* Adding 2 extra byte for the space and '\0' at the end*/
> +               slen += 2;
> +               each_str = calloc(1, slen);
> +               if (!each_str)
> +                       return 1;
> +               if (module)
> +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> +               else
> +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> +
> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes*/
> +               if (size < write_size) {
> +                       if (errs) {
> +                               errs[j++] = &filters[i];
> +                               ret -= 1;
> +                       }
> +               }
> +               free(each_str);
> +       }
> +       errs[j] = NULL;
> +       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
>

[PATCH] libtracefs: An API to set the filtering of functions

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

diff --git a/include/tracefs.h b/include/tracefs.h
index f3eec62..a2249d0 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -50,6 +50,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 **filters, const char *module, bool reset, const char ***errs);
 
 /**
  * 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 e2dfc7b..4e168df 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,96 @@ 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;
+	int write_size;
+	char *each_str;
+	int ret = 0;
+	int j = 0;
+	int size;
+	int slen;
+	int fd;
+	int i;
+
+	fd = open(filter_path, O_WRONLY | flags);
+	if (fd < 0)
+		return 1;
+
+	for (i = 0; filters[i]; i++) {
+		slen = strlen(filters[i]);
+		if (!slen)
+			continue;
+
+		if (module)
+			/* Adding 5 extra bytes for ":mod:"*/
+			slen += strlen(module) + 5;
+
+		/* Adding 2 extra byte for the space and '\0' at the end*/
+		slen += 2;
+		each_str = calloc(1, slen);
+		if (!each_str)
+			return 1;
+		if (module)
+			write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
+		else
+			write_size = snprintf(each_str, slen, "%s ", filters[i]);
+
+		size = write(fd, each_str, write_size);
+		/* compare written bytes*/
+		if (size < write_size) {
+			if (errs) {
+				errs[j++] = &filters[i];
+				ret -= 1;
+			}
+		}
+		free(each_str);
+	}
+	errs[j] = NULL;
+	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

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Tzvetomir Stoyanov]

On Tue, Mar 9, 2021 at 6:51 PM Sameeruddin Shaik
<sameeruddin.shaik8@gmail.com> wrote:
>
> This is my test code, I tried handling the triple pointers in the library,
> but when it comes to user program, at first "errs" has NULL at the
> end(checked it using the gdb), but when we try to dereference and
> print, gradually position holding NULL in "errs" is getting assinged
> to some other location, and i am getting the segmentation fault.
>
> >Since a triple pointer is difficult to manage in the code, you could have:
> >
> >        const char **e = NULL;
> >
> >
> >               if (errs) {
> >                        e = realloc(sizeof(*e), j + 1);
> >                        e[j++] = filters[i];
> >                }
> >
> >Then at the end:
> >
> >        if (errs)
> >                *errs = e;
>
> i didn't got the clear picture of what, the above code snippet is doing.
> particularly the below line
>
> >e = realloc(sizeof(*e), j + 1);
 It should be just
   e = realloc(e, (j + 1) * sizeof(char *));
we need to store one more pointer to char,
 On the first call, e is NULL and j is 0 and this behaves
 as a regular malloc:
   e = realloc(NULL, 1 * sizeof(char *));
on each subsequent call it allocates memory for
one additional pointer to char, at the end of the existing
array e.
Note that the allocated memory is not set to 0, it contains
random data.

>
> From man page i got
>
> void *realloc(void *ptr, size_t size);
>
> could you please brief me with an example?
>
> Subject: [PATCH] test: Test code
> diff --git a/test.c b/test.c
> index d38fc92..3e90711 100644
> --- a/test.c
> +++ b/test.c
> @@ -1,7 +1,39 @@
>  #include <tracefs.h>
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +
> +
> +#define INST "dummy"
> +
> +const char *filters[] = {"kvm_pmu_reset", "kvm_pmu_init",
> "dir_item_err", "tzvetomir", "sameer", "steve", NULL};
>
>  int main()
>  {
> -    tracefs_tracing_dir();
> +    struct tracefs_instance *instance = NULL;
> +    const char ***errs;

For the user of the API, errs is just an array of strings,
regular double pointer. It is not allocated by the caller,
so it is important to be initialised with NULL;

 const char **errs = NULL;

> +    int ret;
> +    int fd;
> +    int n;
> +    int i = 0;
> +
> +    errs = malloc(sizeof(char *));

No need to allocate it here, as the size is not known -
it depends on the number of failed filters. The API
will do the job.

> +    printf("%s\n",tracefs_tracing_dir());
> +    instance = tracefs_instance_create(INST);
> +    ret = tracefs_function_filter(instance, filters, "kvm", 1, errs);

As the errs will be allocated by the API, you should pass the address
of it:
   ret = tracefs_function_filter(instance, filters, "kvm", 1, &errs);
That makes the pointer triple, and it is important to be initialised
with NULL.

If the caller is not interested of the errors, it could pass NULL:
   ret = tracefs_function_filter(instance, filters, "kvm", 1, NULL);

> +
> +    if(ret < -1)
> +        if (errs)
> +            while (errs[i])
> +                printf("%s\n", *errs[i++]);
Yes, that is exactly how it should be used, except
that additional "*":
if (ret < 0 && errs) {
    while (errs[i])
          printf("%s\n", errs[i++]);
}

> +
> +    getchar();
> +    tracefs_instance_destroy(instance);
> +    tracefs_instance_free(instance);
> +    free(errs);
>      return 0;
>  }
>
> On Tue, Mar 9, 2021 at 9:51 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>
> >
> > diff --git a/include/tracefs.h b/include/tracefs.h
> > index f3eec62..a2249d0 100644
> > --- a/include/tracefs.h
> > +++ b/include/tracefs.h
> > @@ -50,6 +50,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 **filters, const char *module, bool reset, const char ***errs);
> >
> >  /**
> >   * 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 e2dfc7b..4e168df 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,96 @@ 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;
> > +       int write_size;
> > +       char *each_str;
> > +       int ret = 0;
> > +       int j = 0;
> > +       int size;
> > +       int slen;
> > +       int fd;
> > +       int i;
> > +
> > +       fd = open(filter_path, O_WRONLY | flags);
> > +       if (fd < 0)
> > +               return 1;
> > +
> > +       for (i = 0; filters[i]; i++) {
> > +               slen = strlen(filters[i]);
> > +               if (!slen)
> > +                       continue;
> > +
> > +               if (module)
> > +                       /* Adding 5 extra bytes for ":mod:"*/
> > +                       slen += strlen(module) + 5;
> > +
> > +               /* Adding 2 extra byte for the space and '\0' at the end*/
> > +               slen += 2;
> > +               each_str = calloc(1, slen);
> > +               if (!each_str)
> > +                       return 1;
> > +               if (module)
> > +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> > +               else
> > +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> > +
> > +               size = write(fd, each_str, write_size);
> > +               /* compare written bytes*/
> > +               if (size < write_size) {
> > +                       if (errs) {
> > +                               errs[j++] = &filters[i];
> > +                               ret -= 1;
> > +                       }
> > +               }
> > +               free(each_str);
> > +       }
> > +       errs[j] = NULL;
> > +       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
> >



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Tzvetomir Stoyanov]

Hi Sameer,

On Tue, Mar 9, 2021 at 6:24 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>
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..a2249d0 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -50,6 +50,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 **filters, const char *module, bool reset, const char ***errs);

I would break that in two lines, try to keep the lines up to 100
characters, when possible. It is very subjective to find the
balance between that 70-100 char limit and readable code.
There could be exceptions, but in this case I think it is better
to split it in two lines:

int tracefs_function_filter(struct tracefs_instance *instance, const
char **filters,
                                        const char *module, bool
reset, const char ***errs);

Note the second line - you should use tabs and spaces to align the  arguments
on the second line with the arguments on the first line. I cannot put
tabs in this
reply because of the strange limitations of my mail client, but in the
code - use
tabs (set to 8 spaces each) + spaces, if needed, to align it.

>
>  /**
>   * 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 e2dfc7b..4e168df 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"

Use tabs instead of spaces to align the string with the previous define.

>
>  static const char * const options_map[] = {
>         "unknown",
> @@ -387,3 +388,96 @@ 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)

I would split that in two lines, aligning the arguments on both lines - see
the comment above.

> +{
> +       int flags = reset ? O_TRUNC : O_APPEND;
> +       int write_size;
> +       char *each_str;
> +       int ret = 0;
> +       int j = 0;
> +       int size;
> +       int slen;
> +       int fd;
> +       int i;
> +
> +       fd = open(filter_path, O_WRONLY | flags);
> +       if (fd < 0)
> +               return 1;
> +
> +       for (i = 0; filters[i]; i++) {
> +               slen = strlen(filters[i]);
> +               if (!slen)
> +                       continue;
> +
> +               if (module)
> +                       /* Adding 5 extra bytes for ":mod:"*/
> +                       slen += strlen(module) + 5;
> +
> +               /* Adding 2 extra byte for the space and '\0' at the end*/
> +               slen += 2;
> +               each_str = calloc(1, slen);
> +               if (!each_str)
> +                       return 1;

This return will leak the opened fd. It is better to have a "goto error"
label and to clean all allocated resources in case of an error return.
I would suggest to initialize all resources that are allocated internally
with some default values (i.e. fd with -1, each_str with NULL) and
in this "error clean up logic" to free them, if they are allocated.

> +               if (module)
> +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> +               else
> +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);

You could use asprintf() instead of snprintf(), it will take care of the string
allocation and size instead of you, and will simplify the code. But note
that asprintf() has different behaviour in case of an error, it will not set
each_str to NULL. Take this into account in your logic.

> +
> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes*/
> +               if (size < write_size) {
> +                       if (errs) {
> +                               errs[j++] = &filters[i];

The errs must be reallocated here, I'll comment that on
your second email. Without realloc, it will cause a seg fault.

> +                               ret -= 1;
> +                       }
> +               }
> +               free(each_str);
> +       }
> +       errs[j] = NULL;

errs is an optional argument, it could be NULL - check it
before writing to it. Also, errs is a triple pointer:

if (errs)
    (*errs)[j] = NULL;

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

This also should be splitted in two lines.

> +{
> +       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 the next version, Sameer!

One hint when sending the next versions of the same patch:
use the "-v" option of the "git format-patch" command, that helps
to track the patch history. Also, it is useful to write the changes of
each version of the patch. In case of a patch set - the version changes
should be in the cover letter. In your case, as it is a single patch and
there is no need of a cover letter - add the version changes before
sending the patch just after the "---" line, manually. That way this
will not be part of the patch description, but still will be part of the
patch.

-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

>We are happy to have you working with us.
>
>I asked because the API you happened to pick is probably the most
>complex one to implement compared to some of the other APIs listed in the
>bugzilla, which is why I asked if you planned on doing anything with it, as
>it is also one of the more crucial APIs to get right.

At first I thought, my work will be done, if I write to that file, but
complexity
increased gradually, but let me try, since i have started i will finish this.

>So yes, let's go with "const char **" as we want to show that the
>strings will not be modified, and have "const char ***" for errs.

okay then i will change the errors declaration alone.

Thanks
sameer.

On Sat, Mar 6, 2021 at 8:35 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sat, 6 Mar 2021 07:25:18 +0530
> Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > i have one doubt.
> > >Note, @filters should be of type: const char * const * filters,  as not
> > >only is filters pointing to constant strings, the array itself will not be
> > >modified.
> >
> > what If the user wants to capture the filters at run time like below ?
> > let's say
> >
> >   filters = malloc(sizeof(char *));
> >         if (!filters)
> >                 return 1;
> >         printf("please enter the input filters count\n");
> >         scanf("%d", &fil_count);
> >         while(i < fil_count) {
> >                 scanf("%s", buf);
> >                 slen = strlen(buf);
> >                 if (!slen)
> >                         return 1;
> >                 filters[i] = calloc(1, slen);
> >                 strncpy(filters[i++], buf, slen);
> >         }
> > at that time, this declaration will be problematic right?, because we
> > are trying to modify
> > the read-only memory. Are we expecting the user to supply filters at
> > compile time like below?
> > const char * const *filters = {"kvm_pmu_reset", "kvm_pmu_init",
> > "dir_item_err", NULL};
>
> OK, my apologies, I see the issue that you are having, and you are
> correct.
>
> Because newer compiles will warn if you pass "char **" to a
> "const char * const *" parameter. Because it assumes that the two types
> are different, even when they shouldn't be. I'm not sure why the
> compiler wont let you pass in a char ** to a const char * const *, but
> it does indeed make them different.
>
> Even though there's ways to always pass strings via this logic, (you
> can create a "const char **" array and assign it to the dynamic one,
> and pass that in just fine). I looked at other prototypes, and see that
> the common method is.
>
>  const char **
>
> A couple do the "const char * const *" but they look to be special
> cases.
>
> So yes, let's go with "const char **" as we want to show that the
> strings will not be modified, and have "const char ***" for errs.
>
> Thanks!
>
> -- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Sat, 6 Mar 2021 07:25:18 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> i have one doubt.
> >Note, @filters should be of type: const char * const * filters,  as not
> >only is filters pointing to constant strings, the array itself will not be
> >modified.  
> 
> what If the user wants to capture the filters at run time like below ?
> let's say
> 
>   filters = malloc(sizeof(char *));
>         if (!filters)
>                 return 1;
>         printf("please enter the input filters count\n");
>         scanf("%d", &fil_count);
>         while(i < fil_count) {
>                 scanf("%s", buf);
>                 slen = strlen(buf);
>                 if (!slen)
>                         return 1;
>                 filters[i] = calloc(1, slen);
>                 strncpy(filters[i++], buf, slen);
>         }
> at that time, this declaration will be problematic right?, because we
> are trying to modify
> the read-only memory. Are we expecting the user to supply filters at
> compile time like below?
> const char * const *filters = {"kvm_pmu_reset", "kvm_pmu_init",
> "dir_item_err", NULL};

OK, my apologies, I see the issue that you are having, and you are
correct.

Because newer compiles will warn if you pass "char **" to a
"const char * const *" parameter. Because it assumes that the two types
are different, even when they shouldn't be. I'm not sure why the
compiler wont let you pass in a char ** to a const char * const *, but
it does indeed make them different.

Even though there's ways to always pass strings via this logic, (you
can create a "const char **" array and assign it to the dynamic one,
and pass that in just fine). I looked at other prototypes, and see that
the common method is.

 const char **

A couple do the "const char * const *" but they look to be special
cases.

So yes, let's go with "const char **" as we want to show that the
strings will not be modified, and have "const char ***" for errs.

Thanks!

-- Steve

[PATCH] libtracefs: An API to set the filtering of functions

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

diff --git a/include/tracefs.h b/include/tracefs.h
index f3eec62..2e5d3e3 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -50,6 +50,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 **filters, const char *module, bool reset, const char **errs);
 
 /**
  * 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 e2dfc7b..8311191 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,90 @@ 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;
+	int write_size;
+	char *each_str;
+	int ret = 0;
+	int j = 0;
+	int size;
+	int slen;
+	int fd;
+	int i;
+
+	fd = open(filter_path, O_WRONLY | flags);
+	if (fd < 0)
+		return -1;
+
+	for (i = 0; filters[i]; i++) {
+		slen = strlen(filters[i]);
+		if (!slen)
+			continue;
+
+		if (module)
+			/* Adding 5 extra bytes for ":mod:"*/
+			slen += strlen(module) + 5;
+
+		/* Adding 2 extra byte for the space and '\0' at the end*/
+		slen += 2;
+		each_str = calloc(1, slen);
+		if (!each_str)
+			return -1;
+		if (module)
+			write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
+		else
+			write_size = snprintf(each_str, slen, "%s ", filters[i]);
+
+		size = write(fd, each_str, write_size);
+		/* compare written bytes and also compare the written bytes with difference of added 5 bytes to string for ":mod:"*/
+		if ((size < write_size) && (size < (write_size - 5))) {
+			errs[j++] = filters[i];
+			ret -= 1;
+		}
+		free(each_str);
+	}
+	errs[j] = NULL;
+	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: An Array of failed function names ending with a NULL pointer
+ *
+ * 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 @filter. Otherwise, the functions set by @filter
+ * will be appended to the filter file
+ *
+ * The @errs is an array of strings, where each string is a failed function
+ * name
+ *
+ * returns -x (where x is number of failed filter srtings or it can be
+ * 1 for general errors), or 0 if there are no errors.
+ */
+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)
+		goto out;
+
+	ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
+ out:
+	tracefs_put_tracing_file(ftrace_filter_path);
+	return ret;
+}
-- 
2.7.4

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Sat, 6 Mar 2021 09:59:34 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> Okay steve
> 
> Actually I chose this API randomly from bugzilla, i don't have any
> plans as of now.
> I am developing it, as per your requirement.
> 
> Out of curiosity, I started implementing this API. Actually I am a
> novice in development,
> i know somewhat like how to write code logically and make it work, but
> i haven't followed any standard
> till now. I am learning some new things from your reviews.

We are happy to have you working with us.

I asked because the API you happened to pick is probably the most
complex one to implement compared to some of the other APIs listed in the
bugzilla, which is why I asked if you planned on doing anything with it, as
it is also one of the more crucial APIs to get right.

-- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

Okay steve

Actually I chose this API randomly from bugzilla, i don't have any
plans as of now.
I am developing it, as per your requirement.

Out of curiosity, I started implementing this API. Actually I am a
novice in development,
i know somewhat like how to write code logically and make it work, but
i haven't followed any standard
till now. I am learning some new things from your reviews.
Thanks,
sameer.

On Sat, 6 Mar, 2021, 9:08 am Steven Rostedt, <rostedt@goodmis.org> wrote:
>
> On Sat, 6 Mar 2021 07:25:18 +0530
> Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:
>
> > hi steve,
> >
> > i have one doubt.
> > >Note, @filters should be of type: const char * const * filters,  as not
> > >only is filters pointing to constant strings, the array itself will not be
> > >modified.
> >
> > what If the user wants to capture the filters at run time like below ?
> > let's say
> >
> >   filters = malloc(sizeof(char *));
> >         if (!filters)
> >                 return 1;
> >         printf("please enter the input filters count\n");
> >         scanf("%d", &fil_count);
> >         while(i < fil_count) {
> >                 scanf("%s", buf);
> >                 slen = strlen(buf);
> >                 if (!slen)
> >                         return 1;
> >                 filters[i] = calloc(1, slen);
> >                 strncpy(filters[i++], buf, slen);
> >         }
> > at that time, this declaration will be problematic right?, because we
> > are trying to modify
>
> No it wont. You can assign const pointers to dynamic pointers, but not
> the other way around. It's a way to show that the function you are
> calling wont do anything with the array you pass to it.
>
> > the read-only memory. Are we expecting the user to supply filters at
> > compile time like below?
> > const char * const *filters = {"kvm_pmu_reset", "kvm_pmu_init",
> > "dir_item_err", NULL};
>
> No, as explained above.
>
> >
> > Tzvetomir & steve,
> > >Since a triple pointer is difficult to manage in the code, you could have:
> > >
> > >       const char **e = NULL;
> > >
> > >
> > >               if (errs) {
> > >                        e = realloc(sizeof(*e), j + 1);
> > >                        e[j++] = filters[i];
> > >               }
> > >
> > >Then at the end:
> > >
> > >       if (errs)
> > >                *errs = e;
> > i have a concern here
> > when a double pointer is doing our work here without any overhead, why
> > we want to make it a triple pointer?
>
> What overhead? A string is a pointer, an array of strings is a double
> pointer, and passing in the address to an array of strings so you can
> modify that array is a triple pointer, and that's exactly what you need
> for errs.
>
> This is basic C coding, nothing special here.
>
> I'm curious to why you picked this particular API to implement. Is
> there something you are planning on using this for?
>
> -- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Sat, 6 Mar 2021 07:25:18 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> hi steve,
> 
> i have one doubt.
> >Note, @filters should be of type: const char * const * filters,  as not
> >only is filters pointing to constant strings, the array itself will not be
> >modified.  
> 
> what If the user wants to capture the filters at run time like below ?
> let's say
> 
>   filters = malloc(sizeof(char *));
>         if (!filters)
>                 return 1;
>         printf("please enter the input filters count\n");
>         scanf("%d", &fil_count);
>         while(i < fil_count) {
>                 scanf("%s", buf);
>                 slen = strlen(buf);
>                 if (!slen)
>                         return 1;
>                 filters[i] = calloc(1, slen);
>                 strncpy(filters[i++], buf, slen);
>         }
> at that time, this declaration will be problematic right?, because we
> are trying to modify

No it wont. You can assign const pointers to dynamic pointers, but not
the other way around. It's a way to show that the function you are
calling wont do anything with the array you pass to it.

> the read-only memory. Are we expecting the user to supply filters at
> compile time like below?
> const char * const *filters = {"kvm_pmu_reset", "kvm_pmu_init",
> "dir_item_err", NULL};

No, as explained above.

> 
> Tzvetomir & steve,
> >Since a triple pointer is difficult to manage in the code, you could have:
> >
> >       const char **e = NULL;
> >
> >
> >               if (errs) {
> >                        e = realloc(sizeof(*e), j + 1);
> >                        e[j++] = filters[i];
> >               }
> >
> >Then at the end:
> >
> >       if (errs)
> >                *errs = e;  
> i have a concern here
> when a double pointer is doing our work here without any overhead, why
> we want to make it a triple pointer?

What overhead? A string is a pointer, an array of strings is a double
pointer, and passing in the address to an array of strings so you can
modify that array is a triple pointer, and that's exactly what you need
for errs.

This is basic C coding, nothing special here.

I'm curious to why you picked this particular API to implement. Is
there something you are planning on using this for?

-- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Sameeruddin Shaik]

hi steve,

i have one doubt.
>Note, @filters should be of type: const char * const * filters,  as not
>only is filters pointing to constant strings, the array itself will not be
>modified.

what If the user wants to capture the filters at run time like below ?
let's say

  filters = malloc(sizeof(char *));
        if (!filters)
                return 1;
        printf("please enter the input filters count\n");
        scanf("%d", &fil_count);
        while(i < fil_count) {
                scanf("%s", buf);
                slen = strlen(buf);
                if (!slen)
                        return 1;
                filters[i] = calloc(1, slen);
                strncpy(filters[i++], buf, slen);
        }
at that time, this declaration will be problematic right?, because we
are trying to modify
the read-only memory. Are we expecting the user to supply filters at
compile time like below?
const char * const *filters = {"kvm_pmu_reset", "kvm_pmu_init",
"dir_item_err", NULL};

Tzvetomir & steve,
>Since a triple pointer is difficult to manage in the code, you could have:
>
>       const char **e = NULL;
>
>
>               if (errs) {
>                        e = realloc(sizeof(*e), j + 1);
>                        e[j++] = filters[i];
>               }
>
>Then at the end:
>
>       if (errs)
>                *errs = e;
i have a concern here
when a double pointer is doing our work here without any overhead, why
we want to make it a triple pointer?

Thanks,
sameer.

On Fri, Mar 5, 2021 at 8:24 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 5 Mar 2021 09:39:46 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > > > + * 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 @filter. Otherwise, the functions set by @filter
> > > > + * will be appended to the filter file
> > > > + *
> > > > + * The @errs is an array of strings, where each string is a failed function
> > > > + * name
> > > > + *
> > > > + * returns -x (where x is number of failed filter srtings or it can be
> > > > + * 1 for general errors), or 0 if there are no errors.
> > > > + */
>
>
> We should for the return statement:
>
>  * returns -x on filter errorrs (where x is number of failed filter strings)
>  *         and @errs if non-NULL will be an allocated string array pointing
>  *         to the strings in @filter that failed, and must be freed with
>  *         free().
>  *
>  * returns 1 on general errors not related to setting the filter.
>  *         @errs is not set, even if supplied.
>  *
>  * returns 0 on success, and @errs is not set.
>
> -- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Fri, 5 Mar 2021 09:39:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > > + * 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 @filter. Otherwise, the functions set by @filter
> > > + * will be appended to the filter file
> > > + *
> > > + * The @errs is an array of strings, where each string is a failed function
> > > + * name
> > > + *
> > > + * returns -x (where x is number of failed filter srtings or it can be
> > > + * 1 for general errors), or 0 if there are no errors.
> > > + */


We should for the return statement:

 * returns -x on filter errorrs (where x is number of failed filter strings)
 *         and @errs if non-NULL will be an allocated string array pointing
 *         to the strings in @filter that failed, and must be freed with
 *         free().
 *
 * returns 1 on general errors not related to setting the filter.
 *         @errs is not set, even if supplied.
 *
 * returns 0 on success, and @errs is not set.

-- Steve

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Steven Rostedt]

On Fri, 5 Mar 2021 14:20:15 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > +               if (module)
> > +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> > +               else
> > +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> > +
> > +               size = write(fd, each_str, write_size);
> > +               /* compare written bytes and also compare the written bytes with difference of added 5 bytes to string for ":mod:"*/
> > +               if ((size < write_size) && (size < (write_size - 5))) {  
> 
> The second comparison is not needed, the size must be equal to
> write_size in case of success.
> 
> > +                       errs[j++] = filters[i];  
> 
> The errs must be (re)allocated here, to hold additional pointer
> to string. The user does not know in advance the number of
> failed filters and it is not expected to provide an already
> allocated array. So it should be a triple pointer.
> 
> It is OK to assign directly failed input string to the error array,
> but this should be written in the API description, i.e. the user
> should not free the strings from the @errs array as it is a subset
> of the @filters array, or something like that. But note, the user
> still must free the @errs itself.

A check to see if "errs" is NULL is needed too. If the user doesn't care
about the errors, they can pass in NULL to errs, in which case, they should
be ignored.

> 
> > +                       ret -= 1;
> > +               }
> > +               free(each_str);
> > +       }
> > +       errs[j] = NULL;
> > +       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: An Array of failed function names ending with a NULL pointer

Thus the description for @errs should be:

 @errs: A pointer to an array of const strings that will be allocated on
        a 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 @filter. Otherwise, the functions set by @filter
> > + * will be appended to the filter file
> > + *
> > + * The @errs is an array of strings, where each string is a failed function
> > + * name
> > + *
> > + * returns -x (where x is number of failed filter srtings or it can be
> > + * 1 for general errors), or 0 if there are no errors.
> > + */
> > +int tracefs_function_filter(struct tracefs_instance *instance, const char **filters, const char *module, bool reset, const char **errs)

Note, @filters should be of type: const char * const * filters,  as not
only is filters pointing to constant strings, the array itself will not be
modified. And errs, should be of type: const char ***errs. Where it will be
allocated.

Since a triple pointer is difficult to manage in the code, you could have:

	const char **e = NULL;

	
		if (errs) {
			e = realloc(sizeof(*e), j + 1);
			e[j++] = filters[i];
		}

Then at the end:

	if (errs)
		*errs = e;


> > +{
> > +       char *ftrace_filter_path;
> > +       int ret = 0;  
> 
> This should be initialised to the error return code, to ensure that
> every "goto out" will return error.
> 
> > +
> > +       if (!filters)
> > +               return -1;  
> 
> Should return 1, according to the API description.
> 
> > +
> > +       ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> > +       if (!ftrace_filter_path)
> > +               goto out;  
> 
> "ret" is 0 here, so success will be returned, which is not the desired
> behaviour.
> "ftrace_filter_path" is NULL and the tracefs_put_tracing_file()
> will be called with NULL, but this is OK, should not be a problem.

It shouldn't be a problem, but it is also fine to return directly here.

	if (!ftrace_filter_path)
		return 1;

Thanks!

-- Steve

> 
> > +
> > +       ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
> > + out:
> > +       tracefs_put_tracing_file(ftrace_filter_path);
> > +       return ret;
> > +}
> > --
> > 2.7.4
> >  
> 
> It is a good starting point, thanks for sending this patch Sameer!
> 

Re: [PATCH] libtracefs: An API to set the filtering of functions

[Tzvetomir Stoyanov]

Hi Sameer,

On Fri, Mar 5, 2021 at 1:21 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>
>
> diff --git a/include/tracefs.h b/include/tracefs.h
> index f3eec62..2e5d3e3 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -50,6 +50,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 **filters, const char *module, bool reset, const char **errs);
>
>  /**
>   * 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 e2dfc7b..8311191 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,90 @@ 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;
> +       int write_size;
> +       char *each_str;
> +       int ret = 0;
> +       int j = 0;
> +       int size;
> +       int slen;
> +       int fd;
> +       int i;
> +
> +       fd = open(filter_path, O_WRONLY | flags);
> +       if (fd < 0)
> +               return -1;

The error return code should be 1, or the caller of this internal
function should translate this "-1" to the error code according
to the API description.

> +
> +       for (i = 0; filters[i]; i++) {
> +               slen = strlen(filters[i]);
> +               if (!slen)
> +                       continue;
> +
> +               if (module)
> +                       /* Adding 5 extra bytes for ":mod:"*/
> +                       slen += strlen(module) + 5;
> +
> +               /* Adding 2 extra byte for the space and '\0' at the end*/
> +               slen += 2;
> +               each_str = calloc(1, slen);
> +               if (!each_str)
> +                       return -1;

Same here.

> +               if (module)
> +                       write_size = snprintf(each_str, slen, "%s:mod:%s ", filters[i], module);
> +               else
> +                       write_size = snprintf(each_str, slen, "%s ", filters[i]);
> +
> +               size = write(fd, each_str, write_size);
> +               /* compare written bytes and also compare the written bytes with difference of added 5 bytes to string for ":mod:"*/
> +               if ((size < write_size) && (size < (write_size - 5))) {

The second comparison is not needed, the size must be equal to
write_size in case of success.

> +                       errs[j++] = filters[i];

The errs must be (re)allocated here, to hold additional pointer
to string. The user does not know in advance the number of
failed filters and it is not expected to provide an already
allocated array. So it should be a triple pointer.

It is OK to assign directly failed input string to the error array,
but this should be written in the API description, i.e. the user
should not free the strings from the @errs array as it is a subset
of the @filters array, or something like that. But note, the user
still must free the @errs itself.

> +                       ret -= 1;
> +               }
> +               free(each_str);
> +       }
> +       errs[j] = NULL;
> +       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: An Array of failed function names ending with a NULL pointer
> + *
> + * 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 @filter. Otherwise, the functions set by @filter
> + * will be appended to the filter file
> + *
> + * The @errs is an array of strings, where each string is a failed function
> + * name
> + *
> + * returns -x (where x is number of failed filter srtings or it can be
> + * 1 for general errors), or 0 if there are no errors.
> + */
> +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;

This should be initialised to the error return code, to ensure that
every "goto out" will return error.

> +
> +       if (!filters)
> +               return -1;

Should return 1, according to the API description.

> +
> +       ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
> +       if (!ftrace_filter_path)
> +               goto out;

"ret" is 0 here, so success will be returned, which is not the desired
behaviour.
"ftrace_filter_path" is NULL and the tracefs_put_tracing_file()
will be called with NULL, but this is OK, should not be a problem.

> +
> +       ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
> + out:
> +       tracefs_put_tracing_file(ftrace_filter_path);
> +       return ret;
> +}
> --
> 2.7.4
>

It is a good starting point, thanks for sending this patch Sameer!

-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center