[PATCH 0/3] trace-cmd: Various updates and fixes

[PATCH 0/3] trace-cmd: Various updates and fixes

[Steven Rostedt]

Steven Rostedt (VMware) (3):
      tracefs: Include -ltracecmd to libtracefs.so build
      trace-cmd: Remove error processing from write_file() use error_log instead
      trace-cmd: Fix the looping to clear triggers and filters

----
 lib/tracefs/Makefile    |   2 +
 tracecmd/trace-record.c | 310 +++++++++++++++++++++++++++++++++---------------
 2 files changed, 219 insertions(+), 93 deletions(-)

[PATCH 1/3] tracefs: Include -ltracecmd to libtracefs.so build

[Steven Rostedt]

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

There's functions that libtracefs.so uses that require libtracecmd.so
compiled as well. Instead of requiring applicatons that use -ltracefs to
also include -ltracecmd, simply add it to the build of libtracefs.so.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/tracefs/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/tracefs/Makefile b/lib/tracefs/Makefile
index 5763e06d0326..f6d93ec84236 100644
--- a/lib/tracefs/Makefile
+++ b/lib/tracefs/Makefile
@@ -22,6 +22,8 @@ $(bdir):
 $(OBJS): | $(bdir)
 $(DEPS): | $(bdir)
 
+LIBS = -L$(obj)/lib/trace-cmd -ltracecmd
+
 $(bdir)/libtracefs.a: $(OBJS)
 	$(Q)$(call do_build_static_lib)
 
-- 
2.24.1

[PATCH 2/3] trace-cmd: Remove error processing from write_file() use error_log instead

[Steven Rostedt]

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

When write_file() fails to write the contents given to it, it reads the file
it tried to write to and prints that. This doesn't work for all intsance
files, and newer kernels now have an "error_log" that is written to when
something fails.

Move the processing of reading the file out of write_file() and check if
error_log exists, and read from that when possible, and then default back to
the file if error_log does not exist.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tracecmd/trace-record.c | 135 +++++++++++++++++++++++++++++++++-------
 1 file changed, 114 insertions(+), 21 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index efa7f8787bb7..bfe4f7976cc2 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -790,28 +790,16 @@ static int set_ftrace(int set, int use_proc)
 	return 0;
 }
 
-static int write_file(const char *file, const char *str, const char *type)
+static int write_file(const char *file, const char *str)
 {
-	char buf[BUFSIZ];
-	int fd;
 	int ret;
+	int fd;
 
 	fd = open(file, O_WRONLY | O_TRUNC);
 	if (fd < 0)
 		die("opening to '%s'", file);
 	ret = write(fd, str, strlen(str));
 	close(fd);
-	if (ret < 0 && type) {
-		/* write failed */
-		fd = open(file, O_RDONLY);
-		if (fd < 0)
-			die("writing to '%s'", file);
-		/* the filter has the error */
-		while ((ret = read(fd, buf, BUFSIZ)) > 0)
-			fprintf(stderr, "%.*s", ret, buf);
-		die("Failed %s of %s\n", type, file);
-		close(fd);
-	}
 	return ret;
 }
 
@@ -1949,9 +1937,112 @@ static int find_trigger(const char *file, char *buf, int size, int fields)
 	return len;
 }
 
