[PATCH 0/3] Fix applying triggers to multiple events

[PATCH 0/3] Fix applying triggers to multiple events

[Tzvetomir Stoyanov (VMware)]

trace-cmd supports applying filters to multiple events from given:
 "trace-cmd record -e sched -R stacktrace"
 "trace-cmd record -e sched: -R stacktrace"
 "trace-cmd record -e sched:* -R stacktrace"
but this functionality was not tested, there were few problems with
it.

Tzvetomir Stoyanov (VMware) (3):
  trace-cmd: Fix check for trigger file in create_event()
  trace-cmd: Fix double free error when triggers are configured on
    multiple events
  trace-cmd: Fix setting triggers to all events from given system

 tracecmd/trace-record.c | 101 ++++++++++++++++++++++++++--------------
 1 file changed, 67 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH 1/3] trace-cmd: Fix check for trigger file in create_event()

[Tzvetomir Stoyanov (VMware)]

When a trigger is configured on an event with trace-cmd "-R"
option there is a check in create_event() if the trigger file
exist, using the stat() call. It returns 0 if the file exists
or -1 in case of an error. The check of the stat()'s return code
is for positive number, which is always false and errors are never
detected.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index c8c853f8..ec3bcae9 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2707,7 +2707,7 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
 		if (ret < 0)
 			die("Failed to allocate trigger path for %s", path);
 		ret = stat(p, &st);
-		if (ret > 0)
+		if (ret < 0)
 			die("trigger specified but not supported by this kernel");
 		event->trigger_file = p;
 	}
-- 
2.25.1

[PATCH 2/3] trace-cmd: Fix double free error when triggers are configured on multiple events

[Tzvetomir Stoyanov (VMware)]

When using trace-cmd "-e sched:* -R ..." command to set triggers to the all events
of given subsystem, trace-cmd fails with a double free error.
The problem is in create_event() function, where the config is expanded to the all
events. The same "event->trigger" pointer is copied to all events. Later, when the
trigger is applied, the update_event() frees this pointer multiple times.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index ec3bcae9..7dbf599a 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2702,7 +2702,8 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
 	else
 		free(p);
 
-	if (event->trigger) {
+	if (old_event->trigger) {
+		event->trigger = strdup(old_event->trigger);
 		ret = asprintf(&p, "%s/trigger", path_dirname);
 		if (ret < 0)
 			die("Failed to allocate trigger path for %s", path);
@@ -2883,6 +2884,7 @@ static void expand_event_instance(struct buffer_instance *instance)
 		event = compressed_list;
 		compressed_list = event->next;
 		expand_event(instance, event);
+		free(event->trigger);
 		free(event);
 	}
 }
-- 
2.25.1

[PATCH 3/3] trace-cmd: Fix setting triggers to all events from given system

[Tzvetomir Stoyanov (VMware)]

When running the "trace-cmd record -e (system) -R (trigger)" command for an event system, it fails with
"no such file or directory error". This use case is supposed to apply the given trigger to all events
from the system.

Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=206737
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 99 +++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 34 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7dbf599a..76305d1a 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2658,6 +2658,21 @@ static void set_max_graph_depth(struct buffer_instance *instance, char *max_grap
 		die("could not write to max_graph_depth");
 }
 
+static bool check_file_in_dir(char *dir, char *file)
+{
+	struct stat st;
+	char *path;
+	int ret;
+
+	ret = asprintf(&path, "%s/%s", dir, file);
+	if (ret < 0)
+		die("Failed to allocate id file path for %s/%s", dir, file);
+	ret = stat(path, &st);
+	free(path);
+	if (ret < 0 || S_ISDIR(st.st_mode))
+		return false;
+	return true;
+}
 
 /**
  * create_event - create and event descriptor
@@ -2703,14 +2718,19 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
 		free(p);
 
 	if (old_event->trigger) {
-		event->trigger = strdup(old_event->trigger);
-		ret = asprintf(&p, "%s/trigger", path_dirname);
-		if (ret < 0)
-			die("Failed to allocate trigger path for %s", path);
-		ret = stat(p, &st);
-		if (ret < 0)
-			die("trigger specified but not supported by this kernel");
-		event->trigger_file = p;
+		if (check_file_in_dir(path_dirname, "trigger")) {
+			event->trigger = strdup(old_event->trigger);
+			ret = asprintf(&p, "%s/trigger", path_dirname);
+			if (ret < 0)
+				die("Failed to allocate trigger path for %s", path);
+			event->trigger_file = p;
+		} else {
+			/* Check if this is event or system.
+			 * Systems do not have trigger files by design
+			 */
+			if (check_file_in_dir(path_dirname, "id"))
+				die("trigger specified but not supported by this kernel");
+		}
 	}
 
 	return event;
@@ -2815,14 +2835,29 @@ static int expand_event_files(struct buffer_instance *instance,
 	return save_event_tail == instance->event_next;
 }
 
+static int expand_events_all(struct buffer_instance *instance,
+			     char *system_name, char *event_name,
+			     struct event_list *event)
+{
+	char *name;
+	int ret;
+
+	ret = asprintf(&name, "%s/%s", system_name, event_name);
+	if (ret < 0)
+		die("Failed to allocate system/event for %s/%s",
+		     system_name, event_name);
+	ret = expand_event_files(instance, name, event);
+	free(name);
+
+	return ret;
+}
+
 static void expand_event(struct buffer_instance *instance, struct event_list *event)
 {
 	const char *name = event->event;
 	char *str;
 	char *ptr;
-	int len;
 	int ret;
-	int ret2;
 
 	/*
 	 * We allow the user to use "all" to enable all events.
@@ -2833,41 +2868,37 @@ static void expand_event(struct buffer_instance *instance, struct event_list *ev
 		return;
 	}
 
-	ptr = strchr(name, ':');
+	str = strdup(name);
+	if (!str)
+		die("Failed to allocate %s string", name);
 
+	ptr = strchr(str, ':');
 	if (ptr) {
-		len = ptr - name;
-		str = malloc(strlen(name) + 1); /* may add '*' */
-		if (!str)
-			die("Failed to allocate event for %s", name);
-		strcpy(str, name);
-		str[len] = '/';
+		*ptr = '\0';
 		ptr++;
-		if (!strlen(ptr)) {
-			str[len + 1] = '*';
-			str[len + 2] = '\0';
-		}
 
-		ret = expand_event_files(instance, str, event);
+		if (strlen(ptr))
+			ret = expand_events_all(instance, str, ptr, event);
+		else
+			ret = expand_events_all(instance, str, "*", event);
+
 		if (!ignore_event_not_found && ret)
 			die("No events enabled with %s", name);
-		free(str);
-		return;
+
+		goto out;
 	}
 
 	/* No ':' so enable all matching systems and events */
-	ret = expand_event_files(instance, name, event);
-
-	len = strlen(name) + strlen("*/") + 1;
-	str = malloc(len);
-	if (!str)
-		die("Failed to allocate event for %s", name);
-	snprintf(str, len, "*/%s", name);
-	ret2 = expand_event_files(instance, str, event);
-	free(str);
+	ret = expand_event_files(instance, str, event);
+	ret &= expand_events_all(instance, "*", str, event);
+	if (event->trigger)
+		ret &= expand_events_all(instance, str, "*", event);
 
-	if (!ignore_event_not_found && ret && ret2)
+	if (!ignore_event_not_found && ret)
 		die("No events enabled with %s", name);
+
+out:
+	free(str);
 }
 
 static void expand_event_instance(struct buffer_instance *instance)
-- 
2.25.1