[PATCH v3 00/21] trace-cmd fixes and clean-ups

[PATCH v3 00/21] trace-cmd fixes and clean-ups

[Tzvetomir Stoyanov (VMware)]

Various trace-cmd application and library fixes, clean-ups and internal
refactoring, needed for trace file version 7 and compression changes.

This patch-set supersedes "[v2,00/87] Trace file version 7":
   https://lore.kernel.org/linux-trace-devel/20210729050959.12263-1-tz.stoyanov@gmail.com/

v3 changes:
 - fixed issues of split and convert commands with some corner cases
v2 changes:
 - more cleanups, forgotten in the first version

Tzvetomir Stoyanov (VMware) (21):
  trace-cmd library: Read option id with correct endian
  trace-cmd report: Fix typos in error messages
  trace-cmd library: Fix version string memory leak
  trace-cmd library: Fixed a memory leak on input handler close
  trace-cmd library: Do not pass guests list to a buffer instance
  trace-cmd library: Set long size to the input tep handler when it is
    read from the file
  trace-cmd library: Do not use local variables when reading CPU stat
    option
  trace-cmd library: Reuse within the library the function that checks
    file state.
  trace-cmd library: Fix possible memory leak in read_ftrace_files()
  trace-cmd library: Fix possible memory leak in read_event_files()
  trace-cmd library: Fix possible memory leak in read_proc_kallsyms()
  trace-cmd library: Fix possible memory leak in read_ftrace_printk()
  trace-cmd library: Fix possible memory leak in
    read_and_parse_cmdlines()
  trace-cmd library: Set input handler default values in allocation
    function
  trace-cmd library: Track maximum CPUs count in input handler
  trace-cmd report: Close input file handlers on exit
  trace-cmd report: Do not print empty buffer name
  trace-cmd library: Set correct CPU to the record, retrieved with
    tracecmd_peek_data
  trace-cmd library: Refactor APIs for creating output handler
  trace-cmd library: Refactor the logic for writing trace data in the
    file
  trace-cmd report: Init the top trace instance earlier

 .../include/private/trace-cmd-private.h       |  35 +-
 lib/trace-cmd/include/trace-cmd-local.h       |  19 +
 lib/trace-cmd/trace-input.c                   | 211 +++--
 lib/trace-cmd/trace-output.c                  | 779 +++++++++++-------
 lib/trace-cmd/trace-util.c                    |  28 +
 tracecmd/trace-listen.c                       |   2 +-
 tracecmd/trace-read.c                         |  21 +-
 tracecmd/trace-record.c                       |  73 +-
 tracecmd/trace-restore.c                      |  32 +-
 9 files changed, 783 insertions(+), 417 deletions(-)

-- 
2.31.1

[PATCH v3 01/21] trace-cmd library: Read option id with correct endian

[Tzvetomir Stoyanov (VMware)]

The id of a trace option is 2 bytes short integer. When reading it from
the trace file, use the tep handler associated with the file, if
available, to convert the option with the correct endian order.
A new helper function is introduced to read and convert 2 byte integer.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index ac57bc4f..0dbcdbdc 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -336,6 +336,18 @@ static char *read_string(struct tracecmd_input *handle)
 	return NULL;
 }
 
