[PATCH v3 0/6] libtracefs fixes and improvements

[PATCH v3 0/6] libtracefs fixes and improvements

[Tzvetomir Stoyanov (VMware)]

A few final tracefs library fixes and improvements, before releasing it as
an official library.

v3 chabges:
 - Addressed Steven's comments.
v2 changes:
 - Change tracefs_instance_create() to return pointer to allocated trace
   instance structure
 - Add new API: tracefs_instance_is_new() to check if the trace instance is
   newly created by the library.

Tzvetomir Stoyanov (VMware) (6):
  trace-cmd: Change tracefs.h include path
  libtracefs: Change get name API to return constant string
  libtracefs: Add new API to check if instance exists
  libtracefs: Combine allocate and create APIs into one
  libtracefs: Add new tracefs API tracefs_instances_walk()
  trace-cmd: Add new libtrasefs API to get the current trace clock

 include/trace-cmd/trace-cmd.h       |   1 +
 include/tracefs/tracefs.h           |  10 +-
 lib/trace-cmd/trace-timesync.c      |   3 +-
 lib/tracefs/include/tracefs-local.h |   1 +
 lib/tracefs/tracefs-events.c        |  26 ++---
 lib/tracefs/tracefs-instance.c      | 173 +++++++++++++++++++++++++---
 tracecmd/include/trace-local.h      |   4 +-
 tracecmd/trace-record.c             |  72 ++++++------
 tracecmd/trace-show.c               |   2 +-
 tracecmd/trace-stat.c               |  83 ++++---------
 utest/tracefs-utest.c               | 107 ++++++++++++++---
 11 files changed, 327 insertions(+), 155 deletions(-)

-- 
2.28.0

[PATCH v3 1/6] trace-cmd: Change tracefs.h include path

[Tzvetomir Stoyanov (VMware)]

The tracefs.h file is installed in
	<install_prefix>/include/tracefs/tracefs.h.
However, in trace-cmd.h it is inluded just as "tracefs.h".
When trace-cmd.h is used in trace-cmd build context that include is not
a problem, as all local header paths are described in the Makefiles. But
when other application uses trace-cmd.h and tracefs.h, installed in the
system, the compilation fails due to this broken include.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/trace-cmd/trace-cmd.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index f3c95f30..3c2b4745 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -7,6 +7,7 @@
 #define _TRACE_CMD_H
 
 #include "traceevent/event-parse.h"
+#include "tracefs/tracefs.h"
 
 #define TRACECMD_MAGIC { 23, 8, 68 }
 
-- 
2.28.0

[PATCH v3 2/6] libtracefs: Change get name API to return constant string

[Tzvetomir Stoyanov (VMware)]

The tracefs_instance_get_name() API returns a pointer to internal
string. This string is not meant to be changed by the API callers,
that's why it should be constant.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h      |  2 +-
 lib/tracefs/tracefs-events.c   | 12 ++----------
 lib/tracefs/tracefs-instance.c |  2 +-
 tracecmd/trace-record.c        |  2 +-
 utest/tracefs-utest.c          |  2 +-
 5 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index 8ee7ba6e..1cf8de48 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -24,7 +24,7 @@ struct tracefs_instance *tracefs_instance_alloc(const char *name);
 void tracefs_instance_free(struct tracefs_instance *instance);
 int tracefs_instance_create(struct tracefs_instance *instance);
 int tracefs_instance_destroy(struct tracefs_instance *instance);
-char *tracefs_instance_get_name(struct tracefs_instance *instance);
+const char *tracefs_instance_get_name(struct tracefs_instance *instance);
 char *
 tracefs_instance_get_file(struct tracefs_instance *instance, const char *file);
 char *tracefs_instance_get_dir(struct tracefs_instance *instance);
diff --git a/lib/tracefs/tracefs-events.c b/lib/tracefs/tracefs-events.c
index 8e825f50..6b796382 100644
--- a/lib/tracefs/tracefs-events.c
+++ b/lib/tracefs/tracefs-events.c
@@ -481,11 +481,7 @@ next_event:
 			failure = ret;
 	}
 
-	if (events) {
-		for (i = 0; events[i]; i++)
-			free(events[i]);
-		free(events);
-	}
+	tracefs_list_free(events);
 	return failure;
 }
 
@@ -564,11 +560,7 @@ static int fill_local_events_system(const char *tracing_dir,
 	/* always succeed because parsing failures are not critical */
 	ret = 0;
 out:
-	if (systems) {
-		for (i = 0; systems[i]; i++)
-			free(systems[i]);
-		free(systems);
-	}
+	tracefs_list_free(systems);
 	return ret;
 }
 
diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index 50e88534..e37d93d1 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -169,7 +169,7 @@ char *tracefs_instance_get_dir(struct tracefs_instance *instance)
  * Returns the name of the given @instance.
  * The returned string must *not* be freed.
  */
