linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] libtracfes: Add tracefs_function_filter()
@ 2021-03-23  1:27 Steven Rostedt
  2021-03-23  1:27 ` [PATCH 1/7] libtracefs: An API to set the filtering of functions Steven Rostedt
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:27 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

This adds a new API tracefs_function_filter() as described in:

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

It will use regular expressions against available_filter_functions (or even
the kernel glob expression) to enable the function by the index method if it
is supported. If it is not supported, it will go back to the writing of the
filter strings directly into the set_ftrace_filter file.


Sameeruddin shaik (1):
      libtracefs: An API to set the filtering of functions

Steven Rostedt (VMware) (6):
      libtracefs: Move opening of file out of controlled_write()
      libtracefs: Add add_errors() helper function for control_write()
      libtracefs: Add checking of available_filter_functions to tracefs_function_filter()
      libtracefs: Add write_filter() helper function
      libtracefs: Allow for setting filters with regex expressions
      libtracefs: Add indexing to set functions in tracefs_function_filter()

----
 include/tracefs.h   |   3 +-
 src/tracefs-tools.c | 521 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 522 insertions(+), 2 deletions(-)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/7] libtracefs: An API to set the filtering of functions
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
@ 2021-03-23  1:27 ` Steven Rostedt
  2021-03-23  1:27 ` [PATCH 2/7] libtracefs: Move opening of file out of controlled_write() Steven Rostedt
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:27 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik, Tzvetomir Stoyanov (VMware)

From: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>

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

Link: https://lore.kernel.org/linux-trace-devel/1616171938-18914-1-git-send-email-sameeruddin.shaik8@gmail.com

Acked-by: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Signed-off-by: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/tracefs.h   |   3 +-
 src/tracefs-tools.c | 111 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/include/tracefs.h b/include/tracefs.h
index f3eec62025f5..c1f07b0914f0 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -145,5 +145,6 @@ bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_o
 int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id);
 int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id);
 const char *tracefs_option_name(enum tracefs_option_id id);
-
+int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
+			    const char *module, bool reset, const char ***errs);
 #endif /* _TRACE_FS_H */
diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index e2dfc7b8acff..4f8bcdc299df 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,113 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt
 	if (options && id > TRACEFS_OPTION_INVALID)
 		options->mask &= ~(1ULL << (id - 1));
 }
+
+static int controlled_write(const char *filter_path, const char **filters,
+			    const char *module, bool reset, const char ***errs)
+{
+	int flags = reset ? O_TRUNC : O_APPEND;
+	const char **temp = NULL;
+	const char **e = NULL;
+	char *each_str = NULL;
+	int write_size = 0;
+	int size = 0;
+	int fd = -1;
+	int ret = 0;
+	int j = 0;
+	int i;
+
+	fd = open(filter_path, O_WRONLY | flags);
+	if (fd < 0)
+		return 1;
+
+	for (i = 0; filters[i]; i++) {
+		if (module)
+			write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module);
+		else
+			write_size = asprintf(&each_str, "%s ", filters[i]);
+		if (write_size < 0) {
+			ret = 1;
+			goto error;
+		}
+		size = write(fd, each_str, write_size);
+		/* compare written bytes*/
+		if (size < write_size) {
+			if (errs) {
+				temp = realloc(e, (j + 1) * (sizeof(char *)));
+				if (!temp) {
+					free(e);
+					ret = 1;
+					goto error;
+				} else
+					e = temp;
+
+				e[j++] = filters[i];
+				ret -= 1;
+			}
+		}
+		free(each_str);
+		each_str = NULL;
+	}
+	if (errs) {
+		temp = realloc(e, (j + 1) * (sizeof(char *)));
+		if (!temp) {
+			free(e);
+			ret = 1;
+			goto error;
+		} else
+			e = temp;
+		e[j] = NULL;
+		*errs = e;
+	}
+ error:
+	if (each_str)
+		free(each_str);
+	close(fd);
+	return ret;
+}
+
+/**
+ * tracefs_function_filter - write to set_ftrace_filter file to trace
+ * particular functions
+ * @instance: ftrace instance, can be NULL for top tracing instance
+ * @filters: An array of function names ending with a NULL pointer
+ * @module: Module to be traced
+ * @reset: set to true to reset the file before applying the filter
+ * @errs: A pointer to array of constant strings that will be allocated
+ * on negative return of this function, pointing to the filters that
+ * failed.May be NULL, in which case this field will be ignored.
+ *
+ * The @filters is an array of strings, where each string will be used
+ * to set a function or functions to be traced.
+ *
+ * If @reset is true, then all functions in the filter are cleared
+ * before adding functions from @filters. Otherwise, the functions set
+ * by @filters will be appended to the filter file
+ *
+ * returns -x on filter errors (where x is number of failed filter
+ * srtings) and if @errs is not NULL will be an allocated string array
+ * pointing to the strings in @filters that failed and must be freed
+ * with free().
+ *
+ * returns 1 on general errors not realted to setting the filter.
+ * @errs is not set even if supplied.
+ *
+ * return 0 on success and @errs is not set.
+ */
+int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
+			    const char *module, bool reset, const char ***errs)
+{
+	char *ftrace_filter_path;
+	int ret = 0;
+
+	if (!filters)
+		return 1;
+
+	ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
+	if (!ftrace_filter_path)
+		return 1;
+
+	ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
+	tracefs_put_tracing_file(ftrace_filter_path);
+	return ret;
+}
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/7] libtracefs: Move opening of file out of controlled_write()
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
  2021-03-23  1:27 ` [PATCH 1/7] libtracefs: An API to set the filtering of functions Steven Rostedt
@ 2021-03-23  1:27 ` Steven Rostedt
  2021-03-23  1:27 ` [PATCH 3/7] libtracefs: Add add_errors() helper function for control_write() Steven Rostedt
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:27 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

In order to try multiple methods of writing to the filter file, the opening
of the filter file needs to be in the tracefs_function_filter() call, and
allow that to be passed to multiple ways of writing to the filter.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 4f8bcdc299df..02e43168a810 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -389,24 +389,18 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt
 		options->mask &= ~(1ULL << (id - 1));
 }
 