+static int read2(struct tracecmd_input *handle, unsigned short *size)
+{
+	struct tep_handle *pevent = handle->pevent;
+	unsigned short data;
+
+	if (do_read_check(handle, &data, 2))
+		return -1;
+
+	*size = tep_read_number(pevent, &data, 2);
+	return 0;
+}
+
 static int read4(struct tracecmd_input *handle, unsigned int *size)
 {
 	struct tep_handle *pevent = handle->pevent;
@@ -2660,16 +2672,15 @@ static int handle_options(struct tracecmd_input *handle)
 	handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
 
 	for (;;) {
-		if (do_read_check(handle, &option, 2))
+		if (read2(handle, &option))
 			return -1;
 
 		if (option == TRACECMD_OPTION_DONE)
 			break;
 
 		/* next 4 bytes is the size of the option */
-		if (do_read_check(handle, &size, 4))
+		if (read4(handle, &size))
 			return -1;
-		size = tep_read_number(handle->pevent, &size, 4);
 		buf = malloc(size);
 		if (!buf)
 			return -ENOMEM;
-- 
2.31.1

[PATCH v3 02/21] trace-cmd report: Fix typos in error messages

[Tzvetomir Stoyanov (VMware)]

Fixed typos in "trace-cmd report" messages that report reading and
parsing errors.

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index 6f43c1d2..31724b09 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -1290,7 +1290,7 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 					die("error in reading buffer instance");
 				new_handle = tracecmd_buffer_instance_handle(handles->handle, i);
 				if (!new_handle) {
-					warning("could not retreive handle %s", name);
+					warning("could not retrieve handle %s", name);
 					continue;
 				}
 				add_handle(new_handle, name);
@@ -1324,7 +1324,7 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 		if (last_record) {
 			int cpu = last_record->cpu;
 			if (cpu >= last_handle->cpus)
-				die("cpu %d creater than %d\n", cpu, last_handle->cpus);
+				die("cpu %d greater than %d\n", cpu, last_handle->cpus);
 			if (tscheck &&
 			    last_handle->last_timestamp[cpu] > last_record->ts) {
 				errno = 0;
-- 
2.31.1

[PATCH v3 03/21] trace-cmd library: Fix version string memory leak

[Tzvetomir Stoyanov (VMware)]

The version string is allocated when a VERSION option is processed, but
is never freed. Free it on input handler close.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 0dbcdbdc..9253bc37 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3518,6 +3518,7 @@ void tracecmd_close(struct tracecmd_input *handle)
 	free(handle->cpu_data);
 	free(handle->uname);
 	free(handle->trace_clock);
+	free(handle->version);
 	close(handle->fd);
 
 	tracecmd_free_hooks(handle->hooks);
@@ -3959,6 +3960,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 	new_handle->cpu_data = NULL;
 	new_handle->nr_buffers = 0;
 	new_handle->buffers = NULL;
+	new_handle->version = NULL;
 	new_handle->ref = 1;
 	if (handle->trace_clock) {
 		new_handle->trace_clock = strdup(handle->trace_clock);
-- 
2.31.1

[PATCH v3 04/21] trace-cmd library: Fixed a memory leak on input handler close

[Tzvetomir Stoyanov (VMware)]

When an input handler to a trace file is closed with tracecmd_close(),
the list with buffers is not freed. This leads to a memory leak. Added
logic to free that list.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 9253bc37..ffe87e8a 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3484,6 +3484,7 @@ void tracecmd_ref(struct tracecmd_input *handle)
 void tracecmd_close(struct tracecmd_input *handle)
 {
 	int cpu;
+	int i;
 
 	if (!handle)
 		return;
@@ -3521,6 +3522,10 @@ void tracecmd_close(struct tracecmd_input *handle)
 	free(handle->version);
 	close(handle->fd);
 
+	for (i = 0; i < handle->nr_buffers; i++)
+		free(handle->buffers[i].name);
+	free(handle->buffers);
+
 	tracecmd_free_hooks(handle->hooks);
 	handle->hooks = NULL;
 
-- 
2.31.1

[PATCH v3 05/21] trace-cmd library: Do not pass guests list to a buffer instance

[Tzvetomir Stoyanov (VMware)]

When a new trace buffer is read from the trace file, a new input handler
is duplicated from the top one. The pointer to the guests list should
not be duplicated, as it could lead to a memory corruption on
handler close.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index ffe87e8a..e5c8c332 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3966,6 +3966,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 	new_handle->nr_buffers = 0;
 	new_handle->buffers = NULL;
 	new_handle->version = NULL;
+	new_handle->guest = NULL;
 	new_handle->ref = 1;
 	if (handle->trace_clock) {
 		new_handle->trace_clock = strdup(handle->trace_clock);
-- 
2.31.1

[PATCH v3 06/21] trace-cmd library: Set long size to the input tep handler when it is read from the file

[Tzvetomir Stoyanov (VMware)]

Setting the long size to the input tep handler in tracecmd_read_headers()
API may be too late, as this tep handler is used to read and parse data
from the file before that. The most suitable place for that is
tracecmd_alloc_fd() API, right after reading the long size from the
file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index e5c8c332..3d7f1c48 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -860,8 +860,6 @@ int tracecmd_read_headers(struct tracecmd_input *handle,
 	if (ret < 0)
 		return -1;
 
-	tep_set_long_size(handle->pevent, handle->long_size);
-
 	if (state <= handle->file_state)
 		return 0;
 
@@ -3337,6 +3335,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 
 	do_read_check(handle, buf, 1);
 	handle->long_size = buf[0];
+	tep_set_long_size(handle->pevent, handle->long_size);
 
 	read4(handle, &page_size);
 	handle->page_size = page_size;
-- 
2.31.1

[PATCH v3 07/21] trace-cmd library: Do not use local variables when reading CPU stat option

[Tzvetomir Stoyanov (VMware)]

Using a local variable to read all CPUSTAT options assumes that all of
them are in a single option section. While this is true for the current
trace file version format, this assumption limits the design of a more
flexible format with multiple options sections. Use input handler context
instead of the local variable.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 3d7f1c48..d020cfae 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -138,6 +138,7 @@ struct tracecmd_input {
 
 	struct host_trace_info	host;
 	double			ts2secs;
+	unsigned int		cpustats_size;
 	char *			cpustats;
 	char *			uname;
 	char *			version;
@@ -2658,7 +2659,6 @@ static int handle_options(struct tracecmd_input *handle)
 	unsigned short option;
 	unsigned int size;
 	char *cpustats = NULL;
-	unsigned int cpustats_size = 0;
 	struct input_buffer_instance *buffer;
 	struct hook_list *hook;
 	char *buf;
@@ -2738,12 +2738,16 @@ static int handle_options(struct tracecmd_input *handle)
 			break;
 		case TRACECMD_OPTION_CPUSTAT:
 			buf[size-1] = '\n';
-			cpustats = realloc(cpustats, cpustats_size + size + 1);
-			if (!cpustats)
-				return -ENOMEM;
-			memcpy(cpustats + cpustats_size, buf, size);
-			cpustats_size += size;
-			cpustats[cpustats_size] = 0;
+			cpustats = realloc(handle->cpustats,
+					   handle->cpustats_size + size + 1);
+			if (!cpustats) {
+				ret = -ENOMEM;
+				return ret;
+			}
+			memcpy(cpustats + handle->cpustats_size, buf, size);
+			handle->cpustats_size += size;
+			cpustats[handle->cpustats_size] = 0;
+			handle->cpustats = cpustats;
 			break;
 		case TRACECMD_OPTION_BUFFER:
 			/* A buffer instance is saved at the end of the file */
@@ -2813,8 +2817,6 @@ static int handle_options(struct tracecmd_input *handle)
 
 	}
 
-	handle->cpustats = cpustats;
-
 	return 0;
 }
 
-- 
2.31.1

[PATCH v3 08/21] trace-cmd library: Reuse within the library the function that checks file state.

[Tzvetomir Stoyanov (VMware)]

Make the function, that checks if the next file state is valid, global
for the trace-cmd library, so it can be reused. It is important the same check
logic to be used in the whole library.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/include/trace-cmd-local.h |  2 +
 lib/trace-cmd/trace-input.c             |  1 +
 lib/trace-cmd/trace-output.c            | 66 ++++++++-----------------
 lib/trace-cmd/trace-util.c              | 28 +++++++++++
 4 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index 821b5cdb..7f3b45a8 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -31,5 +31,7 @@ void tracecmd_info(const char *fmt, ...);
 #endif
 #endif
 
+bool check_file_state(unsigned long file_version, int current_state, int new_state);
+bool check_out_state(struct tracecmd_output *handle, int new_state);
 
 #endif /* _TRACE_CMD_LOCAL_H */
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index d020cfae..bd0517e1 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -4222,3 +4222,4 @@ int tracecmd_enable_tsync(struct tracecmd_input *handle, bool enable)
 
 	return 0;
 }
+
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index a8de107c..fe1d1aaa 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -307,33 +307,6 @@ int tracecmd_ftrace_enable(int set)
 	return ret;
 }
 
-static int check_out_state(struct tracecmd_output *handle, int new_state)
-{
-	if (!handle)
-		return -1;
-
-	switch (new_state) {
-	case TRACECMD_FILE_HEADERS:
-	case TRACECMD_FILE_FTRACE_EVENTS:
-	case TRACECMD_FILE_ALL_EVENTS:
-	case TRACECMD_FILE_KALLSYMS:
-	case TRACECMD_FILE_PRINTK:
-	case TRACECMD_FILE_CMD_LINES:
-	case TRACECMD_FILE_CPU_COUNT:
-	case TRACECMD_FILE_OPTIONS:
-		if (handle->file_state == (new_state - 1))
-			return 0;
-		break;
-	case TRACECMD_FILE_CPU_LATENCY:
-	case TRACECMD_FILE_CPU_FLYRECORD:
-		if (handle->file_state == TRACECMD_FILE_OPTIONS)
-			return 0;
-		break;
-	}
-
-	return -1;
-}
-
 static int read_header_files(struct tracecmd_output *handle)
 {
 	tsize_t size, check_size, endian8;
@@ -342,7 +315,7 @@ static int read_header_files(struct tracecmd_output *handle)
 	int fd;
 	int ret;
 
-	if (check_out_state(handle, TRACECMD_FILE_HEADERS) < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_HEADERS)) {
 		tracecmd_warning("Cannot read header files, unexpected state 0x%X",
 				 handle->file_state);
 		return -1;
@@ -654,7 +627,7 @@ static int read_ftrace_files(struct tracecmd_output *handle)
 	struct tracecmd_event_list list = { .glob = "ftrace/*" };
 	int ret;
 
-	if (check_out_state(handle, TRACECMD_FILE_FTRACE_EVENTS) < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_FTRACE_EVENTS)) {
 		tracecmd_warning("Cannot read ftrace files, unexpected state 0x%X",
 				 handle->file_state);
 		return -1;
@@ -695,7 +668,7 @@ static int read_event_files(struct tracecmd_output *handle,
 	int endian4;
 	int ret;
 
-	if (check_out_state(handle, TRACECMD_FILE_ALL_EVENTS) < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_ALL_EVENTS)) {
 		tracecmd_warning("Cannot read event files, unexpected state 0x%X",
 				 handle->file_state);
 		return -1;
@@ -787,7 +760,7 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
 	struct stat st;
 	int ret;
 
-	if (check_out_state(handle, TRACECMD_FILE_KALLSYMS) < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_KALLSYMS)) {
 		tracecmd_warning("Cannot read kallsyms, unexpected state 0x%X",
 				 handle->file_state);
 		return -1;
@@ -832,7 +805,7 @@ static int read_ftrace_printk(struct tracecmd_output *handle)
 	char *path;
 	int ret;
 
-	if (check_out_state(handle, TRACECMD_FILE_PRINTK) < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_PRINTK)) {
 		tracecmd_warning("Cannot read printk, unexpected state 0x%X",
 				 handle->file_state);
 		return -1;
@@ -1134,11 +1107,10 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
 {
 	int ret;
 
-	ret = check_out_state(handle, TRACECMD_FILE_CPU_COUNT);
-	if (ret < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_CPU_COUNT)) {
 		tracecmd_warning("Cannot write CPU count into the file, unexpected state 0x%X",
 				 handle->file_state);
-		return ret;
+		return -1;
 	}
 	cpus = convert_endian_4(handle, cpus);
 	ret = do_write_check(handle, &cpus, 4);
@@ -1154,16 +1126,14 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 	unsigned short option;
 	unsigned short endian2;
 	unsigned int endian4;
-	int ret;
 
 	/* If already written, ignore */
 	if (handle->file_state == TRACECMD_FILE_OPTIONS)
 		return 0;
-	ret = check_out_state(handle, TRACECMD_FILE_OPTIONS);
-	if (ret < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_OPTIONS)) {
 		tracecmd_warning("Cannot write options into the file, unexpected state 0x%X",
 				 handle->file_state);
-		return ret;
+		return -1;
 	}
 
 	if (do_write_check(handle, "options  ", 10))
@@ -1281,11 +1251,10 @@ int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 {
 	int ret;
 
-	ret = check_out_state(handle, TRACECMD_FILE_CMD_LINES);
-	if (ret < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_CMD_LINES)) {
 		tracecmd_warning("Cannot write command lines into the file, unexpected state 0x%X",
 				 handle->file_state);
-		return ret;
+		return -1;
 	}
 	ret = save_tracing_file_data(handle, "saved_cmdlines");
 	if (ret < 0)
@@ -1298,7 +1267,6 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 {
 	struct tracecmd_output *handle;
 	char *path;
-	int ret;
 
 	handle = create_file(output_file, NULL, NULL, NULL, &all_event_list);
 	if (!handle)
@@ -1316,8 +1284,7 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 	if (tracecmd_write_options(handle) < 0)
 		goto out_free;
 
-	ret = check_out_state(handle, TRACECMD_FILE_CPU_LATENCY);
-	if (ret < 0) {
+	if (!check_out_state(handle, TRACECMD_FILE_CPU_LATENCY)) {
 		tracecmd_warning("Cannot write latency data into the file, unexpected state 0x%X",
 				 handle->file_state);
 		goto out_free;
@@ -1399,7 +1366,9 @@ int tracecmd_write_cpu_data(struct tracecmd_output *handle,
 
 	/* This can be called multiple times (when recording instances) */
 	ret = handle->file_state == TRACECMD_FILE_CPU_FLYRECORD ? 0 :
-		check_out_state(handle, TRACECMD_FILE_CPU_FLYRECORD);
+				    check_file_state(handle->file_version,
+						     handle->file_state,
+						     TRACECMD_FILE_CPU_FLYRECORD);
 	if (ret < 0) {
 		tracecmd_warning("Cannot write trace data into the file, unexpected state 0x%X",
 				 handle->file_state);
@@ -1661,3 +1630,8 @@ out_free:
 	tracecmd_output_close(handle);
 	return NULL;
 }
+
+__hidden bool check_out_state(struct tracecmd_output *handle, int new_state)
+{
+	return check_file_state(handle->file_version, handle->file_state, new_state);
+}
diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index 9bc94822..db7ce7ee 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -624,3 +624,31 @@ bool tracecmd_is_version_supported(unsigned int version)
 		return true;
 	return false;
 }
+
+__hidden bool check_file_state(unsigned long file_version, int current_state, int new_state)
+{
+	switch (new_state) {
+	case TRACECMD_FILE_HEADERS:
+	case TRACECMD_FILE_FTRACE_EVENTS:
+	case TRACECMD_FILE_ALL_EVENTS:
+	case TRACECMD_FILE_KALLSYMS:
+	case TRACECMD_FILE_PRINTK:
+	case TRACECMD_FILE_CMD_LINES:
+	case TRACECMD_FILE_CPU_COUNT:
+		if (current_state == (new_state - 1))
+			return true;
+		break;
+	case TRACECMD_FILE_OPTIONS:
+		if (current_state == (new_state - 1))
+			return true;
+		break;
+	case TRACECMD_FILE_CPU_LATENCY:
+	case TRACECMD_FILE_CPU_FLYRECORD:
+		if (current_state == TRACECMD_FILE_OPTIONS)
+			return true;
+		break;
+	}
+
+	return false;
+}
+
-- 
2.31.1

[PATCH v3 09/21] trace-cmd library: Fix possible memory leak in read_ftrace_files()

[Tzvetomir Stoyanov (VMware)]

Some error paths in read_ftrace_files() may lead to a memory leak.
Improved the error handling of this internal function to avoid it.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index bd0517e1..94fd8693 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -616,28 +616,30 @@ static int read_ftrace_files(struct tracecmd_input *handle, const char *regex)
 		}
 	}
 
-	if (read4(handle, &count) < 0)
-		return -1;
+	ret = read4(handle, &count);
+	if (ret < 0)
+		goto out;
 
 	for (i = 0; i < count; i++) {
-		if (read8(handle, &size) < 0)
-			return -1;
+		ret = read8(handle, &size);
+		if (ret < 0)
+			goto out;
 		ret = read_ftrace_file(handle, size, print_all, ereg);
 		if (ret < 0)
-			return -1;
+			goto out;
 	}
 
 	handle->event_files_start =
 		lseek64(handle->fd, 0, SEEK_CUR);
-
+	handle->file_state = TRACECMD_FILE_FTRACE_EVENTS;
+	ret = 0;
+out:
 	if (sreg) {
 		regfree(sreg);
 		regfree(ereg);
 	}
 
-	handle->file_state = TRACECMD_FILE_FTRACE_EVENTS;
-
-	return 0;
+	return ret;
 }
 
 static int read_event_files(struct tracecmd_input *handle, const char *regex)
-- 
2.31.1

[PATCH v3 10/21] trace-cmd library: Fix possible memory leak in read_event_files()

[Tzvetomir Stoyanov (VMware)]

Some error paths in read_event_files() may lead to a memory leak.
Improved the error handling of this internal function to avoid it.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 39 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 94fd8693..32358ce9 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -645,7 +645,7 @@ out:
 static int read_event_files(struct tracecmd_input *handle, const char *regex)
 {
 	unsigned long long size;
-	char *system;
+	char *system = NULL;
 	regex_t spreg;
 	regex_t epreg;
 	regex_t *sreg = NULL;
@@ -670,13 +670,16 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
 			return -1;
 	}
 
-	if (read4(handle, &systems) < 0)
-		return -1;
+	ret = read4(handle, &systems);
+	if (ret < 0)
+		goto out;
 
 	for (i = 0; i < systems; i++) {
 		system = read_string(handle);
-		if (!system)
-			return -1;
+		if (!system) {
+			ret = -1;
+			goto out;
+		}
 
 		sys_printed = 0;
 		print_all = 0;
@@ -703,39 +706,35 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
 			}
 		}
 
-		if (read4(handle, &count) < 0)
-			goto failed;
+		ret = read4(handle, &count);
+		if (ret < 0)
+			goto out;
 
 		for (x=0; x < count; x++) {
-			if (read8(handle, &size) < 0)
-				goto failed;
+			ret = read8(handle, &size);
+			if (ret < 0)
+				goto out;
 
 			ret = read_event_file(handle, system, size,
 					      print_all, &sys_printed,
 					      reg);
 			if (ret < 0)
-				goto failed;
+				goto out;
 		}
 		free(system);
-	}
-
-	if (sreg) {
-		regfree(sreg);
-		regfree(ereg);
+		system = NULL;
 	}
 
 	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
-
-	return 0;
-
- failed:
+	ret = 0;
+ out:
 	if (sreg) {
 		regfree(sreg);
 		regfree(ereg);
 	}
 
 	free(system);
-	return -1;
+	return ret;
 }
 
 static int read_proc_kallsyms(struct tracecmd_input *handle)
-- 
2.31.1

[PATCH v3 11/21] trace-cmd library: Fix possible memory leak in read_proc_kallsyms()

[Tzvetomir Stoyanov (VMware)]

Some error paths in read_proc_kallsyms() may lead to a memory leak.
Improved the error handling of this internal function to avoid it.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 39 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 32358ce9..9b23063e 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -739,34 +739,39 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
 
 static int read_proc_kallsyms(struct tracecmd_input *handle)
 {
-	struct tep_handle *pevent = handle->pevent;
+	struct tep_handle *tep = handle->pevent;
 	unsigned int size;
-	char *buf;
+	char *buf = NULL;
+	int ret;
 
 	if (handle->file_state >= TRACECMD_FILE_KALLSYMS)
 		return 0;
 
-	if (read4(handle, &size) < 0)
-		return -1;
-	if (!size)
-		return 0; /* OK? */
+	ret = read4(handle, &size);
+	if (ret < 0)
+		goto out;
+	if (!size) {
+		handle->file_state = TRACECMD_FILE_KALLSYMS;
+		goto out; /* OK? */
+	}
 
 	buf = malloc(size+1);
-	if (!buf)
-		return -1;
-	if (do_read_check(handle, buf, size)){
-		free(buf);
-		return -1;
+	if (!buf) {
+		ret = -1;
+		goto out;
 	}
-	buf[size] = 0;
-
-	tep_parse_kallsyms(pevent, buf);
+	ret = do_read_check(handle, buf, size);
+	if (ret < 0)
+		goto out;
 
-	free(buf);
+	buf[size] = 0;
+	tep_parse_kallsyms(tep, buf);
 
 	handle->file_state = TRACECMD_FILE_KALLSYMS;
-
-	return 0;
+	ret = 0;
+out:
+	free(buf);
+	return ret;
 }
 
 static int read_ftrace_printk(struct tracecmd_input *handle)
-- 
2.31.1

[PATCH v3 12/21] trace-cmd library: Fix possible memory leak in read_ftrace_printk()

[Tzvetomir Stoyanov (VMware)]

Some error paths in read_ftrace_printk() may lead to a memory leak.
Improved the error handling of this internal function to avoid it.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 9b23063e..60d75d47 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -777,33 +777,39 @@ out:
 static int read_ftrace_printk(struct tracecmd_input *handle)
 {
 	unsigned int size;
-	char *buf;
+	char *buf = NULL;
+	int ret;
 
 	if (handle->file_state >= TRACECMD_FILE_PRINTK)
 		return 0;
 
-	if (read4(handle, &size) < 0)
-		return -1;
-	if (!size)
-		return 0; /* OK? */
+	ret = read4(handle, &size);
+	if (ret < 0)
+		goto out;
+	if (!size) {
+		handle->file_state = TRACECMD_FILE_PRINTK;
+		goto out; /* OK? */
+	}
 
 	buf = malloc(size + 1);
-	if (!buf)
-		return -1;
-	if (do_read_check(handle, buf, size)) {
-		free(buf);
-		return -1;
+	if (!buf) {
+		ret = -1;
+		goto out;
 	}
+	ret = do_read_check(handle, buf, size);
+	if (ret < 0)
+		goto out;
 
 	buf[size] = 0;
 
 	tep_parse_printk_formats(handle->pevent, buf);
 
-	free(buf);
-
 	handle->file_state = TRACECMD_FILE_PRINTK;
+	ret = 0;
 
-	return 0;
+out:
+	free(buf);
+	return ret;
 }
 
 static int read_and_parse_cmdlines(struct tracecmd_input *handle);
-- 
2.31.1

[PATCH v3 13/21] trace-cmd library: Fix possible memory leak in read_and_parse_cmdlines()

[Tzvetomir Stoyanov (VMware)]

Some error paths in read_and_parse_cmdlines() may lead to a memory leak.
Improved the error handling of this internal function to avoid it.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 60d75d47..9fd7e6e8 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2999,20 +2999,26 @@ static int read_and_parse_cmdlines(struct tracecmd_input *handle)
 {
 	struct tep_handle *pevent = handle->pevent;
 	unsigned long long size;
-	char *cmdlines;
+	char *cmdlines = NULL;
+	int ret;
 
 	if (handle->file_state >= TRACECMD_FILE_CMD_LINES)
 		return 0;
 
-	if (read_data_and_size(handle, &cmdlines, &size) < 0)
-		return -1;
+	ret = read_data_and_size(handle, &cmdlines, &size);
+	if (ret < 0)
+		goto out;
+	if (!size) {
+		handle->file_state = TRACECMD_FILE_CMD_LINES;
+		goto out;
+	}
 	cmdlines[size] = 0;
 	tep_parse_saved_cmdlines(pevent, cmdlines);
-	free(cmdlines);
-
 	handle->file_state = TRACECMD_FILE_CMD_LINES;
-
-	return 0;
+	ret = 0;
+out:
+	free(cmdlines);
+	return ret;
 }
 
 static void extract_trace_clock(struct tracecmd_input *handle, char *line)
-- 
2.31.1

[PATCH v3 14/21] trace-cmd library: Set input handler default values in allocation function

[Tzvetomir Stoyanov (VMware)]

Set usecs flag by default when the input handler is allocated, it makes
more sense than setting it when options are handled. This clean up is
needed for the next trace file version, where multiple options
sections may exist.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 9fd7e6e8..cb8b9f14 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2677,8 +2677,6 @@ static int handle_options(struct tracecmd_input *handle)
 	int cpus;
 	int ret;
 
-	/* By default, use usecs, unless told otherwise */
-	handle->flags |= TRACECMD_FL_IN_USECS;
 	handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
 
 	for (;;) {
@@ -3310,6 +3308,8 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 
 	handle->fd = fd;
 	handle->ref = 1;
+	/* By default, use usecs, unless told otherwise */
+	handle->flags |= TRACECMD_FL_IN_USECS;
 
 	if (do_read_check(handle, buf, 3))
 		goto failed_read;
-- 
2.31.1

[PATCH v3 15/21] trace-cmd library: Track maximum CPUs count in input handler

[Tzvetomir Stoyanov (VMware)]

This clean up is needed for the design of the next trace file version,
where only CPUs with trace data could be stored in the file. Each trace
instance may have its own CPU count, depending on collected traces.
As the main input handler is used by the top trace instance, the
CPU count there is for the top trace instance and may differ with cpu
counts of the other instances. Added a new "max_cpu" member of the input
handler, that tracks the maximum CPU count of all instances, recorded in
the file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-input.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index cb8b9f14..dc82d3ea 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -125,6 +125,7 @@ struct tracecmd_input {
 	int			long_size;
 	int			page_size;
 	int			page_map_size;
+	int			max_cpu;
 	int			cpus;
 	int			ref;
 	int			nr_buffers;	/* buffer instances */
@@ -838,6 +839,7 @@ static int read_cpus(struct tracecmd_input *handle)
 		return -1;
 
 	handle->cpus = cpus;
+	handle->max_cpu = cpus;
 	tep_set_cpus(handle->pevent, handle->cpus);
 	handle->file_state = TRACECMD_FILE_CPU_COUNT;
 
@@ -2794,6 +2796,8 @@ static int handle_options(struct tracecmd_input *handle)
 		case TRACECMD_OPTION_CPUCOUNT:
 			cpus = *(int *)buf;
 			handle->cpus = tep_read_number(handle->pevent, &cpus, 4);
+			handle->max_cpu = handle->cpus;
+			tep_set_cpus(handle->pevent, handle->cpus);
 			break;
 		case TRACECMD_OPTION_PROCMAPS:
 			if (buf[size-1] == '\0')
@@ -4074,7 +4078,7 @@ int tracecmd_page_size(struct tracecmd_input *handle)
  */
 int tracecmd_cpus(struct tracecmd_input *handle)
 {
-	return handle->cpus;
+	return handle->max_cpu;
 }
 
 /**
-- 
2.31.1

[PATCH v3 16/21] trace-cmd report: Close input file handlers on exit

[Tzvetomir Stoyanov (VMware)]

When "trace-cmd report" is interrupted with "ctrl-c", close the input
handlers to opened trace files, to delete any temporary files used when
reading the trace data. This clean up is needed for the design of the
next version of the trace file.

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index 31724b09..4261088d 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -1363,7 +1363,14 @@ struct tracecmd_input *read_trace_header(const char *file, int flags)
 
 static void sig_end(int sig)
 {
+	struct handle_list *handles;
+
 	fprintf(stderr, "trace-cmd: Received SIGINT\n");
+
+	list_for_each_entry(handles, &handle_list, list) {
+		tracecmd_close(handles->handle);
+	}
+
 	exit(0);
 }
 
-- 
2.31.1

[PATCH v3 17/21] trace-cmd report: Do not print empty buffer name

[Tzvetomir Stoyanov (VMware)]

This clean up is needed for the design of the next version of the trace
file, where the top buffer is saved with its empty file name, string "".

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index 4261088d..02f75cb9 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -1173,7 +1173,7 @@ static void print_handle_file(struct handle_list *handles)
 	/* Only print file names if more than one file is read */
 	if (!multi_inputs && !instances)
 		return;
-	if (handles->file)
+	if (handles->file && *handles->file != '\0')
 		printf("%*s: ", max_file_size, handles->file);
 	else
 		printf("%*s  ", max_file_size, "");
-- 
2.31.1

[PATCH v3 18/21] trace-cmd library: Set correct CPU to the record, retrieved with tracecmd_peek_data

[Tzvetomir Stoyanov (VMware)]

This clean up is needed for the design of the next version of the trace
file, where only CPUs with trace data are stored in the file - empty CPUs
are omitted. Changed tracecmd_peek_data() to use the CPU id, instead of
the CPU index.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index dc82d3ea..de8e0d72 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2019,7 +2019,7 @@ read_again:
 
 	record->ts = handle->cpu_data[cpu].timestamp;
 	record->size = kbuffer_event_size(kbuf);
-	record->cpu = cpu;
+	record->cpu = handle->cpu_data[cpu].cpu;
 	record->data = data;
 	record->offset = handle->cpu_data[cpu].offset + index;
 	record->missed_events = kbuffer_missed_events(kbuf);
-- 
2.31.1

[PATCH v3 19/21] trace-cmd library: Refactor APIs for creating output handler

[Tzvetomir Stoyanov (VMware)]

In the trace-cmd library there are various APIs for allocating and
initializing output handler to a trace file. The existing APIs are use
case oriented, with a lot of parameters. Extending them for new use
cases, adding more input parameters, will make the library more complex
and not easy to use.
Almost all use case oriented APIs for output handler creation are
removed and replaced with new flow, which is easier to be extended with
new creation parameters.

Removed APIs:
 tracecmd_create_init_fd_msg()
 tracecmd_create_init_file_glob()
 tracecmd_create_init_fd_glob()
 tracecmd_create_init_file_override()

New APIs:
 tracecmd_output_allocate()
 tracecmd_output_set_msg()
 tracecmd_output_set_trace_dir()
 tracecmd_output_set_kallsyms()
 tracecmd_output_set_from_input()
 tracecmd_output_write_init()
 tracecmd_output_write_headers()

The new tracecmd_output_allocate() API allocates memory and performs
minimal initialization of an output handler to a trace file. No data
is written in the file.
The tracecmd_output_set_...() APIs can be used to set various
parameters to the newly allocated output handler, that affect the way
the data is written into the file.
When the output handler is configured for the desired use case, the
tracecmd_output_write_init() is used to start writing to the file, it
writes initial magic bytes.
The tracecmd_output_write_headers() API is used to write the initial
headers into the file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  25 +-
 lib/trace-cmd/trace-output.c                  | 406 ++++++++++++------
 tracecmd/trace-record.c                       |  55 ++-
 tracecmd/trace-restore.c                      |  32 +-
 4 files changed, 356 insertions(+), 162 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index c58b09e9..167f3804 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -102,7 +102,8 @@ static inline int tracecmd_host_bigendian(void)
 /* --- Opening and Reading the trace.dat file --- */
 
 enum tracecmd_file_states {
-	TRACECMD_FILE_INIT = 1,
+	TRACECMD_FILE_ALLOCATED = 0,
+	TRACECMD_FILE_INIT,
 	TRACECMD_FILE_HEADERS,
 	TRACECMD_FILE_FTRACE_EVENTS,
 	TRACECMD_FILE_ALL_EVENTS,
@@ -268,20 +269,20 @@ struct tracecmd_event_list {
 struct tracecmd_option;
 struct tracecmd_msg_handle;
 
+struct tracecmd_output *tracecmd_output_allocate(int fd);
+int tracecmd_output_set_msg(struct tracecmd_output *handler,
+			    struct tracecmd_msg_handle *msg_handle);
+int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir);
+int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms);
+int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle);
+int tracecmd_output_write_init(struct tracecmd_output *handler);
+int tracecmd_output_write_headers(struct tracecmd_output *handler,
+				  struct tracecmd_event_list *list);
+
 struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus);
-struct tracecmd_output *
-tracecmd_create_init_file_glob(const char *output_file,
-			       struct tracecmd_event_list *list);
 struct tracecmd_output *tracecmd_create_init_fd(int fd);
-struct tracecmd_output *
-tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list);
-struct tracecmd_output *
-tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
-			    struct tracecmd_event_list *list);
+
 struct tracecmd_output *tracecmd_create_init_file(const char *output_file);
-struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
-							   const char *tracing_dir,
-							   const char *kallsyms);
 struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
 					    unsigned short id, int size,
 					    const void *data);
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index fe1d1aaa..20116d99 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -31,11 +31,6 @@
 typedef unsigned long long	tsize_t;
 typedef long long		stsize_t;
 
-static struct tracecmd_event_list all_event_list = {
-	.next = NULL,
-	.glob = "all"
-};
-
 struct tracecmd_option {
 	unsigned short	id;
 	int		size;
@@ -54,11 +49,14 @@ struct tracecmd_output {
 	int			cpus;
 	struct tep_handle	*pevent;
 	char			*tracing_dir;
+	char			*kallsyms;
+	struct tracecmd_event_list *events;
 	int			nr_options;
 	bool			quiet;
 	unsigned long		file_state;
 	unsigned long		file_version;
 	size_t			options_start;
+	bool			big_endian;
 
 	struct list_head	options;
 	struct tracecmd_msg_handle *msg_handle;
@@ -682,7 +680,7 @@ static int read_event_files(struct tracecmd_output *handle,
 			break;
 	}
 	/* all events are listed, use a global glob */
-	if (list)
+	if (!event_list || list)
 		event_list = &all_events;
 
 	systems = create_event_list(handle, event_list);
@@ -752,8 +750,7 @@ err:
 		tracecmd_warning("can't set kptr_restrict");
 }
 
-static int read_proc_kallsyms(struct tracecmd_output *handle,
-			      const char *kallsyms)
+static int read_proc_kallsyms(struct tracecmd_output *handle)
 {
 	unsigned int size, check_size, endian4;
 	const char *path = "/proc/kallsyms";
@@ -766,8 +763,8 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
 		return -1;
 	}
 
-	if (kallsyms)
-		path = kallsyms;
+	if (handle->kallsyms)
+		path = handle->kallsyms;
 
 	ret = stat(path, &st);
 	if (ret < 0) {
@@ -883,137 +880,247 @@ out_free:
 	return ret;
 }
 
-static int select_file_version(struct tracecmd_output *handle,
-				struct tracecmd_input *ihandle)
-{
-	if (ihandle)
-		handle->file_version = tracecmd_get_in_file_version(ihandle);
-	else
-		handle->file_version = FILE_VERSION;
-
-	return 0;
-}
-
-static struct tracecmd_output *
-create_file_fd(int fd, struct tracecmd_input *ihandle,
-	       const char *tracing_dir,
-	       const char *kallsyms,
-	       struct tracecmd_event_list *list,
-	       struct tracecmd_msg_handle *msg_handle)
+/**
+ * tracecmd_output_allocate - allocate new output handler to a trace file
+ * @handle: file descriptor to an empty file, it can be -1 if the handler
+ *	    will not write to a file
+ *
+ * This API only allocates a handler and performs minimal initialization.
+ * No data is written in the file.
+ *
+ * Returns pointer to a newly allocated file descriptor, that can be used
+ * with other library APIs. In case of an error, NULL is returned. The returned
+ * handler must be freed with tracecmd_output_close() or tracecmd_output_free()
+ */
+struct tracecmd_output *tracecmd_output_allocate(int fd)
 {
 	struct tracecmd_output *handle;
-	struct tep_handle *pevent;
-	char buf[BUFSIZ];
-	int endian4;
 
-	handle = malloc(sizeof(*handle));
+	handle = calloc(1, sizeof(*handle));
 	if (!handle)
 		return NULL;
-	memset(handle, 0, sizeof(*handle));
-
-	list_head_init(&handle->options);
 
 	handle->fd = fd;
-	if (tracing_dir) {
-		handle->tracing_dir = strdup(tracing_dir);
-		if (!handle->tracing_dir)
-			goto out_free;
-	}
 
-	handle->msg_handle = msg_handle;
+	handle->file_version = FILE_VERSION;
 
-	if (select_file_version(handle, ihandle))
-		goto out_free;
+	handle->page_size = getpagesize();
 
-	buf[0] = 23;
-	buf[1] = 8;
-	buf[2] = 68;
-	memcpy(buf + 3, "tracing", 7);
+	if (tracecmd_host_bigendian())
+		handle->big_endian = true;
+	else
+		handle->big_endian = false;
 
-	if (do_write_check(handle, buf, 10))
-		goto out_free;
+	list_head_init(&handle->options);
 
-	sprintf(buf, "%lu", handle->file_version);
-	if (do_write_check(handle, buf, strlen(buf) + 1))
-		goto out_free;
+	handle->file_state = TRACECMD_FILE_ALLOCATED;
 
-	/* get endian and page size */
-	if (ihandle) {
-		pevent = tracecmd_get_tep(ihandle);
-		/* Use the pevent of the ihandle for later writes */
-		handle->pevent = tracecmd_get_tep(ihandle);
-		tep_ref(pevent);
-		if (tep_is_file_bigendian(pevent))
-			buf[0] = 1;
-		else
-			buf[0] = 0;
-		handle->page_size = tracecmd_page_size(ihandle);
-	} else {
-		if (tracecmd_host_bigendian())
-			buf[0] = 1;
-		else
-			buf[0] = 0;
-		handle->page_size = getpagesize();
-	}
+	return handle;
+}
 
-	if (do_write_check(handle, buf, 1))
-		goto out_free;
+/**
+ * tracecmd_output_set_msg - associated an output file handler with network message handler
+ * @handle: output handler to a trace file.
+ * @msg_handle: network handler, allocated by tracecmd_msg_handle_alloc()
+ *
+ * This API associates an output file handler with a network stream. All subsequent API calls
+ * with this output file handler will send data over the network using the @msg_handle, instead
+ * of writing to a file.
+ * This API must be called after the handler file version is set and before
+ * tracecmd_output_write_init().
+ *
+ * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
+ */
+int tracecmd_output_set_msg(struct tracecmd_output *handler, struct tracecmd_msg_handle *msg_handle)
+{
+	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
 
-	/* save size of long (this may not be what the kernel is) */
-	buf[0] = sizeof(long);
-	if (do_write_check(handle, buf, 1))
-		goto out_free;
+	handler->msg_handle = msg_handle;
 
-	endian4 = convert_endian_4(handle, handle->page_size);
-	if (do_write_check(handle, &endian4, 4))
-		goto out_free;
-	handle->file_state = TRACECMD_FILE_INIT;
+	return 0;
+}
 
-	if (ihandle)
-		return handle;
+/**
+ * tracecmd_output_set_trace_dir - Set a custom tracing dir, instead of system default
+ * @handle: output handler to a trace file.
+ * @tracing_dir: full path to a directory with tracing files
+ *
+ * This API associates an output file handler with a custom tracing directory, to be used when
+ * creating the trace file instead of the system default tracing directory.
+ * This API must be called before tracecmd_output_write_init().
+ *
+ * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
+ */
+int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir)
+{
+	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
 
-	if (read_header_files(handle))
-		goto out_free;
+	free(handler->tracing_dir);
+	if (tracing_dir) {
+		handler->tracing_dir = strdup(tracing_dir);
+		if (!handler->tracing_dir)
+			return -1;
+	} else
+		handler->tracing_dir = NULL;
 
-	if (read_ftrace_files(handle))
-		goto out_free;
+	return 0;
+}
 
-	if (read_event_files(handle, list))
-		goto out_free;
+/**
+ * tracecmd_output_set_kallsyms - Set a custom kernel symbols file, instead of system default
+ * @handle: output handler to a trace file.
+ * @tracing_dir: full path to a file with kernel symbols
+ *
+ * This API associates an output file handler with a custom kernel symbols file, to be used when
+ * creating the trace file instead of the system default kernel symbols file.
+ * This API must be called before tracecmd_output_write_init().
+ *
+ * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
+ */
+int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms)
+{
+	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
 
-	if (read_proc_kallsyms(handle, kallsyms))
-		goto out_free;
+	free(handler->kallsyms);
+	if (kallsyms) {
+		handler->kallsyms = strdup(kallsyms);
+		if (!handler->kallsyms)
+			return -1;
+	} else
+		handler->kallsyms = NULL;
 
-	if (read_ftrace_printk(handle))
-		goto out_free;
+	return 0;
+}
 