+static char *read_file(const char *file)
+{
+	char stbuf[BUFSIZ];
+	char *buf = NULL;
+	int size = 0;
+	char *nbuf;
+	int fd;
+	int r;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+
+	do {
+		r = read(fd, stbuf, BUFSIZ);
+		if (r <= 0)
+			continue;
+		nbuf = realloc(buf, size+r+1);
+		if (!nbuf) {
+			free(buf);
+			buf = NULL;
+			break;
+		}
+		buf = nbuf;
+		memcpy(buf+size, stbuf, r);
+		size += r;
+	} while (r > 0);
+
+	close(fd);
+	if (r == 0 && size > 0)
+		buf[size] = '\0';
+
+	return buf;
+}
+
+static void read_error_log(const char *log)
+{
+	char *buf, *line;
+	char *start = NULL;
+	char *p;
+
+	buf = read_file(log);
+	if (!buf)
+		return;
+
+	line = buf;
+
+	/* Only the last lines have meaning */
+	while ((p = strstr(line, "\n")) && p[1]) {
+		if (line[0] != ' ')
+			start = line;
+		line = p + 1;
+	}
+
+	if (start)
+		printf("%s", start);
+
+	free(buf);
+}
+
+static void show_error(const char *file, const char *type)
+{
+	struct stat st;
+	char *path = strdup(file);
+	char *p;
+	int ret;
+
+	if (!path)
+		die("Could not allocate memory");
+
+	p = strstr(path, "tracing");
+	if (p) {
+		if (strncmp(p + sizeof("tracing"), "instances", sizeof("instances") - 1) == 0) {
+			p = strstr(p + sizeof("tracing") + sizeof("instances"), "/");
+			if (!p)
+				goto read_file;
+		} else {
+			p += sizeof("tracing") - 1;
+		}
+		ret = asprintf(&p, "%.*s/error_log", (int)(p - path), path);
+		if (ret < 0)
+			die("Could not allocate memory");
+		ret = stat(p, &st);
+		if (ret < 0) {
+			free(p);
+			goto read_file;
+		}
+		read_error_log(p);
+		goto out;
+	}
+
+ read_file:
+	p = read_file(path);
+	if (p)
+		printf("%s", p);
+
+ out:
+	printf("Failed %s of %s\n", type, file);
+	free(path);
+	return;
+}
+
 static void write_filter(const char *file, const char *filter)
 {
-	write_file(file, filter, "filter");
+	if (write_file(file, filter) < 0)
+		show_error(file, "filter");
 }
 
 static void clear_filter(const char *file)
@@ -1961,12 +2052,14 @@ static void clear_filter(const char *file)
 
 static void write_trigger(const char *file, const char *trigger)
 {
-	write_file(file, trigger, "trigger");
+	if (write_file(file, trigger) < 0)
+		show_error(file, "trigger");
 }
 
 static void write_func_filter(const char *file, const char *trigger)
 {
-	write_file(file, trigger, "function filter");
+	if (write_file(file, trigger) < 0)
+		show_error(file, "function filter");
 }
 
 static void clear_trigger(const char *file)
@@ -2059,7 +2152,7 @@ static void update_reset_files(void)
 		reset_files = reset->next;
 
 		if (!keep)
-			write_file(reset->path, reset->reset, "reset");
+			write_file(reset->path, reset->reset);
 		free(reset->path);
 		free(reset->reset);
 		free(reset);
@@ -4193,7 +4286,7 @@ static unsigned long long find_time_stamp(struct tep_handle *tep)
 }
 
 
-static char *read_file(char *file, int *psize)
+static char *read_top_file(char *file, int *psize)
 {
 	return tracefs_instance_file_read(top_instance.tracefs, file, psize);
 }
@@ -4231,7 +4324,7 @@ static char *get_date_to_ts(void)
 
 	tep_set_file_bigendian(tep, tracecmd_host_bigendian());
 
-	buf = read_file("events/header_page", &size);
+	buf = read_top_file("events/header_page", &size);
 	if (!buf)
 		goto out_pevent;
 	ret = tep_parse_header_page(tep, buf, size, sizeof(unsigned long));
@@ -4596,7 +4689,7 @@ static void check_plugin(const char *plugin)
 	if (strcmp(plugin, "nop") == 0)
 		return;
 
-	buf = read_file("available_tracers", NULL);
+	buf = read_top_file("available_tracers", NULL);
 	if (!buf)
 		die("No plugins available");
 
-- 
2.24.1

[PATCH 3/3] trace-cmd: Fix the looping to clear triggers and filters

[Steven Rostedt]

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

The resetting of filters and triggers did a loop on each trigger and filter
till it was cleared. The point being, it removed one trigger or filter at a
time. This could cause an infinite loop due to some triggers requiring an
"ordering" of removal. When histograms are used with synthetic events, one
histogram will be attached to another histogram, and the remove of the
first histogram will fail to be removed if the second histogram is still
using it.

As the code would just keep iterating on the same trigger file, and that
file would never be cleared due to the histogram dependency, it went into an
infinite loop.

Instead of a loop that would read the file and try to remove the first
trigger, read the entire file at once, then try removing the each of the
the triggers that was read one at a time. After all have been attempted to
be removed, re-read the file. If the file still has data, take note, and
continue to the other files.