-static int controlled_write(const char *filter_path, const char **filters,
-			    const char *module, bool reset, const char ***errs)
+static int controlled_write(int fd, const char **filters,
+			    const char *module, const char ***errs)
 {
-	int flags = reset ? O_TRUNC : O_APPEND;
 	const char **temp = NULL;
 	const char **e = NULL;
 	char *each_str = NULL;
 	int write_size = 0;
 	int size = 0;
-	int fd = -1;
 	int ret = 0;
 	int j = 0;
 	int i;
 
-	fd = open(filter_path, O_WRONLY | flags);
-	if (fd < 0)
-		return 1;
-
 	for (i = 0; filters[i]; i++) {
 		if (module)
 			write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module);
@@ -449,7 +443,6 @@ static int controlled_write(const char *filter_path, const char **filters,
  error:
 	if (each_str)
 		free(each_str);
-	close(fd);
 	return ret;
 }
 
@@ -486,6 +479,8 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 {
 	char *ftrace_filter_path;
 	int ret = 0;
+	int flags;
+	int fd;
 
 	if (!filters)
 		return 1;
@@ -494,7 +489,16 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 	if (!ftrace_filter_path)
 		return 1;
 
-	ret = controlled_write(ftrace_filter_path, filters, module, reset, errs);
+	flags = reset ? O_TRUNC : O_APPEND;
+
+	fd = open(ftrace_filter_path, O_WRONLY | flags);
 	tracefs_put_tracing_file(ftrace_filter_path);
+	if (fd < 0)
+		return 1;
+
+	ret = controlled_write(fd, filters, module, errs);
+
+	close(fd);
+
 	return ret;
 }
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/7] libtracefs: Add add_errors() helper function for control_write()
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
  2021-03-23  1:27 ` [PATCH 1/7] libtracefs: An API to set the filtering of functions Steven Rostedt
  2021-03-23  1:27 ` [PATCH 2/7] libtracefs: Move opening of file out of controlled_write() Steven Rostedt
@ 2021-03-23  1:27 ` Steven Rostedt
  2021-03-23  1:27 ` [PATCH 4/7] libtracefs: Add checking of available_filter_functions to tracefs_function_filter() Steven Rostedt
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:27 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Move the code that manages errors out of control_write() as this will be
used by other methods in setting the filters.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 61 ++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 02e43168a810..1348542a3419 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -389,16 +389,40 @@ void tracefs_option_clear(struct tracefs_options_mask *options, enum tracefs_opt
 		options->mask &= ~(1ULL << (id - 1));
 }
 
+static void add_errors(const char ***errs, const char *filter, int ret)
+{
+	const char **e;
+
+	if (!errs)
+		return;
+
+	/* Negative is passed in */
+	ret = -ret;
+	e = *errs;
+
+	/* If this previously failed to allocate stop processing */
+	if (!e && ret)
+		return;
+
+	/* Add 2, one for the new entry, and one for NULL */
+	e = realloc(e, sizeof(*e) * (ret + 2));
+	if (!e) {
+		free(*errs);
+		*errs = NULL;
+		return;
+	}
+	e[ret] = filter;
+	e[ret + 1] = NULL;
+	*errs = e;
+}
+
 static int controlled_write(int fd, const char **filters,
 			    const char *module, const char ***errs)
 {
-	const char **temp = NULL;
-	const char **e = NULL;
 	char *each_str = NULL;
 	int write_size = 0;
 	int size = 0;
 	int ret = 0;
-	int j = 0;
 	int i;
 
 	for (i = 0; filters[i]; i++) {
@@ -412,34 +436,11 @@ static int controlled_write(int fd, const char **filters,
 		}
 		size = write(fd, each_str, write_size);
 		/* compare written bytes*/
-		if (size < write_size) {
-			if (errs) {
-				temp = realloc(e, (j + 1) * (sizeof(char *)));
-				if (!temp) {
-					free(e);
-					ret = 1;
-					goto error;
-				} else
-					e = temp;
-
-				e[j++] = filters[i];
-				ret -= 1;
-			}
-		}
+		if (size < write_size)
+			add_errors(errs, filters[i], ret--);
 		free(each_str);
 		each_str = NULL;
 	}
-	if (errs) {
-		temp = realloc(e, (j + 1) * (sizeof(char *)));
-		if (!temp) {
-			free(e);
-			ret = 1;
-			goto error;
-		} else
-			e = temp;
-		e[j] = NULL;
-		*errs = e;
-	}
  error:
 	if (each_str)
 		free(each_str);
@@ -496,6 +497,10 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 	if (fd < 0)
 		return 1;
 
+	/* Make sure errs is NULL to start with, realloc() depends on it. */
+	if (errs)
+		*errs = NULL;
+
 	ret = controlled_write(fd, filters, module, errs);
 
 	close(fd);
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/7] libtracefs: Add checking of available_filter_functions to tracefs_function_filter()
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
                   ` (2 preceding siblings ...)
  2021-03-23  1:27 ` [PATCH 3/7] libtracefs: Add add_errors() helper function for control_write() Steven Rostedt
@ 2021-03-23  1:27 ` Steven Rostedt
  2021-03-23  1:28 ` [PATCH 5/7] libtracefs: Add write_filter() helper function Steven Rostedt
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:27 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Instead of modifying the set_ftrace_filter file with a filter that
possibly does not match anything, check the available_filter_functions
file first to make sure that each filter will match something. Then if
"reset" is enabled, it wont clear the filter file and not set any
filtering, which could cause issues if the application is not expecting
all functions to be enabled.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 183 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 178 insertions(+), 5 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 1348542a3419..d75cb3adcb9f 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
+#include <regex.h>
 #include <dirent.h>
 #include <limits.h>
 #include <errno.h>
@@ -416,6 +417,112 @@ static void add_errors(const char ***errs, const char *filter, int ret)
 	*errs = e;
 }
 