-	return handle;
+/**
+ * tracecmd_output_set_from_input - Inherit parameters from an existing trace file
+ * @handle: output handler to a trace file.
+ * @ihandle: input handler to an existing trace file.
+ *
+ * This API copies parameters from input handler @ihandle, associated with an existing trace file,
+ * to the output handler @handle, associated with file that is going to be created.
+ * These parameters are copied:
+ *  - tep handler
+ *  - page size
+ *  - file endian
+ *  - file version
+ *  - file compression protocol
+ * This API must be called before tracecmd_output_write_init().
+ *
+ * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
+ */
+int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle)
+{
+	if (!handler || !ihandle || handler->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
 
- out_free:
-	tracecmd_output_close(handle);
-	return NULL;
+	/* get endian, page size, file version and compression */
+	/* Use the pevent of the ihandle for later writes */
+	handler->pevent = tracecmd_get_tep(ihandle);
+	tep_ref(handler->pevent);
+	handler->page_size = tracecmd_page_size(ihandle);
+	handler->file_version = tracecmd_get_in_file_version(ihandle);
+	if (tep_is_file_bigendian(handler->pevent))
+		handler->big_endian = true;
+	else
+		handler->big_endian = false;
+
+
+	return 0;
 }
 
-static struct tracecmd_output *create_file(const char *output_file,
-					   struct tracecmd_input *ihandle,
-					   const char *tracing_dir,
-					   const char *kallsyms,
-					   struct tracecmd_event_list *list)
+/**
+ * tracecmd_output_write_init - Write the initial magics in the trace file
+ * @handle: output handler to a trace file.
+ *
+ * This API must be called after all tracecmd_output_set_...() APIs and before writing anything
+ * to the trace file. This initial information is written in the file:
+ *  - initial file magic bytes
+ *  - file version
+ *  - data endian
+ *  - long size
+ *  - page size
+ *  - compression header
+ *
+ * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
+ */
+int tracecmd_output_write_init(struct tracecmd_output *handler)
 {
-	struct tracecmd_output *handle;
-	int fd;
+	char buf[BUFSIZ];
+	int endian4;
 
-	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
-	if (fd < 0)
-		return NULL;
+	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
 
-	handle = create_file_fd(fd, ihandle, tracing_dir, kallsyms, list, NULL);
-	if (!handle) {
-		close(fd);
-		unlink(output_file);
-	}
+	buf[0] = 23;
+	buf[1] = 8;
+	buf[2] = 68;
+	memcpy(buf + 3, "tracing", 7);
 
-	return handle;
+	if (do_write_check(handler, buf, 10))
+		return -1;
+
+	sprintf(buf, "%lu", handler->file_version);
+	if (do_write_check(handler, buf, strlen(buf) + 1))
+		return -1;
+
+	if (handler->big_endian)
+		buf[0] = 1;
+	else
+		buf[0] = 0;
+	if (do_write_check(handler, buf, 1))
+		return -1;
+
+	/* save size of long (this may not be what the kernel is) */
+	buf[0] = sizeof(long);
+	if (do_write_check(handler, buf, 1))
+		return -1;
+
+	endian4 = convert_endian_4(handler, handler->page_size);
+	if (do_write_check(handler, &endian4, 4))
+		return -1;
+	handler->file_state = TRACECMD_FILE_INIT;
+	return 0;
+}
+
+/**
+ * tracecmd_output_write_headers - Write the trace file headers
+ * @handle: output handler to a trace file.
+ * @list: desired events that will be included in the trace file.
+ *	  It can be NULL for all available events
+ *
+ * These headers are written in the file:
+ *  - header files from the tracing directory
+ *  - ftrace events from the tracing directory
+ *  - event file from the tracing directory - all or only the one from @list
+ *  - kernel symbols from the tracing directory
+ *  - kernel printk strings from the tracing directory
+ *
+ * Returns 0 on success, or -1 in case of an error.
+ */
+int tracecmd_output_write_headers(struct tracecmd_output *handler,
+				  struct tracecmd_event_list *list)
+{
+	if (!handler || handler->file_state < TRACECMD_FILE_ALLOCATED)
+		return -1;
+
+	/* Write init data, if not written yet */
+	if (handler->file_state < TRACECMD_FILE_INIT && tracecmd_output_write_init(handler))
+		return -1;
+	if (read_header_files(handler))
+		return -1;
+	if (read_ftrace_files(handler))
+		return -1;
+	if (read_event_files(handler, list))
+		return -1;
+	if (read_proc_kallsyms(handler))
+		return -1;
+	if (read_ftrace_printk(handler))
+		return -1;
+	return 0;
 }
 
 /**
@@ -1267,11 +1374,19 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 {
 	struct tracecmd_output *handle;
 	char *path;
+	int fd;
 
-	handle = create_file(output_file, NULL, NULL, NULL, &all_event_list);
-	if (!handle)
+	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
 		return NULL;
 
+	handle = tracecmd_output_allocate(fd);
+	if (!handle)
+		goto out_free;
+	if (tracecmd_output_write_init(handle))
+		goto out_free;
+	if (tracecmd_output_write_headers(handle, NULL))
+		goto out_free;
 	/*
 	 * Save the command lines;
 	 */
@@ -1565,39 +1680,38 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
 
 struct tracecmd_output *tracecmd_create_init_fd(int fd)
 {
-	return create_file_fd(fd, NULL, NULL, NULL, &all_event_list, NULL);
-}
-
-struct tracecmd_output *
-tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
-			    struct tracecmd_event_list *list)
-{
-	return create_file_fd(msg_handle->fd, NULL, NULL, NULL, list, msg_handle);
-}
+	struct tracecmd_output *out;
 
-struct tracecmd_output *
-tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list)
-{
-	return create_file_fd(fd, NULL, NULL, NULL, list, NULL);
-}
-
-struct tracecmd_output *
-tracecmd_create_init_file_glob(const char *output_file,
-			       struct tracecmd_event_list *list)
-{
-	return create_file(output_file, NULL, NULL, NULL, list);
+	out = tracecmd_output_allocate(fd);
+	if (!out)
+		return NULL;
+	if (tracecmd_output_write_init(out))
+		goto error;
+	if (tracecmd_output_write_headers(out, NULL))
+		goto error;
+
+	return out;
+error:
+	tracecmd_output_close(out);
+	return NULL;
 }
 
 struct tracecmd_output *tracecmd_create_init_file(const char *output_file)
 {
-	return create_file(output_file, NULL, NULL, NULL, &all_event_list);
-}
+	struct tracecmd_output *handle;
+	int fd;
 
-struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
-							   const char *tracing_dir,
-							   const char *kallsyms)
-{
-	return create_file(output_file, NULL, tracing_dir, kallsyms, &all_event_list);
+	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
+		return NULL;
+	handle = tracecmd_create_init_fd(fd);
+	if (!handle) {
+		close(fd);
+		unlink(output_file);
+		return NULL;
+	}
+
+	return handle;
 }
 
 /**
@@ -1613,11 +1727,19 @@ struct tracecmd_output *tracecmd_copy(struct tracecmd_input *ihandle,
 				      const char *file)
 {
 	struct tracecmd_output *handle;
+	int fd;
 
-	handle = create_file(file, ihandle, NULL, NULL, &all_event_list);
-	if (!handle)
+	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
 		return NULL;
 
+	handle = tracecmd_output_allocate(fd);
+	if (!handle)
+		goto out_free;
+	if (tracecmd_output_set_from_input(handle, ihandle))
+		goto out_free;
+	tracecmd_output_write_init(handle);
+
 	if (tracecmd_copy_headers(ihandle, handle->fd, 0, 0) < 0)
 		goto out_free;
 
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 1767a6c6..15e07cf0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3689,6 +3689,25 @@ again:
 
 static void add_options(struct tracecmd_output *handle, struct common_record_context *ctx);
 
+static struct tracecmd_output *create_net_output(struct common_record_context *ctx,
+						 struct tracecmd_msg_handle *msg_handle)
+{
+	struct tracecmd_output *out;
+
+	out = tracecmd_output_allocate(-1);
+	if (!out)
+		return NULL;
+	if (tracecmd_output_set_msg(out, msg_handle))
+		goto error;
+	if (tracecmd_output_write_headers(out, listed_events))
+		goto error;
+
+	return out;
+error:
+	tracecmd_output_close(out);
+	return NULL;
+}
+
 static struct tracecmd_msg_handle *
 setup_connection(struct buffer_instance *instance, struct common_record_context *ctx)
 {
@@ -3700,7 +3719,7 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
 
 	/* Now create the handle through this socket */
 	if (msg_handle->version == V3_PROTOCOL) {
-		network_handle = tracecmd_create_init_fd_msg(msg_handle, listed_events);
+		network_handle = create_net_output(ctx, msg_handle);
 		if (!network_handle)
 			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
@@ -3718,10 +3737,11 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
 		if (ret)
 			goto error;
 	} else {
-		network_handle = tracecmd_create_init_fd_glob(msg_handle->fd,
-							      listed_events);
+		network_handle = tracecmd_output_allocate(msg_handle->fd);
 		if (!network_handle)
 			goto error;
+		if (tracecmd_output_write_headers(network_handle, listed_events))
+			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
 	}
 
