[PATCH 0/2] Enhancements to trace-cmd clear

[PATCH 0/2] Enhancements to trace-cmd clear

[Tzvetomir Stoyanov (VMware)]

Added new options to trace-cmd clear, alowwing it to be ftrace instance
aware.
Implemented new tracefs API for iterating over the configured ftrace
instances.

Tzvetomir Stoyanov (VMware) (2):
  trace-cmd: Add new tracefs API tracefs_instances_walk()
  trace-cmd: Enhance "trace-cmd clear" to be instance aware

 Documentation/trace-cmd-clear.1.txt |  13 ++-
 include/tracefs/tracefs.h           |   1 +
 lib/tracefs/include/tracefs-local.h |   1 +
 lib/tracefs/tracefs-events.c        |  14 ++--
 lib/tracefs/tracefs-instance.c      |  52 ++++++++++++
 tracecmd/Makefile                   |   1 +
 tracecmd/trace-clear.c              | 124 ++++++++++++++++++++++++++++
 tracecmd/trace-record.c             |  24 ------
 tracecmd/trace-stat.c               |  52 +++---------
 tracecmd/trace-usage.c              |   4 +-
 10 files changed, 214 insertions(+), 72 deletions(-)
 create mode 100644 tracecmd/trace-clear.c

-- 
2.26.2

[PATCH 1/2] trace-cmd: Add new tracefs API tracefs_instances_walk()

[Tzvetomir Stoyanov (VMware)]

The logic for finding all configured ftrace instances is encapuslated in
the new tracefs_instances_walk() API. A user specified callback is
called for each ftrace instance in the system, excpet for the top level
one.
The implementation of "trace-cmd stat" is modified to use the new API.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h           |  1 +
 lib/tracefs/include/tracefs-local.h |  1 +
 lib/tracefs/tracefs-events.c        | 14 ++++----
 lib/tracefs/tracefs-instance.c      | 52 +++++++++++++++++++++++++++++
 tracecmd/trace-stat.c               | 52 ++++++++---------------------
 5 files changed, 74 insertions(+), 46 deletions(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index 8ee7ba6e..0dd8046f 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -32,6 +32,7 @@ int tracefs_instance_file_write(struct tracefs_instance *instance,
 				const char *file, const char *str);
 char *tracefs_instance_file_read(struct tracefs_instance *instance,
 				 char *file, int *psize);
+int tracefs_instances_walk(int (*callback)(const char *, void *), void *context);
 
 bool tracefs_file_exists(struct tracefs_instance *instance, char *name);
 bool tracefs_dir_exists(struct tracefs_instance *instance, char *name);
diff --git a/lib/tracefs/include/tracefs-local.h b/lib/tracefs/include/tracefs-local.h
index fe327a0f..08b67fa9 100644
--- a/lib/tracefs/include/tracefs-local.h
+++ b/lib/tracefs/include/tracefs-local.h
@@ -9,5 +9,6 @@
 /* Can be overridden */
 void warning(const char *fmt, ...);
 int str_read_file(const char *file, char **buffer);
+char *trace_append_file(const char *dir, const char *name);
 
 #endif /* _TRACE_FS_LOCAL_H */
diff --git a/lib/tracefs/tracefs-events.c b/lib/tracefs/tracefs-events.c
index 8e825f50..564a5ab2 100644
--- a/lib/tracefs/tracefs-events.c
+++ b/lib/tracefs/tracefs-events.c
@@ -210,7 +210,7 @@ static char **add_list_string(char **list, const char *name, int len)
 	return list;
 }
 
-static char *append_file(const char *dir, const char *name)
+char *trace_append_file(const char *dir, const char *name)
 {
 	char *file;
 	int ret;
@@ -265,7 +265,7 @@ char **tracefs_event_systems(const char *tracing_dir)
 	if (!tracing_dir)
 		return NULL;
 
-	events_dir = append_file(tracing_dir, "events");
+	events_dir = trace_append_file(tracing_dir, "events");
 	if (!events_dir)
 		return NULL;
 
@@ -290,14 +290,14 @@ char **tracefs_event_systems(const char *tracing_dir)
 		    strcmp(name, "..") == 0)
 			continue;
 
-		sys = append_file(events_dir, name);
+		sys = trace_append_file(events_dir, name);
 		ret = stat(sys, &st);
 		if (ret < 0 || !S_ISDIR(st.st_mode)) {
 			free(sys);
 			continue;
 		}
 
-		enable = append_file(sys, "enable");
+		enable = trace_append_file(sys, "enable");
 
 		ret = stat(enable, &st);
 		if (ret >= 0)
@@ -359,7 +359,7 @@ char **tracefs_system_events(const char *tracing_dir, const char *system)
 		    strcmp(name, "..") == 0)
 			continue;
 