+struct func_filter {
+	const char		*filter;
+	regex_t			re;
+	bool			set;
+};
+
+/*
+ * Convert a glob into a regular expression.
+ */
+static char *make_regex(const char *glob)
+{
+	char *str;
+	int cnt = 0;
+	int i, j;
+
+	for (i = 0; glob[i]; i++) {
+		if (glob[i] == '*'|| glob[i] == '.')
+			cnt++;
+	}
+
+	/* '^' + ('*'->'.*' or '.' -> '\.') + '$' + '\0' */
+	str = malloc(i + cnt + 3);
+	if (!str)
+		return NULL;
+
+	str[0] = '^';
+	for (i = 0, j = 1; glob[i]; i++, j++) {
+		if (glob[i] == '*')
+			str[j++] = '.';
+		/* Dots can be part of a function name */
+		if (glob[i] == '.')
+			str[j++] = '\\';
+		str[j] = glob[i];
+	}
+	str[j++] = '$';
+	str[j] = '\0';
+	return str;
+}
+
+static bool match(const char *str, struct func_filter *func_filter)
+{
+	return regexec(&func_filter->re, str, 0, NULL, 0) == 0;
+}
+
+static int check_available_filters(struct func_filter *func_filters,
+				   const char *module, const char ***errs)
+{
+	char *line = NULL;
+	size_t size = 0;
+	char *path;
+	FILE *fp;
+	int ret = 1;
+	int mlen;
+	int i;
+
+	path = tracefs_get_tracing_file("available_filter_functions");
+	if (!path)
+		return 1;
+
+	fp = fopen(path, "r");
+	tracefs_put_tracing_file(path);
+
+	if (!fp)
+		return 1;
+
+	if (module)
+		mlen = strlen(module);
+
+	while (getline(&line, &size, fp) >= 0) {
+		char *saveptr = NULL;
+		char *tok, *mtok;
+		int len = strlen(line);
+
+		if (line[len - 1] == '\n')
+			line[len - 1] = '\0';
+		tok = strtok_r(line, " ", &saveptr);
+		if (!tok)
+			goto next;
+		if (module) {
+			mtok = strtok_r(line, " ", &saveptr);
+			if (!mtok)
+				goto next;
+			if ((strncmp(mtok + 1, module, mlen) != 0) ||
+			    (mtok[mlen + 1] != ']'))
+				goto next;
+		}
+		for (i = 0; func_filters[i].filter; i++) {
+			if (match(tok, &func_filters[i]))
+				func_filters[i].set = true;
+		}
+	next:
+		free(line);
+		line = NULL;
+		len = 0;
+	}
+	fclose(fp);
+
+	ret = 0;
+	for (i = 0; func_filters[i].filter; i++) {
+		if (!func_filters[i].set)
+			add_errors(errs, func_filters[i].filter, ret--);
+	}
+
+	return ret;
+}
+
 static int controlled_write(int fd, const char **filters,
 			    const char *module, const char ***errs)
 {
@@ -447,6 +554,62 @@ static int controlled_write(int fd, const char **filters,
 	return ret;
 }
 
+static int init_func_filter(struct func_filter *func_filter, const char *filter)
+{
+	char *str;
+	int ret;
+
+	str = make_regex(filter);
+	if (!str)
+		return -1;
+
+	ret = regcomp(&func_filter->re, str, REG_ICASE|REG_NOSUB);
+	free(str);
+
+	if (ret < 0)
+		return -1;
+
+	func_filter->filter = filter;
+	return 0;
+}
+
+static void free_func_filters(struct func_filter *func_filters)
+{
+	int i;
+
+	if (!func_filters)
+		return;
+
+	for (i = 0; func_filters[i].filter; i++) {
+		regfree(&func_filters[i].re);
+	}
+}
+
+static struct func_filter *make_func_filters(const char **filters)
+{
+	struct func_filter *func_filters = NULL;
+	int i;
+
+	for (i = 0; filters[i]; i++)
+		;
+
+	if (!i)
+		return NULL;
+
+	func_filters = calloc(i + 1, sizeof(*func_filters));
+	if (!func_filters)
+		return NULL;
+
+	for (i = 0; filters[i]; i++) {
+		if (init_func_filter(&func_filters[i], filters[i]) < 0)
+			goto out_err;
+	}
+	return func_filters;
+ out_err:
+	free_func_filters(func_filters);
+	return NULL;
+}
+
 /**
  * tracefs_function_filter - write to set_ftrace_filter file to trace
  * particular functions
@@ -478,14 +641,28 @@ static int controlled_write(int fd, const char **filters,
 int tracefs_function_filter(struct tracefs_instance *instance, const char **filters,
 			    const char *module, bool reset, const char ***errs)
 {
+	struct func_filter *func_filters;
 	char *ftrace_filter_path;
-	int ret = 0;
 	int flags;
+	int ret;
 	int fd;
 
 	if (!filters)
 		return 1;
 
+	func_filters = make_func_filters(filters);
+	if (!func_filters)
+		return 1;
+
+	/* Make sure errs is NULL to start with, realloc() depends on it. */
+	if (errs)
+		*errs = NULL;
+
+	ret = check_available_filters(func_filters, module, errs);
+	free_func_filters(func_filters);
+	if (ret)
+		return ret;
+
 	ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
 	if (!ftrace_filter_path)
 		return 1;
@@ -497,10 +674,6 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 	if (fd < 0)
 		return 1;
 
-	/* Make sure errs is NULL to start with, realloc() depends on it. */
-	if (errs)
-		*errs = NULL;
-
 	ret = controlled_write(fd, filters, module, errs);
 
 	close(fd);
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/7] libtracefs: Add write_filter() helper function
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
                   ` (3 preceding siblings ...)
  2021-03-23  1:27 ` [PATCH 4/7] libtracefs: Add checking of available_filter_functions to tracefs_function_filter() Steven Rostedt
@ 2021-03-23  1:28 ` Steven Rostedt
  2021-03-23  1:28 ` [PATCH 6/7] libtracefs: Allow for setting filters with regex expressions Steven Rostedt
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:28 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Wrap the writing of the filter into the filter file in a helper function
such that it can be used in more places.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 58 ++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index d75cb3adcb9f..5b12127a61d9 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -461,6 +461,33 @@ static bool match(const char *str, struct func_filter *func_filter)
 	return regexec(&func_filter->re, str, 0, NULL, 0) == 0;
 }
 