@@ -4067,8 +4087,7 @@ static void setup_agent(struct buffer_instance *instance,
 {
 	struct tracecmd_output *network_handle;
 
-	network_handle = tracecmd_create_init_fd_msg(instance->msg_handle,
-						     listed_events);
+	network_handle = create_net_output(ctx, instance->msg_handle);
 	add_options(network_handle, ctx);
 	tracecmd_write_cmdlines(network_handle);
 	tracecmd_write_cpus(network_handle, instance->cpu_count);
@@ -4437,6 +4456,30 @@ static void write_guest_file(struct buffer_instance *instance)
 	free(temp_files);
 }
 
+static struct tracecmd_output *create_output(struct common_record_context *ctx)
+{
+	struct tracecmd_output *out;
+	int fd;
+
+	fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
+		return NULL;
+
+	out = tracecmd_output_allocate(fd);
+	if (!out)
+		goto error;
+	if (tracecmd_output_write_headers(out, listed_events))
+		goto error;
+	return out;
+error:
+	if (out)
+		tracecmd_output_close(out);
+	else
+		close(fd);
+	unlink(ctx->output);
+	return NULL;
+}
+
 static void record_data(struct common_record_context *ctx)
 {
 	struct tracecmd_option **buffer_options;
@@ -4491,7 +4534,7 @@ static void record_data(struct common_record_context *ctx)
 				touch_file(temp_files[i]);
 		}
 
-		handle = tracecmd_create_init_file_glob(ctx->output, listed_events);
+		handle = create_output(ctx);
 		if (!handle)
 			die("Error creating output file");
 		tracecmd_set_quiet(handle, quiet);
diff --git a/tracecmd/trace-restore.c b/tracecmd/trace-restore.c
index 280a37f0..8d2fcae8 100644
--- a/tracecmd/trace-restore.c
+++ b/tracecmd/trace-restore.c
@@ -22,6 +22,35 @@
 
 #include "trace-local.h"
 
+static struct tracecmd_output *create_output(const char *file,
+					     const char *tracing_dir, const char *kallsyms)
+{
+	struct tracecmd_output *out;
+	int fd;
+
+	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
+		return NULL;
+
+	out = tracecmd_output_allocate(fd);
+	if (!out)
+		goto error;
+	if (tracing_dir && tracecmd_output_set_trace_dir(out, tracing_dir))
+		goto error;
+	if (kallsyms && tracecmd_output_set_kallsyms(out, kallsyms))
+		goto error;
+	if (tracecmd_output_write_headers(out, NULL))
+		goto error;
+	return out;
+error:
+	if (out)
+		tracecmd_output_close(out);
+	else
+		close(fd);
+	unlink(file);
+	return NULL;
+}
+
 void trace_restore (int argc, char **argv)
 {
 	struct tracecmd_output *handle;
@@ -90,8 +119,7 @@ void trace_restore (int argc, char **argv)
 			usage(argv);
 		}
 
-		handle = tracecmd_create_init_file_override(output, tracing_dir,
-							    kallsyms);
+		handle = create_output(output, tracing_dir, kallsyms);
 		if (!handle)
 			die("Unabled to create output file %s", output);
 		if (tracecmd_write_cmdlines(handle) < 0)
-- 
2.31.1

[PATCH v3 20/21] trace-cmd library: Refactor the logic for writing trace data in the file

[Tzvetomir Stoyanov (VMware)]

When a trace buffer data are written in the trace file, the buffer
option in the file metadata is updated with the file offset of the
tracing data. Hide this logic into the trace-cmd library.
Added new APIs:
 tracecmd_add_buffer_info()
 tracecmd_write_buffer_info()
Changed APIs:
 tracecmd_append_buffer_cpu_data()
Removed APIs:
 tracecmd_add_buffer_option()

Refactored the internal logic of tracecmd_write_cpu_data() API to be
suitable for upcoming trace file format changes and data compression.
The size and the offset of the trace data is saved in the file right
after the data is written. The old logic calculates the size and offset
in advance, but when the trace data is compressed it is hard to use
that approach.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  10 +-
 lib/trace-cmd/include/trace-cmd-local.h       |  17 +
 lib/trace-cmd/trace-output.c                  | 307 ++++++++++++------
 tracecmd/trace-listen.c                       |   2 +-
 tracecmd/trace-record.c                       |  18 +-
 5 files changed, 235 insertions(+), 119 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 167f3804..97cd82f8 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -290,8 +290,8 @@ struct tracecmd_option *
 tracecmd_add_option_v(struct tracecmd_output *handle,
 		      unsigned short id, const struct iovec *vector, int count);
 
-struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle,
-						   const char *name, int cpus);
+int tracecmd_add_buffer_info(struct tracecmd_output *handle, const char *name, int cpus);
+int tracecmd_write_buffer_info(struct tracecmd_output *handle);
 
 int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus);
 int tracecmd_write_cmdlines(struct tracecmd_output *handle);
@@ -303,13 +303,11 @@ struct tracecmd_output *tracecmd_copy(struct tracecmd_input *ihandle,
 				      const char *file);
 
 int tracecmd_write_cpu_data(struct tracecmd_output *handle,
-			    int cpus, char * const *cpu_data_files);
+			    int cpus, char * const *cpu_data_files, const char *buff_name);
 int tracecmd_append_cpu_data(struct tracecmd_output *handle,
 			     int cpus, char * const *cpu_data_files);
 int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
-				    struct tracecmd_option *option,
-				    int cpus, char * const *cpu_data_files);
-
+				    const char *name, int cpus, char * const *cpu_data_files);
 struct tracecmd_output *tracecmd_get_output_handle_fd(int fd);
 
 /* --- Reading the Fly Recorder Trace --- */
diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index 7f3b45a8..547ee5c3 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -31,7 +31,24 @@ void tracecmd_info(const char *fmt, ...);
 #endif
 #endif
 
+struct data_file_write {
+	unsigned long long	file_size;
+	unsigned long long	write_size;
+	unsigned long long	soffset;
+	unsigned long long	data_offset;
+	unsigned long long	doffset;
+};
+
 bool check_file_state(unsigned long file_version, int current_state, int new_state);
 bool check_out_state(struct tracecmd_output *handle, int new_state);
 
+struct cpu_data_source {
+	int fd;
+	int size;
+	off64_t offset;
+};
+
+int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
+		       struct cpu_data_source *data, const char *buff_name);
+
 #endif /* _TRACE_CMD_LOCAL_H */
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 20116d99..c1bfcb66 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -39,6 +39,14 @@ struct tracecmd_option {
 	struct list_head list;
 };
 
+struct tracecmd_buffer {
+	int				cpus;
+	void				*name;
+	tsize_t				offset;
+	struct tracecmd_option		*option;
+	struct list_head		list;
+};
+
 enum {
 	OUTPUT_FL_SEND_META	= (1 << 0),
 };
@@ -59,6 +67,7 @@ struct tracecmd_output {
 	bool			big_endian;
 
 	struct list_head	options;
+	struct list_head	buffers;
 	struct tracecmd_msg_handle *msg_handle;
 	char			*trace_clock;
 };
@@ -141,6 +150,7 @@ bool tracecmd_get_quiet(struct tracecmd_output *handle)
 void tracecmd_output_free(struct tracecmd_output *handle)
 {
 	struct tracecmd_option *option;
+	struct tracecmd_buffer *buffer;
 
 	if (!handle)
 		return;
@@ -151,6 +161,13 @@ void tracecmd_output_free(struct tracecmd_output *handle)
 	if (handle->pevent)
 		tep_unref(handle->pevent);
 
+	while (!list_empty(&handle->buffers)) {
+		buffer = container_of(handle->buffers.next,
+				      struct tracecmd_buffer, list);
+		list_del(&buffer->list);
+		free(buffer->name);
+		free(buffer);
+	}
 	while (!list_empty(&handle->options)) {
 		option = container_of(handle->options.next,
 				      struct tracecmd_option, list);
@@ -912,6 +929,7 @@ struct tracecmd_output *tracecmd_output_allocate(int fd)
 		handle->big_endian = false;
 
 	list_head_init(&handle->options);
+	list_head_init(&handle->buffers);
 
 	handle->file_state = TRACECMD_FILE_ALLOCATED;
 
@@ -1324,15 +1342,14 @@ int tracecmd_append_options(struct tracecmd_output *handle)
 	return 0;
 }
 
-struct tracecmd_option *
-tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
-			   int cpus)
+static struct tracecmd_option *
+add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
 {
 	struct tracecmd_option *option;
 	char *buf;
 	int size = 8 + strlen(name) + 1;
 
-	buf = malloc(size);
+	buf = calloc(1, size);
 	if (!buf) {
 		tracecmd_warning("Failed to malloc buffer");
 		return NULL;
@@ -1354,6 +1371,52 @@ tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
 	return option;
 }
 
+int tracecmd_add_buffer_info(struct tracecmd_output *handle, const char *name, int cpus)
+{
+	struct tracecmd_buffer *buf;
+
+	buf = calloc(1, sizeof(struct tracecmd_buffer));
+	if (!buf)
+		return -1;
+	buf->name = strdup(name);
+	buf->cpus = cpus;
+	if (!buf->name) {
+		free(buf);
+		return -1;
+	}
+	list_add_tail(&buf->list, &handle->buffers);
+	return 0;
+}
+
+int tracecmd_write_buffer_info(struct tracecmd_output *handle)
+{
+	struct tracecmd_option *option;
+	struct tracecmd_buffer *buf;
+
+	list_for_each_entry(buf, &handle->buffers, list) {
+		option = add_buffer_option(handle, buf->name, buf->cpus);
+		if (!option)
+			return -1;
+		buf->option = option;
+	}
+
+	return 0;
+}
+
+static tsize_t get_buffer_file_offset(struct tracecmd_output *handle, const char *name)
+{
+	struct tracecmd_buffer *buf;
+
+	list_for_each_entry(buf, &handle->buffers, list) {
+		if (strlen(name) == strlen(buf->name) && !strcmp(name, buf->name)) {
+			if (!buf->option)
+				break;
+			return buf->option->offset;
+		}
+	}
+	return 0;
+}
+
 int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 {
 	int ret;
@@ -1446,6 +1509,37 @@ out:
 	return ret;
 }
 
+static int update_buffer_cpu_offset(struct tracecmd_output *handle,
+				    const char *name, tsize_t offset)
+{
+	tsize_t b_offset;
+	tsize_t current;
+
+	b_offset = get_buffer_file_offset(handle, name);
+	if (!b_offset) {
+		tracecmd_warning("Cannot find description for buffer %s\n", name);
+		return -1;
+	}
+	current = lseek64(handle->fd, 0, SEEK_CUR);
+
+	/* Go to the option data, where will write the offest */
+	if (lseek64(handle->fd, b_offset, SEEK_SET) == (off64_t)-1) {
+		tracecmd_warning("could not seek to %lld\n", b_offset);
+		return -1;
+	}
+
+	if (do_write_check(handle, &offset, 8))
+		return -1;
+
+	/* Go back to end of file */
+	if (lseek64(handle->fd, current, SEEK_SET) == (off64_t)-1) {
+		tracecmd_warning("could not seek to %lld\n", offset);
+		return -1;
+	}
+	return 0;
+}
+
+
 static char *get_clock(struct tracecmd_output *handle)
 {
 	struct tracefs_instance *inst;
@@ -1465,17 +1559,14 @@ static char *get_clock(struct tracecmd_output *handle)
 	return handle->trace_clock;
 }
 
-int tracecmd_write_cpu_data(struct tracecmd_output *handle,
-			    int cpus, char * const *cpu_data_files)
+__hidden int out_write_cpu_data(struct tracecmd_output *handle,
+				int cpus, struct cpu_data_source *data, const char *buff_name)
 {
-	off64_t *offsets = NULL;
-	unsigned long long *sizes = NULL;
-	off64_t offset;
+	struct data_file_write *data_files = NULL;
+	tsize_t data_offs, offset;
 	unsigned long long endian8;
-	char *clock = NULL;
-	off64_t check_size;
-	char *file;
-	struct stat st;
+	unsigned long long read_size;
+	char *clock;
 	int ret;
 	int i;
 
@@ -1490,97 +1581,133 @@ int tracecmd_write_cpu_data(struct tracecmd_output *handle,
 		goto out_free;
 	}
 
+	data_offs = lseek64(handle->fd, 0, SEEK_CUR);
 	if (do_write_check(handle, "flyrecord", 10))
 		goto out_free;
 
-	offsets = malloc(sizeof(*offsets) * cpus);
-	if (!offsets)
+	data_files = calloc(cpus, sizeof(struct data_file_write));
+	if (!data_files)
 		goto out_free;
-	sizes = malloc(sizeof(*sizes) * cpus);
-	if (!sizes)
-		goto out_free;
-
-	offset = lseek64(handle->fd, 0, SEEK_CUR);
-
-	/* hold any extra data for data */
-	offset += cpus * (16);
-
-	/*
-	 * Unfortunately, the trace_clock data was placed after the
-	 * cpu data, and wasn't accounted for with the offsets.
-	 * We need to save room for the trace_clock file. This means
-	 * we need to find the size of it before we define the final
-	 * offsets.
-	 */
-	clock = get_clock(handle);
-	if (!clock)
-		goto out_free;
-	/* Save room for storing the size */
-	offset += 8;
-	offset += strlen(clock);
-	/* 2 bytes for [] around the clock */
-	offset += 2;
-
-	/* Page align offset */
-	offset = (offset + (handle->page_size - 1)) & ~(handle->page_size - 1);
 
 	for (i = 0; i < cpus; i++) {
-		file = cpu_data_files[i];
-		ret = stat(file, &st);
-		if (ret < 0) {
-			tracecmd_warning("can not stat '%s'", file);
-			goto out_free;
-		}
-		offsets[i] = offset;
-		sizes[i] = st.st_size;
-		offset += st.st_size;
-		offset = (offset + (handle->page_size - 1)) & ~(handle->page_size - 1);
-
-		endian8 = convert_endian_8(handle, offsets[i]);
+		data_files[i].file_size = data[i].size;
+		/* Write 0 for trace data offset and size and store offsets of these fields */
+		endian8 = 0;
+		data_files[i].doffset = lseek64(handle->fd, 0, SEEK_CUR);
 		if (do_write_check(handle, &endian8, 8))
 			goto out_free;
-		endian8 = convert_endian_8(handle, sizes[i]);
+		data_files[i].soffset = lseek64(handle->fd, 0, SEEK_CUR);
 		if (do_write_check(handle, &endian8, 8))
 			goto out_free;
 	}
 
-	if (save_clock(handle, clock))
+	update_buffer_cpu_offset(handle, buff_name, data_offs);
+	clock = get_clock(handle);
+	if (clock && save_clock(handle, clock))
 		goto out_free;
-
 	for (i = 0; i < cpus; i++) {
+		data_files[i].data_offset = lseek64(handle->fd, 0, SEEK_CUR);
+		/* Page align offset */
+		data_files[i].data_offset = (data_files[i].data_offset + (handle->page_size - 1)) & ~(handle->page_size - 1);
+		data_files[i].data_offset = lseek64(handle->fd, data_files[i].data_offset, SEEK_SET);
+		if (data_files[i].data_offset == (off64_t)-1)
+			goto out_free;
 		if (!tracecmd_get_quiet(handle))
 			fprintf(stderr, "CPU%d data recorded at offset=0x%llx\n",
-				i, (unsigned long long) offsets[i]);
-		offset = lseek64(handle->fd, offsets[i], SEEK_SET);
-		if (offset == (off64_t)-1) {
-			tracecmd_warning("could not seek to %lld\n", offsets[i]);
-			goto out_free;
+				i, (unsigned long long) data_files[i].data_offset);
+		if (data[i].size) {
+			if (lseek64(data[i].fd, data[i].offset, SEEK_SET) == (off64_t)-1)
+				goto out_free;
+			read_size = copy_file_fd(handle, data[i].fd);
+			if (read_size != data_files[i].file_size) {
+				errno = EINVAL;
+				tracecmd_warning("did not match size of %lld to %lld",
+						 read_size, data_files[i].file_size);
+				goto out_free;
+			}
+			data_files[i].write_size = read_size;
+		} else {
+			data_files[i].write_size = 0;
 		}
-		check_size = copy_file(handle, cpu_data_files[i]);
-		if (check_size != sizes[i]) {
-			errno = EINVAL;
-			tracecmd_warning("did not match size of %lld to %lld",
-					 check_size, sizes[i]);
+
+		/* Write the real CPU data offset in the file */
+		if (lseek64(handle->fd, data_files[i].doffset, SEEK_SET) == (off64_t)-1)
+			goto out_free;
+		endian8 = convert_endian_8(handle, data_files[i].data_offset);
+		if (do_write_check(handle, &endian8, 8))
+			goto out_free;
+		/* Write the real CPU data size in the file */
+		if (lseek64(handle->fd, data_files[i].soffset, SEEK_SET) == (off64_t)-1)
+			goto out_free;
+		endian8 = convert_endian_8(handle, data_files[i].write_size);
+		if (do_write_check(handle, &endian8, 8))
+			goto out_free;
+		offset = data_files[i].data_offset + data_files[i].write_size;
+		if (lseek64(handle->fd, offset, SEEK_SET) == (off64_t)-1)
 			goto out_free;
-		}
 		if (!tracecmd_get_quiet(handle))
 			fprintf(stderr, "    %llu bytes in size\n",
-				(unsigned long long)check_size);
+				(unsigned long long)data_files[i].write_size);
 	}
 
-	free(offsets);
-	free(sizes);
+	if (lseek64(handle->fd, 0, SEEK_END) == (off64_t)-1)
+		goto out_free;
 
+	free(data_files);
 	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
 
 	return 0;
 
  out_free:
-	free(offsets);
-	free(sizes);
+	lseek64(handle->fd, 0, SEEK_END);
+	free(data_files);
 	return -1;
 }
 
+int tracecmd_write_cpu_data(struct tracecmd_output *handle,
+			    int cpus, char * const *cpu_data_files, const char *buff_name)
+{
+	struct cpu_data_source *data;
+	struct stat st;
+	int size = 0;
+	int ret;
+	int i;
+
+	data = calloc(cpus, sizeof(struct cpu_data_source));
+	if (!data)
+		return -1;
+	for (i = 0; i < cpus; i++)
+		data[i].fd = -1;
+	for (i = 0; i < cpus; i++) {
+		ret = stat(cpu_data_files[i], &st);
+		if (ret < 0) {
+			tracecmd_warning("can not stat '%s'", cpu_data_files[i]);
+			break;
+		}
+		data[i].fd = open(cpu_data_files[i], O_RDONLY);
+		if (data[i].fd < 0) {
+			tracecmd_warning("Can't read '%s'", data[i].fd);
+			break;
+		}
+
+		data[i].size = st.st_size;
+		data[i].offset = 0;
+		size += st.st_size;
+	}
+
+	if (i < cpus)
+		ret = -1;
+	else
+		ret = out_write_cpu_data(handle, cpus, data, buff_name);
+
+	for (i = 0; i < cpus; i++) {
+		if (data[i].fd >= 0)
+			close(data[i].fd);
+	}
+	free(data);
+	return ret;
+}
+
 int tracecmd_append_cpu_data(struct tracecmd_output *handle,
 			     int cpus, char * const *cpu_data_files)
 {
@@ -1589,41 +1716,20 @@ int tracecmd_append_cpu_data(struct tracecmd_output *handle,
 	ret = tracecmd_write_cpus(handle, cpus);
 	if (ret)
 		return ret;
-
+	ret = tracecmd_write_buffer_info(handle);
+	if (ret)
+		return ret;
 	ret = tracecmd_write_options(handle);
 	if (ret)
 		return ret;
 
-	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files);
+	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files, "");
 }
 
 int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