-char *tracefs_instance_get_name(struct tracefs_instance *instance)
+const char *tracefs_instance_get_name(struct tracefs_instance *instance)
 {
 	if (instance)
 		return instance->name;
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 72a5c8c9..8cd44dd0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3861,7 +3861,7 @@ static void connect_to_agent(struct buffer_instance *instance)
 	unsigned int *ports;
 	int i, *fds = NULL;
 	bool use_fifos = false;
-	char *name;
+	const char *name;
 
 	name = tracefs_instance_get_name(instance->tracefs);
 	if (!no_fifos) {
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index 1c146576..b5296963 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -180,7 +180,7 @@ static void test_instance_file(void)
 {
 	struct tracefs_instance *instance = NULL;
 	const char *name = get_rand_str();
-	char *inst_name = NULL;
+	const char *inst_name = NULL;
 	const char *tdir;
 	char *inst_file;
 	char *inst_dir;
-- 
2.28.0

Re: [PATCH v3 2/6] libtracefs: Change get name API to return constant string

[Steven Rostedt]

On Thu, 12 Nov 2020 11:11:05 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The tracefs_instance_get_name() API returns a pointer to internal
> string. This string is not meant to be changed by the API callers,
> that's why it should be constant.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/tracefs/tracefs.h      |  2 +-
>  lib/tracefs/tracefs-events.c   | 12 ++----------
>  lib/tracefs/tracefs-instance.c |  2 +-
>  tracecmd/trace-record.c        |  2 +-
>  utest/tracefs-utest.c          |  2 +-
>  5 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
> index 8ee7ba6e..1cf8de48 100644
> --- a/include/tracefs/tracefs.h
> +++ b/include/tracefs/tracefs.h
> @@ -24,7 +24,7 @@ struct tracefs_instance *tracefs_instance_alloc(const char *name);
>  void tracefs_instance_free(struct tracefs_instance *instance);
>  int tracefs_instance_create(struct tracefs_instance *instance);
>  int tracefs_instance_destroy(struct tracefs_instance *instance);
> -char *tracefs_instance_get_name(struct tracefs_instance *instance);
> +const char *tracefs_instance_get_name(struct tracefs_instance *instance);
>  char *
>  tracefs_instance_get_file(struct tracefs_instance *instance, const char *file);
>  char *tracefs_instance_get_dir(struct tracefs_instance *instance);
> diff --git a/lib/tracefs/tracefs-events.c b/lib/tracefs/tracefs-events.c
> index 8e825f50..6b796382 100644
> --- a/lib/tracefs/tracefs-events.c
> +++ b/lib/tracefs/tracefs-events.c
> @@ -481,11 +481,7 @@ next_event:
>  			failure = ret;
>  	}
>  
> -	if (events) {
> -		for (i = 0; events[i]; i++)
> -			free(events[i]);
> -		free(events);
> -	}
> +	tracefs_list_free(events);
>  	return failure;
>  }
>  
> @@ -564,11 +560,7 @@ static int fill_local_events_system(const char *tracing_dir,
>  	/* always succeed because parsing failures are not critical */
>  	ret = 0;
>  out:
> -	if (systems) {
> -		for (i = 0; systems[i]; i++)
> -			free(systems[i]);
> -		free(systems);
> -	}
> +	tracefs_list_free(systems);
>  	return ret;
>  }
>  

The above looks like it belongs in its own patch (not related to this
change).

-- Steve

[PATCH v3 3/6] libtracefs: Add new API to check if instance exists

[Tzvetomir Stoyanov (VMware)]

A new API is implemented:
 tracefs_instance_exists()
which can be used to check if ftrace instance with given name exists in
the system.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h      |  1 +
 lib/tracefs/tracefs-instance.c | 22 +++++++++++++++++++++-
 utest/tracefs-utest.c          |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index 1cf8de48..ef806d3c 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -33,6 +33,7 @@ int tracefs_instance_file_write(struct tracefs_instance *instance,
 char *tracefs_instance_file_read(struct tracefs_instance *instance,
 				 char *file, int *psize);
 
+bool tracefs_instance_exists(const char *name);
 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/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index e37d93d1..06f33c35 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -257,7 +257,10 @@ static bool check_file_exists(struct tracefs_instance *instance,
 	int ret;
 
 	path = tracefs_instance_get_dir(instance);
-	snprintf(file, PATH_MAX, "%s/%s", path, name);
+	if (name)
+		snprintf(file, PATH_MAX, "%s/%s", path, name);
+	else
+		snprintf(file, PATH_MAX, "%s", path);
 	tracefs_put_tracing_file(path);
 	ret = stat(file, &st);
 	if (ret < 0)
@@ -266,6 +269,23 @@ static bool check_file_exists(struct tracefs_instance *instance,
 	return !dir == !S_ISDIR(st.st_mode);
 }
 
+/**
+ * tracefs_instance_exists - Check an instance with given name exists
+ * @name: name of the instance
+ *
+ * Returns true if the instance exists, false otherwise
+ *
+ */
+bool tracefs_instance_exists(const char *name)
+{
+	char file[PATH_MAX];
+
+	if (!name)
+		return false;
+	snprintf(file, PATH_MAX, "instances/%s", name);
+	return check_file_exists(NULL, file, true);
+}
+
 /**
  * tracefs_file_exists - Check if a file with given name exists in given instance
  * @instance: ftrace instance, can be NULL for the top instance
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index b5296963..2febdb88 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -197,6 +197,7 @@ static void test_instance_file(void)
 	CU_TEST(asprintf(&inst_dir, "%s/instances/%s", tdir, name) > 0);
 	CU_TEST(stat(inst_dir, &st) != 0);
 
+	CU_TEST(tracefs_instance_exists(name) == false);
 	instance = tracefs_instance_alloc(name);
 	CU_TEST(instance != NULL);
 	CU_TEST(stat(inst_dir, &st) != 0);
@@ -205,6 +206,7 @@ static void test_instance_file(void)
 	CU_TEST(strcmp(inst_name, name) == 0);
 
 	CU_TEST(tracefs_instance_create(instance) == 0);
+	CU_TEST(tracefs_instance_exists(name) == true);
 	CU_TEST(stat(inst_dir, &st) == 0);
 	CU_TEST(S_ISDIR(st.st_mode));
 
-- 
2.28.0

[PATCH v3 4/6] libtracefs: Combine allocate and create APIs into one

[Tzvetomir Stoyanov (VMware)]

Having two APIs for allocating and creating trace instance can be
confusing. The tracefs_instance_alloc() is removed and its logic is
moved to tracefs_instance_create() API. This single API first creates
the instance in the system (if it does not exist) and then allocates and
initializes tracefs_instance structure. Trace-cmd code and unit tests are
modified to use the new API.
A new API is introduced, to check if the tracefs instance is newly
created by the library or if it already exist.
  tracefs_instance_is_new()

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h      |  4 +-
 lib/trace-cmd/trace-timesync.c |  3 +-
 lib/tracefs/tracefs-instance.c | 59 +++++++++++++++++++++-------
 tracecmd/include/trace-local.h |  4 +-
 tracecmd/trace-record.c        | 70 ++++++++++++++++------------------
 tracecmd/trace-show.c          |  2 +-
 tracecmd/trace-stat.c          |  2 +-
 utest/tracefs-utest.c          | 25 ++++++------
 8 files changed, 96 insertions(+), 73 deletions(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index ef806d3c..388d8f94 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -20,10 +20,10 @@ char *tracefs_find_tracing_dir(void);
 /* ftarce instances */
 struct tracefs_instance;
 
-struct tracefs_instance *tracefs_instance_alloc(const char *name);
 void tracefs_instance_free(struct tracefs_instance *instance);
-int tracefs_instance_create(struct tracefs_instance *instance);
+struct tracefs_instance *tracefs_instance_create(const char *name);
 int tracefs_instance_destroy(struct tracefs_instance *instance);
+bool tracefs_instance_is_new(struct tracefs_instance *instance);
 const char *tracefs_instance_get_name(struct tracefs_instance *instance);
 char *
 tracefs_instance_get_file(struct tracefs_instance *instance, const char *file);
diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 35a41394..7a6a7eb6 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -240,11 +240,10 @@ clock_synch_create_instance(const char *clock, unsigned int cid)
 
 	snprintf(inst_name, 256, "clock_synch-%d", cid);
 
-	instance = tracefs_instance_alloc(inst_name);
+	instance = tracefs_instance_create(inst_name);
 	if (!instance)
 		return NULL;
 
-	tracefs_instance_create(instance);
 	tracefs_instance_file_write(instance, "trace", "\0");
 	if (clock)
 		tracefs_instance_file_write(instance, "trace_clock", clock);
diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index 06f33c35..1c0958cf 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -17,17 +17,19 @@
 #include "tracefs.h"
 #include "tracefs-local.h"
 
+#define FLAG_INSTANCE_NEWLY_CREATED	(1 << 0)
 struct tracefs_instance {
-	char *name;
+	char	*name;
+	int	flags;
 };
 
 /**
- * tracefs_instance_alloc - allocate a new ftrace instance
+ * instance_alloc - allocate a new ftrace instance
  * @name: The name of the instance (instance will point to this)
  *
  * Returns a newly allocated instance, or NULL in case of an error.
  */
-struct tracefs_instance *tracefs_instance_alloc(const char *name)
+static struct tracefs_instance *instance_alloc(const char *name)
 {
 	struct tracefs_instance *instance;
 
@@ -45,7 +47,7 @@ struct tracefs_instance *tracefs_instance_alloc(const char *name)
 
 /**
  * tracefs_instance_free - Free an instance, previously allocated by
-			   tracefs_instance_alloc()
+			   tracefs_instance_create()
  * @instance: Pointer to the instance to be freed
  *
  */
@@ -57,27 +59,56 @@ void tracefs_instance_free(struct tracefs_instance *instance)
 	free(instance);
 }
 
+/**
+ * tracefs_instance_is_new - Check if the instance is newly created by the library
+ * @instance: Pointer to an ftrace instance
+ *
+ * Returns true, if the ftrace instance is newly created by the library or
+ * false otherwise.
+ */
+bool tracefs_instance_is_new(struct tracefs_instance *instance)
+{
+	if (instance && (instance->flags & FLAG_INSTANCE_NEWLY_CREATED))
+		return true;
+	return false;
+}
+
 /**
  * tracefs_instance_create - Create a new ftrace instance
- * @instance: Pointer to the instance to be created
+ * @name: Name of the instance to be created
  *
- * Returns 1 if the instance already exist, 0 if the instance
- * is created successful or -1 in case of an error
+ * Allocates and initializes a new instance structure. If the instance does not
+ * exist in the system, create it.
+ * Returns a pointer to a newly allocated instance, or NULL in case of an error.
+ * The returned instance must be freed by tracefs_instance_free().
  */
-int tracefs_instance_create(struct tracefs_instance *instance)
+struct tracefs_instance *tracefs_instance_create(const char *name)
 {
+	struct tracefs_instance *inst = NULL;
 	struct stat st;
 	char *path;
 	int ret;
 
-	path = tracefs_instance_get_dir(instance);
+	inst = instance_alloc(name);
+	if (!inst)
+		return NULL;
+
+	path = tracefs_instance_get_dir(inst);
 	ret = stat(path, &st);
-	if (ret < 0)
-		ret = mkdir(path, 0777);
-	else
-		ret = 1;
+	if (ret < 0) {
+		/* Cannot create the top instance, if it does not exist! */
+		if (!name)
+			goto error;
+		if (mkdir(path, 0777))
+			goto error;
+		inst->flags |= FLAG_INSTANCE_NEWLY_CREATED;
+	}
 	tracefs_put_tracing_file(path);
-	return ret;
+	return inst;
+
+error:
+	tracefs_instance_free(inst);
+	return NULL;
 }
 
 /**
diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
index d148aa16..207aa68f 100644
--- a/tracecmd/include/trace-local.h
+++ b/tracecmd/include/trace-local.h
@@ -199,6 +199,7 @@ struct filter_pids {
 
 struct buffer_instance {
 	struct buffer_instance	*next;
+	char			*name;
 	struct tracefs_instance	*tracefs;
 	unsigned long long	trace_id;
 	char			*cpumask;
@@ -277,7 +278,7 @@ extern struct buffer_instance *first_instance;
 #define is_agent(instance)	((instance)->flags & BUFFER_FL_AGENT)
 #define is_guest(instance)	((instance)->flags & BUFFER_FL_GUEST)
 
-struct buffer_instance *create_instance(const char *name);
+struct buffer_instance *allocate_instance(const char *name);
 void add_instance(struct buffer_instance *instance, int cpu_count);
 void update_first_instance(struct buffer_instance *instance, int topt);
 
@@ -286,7 +287,6 @@ void show_instance_file(struct buffer_instance *instance, const char *name);
 int get_guest_vcpu_pid(unsigned int guest_cid, unsigned int guest_vcpu);
 
 /* moved from trace-cmd.h */
-void tracecmd_create_top_instance(char *name);
 void tracecmd_remove_instances(void);
 int tracecmd_add_event(const char *event_str, int stack);
 void tracecmd_enable_events(void);
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 8cd44dd0..908adb93 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -351,27 +351,37 @@ static void test_set_event_pid(struct buffer_instance *instance)
 }
 
 /**
- * create_instance - allocate a new buffer instance
+ * allocate_instance - allocate a new buffer instance,
+ *			it must exist in the ftrace system
  * @name: The name of the instance (instance will point to this)
  *
- * Returns a newly allocated instance. Note that @name will not be
- * copied, and the instance buffer will point to the string itself.
+ * Returns a newly allocated instance. In case of an error or if the
+ * instance does not exist in the ftrace system, NULL is returned.
  */
-struct buffer_instance *create_instance(const char *name)
+struct buffer_instance *allocate_instance(const char *name)
 {
 	struct buffer_instance *instance;
 
-	instance = malloc(sizeof(*instance));
+	instance = calloc(1, sizeof(*instance));
 	if (!instance)
 		return NULL;
-	memset(instance, 0, sizeof(*instance));
-	instance->tracefs = tracefs_instance_alloc(name);
-	if (!instance->tracefs) {
-		free(instance);
-		return NULL;
+	if (name)
+		instance->name = strdup(name);
+	if (tracefs_instance_exists(name)) {
+		instance->tracefs = tracefs_instance_create(name);
+		if (!instance->tracefs)
+			goto error;
 	}
 
 	return instance;
+
+error:
+	if (instance) {
+		free(instance->name);
+		tracefs_instance_free(instance->tracefs);
+		free(instance);
+	}
+	return NULL;
 }
 
 static int __add_all_instances(const char *tracing_dir)
@@ -418,7 +428,7 @@ static int __add_all_instances(const char *tracing_dir)
 		}
 		free(instance_path);
 
-		instance = create_instance(name);
+		instance = allocate_instance(name);
 		if (!instance)
 			die("Failed to create instance");
 		add_instance(instance, local_cpu_count);
@@ -5034,9 +5044,11 @@ static void make_instances(void)
 	for_each_instance(instance) {
 		if (is_guest(instance))
 			continue;
-		if (tracefs_instance_create(instance->tracefs) > 0) {
+		if (instance->name && !instance->tracefs) {
+			instance->tracefs = tracefs_instance_create(instance->name);
 			/* Don't delete instances that already exist */
-			instance->flags |= BUFFER_FL_KEEP;
+			if (instance->tracefs && !tracefs_instance_is_new(instance->tracefs))
+				instance->flags |= BUFFER_FL_KEEP;
 		}
 	}
 }
@@ -5057,25 +5069,6 @@ void tracecmd_remove_instances(void)
 	}
 }
 