+/*
+ * Return 0 on success, -1 error writing, 1 on other errors.
+ */
+static int write_filter(int fd, const char *filter, const char *module)
+{
+	char *each_str = NULL;
+	int write_size;
+	int size;
+
+	if (module)
+		write_size = asprintf(&each_str, "%s:mod:%s ", filter, module);
+	else
+		write_size = asprintf(&each_str, "%s ", filter);
+
+	if (write_size < 0)
+		return 1;
+
+	size = write(fd, each_str, write_size);
+	free(each_str);
+
+	/* compare written bytes*/
+	if (size < write_size)
+		return -1;
+
+	return 0;
+}
+
 static int check_available_filters(struct func_filter *func_filters,
 				   const char *module, const char ***errs)
 {
@@ -526,31 +553,24 @@ static int check_available_filters(struct func_filter *func_filters,
 static int controlled_write(int fd, const char **filters,
 			    const char *module, const char ***errs)
 {
-	char *each_str = NULL;
-	int write_size = 0;
-	int size = 0;
 	int ret = 0;
 	int i;
 
 	for (i = 0; filters[i]; i++) {
-		if (module)
-			write_size = asprintf(&each_str, "%s:mod:%s ", filters[i], module);
-		else
-			write_size = asprintf(&each_str, "%s ", filters[i]);
-		if (write_size < 0) {
-			ret = 1;
-			goto error;
-		}
-		size = write(fd, each_str, write_size);
-		/* compare written bytes*/
-		if (size < write_size)
+		int r;
+
+		r = write_filter(fd, filters[i], module);
+		if (r < 0) {
 			add_errors(errs, filters[i], ret--);
-		free(each_str);
-		each_str = NULL;
+		} else if (r > 0) {
+			/* Not filter error */
+			if (errs) {
+				free(*errs);
+				*errs = NULL;
+			}
+			return 1;
+		}
 	}
- error:
-	if (each_str)
-		free(each_str);
 	return ret;
 }
 
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/7] libtracefs: Allow for setting filters with regex expressions
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
                   ` (4 preceding siblings ...)
  2021-03-23  1:28 ` [PATCH 5/7] libtracefs: Add write_filter() helper function Steven Rostedt
@ 2021-03-23  1:28 ` Steven Rostedt
  2021-03-23  1:28 ` [PATCH 7/7] libtracefs: Add indexing to set functions in tracefs_function_filter() Steven Rostedt
  2021-03-23 12:52 ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:28 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

All for full "regex(3)" processing of setting functions in the
set_ftrace_filter file. Check if the filter passed in is just a glob
expression that the kernel can process, or if it is a regex that should look
at the available_filter_functions list instead.

If it is a regex, it will read the available_filter_functions and write in
each function as it finds it.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 139 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 118 insertions(+), 21 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 5b12127a61d9..5e80c5e196b1 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -18,8 +18,9 @@
 #include "tracefs.h"
 #include "tracefs-local.h"
 
-#define TRACE_CTRL	"tracing_on"
-#define TRACE_FILTER	"set_ftrace_filter"
+#define TRACE_CTRL		"tracing_on"
+#define TRACE_FILTER		"set_ftrace_filter"
+#define TRACE_FILTER_LIST	"available_filter_functions"
 
 static const char * const options_map[] = {
 	"unknown",
@@ -421,8 +422,53 @@ struct func_filter {
 	const char		*filter;
 	regex_t			re;
 	bool			set;
+	bool			is_regex;
 };
 
+static bool is_regex(const char *str)
+{
+	int i;
+
+	for (i = 0; str[i]; i++) {
+		switch (str[i]) {
+		case 'a' ... 'z':
+		case 'A'...'Z':
+		case '_':
+		case '0'...'9':
+		case '*':
+		case '.':
+			/* Dots can be part of a function name */
+		case '?':
+			continue;
+		default:
+			return true;
+		}
+	}
+	return false;
+}
+
+static char *update_regex(const char *reg)
+{
+	int len = strlen(reg);
+	char *str;
+
+	if (reg[0] == '^' && reg[len - 1] == '$')
+		return strdup(reg);
+
+	str = malloc(len + 3);
+	if (reg[0] == '^') {
+		strcpy(str, reg);
+	} else {
+		str[0] = '^';
+		strcpy(str + 1, reg);
+		len++; /* add ^ */
+	}
+	if (str[len - 1] != '$')
+		str[len++]= '$';
+	str[len] = '\0';
+	return str;
+}
+
 /*
  * Convert a glob into a regular expression.
  */
@@ -488,8 +534,13 @@ static int write_filter(int fd, const char *filter, const char *module)
 	return 0;
 }
 
-static int check_available_filters(struct func_filter *func_filters,
-				   const char *module, const char ***errs)
+enum match_type {
+	FILTER_CHECK,
+	FILTER_WRITE,
+};
+
+static int match_filters(int fd, struct func_filter *func_filters,
+			 const char *module, enum match_type type)
 {
 	char *line = NULL;
 	size_t size = 0;
@@ -499,7 +550,7 @@ static int check_available_filters(struct func_filter *func_filters,
 	int mlen;
 	int i;
 
-	path = tracefs_get_tracing_file("available_filter_functions");
+	path = tracefs_get_tracing_file(TRACE_FILTER_LIST);
 	if (!path)
 		return 1;
 
@@ -530,39 +581,76 @@ static int check_available_filters(struct func_filter *func_filters,
 			    (mtok[mlen + 1] != ']'))
 				goto next;
 		}
-		for (i = 0; func_filters[i].filter; i++) {
-			if (match(tok, &func_filters[i]))
-				func_filters[i].set = true;
+		switch (type) {
+		case FILTER_CHECK:
+			/* Check, checks a list of filters */
+			for (i = 0; func_filters[i].filter; i++) {
+				if (match(tok, &func_filters[i]))
+					func_filters[i].set = true;
+			}
+			break;
+		case FILTER_WRITE:
+			/* Writes only have one filter */
+			if (match(tok, func_filters)) {
+				ret = write_filter(fd, tok, module);
+				if (ret)
+					goto out;
+			}
+			break;
 		}
 	next:
 		free(line);
 		line = NULL;
 		len = 0;
 	}
+ out:
+	free(line);
 	fclose(fp);
 
+	return ret;
+}
+
+static int check_available_filters(struct func_filter *func_filters,
+				   const char *module, const char ***errs)
+{
+	int ret;
+	int i;
+
+	ret = match_filters(-1, func_filters, module, FILTER_CHECK);
+	/* Return here if success or non filter error */
+	if (ret >= 0)
+		return ret;
+
+	/* Failed on filter, set the errors */
 	ret = 0;
 	for (i = 0; func_filters[i].filter; i++) {
 		if (!func_filters[i].set)
 			add_errors(errs, func_filters[i].filter, ret--);
 	}
-
 	return ret;
 }
 
-static int controlled_write(int fd, const char **filters,
+static int set_regex_filter(int fd, struct func_filter *func_filter,
+			    const char *module)
+{
+	return match_filters(fd, func_filter, module, FILTER_WRITE);
+}
+
+static int controlled_write(int fd, struct func_filter *func_filters,
 			    const char *module, const char ***errs)
 {
 	int ret = 0;
 	int i;
 
-	for (i = 0; filters[i]; i++) {
+	for (i = 0; func_filters[i].filter; i++) {
+		const char *filter = func_filters[i].filter;
 		int r;
 
-		r = write_filter(fd, filters[i], module);
-		if (r < 0) {
-			add_errors(errs, filters[i], ret--);
-		} else if (r > 0) {
+		if (func_filters[i].is_regex)
+			r = set_regex_filter(fd, &func_filters[i], module);
+		else
+			r = write_filter(fd, filter, module);
+		if (r > 0) {
 			/* Not filter error */
 			if (errs) {
 				free(*errs);
@@ -570,6 +658,8 @@ static int controlled_write(int fd, const char **filters,
 			}
 			return 1;
 		}
+		if (r < 0)
+			add_errors(errs, filter, ret--);
 	}
 	return ret;
 }
@@ -579,7 +669,11 @@ static int init_func_filter(struct func_filter *func_filter, const char *filter)
 	char *str;
 	int ret;
 
-	str = make_regex(filter);
+	if (!(func_filter->is_regex = is_regex(filter)))
+		str = make_regex(filter);
+	else
+		str = update_regex(filter);
+
 	if (!str)
 		return -1;
 
@@ -679,24 +773,27 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 		*errs = NULL;
 
 	ret = check_available_filters(func_filters, module, errs);
-	free_func_filters(func_filters);
 	if (ret)
-		return ret;
+		goto out_free;
 
+	ret = 1;
 	ftrace_filter_path = tracefs_instance_get_file(instance, TRACE_FILTER);
 	if (!ftrace_filter_path)
-		return 1;
+		goto out_free;
 
 	flags = reset ? O_TRUNC : O_APPEND;
 
 	fd = open(ftrace_filter_path, O_WRONLY | flags);
 	tracefs_put_tracing_file(ftrace_filter_path);
 	if (fd < 0)
-		return 1;
+		goto out_free;
 
-	ret = controlled_write(fd, filters, module, errs);
+	ret = controlled_write(fd, func_filters, module, errs);
 
 	close(fd);
 
+ out_free:
+	free_func_filters(func_filters);
+
 	return ret;
 }
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 7/7] libtracefs: Add indexing to set functions in tracefs_function_filter()
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
                   ` (5 preceding siblings ...)
  2021-03-23  1:28 ` [PATCH 6/7] libtracefs: Allow for setting filters with regex expressions Steven Rostedt
@ 2021-03-23  1:28 ` Steven Rostedt
  2021-03-23 12:52 ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
  7 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23  1:28 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Starting in Linux kernel 5.1, it is possible to write the index of the
available_filter_functions (where the first function is 1), which is a
much faster way to set functions than with the glob expressions. By
finding the matches of the available_filter_functions and using the index
of the function, this can make the function call of
tracefs_function_filter() return much faster.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-tools.c | 123 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 116 insertions(+), 7 deletions(-)

diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 5e80c5e196b1..f97703d5e317 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -418,6 +418,12 @@ static void add_errors(const char ***errs, const char *filter, int ret)
 	*errs = e;
 }
 
+struct func_list {
+	struct func_list	*next;
+	unsigned int		start;
+	unsigned int		end;
+};
+
 struct func_filter {
 	const char		*filter;
 	regex_t			re;
@@ -534,18 +540,54 @@ static int write_filter(int fd, const char *filter, const char *module)
 	return 0;
 }
 
+static int add_func(struct func_list ***next_func_ptr, unsigned int index)
+{
+	struct func_list **next_func = *next_func_ptr;
+	struct func_list *func_list = *next_func;
+
+	if (!func_list) {
+		func_list = calloc(1, sizeof(*func_list));
+		if (!func_list)
+			return -1;
+		func_list->start = index;
+		func_list->end = index;
+		*next_func = func_list;
+		return 0;
+	}
+
+	if (index == func_list->end + 1) {
+		func_list->end = index;
+		return 0;
+	}
+	*next_func_ptr = &func_list->next;
+	return add_func(next_func_ptr, index);
+}
+
+static void free_func_list(struct func_list *func_list)
+{
+	struct func_list *f;
+
+	while (func_list) {
+		f = func_list;
+		func_list = f->next;
+		free(f);
+	}
+}
+
 enum match_type {
 	FILTER_CHECK,
 	FILTER_WRITE,
 };
 
 static int match_filters(int fd, struct func_filter *func_filters,
-			 const char *module, enum match_type type)
+			 const char *module, struct func_list **func_list,
+			 enum match_type type)
 {
 	char *line = NULL;
 	size_t size = 0;
 	char *path;
 	FILE *fp;
+	int index = 0;
 	int ret = 1;
 	int mlen;
 	int i;
@@ -567,12 +609,16 @@ static int match_filters(int fd, struct func_filter *func_filters,
 		char *saveptr = NULL;
 		char *tok, *mtok;
 		int len = strlen(line);
+		bool first = true;
 
 		if (line[len - 1] == '\n')
 			line[len - 1] = '\0';
 		tok = strtok_r(line, " ", &saveptr);
 		if (!tok)
 			goto next;
+
+		index++;
+
 		if (module) {
 			mtok = strtok_r(line, " ", &saveptr);
 			if (!mtok)
@@ -585,8 +631,23 @@ static int match_filters(int fd, struct func_filter *func_filters,
 		case FILTER_CHECK:
 			/* Check, checks a list of filters */
 			for (i = 0; func_filters[i].filter; i++) {
-				if (match(tok, &func_filters[i]))
+				/*
+				 * If a match was found, still need to
+				 * check if other filters would match
+				 * to make sure that all filters have a
+				 * match, as some filters may overlap.
+				 */
+				if (!first && func_filters[i].set)
+					continue;
+				if (match(tok, &func_filters[i])) {
 					func_filters[i].set = true;
+					if (first) {
+						first = false;
+						ret = add_func(&func_list, index);
+						if (ret)
+							goto out;
+					}
+				}
 			}
 			break;
 		case FILTER_WRITE:
@@ -611,12 +672,13 @@ static int match_filters(int fd, struct func_filter *func_filters,
 }
 
 static int check_available_filters(struct func_filter *func_filters,
-				   const char *module, const char ***errs)
+				   const char *module, const char ***errs,
+				   struct func_list **func_list)
 {
 	int ret;
 	int i;
 
-	ret = match_filters(-1, func_filters, module, FILTER_CHECK);
+	ret = match_filters(-1, func_filters, module, func_list, FILTER_CHECK);
 	/* Return here if success or non filter error */
 	if (ret >= 0)
 		return ret;
@@ -633,7 +695,7 @@ static int check_available_filters(struct func_filter *func_filters,
 static int set_regex_filter(int fd, struct func_filter *func_filter,
 			    const char *module)
 {
-	return match_filters(fd, func_filter, module, FILTER_WRITE);
+	return match_filters(fd, func_filter, module, NULL, FILTER_WRITE);
 }
 
 static int controlled_write(int fd, struct func_filter *func_filters,
@@ -724,6 +786,49 @@ static struct func_filter *make_func_filters(const char **filters)
 	return NULL;
 }
 
+static int write_number(int fd, unsigned int start, unsigned int end)
+{
+	char buf[64];
+	unsigned int i;
+	int n, ret;
+
+	for (i = start; i <= end; i++) {
+		n = snprintf(buf, 64, "%d ", i);
+		ret = write(fd, buf, n);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * This will try to write the first number, if that fails, it
+ * will assume that it is not supported and return 1.
+ * If the first write succeeds, but a following write fails, then
+ * the kernel does support this, but something else went wrong,
+ * in this case, return -1.
+ */
+static int write_func_list(int fd, struct func_list *list)
+{
+	int ret;
+
+	if (!list)
+		return 0;
+
+	ret = write_number(fd, list->start, list->end);
+	if (ret)
+		return 1; // try a different way
+	list = list->next;
+	while (list) {
+		ret = write_number(fd, list->start, list->end);
+		if (ret)
+			return -1;
+		list = list->next;
+	}
+	return 0;
+}
+
 /**
  * tracefs_function_filter - write to set_ftrace_filter file to trace
  * particular functions
@@ -756,6 +861,7 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 			    const char *module, bool reset, const char ***errs)
 {
 	struct func_filter *func_filters;
+	struct func_list *func_list = NULL;
 	char *ftrace_filter_path;
 	int flags;
 	int ret;
@@ -772,7 +878,7 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 	if (errs)
 		*errs = NULL;
 
-	ret = check_available_filters(func_filters, module, errs);
+	ret = check_available_filters(func_filters, module, errs, &func_list);
 	if (ret)
 		goto out_free;
 
@@ -788,11 +894,14 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char **filt
 	if (fd < 0)
 		goto out_free;
 
-	ret = controlled_write(fd, func_filters, module, errs);
+	ret = write_func_list(fd, func_list);
+	if (ret > 0)
+		ret = controlled_write(fd, func_filters, module, errs);
 
 	close(fd);
 
  out_free:
+	free_func_list(func_list);
 	free_func_filters(func_filters);
 
 	return ret;
-- 
2.30.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
  2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
                   ` (6 preceding siblings ...)
  2021-03-23  1:28 ` [PATCH 7/7] libtracefs: Add indexing to set functions in tracefs_function_filter() Steven Rostedt
@ 2021-03-23 12:52 ` Steven Rostedt
  2021-03-24 10:13   ` Sameeruddin Shaik
  2021-03-26  0:25   ` sameeruddin shaik
  7 siblings, 2 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-23 12:52 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Sameeruddin shaik

On Mon, 22 Mar 2021 21:27:55 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This adds a new API tracefs_function_filter() as described in:
> 
>  https://bugzilla.kernel.org/show_bug.cgi?id=210643
> 
> It will use regular expressions against available_filter_functions (or even
> the kernel glob expression) to enable the function by the index method if it
> is supported. If it is not supported, it will go back to the writing of the
> filter strings directly into the set_ftrace_filter file.
> 
> 

Playing with the interface some more, I feel it's not quite adequate.

The returning the negative number of filters that failed, isn't very
useful. Having the errs array that points to those filters gives us that
information.

Also, because of the way that file works in the kernel, we need to be able
to call this function several times without closing the file. That's
because the actions take place when the file is closed, *not* when a write
is made. Having the interface return errors without closing the file would
allow the user to fix the failed filters and try again.

I plan on committing this patch series, so any new changes will be done on
top of them. But here's what I think needs to be done to make the interface
more usable.

 - Create a tracefs_function_filter_commit(instance) API that will close
   the file and commit the changes.

 - Have the tracefs_function_filter() open the set_ftrace_filter file if it
   is not already opened. This will allow for the function to be called
   multiple times. The file descriptor will be part of the instance
   descriptor or a global variable for the top level instance. The opening
   of the files will need to be protected by a pthread mutex.
   Note, the "reset" parameter is only applicable if the file is not
   already opened.

 - On success, return 0 and tracefs_function_filter_commit() must be called.

 - If the file descriptor is opened and an error happens, return 1, and the
   tracefs_function_filter_commit() still needs to be called, but the user
   can call tracefs_function_filter() again to try to fix the problem.

 - If an error is found before the file descriptor is opened, then
   tracefs_function_filter_commit() does not need to be called.

That is:

	ret = tracefs_function_filter(...);
	if (ret >= 0)
		tracefs_function_filter_commit();

 - Now for errs, it will be allocated if a problem happened on a filter,
   and may be set if the return value is non zero (1 or -1). The user can
   use it to know if the problem happened on a filter as well as find out
   which filter had the problem.

 - We can have a tracefs_read_function_filter() that returns an array of
   strings which ends with a NULL pointer (and needs to be freed with
   tracefs_list_free(), and have this:

   save_filters = tracefs_read_function_filter(instance);

   ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
   if (ret > 0 && errs) {
	/* Modified but failed */
	int i, j;
	for (i = 0; filters[i]; i++)
		;
	for (j = 0; errs[i]; j++)
		;
	if (i == j)
		/* all filters failed! Put back the original */
		tracefs_function_filter(instance, save_filters, NULL, false, NULL);
   }
   if (ret >= 0)
	tracefs_function_filter_commit(instance);

Another reason to have it not close the file is because we only support
passing in one module at a time. If the user wants to enable functions in
two modules, they would need to call this twice, and we want them to be
able to do so and have both changes take affect at the same time.

Thoughts?

-- Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
  2021-03-23 12:52 ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
@ 2021-03-24 10:13   ` Sameeruddin Shaik
  2021-03-24 14:08     ` Steven Rostedt
  2021-03-26  0:25   ` sameeruddin shaik
  1 sibling, 1 reply; 13+ messages in thread
From: Sameeruddin Shaik @ 2021-03-24 10:13 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel

Hi steve,
I have doubt

On Tue, Mar 23, 2021 at 6:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 22 Mar 2021 21:27:55 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > This adds a new API tracefs_function_filter() as described in:
> >
> >  https://bugzilla.kernel.org/show_bug.cgi?id=210643
> >
> > It will use regular expressions against available_filter_functions (or even
> > the kernel glob expression) to enable the function by the index method if it
> > is supported. If it is not supported, it will go back to the writing of the
> > filter strings directly into the set_ftrace_filter file.
> >
> >
>
> Playing with the interface some more, I feel it's not quite adequate.
>
> The returning the negative number of filters that failed, isn't very
> useful. Having the errs array that points to those filters gives us that
> information.
>
> Also, because of the way that file works in the kernel, we need to be able
> to call this function several times without closing the file. That's
> because the actions take place when the file is closed, *not* when a write
> is made. Having the interface return errors without closing the file would
> allow the user to fix the failed filters and try again.
>
> I plan on committing this patch series, so any new changes will be done on
> top of them. But here's what I think needs to be done to make the interface
> more usable.
>
>  - Create a tracefs_function_filter_commit(instance) API that will close
>    the file and commit the changes.
>
>  - Have the tracefs_function_filter() open the set_ftrace_filter file if it
>    is not already opened. This will allow for the function to be called
>    multiple times. The file descriptor will be part of the instance
>    descriptor or a global variable for the top level instance. The opening
>    of the files will need to be protected by a pthread mutex.
>    Note, the "reset" parameter is only applicable if the file is not
>    already opened.
>
>  - On success, return 0 and tracefs_function_filter_commit() must be called.
>
>  - If the file descriptor is opened and an error happens, return 1, and the
>    tracefs_function_filter_commit() still needs to be called, but the user
>    can call tracefs_function_filter() again to try to fix the problem.
>
>  - If an error is found before the file descriptor is opened, then
>    tracefs_function_filter_commit() does not need to be called.
>
> That is:
>
>         ret = tracefs_function_filter(...);
>         if (ret >= 0)
>                 tracefs_function_filter_commit();
>
>  - Now for errs, it will be allocated if a problem happened on a filter,
>    and may be set if the return value is non zero (1 or -1). The user can
>    use it to know if the problem happened on a filter as well as find out
>    which filter had the problem.
>
>  - We can have a tracefs_read_function_filter() that returns an array of
>    strings which ends with a NULL pointer (and needs to be freed with
>    tracefs_list_free(), and have this:
>
>    save_filters = tracefs_read_function_filter(instance);

are we going to use the tracefs_read_function_filter() function to
read the set_ftrace_file for getting written filters?
>
>    ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
>    if (ret > 0 && errs) {
>         /* Modified but failed */
>         int i, j;
>         for (i = 0; filters[i]; i++)
>                 ;
>         for (j = 0; errs[i]; j++)
>                 ;
>         if (i == j)
>                 /* all filters failed! Put back the original */
>                 tracefs_function_filter(instance, save_filters, NULL, false, NULL);
>    }
>    if (ret >= 0)
>         tracefs_function_filter_commit(instance);
>
> Another reason to have it not close the file is because we only support
> passing in one module at a time. If the user wants to enable functions in
> two modules, they would need to call this twice, and we want them to be
> able to do so and have both changes take affect at the same time.
>
> Thoughts?
>
> -- Steve

Thanks,
sameer.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
  2021-03-24 10:13   ` Sameeruddin Shaik
@ 2021-03-24 14:08     ` Steven Rostedt
  0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-24 14:08 UTC (permalink / raw)
  To: Sameeruddin Shaik; +Cc: Linux Trace Devel

On Wed, 24 Mar 2021 15:43:28 +0530
Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote:

> >  - We can have a tracefs_read_function_filter() that returns an array of
> >    strings which ends with a NULL pointer (and needs to be freed with
> >    tracefs_list_free(), and have this:
> >
> >    save_filters = tracefs_read_function_filter(instance);  
> 
> are we going to use the tracefs_read_function_filter() function to
> read the set_ftrace_file for getting written filters?

Yes, why not?

But it would not return probes, only filters.

That is, the set_ftrace_filter also shows probes, and they are different.
But the tracefs_read_function_filter() should not return them. For them, we
should have a tracefs_read_function_probes() API.

-- Steve


> >
> >    ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
> >    if (ret > 0 && errs) {
> >         /* Modified but failed */
> >         int i, j;
> >         for (i = 0; filters[i]; i++)
> >                 ;
> >         for (j = 0; errs[i]; j++)
> >                 ;
> >         if (i == j)
> >                 /* all filters failed! Put back the original */
> >                 tracefs_function_filter(instance, save_filters, NULL, false, NULL);
> >    }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
  2021-03-26  0:25   ` sameeruddin shaik
@ 2021-03-25  0:35     ` Steven Rostedt
  0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2021-03-25  0:35 UTC (permalink / raw)
  To: sameeruddin shaik; +Cc: linux-trace-devel

On Fri, 26 Mar 2021 05:55:46 +0530
sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote:

> 
> On 23/03/21 6:22 pm, Steven Rostedt wrote:
> > On Mon, 22 Mar 2021 21:27:55 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >> This adds a new API tracefs_function_filter() as described in:
> >>
> >>   https://bugzilla.kernel.org/show_bug.cgi?id=210643
> >>
> >> It will use regular expressions against available_filter_functions (or even
> >> the kernel glob expression) to enable the function by the index method if it
> >> is supported. If it is not supported, it will go back to the writing of the
> >> filter strings directly into the set_ftrace_filter file.
> >>
> >>
> > Playing with the interface some more, I feel it's not quite adequate.
> >
> > The returning the negative number of filters that failed, isn't very
> > useful. Having the errs array that points to those filters gives us that
> > information.
> Yeah, Anyway we are pointing the failed filters using the errs pointer. 
> But to differentiate
> 
> the failed filters from the general errors, we can return -1, what you say?
>

No, I was thinking that -1 means that the file was never opened, but 1
means an error happened after the file was opened (and thus it must be
closed, or "committed"). In either case, the errs can be set.

I have the function checking the available_filter_functions to see if
there's any matches before it even opens the file, and if any filter
fails, I was thinking it should return -1 (meaning failure before
opening) and the errs will be set to the filters that failed (if it was
due to a filter failing. We should clear errno, at the start, so that
if a filter failed, errno is zero, and errs points to the failed
filters, if there was another failure, then errno would be set instead).

If all filters match, and we use your old method and there's some other
kind of error in writing the filter to set_ftrace_filter, then we could
have errs point to that filter, but still return 1 because the file was
opened and it still needs to be closed / committed. Note, this wont
happen on kernels 5.1 and beyond, as they support indexing, and once we
get to the indexing part, errs wont be used.

-- Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
  2021-03-23 12:52 ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
  2021-03-24 10:13   ` Sameeruddin Shaik
@ 2021-03-26  0:25   ` sameeruddin shaik
  2021-03-25  0:35     ` Steven Rostedt
  1 sibling, 1 reply; 13+ messages in thread
From: sameeruddin shaik @ 2021-03-26  0:25 UTC (permalink / raw)
  To: Steven Rostedt, linux-trace-devel


On 23/03/21 6:22 pm, Steven Rostedt wrote:
> On Mon, 22 Mar 2021 21:27:55 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> This adds a new API tracefs_function_filter() as described in:
>>
>>   https://bugzilla.kernel.org/show_bug.cgi?id=210643
>>
>> It will use regular expressions against available_filter_functions (or even
>> the kernel glob expression) to enable the function by the index method if it
>> is supported. If it is not supported, it will go back to the writing of the
>> filter strings directly into the set_ftrace_filter file.
>>
>>
> Playing with the interface some more, I feel it's not quite adequate.
>
> The returning the negative number of filters that failed, isn't very
> useful. Having the errs array that points to those filters gives us that
> information.
Yeah, Anyway we are pointing the failed filters using the errs pointer. 
But to differentiate

the failed filters from the general errors, we can return -1, what you say?
>
> Also, because of the way that file works in the kernel, we need to be able
> to call this function several times without closing the file. That's
> because the actions take place when the file is closed, *not* when a write
> is made. Having the interface return errors without closing the file would
> allow the user to fix the failed filters and try again.
>
> I plan on committing this patch series, so any new changes will be done on
> top of them. But here's what I think needs to be done to make the interface
> more usable.
>
>   - Create a tracefs_function_filter_commit(instance) API that will close
>     the file and commit the changes.
>
>   - Have the tracefs_function_filter() open the set_ftrace_filter file if it
>     is not already opened. This will allow for the function to be called
>     multiple times. The file descriptor will be part of the instance
>     descriptor or a global variable for the top level instance. The opening
>     of the files will need to be protected by a pthread mutex.
>     Note, the "reset" parameter is only applicable if the file is not
>     already opened.
>
>   - On success, return 0 and tracefs_function_filter_commit() must be called.
>
>   - If the file descriptor is opened and an error happens, return 1, and the
>     tracefs_function_filter_commit() still needs to be called, but the user
>     can call tracefs_function_filter() again to try to fix the problem.
>
>   - If an error is found before the file descriptor is opened, then
>     tracefs_function_filter_commit() does not need to be called.
>
> That is:
>
> 	ret = tracefs_function_filter(...);
> 	if (ret >= 0)
> 		tracefs_function_filter_commit();
>
>   - Now for errs, it will be allocated if a problem happened on a filter,
>     and may be set if the return value is non zero (1 or -1). The user can
>     use it to know if the problem happened on a filter as well as find out
>     which filter had the problem.
>
>   - We can have a tracefs_read_function_filter() that returns an array of
>     strings which ends with a NULL pointer (and needs to be freed with
>     tracefs_list_free(), and have this:
>
>     save_filters = tracefs_read_function_filter(instance);
>
>     ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
>     if (ret > 0 && errs) {
> 	/* Modified but failed */
> 	int i, j;
> 	for (i = 0; filters[i]; i++)
> 		;
> 	for (j = 0; errs[i]; j++)
> 		;
> 	if (i == j)
> 		/* all filters failed! Put back the original */
> 		tracefs_function_filter(instance, save_filters, NULL, false, NULL);
>     }
>     if (ret >= 0)
> 	tracefs_function_filter_commit(instance);
>
> Another reason to have it not close the file is because we only support
> passing in one module at a time. If the user wants to enable functions in
> two modules, they would need to call this twice, and we want them to be
> able to do so and have both changes take affect at the same time.
>
> Thoughts?
>
> -- Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-03-25  0:36 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23  1:27 [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
2021-03-23  1:27 ` [PATCH 1/7] libtracefs: An API to set the filtering of functions Steven Rostedt
2021-03-23  1:27 ` [PATCH 2/7] libtracefs: Move opening of file out of controlled_write() Steven Rostedt
2021-03-23  1:27 ` [PATCH 3/7] libtracefs: Add add_errors() helper function for control_write() Steven Rostedt
2021-03-23  1:27 ` [PATCH 4/7] libtracefs: Add checking of available_filter_functions to tracefs_function_filter() Steven Rostedt
2021-03-23  1:28 ` [PATCH 5/7] libtracefs: Add write_filter() helper function Steven Rostedt
2021-03-23  1:28 ` [PATCH 6/7] libtracefs: Allow for setting filters with regex expressions Steven Rostedt
2021-03-23  1:28 ` [PATCH 7/7] libtracefs: Add indexing to set functions in tracefs_function_filter() Steven Rostedt
2021-03-23 12:52 ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Steven Rostedt
2021-03-24 10:13   ` Sameeruddin Shaik
2021-03-24 14:08     ` Steven Rostedt
2021-03-26  0:25   ` sameeruddin shaik
2021-03-25  0:35     ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).