-				    struct tracecmd_option *option,
-				    int cpus, char * const *cpu_data_files)
+				    const char *name, int cpus, char * const *cpu_data_files)
 {
-	tsize_t offset;
-	stsize_t ret;
-
-	offset = lseek64(handle->fd, 0, SEEK_CUR);
-
-	/* Go to the option data, where will write the offest */
-	ret = lseek64(handle->fd, option->offset, SEEK_SET);
-	if (ret == (off64_t)-1) {
-		tracecmd_warning("could not seek to %lld\n", option->offset);
-		return -1;
-	}
-
-	if (do_write_check(handle, &offset, 8))
-		return -1;
-
-	/* Go back to end of file */
-	ret = lseek64(handle->fd, offset, SEEK_SET);
-	if (ret == (off64_t)-1) {
-		tracecmd_warning("could not seek to %lld\n", offset);
-		return -1;
-	}
-
-	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files);
+	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files, name);
 }
 
 struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
@@ -1667,6 +1773,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
 	handle->file_version = tracecmd_get_in_file_version(ihandle);
 	handle->options_start = tracecmd_get_options_offset(ihandle);
 	list_head_init(&handle->options);
+	list_head_init(&handle->buffers);
 
 	tracecmd_close(ihandle);
 
diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 0cb70b7d..d812145b 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -610,7 +610,7 @@ static int put_together_file(int cpus, int ofd, const char *node,
 		if (ret)
 			goto out;
 	}
-	ret = tracecmd_write_cpu_data(handle, cpus, temp_files);
+	ret = tracecmd_write_cpu_data(handle, cpus, temp_files, "");
 
 out:
 	tracecmd_output_close(handle);
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 15e07cf0..78b0f5ec 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -4169,7 +4169,6 @@ static void touch_file(const char *file)
 }
 
 static void append_buffer(struct tracecmd_output *handle,
-			  struct tracecmd_option *buffer_option,
 			  struct buffer_instance *instance,
 			  char **temp_files)
 {
@@ -4197,7 +4196,7 @@ static void append_buffer(struct tracecmd_output *handle,
 			touch_file(temp_files[i]);
 	}
 
-	tracecmd_append_buffer_cpu_data(handle, buffer_option,
+	tracecmd_append_buffer_cpu_data(handle, tracefs_instance_get_name(instance->tracefs),
 					cpu_count, temp_files);
 
 	for (i = 0; i < instance->cpu_count; i++) {
@@ -4447,7 +4446,7 @@ static void write_guest_file(struct buffer_instance *instance)
 			die("failed to allocate memory");
 	}
 
-	if (tracecmd_write_cpu_data(handle, cpu_count, temp_files) < 0)
+	if (tracecmd_write_cpu_data(handle, cpu_count, temp_files, "") < 0)
 		die("failed to write CPU data");
 	tracecmd_output_close(handle);
 
@@ -4482,7 +4481,6 @@ error:
 
 static void record_data(struct common_record_context *ctx)
 {
-	struct tracecmd_option **buffer_options;
 	struct tracecmd_output *handle;
 	struct buffer_instance *instance;
 	bool local = false;
@@ -4551,9 +4549,6 @@ static void record_data(struct common_record_context *ctx)
 		}
 
 		if (buffers) {
-			buffer_options = malloc(sizeof(*buffer_options) * buffers);
-			if (!buffer_options)
-				die("Failed to allocate buffer options");
 			i = 0;
 			for_each_instance(instance) {
 				int cpus = instance->cpu_count != local_cpu_count ?
@@ -4561,10 +4556,9 @@ static void record_data(struct common_record_context *ctx)
 
 				if (instance->msg_handle)
 					continue;
-
-				buffer_options[i++] = tracecmd_add_buffer_option(handle,
-										 tracefs_instance_get_name(instance->tracefs),
-										 cpus);
+				tracecmd_add_buffer_info(handle,
+							tracefs_instance_get_name(instance->tracefs),
+							cpus);
 				add_buffer_stat(handle, instance);
 			}
 		}
@@ -4599,7 +4593,7 @@ static void record_data(struct common_record_context *ctx)
 				if (instance->msg_handle)
 					continue;
 				print_stat(instance);
-				append_buffer(handle, buffer_options[i++], instance, temp_files);
+				append_buffer(handle, instance, temp_files);
 			}
 		}
 
-- 
2.31.1

[PATCH v3 21/21] trace-cmd report: Init the top trace instance earlier

[Tzvetomir Stoyanov (VMware)]

Moved initialization of the instance before call to the tracecmd_cpus()
API. This change is required for the upcoming trace file changes, where
CPU count is set when the trace data are initialized.

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index 02f75cb9..f7ffb89e 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -1215,6 +1215,11 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 	list_for_each_entry(handles, handle_list, list) {
 		int cpus;
 
+		if (!tracecmd_is_buffer_instance(handles->handle)) {
+			ret = tracecmd_init_data(handles->handle);
+			if (ret < 0)
+				die("failed to init data");
+		}
 		cpus = tracecmd_cpus(handles->handle);
 		handles->cpus = cpus;
 		handles->last_timestamp = calloc(cpus, sizeof(*handles->last_timestamp));
@@ -1225,9 +1230,6 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 		if (tracecmd_is_buffer_instance(handles->handle))
 			continue;
 
-		ret = tracecmd_init_data(handles->handle);
-		if (ret < 0)
-			die("failed to init data");
 		if (align_ts) {
 			ts = tracecmd_get_first_ts(handles->handle);
 			if (first || first_ts > ts)
-- 
2.31.1

Re: [PATCH v3 07/21] trace-cmd library: Do not use local variables when reading CPU stat option

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:18 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Using a local variable to read all CPUSTAT options assumes that all of
> them are in a single option section. While this is true for the current
> trace file version format, this assumption limits the design of a more
> flexible format with multiple options sections. Use input handler context
> instead of the local variable.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 3d7f1c48..d020cfae 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -138,6 +138,7 @@ struct tracecmd_input {
>  
>  	struct host_trace_info	host;
>  	double			ts2secs;
> +	unsigned int		cpustats_size;
>  	char *			cpustats;

The above places cpustats_size (4 bytes) between ts2secs (8 bytes) and
cpustats (8 bytes) on 64 bit machines. Thus, it creates a hole of 4 bytes
of padding.

Best to try to place 4 bytes integers in pairs, where they always take up 8
bytes.

Above this, there's:

	// 8 bytes
	unsigned long long	trace_id;

	// pair 1
	int			fd;
	int			long_size;

	// pair 2
	int			page_size;
	int			page_map_size;

	// pair 3
	int			cpus;
	int			ref;

	// pair 4
	int			nr_buffers;	/* buffer instances */
	bool			use_trace_clock;

	// pair 5
	bool			read_page;
	bool			use_pipe;

	// uneven set of 4 bytes
	int			file_version;

	// 8 bytes
	struct cpu_data 	*cpu_data;

You can move the 4 byte field after file_version to fill in that hole.

>  	char *			uname;
>  	char *			version;
> @@ -2658,7 +2659,6 @@ static int handle_options(struct tracecmd_input *handle)
>  	unsigned short option;
>  	unsigned int size;
>  	char *cpustats = NULL;
> -	unsigned int cpustats_size = 0;
>  	struct input_buffer_instance *buffer;
>  	struct hook_list *hook;
>  	char *buf;
> @@ -2738,12 +2738,16 @@ static int handle_options(struct tracecmd_input *handle)
>  			break;
>  		case TRACECMD_OPTION_CPUSTAT:
>  			buf[size-1] = '\n';
> -			cpustats = realloc(cpustats, cpustats_size + size + 1);
> -			if (!cpustats)
> -				return -ENOMEM;
> -			memcpy(cpustats + cpustats_size, buf, size);
> -			cpustats_size += size;
> -			cpustats[cpustats_size] = 0;
> +			cpustats = realloc(handle->cpustats,
> +					   handle->cpustats_size + size + 1);
> +			if (!cpustats) {
> +				ret = -ENOMEM;
> +				return ret;
> +			}

Changing:

			if (!cpustats)
				return -ENOMEM;

to
			if (!cpustats) {
				ret = -ENOMEM;
				return ret;
			}

Doesn't make sense, as you are just adding a useless use of a local
variable.

-- Steve


> +			memcpy(cpustats + handle->cpustats_size, buf, size);
> +			handle->cpustats_size += size;
> +			cpustats[handle->cpustats_size] = 0;
> +			handle->cpustats = cpustats;
>  			break;
>  		case TRACECMD_OPTION_BUFFER:
>  			/* A buffer instance is saved at the end of the file */
> @@ -2813,8 +2817,6 @@ static int handle_options(struct tracecmd_input *handle)
>  
>  	}
>  
> -	handle->cpustats = cpustats;
> -
>  	return 0;
>  }
>  

Re: [PATCH v3 08/21] trace-cmd library: Reuse within the library the function that checks file state.

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:19 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Make the function, that checks if the next file state is valid, global
> for the trace-cmd library, so it can be reused. It is important the same check
> logic to be used in the whole library.

FYI, Normally this would be broken into two patches.

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/include/trace-cmd-local.h |  2 +
>  lib/trace-cmd/trace-input.c             |  1 +
>  lib/trace-cmd/trace-output.c            | 66 ++++++++-----------------
>  lib/trace-cmd/trace-util.c              | 28 +++++++++++
>  4 files changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> index 821b5cdb..7f3b45a8 100644
> --- a/lib/trace-cmd/include/trace-cmd-local.h
> +++ b/lib/trace-cmd/include/trace-cmd-local.h
> @@ -31,5 +31,7 @@ void tracecmd_info(const char *fmt, ...);
>  #endif
>  #endif
>  
> +bool check_file_state(unsigned long file_version, int current_state, int new_state);
> +bool check_out_state(struct tracecmd_output *handle, int new_state);

The first just adding the check_out_state.



> @@ -1661,3 +1630,8 @@ out_free:
>  	tracecmd_output_close(handle);
>  	return NULL;
>  }
> +
> +__hidden bool check_out_state(struct tracecmd_output *handle, int new_state)
> +{
> +	return check_file_state(handle->file_version, handle->file_state, new_state);
> +}
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 9bc94822..db7ce7ee 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -624,3 +624,31 @@ bool tracecmd_is_version_supported(unsigned int version)
>  		return true;
>  	return false;
>  }

The second patch, adding the file_version parameter, as currently nothing
uses this, and it really isn't part of what this patch should do.

But I'll just take this as is for now.

-- Steve


> +
> +__hidden bool check_file_state(unsigned long file_version, int current_state, int new_state)
> +{
> +	switch (new_state) {
> +	case TRACECMD_FILE_HEADERS:
> +	case TRACECMD_FILE_FTRACE_EVENTS:
> +	case TRACECMD_FILE_ALL_EVENTS:
> +	case TRACECMD_FILE_KALLSYMS:
> +	case TRACECMD_FILE_PRINTK:
> +	case TRACECMD_FILE_CMD_LINES:
> +	case TRACECMD_FILE_CPU_COUNT:
> +		if (current_state == (new_state - 1))
> +			return true;
> +		break;
> +	case TRACECMD_FILE_OPTIONS:
> +		if (current_state == (new_state - 1))
> +			return true;
> +		break;
> +	case TRACECMD_FILE_CPU_LATENCY:
> +	case TRACECMD_FILE_CPU_FLYRECORD:
> +		if (current_state == TRACECMD_FILE_OPTIONS)
> +			return true;
> +		break;
> +	}
> +
> +	return false;
> +}
> +

Re: [PATCH v3 10/21] trace-cmd library: Fix possible memory leak in read_event_files()

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:21 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Some error paths in read_event_files() may lead to a memory leak.
> Improved the error handling of this internal function to avoid it.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 39 ++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 94fd8693..32358ce9 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -645,7 +645,7 @@ out:
>  static int read_event_files(struct tracecmd_input *handle, const char *regex)
>  {
>  	unsigned long long size;
> -	char *system;
> +	char *system = NULL;
>  	regex_t spreg;
>  	regex_t epreg;
>  	regex_t *sreg = NULL;
> @@ -670,13 +670,16 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
>  			return -1;
>  	}
>  
> -	if (read4(handle, &systems) < 0)
> -		return -1;
> +	ret = read4(handle, &systems);
> +	if (ret < 0)
> +		goto out;
>  
>  	for (i = 0; i < systems; i++) {
>  		system = read_string(handle);
> -		if (!system)
> -			return -1;
> +		if (!system) {
> +			ret = -1;
> +			goto out;
> +		}
>  
>  		sys_printed = 0;
>  		print_all = 0;
> @@ -703,39 +706,35 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
>  			}
>  		}
>  
> -		if (read4(handle, &count) < 0)
> -			goto failed;
> +		ret = read4(handle, &count);
> +		if (ret < 0)
> +			goto out;
>  
>  		for (x=0; x < count; x++) {
> -			if (read8(handle, &size) < 0)
> -				goto failed;
> +			ret = read8(handle, &size);
> +			if (ret < 0)
> +				goto out;
>  
>  			ret = read_event_file(handle, system, size,
>  					      print_all, &sys_printed,
>  					      reg);
>  			if (ret < 0)
> -				goto failed;
> +				goto out;
>  		}
>  		free(system);
> -	}
> -
> -	if (sreg) {
> -		regfree(sreg);
> -		regfree(ereg);
> +		system = NULL;
>  	}

Nit, I would have placed "system = NULL;" outside the loop as it gets set
first thing in the loop, but that's just my preference ;-)

Other than that, the patch looks good. No need to update it.

-- Steve


>  
>  	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
> -
> -	return 0;
> -
> - failed:
> +	ret = 0;
> + out:
>  	if (sreg) {
>  		regfree(sreg);
>  		regfree(ereg);
>  	}
>  
>  	free(system);
> -	return -1;
> +	return ret;
>  }
>  
>  static int read_proc_kallsyms(struct tracecmd_input *handle)

Re: [PATCH v3 11/21] trace-cmd library: Fix possible memory leak in read_proc_kallsyms()

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:22 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Some error paths in read_proc_kallsyms() may lead to a memory leak.
> Improved the error handling of this internal function to avoid it.