-		event = append_file(system_dir, name);
+		event = trace_append_file(system_dir, name);
 		ret = stat(event, &st);
 		if (ret < 0 || !S_ISDIR(st.st_mode)) {
 			free(event);
@@ -401,7 +401,7 @@ char **tracefs_tracers(const char *tracing_dir)
 	if (!tracing_dir)
 		return NULL;
 
-	available_tracers = append_file(tracing_dir, "available_tracers");
+	available_tracers = trace_append_file(tracing_dir, "available_tracers");
 	if (!available_tracers)
 		return NULL;
 
@@ -497,7 +497,7 @@ static int read_header(struct tep_handle *tep, const char *tracing_dir)
 	int len;
 	int ret = -1;
 
-	header = append_file(tracing_dir, "events/header_page");
+	header = trace_append_file(tracing_dir, "events/header_page");
 
 	ret = stat(header, &st);
 	if (ret < 0)
diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index 50e88534..befc0333 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <dirent.h>
 #include <linux/limits.h>
 #include "tracefs.h"
 #include "tracefs-local.h"
@@ -291,3 +292,54 @@ bool tracefs_dir_exists(struct tracefs_instance *instance, char *name)
 {
 	return check_file_exists(instance, name, true);
 }
+
+/**
+ * tracefs_instances_walk - Iterate through all ftrace instances in the system
+ * @callback: user callback, called for each instance. Instance name is passed
+ *	      as input parameter. If the @callback returns non-zero,
+ *	      the iteration stops.
+ * @context: user context, passed to the @callback.
+ *
+ * Returns -1 in case of an error, 0 otherwise.
+ */
+int tracefs_instances_walk(int (*callback)(const char *, void *), void *context)
+{
+	struct dirent *dent;
+	char *path = NULL;
+	DIR *dir = NULL;
+	struct stat st;
+	int fret = -1;
+	int ret;
+
+	path = tracefs_get_tracing_file("instances");
+	if (!path)
+		return -1;
+	ret = stat(path, &st);
+	if (ret < 0 || !S_ISDIR(st.st_mode))
+		goto out;
+
+	dir = opendir(path);
+	if (!dir)
+		goto out;
+	fret = 0;
+	while ((dent = readdir(dir))) {
+		char *instance;
+
+		if (strcmp(dent->d_name, ".") == 0 ||
+		    strcmp(dent->d_name, "..") == 0)
+			continue;
+		instance = trace_append_file(path, dent->d_name);
+		ret = stat(instance, &st);
+		free(instance);
+		if (ret < 0 || !S_ISDIR(st.st_mode))
+			continue;
+		if (callback(dent->d_name, context))
+			break;
+	}
+
+out:
+	if (dir)
+		closedir(dir);
+	tracefs_put_tracing_file(path);
+	return fret;
+}
diff --git a/tracecmd/trace-stat.c b/tracecmd/trace-stat.c
index 260d0c37..c242dae2 100644
--- a/tracecmd/trace-stat.c
+++ b/tracecmd/trace-stat.c
@@ -7,7 +7,6 @@
 #include <sys/stat.h>
 #include <stdlib.h>
 #include <stdio.h>
-#include <dirent.h>
 #include <getopt.h>
 #include <unistd.h>
 #include <fcntl.h>
@@ -140,48 +139,23 @@ static void report_file(struct buffer_instance *instance,
 	free(str);
 }
 
-static void report_instances(void)
+static int report_instance(const char *name, void *data)
 {
-	struct dirent *dent;
-	bool first = true;
-	char *path = NULL;
-	DIR *dir = NULL;
-	struct stat st;
-	int ret;
-
-	path = tracefs_get_tracing_file("instances");
-	if (!path)
-		return;
-	ret = stat(path, &st);
-	if (ret < 0 || !S_ISDIR(st.st_mode))
-		goto out;
-
-	dir = opendir(path);
-	if (!dir)
-		goto out;
-
-	while ((dent = readdir(dir))) {
-		char *instance;
+	bool *first = (bool *)data;
 
-		if (strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0)
-			continue;
-		instance = append_file(path, dent->d_name);
-		ret = stat(instance, &st);
-		free(instance);
-		if (ret < 0 || !S_ISDIR(st.st_mode))
-			continue;
-		if (first) {
-			first = false;
-			printf("\nInstances:\n");
-		}
-		printf(" %s\n", dent->d_name);
+	if (*first) {
+		*first = false;
+		printf("\nInstances:\n");
 	}
+	printf(" %s\n", name);
+	return 0;
+}
 
-out:
-	if (dir)
-		closedir(dir);
-	tracefs_put_tracing_file(path);
+static void report_instances(void)
+{
+	bool first = true;
+
+	tracefs_instances_walk(report_instance, &first);
 }
 
 struct event_iter *trace_event_iter_alloc(const char *path)
-- 
2.26.2

[PATCH 2/2] trace-cmd: Enhance "trace-cmd clear" to be instance aware

[Tzvetomir Stoyanov (VMware)]

Added new options "trace-cmd clear -B <name> -a" which allow the command
to work per ftrace instance:
-B clear the given buffer (may specify multiple -B)
-a clear all existing buffers, including the top level one

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 Documentation/trace-cmd-clear.1.txt |  13 ++-
 tracecmd/Makefile                   |   1 +
 tracecmd/trace-clear.c              | 124 ++++++++++++++++++++++++++++
 tracecmd/trace-record.c             |  24 ------
 tracecmd/trace-usage.c              |   4 +-
 5 files changed, 140 insertions(+), 26 deletions(-)
 create mode 100644 tracecmd/trace-clear.c

diff --git a/Documentation/trace-cmd-clear.1.txt b/Documentation/trace-cmd-clear.1.txt
index 67e5851a..a0ae36e9 100644
--- a/Documentation/trace-cmd-clear.1.txt
+++ b/Documentation/trace-cmd-clear.1.txt
@@ -7,12 +7,23 @@ trace-cmd-clear - clear the Ftrace buffer.
 
 SYNOPSIS
 --------
-*trace-cmd clear*
+*trace-cmd clear* ['OPTIONS']
 
 DESCRIPTION
 -----------
 The *trace-cmd(1) clear* clears the content of the Ftrace ring buffer.
 
+OPTIONS
+-------
+*-B* 'buffer-name'::
+    If the kernel supports multiple buffers, this will clear only the given
+    buffer. It does not affect any other buffers. This may be used multiple
+    times to specify different buffers. The top level buffer will not be
+    clearded if this option is given.
+
+*-a*::
+    Clear all existing buffers, including the top level one.
+
 SEE ALSO
 --------
 trace-cmd(1), trace-cmd-record(1), trace-cmd-report(1), trace-cmd-start(1),
diff --git a/tracecmd/Makefile b/tracecmd/Makefile
index 5e59adf8..01f36c61 100644
--- a/tracecmd/Makefile
+++ b/tracecmd/Makefile
@@ -31,6 +31,7 @@ TRACE_CMD_OBJS += trace-show.o
 TRACE_CMD_OBJS += trace-list.o
 TRACE_CMD_OBJS += trace-usage.o
 TRACE_CMD_OBJS += trace-dump.o
+TRACE_CMD_OBJS += trace-clear.o
 ifeq ($(VSOCK_DEFINED), 1)
 TRACE_CMD_OBJS += trace-tsync.o
 endif
diff --git a/tracecmd/trace-clear.c b/tracecmd/trace-clear.c
new file mode 100644
index 00000000..f1b4f309
--- /dev/null
+++ b/tracecmd/trace-clear.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: LGPL-2.1
+/*
+ * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
+ *
+ * Updates:
+ * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
+ *
+ */
+#include <stdlib.h>
+#include <unistd.h>
+#include <getopt.h>
+
+#include "tracefs.h"
+#include "trace-local.h"
+
+struct instances_list {
+	struct instances_list *next;
+	struct tracefs_instance *instance;
+};
+
+static int add_new_instance(struct instances_list **list, char *name)
+{
+	struct instances_list *new;
+
+	new = calloc(1, sizeof(*new));
+	if (!new)
+		return -1;
+	new->instance = tracefs_instance_alloc(name);
+	if (!new->instance) {
+		free(new);
+		return -1;
+	}
+
+	new->next = *list;
+	*list = new;
+	return 0;
+}
+
+static int add_instance_walk(const char *name, void *data)
+{
+	return add_new_instance((struct instances_list **)data, (char *)name);
+}
+
+static void clear_list(struct instances_list *list)
+{
+	struct instances_list *del;
+
+	while (list) {
+		del = list;
+		list = list->next;
+		tracefs_instance_free(del->instance);
+		free(del);
+	}
+}
+
+static void clear_instance_trace(struct tracefs_instance *instance)
+{
+	FILE *fp;
+	char *path;
+
+	/* reset the trace */
+	path = tracefs_instance_get_file(instance, "trace");
+	fp = fopen(path, "w");
+	if (!fp)
+		die("writing to '%s'", path);
+	tracefs_put_tracing_file(path);
+	fwrite("0", 1, 1, fp);
+	fclose(fp);
+}
+
+static void clear_trace(struct instances_list *instances)
+{
+	if (instances) {
+		while (instances) {
+			clear_instance_trace(instances->instance);
+			instances = instances->next;
+		}
+	} else
+		clear_instance_trace(NULL);
+}
+
+void trace_clear(int argc, char **argv)
+{
+	struct instances_list *instances = NULL;
+	bool all = false;
+	int c;
+
+	for (;;) {
+		int option_index = 0;
+		static struct option long_options[] = {
+			{"all", no_argument, NULL, 'a'},
+			{"help", no_argument, NULL, '?'},
+			{NULL, 0, NULL, 0}
+		};
+
+		c = getopt_long (argc-1, argv+1, "+haB:",
+				 long_options, &option_index);
+		if (c == -1)
+			break;
+		switch (c) {
+		case 'B':
+			if (add_new_instance(&instances, optarg))
+				die("Failed to allocate instance %s", optarg);
+			break;
+		case 'a':
+			all = true;
+			if (tracefs_instances_walk(add_instance_walk, &instances))
+				die("Failed to add all instances");
+			break;
+		case 'h':
+		case '?':
+		default:
+			usage(argv);
+			break;
+		}
+	}
+
+	clear_trace(instances);
+	if (all)
+		clear_trace(NULL);
+	clear_list(instances);
+	exit(0);
+}
+
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index bd004574..5009eaa1 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -835,21 +835,6 @@ static void clear_trace_instances(void)
 		__clear_trace(instance);
 }
 
-static void clear_trace(void)
-{
-	FILE *fp;
-	char *path;
-
-	/* reset the trace */
-	path = tracefs_get_tracing_file("trace");
-	fp = fopen(path, "w");
-	if (!fp)
-		die("writing to '%s'", path);
-	tracefs_put_tracing_file(path);
-	fwrite("0", 1, 1, fp);
-	fclose(fp);
-}
-
 static void reset_max_latency(struct buffer_instance *instance)
 {
 	tracefs_instance_file_write(instance->tracefs,
@@ -6704,15 +6689,6 @@ void trace_profile(int argc, char **argv)
 	exit(0);
 }
 
-void trace_clear(int argc, char **argv)
-{
-	if (argc > 2)
-		usage(argv);
-	else
-		clear_trace();
-	exit(0);
-}
-
 void trace_record(int argc, char **argv)
 {
 	struct common_record_context ctx;
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index 3f0b2d07..79610bd3 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -180,7 +180,9 @@ static struct usage_help usage_help[] = {
 	{
 		"clear",
 		"clear the trace buffers",
-		" %s clear\n"
+		" %s clear [-B buf][-a]\n"
+		"          -B clear the given buffer (may specify multiple -B)\n"
+		"          -a clear all existing buffers, including the top level one\n"
 	},
 	{
 		"report",
-- 
2.26.2

Re: [PATCH 1/2] trace-cmd: Add new tracefs API tracefs_instances_walk()

[Steven Rostedt]

On Fri, 17 Jul 2020 11:03:25 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The logic for finding all configured ftrace instances is encapuslated in
> the new tracefs_instances_walk() API. A user specified callback is
> called for each ftrace instance in the system, excpet for the top level
> one.
> The implementation of "trace-cmd stat" is modified to use the new API.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/tracefs/tracefs.h           |  1 +
>  lib/tracefs/include/tracefs-local.h |  1 +
>  lib/tracefs/tracefs-events.c        | 14 ++++----
>  lib/tracefs/tracefs-instance.c      | 52 +++++++++++++++++++++++++++++
>  tracecmd/trace-stat.c               | 52 ++++++++---------------------
>  5 files changed, 74 insertions(+), 46 deletions(-)
> 
> diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
> index 8ee7ba6e..0dd8046f 100644
> --- a/include/tracefs/tracefs.h
> +++ b/include/tracefs/tracefs.h
> @@ -32,6 +32,7 @@ int tracefs_instance_file_write(struct tracefs_instance *instance,
>  				const char *file, const char *str);
>  char *tracefs_instance_file_read(struct tracefs_instance *instance,
>  				 char *file, int *psize);
> +int tracefs_instances_walk(int (*callback)(const char *, void *), void *context);

Perhaps a better name would be: tracefs_instance_iterate()?

I usually consider a "walk" as going through a tree, not a list. If
this was a directory tree, then "walk" would be more appropriate.

An "iterator" is something that walks through a list and triggers a
callback, like this does.

But other than that, the patch looks good.

-- Steve


>  
>  bool tracefs_file_exists(struct tracefs_instance *instance, char *name);
>  bool tracefs_dir_exists(struct tracefs_instance *instance, char *name);
> diff --git a/lib/tracefs/include/tracefs-local.h b/lib/tracefs/include/tracefs-local.h
> index fe327a0f..08b67fa9 100644
> --- a/lib/tracefs/include/tracefs-local.h
> 

Re: [PATCH 2/2] trace-cmd: Enhance "trace-cmd clear" to be instance aware

[Steven Rostedt]

On Fri, 17 Jul 2020 11:03:26 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Added new options "trace-cmd clear -B <name> -a" which allow the command
> to work per ftrace instance:
> -B clear the given buffer (may specify multiple -B)
> -a clear all existing buffers, including the top level one
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  Documentation/trace-cmd-clear.1.txt |  13 ++-
>  tracecmd/Makefile                   |   1 +
>  tracecmd/trace-clear.c              | 124 ++++++++++++++++++++++++++++
>  tracecmd/trace-record.c             |  24 ------
>  tracecmd/trace-usage.c              |   4 +-
>  5 files changed, 140 insertions(+), 26 deletions(-)
>  create mode 100644 tracecmd/trace-clear.c
> 
> diff --git a/Documentation/trace-cmd-clear.1.txt b/Documentation/trace-cmd-clear.1.txt
> index 67e5851a..a0ae36e9 100644
> --- a/Documentation/trace-cmd-clear.1.txt
> +++ b/Documentation/trace-cmd-clear.1.txt
> @@ -7,12 +7,23 @@ trace-cmd-clear - clear the Ftrace buffer.
>  
>  SYNOPSIS
>  --------
> -*trace-cmd clear*
> +*trace-cmd clear* ['OPTIONS']
>  
>  DESCRIPTION
>  -----------
>  The *trace-cmd(1) clear* clears the content of the Ftrace ring buffer.
>  
> +OPTIONS
> +-------
> +*-B* 'buffer-name'::
> +    If the kernel supports multiple buffers, this will clear only the given
> +    buffer. It does not affect any other buffers. This may be used multiple
> +    times to specify different buffers. The top level buffer will not be
> +    clearded if this option is given.

We should give a special name for the top buffer. I think "/" would be
good, as that can not be in the name of any instance.

That way if you have three buffers (top and two instances, say "foo"
and "bar") we could do something like:

  trace-cmd clear -B / -B foo

And it will clear the top buffer and "foo", but leave "bar" alone.


> +
> +*-a*::
> +    Clear all existing buffers, including the top level one.
> +
>  SEE ALSO
>  --------
>  trace-cmd(1), trace-cmd-record(1), trace-cmd-report(1), trace-cmd-start(1),
> diff --git a/tracecmd/Makefile b/tracecmd/Makefile
> index 5e59adf8..01f36c61 100644
> --- a/tracecmd/Makefile
> +++ b/tracecmd/Makefile
> @@ -31,6 +31,7 @@ TRACE_CMD_OBJS += trace-show.o
>  TRACE_CMD_OBJS += trace-list.o
>  TRACE_CMD_OBJS += trace-usage.o
>  TRACE_CMD_OBJS += trace-dump.o
> +TRACE_CMD_OBJS += trace-clear.o
>  ifeq ($(VSOCK_DEFINED), 1)
>  TRACE_CMD_OBJS += trace-tsync.o
>  endif
> diff --git a/tracecmd/trace-clear.c b/tracecmd/trace-clear.c
> new file mode 100644
> index 00000000..f1b4f309
> --- /dev/null
> +++ b/tracecmd/trace-clear.c

Thinking about this more, the functionality of this entire file should
be in libtracefs.  This is specific to the tracefs functionality and
not to trace-cmd in general.

We should have:

 tracefs_clear(instance_name);


Where: tracefs_clear(NULL) clears the top level.
       tracefs_clear("foo") clears the instance named "foo".

And a tracefs_clear_all(bool top);

Where: tracefs_clear_all(false)
   clear's all instances but leaves the top instance untouched.

       tracefs_clear_all(true)
    clears the top level and all instances.

-- Steve


> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +/*
> + * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> + *
> + * Updates:
> + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> + *
> + */
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +
> +#include "tracefs.h"
> +#include "trace-local.h"
> +
> +struct instances_list {
> +	struct instances_list *next;
> +	struct tracefs_instance *instance;
> +};
> +
> +static int add_new_instance(struct instances_list **list, char *name)
> +{
> +	struct instances_list *new;
> +
> +	new = calloc(1, sizeof(*new));
> +	if (!new)
> +		return -1;
> +	new->instance = tracefs_instance_alloc(name);
> +	if (!new->instance) {
> +		free(new);
> +		return -1;
> +	}
> +
> +	new->next = *list;
> +	*list = new;
> +	return 0;
> +}
> +
> +static int add_instance_walk(const char *name, void *data)
> +{
> +	return add_new_instance((struct instances_list **)data, (char *)name);
> +}
> +
> +static void clear_list(struct instances_list *list)
> +{
> +	struct instances_list *del;
> +
> +	while (list) {
> +		del = list;
> +		list = list->next;
> +		tracefs_instance_free(del->instance);
> +		free(del);
> +	}
> +}
> +
> +static void clear_instance_trace(struct tracefs_instance *instance)
> +{
> +	FILE *fp;
> +	char *path;
> +
> +	/* reset the trace */
> +	path = tracefs_instance_get_file(instance, "trace");
> +	fp = fopen(path, "w");
> +	if (!fp)
> +		die("writing to '%s'", path);
> +	tracefs_put_tracing_file(path);
> +	fwrite("0", 1, 1, fp);
> +	fclose(fp);
> +}
> +
> +static void clear_trace(struct instances_list *instances)
> +{
> +	if (instances) {
> +		while (instances) {
> +			clear_instance_trace(instances->instance);
> +			instances = instances->next;
> +		}
> +	} else
> +		clear_instance_trace(NULL);
> +}
> +
> +void trace_clear(int argc, char **argv)
> +{
> +	struct instances_list *instances = NULL;
> +	bool all = false;
> +	int c;
> +
> +	for (;;) {
> +		int option_index = 0;
> +		static struct option long_options[] = {
> +			{"all", no_argument, NULL, 'a'},
> +			{"help", no_argument, NULL, '?'},
> +			{NULL, 0, NULL, 0}
> +		};
> +
> +		c = getopt_long (argc-1, argv+1, "+haB:",
> +				 long_options, &option_index);
> +		if (c == -1)
> +			break;
> +		switch (c) {
> +		case 'B':
> +			if (add_new_instance(&instances, optarg))
> +				die("Failed to allocate instance %s", optarg);
> +			break;
> +		case 'a':
> +			all = true;
> +			if (tracefs_instances_walk(add_instance_walk, &instances))
> +				die("Failed to add all instances");
> +			break;
> +		case 'h':
> +		case '?':
> +		default:
> +			usage(argv);
> +			break;
> +		}
> +	}
> +
> +	clear_trace(instances);
> +	if (all)
> +		clear_trace(NULL);
> +	clear_list(instances);
> +	exit(0);
> +}
> +
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index bd004574..5009eaa1 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -835,21 +835,6 @@ static void clear_trace_instances(void)
>  		__clear_trace(instance);
>  }
>  
> -static void clear_trace(void)
> -{
> -	FILE *fp;
> -	char *path;
> -
> -	/* reset the trace */
> -	path = tracefs_get_tracing_file("trace");
> -	fp = fopen(path, "w");
> -	if (!fp)
> -		die("writing to '%s'", path);
> -	tracefs_put_tracing_file(path);
> -	fwrite("0", 1, 1, fp);
> -	fclose(fp);
> -}
> -
>  static void reset_max_latency(struct buffer_instance *instance)
>  {
>  	tracefs_instance_file_write(instance->tracefs,
> @@ -6704,15 +6689,6 @@ void trace_profile(int argc, char **argv)
>  	exit(0);
>  }
>  
> -void trace_clear(int argc, char **argv)
> -{
> -	if (argc > 2)
> -		usage(argv);
> -	else
> -		clear_trace();
> -	exit(0);
> -}
> -
>  void trace_record(int argc, char **argv)
>  {
>  	struct common_record_context ctx;
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index 3f0b2d07..79610bd3 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -180,7 +180,9 @@ static struct usage_help usage_help[] = {
>  	{
>  		"clear",
>  		"clear the trace buffers",
> -		" %s clear\n"
> +		" %s clear [-B buf][-a]\n"
> +		"          -B clear the given buffer (may specify multiple -B)\n"
> +		"          -a clear all existing buffers, including the top level one\n"
>  	},
>  	{
>  		"report",

Re: [PATCH 2/2] trace-cmd: Enhance "trace-cmd clear" to be instance aware

[Steven Rostedt]

On Fri, 17 Jul 2020 11:03:26 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> --- /dev/null
> +++ b/tracecmd/trace-clear.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +/*
> + * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> + *
> + * Updates:
> + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> + *
> + */
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +
> +#include "tracefs.h"
> +#include "trace-local.h"
> +
> +struct instances_list {
> +	struct instances_list *next;
> +	struct tracefs_instance *instance;
> +};
> +
> +static int add_new_instance(struct instances_list **list, char *name)
> +{
> +	struct instances_list *new;
> +
> +	new = calloc(1, sizeof(*new));
> +	if (!new)
> +		return -1;
> +	new->instance = tracefs_instance_alloc(name);
> +	if (!new->instance) {
> +		free(new);
> +		return -1;
> +	}
> +
> +	new->next = *list;
> +	*list = new;
> +	return 0;
> +}
> +
> +static int add_instance_walk(const char *name, void *data)
> +{
> +	return add_new_instance((struct instances_list **)data, (char *)name);
> +}
> +
> +static void clear_list(struct instances_list *list)
> +{
> +	struct instances_list *del;
> +
> +	while (list) {
> +		del = list;
> +		list = list->next;
> +		tracefs_instance_free(del->instance);
> +		free(del);
> +	}
> +}

BTW, why create this list? (see below).

> +
> +static void clear_instance_trace(struct tracefs_instance *instance)
> +{
> +	FILE *fp;
> +	char *path;
> +
> +	/* reset the trace */
> +	path = tracefs_instance_get_file(instance, "trace");
> +	fp = fopen(path, "w");
> +	if (!fp)
> +		die("writing to '%s'", path);
> +	tracefs_put_tracing_file(path);
> +	fwrite("0", 1, 1, fp);
> +	fclose(fp);
> +}
> +
> +static void clear_trace(struct instances_list *instances)
> +{
> +	if (instances) {
> +		while (instances) {
> +			clear_instance_trace(instances->instance);
> +			instances = instances->next;
> +		}
> +	} else
> +		clear_instance_trace(NULL);
> +}
> +
> +void trace_clear(int argc, char **argv)
> +{
> +	struct instances_list *instances = NULL;
> +	bool all = false;
> +	int c;
> +
> +	for (;;) {
> +		int option_index = 0;
> +		static struct option long_options[] = {
> +			{"all", no_argument, NULL, 'a'},
> +			{"help", no_argument, NULL, '?'},
> +			{NULL, 0, NULL, 0}
> +		};
> +
> +		c = getopt_long (argc-1, argv+1, "+haB:",
> +				 long_options, &option_index);
> +		if (c == -1)
> +			break;
> +		switch (c) {
> +		case 'B':
> +			if (add_new_instance(&instances, optarg))
> +				die("Failed to allocate instance %s", optarg);
> +			break;
> +		case 'a':
> +			all = true;

All you need to do is set the flag, and don't create the list here.

> +			if (tracefs_instances_walk(add_instance_walk, &instances))
> +				die("Failed to add all instances");
> +			break;
> +		case 'h':
> +		case '?':
> +		default:
> +			usage(argv);
> +			break;
> +		}
> +	}
> +
> +	clear_trace(instances);
> +	if (all)

Then here, you would just do:

		tracefs_instances_iterate(clear_instance_iter, NULL);

Where we have:

static int clear_instance_iter(const char *name, void *data)
{
	tracefs_clear(name);
}

No need for the creating of the list.

-- Steve


> +		clear_trace(NULL);
> +	clear_list(instances);
> +	exit(0);
> +}
> +
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index bd004574..5009eaa1 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -835,21 +835,6 @@ static void clear_trace_instances(void)
>  		__clear_trace(instance);
>  }
>  
> -static void clear_trace(void)
> -{
> -	FILE *fp;
> -	char *path;
> -
> -	/* reset the trace */
> -	path = tracefs_get_tracing_file("trace");
> -	fp = fopen(path, "w");
> -	if (!fp)
> -		die("writing to '%s'", path);
> -	tracefs_put_tracing_file(path);
> -	fwrite("0", 1, 1, fp);
> -	fclose(fp);
> -}
> -
>  static void reset_max_latency(struct buffer_instance *instance)
>  {
>  	tracefs_instance_file_write(instance->tracefs,
> @@ -6704,15 +6689,6 @@ void trace_profile(int argc, char **argv)
>  	exit(0);
>  }
>  
> -void trace_clear(int argc, char **argv)
> -{
> -	if (argc > 2)
> -		usage(argv);
> -	else
> -		clear_trace();
> -	exit(0);
> -}
> -
>  void trace_record(int argc, char **argv)
>  {
>  	struct common_record_context ctx;
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index 3f0b2d07..79610bd3 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -180,7 +180,9 @@ static struct usage_help usage_help[] = {
>  	{
>  		"clear",
>  		"clear the trace buffers",
> -		" %s clear\n"
> +		" %s clear [-B buf][-a]\n"
> +		"          -B clear the given buffer (may specify multiple -B)\n"
> +		"          -a clear all existing buffers, including the top level one\n"
>  	},
>  	{
>  		"report",

Re: [PATCH 2/2] trace-cmd: Enhance "trace-cmd clear" to be instance aware

[Tzvetomir Stoyanov]

On Fri, Jul 17, 2020 at 5:06 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 17 Jul 2020 11:03:26 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> > --- /dev/null
> > +++ b/tracecmd/trace-clear.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > +/*
> > + * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> > + *
> > + * Updates:
> > + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> > + *
> > + */
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <getopt.h>
> > +
> > +#include "tracefs.h"
> > +#include "trace-local.h"
> > +
> > +struct instances_list {
> > +     struct instances_list *next;
> > +     struct tracefs_instance *instance;
> > +};
> > +
> > +static int add_new_instance(struct instances_list **list, char *name)
> > +{
> > +     struct instances_list *new;
> > +
> > +     new = calloc(1, sizeof(*new));
> > +     if (!new)
> > +             return -1;
> > +     new->instance = tracefs_instance_alloc(name);
> > +     if (!new->instance) {
> > +             free(new);
> > +             return -1;
> > +     }
> > +
> > +     new->next = *list;
> > +     *list = new;
> > +     return 0;
> > +}
> > +
> > +static int add_instance_walk(const char *name, void *data)
> > +{
> > +     return add_new_instance((struct instances_list **)data, (char *)name);
> > +}
> > +
> > +static void clear_list(struct instances_list *list)
> > +{
> > +     struct instances_list *del;
> > +
> > +     while (list) {
> > +             del = list;
> > +             list = list->next;
> > +             tracefs_instance_free(del->instance);
> > +             free(del);
> > +     }
> > +}
>
> BTW, why create this list? (see below).
>
> > +
> > +static void clear_instance_trace(struct tracefs_instance *instance)
> > +{
> > +     FILE *fp;
> > +     char *path;
> > +
> > +     /* reset the trace */
> > +     path = tracefs_instance_get_file(instance, "trace");
> > +     fp = fopen(path, "w");
> > +     if (!fp)
> > +             die("writing to '%s'", path);
> > +     tracefs_put_tracing_file(path);
> > +     fwrite("0", 1, 1, fp);
> > +     fclose(fp);
> > +}
> > +
> > +static void clear_trace(struct instances_list *instances)
> > +{
> > +     if (instances) {
> > +             while (instances) {
> > +                     clear_instance_trace(instances->instance);
> > +                     instances = instances->next;
> > +             }
> > +     } else
> > +             clear_instance_trace(NULL);
> > +}
> > +
> > +void trace_clear(int argc, char **argv)
> > +{
> > +     struct instances_list *instances = NULL;
> > +     bool all = false;
> > +     int c;
> > +
> > +     for (;;) {
> > +             int option_index = 0;
> > +             static struct option long_options[] = {
> > +                     {"all", no_argument, NULL, 'a'},
> > +                     {"help", no_argument, NULL, '?'},
> > +                     {NULL, 0, NULL, 0}
> > +             };
> > +
> > +             c = getopt_long (argc-1, argv+1, "+haB:",
> > +                              long_options, &option_index);
> > +             if (c == -1)
> > +                     break;
> > +             switch (c) {
> > +             case 'B':
> > +                     if (add_new_instance(&instances, optarg))
> > +                             die("Failed to allocate instance %s", optarg);
> > +                     break;
> > +             case 'a':
> > +                     all = true;
>
> All you need to do is set the flag, and don't create the list here.
>
> > +                     if (tracefs_instances_walk(add_instance_walk, &instances))
> > +                             die("Failed to add all instances");
> > +                     break;
> > +             case 'h':
> > +             case '?':
> > +             default:
> > +                     usage(argv);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     clear_trace(instances);
> > +     if (all)
>
> Then here, you would just do:
>
>                 tracefs_instances_iterate(clear_instance_iter, NULL);
>
> Where we have:
>
> static int clear_instance_iter(const char *name, void *data)
> {
>         tracefs_clear(name);
> }
>
> No need for the creating of the list.

There are  two reasons for creating the list:
 - For consistency with the other flow, the"-B" option.
 - The tracefs APIs require "struct tracefs_instance" as input
parameter to identify the instance, do not work with instance name
The list is not needed for "--all", but still needs to create "struct
tracefs_instance" and to pass it to tracefs_instance_get_file() API


>
> -- Steve
>
>
> > +             clear_trace(NULL);
> > +     clear_list(instances);
> > +     exit(0);
> > +}
> > +
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index bd004574..5009eaa1 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -835,21 +835,6 @@ static void clear_trace_instances(void)
> >               __clear_trace(instance);
> >  }
> >
> > -static void clear_trace(void)
> > -{
> > -     FILE *fp;
> > -     char *path;
> > -
> > -     /* reset the trace */
> > -     path = tracefs_get_tracing_file("trace");
> > -     fp = fopen(path, "w");
> > -     if (!fp)
> > -             die("writing to '%s'", path);
> > -     tracefs_put_tracing_file(path);
> > -     fwrite("0", 1, 1, fp);
> > -     fclose(fp);
> > -}
> > -
> >  static void reset_max_latency(struct buffer_instance *instance)
> >  {
> >       tracefs_instance_file_write(instance->tracefs,
> > @@ -6704,15 +6689,6 @@ void trace_profile(int argc, char **argv)
> >       exit(0);
> >  }
> >
> > -void trace_clear(int argc, char **argv)
> > -{
> > -     if (argc > 2)
> > -             usage(argv);
> > -     else
> > -             clear_trace();
> > -     exit(0);
> > -}
> > -
> >  void trace_record(int argc, char **argv)
> >  {
> >       struct common_record_context ctx;
> > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> > index 3f0b2d07..79610bd3 100644
> > --- a/tracecmd/trace-usage.c
> > +++ b/tracecmd/trace-usage.c
> > @@ -180,7 +180,9 @@ static struct usage_help usage_help[] = {
> >       {
> >               "clear",
> >               "clear the trace buffers",
> > -             " %s clear\n"
> > +             " %s clear [-B buf][-a]\n"
> > +             "          -B clear the given buffer (may specify multiple -B)\n"
> > +             "          -a clear all existing buffers, including the top level one\n"
> >       },
> >       {
> >               "report",
>


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