-/**
- * tracecmd_create_top_instance - create a top named instance
- * @name: name of the instance to use.
- *
- * This is a library function for tools that want to do their tracing inside of
- * an instance.  All it does is create an instance and set it as a top instance,
- * you don't want to call this more than once, and you want to call
- * tracecmd_remove_instances to undo your work.
- */
-void tracecmd_create_top_instance(char *name)
-{
-	struct buffer_instance *instance;
-
-	instance = create_instance(name);
-	add_instance(instance, local_cpu_count);
-	update_first_instance(instance, 0);
-	make_instances();
-}
-
 static void check_plugin(const char *plugin)
 {
 	char *buf;
@@ -5533,7 +5526,7 @@ void update_first_instance(struct buffer_instance *instance, int topt)
 void init_top_instance(void)
 {
 	if (!top_instance.tracefs)
-		top_instance.tracefs = tracefs_instance_alloc(NULL);
+		top_instance.tracefs = tracefs_instance_create(NULL);
 	top_instance.cpu_count = tracecmd_count_cpus();
 	top_instance.flags = BUFFER_FL_KEEP;
 	top_instance.trace_id = tracecmd_generate_traceid();
@@ -5580,7 +5573,7 @@ void trace_stop(int argc, char **argv)
 			usage(argv);
 			break;
 		case 'B':
-			instance = create_instance(optarg);
+			instance = allocate_instance(optarg);
 			if (!instance)
 				die("Failed to create instance");
 			add_instance(instance, local_cpu_count);
@@ -5620,7 +5613,7 @@ void trace_restart(int argc, char **argv)
 			usage(argv);
 			break;
 		case 'B':
-			instance = create_instance(optarg);
+			instance = allocate_instance(optarg);
 			if (!instance)
 				die("Failed to create instance");
 			add_instance(instance, local_cpu_count);
@@ -5678,7 +5671,7 @@ void trace_reset(int argc, char **argv)
 		}
 		case 'B':
 			last_specified_all = 0;
-			instance = create_instance(optarg);
+			instance = allocate_instance(optarg);
 			if (!instance)
 				die("Failed to create instance");
 			add_instance(instance, local_cpu_count);
@@ -5821,6 +5814,7 @@ static inline void remove_instances(struct buffer_instance *instances)
 	while (instances) {
 		del = instances;
 		instances = instances->next;
+		free(del->name);
 		tracefs_instance_destroy(del->tracefs);
 		tracefs_instance_free(del->tracefs);
 		free(del);
@@ -5978,7 +5972,7 @@ static void parse_record_options(int argc,
 					die("Failed to allocate guest name");
 			}
 
-			ctx->instance = create_instance(name);
+			ctx->instance = allocate_instance(name);
 			ctx->instance->flags |= BUFFER_FL_GUEST;
 			ctx->instance->cid = cid;
 			ctx->instance->port = port;
@@ -6173,7 +6167,7 @@ static void parse_record_options(int argc,
 			ctx->instance->buffer_size = atoi(optarg);
 			break;
 		case 'B':
-			ctx->instance = create_instance(optarg);
+			ctx->instance = allocate_instance(optarg);
 			if (!ctx->instance)
 				die("Failed to create instance");
 			ctx->instance->delete = negative;
diff --git a/tracecmd/trace-show.c b/tracecmd/trace-show.c
index a6f21027..eb328527 100644
--- a/tracecmd/trace-show.c
+++ b/tracecmd/trace-show.c
@@ -64,7 +64,7 @@ void trace_show(int argc, char **argv)
 			if (buffer)
 				die("Can only show one buffer at a time");
 			buffer = optarg;
-			instance = create_instance(optarg);
+			instance = allocate_instance(optarg);
 			if (!instance)
 				die("Failed to create instance");
 			break;
diff --git a/tracecmd/trace-stat.c b/tracecmd/trace-stat.c
index 260d0c37..5f79ff8a 100644
--- a/tracecmd/trace-stat.c
+++ b/tracecmd/trace-stat.c
@@ -926,7 +926,7 @@ void trace_stat (int argc, char **argv)
 			usage(argv);
 			break;
 		case 'B':
-			instance = create_instance(optarg);
+			instance = allocate_instance(optarg);
 			if (!instance)
 				die("Failed to create instance");
 			add_instance(instance, tracecmd_count_cpus());
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index 2febdb88..31031661 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -179,6 +179,7 @@ static void test_instance_file_read(struct tracefs_instance *inst, char *fname)
 static void test_instance_file(void)
 {
 	struct tracefs_instance *instance = NULL;
+	struct tracefs_instance *second = NULL;
 	const char *name = get_rand_str();
 	const char *inst_name = NULL;
 	const char *tdir;
@@ -198,17 +199,19 @@ static void test_instance_file(void)
 	CU_TEST(stat(inst_dir, &st) != 0);
 
 	CU_TEST(tracefs_instance_exists(name) == false);
-	instance = tracefs_instance_alloc(name);
+	instance = tracefs_instance_create(name);
 	CU_TEST(instance != NULL);
-	CU_TEST(stat(inst_dir, &st) != 0);
-	inst_name = tracefs_instance_get_name(instance);
-	CU_TEST(inst_name != NULL);
-	CU_TEST(strcmp(inst_name, name) == 0);
-
-	CU_TEST(tracefs_instance_create(instance) == 0);
+	CU_TEST(tracefs_instance_is_new(instance));
+	second = tracefs_instance_create(name);
+	CU_TEST(second != NULL);
+	CU_TEST(!tracefs_instance_is_new(second));
+	tracefs_instance_free(second);
 	CU_TEST(tracefs_instance_exists(name) == true);
 	CU_TEST(stat(inst_dir, &st) == 0);
 	CU_TEST(S_ISDIR(st.st_mode));
+	inst_name = tracefs_instance_get_name(instance);
+	CU_TEST(inst_name != NULL);
+	CU_TEST(strcmp(inst_name, name) == 0);
 
 	fname = tracefs_instance_get_dir(NULL);
 	CU_TEST(fname != NULL);
@@ -474,12 +477,8 @@ static int test_suite_init(void)
 	test_tep = tracefs_local_events_system(NULL, systems);
 	if (test_tep == NULL)
 		return 1;
-
-	test_instance = tracefs_instance_alloc(TEST_INSTANCE_NAME);
-	if (test_instance == NULL)
-		return 1;
-
-	if (tracefs_instance_create(test_instance) < 0)
+	test_instance = tracefs_instance_create(TEST_INSTANCE_NAME);
+	if (!test_instance)
 		return 1;
 
 	return 0;
-- 
2.28.0

Re: [PATCH v3 4/6] libtracefs: Combine allocate and create APIs into one

[Steven Rostedt]

On Thu, 12 Nov 2020 11:11:07 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> +struct tracefs_instance *tracefs_instance_create(const char *name)
>  {
> +	struct tracefs_instance *inst = NULL;
>  	struct stat st;
>  	char *path;
>  	int ret;
>  
> -	path = tracefs_instance_get_dir(instance);
> +	inst = instance_alloc(name);
> +	if (!inst)
> +		return NULL;
> +
> +	path = tracefs_instance_get_dir(inst);
>  	ret = stat(path, &st);
> -	if (ret < 0)
> -		ret = mkdir(path, 0777);
> -	else
> -		ret = 1;
> +	if (ret < 0) {
> +		/* Cannot create the top instance, if it does not exist! */
> +		if (!name)
> +			goto error;
> +		if (mkdir(path, 0777))

Not something you have to address now (but perhaps add a new patch?).
This should not be 0777, but instead be a macro, and define it as 0770, or
possibly even look at the permissions of the top level and use whatever
that is set as?

Again, this doesn't affect this series, but something we should change in
the near future.

-- Steve


> +			goto error;
> +		inst->flags |= FLAG_INSTANCE_NEWLY_CREATED;
> +	}
>  	tracefs_put_tracing_file(path);
> -	return ret;
> +	return inst;
> +
> +error:
> +	tracefs_instance_free(inst);
> +	return NULL;
>  }
>  

[PATCH v3 5/6] libtracefs: Add new tracefs API tracefs_instances_walk()

[Tzvetomir Stoyanov (VMware)]

The logic for finding all configured ftrace instances is encapuslated in
a 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      | 55 +++++++++++++++++++++++++++++
 tracecmd/trace-stat.c               | 52 +++++++--------------------
 utest/tracefs-utest.c               | 52 +++++++++++++++++++++++++++
 6 files changed, 129 insertions(+), 46 deletions(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index 388d8f94..bcf3dd64 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_instance_exists(const char *name);
 bool tracefs_file_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 6b796382..f2c6046c 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;
 
@@ -493,7 +493,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 1c0958cf..7b6d9417 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"
@@ -342,3 +343,57 @@ 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, 1 if the iteration was stopped because of the
+ * callback return value or 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)) {
+			fret = 1;
+			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 5f79ff8a..e6678eab 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)
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index 31031661..c42fea12 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -462,6 +462,55 @@ static void test_local_events(void)
 	tracefs_list_free(systems);
 }
 
+struct test_walk_instance {
+	struct tracefs_instance *instance;
+	bool found;
+};
+#define WALK_COUNT 10
+int test_instances_walk_cb(const char *name, void *data)
+{
+	struct test_walk_instance *instances  = (struct test_walk_instance *)data;
+	int i;
+
+	CU_TEST(instances != NULL);
+	CU_TEST(name != NULL);
+
+	for (i = 0; i < WALK_COUNT; i++) {
+		if (!strcmp(name,
+			    tracefs_instance_get_name(instances[i].instance))) {
+			instances[i].found = true;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static void test_instances_walk(void)
+{
+	struct test_walk_instance instances[WALK_COUNT];
+	int i;
+
+	memset(instances, 0, WALK_COUNT * sizeof(struct test_walk_instance));
+	for (i = 0; i < WALK_COUNT; i++) {
+		instances[i].instance = tracefs_instance_create(get_rand_str());
+		CU_TEST(instances[i].instance != NULL);
+	}
+
+	CU_TEST(tracefs_instances_walk(test_instances_walk_cb, instances) == 0);
+	for (i = 0; i < WALK_COUNT; i++) {
+		CU_TEST(instances[i].found);
+		tracefs_instance_destroy(instances[i].instance);
+		instances[i].found = false;
+	}
+
+	CU_TEST(tracefs_instances_walk(test_instances_walk_cb, instances) == 0);
+	for (i = 0; i < WALK_COUNT; i++) {
+		CU_TEST(!instances[i].found);
+		tracefs_instance_free(instances[i].instance);
+	}
+}
+
 static int test_suite_destroy(void)
 {
 	tracefs_instance_destroy(test_instance);
@@ -505,4 +554,7 @@ void test_tracefs_lib(void)
 		    test_tracers);
 	CU_add_test(suite, "tracefs_local events API",
 		    test_local_events);
+	CU_add_test(suite, "tracefs_instances_walk API",
+		    test_instances_walk);
+
 }
-- 
2.28.0

Re: [PATCH v3 5/6] libtracefs: Add new tracefs API tracefs_instances_walk()

[Steven Rostedt]

On Thu, 12 Nov 2020 11:11:08 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The logic for finding all configured ftrace instances is encapuslated in
> a 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      | 55 +++++++++++++++++++++++++++++
>  tracecmd/trace-stat.c               | 52 +++++++--------------------
>  utest/tracefs-utest.c               | 52 +++++++++++++++++++++++++++
>  6 files changed, 129 insertions(+), 46 deletions(-)
> 
> diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
> index 388d8f94..bcf3dd64 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_instance_exists(const char *name);
>  bool tracefs_file_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 6b796382..f2c6046c 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)

We should get into the habit of adding "__hidden" to anything that is not
static but also not visible outside the library.

-- Steve


>  {
>  	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;
>  
> @@ -493,7 +493,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)
> 

[PATCH v3 6/6] trace-cmd: Add new libtrasefs API to get the current trace clock

[Tzvetomir Stoyanov (VMware)]

A new API is added to tracefs library:
	tracefs_get_clock()
It reads the trace_clock file from the ftrace file system, parses it
and returns a string with the current trace clock. The returned string
must be freed by free().

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h      |  2 ++
 lib/tracefs/tracefs-instance.c | 35 ++++++++++++++++++++++++++++++++++
 tracecmd/trace-stat.c          | 29 +++++++---------------------
 utest/tracefs-utest.c          | 30 ++++++++++++++++++++++++++++-
 4 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index bcf3dd64..3358e33d 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -57,4 +57,6 @@ struct tep_handle *tracefs_local_events_system(const char *tracing_dir,
 int tracefs_fill_local_events(const char *tracing_dir,
 			       struct tep_handle *tep, int *parsing_failures);
 
+char *tracefs_get_clock(struct tracefs_instance *instance);
+
 #endif /* _TRACE_FS_H */
diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index 7b6d9417..d806636e 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -397,3 +397,38 @@ out:
 	tracefs_put_tracing_file(path);
 	return fret;
 }
+
+/**
+ * tracefs_get_clock - Get the current trace clock
+ * @instance: ftrace instance, can be NULL for the top instance
+ *
+ * Returns the current trace clock of the given instance, or NULL in
+ * case of an error.
+ * The return string must be freed by free()
+ */
+char *tracefs_get_clock(struct tracefs_instance *instance)
+{
+	char *all_clocks = NULL;
+	char *ret = NULL;
+	int bytes = 0;
+	char *clock;
+	char *cont;
+
+	all_clocks  = tracefs_instance_file_read(instance, "trace_clock", &bytes);
+	if (!all_clocks || !bytes)
+		goto out;
+
+	clock = strstr(all_clocks, "[");
+	if (!clock)
+		goto out;
+	clock++;
+	cont = strstr(clock, "]");
+	if (!cont)
+		goto out;
+	*cont = '\0';
+
+	ret = strdup(clock);
+out:
+	free(all_clocks);
+	return ret;
+}
diff --git a/tracecmd/trace-stat.c b/tracecmd/trace-stat.c
index e6678eab..9b61a14f 100644
--- a/tracecmd/trace-stat.c
+++ b/tracecmd/trace-stat.c
@@ -710,33 +710,18 @@ static void report_buffers(struct buffer_instance *instance)
 
 static void report_clock(struct buffer_instance *instance)
 {
-	char *str;
-	char *cont;
 	char *clock;
 
-	str = get_instance_file_content(instance, "trace_clock");
-	if (!str)
-		return;
-
-	clock = strstr(str, "[");
-	if (!clock)
-		goto out;
-	clock++;
-
-	cont = strstr(clock, "]");
-	if (!cont) /* should never happen */
-		goto out;
-
-	*cont = '\0';
+	if (instance)
+		clock = tracefs_get_clock(instance->tracefs);
+	else
+		clock = tracefs_get_clock(NULL);
 
 	/* Default clock is "local", only show others */
-	if (strcmp(clock, "local") == 0)
-		goto out;
+	if (clock && !strcmp(clock, "local") == 0)
+		printf("\nClock: %s\n", clock);
 
-	printf("\nClock: %s\n", clock);
-
- out:
-	free(str);
+	free(clock);
 }
 
 static void report_cpumask(struct buffer_instance *instance)
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index c42fea12..78eda481 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -511,6 +511,33 @@ static void test_instances_walk(void)
 	}
 }
 
+static void current_clock_check(const char *clock)
+{
+	int size = 0;
+	char *clocks;
+	char *str;
+
+	clocks = tracefs_instance_file_read(test_instance, "trace_clock", &size);
+	CU_TEST(clocks != NULL);
+	CU_TEST(size > strlen(clock));
+	str = strstr(clocks, clock);
+	CU_TEST(str != NULL);
+	CU_TEST(str != clocks);
+	CU_TEST(*(str - 1) == '[');
+	CU_TEST(*(str + strlen(clock)) == ']');
+	free(clocks);
+}
+
+static void test_get_clock(void)
+{
+	const char *clock;
+
+	clock = tracefs_get_clock(test_instance);
+	CU_TEST(clock != NULL);
+	current_clock_check(clock);
+	free((char *)clock);
+}
+
 static int test_suite_destroy(void)
 {
 	tracefs_instance_destroy(test_instance);
@@ -556,5 +583,6 @@ void test_tracefs_lib(void)
 		    test_local_events);
 	CU_add_test(suite, "tracefs_instances_walk API",
 		    test_instances_walk);
-
+	CU_add_test(suite, "tracefs_get_clock API",
+		    test_get_clock);
 }