How so?

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 32358ce9..9b23063e 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -739,34 +739,39 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
>  
>  static int read_proc_kallsyms(struct tracecmd_input *handle)
>  {
> -	struct tep_handle *pevent = handle->pevent;
> +	struct tep_handle *tep = handle->pevent;
>  	unsigned int size;
> -	char *buf;
> +	char *buf = NULL;
> +	int ret;
>  
>  	if (handle->file_state >= TRACECMD_FILE_KALLSYMS)
>  		return 0;
>  
> -	if (read4(handle, &size) < 0)
> -		return -1;
> -	if (!size)
> -		return 0; /* OK? */
> +	ret = read4(handle, &size);
> +	if (ret < 0)
> +		goto out;
> +	if (!size) {
> +		handle->file_state = TRACECMD_FILE_KALLSYMS;
> +		goto out; /* OK? */
> +	}
>  

Allocation of buf happens below. There's no reason to jump to out. It
should just return here. Then you don't need to initialize buf.

>  	buf = malloc(size+1);
> -	if (!buf)
> -		return -1;
> -	if (do_read_check(handle, buf, size)){
> -		free(buf);

buf is freed here on the error path in the original code.

> -		return -1;
> +	if (!buf) {
> +		ret = -1;
> +		goto out;

The above doesn't even make sense. The only thing jumping to out does here
is to free buf, in which case, there is no buf to free.

>  	}
> -	buf[size] = 0;
> -
> -	tep_parse_kallsyms(pevent, buf);
> +	ret = do_read_check(handle, buf, size);
> +	if (ret < 0)
> +		goto out;
>  
> -	free(buf);
> +	buf[size] = 0;
> +	tep_parse_kallsyms(tep, buf);

If you want to make another patch to do the slight updates to state that
this patch does (but does not express in the change log), that's fine. But
as this patch stands, it does not do what the change log states, so I'm
removing this patch from the series.

-- Steve


>  
>  	handle->file_state = TRACECMD_FILE_KALLSYMS;
> -
> -	return 0;
> +	ret = 0;
> +out:
> +	free(buf);
> +	return ret;
>  }
>  
>  static int read_ftrace_printk(struct tracecmd_input *handle)

Re: [PATCH v3 12/21] trace-cmd library: Fix possible memory leak in read_ftrace_printk()

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:23 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Some error paths in read_ftrace_printk() may lead to a memory leak.
> Improved the error handling of this internal function to avoid it.

Again, I don't see a possible memory leak.

-- Steve

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 9b23063e..60d75d47 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -777,33 +777,39 @@ out:
>  static int read_ftrace_printk(struct tracecmd_input *handle)
>  {
>  	unsigned int size;
> -	char *buf;
> +	char *buf = NULL;
> +	int ret;
>  
>  	if (handle->file_state >= TRACECMD_FILE_PRINTK)
>  		return 0;
>  
> -	if (read4(handle, &size) < 0)
> -		return -1;
> -	if (!size)
> -		return 0; /* OK? */
> +	ret = read4(handle, &size);
> +	if (ret < 0)
> +		goto out;
> +	if (!size) {
> +		handle->file_state = TRACECMD_FILE_PRINTK;
> +		goto out; /* OK? */
> +	}
>  
>  	buf = malloc(size + 1);
> -	if (!buf)
> -		return -1;
> -	if (do_read_check(handle, buf, size)) {
> -		free(buf);
> -		return -1;
> +	if (!buf) {
> +		ret = -1;
> +		goto out;
>  	}
> +	ret = do_read_check(handle, buf, size);
> +	if (ret < 0)
> +		goto out;
>  
>  	buf[size] = 0;
>  
>  	tep_parse_printk_formats(handle->pevent, buf);
>  
> -	free(buf);
> -
>  	handle->file_state = TRACECMD_FILE_PRINTK;
> +	ret = 0;
>  
> -	return 0;
> +out:
> +	free(buf);
> +	return ret;
>  }
>  
>  static int read_and_parse_cmdlines(struct tracecmd_input *handle);

Re: [PATCH v3 13/21] trace-cmd library: Fix possible memory leak in read_and_parse_cmdlines()

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:24 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Some error paths in read_and_parse_cmdlines() may lead to a memory leak.
> Improved the error handling of this internal function to avoid it.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 60d75d47..9fd7e6e8 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -2999,20 +2999,26 @@ static int read_and_parse_cmdlines(struct tracecmd_input *handle)
>  {
>  	struct tep_handle *pevent = handle->pevent;
>  	unsigned long long size;
> -	char *cmdlines;
> +	char *cmdlines = NULL;
> +	int ret;
>  
>  	if (handle->file_state >= TRACECMD_FILE_CMD_LINES)
>  		return 0;
>  
> -	if (read_data_and_size(handle, &cmdlines, &size) < 0)
> -		return -1;
> +	ret = read_data_and_size(handle, &cmdlines, &size);

This function only returns an allocated cmdline on success. That was the
point of this function, was that you didn't have to check and free the
return value.

-- Steve


> +	if (ret < 0)
> +		goto out;
> +	if (!size) {
> +		handle->file_state = TRACECMD_FILE_CMD_LINES;
> +		goto out;
> +	}
>  	cmdlines[size] = 0;
>  	tep_parse_saved_cmdlines(pevent, cmdlines);
> -	free(cmdlines);
> -
>  	handle->file_state = TRACECMD_FILE_CMD_LINES;
> -
> -	return 0;
> +	ret = 0;
> +out:
> +	free(cmdlines);
> +	return ret;
>  }
>  
>  static void extract_trace_clock(struct tracecmd_input *handle, char *line)

Re: [PATCH v3 15/21] trace-cmd library: Track maximum CPUs count in input handler

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:26 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> This clean up is needed for the design of the next trace file version,
> where only CPUs with trace data could be stored in the file. Each trace
> instance may have its own CPU count, depending on collected traces.
> As the main input handler is used by the top trace instance, the
> CPU count there is for the top trace instance and may differ with cpu
> counts of the other instances. Added a new "max_cpu" member of the input
> handler, that tracks the maximum CPU count of all instances, recorded in
> the file.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index cb8b9f14..dc82d3ea 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -125,6 +125,7 @@ struct tracecmd_input {
>  	int			long_size;
>  	int			page_size;
>  	int			page_map_size;
> +	int			max_cpu;
>  	int			cpus;
>  	int			ref;
>  	int			nr_buffers;	/* buffer instances */
> @@ -838,6 +839,7 @@ static int read_cpus(struct tracecmd_input *handle)
>  		return -1;
>  
>  	handle->cpus = cpus;
> +	handle->max_cpu = cpus;
>  	tep_set_cpus(handle->pevent, handle->cpus);
>  	handle->file_state = TRACECMD_FILE_CPU_COUNT;
>  
> @@ -2794,6 +2796,8 @@ static int handle_options(struct tracecmd_input *handle)
>  		case TRACECMD_OPTION_CPUCOUNT:
>  			cpus = *(int *)buf;
>  			handle->cpus = tep_read_number(handle->pevent, &cpus, 4);
> +			handle->max_cpu = handle->cpus;

Shouldn't this be:

			if (handles->cpus > handle->max_cpu)
				handle->max_cpu = handle->cpus;

??

-- Steve

> +			tep_set_cpus(handle->pevent, handle->cpus);
>  			break;
>  		case TRACECMD_OPTION_PROCMAPS:
>  			if (buf[size-1] == '\0')
> @@ -4074,7 +4078,7 @@ int tracecmd_page_size(struct tracecmd_input *handle)
>   */
>  int tracecmd_cpus(struct tracecmd_input *handle)
>  {
> -	return handle->cpus;
> +	return handle->max_cpu;
>  }
>  
>  /**

Re: [PATCH v3 19/21] trace-cmd library: Refactor APIs for creating output handler

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:30 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> In the trace-cmd library there are various APIs for allocating and
> initializing output handler to a trace file. The existing APIs are use
> case oriented, with a lot of parameters. Extending them for new use
> cases, adding more input parameters, will make the library more complex
> and not easy to use.
> Almost all use case oriented APIs for output handler creation are
> removed and replaced with new flow, which is easier to be extended with
> new creation parameters.
> 
> Removed APIs:
>  tracecmd_create_init_fd_msg()
>  tracecmd_create_init_file_glob()
>  tracecmd_create_init_fd_glob()
>  tracecmd_create_init_file_override()
> 
> New APIs:
>  tracecmd_output_allocate()
>  tracecmd_output_set_msg()
>  tracecmd_output_set_trace_dir()
>  tracecmd_output_set_kallsyms()
>  tracecmd_output_set_from_input()
>  tracecmd_output_write_init()
>  tracecmd_output_write_headers()

So this patch is pretty much impossible to review and know if anything
broke, because it does many changes, and not just one. It's basically a
rewrite of a lot of code, and I can't tell what repercussions it may have.

That is, it needs to be broken up into at least two patches, maybe more.

> 
> The new tracecmd_output_allocate() API allocates memory and performs
> minimal initialization of an output handler to a trace file. No data
> is written in the file.
> The tracecmd_output_set_...() APIs can be used to set various
> parameters to the newly allocated output handler, that affect the way
> the data is written into the file.
> When the output handler is configured for the desired use case, the
> tracecmd_output_write_init() is used to start writing to the file, it
> writes initial magic bytes.
> The tracecmd_output_write_headers() API is used to write the initial
> headers into the file.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  .../include/private/trace-cmd-private.h       |  25 +-
>  lib/trace-cmd/trace-output.c                  | 406 ++++++++++++------
>  tracecmd/trace-record.c                       |  55 ++-
>  tracecmd/trace-restore.c                      |  32 +-
>  4 files changed, 356 insertions(+), 162 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index c58b09e9..167f3804 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -102,7 +102,8 @@ static inline int tracecmd_host_bigendian(void)
>  /* --- Opening and Reading the trace.dat file --- */
>  
>  enum tracecmd_file_states {
> -	TRACECMD_FILE_INIT = 1,
> +	TRACECMD_FILE_ALLOCATED = 0,
> +	TRACECMD_FILE_INIT,
>  	TRACECMD_FILE_HEADERS,
>  	TRACECMD_FILE_FTRACE_EVENTS,
>  	TRACECMD_FILE_ALL_EVENTS,
> @@ -268,20 +269,20 @@ struct tracecmd_event_list {
>  struct tracecmd_option;
>  struct tracecmd_msg_handle;
>  
> +struct tracecmd_output *tracecmd_output_allocate(int fd);
> +int tracecmd_output_set_msg(struct tracecmd_output *handler,
> +			    struct tracecmd_msg_handle *msg_handle);
> +int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir);
> +int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms);
> +int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle);
> +int tracecmd_output_write_init(struct tracecmd_output *handler);
> +int tracecmd_output_write_headers(struct tracecmd_output *handler,
> +				  struct tracecmd_event_list *list);
> +
>  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus);
> -struct tracecmd_output *
> -tracecmd_create_init_file_glob(const char *output_file,
> -			       struct tracecmd_event_list *list);
>  struct tracecmd_output *tracecmd_create_init_fd(int fd);
> -struct tracecmd_output *
> -tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list);
> -struct tracecmd_output *
> -tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
> -			    struct tracecmd_event_list *list);
> +
>  struct tracecmd_output *tracecmd_create_init_file(const char *output_file);
> -struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
> -							   const char *tracing_dir,
> -							   const char *kallsyms);
>  struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
>  					    unsigned short id, int size,
>  					    const void *data);
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index fe1d1aaa..20116d99 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -31,11 +31,6 @@
>  typedef unsigned long long	tsize_t;
>  typedef long long		stsize_t;
>  
> -static struct tracecmd_event_list all_event_list = {
> -	.next = NULL,
> -	.glob = "all"
> -};
> -
>  struct tracecmd_option {
>  	unsigned short	id;
>  	int		size;
> @@ -54,11 +49,14 @@ struct tracecmd_output {
>  	int			cpus;
>  	struct tep_handle	*pevent;
>  	char			*tracing_dir;
> +	char			*kallsyms;
> +	struct tracecmd_event_list *events;
>  	int			nr_options;
>  	bool			quiet;
>  	unsigned long		file_state;
>  	unsigned long		file_version;
>  	size_t			options_start;
> +	bool			big_endian;
>  
>  	struct list_head	options;
>  	struct tracecmd_msg_handle *msg_handle;
> @@ -682,7 +680,7 @@ static int read_event_files(struct tracecmd_output *handle,
>  			break;
>  	}
>  	/* all events are listed, use a global glob */
> -	if (list)
> +	if (!event_list || list)
>  		event_list = &all_events;
>  
>  	systems = create_event_list(handle, event_list);
> @@ -752,8 +750,7 @@ err:
>  		tracecmd_warning("can't set kptr_restrict");
>  }
>  
> -static int read_proc_kallsyms(struct tracecmd_output *handle,
> -			      const char *kallsyms)
> +static int read_proc_kallsyms(struct tracecmd_output *handle)
>  {
>  	unsigned int size, check_size, endian4;
>  	const char *path = "/proc/kallsyms";
> @@ -766,8 +763,8 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
>  		return -1;
>  	}
>  
> -	if (kallsyms)
> -		path = kallsyms;
> +	if (handle->kallsyms)
> +		path = handle->kallsyms;
>  
>  	ret = stat(path, &st);
>  	if (ret < 0) {
> @@ -883,137 +880,247 @@ out_free:
>  	return ret;
>  }
>  
> -static int select_file_version(struct tracecmd_output *handle,
> -				struct tracecmd_input *ihandle)
> -{
> -	if (ihandle)
> -		handle->file_version = tracecmd_get_in_file_version(ihandle);
> -	else
> -		handle->file_version = FILE_VERSION;
> -
> -	return 0;
> -}
> -
> -static struct tracecmd_output *
> -create_file_fd(int fd, struct tracecmd_input *ihandle,
> -	       const char *tracing_dir,
> -	       const char *kallsyms,
> -	       struct tracecmd_event_list *list,
> -	       struct tracecmd_msg_handle *msg_handle)

Instead of just removing this, add tarcecmd_output_alloct() that this uses
first.

> +/**
> + * tracecmd_output_allocate - allocate new output handler to a trace file
> + * @handle: file descriptor to an empty file, it can be -1 if the handler
> + *	    will not write to a file
> + *
> + * This API only allocates a handler and performs minimal initialization.
> + * No data is written in the file.
> + *
> + * Returns pointer to a newly allocated file descriptor, that can be used
> + * with other library APIs. In case of an error, NULL is returned. The returned
> + * handler must be freed with tracecmd_output_close() or tracecmd_output_free()
> + */
> +struct tracecmd_output *tracecmd_output_allocate(int fd)
>  {
>  	struct tracecmd_output *handle;
> -	struct tep_handle *pevent;
> -	char buf[BUFSIZ];
> -	int endian4;
>  
> -	handle = malloc(sizeof(*handle));
> +	handle = calloc(1, sizeof(*handle));
>  	if (!handle)
>  		return NULL;
> -	memset(handle, 0, sizeof(*handle));
> -
> -	list_head_init(&handle->options);
>  
>  	handle->fd = fd;
> -	if (tracing_dir) {
> -		handle->tracing_dir = strdup(tracing_dir);
> -		if (!handle->tracing_dir)
> -			goto out_free;
> -	}
>  
> -	handle->msg_handle = msg_handle;
> +	handle->file_version = FILE_VERSION;
>  
> -	if (select_file_version(handle, ihandle))
> -		goto out_free;
> +	handle->page_size = getpagesize();
>  
> -	buf[0] = 23;
> -	buf[1] = 8;
> -	buf[2] = 68;
> -	memcpy(buf + 3, "tracing", 7);
> +	if (tracecmd_host_bigendian())
> +		handle->big_endian = true;
> +	else
> +		handle->big_endian = false;

By the way, the above is the same as:

	handle->big_endian = tracecmd_host_bigendian();

>  
> -	if (do_write_check(handle, buf, 10))
> -		goto out_free;
> +	list_head_init(&handle->options);
>  
> -	sprintf(buf, "%lu", handle->file_version);
> -	if (do_write_check(handle, buf, strlen(buf) + 1))
> -		goto out_free;
> +	handle->file_state = TRACECMD_FILE_ALLOCATED;
>  
> -	/* get endian and page size */
> -	if (ihandle) {
> -		pevent = tracecmd_get_tep(ihandle);
> -		/* Use the pevent of the ihandle for later writes */
> -		handle->pevent = tracecmd_get_tep(ihandle);
> -		tep_ref(pevent);
> -		if (tep_is_file_bigendian(pevent))
> -			buf[0] = 1;
> -		else
> -			buf[0] = 0;
> -		handle->page_size = tracecmd_page_size(ihandle);
> -	} else {
> -		if (tracecmd_host_bigendian())
> -			buf[0] = 1;
> -		else
> -			buf[0] = 0;
> -		handle->page_size = getpagesize();
> -	}
> +	return handle;
> +}
>  
> -	if (do_write_check(handle, buf, 1))
> -		goto out_free;
> +/**
> + * tracecmd_output_set_msg - associated an output file handler with network message handler
> + * @handle: output handler to a trace file.
> + * @msg_handle: network handler, allocated by tracecmd_msg_handle_alloc()
> + *
> + * This API associates an output file handler with a network stream. All subsequent API calls
> + * with this output file handler will send data over the network using the @msg_handle, instead
> + * of writing to a file.
> + * This API must be called after the handler file version is set and before
> + * tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_msg(struct tracecmd_output *handler, struct tracecmd_msg_handle *msg_handle)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	/* save size of long (this may not be what the kernel is) */
> -	buf[0] = sizeof(long);
> -	if (do_write_check(handle, buf, 1))
> -		goto out_free;
> +	handler->msg_handle = msg_handle;
>  
> -	endian4 = convert_endian_4(handle, handle->page_size);
> -	if (do_write_check(handle, &endian4, 4))
> -		goto out_free;
> -	handle->file_state = TRACECMD_FILE_INIT;
> +	return 0;
> +}
>  
> -	if (ihandle)
> -		return handle;
> +/**
> + * tracecmd_output_set_trace_dir - Set a custom tracing dir, instead of system default
> + * @handle: output handler to a trace file.
> + * @tracing_dir: full path to a directory with tracing files
> + *
> + * This API associates an output file handler with a custom tracing directory, to be used when
> + * creating the trace file instead of the system default tracing directory.
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	if (read_header_files(handle))
> -		goto out_free;
> +	free(handler->tracing_dir);
> +	if (tracing_dir) {
> +		handler->tracing_dir = strdup(tracing_dir);
> +		if (!handler->tracing_dir)
> +			return -1;
> +	} else
> +		handler->tracing_dir = NULL;
>  
> -	if (read_ftrace_files(handle))
> -		goto out_free;
> +	return 0;
> +}
>  
> -	if (read_event_files(handle, list))
> -		goto out_free;
> +/**
> + * tracecmd_output_set_kallsyms - Set a custom kernel symbols file, instead of system default
> + * @handle: output handler to a trace file.
> + * @tracing_dir: full path to a file with kernel symbols
> + *
> + * This API associates an output file handler with a custom kernel symbols file, to be used when
> + * creating the trace file instead of the system default kernel symbols file.
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	if (read_proc_kallsyms(handle, kallsyms))
> -		goto out_free;
> +	free(handler->kallsyms);
> +	if (kallsyms) {
> +		handler->kallsyms = strdup(kallsyms);
> +		if (!handler->kallsyms)
> +			return -1;
> +	} else
> +		handler->kallsyms = NULL;
>  
> -	if (read_ftrace_printk(handle))
> -		goto out_free;
> +	return 0;
> +}
>  
> -	return handle;
> +/**
> + * tracecmd_output_set_from_input - Inherit parameters from an existing trace file
> + * @handle: output handler to a trace file.
> + * @ihandle: input handler to an existing trace file.
> + *
> + * This API copies parameters from input handler @ihandle, associated with an existing trace file,
> + * to the output handler @handle, associated with file that is going to be created.
> + * These parameters are copied:
> + *  - tep handler
> + *  - page size
> + *  - file endian
> + *  - file version
> + *  - file compression protocol
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle)
> +{
> +	if (!handler || !ihandle || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> - out_free:
> -	tracecmd_output_close(handle);
> -	return NULL;
> +	/* get endian, page size, file version and compression */
> +	/* Use the pevent of the ihandle for later writes */
> +	handler->pevent = tracecmd_get_tep(ihandle);
> +	tep_ref(handler->pevent);
> +	handler->page_size = tracecmd_page_size(ihandle);
> +	handler->file_version = tracecmd_get_in_file_version(ihandle);
> +	if (tep_is_file_bigendian(handler->pevent))
> +		handler->big_endian = true;
> +	else
> +		handler->big_endian = false;
> +
> +
> +	return 0;
>  }
>  
> -static struct tracecmd_output *create_file(const char *output_file,
> -					   struct tracecmd_input *ihandle,
> -					   const char *tracing_dir,
> -					   const char *kallsyms,
> -					   struct tracecmd_event_list *list)
> +/**
> + * tracecmd_output_write_init - Write the initial magics in the trace file
> + * @handle: output handler to a trace file.
> + *
> + * This API must be called after all tracecmd_output_set_...() APIs and before writing anything
> + * to the trace file. This initial information is written in the file:
> + *  - initial file magic bytes
> + *  - file version
> + *  - data endian
> + *  - long size
> + *  - page size
> + *  - compression header
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_write_init(struct tracecmd_output *handler)
>  {
> -	struct tracecmd_output *handle;
> -	int fd;
> +	char buf[BUFSIZ];
> +	int endian4;
>  
> -	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> -	if (fd < 0)
> -		return NULL;
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	handle = create_file_fd(fd, ihandle, tracing_dir, kallsyms, list, NULL);
> -	if (!handle) {
> -		close(fd);
> -		unlink(output_file);
> -	}
> +	buf[0] = 23;
> +	buf[1] = 8;
> +	buf[2] = 68;
> +	memcpy(buf + 3, "tracing", 7);
>  
> -	return handle;
> +	if (do_write_check(handler, buf, 10))
> +		return -1;
> +
> +	sprintf(buf, "%lu", handler->file_version);
> +	if (do_write_check(handler, buf, strlen(buf) + 1))
> +		return -1;
> +
> +	if (handler->big_endian)
> +		buf[0] = 1;
> +	else
> +		buf[0] = 0;
> +	if (do_write_check(handler, buf, 1))
> +		return -1;
> +
> +	/* save size of long (this may not be what the kernel is) */
> +	buf[0] = sizeof(long);
> +	if (do_write_check(handler, buf, 1))
> +		return -1;
> +
> +	endian4 = convert_endian_4(handler, handler->page_size);
> +	if (do_write_check(handler, &endian4, 4))
> +		return -1;
> +	handler->file_state = TRACECMD_FILE_INIT;
> +	return 0;
> +}
> +
> +/**
> + * tracecmd_output_write_headers - Write the trace file headers
> + * @handle: output handler to a trace file.
> + * @list: desired events that will be included in the trace file.
> + *	  It can be NULL for all available events
> + *
> + * These headers are written in the file:
> + *  - header files from the tracing directory
> + *  - ftrace events from the tracing directory
> + *  - event file from the tracing directory - all or only the one from @list
> + *  - kernel symbols from the tracing directory
> + *  - kernel printk strings from the tracing directory
> + *
> + * Returns 0 on success, or -1 in case of an error.
> + */
> +int tracecmd_output_write_headers(struct tracecmd_output *handler,
> +				  struct tracecmd_event_list *list)
> +{
> +	if (!handler || handler->file_state < TRACECMD_FILE_ALLOCATED)
> +		return -1;
> +
> +	/* Write init data, if not written yet */
> +	if (handler->file_state < TRACECMD_FILE_INIT && tracecmd_output_write_init(handler))
> +		return -1;
> +	if (read_header_files(handler))
> +		return -1;
> +	if (read_ftrace_files(handler))
> +		return -1;
> +	if (read_event_files(handler, list))
> +		return -1;
> +	if (read_proc_kallsyms(handler))
> +		return -1;
> +	if (read_ftrace_printk(handler))
> +		return -1;
> +	return 0;
>  }
>  
>  /**
> @@ -1267,11 +1374,19 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
>  {
>  	struct tracecmd_output *handle;
>  	char *path;
> +	int fd;
>  
> -	handle = create_file(output_file, NULL, NULL, NULL, &all_event_list);
> -	if (!handle)
> +	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
>  		return NULL;
>  
> +	handle = tracecmd_output_allocate(fd);
> +	if (!handle)
> +		goto out_free;
> +	if (tracecmd_output_write_init(handle))
> +		goto out_free;
> +	if (tracecmd_output_write_headers(handle, NULL))
> +		goto out_free;
>  	/*
>  	 * Save the command lines;
>  	 */
> @@ -1565,39 +1680,38 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
>  
>  struct tracecmd_output *tracecmd_create_init_fd(int fd)
>  {
> -	return create_file_fd(fd, NULL, NULL, NULL, &all_event_list, NULL);
> -}
> -
> -struct tracecmd_output *
> -tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
> -			    struct tracecmd_event_list *list)
> -{
> -	return create_file_fd(msg_handle->fd, NULL, NULL, NULL, list, msg_handle);
> -}
> +	struct tracecmd_output *out;
>  
> -struct tracecmd_output *
> -tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list)
> -{
> -	return create_file_fd(fd, NULL, NULL, NULL, list, NULL);
> -}
> -
> -struct tracecmd_output *
> -tracecmd_create_init_file_glob(const char *output_file,
> -			       struct tracecmd_event_list *list)
> -{
> -	return create_file(output_file, NULL, NULL, NULL, list);
> +	out = tracecmd_output_allocate(fd);
> +	if (!out)
> +		return NULL;
> +	if (tracecmd_output_write_init(out))
> +		goto error;
> +	if (tracecmd_output_write_headers(out, NULL))
> +		goto error;
> +
> +	return out;
> +error:
> +	tracecmd_output_close(out);
> +	return NULL;
>  }
>  
>  struct tracecmd_output *tracecmd_create_init_file(const char *output_file)
>  {
> -	return create_file(output_file, NULL, NULL, NULL, &all_event_list);
> -}
> +	struct tracecmd_output *handle;
> +	int fd;
>  
> -struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
> -							   const char *tracing_dir,
> -							   const char *kallsyms)
> -{
> -	return create_file(output_file, NULL, tracing_dir, kallsyms, &all_event_list);
> +	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
> +		return NULL;
> +	handle = tracecmd_create_init_fd(fd);
> +	if (!handle) {
> +		close(fd);
> +		unlink(output_file);
> +		return NULL;
> +	}
> +
> +	return handle;
>  }
>  
>  /**
> @@ -1613,11 +1727,19 @@ struct tracecmd_output *tracecmd_copy(struct tracecmd_input *ihandle,
>  				      const char *file)
>  {
>  	struct tracecmd_output *handle;
> +	int fd;
>  
> -	handle = create_file(file, ihandle, NULL, NULL, &all_event_list);
> -	if (!handle)
> +	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
>  		return NULL;
>  
> +	handle = tracecmd_output_allocate(fd);
> +	if (!handle)
> +		goto out_free;
> +	if (tracecmd_output_set_from_input(handle, ihandle))
> +		goto out_free;
> +	tracecmd_output_write_init(handle);
> +
>  	if (tracecmd_copy_headers(ihandle, handle->fd, 0, 0) < 0)
>  		goto out_free;
>  
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 1767a6c6..15e07cf0 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -3689,6 +3689,25 @@ again:
>  
>  static void add_options(struct tracecmd_output *handle, struct common_record_context *ctx);
>  
> +static struct tracecmd_output *create_net_output(struct common_record_context *ctx,
> +						 struct tracecmd_msg_handle *msg_handle)
> +{
> +	struct tracecmd_output *out;
> +
> +	out = tracecmd_output_allocate(-1);
> +	if (!out)
> +		return NULL;
> +	if (tracecmd_output_set_msg(out, msg_handle))
> +		goto error;
> +	if (tracecmd_output_write_headers(out, listed_events))
> +		goto error;
> +
> +	return out;
> +error:
> +	tracecmd_output_close(out);
> +	return NULL;
> +}
> +
>  static struct tracecmd_msg_handle *
>  setup_connection(struct buffer_instance *instance, struct common_record_context *ctx)
>  {
> @@ -3700,7 +3719,7 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
>  
>  	/* Now create the handle through this socket */
>  	if (msg_handle->version == V3_PROTOCOL) {
> -		network_handle = tracecmd_create_init_fd_msg(msg_handle, listed_events);
> +		network_handle = create_net_output(ctx, msg_handle);

So basically tracecmd_create_init_fd_msg() could had been converted first
to do what create_net_output() does above, and then we can replace them.


>  		if (!network_handle)
>  			goto error;
>  		tracecmd_set_quiet(network_handle, quiet);
> @@ -3718,10 +3737,11 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
>  		if (ret)
>  			goto error;
>  	} else {
> -		network_handle = tracecmd_create_init_fd_glob(msg_handle->fd,
> -							      listed_events);
> +		network_handle = tracecmd_output_allocate(msg_handle->fd);
>  		if (!network_handle)
>  			goto error;
> +		if (tracecmd_output_write_headers(network_handle, listed_events))
> +			goto error;
>  		tracecmd_set_quiet(network_handle, quiet);
>  	}
>  
> @@ -4067,8 +4087,7 @@ static void setup_agent(struct buffer_instance *instance,
>  {
>  	struct tracecmd_output *network_handle;
>  
> -	network_handle = tracecmd_create_init_fd_msg(instance->msg_handle,
> -						     listed_events);
> +	network_handle = create_net_output(ctx, instance->msg_handle);
>  	add_options(network_handle, ctx);
>  	tracecmd_write_cmdlines(network_handle);
>  	tracecmd_write_cpus(network_handle, instance->cpu_count);
> @@ -4437,6 +4456,30 @@ static void write_guest_file(struct buffer_instance *instance)
>  	free(temp_files);
>  }
>  
> +static struct tracecmd_output *create_output(struct common_record_context *ctx)
> +{
> +	struct tracecmd_output *out;
> +	int fd;
> +
> +	fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
> +		return NULL;
> +
> +	out = tracecmd_output_allocate(fd);
> +	if (!out)
> +		goto error;
> +	if (tracecmd_output_write_headers(out, listed_events))
> +		goto error;
> +	return out;
> +error:
> +	if (out)
> +		tracecmd_output_close(out);
> +	else
> +		close(fd);
> +	unlink(ctx->output);
> +	return NULL;
> +}
> +
>  static void record_data(struct common_record_context *ctx)
>  {
>  	struct tracecmd_option **buffer_options;
> @@ -4491,7 +4534,7 @@ static void record_data(struct common_record_context *ctx)
>  				touch_file(temp_files[i]);
>  		}
>  
> -		handle = tracecmd_create_init_file_glob(ctx->output, listed_events);
> +		handle = create_output(ctx);

Same here.

>  		if (!handle)
>  			die("Error creating output file");
>  		tracecmd_set_quiet(handle, quiet);
> diff --git a/tracecmd/trace-restore.c b/tracecmd/trace-restore.c
> index 280a37f0..8d2fcae8 100644
> --- a/tracecmd/trace-restore.c
> +++ b/tracecmd/trace-restore.c
> @@ -22,6 +22,35 @@
>  
>  #include "trace-local.h"
>  
> +static struct tracecmd_output *create_output(const char *file,
> +					     const char *tracing_dir, const char *kallsyms)
> +{
> +	struct tracecmd_output *out;
> +	int fd;
> +
> +	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
> +		return NULL;
> +
> +	out = tracecmd_output_allocate(fd);
> +	if (!out)
> +		goto error;
> +	if (tracing_dir && tracecmd_output_set_trace_dir(out, tracing_dir))
> +		goto error;
> +	if (kallsyms && tracecmd_output_set_kallsyms(out, kallsyms))
> +		goto error;
> +	if (tracecmd_output_write_headers(out, NULL))
> +		goto error;
> +	return out;
> +error:
> +	if (out)
> +		tracecmd_output_close(out);
> +	else
> +		close(fd);
> +	unlink(file);
> +	return NULL;
> +}
> +
>  void trace_restore (int argc, char **argv)
>  {
>  	struct tracecmd_output *handle;
> @@ -90,8 +119,7 @@ void trace_restore (int argc, char **argv)
>  			usage(argv);
>  		}
>  
> -		handle = tracecmd_create_init_file_override(output, tracing_dir,
> -							    kallsyms);
> +		handle = create_output(output, tracing_dir, kallsyms);

And here.

Basically, if you can introduce one API at a time, and have the old API use
the new one (maybe slightly modified), then it is possible to review this.
But as this patch is, I'll have to basically do as much work as you did to
create this patch for me to know if it is correct, and that's not the job of
a reviewer.

-- Steve


>  		if (!handle)
>  			die("Unabled to create output file %s", output);
>  		if (tracecmd_write_cmdlines(handle) < 0)

Re: [PATCH v3 20/21] trace-cmd library: Refactor the logic for writing trace data in the file

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:31 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> When a trace buffer data are written in the trace file, the buffer
> option in the file metadata is updated with the file offset of the
> tracing data. Hide this logic into the trace-cmd library.
> Added new APIs:
>  tracecmd_add_buffer_info()
>  tracecmd_write_buffer_info()
> Changed APIs:
>  tracecmd_append_buffer_cpu_data()
> Removed APIs:
>  tracecmd_add_buffer_option()

I'm thinking this patch can be broken up too.

> 
> Refactored the internal logic of tracecmd_write_cpu_data() API to be
> suitable for upcoming trace file format changes and data compression.
> The size and the offset of the trace data is saved in the file right
> after the data is written. The old logic calculates the size and offset
> in advance, but when the trace data is compressed it is hard to use
> that approach.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  .../include/private/trace-cmd-private.h       |  10 +-
>  lib/trace-cmd/include/trace-cmd-local.h       |  17 +
>  lib/trace-cmd/trace-output.c                  | 307 ++++++++++++------
>  tracecmd/trace-listen.c                       |   2 +-
>  tracecmd/trace-record.c                       |  18 +-
>  5 files changed, 235 insertions(+), 119 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 167f3804..97cd82f8 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -290,8 +290,8 @@ struct tracecmd_option *
>  tracecmd_add_option_v(struct tracecmd_output *handle,
>  		      unsigned short id, const struct iovec *vector, int count);
>  
> -struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle,
> -						   const char *name, int cpus);
> +int tracecmd_add_buffer_info(struct tracecmd_output *handle, const char *name, int cpus);
> +int tracecmd_write_buffer_info(struct tracecmd_output *handle);
>  
>  int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus);
>  int tracecmd_write_cmdlines(struct tracecmd_output *handle);
> @@ -303,13 +303,11 @@ struct tracecmd_output *tracecmd_copy(struct tracecmd_input *ihandle,
>  				      const char *file);
>  
>  int tracecmd_write_cpu_data(struct tracecmd_output *handle,
> -			    int cpus, char * const *cpu_data_files);
> +			    int cpus, char * const *cpu_data_files, const char *buff_name);
>  int tracecmd_append_cpu_data(struct tracecmd_output *handle,
>  			     int cpus, char * const *cpu_data_files);
>  int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
> -				    struct tracecmd_option *option,
> -				    int cpus, char * const *cpu_data_files);
> -
> +				    const char *name, int cpus, char * const *cpu_data_files);
>  struct tracecmd_output *tracecmd_get_output_handle_fd(int fd);
>  
>  /* --- Reading the Fly Recorder Trace --- */
> diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> index 7f3b45a8..547ee5c3 100644
> --- a/lib/trace-cmd/include/trace-cmd-local.h
> +++ b/lib/trace-cmd/include/trace-cmd-local.h
> @@ -31,7 +31,24 @@ void tracecmd_info(const char *fmt, ...);
>  #endif
>  #endif
>  
> +struct data_file_write {
> +	unsigned long long	file_size;
> +	unsigned long long	write_size;
> +	unsigned long long	soffset;
> +	unsigned long long	data_offset;
> +	unsigned long long	doffset;

Took me to look later in the code to know what "doffset" and "soffset"
is. Please keep the more verbose name, so that it is known without a
doubt what they are. "data_offset" and "size_offset" are perfectly
fine. Let's not get too cryptic, because that is what prevents people
from helping out.

> +};
> +
>  bool check_file_state(unsigned long file_version, int current_state, int new_state);
>  bool check_out_state(struct tracecmd_output *handle, int new_state);
>  
> +struct cpu_data_source {
> +	int fd;
> +	int size;
> +	off64_t offset;
> +};
> +
> +int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
> +		       struct cpu_data_source *data, const char *buff_name);
> +
>  #endif /* _TRACE_CMD_LOCAL_H */
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 20116d99..c1bfcb66 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -39,6 +39,14 @@ struct tracecmd_option {
>  	struct list_head list;
>  };
>  
> +struct tracecmd_buffer {
> +	int				cpus;
> +	void				*name;
> +	tsize_t				offset;
> +	struct tracecmd_option		*option;
> +	struct list_head		list;
> +};
> +
>  enum {
>  	OUTPUT_FL_SEND_META	= (1 << 0),
>  };
> @@ -59,6 +67,7 @@ struct tracecmd_output {
>  	bool			big_endian;
>  
>  	struct list_head	options;
> +	struct list_head	buffers;
>  	struct tracecmd_msg_handle *msg_handle;
>  	char			*trace_clock;
>  };
> @@ -141,6 +150,7 @@ bool tracecmd_get_quiet(struct tracecmd_output *handle)
>  void tracecmd_output_free(struct tracecmd_output *handle)
>  {
>  	struct tracecmd_option *option;
> +	struct tracecmd_buffer *buffer;
>  
>  	if (!handle)
>  		return;
> @@ -151,6 +161,13 @@ void tracecmd_output_free(struct tracecmd_output *handle)
>  	if (handle->pevent)
>  		tep_unref(handle->pevent);
>  
> +	while (!list_empty(&handle->buffers)) {
> +		buffer = container_of(handle->buffers.next,
> +				      struct tracecmd_buffer, list);
> +		list_del(&buffer->list);
> +		free(buffer->name);
> +		free(buffer);
> +	}

I'd suggest adding a space here, makes it easier to read.

>  	while (!list_empty(&handle->options)) {
>  		option = container_of(handle->options.next,
>  				      struct tracecmd_option, list);
> @@ -912,6 +929,7 @@ struct tracecmd_output *tracecmd_output_allocate(int fd)
>  		handle->big_endian = false;
>  
>  	list_head_init(&handle->options);
> +	list_head_init(&handle->buffers);
>  
>  	handle->file_state = TRACECMD_FILE_ALLOCATED;
>  
> @@ -1324,15 +1342,14 @@ int tracecmd_append_options(struct tracecmd_output *handle)
>  	return 0;
>  }
>  
> -struct tracecmd_option *
> -tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
> -			   int cpus)
> +static struct tracecmd_option *
> +add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
>  {
>  	struct tracecmd_option *option;
>  	char *buf;
>  	int size = 8 + strlen(name) + 1;
>  
> -	buf = malloc(size);
> +	buf = calloc(1, size);
>  	if (!buf) {
>  		tracecmd_warning("Failed to malloc buffer");
>  		return NULL;
> @@ -1354,6 +1371,52 @@ tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
>  	return option;
>  }
>  
> +int tracecmd_add_buffer_info(struct tracecmd_output *handle, const char *name, int cpus)
> +{
> +	struct tracecmd_buffer *buf;
> +
> +	buf = calloc(1, sizeof(struct tracecmd_buffer));
> +	if (!buf)
> +		return -1;
> +	buf->name = strdup(name);
> +	buf->cpus = cpus;
> +	if (!buf->name) {
> +		free(buf);
> +		return -1;
> +	}
> +	list_add_tail(&buf->list, &handle->buffers);
> +	return 0;
> +}
> +
> +int tracecmd_write_buffer_info(struct tracecmd_output *handle)
> +{
> +	struct tracecmd_option *option;
> +	struct tracecmd_buffer *buf;
> +
> +	list_for_each_entry(buf, &handle->buffers, list) {
> +		option = add_buffer_option(handle, buf->name, buf->cpus);
> +		if (!option)
> +			return -1;
> +		buf->option = option;
> +	}
> +
> +	return 0;
> +}
> +
> +static tsize_t get_buffer_file_offset(struct tracecmd_output *handle, const char *name)
> +{
> +	struct tracecmd_buffer *buf;
> +
> +	list_for_each_entry(buf, &handle->buffers, list) {
> +		if (strlen(name) == strlen(buf->name) && !strcmp(name, buf->name)) {

Why the two strlen()? That actually makes it slower, as it needs to
calculate the string lengths before the compare, which will fail on the
first miss compare. And strcmp() will not match if they are not the same
length.


> +			if (!buf->option)
> +				break;
> +			return buf->option->offset;
> +		}
> +	}
> +	return 0;
> +}
> +
>  int tracecmd_write_cmdlines(struct tracecmd_output *handle)
>  {
>  	int ret;
> @@ -1446,6 +1509,37 @@ out:
>  	return ret;
>  }
>  
> +static int update_buffer_cpu_offset(struct tracecmd_output *handle,
> +				    const char *name, tsize_t offset)
> +{
> +	tsize_t b_offset;
> +	tsize_t current;

Be easier if you added here:

	if (!name)
		name = "";

Then you just pass NULL in the other functions, and don't need to pass
this magical "" value.

> +
> +	b_offset = get_buffer_file_offset(handle, name);
> +	if (!b_offset) {
> +		tracecmd_warning("Cannot find description for buffer %s\n", name);
> +		return -1;
> +	}

Add space here.

> +	current = lseek64(handle->fd, 0, SEEK_CUR);
> +
> +	/* Go to the option data, where will write the offest */
> +	if (lseek64(handle->fd, b_offset, SEEK_SET) == (off64_t)-1) {
> +		tracecmd_warning("could not seek to %lld\n", b_offset);
> +		return -1;
> +	}
> +
> +	if (do_write_check(handle, &offset, 8))
> +		return -1;
> +
> +	/* Go back to end of file */
> +	if (lseek64(handle->fd, current, SEEK_SET) == (off64_t)-1) {
> +		tracecmd_warning("could not seek to %lld\n", offset);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +
>  static char *get_clock(struct tracecmd_output *handle)
>  {
>  	struct tracefs_instance *inst;
> @@ -1465,17 +1559,14 @@ static char *get_clock(struct tracecmd_output *handle)
>  	return handle->trace_clock;
>  }
>  
> -int tracecmd_write_cpu_data(struct tracecmd_output *handle,
> -			    int cpus, char * const *cpu_data_files)
> +__hidden int out_write_cpu_data(struct tracecmd_output *handle,
> +				int cpus, struct cpu_data_source *data, const char *buff_name)
>  {
> -	off64_t *offsets = NULL;
> -	unsigned long long *sizes = NULL;
> -	off64_t offset;
> +	struct data_file_write *data_files = NULL;
> +	tsize_t data_offs, offset;
>  	unsigned long long endian8;
> -	char *clock = NULL;
> -	off64_t check_size;
> -	char *file;
> -	struct stat st;
> +	unsigned long long read_size;
> +	char *clock;
>  	int ret;
>  	int i;
>  
> @@ -1490,97 +1581,133 @@ int tracecmd_write_cpu_data(struct tracecmd_output *handle,
>  		goto out_free;
>  	}
>  
> +	data_offs = lseek64(handle->fd, 0, SEEK_CUR);
>  	if (do_write_check(handle, "flyrecord", 10))
>  		goto out_free;
>  
> -	offsets = malloc(sizeof(*offsets) * cpus);
> -	if (!offsets)
> +	data_files = calloc(cpus, sizeof(struct data_file_write));

I always prefer: data_files = calloc(cpus, sizeof(*data_files));

instead, because, if you accidentally use the wrong structure, or it
changes in the future, it will allocate the wrong size.

> +	if (!data_files)
>  		goto out_free;
> -	sizes = malloc(sizeof(*sizes) * cpus);
> -	if (!sizes)
> -		goto out_free;
> -
> -	offset = lseek64(handle->fd, 0, SEEK_CUR);
> -
> -	/* hold any extra data for data */
> -	offset += cpus * (16);
> -
> -	/*
> -	 * Unfortunately, the trace_clock data was placed after the
> -	 * cpu data, and wasn't accounted for with the offsets.
> -	 * We need to save room for the trace_clock file. This means
> -	 * we need to find the size of it before we define the final
> -	 * offsets.
> -	 */
> -	clock = get_clock(handle);
> -	if (!clock)
> -		goto out_free;
> -	/* Save room for storing the size */
> -	offset += 8;
> -	offset += strlen(clock);
> -	/* 2 bytes for [] around the clock */
> -	offset += 2;
> -
> -	/* Page align offset */
> -	offset = (offset + (handle->page_size - 1)) & ~(handle->page_size - 1);
>  
>  	for (i = 0; i < cpus; i++) {
> -		file = cpu_data_files[i];
> -		ret = stat(file, &st);
> -		if (ret < 0) {
> -			tracecmd_warning("can not stat '%s'", file);
> -			goto out_free;
> -		}
> -		offsets[i] = offset;
> -		sizes[i] = st.st_size;
> -		offset += st.st_size;
> -		offset = (offset + (handle->page_size - 1)) & ~(handle->page_size - 1);
> -
> -		endian8 = convert_endian_8(handle, offsets[i]);
> +		data_files[i].file_size = data[i].size;
> +		/* Write 0 for trace data offset and size and store offsets of these fields */

The above comment is a bit confusing. Do you mean:

	" Place 0 for the data offset and size, and save the offsets to
	  updated them with the correct data later. " ?

> +		endian8 = 0;
> +		data_files[i].doffset = lseek64(handle->fd, 0, SEEK_CUR);
>  		if (do_write_check(handle, &endian8, 8))
>  			goto out_free;
> -		endian8 = convert_endian_8(handle, sizes[i]);
> +		data_files[i].soffset = lseek64(handle->fd, 0, SEEK_CUR);
>  		if (do_write_check(handle, &endian8, 8))
>  			goto out_free;
>  	}
>  
> -	if (save_clock(handle, clock))
> +	update_buffer_cpu_offset(handle, buff_name, data_offs);
> +	clock = get_clock(handle);
> +	if (clock && save_clock(handle, clock))
>  		goto out_free;
> -

Keep the space. There's too much code tightly together that needs
spacing, to read easier.


>  	for (i = 0; i < cpus; i++) {
> +		data_files[i].data_offset = lseek64(handle->fd, 0, SEEK_CUR);

Space.

> +		/* Page align offset */
> +		data_files[i].data_offset = (data_files[i].data_offset + (handle->page_size - 1)) & ~(handle->page_size - 1);

Either break the above into:
fse
		data_files[i].data_offset += handle->page_size - 1;
		data_files[i].data_offset &= !(handle->page_size - 1);

or add a macro called "ALIGN" (in a header, probably in a separate
patch).

#define ALIGN(val, alignment)   ((val) + ((alignment) - 1) & ~((alignment) - 1))

and then have:

		data_files[i].data_offset = ALIGN(data_files[i].data_offset, handle->page_size);


Add space.

> +		data_files[i].data_offset = lseek64(handle->fd, data_files[i].data_offset, SEEK_SET);

Why not just use "ret" since we are setting it, and the data_offset
shouldn't change.

> +		if (data_files[i].data_offset == (off64_t)-1)
> +			goto out_free;

Space.

>  		if (!tracecmd_get_quiet(handle))
>  			fprintf(stderr, "CPU%d data recorded at offset=0x%llx\n",
> -				i, (unsigned long long) offsets[i]);
> -		offset = lseek64(handle->fd, offsets[i], SEEK_SET);
> -		if (offset == (off64_t)-1) {
> -			tracecmd_warning("could not seek to %lld\n", offsets[i]);
> -			goto out_free;
> +				i, (unsigned long long) data_files[i].data_offset);
                                                       ^
No space between typecast and data.

space (line)

> +		if (data[i].size) {
> +			if (lseek64(data[i].fd, data[i].offset, SEEK_SET) == (off64_t)-1)
> +				goto out_free;
> +			read_size = copy_file_fd(handle, data[i].fd);
> +			if (read_size != data_files[i].file_size) {
> +				errno = EINVAL;
> +				tracecmd_warning("did not match size of %lld to %lld",
> +						 read_size, data_files[i].file_size);
> +				goto out_free;
> +			}
> +			data_files[i].write_size = read_size;
> +		} else {
> +			data_files[i].write_size = 0;
>  		}
> -		check_size = copy_file(handle, cpu_data_files[i]);
> -		if (check_size != sizes[i]) {
> -			errno = EINVAL;
> -			tracecmd_warning("did not match size of %lld to %lld",
> -					 check_size, sizes[i]);
> +
> +		/* Write the real CPU data offset in the file */
> +		if (lseek64(handle->fd, data_files[i].doffset, SEEK_SET) == (off64_t)-1)
> +			goto out_free;
> +		endian8 = convert_endian_8(handle, data_files[i].data_offset);
> +		if (do_write_check(handle, &endian8, 8))
> +			goto out_free;

space.

> +		/* Write the real CPU data size in the file */
> +		if (lseek64(handle->fd, data_files[i].soffset, SEEK_SET) == (off64_t)-1)
> +			goto out_free;
> +		endian8 = convert_endian_8(handle, data_files[i].write_size);
> +		if (do_write_check(handle, &endian8, 8))
> +			goto out_free;

space.

> +		offset = data_files[i].data_offset + data_files[i].write_size;
> +		if (lseek64(handle->fd, offset, SEEK_SET) == (off64_t)-1)
>  			goto out_free;
> -		}

space.

>  		if (!tracecmd_get_quiet(handle))
>  			fprintf(stderr, "    %llu bytes in size\n",
> -				(unsigned long long)check_size);
> +				(unsigned long long)data_files[i].write_size);
>  	}
>  
> -	free(offsets);
> -	free(sizes);
> +	if (lseek64(handle->fd, 0, SEEK_END) == (off64_t)-1)
> +		goto out_free;

If this failed here, what makes you think it will succeed later? ;-)

Could just move it to after the free(data_files) and return error if
this happened.

>  
> +	free(data_files);
>  	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
>  
>  	return 0;
>  
>   out_free:
> -	free(offsets);
> -	free(sizes);
> +	lseek64(handle->fd, 0, SEEK_END);
> +	free(data_files);
>  	return -1;
>  }
>  
> +int tracecmd_write_cpu_data(struct tracecmd_output *handle,
> +			    int cpus, char * const *cpu_data_files, const char *buff_name)
> +{
> +	struct cpu_data_source *data;
> +	struct stat st;
> +	int size = 0;
> +	int ret;
> +	int i;
> +
> +	data = calloc(cpus, sizeof(struct cpu_data_source));
> +	if (!data)
> +		return -1;

space.

> +	for (i = 0; i < cpus; i++)
> +		data[i].fd = -1;

space.

/* nit */
Although you don't need the above initialization (see below).

> +	for (i = 0; i < cpus; i++) {
> +		ret = stat(cpu_data_files[i], &st);
> +		if (ret < 0) {
> +			tracecmd_warning("can not stat '%s'", cpu_data_files[i]);
> +			break;
> +		}
> +		data[i].fd = open(cpu_data_files[i], O_RDONLY);
> +		if (data[i].fd < 0) {
> +			tracecmd_warning("Can't read '%s'", data[i].fd);
> +			break;
> +		}
> +
> +		data[i].size = st.st_size;
> +		data[i].offset = 0;
> +		size += st.st_size;
> +	}
> +
> +	if (i < cpus)
> +		ret = -1;
> +	else
> +		ret = out_write_cpu_data(handle, cpus, data, buff_name);
> +

No need to initialize if you have:

	for (i--; i >= 0; i--) {

> +	for (i = 0; i < cpus; i++) {
> +		if (data[i].fd >= 0)

You also don't need this check.

> +			close(data[i].fd);
> +	}
> +	free(data);
> +	return ret;
> +}
> +
>  int tracecmd_append_cpu_data(struct tracecmd_output *handle,
>  			     int cpus, char * const *cpu_data_files)
>  {
> @@ -1589,41 +1716,20 @@ int tracecmd_append_cpu_data(struct tracecmd_output *handle,
>  	ret = tracecmd_write_cpus(handle, cpus);
>  	if (ret)
>  		return ret;
> -
> +	ret = tracecmd_write_buffer_info(handle);
> +	if (ret)
> +		return ret;
>  	ret = tracecmd_write_options(handle);
>  	if (ret)
>  		return ret;
>  
> -	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files);
> +	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files, "");

Should pass NULL instead of "" (as explained above).

>  }
>  
>  int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
> -				    struct tracecmd_option *option,
> -				    int cpus, char * const *cpu_data_files)
> +				    const char *name, int cpus, char * const *cpu_data_files)
>  {
> -	tsize_t offset;
> -	stsize_t ret;
> -
> -	offset = lseek64(handle->fd, 0, SEEK_CUR);
> -
> -	/* Go to the option data, where will write the offest */
> -	ret = lseek64(handle->fd, option->offset, SEEK_SET);
> -	if (ret == (off64_t)-1) {
> -		tracecmd_warning("could not seek to %lld\n", option->offset);
> -		return -1;
> -	}
> -
> -	if (do_write_check(handle, &offset, 8))
> -		return -1;
> -
> -	/* Go back to end of file */
> -	ret = lseek64(handle->fd, offset, SEEK_SET);
> -	if (ret == (off64_t)-1) {
> -		tracecmd_warning("could not seek to %lld\n", offset);
> -		return -1;
> -	}
> -
> -	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files);
> +	return tracecmd_write_cpu_data(handle, cpus, cpu_data_files, name);
>  }
>  
>  struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
> @@ -1667,6 +1773,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
>  	handle->file_version = tracecmd_get_in_file_version(ihandle);
>  	handle->options_start = tracecmd_get_options_offset(ihandle);
>  	list_head_init(&handle->options);
> +	list_head_init(&handle->buffers);
>  
>  	tracecmd_close(ihandle);
>  
> diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
> index 0cb70b7d..d812145b 100644
> --- a/tracecmd/trace-listen.c
> +++ b/tracecmd/trace-listen.c
> @@ -610,7 +610,7 @@ static int put_together_file(int cpus, int ofd, const char *node,
>  		if (ret)
>  			goto out;
>  	}
> -	ret = tracecmd_write_cpu_data(handle, cpus, temp_files);
> +	ret = tracecmd_write_cpu_data(handle, cpus, temp_files, "");