After processing all the files, check to see how many files still had data,
then loop through all the files again, doing the same thing, but only do it
the number of times that there was content in the files, as that should
remove all the triggers no matter what the dependency is.

We could optimize this by only reseting the files that still had data in it,
but that would require making full copy of the event_iter then running on
that. As this doesn't happen much, doing a full scan again shoudn't be too
much of an issue. But if it becomes one, then the optimization can still
happen later.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tracecmd/trace-record.c | 199 +++++++++++++++++++++++-----------------
 1 file changed, 115 insertions(+), 84 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index bfe4f7976cc2..4a49b640b66b 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1899,44 +1899,6 @@ enum {
 	STATE_COPY,
 };
 
-static int find_trigger(const char *file, char *buf, int size, int fields)
-{
-	FILE *fp;
-	int state = STATE_NEWLINE;
-	int ch;
-	int len = 0;
-
-	fp = fopen(file, "r");
-	if (!fp)
-		return 0;
-
-	while ((ch = fgetc(fp)) != EOF) {
-		if (ch == '\n') {
-			if (state == STATE_COPY)
-				break;
-			state = STATE_NEWLINE;
-			continue;
-		}
-		if (state == STATE_SKIP)
-			continue;
-		if (state == STATE_NEWLINE && ch == '#') {
-			state = STATE_SKIP;
-			continue;
-		}
-		if (state == STATE_COPY && ch == ':' && --fields < 1)
-			break;
-
-		state = STATE_COPY;
-		buf[len++] = ch;
-		if (len == size - 1)
-			break;
-	}
-	buf[len] = 0;
-	fclose(fp);
-
-	return len;
-}
-
 static char *read_file(const char *file)
 {
 	char stbuf[BUFSIZ];
@@ -2056,34 +2018,63 @@ static void write_trigger(const char *file, const char *trigger)
 		show_error(file, "trigger");
 }
 
-static void write_func_filter(const char *file, const char *trigger)
-{
-	if (write_file(file, trigger) < 0)
-		show_error(file, "function filter");
-}
-
-static void clear_trigger(const char *file)
+static int clear_trigger(const char *file)
 {
 	char trigger[BUFSIZ];
+	char *save = NULL;
+	char *line;
+	char *buf;
 	int len;
+	int ret;
+
+	buf = read_file(file);
+	if (!buf) {
+		perror(file);
+		return 0;
+	}
 
 	trigger[0] = '!';
 
+	for (line = strtok_r(buf, "\n", &save); line; line = strtok_r(NULL, "\n", &save)) {
+		if (line[0] == '#')
+			continue;
+		len = strlen(line);
+		if (len > BUFSIZ - 2)
+			len = BUFSIZ - 2;
+		strncpy(trigger + 1, line, len);
+		trigger[len + 1] = '\0';
+		/* We don't want any filters or extra on the line */
+		strtok(trigger, " ");
+		write_file(file, trigger);
+	}
+
+	free(buf);
+
 	/*
-	 * To delete a trigger, we need to write a '!trigger'
-	 * to the file for each trigger.
+	 * Some triggers have an order in removing them.
+	 * They will not be removed if done in the wrong order.
 	 */
-	do {
-		len = find_trigger(file, trigger+1, BUFSIZ-1, 3);
-		if (len)
-			write_trigger(file, trigger);
-	} while (len);
+	buf = read_file(file);
+	if (!buf)
+		return 0;
+
+	ret = 0;
+	for (line = strtok(buf, "\n"); line; line = strtok(NULL, "\n")) {
+		if (line[0] == '#')
+			continue;
+		ret = 1;
+		break;
+	}
+	free(buf);
+	return ret;
 }
 
 static void clear_func_filter(const char *file)
 {
-	char trigger[BUFSIZ];
+	char filter[BUFSIZ];
 	struct stat st;
+	char *line;
+	char *buf;
 	char *p;
 	int len;
 	int ret;
@@ -2100,33 +2091,44 @@ static void clear_func_filter(const char *file)
 		die("opening to '%s'", file);
 	close(fd);
 
-	/* Now remove triggers */
-	trigger[0] = '!';
+	buf = read_file(file);
+	if (!buf) {
+		perror(file);
+		return;
+	}
+
+	/* Now remove filters */
+	filter[0] = '!';
 
 	/*
-	 * To delete a trigger, we need to write a '!trigger'
-	 * to the file for each trigger.
+	 * To delete a filter, we need to write a '!filter'
+	 * to the file for each filter.
 	 */
-	do {
-		len = find_trigger(file, trigger+1, BUFSIZ-1, 3);
-		if (len) {
-			/*
-			 * To remove "unlimited" triggers, we must remove
-			 * the ":unlimited" from what we write.
-			 */
-			if ((p = strstr(trigger, ":unlimited"))) {
-				*p = '\0';
-				len = p - trigger;
-			}
-			/*
-			 * The write to this file expects white space
-			 * at the end :-p
-			 */
-			trigger[len] = '\n';
-			trigger[len+1] = '\0';
-			write_func_filter(file, trigger);
+	for (line = strtok(buf, "\n"); line; line = strtok(NULL, "\n")) {
+		if (line[0] == '#')
+			continue;
+		len = strlen(line);
+		if (len > BUFSIZ - 2)
+			len = BUFSIZ - 2;
+
+		strncpy(filter + 1, line, len);
+		filter[len + 1] = '\0';
+		/*
+		 * To remove "unlimited" filters, we must remove
+		 * the ":unlimited" from what we write.
+		 */
+		if ((p = strstr(filter, ":unlimited"))) {
+			*p = '\0';
+			len = p - filter;
 		}
-	} while (len > 0);
+		/*
+		 * The write to this file expects white space
+		 * at the end :-p
+		 */
+		filter[len] = '\n';
+		filter[len+1] = '\0';
+		write_file(file, filter);
+	}
 }
 
 static void update_reset_triggers(void)
@@ -4435,8 +4437,8 @@ void set_buffer_size(void)
 		set_buffer_size_instance(instance);
 }
 