-- 
2.28.0

Re: [PATCH v3 6/6] trace-cmd: Add new libtrasefs API to get the current trace clock

[Steven Rostedt]

On Thu, 12 Nov 2020 11:11:09 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

>  static void report_clock(struct buffer_instance *instance)
>  {
> -	char *str;
> -	char *cont;
>  	char *clock;
>  
> -	str = get_instance_file_content(instance, "trace_clock");
> -	if (!str)
> -		return;
> -
> -	clock = strstr(str, "[");
> -	if (!clock)
> -		goto out;
> -	clock++;
> -
> -	cont = strstr(clock, "]");
> -	if (!cont) /* should never happen */
> -		goto out;
> -
> -	*cont = '\0';
> +	if (instance)
> +		clock = tracefs_get_clock(instance->tracefs);
> +	else
> +		clock = tracefs_get_clock(NULL);

Small nit, but I find it cleaner in a case that both sides of an if
condition do basically the same thing with a different variable, to just
set the variable:

	struct tracefs_instance *tracefs = instance ? instance->tracefs : NULL;

	[..]

	clock = tracefs_get_clock(tracefs);

Other than that, the rest looks good.

Thanks Tzvetomir!

-- Steve


>  
>  	/* Default clock is "local", only show others */
> -	if (strcmp(clock, "local") == 0)
> -		goto out;
> +	if (clock && !strcmp(clock, "local") == 0)
> +		printf("\nClock: %s\n", clock);
>  
> -	printf("\nClock: %s\n", clock);
> -
> - out:
> -	free(str);
> +	free(clock);
>  }
>  
>  static void report_cpumask(struct buffer_instance *instance)
> diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
> index c42fea12..78eda481 100644
> --- a/utest/tracefs-utest.c
> +++ b/utest/tracefs-utest.c
> @@ -511,6 +511,33 @@ static void test_instances_walk(void)
>  	}
>  }
>