Pass in NULL instead of "".

>  
>  out:
>  	tracecmd_output_close(handle);
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 15e07cf0..78b0f5ec 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -4169,7 +4169,6 @@ static void touch_file(const char *file)
>  }
>  
>  static void append_buffer(struct tracecmd_output *handle,
> -			  struct tracecmd_option *buffer_option,
>  			  struct buffer_instance *instance,
>  			  char **temp_files)
>  {
> @@ -4197,7 +4196,7 @@ static void append_buffer(struct tracecmd_output *handle,
>  			touch_file(temp_files[i]);
>  	}
>  
> -	tracecmd_append_buffer_cpu_data(handle, buffer_option,
> +	tracecmd_append_buffer_cpu_data(handle, tracefs_instance_get_name(instance->tracefs),
>  					cpu_count, temp_files);
>  
>  	for (i = 0; i < instance->cpu_count; i++) {
> @@ -4447,7 +4446,7 @@ static void write_guest_file(struct buffer_instance *instance)
>  			die("failed to allocate memory");
>  	}
>  
> -	if (tracecmd_write_cpu_data(handle, cpu_count, temp_files) < 0)
> +	if (tracecmd_write_cpu_data(handle, cpu_count, temp_files, "") < 0)

Pass NULL instead of "".

-- Steve


>  		die("failed to write CPU data");
>  	tracecmd_output_close(handle);
>  
> @@ -4482,7 +4481,6 @@ error:
>  
>  static void record_data(struct common_record_context *ctx)
>  {
> -	struct tracecmd_option **buffer_options;
>  	struct tracecmd_output *handle;
>  	struct buffer_instance *instance;
>  	bool local = false;
> @@ -4551,9 +4549,6 @@ static void record_data(struct common_record_context *ctx)
>  		}
>  
>  		if (buffers) {
> -			buffer_options = malloc(sizeof(*buffer_options) * buffers);
> -			if (!buffer_options)
> -				die("Failed to allocate buffer options");
>  			i = 0;
>  			for_each_instance(instance) {
>  				int cpus = instance->cpu_count != local_cpu_count ?
> @@ -4561,10 +4556,9 @@ static void record_data(struct common_record_context *ctx)
>  
>  				if (instance->msg_handle)
>  					continue;
> -
> -				buffer_options[i++] = tracecmd_add_buffer_option(handle,
> -										 tracefs_instance_get_name(instance->tracefs),
> -										 cpus);
> +				tracecmd_add_buffer_info(handle,
> +							tracefs_instance_get_name(instance->tracefs),
> +							cpus);
>  				add_buffer_stat(handle, instance);
>  			}
>  		}
> @@ -4599,7 +4593,7 @@ static void record_data(struct common_record_context *ctx)
>  				if (instance->msg_handle)
>  					continue;
>  				print_stat(instance);
> -				append_buffer(handle, buffer_options[i++], instance, temp_files);
> +				append_buffer(handle, instance, temp_files);
>  			}
>  		}
>  