-static void
-process_event_trigger(char *path, struct event_iter *iter, enum event_process *processed)
+static int
+process_event_trigger(char *path, struct event_iter *iter)
 {
 	const char *system = iter->system_dent->d_name;
 	const char *event = iter->event_dent->d_name;
@@ -4459,19 +4461,21 @@ process_event_trigger(char *path, struct event_iter *iter, enum event_process *p
 	if (ret < 0)
 		goto out;
 
-	clear_trigger(trigger);
+	ret = clear_trigger(trigger);
  out:
 	free(trigger);
 	free(file);
+	return ret;
 }
 
 static void clear_instance_triggers(struct buffer_instance *instance)
 {
+	enum event_iter_type type;
 	struct event_iter *iter;
-	char *path;
 	char *system;
-	enum event_iter_type type;
-	enum event_process processed = PROCESSED_NONE;
+	char *path;
+	int retry = 0;
+	int ret;
 
 	path = tracefs_instance_get_file(instance->tracefs, "events");
 	if (!path)
@@ -4479,7 +4483,6 @@ static void clear_instance_triggers(struct buffer_instance *instance)
 
 	iter = trace_event_iter_alloc(path);
 
-	processed = PROCESSED_NONE;
 	system = NULL;
 	while ((type = trace_event_iter_next(iter, path, system))) {
 
@@ -4488,11 +4491,39 @@ static void clear_instance_triggers(struct buffer_instance *instance)
 			continue;
 		}
 
-		process_event_trigger(path, iter, &processed);
+		ret = process_event_trigger(path, iter);
+		if (ret > 0)
+			retry++;
 	}
 
 	trace_event_iter_free(iter);
 
+	if (retry) {
+		int i;
+
+		/* Order matters for some triggers */
+		for (i = 0; i < retry; i++) {
+			int tries = 0;
+
+			iter = trace_event_iter_alloc(path);
+			system = NULL;
+			while ((type = trace_event_iter_next(iter, path, system))) {
+
+				if (type == EVENT_ITER_SYSTEM) {
+					system = iter->system_dent->d_name;
+					continue;
+				}
+
+				ret = process_event_trigger(path, iter);
+				if (ret > 0)
+					tries++;
+			}
+			trace_event_iter_free(iter);
+			if (!tries)
+				break;
+		}
+	}
+
 	tracefs_put_tracing_file(path);
 }
 
-- 
2.24.1