Re: [PATCH v3 00/21] trace-cmd fixes and clean-ups

[Steven Rostedt]

On Tue, 14 Sep 2021 16:12:11 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Various trace-cmd application and library fixes, clean-ups and internal
> refactoring, needed for trace file version 7 and compression changes.
> 
> This patch-set supersedes "[v2,00/87] Trace file version 7":
>    https://lore.kernel.org/linux-trace-devel/20210729050959.12263-1-tz.stoyanov@gmail.com/
> 
> v3 changes:
>  - fixed issues of split and convert commands with some corner cases
> v2 changes:
>  - more cleanups, forgotten in the first version
> 
> Tzvetomir Stoyanov (VMware) (21):

I accepted all the patches but:

>   trace-cmd library: Do not use local variables when reading CPU stat
>     option
>   trace-cmd library: Fix possible memory leak in read_ftrace_files()
>   trace-cmd library: Fix possible memory leak in read_event_files()
>   trace-cmd library: Fix possible memory leak in read_proc_kallsyms()
>   trace-cmd library: Fix possible memory leak in read_ftrace_printk()
>   trace-cmd library: Fix possible memory leak in
>     read_and_parse_cmdlines()
>   trace-cmd library: Track maximum CPUs count in input handler
>   trace-cmd library: Refactor APIs for creating output handler
>   trace-cmd library: Refactor the logic for writing trace data in the
>     file

I'm going to run a quick test on them, and then push them upstream.

-- Steve