[PATCH v7 00/25] Trace file version 7 - sections

[PATCH v7 00/25] Trace file version 7 - sections

[Tzvetomir Stoyanov (VMware)]

Introduce the next version of the trace-cmd trace file - 7. Major changes
since version 6 is more flexible extendable structure of the file, based on
independent sections.

v7 changes:
 - Rebased on top of the latest master.
 - Introduced new metadata strings section.
 - Use 8 bytes for section size. 
v6 changes:
 - Rebased on top of the latest master.
 - On trace file v7 close, write any unsaved options in the file - as
   a last section.
 - Removed the patch "Do not write CPUs with empty trace data" from the
   patch set. Empty CPUs are written in v7 trace files, the same way as in
   v6.
v5 changes:
 - Rebased on top of the latest master.
v4 changes:
 - Rebased on top of the latest master.
v3 changes:
 - Fixed issues of split and convert commands with some corner cases.
v2 changes:
 - Code cleanups.

Tzvetomir Stoyanov (VMware) (25):
  trace-cmd library: Define trace file version 7
  trace-cmd library: Add cache functionality to network message handler
  trace-cmd library: New APIs to get and set version of output handler
  trace-cmd library: Add strings section in trace file version 7
  trace-cmd library: Add internal helper function for writing headers
    before file sections
  trace-cmd library: Write header before file sections
  trace-cmd library: Add multiple options sections in trace file version
    7
  trace-cmd library: Do not write CPU count section in trace files
    version 7
  trace-cmd library: Move CPU flyrecord trace metadata into the buffer
    option, for trace file version 7
  trace-cmd library: Add section header before flyrecord trace data
  trace-cmd library: Fit CPU latency trace data in the new trace file
    version 7 format
  trace-cmd library: Add macro to check file state on reading
  trace-cmd library: Introduce sections in trace file reading logic
  trace-cmd library: Initialize internal sections database on file read
  trace-cmd library: Use sections database when reading parts of the
    trace file
  trace-cmd library: Read headers from trace file version 7
  trace-cmd library: Read strings sections on file load
  trace-cmd library: Read extended BUFFER option
  trace-cmd library: Handle the extended DONE option
  trace-cmd library: Initialize CPU data for reading from version 7
    trace files
  trace-cmd library: Handle latency trace in version 7 files
  trace-cmd library: Handle buffer trace data init for version 7 files
  trace-cmd report: Use the new latency API to read data
  trace-cmd: Write buffers metadata in trace files version 6
  trace-cmd report: Add new parameter for trace file version

 include/trace-cmd/trace-cmd.h                 |   5 +
 .../include/private/trace-cmd-private.h       |  29 +-
 lib/trace-cmd/include/trace-cmd-local.h       |  17 +-
 lib/trace-cmd/trace-input.c                   | 824 ++++++++++++++----
 lib/trace-cmd/trace-msg.c                     | 127 ++-
 lib/trace-cmd/trace-output.c                  | 677 +++++++++++---
 lib/trace-cmd/trace-util.c                    |  12 +-
 tracecmd/trace-listen.c                       |   3 +
 tracecmd/trace-read.c                         |  18 +-
 tracecmd/trace-record.c                       |  32 +-
 tracecmd/trace-usage.c                        |   2 +-
 11 files changed, 1438 insertions(+), 308 deletions(-)

-- 
2.33.1

[PATCH v7 01/25] trace-cmd library: Define trace file version 7

[Tzvetomir Stoyanov (VMware)]

Added a define for file version 7, but keep the default file version to
6. Defined the new file version as the first version that supports trace
file sections. A new trace-cmd library API is introduced, to get the
default trace file version if the library:
  tracecmd_default_file_version()

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/include/private/trace-cmd-private.h |  6 ++++++
 lib/trace-cmd/include/trace-cmd-local.h           |  9 ++-------
 lib/trace-cmd/trace-output.c                      |  4 +++-
 lib/trace-cmd/trace-util.c                        | 12 +++++++++++-
 tracecmd/trace-record.c                           |  2 ++
 5 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index a26076de..f0d8960b 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -31,6 +31,11 @@ struct tep_plugin_list *trace_load_plugins(struct tep_handle *tep, int flags);
 
 int *tracecmd_add_id(int *list, int id, int len);
 
+#define FILE_VERSION_MIN		6
+#define FILE_VERSION_MAX		7
+
+#define FILE_VERSION_SECTIONS		7
+
 enum {
 	RINGBUF_TYPE_PADDING		= 29,
 	RINGBUF_TYPE_TIME_EXTEND	= 30,
@@ -43,6 +48,7 @@ void tracecmd_set_debug(bool set_debug);
 bool tracecmd_get_debug(void);
 
 bool tracecmd_is_version_supported(unsigned int version);
+int tracecmd_default_file_version(void);
 
 struct tracecmd_output;
 struct tracecmd_recorder;
diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index a0d6f0a6..4f8f1d76 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -9,18 +9,13 @@
 #include <byteswap.h>
 #include "trace-cmd-private.h"
 
+#define FILE_VERSION_DEFAULT		6
+
 /* Can be overridden */
 void tracecmd_warning(const char *fmt, ...);
 void tracecmd_critical(const char *fmt, ...);
 void tracecmd_info(const char *fmt, ...);
 
-/* trace.dat file format version */
-#define FILE_VERSION 6
-
-#define _STR(x)	#x
-#define STR(x)	_STR(x)
-#define FILE_VERSION_STRING STR(FILE_VERSION)
-
 #ifndef htonll
 # if __BYTE_ORDER == __LITTLE_ENDIAN
 #define htonll(x) __bswap_64(x)
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index f5e2574c..a524415c 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -83,6 +83,8 @@ struct list_event_system {
 	char				*name;
 };
 
+#define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
+
 static stsize_t
 do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size)
 {
@@ -919,7 +921,7 @@ struct tracecmd_output *tracecmd_output_create_fd(int fd)
 
 	handle->fd = fd;
 
-	handle->file_version = FILE_VERSION;
+	handle->file_version = FILE_VERSION_DEFAULT;
 
 	handle->page_size = getpagesize();
 	handle->big_endian = tracecmd_host_bigendian();
diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index 3b2d40ee..21f1b065 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -618,9 +618,19 @@ unsigned long long tracecmd_generate_traceid(void)
 	return hash;
 }
 
+/*
+ * tracecmd_default_file_version - Get default trace file version of the library
+ *
+ * Returns the default trace file version
+ */
+int tracecmd_default_file_version(void)
+{
+	return FILE_VERSION_DEFAULT;
+}
+
 bool tracecmd_is_version_supported(unsigned int version)
 {
-	if (version <= FILE_VERSION)
+	if (version >= FILE_VERSION_MIN && version <= FILE_VERSION_MAX)
 		return true;
 	return false;
 }
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index b4200db1..7b2b59bb 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -214,6 +214,7 @@ struct common_record_context {
 	int topt;
 	int run_command;
 	int saved_cmdlines_size;
+	int file_version;
 };
 
 static void add_reset_file(const char *file, const char *val, int prio)
@@ -5972,6 +5973,7 @@ static void init_common_record_context(struct common_record_context *ctx,
 	ctx->instance = &top_instance;
 	ctx->curr_cmd = curr_cmd;
 	local_cpu_count = tracecmd_count_cpus();
+	ctx->file_version = tracecmd_default_file_version();
 	init_top_instance();
 }
 
-- 
2.33.1

[PATCH v7 02/25] trace-cmd library: Add cache functionality to network message handler

[Tzvetomir Stoyanov (VMware)]

Network message handler is used to send trace metadata through a network
socket, instead writing it into a trace file. There are use cases,
that could require to do a lseek() on the metadata. It is hard to
implement lseek on a network socket, that's why for such use cases a
cache to a local file is introduced. Once the metadata is constructed,
the local cache is send to the socket. A new library API is used to
enable the local cache:
 tracecmd_msg_handle_cache()
The local cache is flushed on the socket when the
 tracecmd_msg_finish_sending_data()
is called.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |   5 +
 lib/trace-cmd/include/trace-cmd-local.h       |   1 +
 lib/trace-cmd/trace-msg.c                     | 127 +++++++++++++-----
 lib/trace-cmd/trace-output.c                  |  45 ++++---
 4 files changed, 127 insertions(+), 51 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index f0d8960b..9ed29e10 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -342,12 +342,16 @@ enum tracecmd_msg_flags {
 };
 
 /* for both client and server */
+#define MSG_CACHE_FILE "/tmp/trace_msg_cacheXXXXXX"
 struct tracecmd_msg_handle {
 	int			fd;
 	short			cpu_count;
 	short			version;	/* Current protocol version */
 	unsigned long		flags;
 	bool			done;
+	bool			cache;
+	int			cfd;
+	char			cfile[sizeof(MSG_CACHE_FILE)];
 };
 
 struct tracecmd_tsync_protos {
@@ -356,6 +360,7 @@ struct tracecmd_tsync_protos {
 
 struct tracecmd_msg_handle *
 tracecmd_msg_handle_alloc(int fd, unsigned long flags);
+int tracecmd_msg_handle_cache(struct tracecmd_msg_handle *msg_handle);
 
 /* Closes the socket and frees the handle */
 void tracecmd_msg_handle_close(struct tracecmd_msg_handle *msg_handle);
diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index 4f8f1d76..7f280533 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -47,5 +47,6 @@ struct cpu_data_source {
 
 int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
 		       struct cpu_data_source *data, const char *buff_name);
+off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
 
 #endif /* _TRACE_CMD_LOCAL_H */
diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
index 6667028e..4910ed5c 100644
--- a/lib/trace-cmd/trace-msg.c
+++ b/lib/trace-cmd/trace-msg.c
@@ -154,33 +154,55 @@ static inline int msg_buf_len(struct tracecmd_msg *msg)
 	return ntohl(msg->hdr.size) - MSG_HDR_LEN - ntohl(msg->hdr.cmd_size);
 }
 
-static int msg_write(int fd, struct tracecmd_msg *msg)
+static int __msg_write(int fd, struct tracecmd_msg *msg, bool network)
 {
-	int cmd = ntohl(msg->hdr.cmd);
 	int msg_size, data_size;
 	int ret;
-
-	if (cmd < 0 || cmd >= MSG_NR_COMMANDS)
-		return -EINVAL;
-
-	dprint("msg send: %d (%s) [%d]\n",
-	       cmd, cmd_to_name(cmd), ntohl(msg->hdr.size));
-
+	int cmd;
+
+	if (network) {
+		cmd = ntohl(msg->hdr.cmd);
+		if (cmd < 0 || cmd >= MSG_NR_COMMANDS)
+			return -EINVAL;
+		dprint("msg send: %d (%s) [%d]\n",
+		       cmd, cmd_to_name(cmd), ntohl(msg->hdr.size));
+	}
 	msg_size = MSG_HDR_LEN + ntohl(msg->hdr.cmd_size);
 	data_size = ntohl(msg->hdr.size) - msg_size;
 	if (data_size < 0)
 		return -EINVAL;
 
-	ret = __do_write_check(fd, msg, msg_size);
-	if (ret < 0)
-		return ret;
-
+	if (network) {
+		ret = __do_write_check(fd, msg, msg_size);
+		if (ret < 0)
+			return ret;
+	}
 	if (!data_size)
 		return 0;
 
 	return __do_write_check(fd, msg->buf, data_size);
 }
 
+__hidden off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off64_t offset, int whence)
+{
+	/*
+	 * lseek works only if the handle is in cache mode,
+	 * cannot seek on a network socket
+	 */
+	if (!msg_handle->cache || msg_handle->cfd < 0)
+		return (off64_t)-1;
+	return lseek64(msg_handle->cfd, offset, whence);
+}
+
+static int msg_write(struct tracecmd_msg_handle *msg_handle, struct tracecmd_msg *msg)
+{
+	if (msg_handle->cache && msg_handle->cfd >= 0)
+		return __msg_write(msg_handle->cfd, msg, false);
+
+
+	return __msg_write(msg_handle->fd, msg, true);
+}
+
 enum msg_trace_flags {
 	MSG_TRACE_USE_FIFOS = 1 << 0,
 };
@@ -274,11 +296,11 @@ static void msg_free(struct tracecmd_msg *msg)
 	memset(msg, 0, sizeof(*msg));
 }
 
-static int tracecmd_msg_send(int fd, struct tracecmd_msg *msg)
+static int tracecmd_msg_send(struct tracecmd_msg_handle *msg_handle, struct tracecmd_msg *msg)
 {
 	int ret = 0;
 
-	ret = msg_write(fd, msg);
+	ret = msg_write(msg_handle, msg);
 	if (ret < 0)
 		ret = -ECOMM;
 
@@ -287,11 +309,11 @@ static int tracecmd_msg_send(int fd, struct tracecmd_msg *msg)
 	return ret;
 }
 
-static int msg_send_nofree(int fd, struct tracecmd_msg *msg)
+static int msg_send_nofree(struct tracecmd_msg_handle *msg_handle, struct tracecmd_msg *msg)
 {
 	int ret = 0;
 
-	ret = msg_write(fd, msg);
+	ret = msg_write(msg_handle, msg);
 	if (ret < 0)
 		ret = -ECOMM;
 
@@ -454,7 +476,7 @@ static int tracecmd_msg_send_notsupp(struct tracecmd_msg_handle *msg_handle)
 	struct tracecmd_msg msg;
 
 	tracecmd_msg_init(MSG_NOT_SUPP, &msg);
-	return tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle, &msg);
 }
 
 static int handle_unexpected_msg(struct tracecmd_msg_handle *msg_handle,
@@ -472,7 +494,6 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 				unsigned int **client_ports)
 {
 	struct tracecmd_msg msg;
-	int fd = msg_handle->fd;
 	unsigned int *ports;
 	int i, cpus, ret;
 	char *p, *buf_end;
@@ -485,13 +506,13 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 	if (ret < 0)
 		goto out;
 
-	ret = tracecmd_msg_send(fd, &msg);
+	ret = tracecmd_msg_send(msg_handle, &msg);
 	if (ret < 0)
 		goto out;
 
 	msg_free(&msg);
 
-	ret = tracecmd_msg_wait_for_msg(fd, &msg);
+	ret = tracecmd_msg_wait_for_msg(msg_handle->fd, &msg);
 	if (ret < 0)
 		goto out;
 
@@ -564,12 +585,53 @@ tracecmd_msg_handle_alloc(int fd, unsigned long flags)
 
 	handle->fd = fd;
 	handle->flags = flags;
+	handle->cfd = -1;
+	handle->cache = false;
 	return handle;
 }
 
+int tracecmd_msg_handle_cache(struct tracecmd_msg_handle *msg_handle)
+{
+	if (msg_handle->cfd < 0) {
+		strcpy(msg_handle->cfile, MSG_CACHE_FILE);
+		msg_handle->cfd = mkstemp(msg_handle->cfile);
+		if (msg_handle->cfd < 0)
+			return -1;
+		unlink(msg_handle->cfile);
+	}
+	msg_handle->cache = true;
+	return 0;
+}
+
+static int flush_cache(struct tracecmd_msg_handle *msg_handle)
+{
+	char buf[MSG_MAX_DATA_LEN];
+	int ret;
+
+	if (!msg_handle->cache || msg_handle->cfd < 0)
+		return 0;
+	msg_handle->cache = false;
+	if (lseek64(msg_handle->cfd, 0, SEEK_SET) == (off64_t)-1)
+		return -1;
+	do {
+		ret = read(msg_handle->cfd, buf, MSG_MAX_DATA_LEN);
+		if (ret <= 0)
+			break;
+		ret = tracecmd_msg_data_send(msg_handle, buf, ret);
+		if (ret < 0)
+			break;
+	} while (ret >= 0);
+
+	close(msg_handle->cfd);
+	msg_handle->cfd = -1;
+	return ret;
+}
+
 void tracecmd_msg_handle_close(struct tracecmd_msg_handle *msg_handle)
 {
 	close(msg_handle->fd);
+	if (msg_handle->cfd >= 0)
+		close(msg_handle->cfd);
 	free(msg_handle);
 }
 
@@ -666,7 +728,7 @@ int tracecmd_msg_send_port_array(struct tracecmd_msg_handle *msg_handle,
 	if (ret < 0)
 		return ret;
 
-	ret = tracecmd_msg_send(msg_handle->fd, &msg);
+	ret = tracecmd_msg_send(msg_handle, &msg);
 	if (ret < 0)
 		return ret;
 
@@ -678,7 +740,7 @@ int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
 	struct tracecmd_msg msg;
 
 	tracecmd_msg_init(MSG_CLOSE, &msg);
-	return tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle, &msg);
 }
 
 int tracecmd_msg_send_close_resp_msg(struct tracecmd_msg_handle *msg_handle)
@@ -686,14 +748,13 @@ int tracecmd_msg_send_close_resp_msg(struct tracecmd_msg_handle *msg_handle)
 	struct tracecmd_msg msg;
 
 	tracecmd_msg_init(MSG_CLOSE_RESP, &msg);
-	return tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle, &msg);
 }
 
 int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
 			   const char *buf, int size)
 {
 	struct tracecmd_msg msg;
-	int fd = msg_handle->fd;
 	int n;
 	int ret;
 	int count = 0;
@@ -721,7 +782,7 @@ int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
 			memcpy(msg.buf, buf + count, n);
 			n = 0;
 		}
-		ret = msg_write(fd, &msg);
+		ret = msg_write(msg_handle, &msg);
 		if (ret < 0)
 			break;
 	}
@@ -735,8 +796,9 @@ int tracecmd_msg_finish_sending_data(struct tracecmd_msg_handle *msg_handle)
 	struct tracecmd_msg msg;
 	int ret;
 
+	flush_cache(msg_handle);
 	tracecmd_msg_init(MSG_FIN_DATA, &msg);
-	ret = tracecmd_msg_send(msg_handle->fd, &msg);
+	ret = tracecmd_msg_send(msg_handle, &msg);
 	if (ret < 0)
 		return ret;
 	return 0;
@@ -752,10 +814,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 	while (!tracecmd_msg_done(msg_handle)) {
 		ret = tracecmd_msg_recv_wait(msg_handle->fd, &msg);
 		if (ret < 0) {
-			if (ret == -ETIMEDOUT)
-				tracecmd_warning("Connection timed out\n");
-			else
-				tracecmd_warning("reading client");
+			tracecmd_warning("reading client %d (%s)", ret, strerror(ret));
 			return ret;
 		}
 
@@ -959,7 +1018,7 @@ int tracecmd_msg_send_trace_req(struct tracecmd_msg_handle *msg_handle,
 	if (ret < 0)
 		return ret;
 
-	return tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle, &msg);
 }
 
 static int get_trace_req_protos(char *buf, int length,
@@ -1151,7 +1210,7 @@ int tracecmd_msg_send_time_sync(struct tracecmd_msg_handle *msg_handle,
 	msg.hdr.size = htonl(ntohl(msg.hdr.size) + payload_size);
 
 	msg.buf = payload;
-	return msg_send_nofree(msg_handle->fd, &msg);
+	return msg_send_nofree(msg_handle, &msg);
 }
 
 /**
@@ -1283,7 +1342,7 @@ int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
 	if (ret < 0)
 		return ret;
 
-	return tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle, &msg);
 }
 
 int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index a524415c..6ed4bbf0 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -94,6 +94,14 @@ do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size)
 	return __do_write_check(handle->fd, data, size);
 }
 
+static inline off64_t do_lseek(struct tracecmd_output *handle, off_t offset, int whence)
+{
+	if (handle->msg_handle)
+		return msg_lseek(handle->msg_handle, offset, whence);
+	else
+		return lseek64(handle->fd, offset, whence);
+}
+
 static short convert_endian_2(struct tracecmd_output *handle, short val)
 {
 	if (!handle->pevent)
@@ -955,6 +963,9 @@ int tracecmd_output_set_msg(struct tracecmd_output *handle, struct tracecmd_msg_
 		return -1;
 
 	handle->msg_handle = msg_handle;
+	/* Force messages to be cached in a temp file before sending through the socket */
+	if (handle->msg_handle && HAS_SECTIONS(handle))
+		tracecmd_msg_handle_cache(handle->msg_handle);
 
 	return 0;
 }
@@ -1281,7 +1292,7 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 			return -1;
 
 		/* Save the data location in case it needs to be updated */
-		options->offset = lseek64(handle->fd, 0, SEEK_CUR);
+		options->offset = do_lseek(handle, 0, SEEK_CUR);
 
 		if (do_write_check(handle, options->data,
 				   options->size))
@@ -1314,9 +1325,9 @@ int tracecmd_append_options(struct tracecmd_output *handle)
 	if (handle->file_state != TRACECMD_FILE_OPTIONS)
 		return -1;
 
-	if (lseek64(handle->fd, 0, SEEK_END) == (off_t)-1)
+	if (do_lseek(handle, 0, SEEK_END) == (off_t)-1)
 		return -1;
-	offset = lseek64(handle->fd, -2, SEEK_CUR);
+	offset = do_lseek(handle, -2, SEEK_CUR);
 	if (offset == (off_t)-1)
 		return -1;
 
@@ -1334,7 +1345,7 @@ int tracecmd_append_options(struct tracecmd_output *handle)
 			return -1;
 
 		/* Save the data location in case it needs to be updated */
-		options->offset = lseek64(handle->fd, 0, SEEK_CUR);
+		options->offset = do_lseek(handle, 0, SEEK_CUR);
 
 		if (do_write_check(handle, options->data,
 				   options->size))
@@ -1525,10 +1536,10 @@ static int update_buffer_cpu_offset(struct tracecmd_output *handle,
 		return -1;
 	}
 
-	current = lseek64(handle->fd, 0, SEEK_CUR);
+	current = do_lseek(handle, 0, SEEK_CUR);
 
 	/* Go to the option data, where will write the offest */
-	if (lseek64(handle->fd, b_offset, SEEK_SET) == (off64_t)-1) {
+	if (do_lseek(handle, b_offset, SEEK_SET) == (off64_t)-1) {
 		tracecmd_warning("could not seek to %lld\n", b_offset);
 		return -1;
 	}
@@ -1537,7 +1548,7 @@ static int update_buffer_cpu_offset(struct tracecmd_output *handle,
 		return -1;
 
 	/* Go back to end of file */
-	if (lseek64(handle->fd, current, SEEK_SET) == (off64_t)-1) {
+	if (do_lseek(handle, current, SEEK_SET) == (off64_t)-1) {
 		tracecmd_warning("could not seek to %lld\n", offset);
 		return -1;
 	}
@@ -1586,7 +1597,7 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 		goto out_free;
 	}
 
-	data_offs = lseek64(handle->fd, 0, SEEK_CUR);
+	data_offs = do_lseek(handle, 0, SEEK_CUR);
 	if (do_write_check(handle, "flyrecord", 10))
 		goto out_free;
 
@@ -1601,10 +1612,10 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 		 * updated them with the correct data later.
 		 */
 		endian8 = 0;
-		data_files[i].file_data_offset = lseek64(handle->fd, 0, SEEK_CUR);
+		data_files[i].file_data_offset = do_lseek(handle, 0, SEEK_CUR);
 		if (do_write_check(handle, &endian8, 8))
 			goto out_free;
-		data_files[i].file_write_size = lseek64(handle->fd, 0, SEEK_CUR);
+		data_files[i].file_write_size = do_lseek(handle, 0, SEEK_CUR);
 		if (do_write_check(handle, &endian8, 8))
 			goto out_free;
 	}
@@ -1615,12 +1626,12 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 		goto out_free;
 
 	for (i = 0; i < cpus; i++) {
-		data_files[i].data_offset = lseek64(handle->fd, 0, SEEK_CUR);
+		data_files[i].data_offset = do_lseek(handle, 0, SEEK_CUR);
 		/* Page align offset */
 		data_files[i].data_offset += handle->page_size - 1;
 		data_files[i].data_offset &= ~(handle->page_size - 1);
 
-		ret = lseek64(handle->fd, data_files[i].data_offset, SEEK_SET);
+		ret = do_lseek(handle, data_files[i].data_offset, SEEK_SET);
 		if (ret == (off64_t)-1)
 			goto out_free;
 
@@ -1644,21 +1655,21 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 		}
 
 		/* Write the real CPU data offset in the file */
-		if (lseek64(handle->fd, data_files[i].file_data_offset, SEEK_SET) == (off64_t)-1)
+		if (do_lseek(handle, data_files[i].file_data_offset, 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].file_write_size, SEEK_SET) == (off64_t)-1)
+		if (do_lseek(handle, data_files[i].file_write_size, 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)
+		if (do_lseek(handle, offset, SEEK_SET) == (off64_t)-1)
 			goto out_free;
 
 		if (!tracecmd_get_quiet(handle))
@@ -1667,7 +1678,7 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 	}
 
 	free(data_files);
-	if (lseek64(handle->fd, 0, SEEK_END) == (off64_t)-1)
+	if (do_lseek(handle, 0, SEEK_END) == (off64_t)-1)
 		return -1;
 
 	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
@@ -1675,7 +1686,7 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 	return 0;
 
  out_free:
-	lseek64(handle->fd, 0, SEEK_END);
+	do_lseek(handle, 0, SEEK_END);
 	free(data_files);
 	return -1;
 }
-- 
2.33.1

[PATCH v7 03/25] trace-cmd library: New APIs to get and set version of output handler

[Tzvetomir Stoyanov (VMware)]

There are cases where the version of the file, associated with given
output handle must be get or set. Added new APIs for such use cases:
  tracecmd_get_out_file_version()
  tracecmd_output_set_version()

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  2 ++
 lib/trace-cmd/trace-output.c                  | 28 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 9ed29e10..ed25d879 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -280,6 +280,7 @@ int tracecmd_output_set_msg(struct tracecmd_output *handle,
 int tracecmd_output_set_trace_dir(struct tracecmd_output *handle, const char *tracing_dir);
 int tracecmd_output_set_kallsyms(struct tracecmd_output *handle, const char *kallsyms);
 int tracecmd_output_set_from_input(struct tracecmd_output *handle, struct tracecmd_input *ihandle);
+int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version);
 int tracecmd_output_write_headers(struct tracecmd_output *handle,
 				  struct tracecmd_event_list *list);
 
@@ -313,6 +314,7 @@ int tracecmd_append_cpu_data(struct tracecmd_output *handle,
 int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
 				    const char *name, int cpus, char * const *cpu_data_files);
 struct tracecmd_output *tracecmd_get_output_handle_fd(int fd);
+unsigned long tracecmd_get_out_file_version(struct tracecmd_output *handle);
 
 /* --- Reading the Fly Recorder Trace --- */
 
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 6ed4bbf0..4d165ac2 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1065,6 +1065,25 @@ int tracecmd_output_set_from_input(struct tracecmd_output *handle, struct tracec
 	return 0;
 }
 
+/**
+ * tracecmd_output_set_version - Set file version of the output handle
+ * @handle: output handle to a trace file.
+ * @file_version: desired file version
+ *
+ * This API must be called before tracecmd_output_write_headers().
+ *
+ * Returns 0 on success, or -1 if the output file handle is not allocated or not in expected state.
+ */
+int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version)
+{
+	if (!handle || handle->file_state != TRACECMD_FILE_ALLOCATED)
+		return -1;
+	if (file_version < FILE_VERSION_MIN || file_version > FILE_VERSION_MAX)
+		return -1;
+	handle->file_version = file_version;
+	return 0;
+}
+
 /**
  * output_write_init - Write the initial data into the trace file
  * @handle: output handle to a trace file.
@@ -1880,3 +1899,12 @@ __hidden bool check_out_state(struct tracecmd_output *handle, int new_state)
 {
 	return check_file_state(handle->file_version, handle->file_state, new_state);
 }
+
+/**
+ * tracecmd_get_out_file_version - return the trace.dat file version
+ * @handle: output handle for the trace.dat file
+ */
+unsigned long tracecmd_get_out_file_version(struct tracecmd_output *handle)
+{
+	return handle->file_version;
+}
-- 
2.33.1

[PATCH v7 04/25] trace-cmd library: Add strings section in trace file version 7

[Tzvetomir Stoyanov (VMware)]

In the trace file metadata there are various dynamic strings. Collecting
all these strings in a dedicated section in the file simplifies parsing
of the metadata. The string section is added in trace files version 7,
at the end of the file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  2 +
 lib/trace-cmd/trace-output.c                  | 63 +++++++++++++++++++
 tracecmd/trace-record.c                       |  1 +
 3 files changed, 66 insertions(+)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index ed25d879..ed8fbf11 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -138,6 +138,7 @@ enum {
 	TRACECMD_OPTION_TIME_SHIFT,
 	TRACECMD_OPTION_GUEST,
 	TRACECMD_OPTION_TSC2NSEC,
+	TRACECMD_OPTION_STRINGS,
 };
 
 enum {
@@ -301,6 +302,7 @@ 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);
 int tracecmd_write_options(struct tracecmd_output *handle);
+int tracecmd_write_meta_strings(struct tracecmd_output *handle);
 int tracecmd_append_options(struct tracecmd_output *handle);
 void tracecmd_output_close(struct tracecmd_output *handle);
 void tracecmd_output_free(struct tracecmd_output *handle);
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 4d165ac2..ed505db6 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -62,6 +62,8 @@ struct tracecmd_output {
 	bool			quiet;
 	unsigned long		file_state;
 	unsigned long		file_version;
+	unsigned long		strings_p;
+	unsigned long		strings_offs;
 	size_t			options_start;
 	bool			big_endian;
 
@@ -69,6 +71,8 @@ struct tracecmd_output {
 	struct list_head	buffers;
 	struct tracecmd_msg_handle *msg_handle;
 	char			*trace_clock;
+	char			*strings;
+
 };
 
 struct list_event {
@@ -85,6 +89,8 @@ struct list_event_system {
 
 #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
 
+static int save_string_section(struct tracecmd_output *handle);
+
 static stsize_t
 do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size)
 {
@@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle,
 	return tep_read_number(handle->pevent, &val, 8);
 }
 
+static long add_string(struct tracecmd_output *handle, const char *string)
+{
+	int size = strlen(string) + 1;
+	int pos = handle->strings_p;
+	char *strings;
+
+	strings = realloc(handle->strings, pos + size);
+	if (!strings)
+		return -1;
+	handle->strings = strings;
+	memcpy(handle->strings + pos, string, size);
+	handle->strings_p += size;
+
+	return handle->strings_offs + pos;
+}
+
 /**
  * tracecmd_set_quiet - Set if to print output to the screen
  * @quiet: If non zero, print no output to the screen
@@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle)
 		free(option);
 	}
 
+	free(handle->strings);
 	free(handle->trace_clock);
 	free(handle);
 }
@@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle)
 	if (!handle)
 		return;
 
+	if (handle->file_version >= FILE_VERSION_SECTIONS) {
+		/* write strings section */
+		save_string_section(handle);
+	}
+
 	if (handle->fd >= 0) {
 		close(handle->fd);
 		handle->fd = -1;
@@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set)
 	return ret;
 }
 
+static int save_string_section(struct tracecmd_output *handle)
+{
+	if (!handle->strings || !handle->strings_p)
+		return 0;
+
+	if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) {
+		tracecmd_warning("Cannot write strings, unexpected state 0x%X",
+				 handle->file_state);
+		return -1;
+	}
+
+	if (do_write_check(handle, handle->strings, handle->strings_p))
+		goto error;
+
+	handle->strings_offs += handle->strings_p;
+	free(handle->strings);
+	handle->strings = NULL;
+	handle->strings_p = 0;
+	handle->file_state = TRACECMD_OPTION_STRINGS;
+	return 0;
+
+error:
+	return -1;
+}
+
+
 static int read_header_files(struct tracecmd_output *handle)
 {
 	tsize_t size, check_size, endian8;
@@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 	return 0;
 }
 
+int tracecmd_write_meta_strings(struct tracecmd_output *handle)
+{
+	if (!HAS_SECTIONS(handle))
+		return 0;
+
+	return save_string_section(handle);
+}
+
+
 int tracecmd_append_options(struct tracecmd_output *handle)
 {
 	struct tracecmd_option *options;
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7b2b59bb..f599610e 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance,
 	tracecmd_write_cmdlines(network_handle);
 	tracecmd_write_cpus(network_handle, instance->cpu_count);
 	tracecmd_write_options(network_handle);
+	tracecmd_write_meta_strings(network_handle);
 	tracecmd_msg_finish_sending_data(instance->msg_handle);
 	instance->network_handle = network_handle;
 }
-- 
2.33.1

Re: [PATCH v7 04/25] trace-cmd library: Add strings section in trace file version 7

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:27 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> In the trace file metadata there are various dynamic strings. Collecting
> all these strings in a dedicated section in the file simplifies parsing
> of the metadata. The string section is added in trace files version 7,
> at the end of the file.

Does it have to be at the end of the file?

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  .../include/private/trace-cmd-private.h       |  2 +
>  lib/trace-cmd/trace-output.c                  | 63 +++++++++++++++++++
>  tracecmd/trace-record.c                       |  1 +
>  3 files changed, 66 insertions(+)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index ed25d879..ed8fbf11 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -138,6 +138,7 @@ enum {
>  	TRACECMD_OPTION_TIME_SHIFT,
>  	TRACECMD_OPTION_GUEST,
>  	TRACECMD_OPTION_TSC2NSEC,
> +	TRACECMD_OPTION_STRINGS,
>  };
>  
>  enum {
> @@ -301,6 +302,7 @@ 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);
>  int tracecmd_write_options(struct tracecmd_output *handle);
> +int tracecmd_write_meta_strings(struct tracecmd_output *handle);
>  int tracecmd_append_options(struct tracecmd_output *handle);
>  void tracecmd_output_close(struct tracecmd_output *handle);
>  void tracecmd_output_free(struct tracecmd_output *handle);
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 4d165ac2..ed505db6 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -62,6 +62,8 @@ struct tracecmd_output {
>  	bool			quiet;
>  	unsigned long		file_state;
>  	unsigned long		file_version;
> +	unsigned long		strings_p;
> +	unsigned long		strings_offs;

Can you add a comment to what the above are to represent?

Especially out of context, it's hard to know how this is suppose to
work.

>  	size_t			options_start;
>  	bool			big_endian;
>  
> @@ -69,6 +71,8 @@ struct tracecmd_output {
>  	struct list_head	buffers;
>  	struct tracecmd_msg_handle *msg_handle;
>  	char			*trace_clock;
> +	char			*strings;
> +

Remove the extra space.

>  };
>  
>  struct list_event {
> @@ -85,6 +89,8 @@ struct list_event_system {
>  
>  #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
>  
> +static int save_string_section(struct tracecmd_output *handle);
> +

I won't ask you to fix it, but it's best not to introduce a static
function that is not used, and if need be, just combine the patches.

Again, without use cases, it's hard to know if this is doing what you
say it is doing.

>  static stsize_t
>  do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size)
>  {
> @@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle,
>  	return tep_read_number(handle->pevent, &val, 8);
>  }
>  
> +static long add_string(struct tracecmd_output *handle, const char *string)
> +{
> +	int size = strlen(string) + 1;
> +	int pos = handle->strings_p;
> +	char *strings;
> +
> +	strings = realloc(handle->strings, pos + size);
> +	if (!strings)
> +		return -1;
> +	handle->strings = strings;
> +	memcpy(handle->strings + pos, string, size);
> +	handle->strings_p += size;
> +
> +	return handle->strings_offs + pos;

What is the above suppose to be returning? What is strings_off?

> +}
> +
>  /**
>   * tracecmd_set_quiet - Set if to print output to the screen
>   * @quiet: If non zero, print no output to the screen
> @@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle)
>  		free(option);
>  	}
>  
> +	free(handle->strings);
>  	free(handle->trace_clock);
>  	free(handle);
>  }
> @@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle)
>  	if (!handle)
>  		return;
>  
> +	if (handle->file_version >= FILE_VERSION_SECTIONS) {

Shouldn't the above be if (HAS_SECTIONS(handle)) ?

-- Steve

> +		/* write strings section */
> +		save_string_section(handle);
> +	}
> +
>  	if (handle->fd >= 0) {
>  		close(handle->fd);
>  		handle->fd = -1;
> @@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set)
>  	return ret;
>  }
>  
> +static int save_string_section(struct tracecmd_output *handle)
> +{
> +	if (!handle->strings || !handle->strings_p)
> +		return 0;
> +
> +	if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) {
> +		tracecmd_warning("Cannot write strings, unexpected state 0x%X",
> +				 handle->file_state);
> +		return -1;
> +	}
> +
> +	if (do_write_check(handle, handle->strings, handle->strings_p))
> +		goto error;
> +
> +	handle->strings_offs += handle->strings_p;
> +	free(handle->strings);
> +	handle->strings = NULL;
> +	handle->strings_p = 0;
> +	handle->file_state = TRACECMD_OPTION_STRINGS;
> +	return 0;
> +
> +error:
> +	return -1;
> +}
> +
> +
>  static int read_header_files(struct tracecmd_output *handle)
>  {
>  	tsize_t size, check_size, endian8;
> @@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle)
>  	return 0;
>  }
>  
> +int tracecmd_write_meta_strings(struct tracecmd_output *handle)
> +{
> +	if (!HAS_SECTIONS(handle))
> +		return 0;
> +
> +	return save_string_section(handle);
> +}
> +
> +
>  int tracecmd_append_options(struct tracecmd_output *handle)
>  {
>  	struct tracecmd_option *options;
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 7b2b59bb..f599610e 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance,
>  	tracecmd_write_cmdlines(network_handle);
>  	tracecmd_write_cpus(network_handle, instance->cpu_count);
>  	tracecmd_write_options(network_handle);
> +	tracecmd_write_meta_strings(network_handle);
>  	tracecmd_msg_finish_sending_data(instance->msg_handle);
>  	instance->network_handle = network_handle;
>  }

Re: [PATCH v7 04/25] trace-cmd library: Add strings section in trace file version 7

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 2:53 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:27 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > In the trace file metadata there are various dynamic strings. Collecting
> > all these strings in a dedicated section in the file simplifies parsing
> > of the metadata. The string section is added in trace files version 7,
> > at the end of the file.
>
> Does it have to be at the end of the file?

The v7 file can have multiple string sections. The last string section
must be at the end of the file, as it contains meta-data strings from
the other sections.

>
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  .../include/private/trace-cmd-private.h       |  2 +
> >  lib/trace-cmd/trace-output.c                  | 63 +++++++++++++++++++
> >  tracecmd/trace-record.c                       |  1 +
> >  3 files changed, 66 insertions(+)
> >
> > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> > index ed25d879..ed8fbf11 100644
> > --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> > @@ -138,6 +138,7 @@ enum {
> >       TRACECMD_OPTION_TIME_SHIFT,
> >       TRACECMD_OPTION_GUEST,
> >       TRACECMD_OPTION_TSC2NSEC,
> > +     TRACECMD_OPTION_STRINGS,
> >  };
> >
> >  enum {
> > @@ -301,6 +302,7 @@ 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);
> >  int tracecmd_write_options(struct tracecmd_output *handle);
> > +int tracecmd_write_meta_strings(struct tracecmd_output *handle);
> >  int tracecmd_append_options(struct tracecmd_output *handle);
> >  void tracecmd_output_close(struct tracecmd_output *handle);
> >  void tracecmd_output_free(struct tracecmd_output *handle);
> > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> > index 4d165ac2..ed505db6 100644
> > --- a/lib/trace-cmd/trace-output.c
> > +++ b/lib/trace-cmd/trace-output.c
> > @@ -62,6 +62,8 @@ struct tracecmd_output {
> >       bool                    quiet;
> >       unsigned long           file_state;
> >       unsigned long           file_version;
> > +     unsigned long           strings_p;
> > +     unsigned long           strings_offs;
>
> Can you add a comment to what the above are to represent?
>
> Especially out of context, it's hard to know how this is suppose to
> work.
>
> >       size_t                  options_start;
> >       bool                    big_endian;
> >
> > @@ -69,6 +71,8 @@ struct tracecmd_output {
> >       struct list_head        buffers;
> >       struct tracecmd_msg_handle *msg_handle;
> >       char                    *trace_clock;
> > +     char                    *strings;
> > +
>
> Remove the extra space.
>
> >  };
> >
> >  struct list_event {
> > @@ -85,6 +89,8 @@ struct list_event_system {
> >
> >  #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
> >
> > +static int save_string_section(struct tracecmd_output *handle);
> > +
>
> I won't ask you to fix it, but it's best not to introduce a static
> function that is not used, and if need be, just combine the patches.
>
> Again, without use cases, it's hard to know if this is doing what you
> say it is doing.
>
> >  static stsize_t
> >  do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size)
> >  {
> > @@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle,
> >       return tep_read_number(handle->pevent, &val, 8);
> >  }
> >
> > +static long add_string(struct tracecmd_output *handle, const char *string)
> > +{
> > +     int size = strlen(string) + 1;
> > +     int pos = handle->strings_p;
> > +     char *strings;
> > +
> > +     strings = realloc(handle->strings, pos + size);
> > +     if (!strings)
> > +             return -1;
> > +     handle->strings = strings;
> > +     memcpy(handle->strings + pos, string, size);
> > +     handle->strings_p += size;
> > +
> > +     return handle->strings_offs + pos;
>
> What is the above suppose to be returning? What is strings_off?
>

I'll add comments on strings_off and strings_p in the next version,
but the main idea is:
 This function returns the virtual offset of the added string into the
string section. There can be multiple string sections in the file, but
the string offset is continuous and does not depend on the number of
string sections and their position in the file.

> > +}
> > +
> >  /**
> >   * tracecmd_set_quiet - Set if to print output to the screen
> >   * @quiet: If non zero, print no output to the screen
> > @@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle)
> >               free(option);
> >       }
> >
> > +     free(handle->strings);
> >       free(handle->trace_clock);
> >       free(handle);
> >  }
> > @@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle)
> >       if (!handle)
> >               return;
> >
> > +     if (handle->file_version >= FILE_VERSION_SECTIONS) {
>
> Shouldn't the above be if (HAS_SECTIONS(handle)) ?
>
> -- Steve
>
> > +             /* write strings section */
> > +             save_string_section(handle);
> > +     }
> > +
> >       if (handle->fd >= 0) {
> >               close(handle->fd);
> >               handle->fd = -1;
> > @@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set)
> >       return ret;
> >  }
> >
> > +static int save_string_section(struct tracecmd_output *handle)
> > +{
> > +     if (!handle->strings || !handle->strings_p)
> > +             return 0;
> > +
> > +     if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) {
> > +             tracecmd_warning("Cannot write strings, unexpected state 0x%X",
> > +                              handle->file_state);
> > +             return -1;
> > +     }
> > +
> > +     if (do_write_check(handle, handle->strings, handle->strings_p))
> > +             goto error;
> > +
> > +     handle->strings_offs += handle->strings_p;
> > +     free(handle->strings);
> > +     handle->strings = NULL;
> > +     handle->strings_p = 0;
> > +     handle->file_state = TRACECMD_OPTION_STRINGS;
> > +     return 0;
> > +
> > +error:
> > +     return -1;
> > +}
> > +
> > +
> >  static int read_header_files(struct tracecmd_output *handle)
> >  {
> >       tsize_t size, check_size, endian8;
> > @@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle)
> >       return 0;
> >  }
> >
> > +int tracecmd_write_meta_strings(struct tracecmd_output *handle)
> > +{
> > +     if (!HAS_SECTIONS(handle))
> > +             return 0;
> > +
> > +     return save_string_section(handle);
> > +}
> > +
> > +
> >  int tracecmd_append_options(struct tracecmd_output *handle)
> >  {
> >       struct tracecmd_option *options;
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index 7b2b59bb..f599610e 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance,
> >       tracecmd_write_cmdlines(network_handle);
> >       tracecmd_write_cpus(network_handle, instance->cpu_count);
> >       tracecmd_write_options(network_handle);
> > +     tracecmd_write_meta_strings(network_handle);
> >       tracecmd_msg_finish_sending_data(instance->msg_handle);
> >       instance->network_handle = network_handle;
> >  }
>


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

Re: [PATCH v7 04/25] trace-cmd library: Add strings section in trace file version 7

[Steven Rostedt]

On Mon, 17 Jan 2022 11:32:10 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > Does it have to be at the end of the file?  
> 
> The v7 file can have multiple string sections. The last string section
> must be at the end of the file, as it contains meta-data strings from
> the other sections.

Right, but it's not a requirement. It's more of what just happens. My
concern is that it reads as a requirement not something that just
happens to be an implementation side effect.

-- Steve

Re: [PATCH v7 04/25] trace-cmd library: Add strings section in trace file version 7

[Tzvetomir Stoyanov]

On Mon, Jan 17, 2022 at 4:40 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 17 Jan 2022 11:32:10 +0200
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > > Does it have to be at the end of the file?
> >
> > The v7 file can have multiple string sections. The last string section
> > must be at the end of the file, as it contains meta-data strings from
> > the other sections.
>
> Right, but it's not a requirement. It's more of what just happens. My
> concern is that it reads as a requirement not something that just
> happens to be an implementation side effect.
>
Yes, it is not a requirement - as there is no strict order, any
section can be anywhere in the file.

> -- Steve



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

[PATCH v7 05/25] trace-cmd library: Add internal helper function for writing headers before file sections

[Tzvetomir Stoyanov (VMware)]

Introduce headers before each file section, in trace file version 7. The
section header has the following format:
 <2 bytes>, header ID
 <4 bytes>, offset within the strings section, where the section
            description string is located.
 <2 bytes>, section flags:
     1: the section is compressed
 <8 bytes>, size of the section

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/trace-cmd/trace-cmd.h                 |  5 ++
 .../include/private/trace-cmd-private.h       |  1 +
 lib/trace-cmd/include/trace-cmd-local.h       |  5 ++
 lib/trace-cmd/trace-output.c                  | 77 ++++++++++++++++++-
 4 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 7fea4e01..5d71e8ba 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -15,6 +15,11 @@ enum tracecmd_open_flags {
 	TRACECMD_FL_LOAD_NO_PLUGINS		= 1 << 0, /* Do not load plugins */
 	TRACECMD_FL_LOAD_NO_SYSTEM_PLUGINS	= 1 << 1, /* Do not load system plugins */
 };
+
+enum tracecmd_section_flags {
+	TRACECMD_SEC_FL_COMPRESS	= 1 << 0, /* the section is compressed */
+};
+
 struct tracecmd_input *tracecmd_open_head(const char *file, int flags);
 struct tracecmd_input *tracecmd_open(const char *file, int flags);
 struct tracecmd_input *tracecmd_open_fd(int fd, int flags);
diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index ed8fbf11..1584d65a 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -139,6 +139,7 @@ enum {
 	TRACECMD_OPTION_GUEST,
 	TRACECMD_OPTION_TSC2NSEC,
 	TRACECMD_OPTION_STRINGS,
+	TRACECMD_OPTION_MAX,
 };
 
 enum {
diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index 7f280533..4a0a691c 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -39,6 +39,11 @@ struct data_file_write {
 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);
 
+unsigned long long
+out_write_section_header(struct tracecmd_output *handle, unsigned short header_id,
+			 char *description, int flags, bool option);
+int out_update_section_header(struct tracecmd_output *handle, unsigned long long offset);
+
 struct cpu_data_source {
 	int fd;
 	int size;
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index ed505db6..457da305 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -360,6 +360,82 @@ int tracecmd_ftrace_enable(int set)
 	return ret;
 }
 
+__hidden unsigned long long
+out_write_section_header(struct tracecmd_output *handle, unsigned short header_id,
+			 char *description, int flags, bool option)
+{
+	tsize_t endian8;
+	tsize_t offset;
+	long long size;
+	short endian2;
+	int endian4;
+	int desc;
+
+	if (header_id >= TRACECMD_OPTION_MAX)
+		return -1;
+	if (!HAS_SECTIONS(handle))
+		return 0;
+	offset = do_lseek(handle, 0, SEEK_CUR);
+	if (option) {
+		endian8 = convert_endian_8(handle, offset);
+		if (!tracecmd_add_option(handle, header_id, 8, &endian8))
+			return -1;
+	}
+	/* Section ID */
+	endian2 = convert_endian_2(handle, header_id);
+	if (do_write_check(handle, &endian2, 2))
+		return (off64_t)-1;
+
+	/* Section description */
+	if (description)
+		desc = add_string(handle, description);
+	else
+		desc = -1;
+	endian4 = convert_endian_4(handle, desc);
+	if (do_write_check(handle, &endian4, 4))
+		return (off64_t)-1;
+
+	/* Section flags */
+	endian2 = convert_endian_2(handle, flags);
+	if (do_write_check(handle, &endian2, 2))
+		return (off64_t)-1;
+
+	offset = do_lseek(handle, 0, SEEK_CUR);
+	size = 0;
+	/* Reserve for section size */
+	if (do_write_check(handle, &size, 8))
+		return (off64_t)-1;
+	return offset;
+}
+
+__hidden int out_update_section_header(struct tracecmd_output *handle, tsize_t offset)
+{
+	tsize_t current;
+	tsize_t endian8;
+	tsize_t size;
+
+	if (!HAS_SECTIONS(handle) || offset == 0)
+		return 0;
+
+	current = do_lseek(handle, 0, SEEK_CUR);
+	/* The real size is the difference between the saved offset and
+	 * the current offset - 8 bytes, the reserved space for the section size.
+	 */
+	size = current - offset;
+	if (size < 8)
+		return -1;
+	size -= 8;
+	if (do_lseek(handle, offset, SEEK_SET) == (off64_t)-1)
+		return -1;
+
+	endian8 = convert_endian_8(handle, size);
+	if (do_write_check(handle, &endian8, 8))
+		return -1;
+	if (do_lseek(handle, current, SEEK_SET) == (off64_t)-1)
+		return -1;
+	return 0;
+}
+
 static int save_string_section(struct tracecmd_output *handle)
 {
 	if (!handle->strings || !handle->strings_p)
@@ -385,7 +461,6 @@ error:
 	return -1;
 }
 
-
 static int read_header_files(struct tracecmd_output *handle)
 {
 	tsize_t size, check_size, endian8;
-- 
2.33.1

Re: [PATCH v7 05/25] trace-cmd library: Add internal helper function for writing headers before file sections

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:28 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Introduce headers before each file section, in trace file version 7. The
> section header has the following format:
>  <2 bytes>, header ID
>  <4 bytes>, offset within the strings section, where the section
>             description string is located.
>  <2 bytes>, section flags:
>      1: the section is compressed

Would in make more sense to have the ID and flags together, followed by
the string? That way you could use a normal structure to represent it.

struct {
	unsigned short			id;
	unsigned short			flags;
	unsigned int			string_offset;
	unsigned long long		size;
};

Otherwise, all tools to read it will be forced to read each part
individually. I can imaging that being annoying in the future.

-- Steve


>  <8 bytes>, size of the section
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---

Re: [PATCH v7 05/25] trace-cmd library: Add internal helper function for writing headers before file sections

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 2:59 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:28 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > Introduce headers before each file section, in trace file version 7. The
> > section header has the following format:
> >  <2 bytes>, header ID
> >  <4 bytes>, offset within the strings section, where the section
> >             description string is located.
> >  <2 bytes>, section flags:
> >      1: the section is compressed
>
> Would in make more sense to have the ID and flags together, followed by
> the string? That way you could use a normal structure to represent it.
>
It would make sense in the previous version of this implementation,
where a dynamic string was stored between the ID and the flags. Now as
all these are in fixed positions, the order should not be a problem.
But anyway, I'll change it in the next version.

> struct {
>         unsigned short                  id;
>         unsigned short                  flags;
>         unsigned int                    string_offset;
>         unsigned long long              size;
> };
>
> Otherwise, all tools to read it will be forced to read each part
> individually. I can imaging that being annoying in the future.
>
> -- Steve
>
>
> >  <8 bytes>, size of the section
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---



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

[PATCH v7 06/25] trace-cmd library: Write header before file sections

[Tzvetomir Stoyanov (VMware)]

Headers are added before these file sections, in trace
file version 7:
 - ftrace events format
 - format of recorded events
 - information of the mapping of function addresses to the function names
 - trace_printk() format strings
 - information of the mapping a PID to a process name
 - strings

New options are defined for each of these sections, holding
the section's offset into the trace file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |   6 +
 lib/trace-cmd/trace-output.c                  | 120 ++++++++++++++----
 2 files changed, 103 insertions(+), 23 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 1584d65a..047fc26f 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -139,6 +139,12 @@ enum {
 	TRACECMD_OPTION_GUEST,
 	TRACECMD_OPTION_TSC2NSEC,
 	TRACECMD_OPTION_STRINGS,
+	TRACECMD_OPTION_HEADER_INFO,
+	TRACECMD_OPTION_FTRACE_EVENTS,
+	TRACECMD_OPTION_EVENT_FORMATS,
+	TRACECMD_OPTION_KALLSYMS,
+	TRACECMD_OPTION_PRINTK,
+	TRACECMD_OPTION_CMDLINES,
 	TRACECMD_OPTION_MAX,
 };
 
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 457da305..aa55aad7 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -438,6 +438,9 @@ __hidden int out_update_section_header(struct tracecmd_output *handle, tsize_t o
 
 static int save_string_section(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
+	tsize_t offset;
+
 	if (!handle->strings || !handle->strings_p)
 		return 0;
 
@@ -447,9 +450,16 @@ static int save_string_section(struct tracecmd_output *handle)
 		return -1;
 	}
 
+	offset = out_write_section_header(handle, TRACECMD_OPTION_STRINGS, "strings", flags, false);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	if (do_write_check(handle, handle->strings, handle->strings_p))
 		goto error;
 
+	if (out_update_section_header(handle, offset))
+		return -1;
+
 	handle->strings_offs += handle->strings_p;
 	free(handle->strings);
 	handle->strings = NULL;
@@ -463,10 +473,12 @@ error:
 
 static int read_header_files(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
 	tsize_t size, check_size, endian8;
 	struct stat st;
+	tsize_t offset;
 	char *path;
-	int fd;
+	int fd = -1;
 	int ret;
 
 	if (!check_out_state(handle, TRACECMD_FILE_HEADERS)) {
@@ -479,26 +491,33 @@ static int read_header_files(struct tracecmd_output *handle)
 	if (!path)
 		return -1;
 
+	offset = out_write_section_header(handle, TRACECMD_OPTION_HEADER_INFO,
+					  "headers", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* old style did not show this info, just add zero */
 		put_tracing_file(path);
 		if (do_write_check(handle, "header_page", 12))
-			return -1;
+			goto out_close;
 		size = 0;
 		if (do_write_check(handle, &size, 8))
-			return -1;
+			goto out_close;
 		if (do_write_check(handle, "header_event", 13))
-			return -1;
+			goto out_close;
 		if (do_write_check(handle, &size, 8))
-			return -1;
+			goto out_close;
+		if (out_update_section_header(handle, offset))
+			goto out_close;
 		return 0;
 	}
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		return -1;
+		goto out_close;
 	}
 
 	/* unfortunately, you can not stat debugfs files for size */
@@ -514,18 +533,18 @@ static int read_header_files(struct tracecmd_output *handle)
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s' size=%lld read=%lld", path, size, check_size);
 		errno = EINVAL;
-		return -1;
+		goto out_close;
 	}
 	put_tracing_file(path);
 
 	path = get_tracing_file(handle, "events/header_event");
 	if (!path)
-		return -1;
+		goto out_close;
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		tracecmd_warning("can't read '%s'", path);
-		return -1;
+		goto out_close;
 	}
 
 	size = get_size_fd(fd);
@@ -539,16 +558,18 @@ static int read_header_files(struct tracecmd_output *handle)
 	close(fd);
 	if (size != check_size) {
 		tracecmd_warning("wrong size for '%s'", path);
-		return -1;
+		goto out_close;
 	}
 	put_tracing_file(path);
-
+	if (out_update_section_header(handle, offset))
+		goto out_close;
 	handle->file_state = TRACECMD_FILE_HEADERS;
 
 	return 0;
 
  out_close:
-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	return -1;
 }
 
@@ -777,8 +798,10 @@ create_event_list_item(struct tracecmd_output *handle,
 
 static int read_ftrace_files(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
 	struct list_event_system *systems = NULL;
 	struct tracecmd_event_list list = { .glob = "ftrace/*" };
+	tsize_t offset;
 	int ret;
 
 	if (!check_out_state(handle, TRACECMD_FILE_FTRACE_EVENTS)) {
@@ -787,11 +810,20 @@ static int read_ftrace_files(struct tracecmd_output *handle)
 		return -1;
 	}
 
+	offset = out_write_section_header(handle, TRACECMD_OPTION_FTRACE_EVENTS,
+					  "ftrace events", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	create_event_list_item(handle, &systems, &list);
 
 	ret = copy_event_system(handle, systems);
 
 	free_list_events(systems);
+	if (ret)
+		return ret;
+	if (out_update_section_header(handle, offset))
+		return -1;
 
 	handle->file_state = TRACECMD_FILE_FTRACE_EVENTS;
 
@@ -814,11 +846,13 @@ create_event_list(struct tracecmd_output *handle,
 static int read_event_files(struct tracecmd_output *handle,
 			    struct tracecmd_event_list *event_list)
 {
+	enum tracecmd_section_flags flags = 0;
 	struct list_event_system *systems;
 	struct list_event_system *slist;
 	struct tracecmd_event_list *list;
 	struct tracecmd_event_list all_events = { .glob = "*/*" };
 	int count = 0;
+	tsize_t offset;
 	int endian4;
 	int ret;
 
@@ -827,6 +861,11 @@ static int read_event_files(struct tracecmd_output *handle,
 				 handle->file_state);
 		return -1;
 	}
+
+	offset = out_write_section_header(handle, TRACECMD_OPTION_EVENT_FORMATS,
+					  "events format", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
 	/*
 	 * If any of the list is the special keyword "all" then
 	 * just do all files.
@@ -858,9 +897,14 @@ static int read_event_files(struct tracecmd_output *handle,
 		}
 		ret = copy_event_system(handle, slist);
 	}
+	if (ret)
+		goto out_free;
+	ret = out_update_section_header(handle, offset);
 
-	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
  out_free:
+	if (!ret)
+		handle->file_state = TRACECMD_FILE_ALL_EVENTS;
+
 	free_list_events(systems);
 
 	return ret;
@@ -908,8 +952,10 @@ err:
 
 static int read_proc_kallsyms(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
 	unsigned int size, check_size, endian4;
 	const char *path = "/proc/kallsyms";
+	tsize_t offset;
 	struct stat st;
 	int ret;
 
@@ -922,19 +968,24 @@ static int read_proc_kallsyms(struct tracecmd_output *handle)
 	if (handle->kallsyms)
 		path = handle->kallsyms;
 
+	offset = out_write_section_header(handle, TRACECMD_OPTION_KALLSYMS,
+					  "kallsyms", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
 		size = 0;
 		endian4 = convert_endian_4(handle, size);
-		if (do_write_check(handle, &endian4, 4))
-			return -1;
-		return 0;
+		ret = do_write_check(handle, &endian4, 4);
+		goto out;
 	}
 	size = get_size(path);
 	endian4 = convert_endian_4(handle, size);
-	if (do_write_check(handle, &endian4, 4))
-		return -1;
+	ret = do_write_check(handle, &endian4, 4);
+	if (ret)
+		goto out;
 
 	set_proc_kptr_restrict(0);
 	check_size = copy_file(handle, path);
@@ -942,18 +993,23 @@ static int read_proc_kallsyms(struct tracecmd_output *handle)
 		errno = EINVAL;
 		tracecmd_warning("error in size of file '%s'", path);
 		set_proc_kptr_restrict(1);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 	set_proc_kptr_restrict(1);
 
-	handle->file_state = TRACECMD_FILE_KALLSYMS;
-
-	return 0;
+	ret = out_update_section_header(handle, offset);
+out:
+	if (!ret)
+		handle->file_state = TRACECMD_FILE_KALLSYMS;
+	return ret;
 }
 
 static int read_ftrace_printk(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
 	unsigned int size, check_size, endian4;
+	tsize_t offset;
 	struct stat st;
 	char *path;
 	int ret;
@@ -968,6 +1024,10 @@ static int read_ftrace_printk(struct tracecmd_output *handle)
 	if (!path)
 		return -1;
 
+	offset = out_write_section_header(handle, TRACECMD_OPTION_PRINTK, "printk", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
@@ -989,8 +1049,10 @@ static int read_ftrace_printk(struct tracecmd_output *handle)
 	}
 
  out:
-	handle->file_state = TRACECMD_FILE_PRINTK;
 	put_tracing_file(path);
+	if (out_update_section_header(handle, offset))
+		return -1;
+	handle->file_state = TRACECMD_FILE_PRINTK;
 	return 0;
  fail:
 	put_tracing_file(path);
@@ -1594,6 +1656,8 @@ static tsize_t get_buffer_file_offset(struct tracecmd_output *handle, const char
 
 int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 {
+	enum tracecmd_section_flags flags = 0;
+	tsize_t offset;
 	int ret;
 
 	if (!check_out_state(handle, TRACECMD_FILE_CMD_LINES)) {
@@ -1601,9 +1665,19 @@ int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 				 handle->file_state);
 		return -1;
 	}
+
+	offset = out_write_section_header(handle, TRACECMD_OPTION_CMDLINES,
+					  "command lines", flags, true);
+	if (offset == (off64_t)-1)
+		return -1;
+
 	ret = save_tracing_file_data(handle, "saved_cmdlines");
 	if (ret < 0)
 		return ret;
+
+	if (out_update_section_header(handle, offset))
+		return -1;
+
 	handle->file_state = TRACECMD_FILE_CMD_LINES;
 	return 0;
 }
-- 
2.33.1

[PATCH v7 07/25] trace-cmd library: Add multiple options sections in trace file version 7

[Tzvetomir Stoyanov (VMware)]

Introduced chain of options sections in trace file version 7. Extended
the "DONE" option to hold the offset into the file to the next options
section.
Format of trace file version 7 is extended with a new mandatory field
after the compression algorithm header:
 <8 bytes>, unsigned long long integer - offset into the trace file
where the first options section is located. This allows to place this
section anywhere in the file. As all other sections have corresponding
options, describing their offsets into the trace file, this change makes
the structure of trace file version 7 flexible.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index aa55aad7..6e07fece 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -64,7 +64,7 @@ struct tracecmd_output {
 	unsigned long		file_version;
 	unsigned long		strings_p;
 	unsigned long		strings_offs;
-	size_t			options_start;
+	tsize_t			options_start;
 	bool			big_endian;
 
 	struct list_head	options;
@@ -89,6 +89,7 @@ struct list_event_system {
 
 #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
 
+static int write_options_v7(struct tracecmd_output *handle);
 static int save_string_section(struct tracecmd_output *handle);
 
 static stsize_t
@@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle)
 		return;
 
 	if (handle->file_version >= FILE_VERSION_SECTIONS) {
+		/* write any unsaved options at the end of trace files with sections */
+		write_options_v7(handle);
+
 		/* write strings section */
 		save_string_section(handle);
 	}
@@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version
  */
 static int output_write_init(struct tracecmd_output *handle)
 {
+	unsigned long long offset;
 	char buf[BUFSIZ];
 	int endian4;
 
@@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle)
 	endian4 = convert_endian_4(handle, handle->page_size);
 	if (do_write_check(handle, &endian4, 4))
 		return -1;
+	if (HAS_SECTIONS(handle)) {
+		/* Write 0 as options offset and save its location */
+		offset = 0;
+		handle->options_start = do_lseek(handle, 0, SEEK_CUR);
+		if (do_write_check(handle, &offset, 8))
+			return -1;
+	}
+
 	handle->file_state = TRACECMD_FILE_INIT;
 	return 0;
 }
@@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
 	 * We can only add options before tracing data were written.
 	 * This may change in the future.
 	 */
-	if (handle->file_state > TRACECMD_FILE_OPTIONS)
+	if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS)
 		return NULL;
 
 	for (i = 0; i < count; i++)
@@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
 			return NULL;
 		}
 	}
-
-	option = malloc(sizeof(*option));
+	option = calloc(1, sizeof(*option));
 	if (!option) {
 		tracecmd_warning("Could not allocate space for option");
 		free(data);
@@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
 	return 0;
 }
 
-int tracecmd_write_options(struct tracecmd_output *handle)
+static int write_options_v6(struct tracecmd_output *handle)
 {
 	struct tracecmd_option *options;
 	unsigned short option;
@@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 
 	if (do_write_check(handle, "options  ", 10))
 		return -1;
-
+	handle->options_start = do_lseek(handle, 0, SEEK_CUR);
 	list_for_each_entry(options, &handle->options, list) {
 		endian2 = convert_endian_2(handle, options->id);
 		if (do_write_check(handle, &endian2, 2))
@@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 		return -1;
 
 	handle->file_state = TRACECMD_FILE_OPTIONS;
+	return 0;
+}
+
+static int write_options_v7(struct tracecmd_output *handle)
+{
+	struct tracecmd_option *options;
+	unsigned long long endian8;
+	unsigned short endian2;
+	unsigned int endian4;
+	bool new = false;
+	tsize_t offset;
+
+	/* Check if there are unsaved options */
+	list_for_each_entry(options, &handle->options, list) {
+		if (!options->offset) {
+			new = true;
+			break;
+		}
+	}
+
+	if (!new)
+		return 0;
+	offset = do_lseek(handle, 0, SEEK_CUR);
+	/* Append to the previous options section, if any */
+	if (handle->options_start) {
+		if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1)
+			return -1;
+		endian8 = convert_endian_8(handle, offset);
+		if (do_write_check(handle, &endian8, 8))
+			return -1;
+		if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1)
+			return -1;
+	}
+	offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false);
+	if (offset == (off_t)-1)
+		return -1;
+	list_for_each_entry(options, &handle->options, list) {
+		/* Option is already saved, skip it */
+		if (options->offset)
+			continue;
+		endian2 = convert_endian_2(handle, options->id);
+		if (do_write_check(handle, &endian2, 2))
+			return -1;
+		endian4 = convert_endian_4(handle, options->size);
+		if (do_write_check(handle, &endian4, 4))
+			return -1;
+		/* Save the data location */
+		options->offset = do_lseek(handle, 0, SEEK_CUR);
+		if (do_write_check(handle, options->data, options->size))
+			return -1;
+	}
+
+	endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE);
+	if (do_write_check(handle, &endian2, 2))
+		return -1;
+	endian4 = convert_endian_4(handle, 8);
+	if (do_write_check(handle, &endian4, 4))
+		return -1;
+	endian8 = 0;
+	handle->options_start = do_lseek(handle, 0, SEEK_CUR);
+	if (do_write_check(handle, &endian8, 8))
+		return -1;
+	if (out_update_section_header(handle, offset))
+		return -1;
 
 	return 0;
 }
@@ -1527,8 +1603,14 @@ int tracecmd_write_meta_strings(struct tracecmd_output *handle)
 	return save_string_section(handle);
 }
 
+int tracecmd_write_options(struct tracecmd_output *handle)
+{
+	if (!HAS_SECTIONS(handle))
+		return write_options_v6(handle);
+	return write_options_v7(handle);
+}
 
-int tracecmd_append_options(struct tracecmd_output *handle)
+static int append_options_v6(struct tracecmd_output *handle)
 {
 	struct tracecmd_option *options;
 	unsigned short option;
@@ -1579,6 +1661,13 @@ int tracecmd_append_options(struct tracecmd_output *handle)
 	return 0;
 }
 
+int tracecmd_append_options(struct tracecmd_output *handle)
+{
+	if (!HAS_SECTIONS(handle))
+		return append_options_v6(handle);
+	return write_options_v7(handle);
+}
+
 static struct tracecmd_option *
 add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
 {
-- 
2.33.1

Re: [PATCH v7 07/25] trace-cmd library: Add multiple options sections in trace file version 7

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:30 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

Nit, paragraphs should really be separated by spaces ;-)

> Introduced chain of options sections in trace file version 7. Extended
> the "DONE" option to hold the offset into the file to the next options
> section.

> Format of trace file version 7 is extended with a new mandatory field
> after the compression algorithm header:

>  <8 bytes>, unsigned long long integer - offset into the trace file
> where the first options section is located. This allows to place this
> section anywhere in the file. As all other sections have corresponding
> options, describing their offsets into the trace file, this change makes
> the structure of trace file version 7 flexible.

Makes it easier to read. Especially when you get older and your eyes
don't work as well.

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-output.c | 103 ++++++++++++++++++++++++++++++++---
>  1 file changed, 96 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index aa55aad7..6e07fece 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -64,7 +64,7 @@ struct tracecmd_output {
>  	unsigned long		file_version;
>  	unsigned long		strings_p;
>  	unsigned long		strings_offs;
> -	size_t			options_start;
> +	tsize_t			options_start;

I have to ask. Why use tsize_t? (I see it used elsewhere as well, but
just really noticed it now)

The only reference to tsize_t I see in the wild is for libtiff.

>  	bool			big_endian;
>  
>  	struct list_head	options;
> @@ -89,6 +89,7 @@ struct list_event_system {
>  
>  #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
>  
> +static int write_options_v7(struct tracecmd_output *handle);

I'm thinking that since v7 will be the main method, we could just
remove all the references to "v7", as we don't know if v8 will use this
as well. That is, the old versions should be labeled as such ("v6"),
but the new versions shouldn't have to be.

>  static int save_string_section(struct tracecmd_output *handle);
>  
>  static stsize_t
> @@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle)
>  		return;
>  
>  	if (handle->file_version >= FILE_VERSION_SECTIONS) {
> +		/* write any unsaved options at the end of trace files with sections */
> +		write_options_v7(handle);
> +
>  		/* write strings section */
>  		save_string_section(handle);
>  	}
> @@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version
>   */
>  static int output_write_init(struct tracecmd_output *handle)
>  {
> +	unsigned long long offset;
>  	char buf[BUFSIZ];
>  	int endian4;
>  
> @@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle)
>  	endian4 = convert_endian_4(handle, handle->page_size);
>  	if (do_write_check(handle, &endian4, 4))
>  		return -1;
> +	if (HAS_SECTIONS(handle)) {
> +		/* Write 0 as options offset and save its location */
> +		offset = 0;
> +		handle->options_start = do_lseek(handle, 0, SEEK_CUR);
> +		if (do_write_check(handle, &offset, 8))
> +			return -1;
> +	}
> +
>  	handle->file_state = TRACECMD_FILE_INIT;
>  	return 0;
>  }
> @@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
>  	 * We can only add options before tracing data were written.
>  	 * This may change in the future.
>  	 */
> -	if (handle->file_state > TRACECMD_FILE_OPTIONS)
> +	if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS)
>  		return NULL;
>  
>  	for (i = 0; i < count; i++)
> @@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
>  			return NULL;
>  		}
>  	}
> -
> -	option = malloc(sizeof(*option));
> +	option = calloc(1, sizeof(*option));
>  	if (!option) {
>  		tracecmd_warning("Could not allocate space for option");
>  		free(data);
> @@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
>  	return 0;
>  }
>  
> -int tracecmd_write_options(struct tracecmd_output *handle)
> +static int write_options_v6(struct tracecmd_output *handle)
>  {
>  	struct tracecmd_option *options;
>  	unsigned short option;
> @@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle)
>  
>  	if (do_write_check(handle, "options  ", 10))
>  		return -1;
> -
> +	handle->options_start = do_lseek(handle, 0, SEEK_CUR);
>  	list_for_each_entry(options, &handle->options, list) {
>  		endian2 = convert_endian_2(handle, options->id);
>  		if (do_write_check(handle, &endian2, 2))
> @@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle)
>  		return -1;
>  
>  	handle->file_state = TRACECMD_FILE_OPTIONS;
> +	return 0;
> +}
> +
> +static int write_options_v7(struct tracecmd_output *handle)
> +{
> +	struct tracecmd_option *options;
> +	unsigned long long endian8;
> +	unsigned short endian2;
> +	unsigned int endian4;
> +	bool new = false;
> +	tsize_t offset;
> +
> +	/* Check if there are unsaved options */
> +	list_for_each_entry(options, &handle->options, list) {
> +		if (!options->offset) {
> +			new = true;
> +			break;
> +		}
> +	}
> +
> +	if (!new)
> +		return 0;
> +	offset = do_lseek(handle, 0, SEEK_CUR);

The spacing is backwards above. You can remove the space before the
 "if (!new)" and add one after the if condition.

The if check is the result of the previous loop, and should be
associated with it, just like one would do in any language.

Code spacing is important. It should be used to show how lines of code
are associated with each other, just like paragraphs in text.

> +	/* Append to the previous options section, if any */
> +	if (handle->options_start) {
> +		if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1)
> +			return -1;
> +		endian8 = convert_endian_8(handle, offset);
> +		if (do_write_check(handle, &endian8, 8))
> +			return -1;
> +		if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1)
> +			return -1;
> +	}

space

> +	offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false);
> +	if (offset == (off_t)-1)
> +		return -1;

space.

> +	list_for_each_entry(options, &handle->options, list) {
> +		/* Option is already saved, skip it */
> +		if (options->offset)
> +			continue;
> +		endian2 = convert_endian_2(handle, options->id);
> +		if (do_write_check(handle, &endian2, 2))
> +			return -1;
> +		endian4 = convert_endian_4(handle, options->size);
> +		if (do_write_check(handle, &endian4, 4))
> +			return -1;
> +		/* Save the data location */
> +		options->offset = do_lseek(handle, 0, SEEK_CUR);
> +		if (do_write_check(handle, options->data, options->size))
> +			return -1;
> +	}
> +
> +	endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE);
> +	if (do_write_check(handle, &endian2, 2))
> +		return -1;
> +	endian4 = convert_endian_4(handle, 8);
> +	if (do_write_check(handle, &endian4, 4))
> +		return -1;
> +	endian8 = 0;
> +	handle->options_start = do_lseek(handle, 0, SEEK_CUR);
> +	if (do_write_check(handle, &endian8, 8))
> +		return -1;
> +	if (out_update_section_header(handle, offset))
> +		return -1;
>  
>  	return 0;
>  }

-- Steve

Re: [PATCH v7 07/25] trace-cmd library: Add multiple options sections in trace file version 7

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 4:57 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:30 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> Nit, paragraphs should really be separated by spaces ;-)
>
> > Introduced chain of options sections in trace file version 7. Extended
> > the "DONE" option to hold the offset into the file to the next options
> > section.
>
> > Format of trace file version 7 is extended with a new mandatory field
> > after the compression algorithm header:
>
> >  <8 bytes>, unsigned long long integer - offset into the trace file
> > where the first options section is located. This allows to place this
> > section anywhere in the file. As all other sections have corresponding
> > options, describing their offsets into the trace file, this change makes
> > the structure of trace file version 7 flexible.
>
> Makes it easier to read. Especially when you get older and your eyes
> don't work as well.
>
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  lib/trace-cmd/trace-output.c | 103 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 96 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> > index aa55aad7..6e07fece 100644
> > --- a/lib/trace-cmd/trace-output.c
> > +++ b/lib/trace-cmd/trace-output.c
> > @@ -64,7 +64,7 @@ struct tracecmd_output {
> >       unsigned long           file_version;
> >       unsigned long           strings_p;
> >       unsigned long           strings_offs;
> > -     size_t                  options_start;
> > +     tsize_t                 options_start;
>
> I have to ask. Why use tsize_t? (I see it used elsewhere as well, but
> just really noticed it now)
>
> The only reference to tsize_t I see in the wild is for libtiff.

That is the comment where tsize_t is defined, at the top of this file:
    /* We can't depend on the host size for size_t, all must be 64 bit */

As I understand, on 32 bit arch, size_t is 32 bit. "options_start" is
offset in the file, it should be always 64 bit. I can just use
"unsigned long long" instead.


>
> >       bool                    big_endian;
> >
> >       struct list_head        options;
> > @@ -89,6 +89,7 @@ struct list_event_system {
> >
> >  #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS)
> >
> > +static int write_options_v7(struct tracecmd_output *handle);
>
> I'm thinking that since v7 will be the main method, we could just
> remove all the references to "v7", as we don't know if v8 will use this
> as well. That is, the old versions should be labeled as such ("v6"),
> but the new versions shouldn't have to be.
>
> >  static int save_string_section(struct tracecmd_output *handle);
> >
> >  static stsize_t
> > @@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle)
> >               return;
> >
> >       if (handle->file_version >= FILE_VERSION_SECTIONS) {
> > +             /* write any unsaved options at the end of trace files with sections */
> > +             write_options_v7(handle);
> > +
> >               /* write strings section */
> >               save_string_section(handle);
> >       }
> > @@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version
> >   */
> >  static int output_write_init(struct tracecmd_output *handle)
> >  {
> > +     unsigned long long offset;
> >       char buf[BUFSIZ];
> >       int endian4;
> >
> > @@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle)
> >       endian4 = convert_endian_4(handle, handle->page_size);
> >       if (do_write_check(handle, &endian4, 4))
> >               return -1;
> > +     if (HAS_SECTIONS(handle)) {
> > +             /* Write 0 as options offset and save its location */
> > +             offset = 0;
> > +             handle->options_start = do_lseek(handle, 0, SEEK_CUR);
> > +             if (do_write_check(handle, &offset, 8))
> > +                     return -1;
> > +     }
> > +
> >       handle->file_state = TRACECMD_FILE_INIT;
> >       return 0;
> >  }
> > @@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
> >        * We can only add options before tracing data were written.
> >        * This may change in the future.
> >        */
> > -     if (handle->file_state > TRACECMD_FILE_OPTIONS)
> > +     if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS)
> >               return NULL;
> >
> >       for (i = 0; i < count; i++)
> > @@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle,
> >                       return NULL;
> >               }
> >       }
> > -
> > -     option = malloc(sizeof(*option));
> > +     option = calloc(1, sizeof(*option));
> >       if (!option) {
> >               tracecmd_warning("Could not allocate space for option");
> >               free(data);
> > @@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
> >       return 0;
> >  }
> >
> > -int tracecmd_write_options(struct tracecmd_output *handle)
> > +static int write_options_v6(struct tracecmd_output *handle)
> >  {
> >       struct tracecmd_option *options;
> >       unsigned short option;
> > @@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle)
> >
> >       if (do_write_check(handle, "options  ", 10))
> >               return -1;
> > -
> > +     handle->options_start = do_lseek(handle, 0, SEEK_CUR);
> >       list_for_each_entry(options, &handle->options, list) {
> >               endian2 = convert_endian_2(handle, options->id);
> >               if (do_write_check(handle, &endian2, 2))
> > @@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle)
> >               return -1;
> >
> >       handle->file_state = TRACECMD_FILE_OPTIONS;
> > +     return 0;
> > +}
> > +
> > +static int write_options_v7(struct tracecmd_output *handle)
> > +{
> > +     struct tracecmd_option *options;
> > +     unsigned long long endian8;
> > +     unsigned short endian2;
> > +     unsigned int endian4;
> > +     bool new = false;
> > +     tsize_t offset;
> > +
> > +     /* Check if there are unsaved options */
> > +     list_for_each_entry(options, &handle->options, list) {
> > +             if (!options->offset) {
> > +                     new = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (!new)
> > +             return 0;
> > +     offset = do_lseek(handle, 0, SEEK_CUR);
>
> The spacing is backwards above. You can remove the space before the
>  "if (!new)" and add one after the if condition.
>
> The if check is the result of the previous loop, and should be
> associated with it, just like one would do in any language.
>
> Code spacing is important. It should be used to show how lines of code
> are associated with each other, just like paragraphs in text.
>
> > +     /* Append to the previous options section, if any */
> > +     if (handle->options_start) {
> > +             if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1)
> > +                     return -1;
> > +             endian8 = convert_endian_8(handle, offset);
> > +             if (do_write_check(handle, &endian8, 8))
> > +                     return -1;
> > +             if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1)
> > +                     return -1;
> > +     }
>
> space
>
> > +     offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false);
> > +     if (offset == (off_t)-1)
> > +             return -1;
>
> space.
>
> > +     list_for_each_entry(options, &handle->options, list) {
> > +             /* Option is already saved, skip it */
> > +             if (options->offset)
> > +                     continue;
> > +             endian2 = convert_endian_2(handle, options->id);
> > +             if (do_write_check(handle, &endian2, 2))
> > +                     return -1;
> > +             endian4 = convert_endian_4(handle, options->size);
> > +             if (do_write_check(handle, &endian4, 4))
> > +                     return -1;
> > +             /* Save the data location */
> > +             options->offset = do_lseek(handle, 0, SEEK_CUR);
> > +             if (do_write_check(handle, options->data, options->size))
> > +                     return -1;
> > +     }
> > +
> > +     endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE);
> > +     if (do_write_check(handle, &endian2, 2))
> > +             return -1;
> > +     endian4 = convert_endian_4(handle, 8);
> > +     if (do_write_check(handle, &endian4, 4))
> > +             return -1;
> > +     endian8 = 0;
> > +     handle->options_start = do_lseek(handle, 0, SEEK_CUR);
> > +     if (do_write_check(handle, &endian8, 8))
> > +             return -1;
> > +     if (out_update_section_header(handle, offset))
> > +             return -1;
> >
> >       return 0;
> >  }
>
> -- Steve



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

[PATCH v7 08/25] trace-cmd library: Do not write CPU count section in trace files version 7

[Tzvetomir Stoyanov (VMware)]

Writing CPU count as a separate section in the trace file is redundant,
as there is already an option for that. Use that option in trace files
version 7.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 6e07fece..59093be2 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1477,10 +1477,16 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
 				 handle->file_state);
 		return -1;
 	}
-	cpus = convert_endian_4(handle, cpus);
-	ret = do_write_check(handle, &cpus, 4);
-	if (ret < 0)
-		return ret;
+
+	if (!HAS_SECTIONS(handle)) {
+		cpus = convert_endian_4(handle, cpus);
+		ret = do_write_check(handle, &cpus, 4);
+		if (ret < 0)
+			return ret;
+	} else {
+		tracecmd_add_option(handle, TRACECMD_OPTION_CPUCOUNT, sizeof(int), &cpus);
+	}
+
 	handle->file_state = TRACECMD_FILE_CPU_COUNT;
 	return 0;
 }
-- 
2.33.1

[PATCH v7 09/25] trace-cmd library: Move CPU flyrecord trace metadata into the buffer option, for trace file version 7

[Tzvetomir Stoyanov (VMware)]

Extended the BUFFER trace option in trace file version 7 with CPU
flyrecord trace metadata. In the version 6 of the trace file, this metadata
is located at several places in the file. Part of the metadata is only for
the top trace instance, thus limiting per-instance configuration. Moving
all CPU trace related metadata in the BUFFER option simplifies the parsing
and makes per-instance configuration more flexible. In the new file
structure, the top instance is treated as any other instances.
The format of the extended BUFFER option is:
 - offset of the buffer in the trace file
 - name of the buffer
 - trace clock, used in this buffer for events timestamps
 - count of CPUs with trace data
 - array, describing each CPU with trace data:
   - CPU id
   - offset of CPU trace data in the trace file
   - size of the recorded CPU trace data

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 59093be2..6e40c929 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1675,7 +1675,7 @@ int tracecmd_append_options(struct tracecmd_output *handle)
 }
 
 static struct tracecmd_option *
-add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
+add_buffer_option_v6(struct tracecmd_output *handle, const char *name, int cpus)
 {
 	struct tracecmd_option *option;
 	char *buf;
@@ -1725,8 +1725,11 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle)
 	struct tracecmd_option *option;
 	struct tracecmd_buffer *buf;
 
+	if (HAS_SECTIONS(handle))
+		return 0;
+
 	list_for_each_entry(buf, &handle->buffers, list) {
-		option = add_buffer_option(handle, buf->name, buf->cpus);
+		option = add_buffer_option_v6(handle, buf->name, buf->cpus);
 		if (!option)
 			return -1;
 		buf->option = option;
@@ -1777,6 +1780,98 @@ int tracecmd_write_cmdlines(struct tracecmd_output *handle)
 	return 0;
 }
 
+static char *get_clock(struct tracecmd_output *handle)
+{
+	struct tracefs_instance *inst;
+
+	if (handle->trace_clock)
+		return handle->trace_clock;
+
+	/*
+	 * If no clock is set on this handle, get the trace clock of
+	 * the top instance in the handle's tracing dir
+	 */
+	inst = tracefs_instance_alloc(handle->tracing_dir, NULL);
+	if (!inst)
+		return NULL;
+	handle->trace_clock = tracefs_get_clock(inst);
+	tracefs_instance_free(inst);
+	return handle->trace_clock;
+}
+
+__hidden struct tracecmd_option *
+out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
+			 unsigned short id, unsigned long long data_offset,
+			 int cpus, struct data_file_write *cpu_data)
+{
+	struct tracecmd_option *option;
+	int i, j = 0, k = 0;
+	int *cpu_ids = NULL;
+	struct iovec *vect;
+	char *clock;
+
+	if (!HAS_SECTIONS(handle))
+		return NULL;
+
+	clock = get_clock(handle);
+
+	/* Buffer flyrecord option, v7:
+	 *  - trace data offset in the file
+	 *  - buffer name
+	 *  - buffer clock
+	 *  - CPU count
+	 *  - for each CPU:
+	 *    - CPU id
+	 *    - CPU trace data offset in the file
+	 *    - CPU trace data size
+	 */
+
+	/* Buffer latency option, v7:
+	 *  - trace data offset in the file
+	 *  - buffer name
+	 *  - buffer clock
+	 */
+
+	vect = calloc(5 + (cpus * 3), sizeof(struct iovec));
+	if (!vect)
+		return NULL;
+	if (cpus) {
+		cpu_ids = calloc(cpus, sizeof(int));
+		if (!cpu_ids) {
+			free(vect);
+			return NULL;
+		}
+	}
+	vect[j].iov_base = (void *) &data_offset;
+	vect[j++].iov_len = 8;
+	vect[j].iov_base = (void *) name;
+	vect[j++].iov_len = strlen(name) + 1;
+	vect[j].iov_base = (void *) clock;
+	vect[j++].iov_len = strlen(clock) + 1;
+	if (id == TRACECMD_OPTION_BUFFER) {
+		vect[j].iov_base = (void *) &k;
+		vect[j++].iov_len = 4;
+		for (i = 0; i < cpus; i++) {
+			if (!cpu_data[i].file_size)
+				continue;
+			cpu_ids[i] = i;
+			vect[j].iov_base = &cpu_ids[i];
+			vect[j++].iov_len = 4;
+			vect[j].iov_base = &cpu_data[i].data_offset;
+			vect[j++].iov_len = 8;
+			vect[j].iov_base = &cpu_data[i].write_size;
+			vect[j++].iov_len = 8;
+			k++;
+		}
+	}
+
+	option = tracecmd_add_option_v(handle, id, vect, j);
+	free(vect);
+	free(cpu_ids);
+
+	return option;
+}
+
 struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
 {
 	struct tracecmd_output *handle;
@@ -1847,8 +1942,8 @@ out:
 	return ret;
 }
 
-static int update_buffer_cpu_offset(struct tracecmd_output *handle,
-				    const char *name, tsize_t offset)
+static int update_buffer_cpu_offset_v6(struct tracecmd_output *handle,
+				       const char *name, tsize_t offset)
 {
 	tsize_t b_offset;
 	tsize_t current;
@@ -1881,26 +1976,6 @@ static int update_buffer_cpu_offset(struct tracecmd_output *handle,
 	return 0;
 }
 
-
-static char *get_clock(struct tracecmd_output *handle)
-{
-	struct tracefs_instance *inst;
-
-	if (handle->trace_clock)
-		return handle->trace_clock;
-
-	/*
-	 * If no clock is set on this handle, get the trace clock of
-	 * the top instance in the handle's tracing dir
-	 */
-	inst = tracefs_instance_alloc(handle->tracing_dir, NULL);
-	if (!inst)
-		return NULL;
-	handle->trace_clock = tracefs_get_clock(inst);
-	tracefs_instance_free(inst);
-	return handle->trace_clock;
-}
-
 __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 				int cpus, struct cpu_data_source *data, const char *buff_name)
 {
@@ -1924,7 +1999,7 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 	}
 
 	data_offs = do_lseek(handle, 0, SEEK_CUR);
-	if (do_write_check(handle, "flyrecord", 10))
+	if (!HAS_SECTIONS(handle) && do_write_check(handle, "flyrecord", 10))
 		goto out_free;
 
 	data_files = calloc(cpus, sizeof(*data_files));
@@ -1937,19 +2012,23 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 		 * 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].file_data_offset = do_lseek(handle, 0, SEEK_CUR);
-		if (do_write_check(handle, &endian8, 8))
-			goto out_free;
-		data_files[i].file_write_size = do_lseek(handle, 0, SEEK_CUR);
-		if (do_write_check(handle, &endian8, 8))
-			goto out_free;
+		if (!HAS_SECTIONS(handle)) {
+			endian8 = 0;
+			data_files[i].file_data_offset = do_lseek(handle, 0, SEEK_CUR);
+			if (do_write_check(handle, &endian8, 8))
+				goto out_free;
+			data_files[i].file_write_size = do_lseek(handle, 0, SEEK_CUR);
+			if (do_write_check(handle, &endian8, 8))
+				goto out_free;
+		}
 	}
 
-	update_buffer_cpu_offset(handle, buff_name, data_offs);
-	clock = get_clock(handle);
-	if (clock && save_clock(handle, clock))
-		goto out_free;
+	if (!HAS_SECTIONS(handle)) {
+		update_buffer_cpu_offset_v6(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 = do_lseek(handle, 0, SEEK_CUR);
@@ -1980,29 +2059,33 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 			data_files[i].write_size = 0;
 		}
 
-		/* Write the real CPU data offset in the file */
-		if (do_lseek(handle, data_files[i].file_data_offset, 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 (do_lseek(handle, data_files[i].file_write_size, 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 (do_lseek(handle, offset, SEEK_SET) == (off64_t)-1)
-			goto out_free;
-
+		if (!HAS_SECTIONS(handle)) {
+			/* Write the real CPU data offset in the file */
+			if (do_lseek(handle, data_files[i].file_data_offset, 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 (do_lseek(handle, data_files[i].file_write_size, 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 (do_lseek(handle, offset, SEEK_SET) == (off64_t)-1)
+				goto out_free;
+		}
 		if (!tracecmd_get_quiet(handle))
 			fprintf(stderr, "    %llu bytes in size\n",
 				(unsigned long long)data_files[i].write_size);
 	}
 
+	if (HAS_SECTIONS(handle) &&
+	    !out_add_buffer_option_v7(handle, buff_name,
+				      TRACECMD_OPTION_BUFFER, data_offs, cpus, data_files))
+		goto out_free;
+
 	free(data_files);
 	if (do_lseek(handle, 0, SEEK_END) == (off64_t)-1)
 		return -1;
-- 
2.33.1

Re: [PATCH v7 09/25] trace-cmd library: Move CPU flyrecord trace metadata into the buffer option, for trace file version 7

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:32 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

I'm going to keep mentioning spacing ;-)

> Extended the BUFFER trace option in trace file version 7 with CPU
> flyrecord trace metadata. In the version 6 of the trace file, this metadata
> is located at several places in the file. Part of the metadata is only for
> the top trace instance, thus limiting per-instance configuration. Moving
> all CPU trace related metadata in the BUFFER option simplifies the parsing
> and makes per-instance configuration more flexible. In the new file
> structure, the top instance is treated as any other instances.

space

> The format of the extended BUFFER option is:
>  - offset of the buffer in the trace file
>  - name of the buffer
>  - trace clock, used in this buffer for events timestamps
>  - count of CPUs with trace data
>  - array, describing each CPU with trace data:
>    - CPU id
>    - offset of CPU trace data in the trace file
>    - size of the recorded CPU trace data
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-output.c | 191 +++++++++++++++++++++++++----------
>  1 file changed, 137 insertions(+), 54 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 59093be2..6e40c929 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -1675,7 +1675,7 @@ int tracecmd_append_options(struct tracecmd_output *handle)
>  }
>  
>  static struct tracecmd_option *
> -add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
> +add_buffer_option_v6(struct tracecmd_output *handle, const char *name, int cpus)
>  {
>  	struct tracecmd_option *option;
>  	char *buf;
> @@ -1725,8 +1725,11 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle)
>  	struct tracecmd_option *option;
>  	struct tracecmd_buffer *buf;
>  
> +	if (HAS_SECTIONS(handle))
> +		return 0;
> +
>  	list_for_each_entry(buf, &handle->buffers, list) {
> -		option = add_buffer_option(handle, buf->name, buf->cpus);
> +		option = add_buffer_option_v6(handle, buf->name, buf->cpus);
>  		if (!option)
>  			return -1;
>  		buf->option = option;
> @@ -1777,6 +1780,98 @@ int tracecmd_write_cmdlines(struct tracecmd_output *handle)
>  	return 0;
>  }
>  
> +static char *get_clock(struct tracecmd_output *handle)
> +{
> +	struct tracefs_instance *inst;
> +
> +	if (handle->trace_clock)
> +		return handle->trace_clock;
> +
> +	/*
> +	 * If no clock is set on this handle, get the trace clock of
> +	 * the top instance in the handle's tracing dir
> +	 */
> +	inst = tracefs_instance_alloc(handle->tracing_dir, NULL);
> +	if (!inst)
> +		return NULL;
> +	handle->trace_clock = tracefs_get_clock(inst);

Why allocate an instance, doesn't:

	tracefs_get_clock(NULL);

do the same? (at least the man page says it does).

Or are you worried that this has to do something with the tracing_dir?
But do we care?

> +	tracefs_instance_free(inst);
> +	return handle->trace_clock;
> +}
> +
> +__hidden struct tracecmd_option *
> +out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,

Again, I think it's safe to remove all the references to v7, and just
have that be the default.

> +			 unsigned short id, unsigned long long data_offset,
> +			 int cpus, struct data_file_write *cpu_data)
> +{
> +	struct tracecmd_option *option;
> +	int i, j = 0, k = 0;
> +	int *cpu_ids = NULL;
> +	struct iovec *vect;
> +	char *clock;
> +
> +	if (!HAS_SECTIONS(handle))
> +		return NULL;
> +
> +	clock = get_clock(handle);
> +
> +	/* Buffer flyrecord option, v7:

Including here (no need to say "v7")

> +	 *  - trace data offset in the file
> +	 *  - buffer name
> +	 *  - buffer clock
> +	 *  - CPU count
> +	 *  - for each CPU:
> +	 *    - CPU id
> +	 *    - CPU trace data offset in the file
> +	 *    - CPU trace data size
> +	 */
> +
> +	/* Buffer latency option, v7:

and here.

> +	 *  - trace data offset in the file
> +	 *  - buffer name
> +	 *  - buffer clock
> +	 */
> +
> +	vect = calloc(5 + (cpus * 3), sizeof(struct iovec));

What's with the magical 5 and 3 numbers above?

As from the comments above, I only count 4 and 3.

 4 : offset, name, clock, count
 3 : cpu offset, name, clock

Could include the above in a comment too.

-- Steve

> +	if (!vect)
> +		return NULL;
> +	if (cpus) {
> +		cpu_ids = calloc(cpus, sizeof(int));
> +		if (!cpu_ids) {
> +			free(vect);
> +			return NULL;
> +		}
> +	}
> +	vect[j].iov_base = (void *) &data_offset;
> +	vect[j++].iov_len = 8;
> +	vect[j].iov_base = (void *) name;
> +	vect[j++].iov_len = strlen(name) + 1;
> +	vect[j].iov_base = (void *) clock;
> +	vect[j++].iov_len = strlen(clock) + 1;
> +	if (id == TRACECMD_OPTION_BUFFER) {
> +		vect[j].iov_base = (void *) &k;
> +		vect[j++].iov_len = 4;
> +		for (i = 0; i < cpus; i++) {
> +			if (!cpu_data[i].file_size)
> +				continue;
> +			cpu_ids[i] = i;
> +			vect[j].iov_base = &cpu_ids[i];
> +			vect[j++].iov_len = 4;
> +			vect[j].iov_base = &cpu_data[i].data_offset;
> +			vect[j++].iov_len = 8;
> +			vect[j].iov_base = &cpu_data[i].write_size;
> +			vect[j++].iov_len = 8;
> +			k++;
> +		}
> +	}
> +
> +	option = tracecmd_add_option_v(handle, id, vect, j);
> +	free(vect);
> +	free(cpu_ids);
> +
> +	return option;
> +}
> +
>  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
>  {
>  	struct tracecmd_output *handle;
> @@ -1847,8 +1942,8 @@ out:
>  	return ret;
>  }

Re: [PATCH v7 09/25] trace-cmd library: Move CPU flyrecord trace metadata into the buffer option, for trace file version 7

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 5:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:32 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> I'm going to keep mentioning spacing ;-)
>
> > Extended the BUFFER trace option in trace file version 7 with CPU
> > flyrecord trace metadata. In the version 6 of the trace file, this metadata
> > is located at several places in the file. Part of the metadata is only for
> > the top trace instance, thus limiting per-instance configuration. Moving
> > all CPU trace related metadata in the BUFFER option simplifies the parsing
> > and makes per-instance configuration more flexible. In the new file
> > structure, the top instance is treated as any other instances.
>
> space
>
> > The format of the extended BUFFER option is:
> >  - offset of the buffer in the trace file
> >  - name of the buffer
> >  - trace clock, used in this buffer for events timestamps
> >  - count of CPUs with trace data
> >  - array, describing each CPU with trace data:
> >    - CPU id
> >    - offset of CPU trace data in the trace file
> >    - size of the recorded CPU trace data
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  lib/trace-cmd/trace-output.c | 191 +++++++++++++++++++++++++----------
> >  1 file changed, 137 insertions(+), 54 deletions(-)
> >
> > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> > index 59093be2..6e40c929 100644
> > --- a/lib/trace-cmd/trace-output.c
> > +++ b/lib/trace-cmd/trace-output.c
> > @@ -1675,7 +1675,7 @@ int tracecmd_append_options(struct tracecmd_output *handle)
> >  }
> >
> >  static struct tracecmd_option *
> > -add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
> > +add_buffer_option_v6(struct tracecmd_output *handle, const char *name, int cpus)
> >  {
> >       struct tracecmd_option *option;
> >       char *buf;
> > @@ -1725,8 +1725,11 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle)
> >       struct tracecmd_option *option;
> >       struct tracecmd_buffer *buf;
> >
> > +     if (HAS_SECTIONS(handle))
> > +             return 0;
> > +
> >       list_for_each_entry(buf, &handle->buffers, list) {
> > -             option = add_buffer_option(handle, buf->name, buf->cpus);
> > +             option = add_buffer_option_v6(handle, buf->name, buf->cpus);
> >               if (!option)
> >                       return -1;
> >               buf->option = option;
> > @@ -1777,6 +1780,98 @@ int tracecmd_write_cmdlines(struct tracecmd_output *handle)
> >       return 0;
> >  }
> >
> > +static char *get_clock(struct tracecmd_output *handle)
> > +{
> > +     struct tracefs_instance *inst;
> > +
> > +     if (handle->trace_clock)
> > +             return handle->trace_clock;
> > +
> > +     /*
> > +      * If no clock is set on this handle, get the trace clock of
> > +      * the top instance in the handle's tracing dir
> > +      */
> > +     inst = tracefs_instance_alloc(handle->tracing_dir, NULL);
> > +     if (!inst)
> > +             return NULL;
> > +     handle->trace_clock = tracefs_get_clock(inst);
>
> Why allocate an instance, doesn't:
>
>         tracefs_get_clock(NULL);
>
> do the same? (at least the man page says it does).
>
> Or are you worried that this has to do something with the tracing_dir?
> But do we care?

Yes, because of tracing_dir. I'll add a check:

if (!handle->tracing_dir) {
      handle->trace_clock = tracefs_get_clock(NULL);
      return handle->trace_clock;
}

which is the most common case. If there is tracing_dir associated with
the handle, the old logic should be used - just to be on the safe
side.

>
> > +     tracefs_instance_free(inst);
> > +     return handle->trace_clock;
> > +}
> > +
> > +__hidden struct tracecmd_option *
> > +out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
>
> Again, I think it's safe to remove all the references to v7, and just
> have that be the default.
>
> > +                      unsigned short id, unsigned long long data_offset,
> > +                      int cpus, struct data_file_write *cpu_data)
> > +{
> > +     struct tracecmd_option *option;
> > +     int i, j = 0, k = 0;
> > +     int *cpu_ids = NULL;
> > +     struct iovec *vect;
> > +     char *clock;
> > +
> > +     if (!HAS_SECTIONS(handle))
> > +             return NULL;
> > +
> > +     clock = get_clock(handle);
> > +
> > +     /* Buffer flyrecord option, v7:
>
> Including here (no need to say "v7")
>
> > +      *  - trace data offset in the file
> > +      *  - buffer name
> > +      *  - buffer clock
> > +      *  - CPU count
> > +      *  - for each CPU:
> > +      *    - CPU id
> > +      *    - CPU trace data offset in the file
> > +      *    - CPU trace data size
> > +      */
> > +
> > +     /* Buffer latency option, v7:
>
> and here.
>
> > +      *  - trace data offset in the file
> > +      *  - buffer name
> > +      *  - buffer clock
> > +      */
> > +
> > +     vect = calloc(5 + (cpus * 3), sizeof(struct iovec));
>
> What's with the magical 5 and 3 numbers above?
>
> As from the comments above, I only count 4 and 3.
>
>  4 : offset, name, clock, count
>  3 : cpu offset, name, clock
>
> Could include the above in a comment too.
>
> -- Steve
>
> > +     if (!vect)
> > +             return NULL;
> > +     if (cpus) {
> > +             cpu_ids = calloc(cpus, sizeof(int));
> > +             if (!cpu_ids) {
> > +                     free(vect);
> > +                     return NULL;
> > +             }
> > +     }
> > +     vect[j].iov_base = (void *) &data_offset;
> > +     vect[j++].iov_len = 8;
> > +     vect[j].iov_base = (void *) name;
> > +     vect[j++].iov_len = strlen(name) + 1;
> > +     vect[j].iov_base = (void *) clock;
> > +     vect[j++].iov_len = strlen(clock) + 1;
> > +     if (id == TRACECMD_OPTION_BUFFER) {
> > +             vect[j].iov_base = (void *) &k;
> > +             vect[j++].iov_len = 4;
> > +             for (i = 0; i < cpus; i++) {
> > +                     if (!cpu_data[i].file_size)
> > +                             continue;
> > +                     cpu_ids[i] = i;
> > +                     vect[j].iov_base = &cpu_ids[i];
> > +                     vect[j++].iov_len = 4;
> > +                     vect[j].iov_base = &cpu_data[i].data_offset;
> > +                     vect[j++].iov_len = 8;
> > +                     vect[j].iov_base = &cpu_data[i].write_size;
> > +                     vect[j++].iov_len = 8;
> > +                     k++;
> > +             }
> > +     }
> > +
> > +     option = tracecmd_add_option_v(handle, id, vect, j);
> > +     free(vect);
> > +     free(cpu_ids);
> > +
> > +     return option;
> > +}
> > +
> >  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
> >  {
> >       struct tracecmd_output *handle;
> > @@ -1847,8 +1942,8 @@ out:
> >       return ret;
> >  }



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

[PATCH v7 10/25] trace-cmd library: Add section header before flyrecord trace data

[Tzvetomir Stoyanov (VMware)]

In trace file version 7, write a section header at the beginning of each
section with flyrecord trace data.

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

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 6e40c929..44050dc8 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1980,10 +1980,12 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 				int cpus, struct cpu_data_source *data, const char *buff_name)
 {
 	struct data_file_write *data_files = NULL;
+	enum tracecmd_section_flags flags = 0;
 	tsize_t data_offs, offset;
 	unsigned long long endian8;
 	unsigned long long read_size;
 	char *clock;
+	char *str;
 	int ret;
 	int i;
 
@@ -2002,6 +2004,13 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 	if (!HAS_SECTIONS(handle) && do_write_check(handle, "flyrecord", 10))
 		goto out_free;
 
+	if (asprintf(&str, "buffer flyrecord %s", buff_name) < 1)
+		goto out_free;
+	offset = out_write_section_header(handle, TRACECMD_OPTION_BUFFER, str, flags, false);
+	free(str);
+	if (offset == (off_t)-1)
+		goto out_free;
+
 	data_files = calloc(cpus, sizeof(*data_files));
 	if (!data_files)
 		goto out_free;
@@ -2090,6 +2099,9 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
 	if (do_lseek(handle, 0, SEEK_END) == (off64_t)-1)
 		return -1;
 
+	if (out_update_section_header(handle, offset))
+		goto out_free;
+
 	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
 
 	return 0;
-- 
2.33.1

[PATCH v7 11/25] trace-cmd library: Fit CPU latency trace data in the new trace file version 7 format

[Tzvetomir Stoyanov (VMware)]

Trace file version 7 format is based on sections. To fit the latency
trace data in this structure, a new section and option for it is
defined:
  BUFFER_TEXT
It is similar to the BUFFER section which holds the flyrecord binary
data, but has a latency specific design for text data. The BUFFER_TEXT
section has:
 - section header, as all other sections
 - compression of the trace data, optional
 - corresponding trace option, pointing to the section

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  1 +
 lib/trace-cmd/trace-output.c                  | 24 ++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 047fc26f..d8ee9b74 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -145,6 +145,7 @@ enum {
 	TRACECMD_OPTION_KALLSYMS,
 	TRACECMD_OPTION_PRINTK,
 	TRACECMD_OPTION_CMDLINES,
+	TRACECMD_OPTION_BUFFER_TEXT,
 	TRACECMD_OPTION_MAX,
 };
 
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 44050dc8..47227728 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1874,7 +1874,9 @@ out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
 
 struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
 {
+	enum tracecmd_section_flags flags = 0;
 	struct tracecmd_output *handle;
+	tsize_t offset;
 	char *path;
 
 	handle = tracecmd_output_create(output_file);
@@ -1891,7 +1893,8 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 
 	if (tracecmd_write_cpus(handle, cpus) < 0)
 		goto out_free;
-
+	if (tracecmd_write_buffer_info(handle) < 0)
+		goto out_free;
 	if (tracecmd_write_options(handle) < 0)
 		goto out_free;
 
@@ -1901,23 +1904,38 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 		goto out_free;
 	}
 
-	if (do_write_check(handle, "latency  ", 10))
+	if (!HAS_SECTIONS(handle) && do_write_check(handle, "latency  ", 10))
 		goto out_free;
 
 	path = get_tracing_file(handle, "trace");
 	if (!path)
 		goto out_free;
 
+	offset = do_lseek(handle, 0, SEEK_CUR);
+	if (HAS_SECTIONS(handle) &&
+	    !out_add_buffer_option_v7(handle, "", TRACECMD_OPTION_BUFFER_TEXT, offset, 0, NULL))
+		goto out_free;
+
+	offset = out_write_section_header(handle, TRACECMD_OPTION_BUFFER_TEXT,
+					  "buffer latency", flags, false);
+
 	copy_file(handle, path);
+	if (out_update_section_header(handle, offset))
+		goto out_free;
 
 	put_tracing_file(path);
 
 	handle->file_state = TRACECMD_FILE_CPU_LATENCY;
 
+	if (HAS_SECTIONS(handle))
+		tracecmd_write_options(handle);
+
 	return handle;
 
 out_free:
-	tracecmd_output_close(handle);
+	if (handle)
+		tracecmd_output_close(handle);
+	unlink(output_file);
 	return NULL;
 }
 
-- 
2.33.1

Re: [PATCH v7 11/25] trace-cmd library: Fit CPU latency trace data in the new trace file version 7 format

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:34 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

More nitpicking about spacing ;-)

> Trace file version 7 format is based on sections. To fit the latency
> trace data in this structure, a new section and option for it is
> defined:
>   BUFFER_TEXT

space

> It is similar to the BUFFER section which holds the flyrecord binary
> data, but has a latency specific design for text data. The BUFFER_TEXT
> section has:
>  - section header, as all other sections
>  - compression of the trace data, optional
>  - corresponding trace option, pointing to the section
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  .../include/private/trace-cmd-private.h       |  1 +
>  lib/trace-cmd/trace-output.c                  | 24 ++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 047fc26f..d8ee9b74 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -145,6 +145,7 @@ enum {
>  	TRACECMD_OPTION_KALLSYMS,
>  	TRACECMD_OPTION_PRINTK,
>  	TRACECMD_OPTION_CMDLINES,
> +	TRACECMD_OPTION_BUFFER_TEXT,
>  	TRACECMD_OPTION_MAX,
>  };
>  
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 44050dc8..47227728 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -1874,7 +1874,9 @@ out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
>  
>  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
>  {
> +	enum tracecmd_section_flags flags = 0;
>  	struct tracecmd_output *handle;
> +	tsize_t offset;
>  	char *path;
>  
>  	handle = tracecmd_output_create(output_file);
> @@ -1891,7 +1893,8 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
>  
>  	if (tracecmd_write_cpus(handle, cpus) < 0)
>  		goto out_free;
> -
> +	if (tracecmd_write_buffer_info(handle) < 0)
> +		goto out_free;
>  	if (tracecmd_write_options(handle) < 0)
>  		goto out_free;
>  
> @@ -1901,23 +1904,38 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
>  		goto out_free;
>  	}
>  
> -	if (do_write_check(handle, "latency  ", 10))
> +	if (!HAS_SECTIONS(handle) && do_write_check(handle, "latency  ", 10))
>  		goto out_free;
>  
>  	path = get_tracing_file(handle, "trace");
>  	if (!path)
>  		goto out_free;
>  
> +	offset = do_lseek(handle, 0, SEEK_CUR);
> +	if (HAS_SECTIONS(handle) &&
> +	    !out_add_buffer_option_v7(handle, "", TRACECMD_OPTION_BUFFER_TEXT, offset, 0, NULL))
> +		goto out_free;
> +
> +	offset = out_write_section_header(handle, TRACECMD_OPTION_BUFFER_TEXT,
> +					  "buffer latency", flags, false);
> +
>  	copy_file(handle, path);
> +	if (out_update_section_header(handle, offset))
> +		goto out_free;
>  
>  	put_tracing_file(path);
>  
>  	handle->file_state = TRACECMD_FILE_CPU_LATENCY;
>  
> +	if (HAS_SECTIONS(handle))
> +		tracecmd_write_options(handle);
> +
>  	return handle;


>  
>  out_free:
> -	tracecmd_output_close(handle);
> +	if (handle)
> +		tracecmd_output_close(handle);
> +	unlink(output_file);

Hmm, how does the above play a role in this patch?

That is, what about this new BUFFER_TEXT required this change?
I mean, output_file is being removed now, but I don't see anything in
the rest of the patch to warrant that?

-- Steve


>  	return NULL;
>  }
>  

Re: [PATCH v7 11/25] trace-cmd library: Fit CPU latency trace data in the new trace file version 7 format

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 5:20 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:34 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> More nitpicking about spacing ;-)
>
> > Trace file version 7 format is based on sections. To fit the latency
> > trace data in this structure, a new section and option for it is
> > defined:
> >   BUFFER_TEXT
>
> space
>
> > It is similar to the BUFFER section which holds the flyrecord binary
> > data, but has a latency specific design for text data. The BUFFER_TEXT
> > section has:
> >  - section header, as all other sections
> >  - compression of the trace data, optional
> >  - corresponding trace option, pointing to the section
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  .../include/private/trace-cmd-private.h       |  1 +
> >  lib/trace-cmd/trace-output.c                  | 24 ++++++++++++++++---
> >  2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> > index 047fc26f..d8ee9b74 100644
> > --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> > @@ -145,6 +145,7 @@ enum {
> >       TRACECMD_OPTION_KALLSYMS,
> >       TRACECMD_OPTION_PRINTK,
> >       TRACECMD_OPTION_CMDLINES,
> > +     TRACECMD_OPTION_BUFFER_TEXT,
> >       TRACECMD_OPTION_MAX,
> >  };
> >
> > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> > index 44050dc8..47227728 100644
> > --- a/lib/trace-cmd/trace-output.c
> > +++ b/lib/trace-cmd/trace-output.c
> > @@ -1874,7 +1874,9 @@ out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
> >
> >  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
> >  {
> > +     enum tracecmd_section_flags flags = 0;
> >       struct tracecmd_output *handle;
> > +     tsize_t offset;
> >       char *path;
> >
> >       handle = tracecmd_output_create(output_file);
> > @@ -1891,7 +1893,8 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
> >
> >       if (tracecmd_write_cpus(handle, cpus) < 0)
> >               goto out_free;
> > -
> > +     if (tracecmd_write_buffer_info(handle) < 0)
> > +             goto out_free;
> >       if (tracecmd_write_options(handle) < 0)
> >               goto out_free;
> >
> > @@ -1901,23 +1904,38 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
> >               goto out_free;
> >       }
> >
> > -     if (do_write_check(handle, "latency  ", 10))
> > +     if (!HAS_SECTIONS(handle) && do_write_check(handle, "latency  ", 10))
> >               goto out_free;
> >
> >       path = get_tracing_file(handle, "trace");
> >       if (!path)
> >               goto out_free;
> >
> > +     offset = do_lseek(handle, 0, SEEK_CUR);
> > +     if (HAS_SECTIONS(handle) &&
> > +         !out_add_buffer_option_v7(handle, "", TRACECMD_OPTION_BUFFER_TEXT, offset, 0, NULL))
> > +             goto out_free;
> > +
> > +     offset = out_write_section_header(handle, TRACECMD_OPTION_BUFFER_TEXT,
> > +                                       "buffer latency", flags, false);
> > +
> >       copy_file(handle, path);
> > +     if (out_update_section_header(handle, offset))
> > +             goto out_free;
> >
> >       put_tracing_file(path);
> >
> >       handle->file_state = TRACECMD_FILE_CPU_LATENCY;
> >
> > +     if (HAS_SECTIONS(handle))
> > +             tracecmd_write_options(handle);
> > +
> >       return handle;
>
>
> >
> >  out_free:
> > -     tracecmd_output_close(handle);
> > +     if (handle)
> > +             tracecmd_output_close(handle);
> > +     unlink(output_file);
>
> Hmm, how does the above play a role in this patch?
>
> That is, what about this new BUFFER_TEXT required this change?
> I mean, output_file is being removed now, but I don't see anything in
> the rest of the patch to warrant that?
>

That looks like a leftover from a previous version of this patch.

> -- Steve
>
>
> >       return NULL;
> >  }
> >
>


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

[PATCH v7 12/25] trace-cmd library: Add macro to check file state on reading

[Tzvetomir Stoyanov (VMware)]

Trace file version 7 has flexible structure. It allows reading almost
any part of the file at any time, unlike the version 6 file when reading
state must be validated at each step. Added a macro to handle these
checks.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 31e5e674..0375afba 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -166,6 +166,8 @@ struct tracecmd_input {
 
 __thread struct tracecmd_input *tracecmd_curr_thread_handle;
 
+#define CHECK_READ_STATE(H, S) ((H)->file_version < FILE_VERSION_SECTIONS && (H)->file_state >= (S))
+
 static int read_options_type(struct tracecmd_input *handle);
 
 void tracecmd_set_flag(struct tracecmd_input *handle, int flag)
@@ -382,7 +384,7 @@ static int read_header_files(struct tracecmd_input *handle)
 	char *header;
 	char buf[BUFSIZ];
 
-	if (handle->file_state >= TRACECMD_FILE_HEADERS)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_HEADERS))
 		return 0;
 
 	if (do_read_check(handle, buf, 12))
@@ -588,7 +590,7 @@ static int read_ftrace_files(struct tracecmd_input *handle, const char *regex)
 	int unique;
 	int ret;
 
-	if (handle->file_state >= TRACECMD_FILE_FTRACE_EVENTS)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_FTRACE_EVENTS))
 		return 0;
 
 	if (regex) {
@@ -661,7 +663,7 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
 	int unique;
 	int ret;
 
-	if (handle->file_state >= TRACECMD_FILE_ALL_EVENTS)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_ALL_EVENTS))
 		return 0;
 
 	if (regex) {
@@ -745,7 +747,7 @@ static int read_proc_kallsyms(struct tracecmd_input *handle)
 	unsigned int size;
 	char *buf;
 
-	if (handle->file_state >= TRACECMD_FILE_KALLSYMS)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_KALLSYMS))
 		return 0;
 
 	if (read4(handle, &size) < 0)
@@ -778,7 +780,7 @@ static int read_ftrace_printk(struct tracecmd_input *handle)
 	unsigned int size;
 	char *buf;
 
-	if (handle->file_state >= TRACECMD_FILE_PRINTK)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_PRINTK))
 		return 0;
 
 	if (read4(handle, &size) < 0)
@@ -826,7 +828,7 @@ static int read_cpus(struct tracecmd_input *handle)
 {
 	unsigned int cpus;
 
-	if (handle->file_state >= TRACECMD_FILE_CPU_COUNT)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_CPU_COUNT))
 		return 0;
 
 	if (read4(handle, &cpus) < 0)
@@ -2829,7 +2831,7 @@ static int read_options_type(struct tracecmd_input *handle)
 {
 	char buf[10];
 
-	if (handle->file_state >= TRACECMD_FILE_CPU_LATENCY)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_CPU_LATENCY))
 		return 0;
 
 	if (do_read_check(handle, buf, 10))
@@ -2994,7 +2996,7 @@ static int read_and_parse_cmdlines(struct tracecmd_input *handle)
 	unsigned long long size;
 	char *cmdlines;
 
-	if (handle->file_state >= TRACECMD_FILE_CMD_LINES)
+	if (CHECK_READ_STATE(handle, TRACECMD_FILE_CMD_LINES))
 		return 0;
 
 	if (read_data_and_size(handle, &cmdlines, &size) < 0)
-- 
2.33.1

[PATCH v7 13/25] trace-cmd library: Introduce sections in trace file reading logic

[Tzvetomir Stoyanov (VMware)]

Trace file version 7 is based on sections. Added an internal sections
database and new helper functions to add, read, open and close file
sections.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 0375afba..78f9effd 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -115,6 +115,14 @@ struct tsc2nsec {
 	unsigned long long offset;
 };
 
+struct file_section {
+	unsigned long long		section_offset;
+	unsigned long long		data_offset;
+	int				id;
+	int				flags;
+	struct file_section		*next;
+};
+
 struct tracecmd_input {
 	struct tep_handle	*pevent;
 	unsigned long		file_state;
@@ -154,6 +162,7 @@ struct tracecmd_input {
 	struct hook_list	*hooks;
 	struct pid_addr_maps	*pid_maps;
 	/* file information */
+	struct file_section	*sections;
 	size_t			header_files_start;
 	size_t			ftrace_files_start;
 	size_t			event_files_start;
@@ -377,6 +386,58 @@ static int read8(struct tracecmd_input *handle, unsigned long long *size)
 	return 0;
 }
 
+static struct file_section *section_get(struct tracecmd_input *handle, int id)
+{
+	struct file_section *sec;
+
+	for (sec = handle->sections; sec; sec = sec->next) {
+		if (sec->id == id)
+			return sec;
+	}
+
+	return NULL;
+}
+
+static struct file_section *section_open(struct tracecmd_input *handle, int id)
+{
+	struct file_section *sec = section_get(handle, id);
+
+	if (!sec)
+		return NULL;
+
+	if (lseek64(handle->fd, sec->data_offset, SEEK_SET) == (off64_t)-1)
+		return NULL;
+	return sec;
+}
+
+static void section_close(struct tracecmd_input *handle, struct file_section *sec)
+{
+	/* To Do */
+}
+
+static int section_add_or_update(struct tracecmd_input *handle, int id, int flags,
+				 unsigned long long section_offset,
+				 unsigned long long data_offset)
+{
+	struct file_section *sec = section_get(handle, id);
+
+	if (!sec) {
+		sec = calloc(1, sizeof(struct file_section));
+		if (!sec)
+			return -1;
+		sec->next = handle->sections;
+		handle->sections = sec;
+	}
+	sec->id = id;
+	if (section_offset)
+		sec->section_offset = section_offset;
+	if (data_offset)
+		sec->data_offset = data_offset;
+	if (flags > 0)
+		sec->flags = flags;
+	return 0;
+}
+
 static int read_header_files(struct tracecmd_input *handle)
 {
 	struct tep_handle *pevent = handle->pevent;
@@ -3493,6 +3554,7 @@ void tracecmd_ref(struct tracecmd_input *handle)
  */
 void tracecmd_close(struct tracecmd_input *handle)
 {
+	struct file_section *del_sec;
 	int cpu;
 	int i;
 
@@ -3532,6 +3594,12 @@ void tracecmd_close(struct tracecmd_input *handle)
 	free(handle->version);
 	close(handle->fd);
 
+	while (handle->sections) {
+		del_sec = handle->sections;
+		handle->sections = handle->sections->next;
+		free(del_sec);
+	}
+
 	for (i = 0; i < handle->nr_buffers; i++)
 		free(handle->buffers[i].name);
 	free(handle->buffers);
@@ -3976,6 +4044,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->sections = NULL;
 	new_handle->guest = NULL;
 	new_handle->ref = 1;
 	if (handle->trace_clock) {
-- 
2.33.1

Re: [PATCH v7 13/25] trace-cmd library: Introduce sections in trace file reading logic

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:36 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Trace file version 7 is based on sections. Added an internal sections
> database and new helper functions to add, read, open and close file
> sections.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 69 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 0375afba..78f9effd 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -115,6 +115,14 @@ struct tsc2nsec {
>  	unsigned long long offset;
>  };
>  
> +struct file_section {
> +	unsigned long long		section_offset;
> +	unsigned long long		data_offset;
> +	int				id;
> +	int				flags;
> +	struct file_section		*next;
> +};
> +
>  struct tracecmd_input {
>  	struct tep_handle	*pevent;
>  	unsigned long		file_state;
> @@ -154,6 +162,7 @@ struct tracecmd_input {
>  	struct hook_list	*hooks;
>  	struct pid_addr_maps	*pid_maps;
>  	/* file information */
> +	struct file_section	*sections;
>  	size_t			header_files_start;
>  	size_t			ftrace_files_start;
>  	size_t			event_files_start;
> @@ -377,6 +386,58 @@ static int read8(struct tracecmd_input *handle, unsigned long long *size)
>  	return 0;
>  }
>  
> +static struct file_section *section_get(struct tracecmd_input *handle, int id)
> +{
> +	struct file_section *sec;
> +
> +	for (sec = handle->sections; sec; sec = sec->next) {
> +		if (sec->id == id)
> +			return sec;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct file_section *section_open(struct tracecmd_input *handle, int id)
> +{
> +	struct file_section *sec = section_get(handle, id);
> +
> +	if (!sec)
> +		return NULL;
> +
> +	if (lseek64(handle->fd, sec->data_offset, SEEK_SET) == (off64_t)-1)
> +		return NULL;
> +	return sec;
> +}
> +
> +static void section_close(struct tracecmd_input *handle, struct file_section *sec)
> +{
> +	/* To Do */
> +}
> +
> +static int section_add_or_update(struct tracecmd_input *handle, int id, int flags,
> +				 unsigned long long section_offset,
> +				 unsigned long long data_offset)
> +{
> +	struct file_section *sec = section_get(handle, id);
> +
> +	if (!sec) {
> +		sec = calloc(1, sizeof(struct file_section));
> +		if (!sec)
> +			return -1;
> +		sec->next = handle->sections;
> +		handle->sections = sec;
> +	}
> +	sec->id = id;

Could move the above into the previous if block. As it's only to be
updated if it wasn't already found (with the id).

> +	if (section_offset)
> +		sec->section_offset = section_offset;
> +	if (data_offset)
> +		sec->data_offset = data_offset;
> +	if (flags > 0)

Why the greater than zero check?

Is there a way to clear flags?

> +		sec->flags = flags;
> +	return 0;
> +}

-- Steve

> +
>  static int read_header_files(struct tracecmd_input *handle)
>  {
>  	struct tep_handle *pevent = handle->pevent;
> @@ -3493,6 +3554,7 @@ void tracecmd_ref(struct tracecmd_input *handle)
>   */
>  void tracecmd_close(struct tracecmd_input *handle)
>  {
> +	struct file_section *del_sec;
>  	int cpu;
>  	int i;
>  
> @@ -3532,6 +3594,12 @@ void tracecmd_close(struct tracecmd_input *handle)
>  	free(handle->version);
>  	close(handle->fd);
>  
> +	while (handle->sections) {
> +		del_sec = handle->sections;
> +		handle->sections = handle->sections->next;
> +		free(del_sec);
> +	}
> +
>  	for (i = 0; i < handle->nr_buffers; i++)
>  		free(handle->buffers[i].name);
>  	free(handle->buffers);
> @@ -3976,6 +4044,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->sections = NULL;
>  	new_handle->guest = NULL;
>  	new_handle->ref = 1;
>  	if (handle->trace_clock) {

[PATCH v7 14/25] trace-cmd library: Initialize internal sections database on file read

[Tzvetomir Stoyanov (VMware)]

Add sections in internal database on file reading and parsing.
In trace file version 7, sections are initialized when parsing
corresponding trace options. In version 6 files, sections are retrieved
on file reading, as there they are in a fixed position in the file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  1 +
 lib/trace-cmd/trace-input.c                   | 65 +++++++++++++++----
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index d8ee9b74..2e0c32b6 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -154,6 +154,7 @@ enum {
 	TRACECMD_FL_BUFFER_INSTANCE	= (1 << 1),
 	TRACECMD_FL_IN_USECS		= (1 << 2),
 	TRACECMD_FL_RAW_TS		= (1 << 3),
+	TRACECMD_FL_SECTIONED		= (1 << 4),
 };
 
 struct tracecmd_ftrace {
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 78f9effd..7f24d2e2 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -165,8 +165,7 @@ struct tracecmd_input {
 	struct file_section	*sections;
 	size_t			header_files_start;
 	size_t			ftrace_files_start;
-	size_t			event_files_start;
-	size_t			options_start;
+	unsigned long long	options_start;
 	size_t			total_file_size;
 
 	/* For custom profilers. */
@@ -176,6 +175,7 @@ struct tracecmd_input {
 __thread struct tracecmd_input *tracecmd_curr_thread_handle;
 
 #define CHECK_READ_STATE(H, S) ((H)->file_version < FILE_VERSION_SECTIONS && (H)->file_state >= (S))
+#define HAS_SECTIONS(H) ((H)->flags & TRACECMD_FL_SECTIONED)
 
 static int read_options_type(struct tracecmd_input *handle);
 
@@ -448,6 +448,10 @@ static int read_header_files(struct tracecmd_input *handle)
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_HEADERS))
 		return 0;
 
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_HEADER_INFO, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
+
 	if (do_read_check(handle, buf, 12))
 		return -1;
 
@@ -491,9 +495,6 @@ static int read_header_files(struct tracecmd_input *handle)
 
 	free(header);
 
-	handle->ftrace_files_start =
-		lseek64(handle->fd, 0, SEEK_CUR);
-
 	handle->file_state = TRACECMD_FILE_HEADERS;
 
 	return 0;
@@ -654,6 +655,10 @@ static int read_ftrace_files(struct tracecmd_input *handle, const char *regex)
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_FTRACE_EVENTS))
 		return 0;
 
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_FTRACE_EVENTS, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
+
 	if (regex) {
 		sreg = &spreg;
 		ereg = &epreg;
@@ -694,8 +699,6 @@ static int read_ftrace_files(struct tracecmd_input *handle, const char *regex)
 			goto out;
 	}
 
-	handle->event_files_start =
-		lseek64(handle->fd, 0, SEEK_CUR);
 	handle->file_state = TRACECMD_FILE_FTRACE_EVENTS;
 	ret = 0;
 out:
@@ -727,6 +730,10 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_ALL_EVENTS))
 		return 0;
 
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_EVENT_FORMATS, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
+
 	if (regex) {
 		sreg = &spreg;
 		ereg = &epreg;
@@ -810,6 +817,9 @@ static int read_proc_kallsyms(struct tracecmd_input *handle)
 
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_KALLSYMS))
 		return 0;
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_KALLSYMS, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
 
 	if (read4(handle, &size) < 0)
 		return -1;
@@ -844,6 +854,10 @@ static int read_ftrace_printk(struct tracecmd_input *handle)
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_PRINTK))
 		return 0;
 
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_PRINTK, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
+
 	if (read4(handle, &size) < 0)
 		return -1;
 	if (!size) {
@@ -2876,6 +2890,18 @@ static int handle_options(struct tracecmd_input *handle)
 			handle->tsc_calc.offset = tep_read_number(handle->pevent,
 								  buf + 8, 8);
 			break;
+		case TRACECMD_OPTION_HEADER_INFO:
+		case TRACECMD_OPTION_FTRACE_EVENTS:
+		case TRACECMD_OPTION_EVENT_FORMATS:
+		case TRACECMD_OPTION_KALLSYMS:
+		case TRACECMD_OPTION_PRINTK:
+		case TRACECMD_OPTION_CMDLINES:
+			if (size < 8)
+				break;
+			section_add_or_update(handle, option, -1,
+					      tep_read_number(handle->pevent, buf, 8), 0);
+			break;
+
 		default:
 			tracecmd_warning("unknown option %d", option);
 			break;
@@ -3060,6 +3086,11 @@ static int read_and_parse_cmdlines(struct tracecmd_input *handle)
 	if (CHECK_READ_STATE(handle, TRACECMD_FILE_CMD_LINES))
 		return 0;
 
+	if (!HAS_SECTIONS(handle))
+		section_add_or_update(handle, TRACECMD_OPTION_CMDLINES, 0, 0,
+				      lseek64(handle->fd, 0, SEEK_CUR));
+
+
 	if (read_data_and_size(handle, &cmdlines, &size) < 0)
 		return -1;
 	cmdlines[size] = 0;
@@ -3349,6 +3380,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 	struct tracecmd_input *handle;
 	char test[] = TRACECMD_MAGIC;
 	unsigned int page_size;
+	size_t offset;
 	char *version;
 	char buf[BUFSIZ];
 	unsigned long ver;
@@ -3388,6 +3420,9 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 	handle->file_version = ver;
 	free(version);
 
+	if (handle->file_version >= FILE_VERSION_SECTIONS)
+		handle->flags |= TRACECMD_FL_SECTIONED;
+
 	if (do_read_check(handle, buf, 1))
 		goto failed_read;
 
@@ -3412,14 +3447,16 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 	read4(handle, &page_size);
 	handle->page_size = page_size;
 
-	handle->header_files_start =
-		lseek64(handle->fd, 0, SEEK_CUR);
-
-	handle->total_file_size =
-		lseek64(handle->fd, 0, SEEK_END);
+	offset = lseek64(handle->fd, 0, SEEK_CUR);
+	handle->total_file_size = lseek64(handle->fd, 0, SEEK_END);
+	lseek64(handle->fd, offset, SEEK_SET);
 
-	handle->header_files_start =
-		lseek64(handle->fd, handle->header_files_start, SEEK_SET);
+	if (HAS_SECTIONS(handle)) {
+		if (read8(handle, &(handle->options_start))) {
+			tracecmd_warning("Filed to read the offset of the first option section");
+			goto failed_read;
+		}
+	}
 
 	handle->file_state = TRACECMD_FILE_INIT;
 
-- 
2.33.1

[PATCH v7 15/25] trace-cmd library: Use sections database when reading parts of the trace file

[Tzvetomir Stoyanov (VMware)]

Utilize the internal database with file sections, when reading parts
of a trace file. This logic unifies the way trace file version 6 and 7
are processed.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 7f24d2e2..8f95bf3b 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3268,21 +3268,26 @@ int tracecmd_make_pipe(struct tracecmd_input *handle, int cpu, int fd, int cpus)
  */
 void tracecmd_print_events(struct tracecmd_input *handle, const char *regex)
 {
-	int ret;
+	struct file_section *sec;
 
 	if (!regex)
 		regex = ".*";
 
-	if (!handle->ftrace_files_start) {
-		lseek64(handle->fd, handle->header_files_start, SEEK_SET);
+	sec = section_open(handle, TRACECMD_OPTION_HEADER_INFO);
+	if (sec) {
 		read_header_files(handle);
+		section_close(handle, sec);
+	}
+	sec = section_open(handle, TRACECMD_OPTION_FTRACE_EVENTS);
+	if (sec) {
+		read_ftrace_files(handle, regex);
+		section_close(handle, sec);
+	}
+	sec = section_open(handle, TRACECMD_OPTION_EVENT_FORMATS);
+	if (sec) {
+		read_event_files(handle, regex);
+		section_close(handle, sec);
 	}
-	ret = read_ftrace_files(handle, regex);
-	if (ret < 0)
-		return;
-
-	read_event_files(handle, regex);
-	return;
 }
 
 /* Show the cpu data stats */
@@ -3886,6 +3891,7 @@ int tracecmd_copy_headers(struct tracecmd_input *handle, int fd,
 			  enum tracecmd_file_states start_state,
 			  enum tracecmd_file_states end_state)
 {
+	struct file_section *sec;
 	int ret;
 
 	if (!start_state)
@@ -3901,13 +3907,17 @@ int tracecmd_copy_headers(struct tracecmd_input *handle, int fd,
 
 	if (handle->file_state >= start_state) {
 		/* Set the handle to just before the start state */
-		lseek64(handle->fd, handle->header_files_start, SEEK_SET);
+		sec = section_open(handle, TRACECMD_OPTION_HEADER_INFO);
+		if (!sec)
+			return -1;
 		/* Now that the file handle has moved, change its state */
 		handle->file_state = TRACECMD_FILE_INIT;
 	}
 
 	/* Try to bring the input up to the start state - 1 */
 	ret = tracecmd_read_headers(handle, start_state - 1);
+	if (sec)
+		section_close(handle, sec);
 	if (ret < 0)
 		goto out;
 
-- 
2.33.1

[PATCH v7 16/25] trace-cmd library: Read headers from trace file version 7

[Tzvetomir Stoyanov (VMware)]

Trace file version 7 has more flexible structure than version 6, headers
are not located at fixed position in the file. A new logic is
implemented to read and parse the headers from this new format.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 8f95bf3b..9027729e 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -163,8 +163,7 @@ struct tracecmd_input {
 	struct pid_addr_maps	*pid_maps;
 	/* file information */
 	struct file_section	*sections;
-	size_t			header_files_start;
-	size_t			ftrace_files_start;
+	bool			options_init;
 	unsigned long long	options_start;
 	size_t			total_file_size;
 
@@ -917,19 +916,7 @@ static int read_cpus(struct tracecmd_input *handle)
 	return 0;
 }
 
-/**
- * tracecmd_read_headers - read the header information from trace.dat
- * @handle: input handle for the trace.dat file
- * @state: The state to read up to or zero to read up to options.
- *
- * This reads the trace.dat file for various information. Like the
- * format of the ring buffer, event formats, ftrace formats, kallsyms
- * and printk. This may be called multiple times with different @state
- * values, to read partial data at a time. It will always continue
- * where it left off.
- */
-int tracecmd_read_headers(struct tracecmd_input *handle,
-			  enum tracecmd_file_states state)
+static int read_headers_v6(struct tracecmd_input *handle, enum tracecmd_file_states state)
 {
 	int ret;
 
@@ -995,6 +982,126 @@ int tracecmd_read_headers(struct tracecmd_input *handle,
 	return 0;
 }
 
+static int handle_options(struct tracecmd_input *handle);
+
+static int read_section_header(struct tracecmd_input *handle, unsigned short *id,
+			       unsigned short *flags, unsigned long long *size, const char **description)
+{
+	unsigned short fl;
+	unsigned short sec_id;
+	unsigned long long sz;
+	int desc;
+
+	if (read2(handle, &sec_id))
+		return -1;
+	if (read4(handle, (unsigned int *)&desc))
+		return -1;
+	if (read2(handle, &fl))
+		return -1;
+	if (read8(handle, &sz))
+		return -1;
+
+	if (id)
+		*id = sec_id;
+	if (flags)
+		*flags = fl;
+	if (size)
+		*size = sz;
+
+	return 0;
+}
+
+static int handle_section(struct tracecmd_input *handle, struct file_section *section)
+{
+	unsigned short id, flags;
+	unsigned long long size;
+	int ret;
+
+	if (lseek64(handle->fd, section->section_offset, SEEK_SET) == (off_t)-1)
+		return -1;
+	if (read_section_header(handle, &id, &flags, &size, NULL))
+		return -1;
+	section->flags = flags;
+	if (id != section->id)
+		return -1;
+
+	section->data_offset = lseek64(handle->fd, 0, SEEK_CUR);
+
+	switch (section->id) {
+	case TRACECMD_OPTION_HEADER_INFO:
+		ret = read_header_files(handle);
+		break;
+	case TRACECMD_OPTION_FTRACE_EVENTS:
+		ret = read_ftrace_files(handle, NULL);
+		break;
+	case TRACECMD_OPTION_EVENT_FORMATS:
+		ret = read_event_files(handle, NULL);
+		break;
+	case TRACECMD_OPTION_KALLSYMS:
+		ret = read_proc_kallsyms(handle);
+		break;
+	case TRACECMD_OPTION_PRINTK:
+		ret = read_ftrace_printk(handle);
+		break;
+	case TRACECMD_OPTION_CMDLINES:
+		ret = read_and_parse_cmdlines(handle);
+		break;
+	default:
+		ret = 0;
+		break;
+	}
+
+	return ret;
+}
+
+static int read_headers_v7(struct tracecmd_input *handle)
+{
+	struct file_section *section;
+
+	if (handle->options_init)
+		return 0;
+
+	if (!handle->options_start)
+		return -1;
+
+	if (lseek64(handle->fd, handle->options_start, SEEK_SET) == (off64_t)-1) {
+		tracecmd_warning("Filed to goto options offset %lld", handle->options_start);
+		return -1;
+	}
+
+	if (handle_options(handle))
+		return -1;
+
+	section = handle->sections;
+	while (section) {
+		if (handle_section(handle, section))
+			return -1;
+		section = section->next;
+	}
+
+	handle->options_init = true;
+	return 0;
+}
+
+/**
+ * tracecmd_read_headers - read the header information from trace.dat
+ * @handle: input handle for the trace.dat file
+ * @state: The state to read up to or zero to read up to options.
+ *
+ * This reads the trace.dat file for various information. Like the
+ * format of the ring buffer, event formats, ftrace formats, kallsyms
+ * and printk. This may be called multiple times with different @state
+ * values, to read partial data at a time. It will always continue
+ * where it left off.
+ */
+int tracecmd_read_headers(struct tracecmd_input *handle,
+			  enum tracecmd_file_states state)
+{
+	if (!HAS_SECTIONS(handle))
+		return read_headers_v6(handle, state);
+	return read_headers_v7(handle);
+}
+
 static unsigned long long calc_page_offset(struct tracecmd_input *handle,
 					   unsigned long long offset)
 {
-- 
2.33.1

Re: [PATCH v7 16/25] trace-cmd library: Read headers from trace file version 7

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:39 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Trace file version 7 has more flexible structure than version 6, headers
> are not located at fixed position in the file. A new logic is
> implemented to read and parse the headers from this new format.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 137 ++++++++++++++++++++++++++++++++----
>  1 file changed, 122 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 8f95bf3b..9027729e 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -163,8 +163,7 @@ struct tracecmd_input {
>  	struct pid_addr_maps	*pid_maps;
>  	/* file information */
>  	struct file_section	*sections;
> -	size_t			header_files_start;
> -	size_t			ftrace_files_start;
> +	bool			options_init;
>  	unsigned long long	options_start;
>  	size_t			total_file_size;
>  
> @@ -917,19 +916,7 @@ static int read_cpus(struct tracecmd_input *handle)
>  	return 0;
>  }
>  
> -/**
> - * tracecmd_read_headers - read the header information from trace.dat
> - * @handle: input handle for the trace.dat file
> - * @state: The state to read up to or zero to read up to options.
> - *
> - * This reads the trace.dat file for various information. Like the
> - * format of the ring buffer, event formats, ftrace formats, kallsyms
> - * and printk. This may be called multiple times with different @state
> - * values, to read partial data at a time. It will always continue
> - * where it left off.
> - */
> -int tracecmd_read_headers(struct tracecmd_input *handle,
> -			  enum tracecmd_file_states state)
> +static int read_headers_v6(struct tracecmd_input *handle, enum tracecmd_file_states state)
>  {
>  	int ret;
>  
> @@ -995,6 +982,126 @@ int tracecmd_read_headers(struct tracecmd_input *handle,
>  	return 0;
>  }
>  
> +static int handle_options(struct tracecmd_input *handle);
> +
> +static int read_section_header(struct tracecmd_input *handle, unsigned short *id,
> +			       unsigned short *flags, unsigned long long *size, const char **description)
> +{
> +	unsigned short fl;
> +	unsigned short sec_id;
> +	unsigned long long sz;
> +	int desc;
> +
> +	if (read2(handle, &sec_id))
> +		return -1;
> +	if (read4(handle, (unsigned int *)&desc))
> +		return -1;
> +	if (read2(handle, &fl))
> +		return -1;
> +	if (read8(handle, &sz))
> +		return -1;
> +
> +	if (id)
> +		*id = sec_id;
> +	if (flags)
> +		*flags = fl;
> +	if (size)
> +		*size = sz;
> +
> +	return 0;
> +}
> +
> +static int handle_section(struct tracecmd_input *handle, struct file_section *section)
> +{
> +	unsigned short id, flags;
> +	unsigned long long size;
> +	int ret;
> +
> +	if (lseek64(handle->fd, section->section_offset, SEEK_SET) == (off_t)-1)
> +		return -1;
> +	if (read_section_header(handle, &id, &flags, &size, NULL))
> +		return -1;
> +	section->flags = flags;
> +	if (id != section->id)
> +		return -1;
> +
> +	section->data_offset = lseek64(handle->fd, 0, SEEK_CUR);
> +
> +	switch (section->id) {
> +	case TRACECMD_OPTION_HEADER_INFO:
> +		ret = read_header_files(handle);
> +		break;
> +	case TRACECMD_OPTION_FTRACE_EVENTS:
> +		ret = read_ftrace_files(handle, NULL);
> +		break;
> +	case TRACECMD_OPTION_EVENT_FORMATS:
> +		ret = read_event_files(handle, NULL);
> +		break;
> +	case TRACECMD_OPTION_KALLSYMS:
> +		ret = read_proc_kallsyms(handle);
> +		break;
> +	case TRACECMD_OPTION_PRINTK:
> +		ret = read_ftrace_printk(handle);
> +		break;
> +	case TRACECMD_OPTION_CMDLINES:
> +		ret = read_and_parse_cmdlines(handle);
> +		break;
> +	default:
> +		ret = 0;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int read_headers_v7(struct tracecmd_input *handle)

I really like how these changes are coming. This makes the reading of the
tracing file much more manageable. Great job Tzvetomir!

Only comment to this patch is, as I've said in other patches, I don't think
we need to call this _v7, just "read_headers()" would suffice.

-- Steve


> +{
> +	struct file_section *section;
> +
> +	if (handle->options_init)
> +		return 0;
> +
> +	if (!handle->options_start)
> +		return -1;
> +
> +	if (lseek64(handle->fd, handle->options_start, SEEK_SET) == (off64_t)-1) {
> +		tracecmd_warning("Filed to goto options offset %lld", handle->options_start);
> +		return -1;
> +	}
> +
> +	if (handle_options(handle))
> +		return -1;
> +
> +	section = handle->sections;
> +	while (section) {
> +		if (handle_section(handle, section))
> +			return -1;
> +		section = section->next;
> +	}
> +
> +	handle->options_init = true;
> +	return 0;
> +}
> +
> +/**
> + * tracecmd_read_headers - read the header information from trace.dat
> + * @handle: input handle for the trace.dat file
> + * @state: The state to read up to or zero to read up to options.
> + *
> + * This reads the trace.dat file for various information. Like the
> + * format of the ring buffer, event formats, ftrace formats, kallsyms
> + * and printk. This may be called multiple times with different @state
> + * values, to read partial data at a time. It will always continue
> + * where it left off.
> + */
> +int tracecmd_read_headers(struct tracecmd_input *handle,
> +			  enum tracecmd_file_states state)
> +{
> +	if (!HAS_SECTIONS(handle))
> +		return read_headers_v6(handle, state);
> +	return read_headers_v7(handle);
> +}
> +
>  static unsigned long long calc_page_offset(struct tracecmd_input *handle,
>  					   unsigned long long offset)
>  {

Re: [PATCH v7 16/25] trace-cmd library: Read headers from trace file version 7

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 6:00 PM Steven Rostedt <rostedt@goodmis.org> wrote:
[...]
> > +
> > +static int read_headers_v7(struct tracecmd_input *handle)
>
> I really like how these changes are coming. This makes the reading of the
> tracing file much more manageable. Great job Tzvetomir!
>
> Only comment to this patch is, as I've said in other patches, I don't think
> we need to call this _v7, just "read_headers()" would suffice.

The read header logic is very different, we should name the old
function somehow. Now we have
           read_headers_v6() / read_headers_v7()
I can rename them to
           read_headers_old() / read_headers()
or
           read_headers_nosections() / read_headers_sections()

>
> -- Steve
[...]

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

Re: [PATCH v7 16/25] trace-cmd library: Read headers from trace file version 7

[Steven Rostedt]

On Mon, 17 Jan 2022 17:47:27 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> The read header logic is very different, we should name the old
> function somehow. Now we have
>            read_headers_v6() / read_headers_v7()
> I can rename them to
>            read_headers_old() / read_headers()
> or
>            read_headers_nosections() / read_headers_sections()

The _v6 names are fine. I'm only talking about the new _v7 ones (as
that's now the main ones, and v8 may use it too).

-- Steve

[PATCH v7 17/25] trace-cmd library: Read strings sections on file load

[Tzvetomir Stoyanov (VMware)]

Internal strings database is added to trace input handle, containing all
metadata strings from trace file. When a trace file is opened, all
strings sections are located and the internal strings database is
initialised.

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

diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index 4a0a691c..ac7e7f17 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -53,5 +53,6 @@ struct cpu_data_source {
 int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
 		       struct cpu_data_source *data, const char *buff_name);
 off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
+unsigned int get_meta_strings_size(struct tracecmd_input *handle);
 
 #endif /* _TRACE_CMD_LOCAL_H */
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 9027729e..6cc8ee90 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -147,6 +147,9 @@ struct tracecmd_input {
 	long long		ts_offset;
 	struct tsc2nsec		tsc_calc;
 
+	unsigned int		strings_size;	/* size of the metadata strings */
+	char			*strings;	/* metadata strings */
+
 	struct host_trace_info	host;
 	double			ts2secs;
 	char *			cpustats;
@@ -984,6 +987,14 @@ static int read_headers_v6(struct tracecmd_input *handle, enum tracecmd_file_sta
 
 static int handle_options(struct tracecmd_input *handle);
 
+static const char *get_metadata_string(struct tracecmd_input *handle, int offset)
+{
+	if (!handle || !handle->strings || offset < 0 || handle->strings_size >= offset)
+		return NULL;
+
+	return handle->strings + offset;
+}
+
 static int read_section_header(struct tracecmd_input *handle, unsigned short *id,
 			       unsigned short *flags, unsigned long long *size, const char **description)
 {
@@ -1007,6 +1018,8 @@ static int read_section_header(struct tracecmd_input *handle, unsigned short *id
 		*flags = fl;
 	if (size)
 		*size = sz;
+	if (description)
+		*description = get_metadata_string(handle, desc);
 
 	return 0;
 }
@@ -2839,6 +2852,11 @@ tracecmd_search_task_map(struct tracecmd_input *handle,
 	return lib;
 }
 
+__hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
+{
+	return handle->strings_size;
+}
+
 static int handle_options(struct tracecmd_input *handle)
 {
 	long long offset;
@@ -3472,6 +3490,50 @@ struct hook_list *tracecmd_hooks(struct tracecmd_input *handle)
 	return handle->hooks;
 }
 
+static int init_metadata_strings(struct tracecmd_input *handle, int size)
+{
+	char *tmp;
+
+	tmp = realloc(handle->strings, handle->strings_size + size);
+	if (!tmp)
+		return -1;
+
+	handle->strings = tmp;
+	if (do_read_check(handle, handle->strings + handle->strings_size, size))
+		return -1;
+
+	handle->strings_size += size;
+
+	return 0;
+}
+
+static int read_metadata_strings(struct tracecmd_input *handle)
+{
+	unsigned short flags;
+	int found = 0;
+	unsigned short id;
+	unsigned long long size;
+	off64_t offset;
+
+	offset = lseek64(handle->fd, 0, SEEK_CUR);
+	do {
+		if (read_section_header(handle, &id, &flags, &size, NULL))
+			break;
+		if (id == TRACECMD_OPTION_STRINGS) {
+			found++;
+			init_metadata_strings(handle, size);
+		} else {
+			if (lseek64(handle->fd, size, SEEK_CUR) == (off_t)-1)
+				break;
+		}
+	} while (1);
+
+	if (lseek64(handle->fd, offset, SEEK_SET) == (off_t)-1)
+		return -1;
+
+	return found ? 0 : -1;
+}
+
 /**
  * tracecmd_alloc_fd - create a tracecmd_input handle from a file descriptor
  * @fd: the file descriptor for the trace.dat file
@@ -3568,6 +3630,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 			tracecmd_warning("Filed to read the offset of the first option section");
 			goto failed_read;
 		}
+		read_metadata_strings(handle);
 	}
 
 	handle->file_state = TRACECMD_FILE_INIT;
@@ -3740,6 +3803,7 @@ void tracecmd_close(struct tracecmd_input *handle)
 	free(handle->cpu_data);
 	free(handle->uname);
 	free(handle->trace_clock);
+	free(handle->strings);
 	free(handle->version);
 	close(handle->fd);
 
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 47227728..08f74c87 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -2239,6 +2239,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
 	handle->page_size = tracecmd_page_size(ihandle);
 	handle->file_version = tracecmd_get_in_file_version(ihandle);
 	handle->options_start = tracecmd_get_options_offset(ihandle);
+	handle->strings_offs = get_meta_strings_size(ihandle);
 	list_head_init(&handle->options);
 	list_head_init(&handle->buffers);
 
-- 
2.33.1

Re: [PATCH v7 17/25] trace-cmd library: Read strings sections on file load

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:40 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Internal strings database is added to trace input handle, containing all
> metadata strings from trace file. When a trace file is opened, all
> strings sections are located and the internal strings database is
> initialised.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/include/trace-cmd-local.h |  1 +
>  lib/trace-cmd/trace-input.c             | 64 +++++++++++++++++++++++++
>  lib/trace-cmd/trace-output.c            |  1 +
>  3 files changed, 66 insertions(+)
> 
> diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> index 4a0a691c..ac7e7f17 100644
> --- a/lib/trace-cmd/include/trace-cmd-local.h
> +++ b/lib/trace-cmd/include/trace-cmd-local.h
> @@ -53,5 +53,6 @@ struct cpu_data_source {
>  int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
>  		       struct cpu_data_source *data, const char *buff_name);
>  off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
> +unsigned int get_meta_strings_size(struct tracecmd_input *handle);
>  
>  #endif /* _TRACE_CMD_LOCAL_H */
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 9027729e..6cc8ee90 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -147,6 +147,9 @@ struct tracecmd_input {
>  	long long		ts_offset;
>  	struct tsc2nsec		tsc_calc;
>  
> +	unsigned int		strings_size;	/* size of the metadata strings */
> +	char			*strings;	/* metadata strings */
> +
>  	struct host_trace_info	host;
>  	double			ts2secs;
>  	char *			cpustats;
> @@ -984,6 +987,14 @@ static int read_headers_v6(struct tracecmd_input *handle, enum tracecmd_file_sta
>  
>  static int handle_options(struct tracecmd_input *handle);
>  
> +static const char *get_metadata_string(struct tracecmd_input *handle, int offset)
> +{
> +	if (!handle || !handle->strings || offset < 0 || handle->strings_size >= offset)
> +		return NULL;
> +
> +	return handle->strings + offset;
> +}
> +
>  static int read_section_header(struct tracecmd_input *handle, unsigned short *id,
>  			       unsigned short *flags, unsigned long long *size, const char **description)
>  {
> @@ -1007,6 +1018,8 @@ static int read_section_header(struct tracecmd_input *handle, unsigned short *id
>  		*flags = fl;
>  	if (size)
>  		*size = sz;
> +	if (description)
> +		*description = get_metadata_string(handle, desc);
>  
>  	return 0;
>  }
> @@ -2839,6 +2852,11 @@ tracecmd_search_task_map(struct tracecmd_input *handle,
>  	return lib;
>  }
>  
> +__hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
> +{
> +	return handle->strings_size;
> +}
> +
>  static int handle_options(struct tracecmd_input *handle)
>  {
>  	long long offset;
> @@ -3472,6 +3490,50 @@ struct hook_list *tracecmd_hooks(struct tracecmd_input *handle)
>  	return handle->hooks;
>  }
>  
> +static int init_metadata_strings(struct tracecmd_input *handle, int size)
> +{
> +	char *tmp;
> +
> +	tmp = realloc(handle->strings, handle->strings_size + size);

Hmm, I don't remember in our conversations. Did we say we would have more
than one string section? Or allow it to be broken up? How do offsets work
in such cases?

I was thinking that we could simply mmap the string section, but that won't
work if there's more than one.

-- Steve


> +	if (!tmp)
> +		return -1;
> +
> +	handle->strings = tmp;
> +	if (do_read_check(handle, handle->strings + handle->strings_size, size))
> +		return -1;
> +
> +	handle->strings_size += size;
> +
> +	return 0;
> +}
> +
> +static int read_metadata_strings(struct tracecmd_input *handle)
> +{
> +	unsigned short flags;
> +	int found = 0;
> +	unsigned short id;
> +	unsigned long long size;
> +	off64_t offset;
> +
> +	offset = lseek64(handle->fd, 0, SEEK_CUR);
> +	do {
> +		if (read_section_header(handle, &id, &flags, &size, NULL))
> +			break;
> +		if (id == TRACECMD_OPTION_STRINGS) {
> +			found++;
> +			init_metadata_strings(handle, size);
> +		} else {
> +			if (lseek64(handle->fd, size, SEEK_CUR) == (off_t)-1)
> +				break;
> +		}
> +	} while (1);
> +
> +	if (lseek64(handle->fd, offset, SEEK_SET) == (off_t)-1)
> +		return -1;
> +
> +	return found ? 0 : -1;
> +}
> +
>  /**
>   * tracecmd_alloc_fd - create a tracecmd_input handle from a file descriptor
>   * @fd: the file descriptor for the trace.dat file
> @@ -3568,6 +3630,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
>  			tracecmd_warning("Filed to read the offset of the first option section");
>  			goto failed_read;
>  		}
> +		read_metadata_strings(handle);
>  	}
>  
>  	handle->file_state = TRACECMD_FILE_INIT;
> @@ -3740,6 +3803,7 @@ void tracecmd_close(struct tracecmd_input *handle)
>  	free(handle->cpu_data);
>  	free(handle->uname);
>  	free(handle->trace_clock);
> +	free(handle->strings);
>  	free(handle->version);
>  	close(handle->fd);
>  
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 47227728..08f74c87 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -2239,6 +2239,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
>  	handle->page_size = tracecmd_page_size(ihandle);
>  	handle->file_version = tracecmd_get_in_file_version(ihandle);
>  	handle->options_start = tracecmd_get_options_offset(ihandle);
> +	handle->strings_offs = get_meta_strings_size(ihandle);
>  	list_head_init(&handle->options);
>  	list_head_init(&handle->buffers);
>  

Re: [PATCH v7 17/25] trace-cmd library: Read strings sections on file load

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 6:04 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:40 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > Internal strings database is added to trace input handle, containing all
> > metadata strings from trace file. When a trace file is opened, all
> > strings sections are located and the internal strings database is
> > initialised.
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  lib/trace-cmd/include/trace-cmd-local.h |  1 +
> >  lib/trace-cmd/trace-input.c             | 64 +++++++++++++++++++++++++
> >  lib/trace-cmd/trace-output.c            |  1 +
> >  3 files changed, 66 insertions(+)
> >
> > diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> > index 4a0a691c..ac7e7f17 100644
> > --- a/lib/trace-cmd/include/trace-cmd-local.h
> > +++ b/lib/trace-cmd/include/trace-cmd-local.h
> > @@ -53,5 +53,6 @@ struct cpu_data_source {
> >  int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
> >                      struct cpu_data_source *data, const char *buff_name);
> >  off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
> > +unsigned int get_meta_strings_size(struct tracecmd_input *handle);
> >
> >  #endif /* _TRACE_CMD_LOCAL_H */
> > diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> > index 9027729e..6cc8ee90 100644
> > --- a/lib/trace-cmd/trace-input.c
> > +++ b/lib/trace-cmd/trace-input.c
> > @@ -147,6 +147,9 @@ struct tracecmd_input {
> >       long long               ts_offset;
> >       struct tsc2nsec         tsc_calc;
> >
> > +     unsigned int            strings_size;   /* size of the metadata strings */
> > +     char                    *strings;       /* metadata strings */
> > +
> >       struct host_trace_info  host;
> >       double                  ts2secs;
> >       char *                  cpustats;
> > @@ -984,6 +987,14 @@ static int read_headers_v6(struct tracecmd_input *handle, enum tracecmd_file_sta
> >
> >  static int handle_options(struct tracecmd_input *handle);
> >
> > +static const char *get_metadata_string(struct tracecmd_input *handle, int offset)
> > +{
> > +     if (!handle || !handle->strings || offset < 0 || handle->strings_size >= offset)
> > +             return NULL;
> > +
> > +     return handle->strings + offset;
> > +}
> > +
> >  static int read_section_header(struct tracecmd_input *handle, unsigned short *id,
> >                              unsigned short *flags, unsigned long long *size, const char **description)
> >  {
> > @@ -1007,6 +1018,8 @@ static int read_section_header(struct tracecmd_input *handle, unsigned short *id
> >               *flags = fl;
> >       if (size)
> >               *size = sz;
> > +     if (description)
> > +             *description = get_metadata_string(handle, desc);
> >
> >       return 0;
> >  }
> > @@ -2839,6 +2852,11 @@ tracecmd_search_task_map(struct tracecmd_input *handle,
> >       return lib;
> >  }
> >
> > +__hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
> > +{
> > +     return handle->strings_size;
> > +}
> > +
> >  static int handle_options(struct tracecmd_input *handle)
> >  {
> >       long long offset;
> > @@ -3472,6 +3490,50 @@ struct hook_list *tracecmd_hooks(struct tracecmd_input *handle)
> >       return handle->hooks;
> >  }
> >
> > +static int init_metadata_strings(struct tracecmd_input *handle, int size)
> > +{
> > +     char *tmp;
> > +
> > +     tmp = realloc(handle->strings, handle->strings_size + size);
>
> Hmm, I don't remember in our conversations. Did we say we would have more
> than one string section? Or allow it to be broken up? How do offsets work
> in such cases?
>
> I was thinking that we could simply mmap the string section, but that won't
> work if there's more than one.
>

The first implementation was with a single string section at the end
of the file. But when I tested that design with the trace-cmd
guest-host tracing, I realized it is impossible to work without major
changes in the host-guest protocol. The problem is that in the current
host-guest tracing logic, part of the guest metadata file is written
in the guest context, part in the host context. Each of these parts
can put strings in the string section. That will change the way guest
metadata is written. That's why decided to use multiple string
sections in the file. On file read the strings from all sections are
stored into one memory block, and string offset is the offset in that
block. As these sections are compressed usually, simple mmap will not
work even in case of single section.

> -- Steve
>
>
[...]


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

Re: [PATCH v7 17/25] trace-cmd library: Read strings sections on file load

[Steven Rostedt]

On Mon, 17 Jan 2022 18:11:38 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > Hmm, I don't remember in our conversations. Did we say we would have more
> > than one string section? Or allow it to be broken up? How do offsets work
> > in such cases?
> >
> > I was thinking that we could simply mmap the string section, but that won't
> > work if there's more than one.
> >  
> 
> The first implementation was with a single string section at the end
> of the file. But when I tested that design with the trace-cmd
> guest-host tracing, I realized it is impossible to work without major
> changes in the host-guest protocol. The problem is that in the current
> host-guest tracing logic, part of the guest metadata file is written
> in the guest context, part in the host context. Each of these parts
> can put strings in the string section. That will change the way guest
> metadata is written. That's why decided to use multiple string
> sections in the file. On file read the strings from all sections are
> stored into one memory block, and string offset is the offset in that
> block. As these sections are compressed usually, simple mmap will not
> work even in case of single section.

OK, then we'll just keep it as is.

Thanks,

-- Steve

[PATCH v7 18/25] trace-cmd library: Read extended BUFFER option

[Tzvetomir Stoyanov (VMware)]

The BUFFER option is extended in trace file version 7. It holds CPU
metadata related to the recorded CPU traces. Also, there is a new
BUFFER_TEXT option for describing the latency trace data. A new logic
is implemented for these new options.
In trace file version 7, the top buffer is saved as other buffers in the
file, no special treatment. But saving the top buffer in the list of
buffers in the input handler causes problems. It breaks the legacy logic
of trace-cmd library users, which have special logic for trace buffers
processing. That's why "top_buffer" member is added in the input handler
structure, to hold the top buffer.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 6cc8ee90..5625e367 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -74,9 +74,19 @@ struct cpu_data {
 	int			pipe_fd;
 };
 
+struct cpu_file_data {
+	int			cpu;
+	unsigned long long	offset;
+	unsigned long long	size;
+};
+
 struct input_buffer_instance {
 	char			*name;
 	size_t			offset;
+	char			*clock;
+	bool			latency;
+	int			cpus;
+	struct cpu_file_data	*cpu_data;
 };
 
 struct ts_offset_sample {
@@ -156,6 +166,7 @@ struct tracecmd_input {
 	char *			uname;
 	char *			version;
 	char *			trace_clock;
+	struct input_buffer_instance	top_buffer;
 	struct input_buffer_instance	*buffers;
 	int			parsing_failures;
 	struct guest_trace_info	*guest;
@@ -2857,13 +2868,109 @@ __hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
 	return handle->strings_size;
 }
 
+static inline int save_read_number(struct tep_handle *tep, char *data, int *data_size,
+				   int *read_pos, int bytes, unsigned long long *num)
+{
+	if (bytes > *data_size)
+		return -1;
+	*num = tep_read_number(tep, (data + *read_pos), bytes);
+	*read_pos += bytes;
+	*data_size -= bytes;
+	return 0;
+}
+
+static inline char *save_read_string(char *data, int *data_size, int *read_pos)
+{
+	char *str;
+
+	if (*data_size < 1)
+		return NULL;
+	str = strdup(data + *read_pos);
+	if (!str)
+		return NULL;
+	*data_size -= (strlen(str) + 1);
+	if (*data_size < 0) {
+		free(str);
+		return NULL;
+	}
+	*read_pos += (strlen(str) + 1);
+
+	return str;
+}
+
+static int handle_buffer_option(struct tracecmd_input *handle,
+				unsigned short id, char *data, int size)
+{
+	struct input_buffer_instance *buff;
+	unsigned long long tmp;
+	int rsize = 0;
+	char *name;
+	int i;
+
+	if (save_read_number(handle->pevent, data, &size, &rsize, 8, &tmp))
+		return -1;
+	name = save_read_string(data, &size, &rsize);
+	if (!name)
+		return -1;
+
+	if (*name == '\0') {
+		/* top buffer */
+		buff = &handle->top_buffer;
+	} else {
+		buff = realloc(handle->buffers, sizeof(*handle->buffers) * (handle->nr_buffers + 1));
+		if (!buff) {
+			free(name);
+			return -1;
+		}
+		handle->buffers = buff;
+		handle->nr_buffers++;
+
+		buff = &handle->buffers[handle->nr_buffers - 1];
+	}
+	memset(buff, 0, sizeof(struct input_buffer_instance));
+	buff->name = name;
+	buff->offset = tmp;
+	if (!HAS_SECTIONS(handle))
+		return 0;
+
+	/* file sections specific data */
+	buff->clock = save_read_string(data, &size, &rsize);
+	if (!buff->clock)
+		return -1;
+	if (*name == '\0' && !handle->trace_clock)
+		handle->trace_clock = strdup(buff->clock);
+	if (id == TRACECMD_OPTION_BUFFER) {
+		if (save_read_number(handle->pevent, data, &size, &rsize, 4, &tmp))
+			return -1;
+		buff->cpus = tmp;
+		if (!buff->cpus)
+			return 0;
+		buff->cpu_data = calloc(buff->cpus, sizeof(struct cpu_file_data));
+		if (!buff->cpu_data)
+			return -1;
+		for (i = 0; i < buff->cpus; i++) {
+			if (save_read_number(handle->pevent, data, &size, &rsize, 4, &tmp))
+				return -1;
+			buff->cpu_data[i].cpu = tmp;
+			if (save_read_number(handle->pevent, data,
+					     &size, &rsize, 8, &buff->cpu_data[i].offset))
+				return -1;
+			if (save_read_number(handle->pevent, data,
+					     &size, &rsize, 8, &buff->cpu_data[i].size))
+				return -1;
+		}
+	} else {
+		buff->latency = true;
+	}
+	return 0;
+}
+
 static int handle_options(struct tracecmd_input *handle)
 {
 	long long offset;
 	unsigned short option;
 	unsigned int size;
 	char *cpustats = NULL;
-	struct input_buffer_instance *buffer;
 	struct hook_list *hook;
 	char *buf;
 	int cpus;
@@ -2954,21 +3061,10 @@ static int handle_options(struct tracecmd_input *handle)
 			handle->cpustats = cpustats;
 			break;
 		case TRACECMD_OPTION_BUFFER:
-			/* A buffer instance is saved at the end of the file */
-			handle->nr_buffers++;
-			handle->buffers = realloc(handle->buffers,
-						  sizeof(*handle->buffers) * handle->nr_buffers);
-			if (!handle->buffers)
-				return -ENOMEM;
-			buffer = &handle->buffers[handle->nr_buffers - 1];
-			buffer->name = strdup(buf + 8);
-			if (!buffer->name) {
-				free(handle->buffers);
-				handle->buffers = NULL;
-				return -ENOMEM;
-			}
-			offset = *(unsigned long long *)buf;
-			buffer->offset = tep_read_number(handle->pevent, &offset, 8);
+		case TRACECMD_OPTION_BUFFER_TEXT:
+			ret = handle_buffer_option(handle, option, buf, size);
+			if (ret < 0)
+				return ret;
 			break;
 		case TRACECMD_OPTION_TRACECLOCK:
 			if (!handle->ts2secs)
@@ -3757,6 +3853,13 @@ void tracecmd_ref(struct tracecmd_input *handle)
 	handle->ref++;
 }
 
+static inline void free_buffer(struct input_buffer_instance *buf)
+{
+	free(buf->name);
+	free(buf->clock);
+	free(buf->cpu_data);
+}
+
 /**
  * tracecmd_close - close and free the trace.dat handle
  * @handle: input handle for the trace.dat file
@@ -3813,8 +3916,9 @@ void tracecmd_close(struct tracecmd_input *handle)
 		free(del_sec);
 	}
 
+	free_buffer(&handle->top_buffer);
 	for (i = 0; i < handle->nr_buffers; i++)
-		free(handle->buffers[i].name);
+		free_buffer(&handle->buffers[i]);
 	free(handle->buffers);
 
 	tracecmd_free_hooks(handle->hooks);
@@ -4258,6 +4362,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 		return NULL;
 
 	*new_handle = *handle;
+	memset(&new_handle->top_buffer, 0, sizeof(new_handle->top_buffer));
 	new_handle->cpu_data = NULL;
 	new_handle->nr_buffers = 0;
 	new_handle->buffers = NULL;
-- 
2.33.1

Re: [PATCH v7 18/25] trace-cmd library: Read extended BUFFER option

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:41 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

The "space police" are back!

> The BUFFER option is extended in trace file version 7. It holds CPU
> metadata related to the recorded CPU traces. Also, there is a new
> BUFFER_TEXT option for describing the latency trace data. A new logic
> is implemented for these new options.

space

> In trace file version 7, the top buffer is saved as other buffers in the
> file, no special treatment. But saving the top buffer in the list of
> buffers in the input handler causes problems. It breaks the legacy logic
> of trace-cmd library users, which have special logic for trace buffers
> processing. That's why "top_buffer" member is added in the input handler
> structure, to hold the top buffer.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 139 +++++++++++++++++++++++++++++++-----
>  1 file changed, 122 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 6cc8ee90..5625e367 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -74,9 +74,19 @@ struct cpu_data {
>  	int			pipe_fd;
>  };
>  
> +struct cpu_file_data {
> +	int			cpu;
> +	unsigned long long	offset;
> +	unsigned long long	size;
> +};
> +
>  struct input_buffer_instance {
>  	char			*name;
>  	size_t			offset;
> +	char			*clock;
> +	bool			latency;
> +	int			cpus;
> +	struct cpu_file_data	*cpu_data;
>  };
>  
>  struct ts_offset_sample {
> @@ -156,6 +166,7 @@ struct tracecmd_input {
>  	char *			uname;
>  	char *			version;
>  	char *			trace_clock;
> +	struct input_buffer_instance	top_buffer;
>  	struct input_buffer_instance	*buffers;
>  	int			parsing_failures;
>  	struct guest_trace_info	*guest;
> @@ -2857,13 +2868,109 @@ __hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
>  	return handle->strings_size;
>  }
>  
> +static inline int save_read_number(struct tep_handle *tep, char *data, int *data_size,
> +				   int *read_pos, int bytes, unsigned long long *num)
> +{
> +	if (bytes > *data_size)
> +		return -1;

space

> +	*num = tep_read_number(tep, (data + *read_pos), bytes);
> +	*read_pos += bytes;
> +	*data_size -= bytes;
> +	return 0;
> +}
> +
> +static inline char *save_read_string(char *data, int *data_size, int *read_pos)
> +{
> +	char *str;
> +
> +	if (*data_size < 1)
> +		return NULL;

space

> +	str = strdup(data + *read_pos);
> +	if (!str)
> +		return NULL;

space

> +	*data_size -= (strlen(str) + 1);
> +	if (*data_size < 0) {
> +		free(str);
> +		return NULL;
> +	}

space

> +	*read_pos += (strlen(str) + 1);
> +
> +	return str;
> +}
> +
> +static int handle_buffer_option(struct tracecmd_input *handle,
> +				unsigned short id, char *data, int size)
> +{
> +	struct input_buffer_instance *buff;
> +	unsigned long long tmp;
> +	int rsize = 0;
> +	char *name;
> +	int i;
> +
> +	if (save_read_number(handle->pevent, data, &size, &rsize, 8, &tmp))
> +		return -1;

space

> +	name = save_read_string(data, &size, &rsize);
> +	if (!name)
> +		return -1;
> +
> +	if (*name == '\0') {
> +		/* top buffer */
> +		buff = &handle->top_buffer;
> +	} else {
> +		buff = realloc(handle->buffers, sizeof(*handle->buffers) * (handle->nr_buffers + 1));
> +		if (!buff) {
> +			free(name);
> +			return -1;
> +		}
> +		handle->buffers = buff;
> +		handle->nr_buffers++;
> +
> +		buff = &handle->buffers[handle->nr_buffers - 1];
> +	}
> +	memset(buff, 0, sizeof(struct input_buffer_instance));
> +	buff->name = name;
> +	buff->offset = tmp;

space

> +	if (!HAS_SECTIONS(handle))
> +		return 0;
> +
> +	/* file sections specific data */
> +	buff->clock = save_read_string(data, &size, &rsize);
> +	if (!buff->clock)
> +		return -1;

space

> +	if (*name == '\0' && !handle->trace_clock)
> +		handle->trace_clock = strdup(buff->clock);

space

> +	if (id == TRACECMD_OPTION_BUFFER) {
> +		if (save_read_number(handle->pevent, data, &size, &rsize, 4, &tmp))
> +			return -1;
> +		buff->cpus = tmp;
> +		if (!buff->cpus)
> +			return 0;
> +		buff->cpu_data = calloc(buff->cpus, sizeof(struct cpu_file_data));
> +		if (!buff->cpu_data)
> +			return -1;
> +		for (i = 0; i < buff->cpus; i++) {
> +			if (save_read_number(handle->pevent, data, &size, &rsize, 4, &tmp))
> +				return -1;
> +			buff->cpu_data[i].cpu = tmp;
> +			if (save_read_number(handle->pevent, data,
> +					     &size, &rsize, 8, &buff->cpu_data[i].offset))
> +				return -1;
> +			if (save_read_number(handle->pevent, data,
> +					     &size, &rsize, 8, &buff->cpu_data[i].size))
> +				return -1;
> +		}
> +	} else {
> +		buff->latency = true;
> +	}
> +	return 0;
> +}
> +

-- Steve

>  static int handle_options(struct tracecmd_input *handle)
>  {
>  	long long offset;
>  	unsigned short option;
>  	unsigned int size;
>  	char *cpustats = NULL;
> -	struct input_buffer_instance *buffer;
>  	struct hook_list *hook;
>  	char *buf;
>  	int cpus;
> @@ -2954,21 +3061,10 @@ static int handle_options(struct tracecmd_input *handle)
>  			handle->cpustats = cpustats;
>  			break;
>  		case TRACECMD_OPTION_BUFFER:
> -			/* A buffer instance is saved at the end of the file */
> -			handle->nr_buffers++;
> -			handle->buffers = realloc(handle->buffers,
> -						  sizeof(*handle->buffers) * handle->nr_buffers);
> -			if (!handle->buffers)
> -				return -ENOMEM;
> -			buffer = &handle->buffers[handle->nr_buffers - 1];
> -			buffer->name = strdup(buf + 8);
> -			if (!buffer->name) {
> -				free(handle->buffers);
> -				handle->buffers = NULL;
> -				return -ENOMEM;
> -			}
> -			offset = *(unsigned long long *)buf;
> -			buffer->offset = tep_read_number(handle->pevent, &offset, 8);
> +		case TRACECMD_OPTION_BUFFER_TEXT:
> +			ret = handle_buffer_option(handle, option, buf, size);
> +			if (ret < 0)
> +				return ret;
>  			break;
>  		case TRACECMD_OPTION_TRACECLOCK:
>  			if (!handle->ts2secs)
> @@ -3757,6 +3853,13 @@ void tracecmd_ref(struct tracecmd_input *handle)
>  	handle->ref++;
>  }
>  
> +static inline void free_buffer(struct input_buffer_instance *buf)
> +{
> +	free(buf->name);
> +	free(buf->clock);
> +	free(buf->cpu_data);
> +}
> +
>  /**
>   * tracecmd_close - close and free the trace.dat handle
>   * @handle: input handle for the trace.dat file
> @@ -3813,8 +3916,9 @@ void tracecmd_close(struct tracecmd_input *handle)
>  		free(del_sec);
>  	}
>  
> +	free_buffer(&handle->top_buffer);
>  	for (i = 0; i < handle->nr_buffers; i++)
> -		free(handle->buffers[i].name);
> +		free_buffer(&handle->buffers[i]);
>  	free(handle->buffers);
>  
>  	tracecmd_free_hooks(handle->hooks);
> @@ -4258,6 +4362,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
>  		return NULL;
>  
>  	*new_handle = *handle;
> +	memset(&new_handle->top_buffer, 0, sizeof(new_handle->top_buffer));
>  	new_handle->cpu_data = NULL;
>  	new_handle->nr_buffers = 0;
>  	new_handle->buffers = NULL;

[PATCH v7 19/25] trace-cmd library: Handle the extended DONE option

[Tzvetomir Stoyanov (VMware)]

In trace file version 7 the DONE option is extended to store the offset
in the file to the next options section. This way a list of options
sections can be stored in the file. Added logic to recursively read all
option sections from the file.

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

diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
index ac7e7f17..b4f3d8c8 100644
--- a/lib/trace-cmd/include/trace-cmd-local.h
+++ b/lib/trace-cmd/include/trace-cmd-local.h
@@ -53,6 +53,7 @@ struct cpu_data_source {
 int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
 		       struct cpu_data_source *data, const char *buff_name);
 off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
+unsigned long long get_last_option_offset(struct tracecmd_input *handle);
 unsigned int get_meta_strings_size(struct tracecmd_input *handle);
 
 #endif /* _TRACE_CMD_LOCAL_H */
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 5625e367..a25919a3 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -179,6 +179,7 @@ struct tracecmd_input {
 	struct file_section	*sections;
 	bool			options_init;
 	unsigned long long	options_start;
+	unsigned long long	options_last_offset;
 	size_t			total_file_size;
 
 	/* For custom profilers. */
@@ -2868,6 +2869,30 @@ __hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
 	return handle->strings_size;
 }
 
+__hidden unsigned long long get_last_option_offset(struct tracecmd_input *handle)
+{
+	return handle->options_last_offset;
+}
+
+static int handle_option_done(struct tracecmd_input *handle, char *buf, int size)
+{
+	unsigned long long offset;
+
+	if (size < 8)
+		return -1;
+	offset = lseek64(handle->fd, 0, SEEK_CUR);
+	if (offset >= size)
+		handle->options_last_offset = offset - size;
+	offset = tep_read_number(handle->pevent, buf, 8);
+	if (!offset)
+		return 0;
+
+	if (lseek64(handle->fd, offset, SEEK_SET) == (off_t)-1)
+		return -1;
+
+	return handle_options(handle);
+}
+
 static inline int save_read_number(struct tep_handle *tep, char *data, int *data_size,
 				   int *read_pos, int bytes, unsigned long long *num)
 {
@@ -2970,19 +2995,27 @@ static int handle_options(struct tracecmd_input *handle)
 	long long offset;
 	unsigned short option;
 	unsigned int size;
+	unsigned short id, flags;
 	char *cpustats = NULL;
 	struct hook_list *hook;
 	char *buf;
 	int cpus;
 	int ret;
 
-	handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
+	if (!HAS_SECTIONS(handle)) {
+		handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
+	} else {
+		if (read_section_header(handle, &id, &flags, NULL, NULL))
+			return -1;
+		if (id != TRACECMD_OPTION_DONE)
+			return -1;
+	}
 
 	for (;;) {
 		if (read2(handle, &option))
 			return -1;
 
-		if (option == TRACECMD_OPTION_DONE)
+		if (!HAS_SECTIONS(handle) && option == TRACECMD_OPTION_DONE)
 			break;
 
 		/* next 4 bytes is the size of the option */
@@ -3122,7 +3155,10 @@ static int handle_options(struct tracecmd_input *handle)
 			section_add_or_update(handle, option, -1,
 					      tep_read_number(handle->pevent, buf, 8), 0);
 			break;
-
+		case TRACECMD_OPTION_DONE:
+			ret = handle_option_done(handle, buf, size);
+			free(buf);
+			return ret;
 		default:
 			tracecmd_warning("unknown option %d", option);
 			break;
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 08f74c87..1fa83e93 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -2238,7 +2238,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
 	tep_ref(handle->pevent);
 	handle->page_size = tracecmd_page_size(ihandle);
 	handle->file_version = tracecmd_get_in_file_version(ihandle);
-	handle->options_start = tracecmd_get_options_offset(ihandle);
+	handle->options_start = get_last_option_offset(ihandle);
 	handle->strings_offs = get_meta_strings_size(ihandle);
 	list_head_init(&handle->options);
 	list_head_init(&handle->buffers);
-- 
2.33.1

Re: [PATCH v7 19/25] trace-cmd library: Handle the extended DONE option

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:42 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

I love my spaces ;-)

> In trace file version 7 the DONE option is extended to store the offset
> in the file to the next options section. This way a list of options
> sections can be stored in the file. Added logic to recursively read all
> option sections from the file.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/include/trace-cmd-local.h |  1 +
>  lib/trace-cmd/trace-input.c             | 42 +++++++++++++++++++++++--
>  lib/trace-cmd/trace-output.c            |  2 +-
>  3 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h
> index ac7e7f17..b4f3d8c8 100644
> --- a/lib/trace-cmd/include/trace-cmd-local.h
> +++ b/lib/trace-cmd/include/trace-cmd-local.h
> @@ -53,6 +53,7 @@ struct cpu_data_source {
>  int out_write_cpu_data(struct tracecmd_output *handle, int cpus,
>  		       struct cpu_data_source *data, const char *buff_name);
>  off64_t msg_lseek(struct tracecmd_msg_handle *msg_handle, off_t offset, int whence);
> +unsigned long long get_last_option_offset(struct tracecmd_input *handle);
>  unsigned int get_meta_strings_size(struct tracecmd_input *handle);
>  
>  #endif /* _TRACE_CMD_LOCAL_H */
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 5625e367..a25919a3 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -179,6 +179,7 @@ struct tracecmd_input {
>  	struct file_section	*sections;
>  	bool			options_init;
>  	unsigned long long	options_start;
> +	unsigned long long	options_last_offset;
>  	size_t			total_file_size;
>  
>  	/* For custom profilers. */
> @@ -2868,6 +2869,30 @@ __hidden unsigned int get_meta_strings_size(struct tracecmd_input *handle)
>  	return handle->strings_size;
>  }
>  
> +__hidden unsigned long long get_last_option_offset(struct tracecmd_input *handle)
> +{
> +	return handle->options_last_offset;
> +}
> +
> +static int handle_option_done(struct tracecmd_input *handle, char *buf, int size)
> +{
> +	unsigned long long offset;
> +
> +	if (size < 8)
> +		return -1;

space

> +	offset = lseek64(handle->fd, 0, SEEK_CUR);
> +	if (offset >= size)
> +		handle->options_last_offset = offset - size;

space

> +	offset = tep_read_number(handle->pevent, buf, 8);
> +	if (!offset)
> +		return 0;
> +
> +	if (lseek64(handle->fd, offset, SEEK_SET) == (off_t)-1)
> +		return -1;
> +
> +	return handle_options(handle);
> +}
> +
>  static inline int save_read_number(struct tep_handle *tep, char *data, int *data_size,
>  				   int *read_pos, int bytes, unsigned long long *num)
>  {
> @@ -2970,19 +2995,27 @@ static int handle_options(struct tracecmd_input *handle)
>  	long long offset;
>  	unsigned short option;
>  	unsigned int size;
> +	unsigned short id, flags;
>  	char *cpustats = NULL;
>  	struct hook_list *hook;
>  	char *buf;
>  	int cpus;
>  	int ret;
>  
> -	handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
> +	if (!HAS_SECTIONS(handle)) {
> +		handle->options_start = lseek64(handle->fd, 0, SEEK_CUR);
> +	} else {
> +		if (read_section_header(handle, &id, &flags, NULL, NULL))
> +			return -1;
> +		if (id != TRACECMD_OPTION_DONE)
> +			return -1;
> +	}
>  
>  	for (;;) {
>  		if (read2(handle, &option))
>  			return -1;
>  
> -		if (option == TRACECMD_OPTION_DONE)
> +		if (!HAS_SECTIONS(handle) && option == TRACECMD_OPTION_DONE)
>  			break;
>  
>  		/* next 4 bytes is the size of the option */
> @@ -3122,7 +3155,10 @@ static int handle_options(struct tracecmd_input *handle)
>  			section_add_or_update(handle, option, -1,
>  					      tep_read_number(handle->pevent, buf, 8), 0);
>  			break;
> -
> +		case TRACECMD_OPTION_DONE:
> +			ret = handle_option_done(handle, buf, size);
> +			free(buf);
> +			return ret;

space

>  		default:
>  			tracecmd_warning("unknown option %d", option);
>  			break;

-- Steve

> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 08f74c87..1fa83e93 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -2238,7 +2238,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
>  	tep_ref(handle->pevent);
>  	handle->page_size = tracecmd_page_size(ihandle);
>  	handle->file_version = tracecmd_get_in_file_version(ihandle);
> -	handle->options_start = tracecmd_get_options_offset(ihandle);
> +	handle->options_start = get_last_option_offset(ihandle);
>  	handle->strings_offs = get_meta_strings_size(ihandle);
>  	list_head_init(&handle->options);
>  	list_head_init(&handle->buffers);

[PATCH v7 20/25] trace-cmd library: Initialize CPU data for reading from version 7 trace files

[Tzvetomir Stoyanov (VMware)]

In version 7 trace files, CPU trace data is written in slightly
different way than in version 6 files. Added new CPU data initialization
flow, to handle version 7 files:
 - the top trace instance is saved in the same way as the other trace
   instances.
 - per CPU trace metadata is stored in the buffer option.
 - trace data section has section header.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index a25919a3..fe3d6e77 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3203,34 +3203,18 @@ static int read_options_type(struct tracecmd_input *handle)
 	return 0;
 }
 
-static int read_cpu_data(struct tracecmd_input *handle)
+static int init_cpu_data(struct tracecmd_input *handle)
 {
-	struct tep_handle *pevent = handle->pevent;
 	enum kbuffer_long_size long_size;
 	enum kbuffer_endian endian;
-	unsigned long long size;
 	unsigned long long max_size = 0;
 	unsigned long long pages;
-	int cpus;
 	int cpu;
 
-	/*
-	 * Check if this is a latency report or not.
-	 */
-	if (handle->file_state == TRACECMD_FILE_CPU_LATENCY)
-		return 1;
-
 	/* We expect this to be flyrecord */
 	if (handle->file_state != TRACECMD_FILE_CPU_FLYRECORD)
 		return -1;
 
-	cpus = handle->cpus;
-
-	handle->cpu_data = malloc(sizeof(*handle->cpu_data) * handle->cpus);
-	if (!handle->cpu_data)
-		return -1;
-	memset(handle->cpu_data, 0, sizeof(*handle->cpu_data) * handle->cpus);
-
 	if (force_read)
 		handle->read_page = true;
 
@@ -3245,32 +3229,14 @@ static int read_cpu_data(struct tracecmd_input *handle)
 		endian = KBUFFER_ENDIAN_LITTLE;
 
 	for (cpu = 0; cpu < handle->cpus; cpu++) {
-		unsigned long long offset;
-
-		handle->cpu_data[cpu].cpu = cpu;
-
 		handle->cpu_data[cpu].kbuf = kbuffer_alloc(long_size, endian);
 		if (!handle->cpu_data[cpu].kbuf)
 			goto out_free;
-		if (tep_is_old_format(pevent))
+		if (tep_is_old_format(handle->pevent))
 			kbuffer_set_old_format(handle->cpu_data[cpu].kbuf);
 
-		read8(handle, &offset);
-		read8(handle, &size);
-
-		handle->cpu_data[cpu].file_offset = offset;
-		handle->cpu_data[cpu].file_size = size;
-		if (size > max_size)
-			max_size = size;
-
-		if (size && (offset + size > handle->total_file_size)) {
-			/* this happens if the file got truncated */
-			printf("File possibly truncated. "
-				"Need at least %llu, but file size is %zu.\n",
-				offset + size, handle->total_file_size);
-			errno = EINVAL;
-			goto out_free;
-		}
+		if (handle->cpu_data[cpu].file_size > max_size)
+			max_size = handle->cpu_data[cpu].file_size;
 	}
 
 	/* Calculate about a meg of pages for buffering */
@@ -3288,6 +3254,101 @@ static int read_cpu_data(struct tracecmd_input *handle)
 			goto out_free;
 	}
 
+	return 0;
+
+ out_free:
+	for ( ; cpu >= 0; cpu--) {
+		free_page(handle, cpu);
+		kbuffer_free(handle->cpu_data[cpu].kbuf);
+		handle->cpu_data[cpu].kbuf = NULL;
+	}
+	return -1;
+}
+
+static int init_buffer_cpu_data(struct tracecmd_input *handle, struct input_buffer_instance *buffer)
+{
+	unsigned long long offset;
+	unsigned long long size;
+	unsigned short id, flags;
+	int cpu;
+
+	if (handle->cpu_data)
+		return -1;
+
+	if (lseek64(handle->fd, buffer->offset, SEEK_SET) == (off_t)-1)
+		return -1;
+	if (read_section_header(handle, &id, &flags, NULL, NULL))
+		return -1;
+
+	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
+	handle->cpus = buffer->cpus;
+	if (handle->max_cpu < handle->cpus)
+		handle->max_cpu = handle->cpus;
+
+	handle->cpu_data = calloc(handle->cpus, sizeof(*handle->cpu_data));
+	if (!handle->cpu_data)
+		return -1;
+
+	for (cpu = 0; cpu < handle->cpus; cpu++) {
+		handle->cpu_data[cpu].cpu = buffer->cpu_data[cpu].cpu;
+		offset = buffer->cpu_data[cpu].offset;
+		size = buffer->cpu_data[cpu].size;
+		handle->cpu_data[cpu].file_offset = offset;
+		handle->cpu_data[cpu].file_size = size;
+		if (size && (offset + size > handle->total_file_size)) {
+			/* this happens if the file got truncated */
+			printf("File possibly truncated. "
+				"Need at least %llu, but file size is %zu.\n",
+				offset + size, handle->total_file_size);
+			errno = EINVAL;
+			return -1;
+		}
+	}
+
+	return init_cpu_data(handle);
+}
+
+static int read_cpu_data(struct tracecmd_input *handle)
+{
+	unsigned long long size;
+	int cpus;
+	int cpu;
+
+	/*
+	 * Check if this is a latency report or not.
+	 */
+	if (handle->file_state == TRACECMD_FILE_CPU_LATENCY)
+		return 1;
+
+	/* We expect this to be flyrecord */
+	if (handle->file_state != TRACECMD_FILE_CPU_FLYRECORD)
+		return -1;
+
+	cpus = handle->cpus;
+
+	handle->cpu_data = malloc(sizeof(*handle->cpu_data) * handle->cpus);
+	if (!handle->cpu_data)
+		return -1;
+	memset(handle->cpu_data, 0, sizeof(*handle->cpu_data) * handle->cpus);
+
+	for (cpu = 0; cpu < handle->cpus; cpu++) {
+		unsigned long long offset;
+
+		handle->cpu_data[cpu].cpu = cpu;
+		read8(handle, &offset);
+		read8(handle, &size);
+		handle->cpu_data[cpu].file_offset = offset;
+		handle->cpu_data[cpu].file_size = size;
+		if (size && (offset + size > handle->total_file_size)) {
+			/* this happens if the file got truncated */
+			printf("File possibly truncated. "
+				"Need at least %llu, but file size is %zu.\n",
+				offset + size, handle->total_file_size);
+			errno = EINVAL;
+			return -1;
+		}
+	}
+
 	/*
 	 * It is possible that an option changed the number of CPUs.
 	 * If that happened, then there's "empty" cpu data saved for
@@ -3307,15 +3368,7 @@ static int read_cpu_data(struct tracecmd_input *handle)
 		}
 	}
 
-	return 0;
-
- out_free:
-	for ( ; cpu >= 0; cpu--) {
-		free_page(handle, cpu);
-		kbuffer_free(handle->cpu_data[cpu].kbuf);
-		handle->cpu_data[cpu].kbuf = NULL;
-	}
-	return -1;
+	return init_cpu_data(handle);
 }
 
 static int read_data_and_size(struct tracecmd_input *handle,
@@ -3413,14 +3466,7 @@ static int read_and_parse_trace_clock(struct tracecmd_input *handle,
 	return 0;
 }
 
-/**
- * tracecmd_init_data - prepare reading the data from trace.dat
- * @handle: input handle for the trace.dat file
- *
- * This prepares reading the data from trace.dat. This is called
- * after tracecmd_read_headers() and before tracecmd_read_data().
- */
-int tracecmd_init_data(struct tracecmd_input *handle)
+static int init_data_v6(struct tracecmd_input *handle)
 {
 	struct tep_handle *pevent = handle->pevent;
 	int ret;
@@ -3442,7 +3488,29 @@ int tracecmd_init_data(struct tracecmd_input *handle)
 			tracecmd_parse_trace_clock(handle, clock, 8);
 		}
 	}
+	return ret;
+}
+
+static int init_data_v7(struct tracecmd_input *handle)
+{
+	return init_buffer_cpu_data(handle, &handle->top_buffer);
+}
 
+/**
+ * tracecmd_init_data - prepare reading the data from trace.dat
+ * @handle: input handle for the trace.dat file
+ *
+ * This prepares reading the data from trace.dat. This is called
+ * after tracecmd_read_headers() and before tracecmd_read_data().
+ */
+int tracecmd_init_data(struct tracecmd_input *handle)
+{
+	int ret;
+
+	if (!HAS_SECTIONS(handle))
+		ret = init_data_v6(handle);
+	else
+		ret = init_data_v7(handle);
 	tracecmd_blk_hack(handle);
 
 	return ret;
-- 
2.33.1

[PATCH v7 21/25] trace-cmd library: Handle latency trace in version 7 files

[Tzvetomir Stoyanov (VMware)]

Latency trace data is saved the same was as flyrecord buffer data
in trace files version 7. There is a BUFFER_TEXT option which holds the
latency specific trace metadata and points to the section in the file
with the trace data. A new API is added to read latency data:
 tracecmd_latency_data_read()

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  5 +++-
 lib/trace-cmd/trace-input.c                   | 29 ++++++++++++++++++-
 lib/trace-cmd/trace-output.c                  |  5 +++-
 tracecmd/trace-record.c                       |  3 +-
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 2e0c32b6..6fa3ca3a 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -222,6 +222,8 @@ tracecmd_peek_data_ref(struct tracecmd_input *handle, int cpu)
 	return rec;
 }
 
+int tracecmd_latency_data_read(struct tracecmd_input *handle, char **buf, size_t *size);
+
 struct tep_record *
 tracecmd_read_prev(struct tracecmd_input *handle, struct tep_record *record);
 
@@ -296,7 +298,8 @@ int tracecmd_output_write_headers(struct tracecmd_output *handle,
 
 struct tracecmd_output *tracecmd_output_create(const char *output_file);
 struct tracecmd_output *tracecmd_output_create_fd(int fd);
-struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus);
+struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus,
+						     int file_version);
 
 struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
 					    unsigned short id, int size,
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index fe3d6e77..5c900e94 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3203,6 +3203,24 @@ static int read_options_type(struct tracecmd_input *handle)
 	return 0;
 }
 
+int tracecmd_latency_data_read(struct tracecmd_input *handle, char **buf, size_t *size)
+{
+	if (!handle || !buf || !size)
+		return -1;
+	if (handle->file_state != TRACECMD_FILE_CPU_LATENCY)
+		return -1;
+
+	/* Read data from a file */
+	if (!(*buf)) {
+		*size = BUFSIZ;
+		*buf = malloc(*size);
+		if (!(*buf))
+			return -1;
+	}
+
+	return do_read(handle, *buf, *size);
+}
+
 static int init_cpu_data(struct tracecmd_input *handle)
 {
 	enum kbuffer_long_size long_size;
@@ -3265,6 +3283,12 @@ static int init_cpu_data(struct tracecmd_input *handle)
 	return -1;
 }
 
+int init_latency_data(struct tracecmd_input *handle)
+{
+	/* To do */
+	return 0;
+}
+
 static int init_buffer_cpu_data(struct tracecmd_input *handle, struct input_buffer_instance *buffer)
 {
 	unsigned long long offset;
@@ -3279,7 +3303,10 @@ static int init_buffer_cpu_data(struct tracecmd_input *handle, struct input_buff
 		return -1;
 	if (read_section_header(handle, &id, &flags, NULL, NULL))
 		return -1;
-
+	if (buffer->latency) {
+		handle->file_state = TRACECMD_FILE_CPU_LATENCY;
+		return init_latency_data(handle) == 0 ? 1 : -1;
+	}
 	handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
 	handle->cpus = buffer->cpus;
 	if (handle->max_cpu < handle->cpus)
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 1fa83e93..38a0699b 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -1872,7 +1872,8 @@ out_add_buffer_option_v7(struct tracecmd_output *handle, const char *name,
 	return option;
 }
 
-struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus)
+struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus,
+						     int file_version)
 {
 	enum tracecmd_section_flags flags = 0;
 	struct tracecmd_output *handle;
@@ -1883,6 +1884,8 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
 	if (!handle)
 		return NULL;
 
+	if (file_version && tracecmd_output_set_version(handle, file_version))
+		goto out_free;
 	if (tracecmd_output_write_headers(handle, NULL))
 		goto out_free;
 	/*
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index f599610e..2dea79dc 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -4501,7 +4501,8 @@ static void record_data(struct common_record_context *ctx)
 		return;
 
 	if (latency) {
-		handle = tracecmd_create_file_latency(ctx->output, local_cpu_count);
+		handle = tracecmd_create_file_latency(ctx->output, local_cpu_count,
+						      ctx->file_version);
 		tracecmd_set_quiet(handle, quiet);
 	} else {
 		if (!local_cpu_count)
-- 
2.33.1

[PATCH v7 22/25] trace-cmd library: Handle buffer trace data init for version 7 files

[Tzvetomir Stoyanov (VMware)]

CPU data initialization is different for trace files version 6 and 7.
When a new input handler to trace buffer is created, initialize the CPU
data according to the file version.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 5c900e94..bc4e2885 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -4522,34 +4522,37 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
 	new_handle->flags |= TRACECMD_FL_BUFFER_INSTANCE;
 
 	new_handle->pid_maps = NULL;
+	if (!HAS_SECTIONS(handle)) {
+		/* Save where we currently are */
+		offset = lseek64(handle->fd, 0, SEEK_CUR);
 
-	/* Save where we currently are */
-	offset = lseek64(handle->fd, 0, SEEK_CUR);
-
-	ret = lseek64(handle->fd, buffer->offset, SEEK_SET);
-	if (ret < 0) {
-		tracecmd_warning("could not seek to buffer %s offset %ld\n",
-				  buffer->name, buffer->offset);
-		goto error;
-	}
-
-	/*
-	 * read_options_type() is called right after the CPU count so update
-	 * file state accordingly.
-	 */
-	new_handle->file_state = TRACECMD_FILE_CPU_COUNT;
-	ret = read_options_type(new_handle);
-	if (!ret)
-		ret = read_cpu_data(new_handle);
-	if (ret < 0) {
-		tracecmd_warning("failed to read sub buffer %s\n", buffer->name);
-		goto error;
-	}
+		ret = lseek64(handle->fd, buffer->offset, SEEK_SET);
+		if (ret == (off64_t)-1) {
+			tracecmd_warning("could not seek to buffer %s offset %ld\n",
+					  buffer->name, buffer->offset);
+			goto error;
+		}
+		/*
+		 * read_options_type() is called right after the CPU count so update
+		 * file state accordingly.
+		 */
+		new_handle->file_state = TRACECMD_FILE_CPU_COUNT;
+		ret = read_options_type(new_handle);
+		if (!ret)
+			ret = read_cpu_data(new_handle);
 
-	ret = lseek64(handle->fd, offset, SEEK_SET);
-	if (ret < 0) {
-		tracecmd_warning("could not seek to back to offset %ld\n", offset);
-		goto error;
+		if (ret < 0) {
+			tracecmd_warning("failed to read sub buffer %s\n", buffer->name);
+			goto error;
+		}
+		ret = lseek64(handle->fd, offset, SEEK_SET);
+		if (ret < 0) {
+			tracecmd_warning("could not seek to back to offset %ld\n", offset);
+			goto error;
+		}
+	} else {
+		if (init_buffer_cpu_data(new_handle, buffer) < 0)
+			goto error;
 	}
 
 	return new_handle;
-- 
2.33.1

[PATCH v7 23/25] trace-cmd report: Use the new latency API to read data

[Tzvetomir Stoyanov (VMware)]

When reading latency trace data, use the new API
 tracecmd_latency_data_read()
It handles reading latency trace data from both version 6 and 7 trace
files.

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

diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
index 4b27740a..24177c44 100644
--- a/tracecmd/trace-read.c
+++ b/tracecmd/trace-read.c
@@ -951,18 +951,20 @@ void trace_show_data(struct tracecmd_input *handle, struct tep_record *record)
 	printf("\n");
 }
 
-static void read_rest(void)
+static void read_latency(struct tracecmd_input *handle)
 {
-	char buf[BUFSIZ + 1];
+	char *buf = NULL;
+	size_t size = 0;
 	int r;
 
 	do {
-		r = read(input_fd, buf, BUFSIZ);
-		if (r > 0) {
-			buf[r] = 0;
-			printf("%s", buf);
-		}
+		r = tracecmd_latency_data_read(handle, &buf, &size);
+		if (r > 0)
+			printf("%.*s", r, buf);
 	} while (r > 0);
+
+	printf("\n");
+	free(buf);
 }
 
 static int
@@ -1246,7 +1248,7 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 		if (ret > 0) {
 			if (multi_inputs)
 				die("latency traces do not work with multiple inputs");
-			read_rest();
+			read_latency(handles->handle);
 			return;
 		}
 
-- 
2.33.1

[PATCH v7 24/25] trace-cmd: Write buffers metadata in trace files version 6

[Tzvetomir Stoyanov (VMware)]

When creating a trace file, tracecmd_write_buffer_info() should be
called, to write buffers metadata in file version 6 trace files.

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

diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 45ba1211..874ecd61 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -604,6 +604,9 @@ static int put_together_file(int cpus, int ofd, const char *node,
 
 	if (write_options) {
 		ret = tracecmd_write_cpus(handle, cpus);
+		if (ret)
+			goto out;
+		ret = tracecmd_write_buffer_info(handle);
 		if (ret)
 			goto out;
 		ret = tracecmd_write_options(handle);
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 2dea79dc..8567ae5c 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3729,6 +3729,9 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
 		if (ret)
 			goto error;
 		ret = tracecmd_write_cpus(network_handle, instance->cpu_count);
+		if (ret)
+			goto error;
+		ret = tracecmd_write_buffer_info(network_handle);
 		if (ret)
 			goto error;
 		ret = tracecmd_write_options(network_handle);
@@ -4092,6 +4095,7 @@ static void setup_agent(struct buffer_instance *instance,
 	add_options(network_handle, ctx);
 	tracecmd_write_cmdlines(network_handle);
 	tracecmd_write_cpus(network_handle, instance->cpu_count);
+	tracecmd_write_buffer_info(network_handle);
 	tracecmd_write_options(network_handle);
 	tracecmd_write_meta_strings(network_handle);
 	tracecmd_msg_finish_sending_data(instance->msg_handle);
-- 
2.33.1

[PATCH v7 25/25] trace-cmd report: Add new parameter for trace file version

[Tzvetomir Stoyanov (VMware)]

By default, "trace-cmd report" writes in trace file version 6.
A new parameter is added, which can be used to set desired version
the output trace file.
 "trace-cmd report --file-version <version>"

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 22 +++++++++++++++++++++-
 tracecmd/trace-usage.c  |  2 +-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 8567ae5c..1b806e78 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3698,6 +3698,8 @@ static struct tracecmd_output *create_net_output(struct common_record_context *c
 	out = tracecmd_output_create(NULL);
 	if (!out)
 		return NULL;
+	if (ctx->file_version && tracecmd_output_set_version(out, ctx->file_version))
+		goto error;
 	if (tracecmd_output_set_msg(out, msg_handle))
 		goto error;
 	if (tracecmd_output_write_headers(out, listed_events))
@@ -3744,6 +3746,8 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
 		network_handle = tracecmd_output_create_fd(msg_handle->fd);
 		if (!network_handle)
 			goto error;
+		if (tracecmd_output_set_version(network_handle, ctx->file_version))
+			goto error;
 		if (tracecmd_output_write_headers(network_handle, listed_events))
 			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
@@ -4471,7 +4475,8 @@ static struct tracecmd_output *create_output(struct common_record_context *ctx)
 	out = tracecmd_output_create(ctx->output);
 	if (!out)
 		goto error;
-
+	if (ctx->file_version && tracecmd_output_set_version(out, ctx->file_version))
+		goto error;
 	if (tracecmd_output_write_headers(out, listed_events))
 		goto error;
 
@@ -5780,6 +5785,7 @@ void init_top_instance(void)
 }
 
 enum {
+	OPT_file_ver		= 238,
 	OPT_verbose		= 239,
 	OPT_tsc2nsec		= 240,
 	OPT_fork		= 241,
@@ -6219,6 +6225,7 @@ static void parse_record_options(int argc,
 			{"tsc2nsec", no_argument, NULL, OPT_tsc2nsec},
 			{"poll", no_argument, NULL, OPT_poll},
 			{"verbose", optional_argument, NULL, OPT_verbose},
+			{"file-version", required_argument, NULL, OPT_file_ver},
 			{NULL, 0, NULL, 0}
 		};
 
@@ -6644,6 +6651,19 @@ static void parse_record_options(int argc,
 			cmd_check_die(ctx, CMD_set, *(argv+1), "--poll");
 			recorder_flags |= TRACECMD_RECORD_POLL;
 			break;
+		case OPT_file_ver:
+			cmd_check_die(ctx, CMD_start, *(argv+1), "--file_version");
+			cmd_check_die(ctx, CMD_set, *(argv+1), "--file_version");
+			cmd_check_die(ctx, CMD_extract, *(argv+1), "--file_version");
+			cmd_check_die(ctx, CMD_stream, *(argv+1), "--file_version");
+			cmd_check_die(ctx, CMD_profile, *(argv+1), "--file_version");
+			ctx->file_version = atoi(optarg);
+			if (ctx->file_version < FILE_VERSION_MIN ||
+			    ctx->file_version > FILE_VERSION_MAX)
+				die("Unsupported file version %d, "
+				    "supported versions are from %d to %d",
+				    ctx->file_version, FILE_VERSION_MIN, FILE_VERSION_MAX);
+			break;
 		case OPT_quiet:
 		case 'q':
 			quiet = true;
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index 32b38bfd..ac12b066 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -69,7 +69,7 @@ static struct usage_help usage_help[] = {
 		"               If 0 is specified, no loop is performed - timestamps offset is calculated only twice,"
 		"                                                         at the beginnig and at the end of the trace\n"
 		"          --poll don't block while reading from the trace buffer\n"
-		"          --verbose 'level' Set the desired log level\n"
+		"          --file-version set the desired trace file version\n"
 	},
 	{
 		"set",
-- 
2.33.1

Re: [PATCH v7 25/25] trace-cmd report: Add new parameter for trace file version

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:48 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> By default, "trace-cmd report" writes in trace file version 6.

I think you mean "trace-cmd record" (and for the subject as well).

> A new parameter is added, which can be used to set desired version
> the output trace file.

space

>  "trace-cmd report --file-version <version>"

   "trace-cmd record --file-version <version>"

> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  tracecmd/trace-record.c | 22 +++++++++++++++++++++-
>  tracecmd/trace-usage.c  |  2 +-
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 8567ae5c..1b806e78 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -3698,6 +3698,8 @@ static struct tracecmd_output *create_net_output(struct common_record_context *c
>  	out = tracecmd_output_create(NULL);
>  	if (!out)
>  		return NULL;
> +	if (ctx->file_version && tracecmd_output_set_version(out, ctx->file_version))
> +		goto error;
>  	if (tracecmd_output_set_msg(out, msg_handle))
>  		goto error;
>  	if (tracecmd_output_write_headers(out, listed_events))
> @@ -3744,6 +3746,8 @@ setup_connection(struct buffer_instance *instance, struct common_record_context
>  		network_handle = tracecmd_output_create_fd(msg_handle->fd);
>  		if (!network_handle)
>  			goto error;
> +		if (tracecmd_output_set_version(network_handle, ctx->file_version))
> +			goto error;
>  		if (tracecmd_output_write_headers(network_handle, listed_events))
>  			goto error;
>  		tracecmd_set_quiet(network_handle, quiet);
> @@ -4471,7 +4475,8 @@ static struct tracecmd_output *create_output(struct common_record_context *ctx)
>  	out = tracecmd_output_create(ctx->output);
>  	if (!out)
>  		goto error;
> -
> +	if (ctx->file_version && tracecmd_output_set_version(out, ctx->file_version))
> +		goto error;
>  	if (tracecmd_output_write_headers(out, listed_events))
>  		goto error;
>  
> @@ -5780,6 +5785,7 @@ void init_top_instance(void)
>  }
>  
>  enum {
> +	OPT_file_ver		= 238,
>  	OPT_verbose		= 239,
>  	OPT_tsc2nsec		= 240,
>  	OPT_fork		= 241,
> @@ -6219,6 +6225,7 @@ static void parse_record_options(int argc,
>  			{"tsc2nsec", no_argument, NULL, OPT_tsc2nsec},
>  			{"poll", no_argument, NULL, OPT_poll},
>  			{"verbose", optional_argument, NULL, OPT_verbose},
> +			{"file-version", required_argument, NULL, OPT_file_ver},
>  			{NULL, 0, NULL, 0}
>  		};
>  
> @@ -6644,6 +6651,19 @@ static void parse_record_options(int argc,
>  			cmd_check_die(ctx, CMD_set, *(argv+1), "--poll");
>  			recorder_flags |= TRACECMD_RECORD_POLL;
>  			break;
> +		case OPT_file_ver:
> +			cmd_check_die(ctx, CMD_start, *(argv+1), "--file_version");
> +			cmd_check_die(ctx, CMD_set, *(argv+1), "--file_version");
> +			cmd_check_die(ctx, CMD_extract, *(argv+1), "--file_version");
> +			cmd_check_die(ctx, CMD_stream, *(argv+1), "--file_version");
> +			cmd_check_die(ctx, CMD_profile, *(argv+1), "--file_version");

Perhaps we should have something that just tests against record, instead of
listing everything that isn't record?

-- Steve

> +			ctx->file_version = atoi(optarg);
> +			if (ctx->file_version < FILE_VERSION_MIN ||
> +			    ctx->file_version > FILE_VERSION_MAX)
> +				die("Unsupported file version %d, "
> +				    "supported versions are from %d to %d",
> +				    ctx->file_version, FILE_VERSION_MIN, FILE_VERSION_MAX);
> +			break;
>  		case OPT_quiet:
>  		case 'q':
>  			quiet = true;
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index 32b38bfd..ac12b066 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -69,7 +69,7 @@ static struct usage_help usage_help[] = {
>  		"               If 0 is specified, no loop is performed - timestamps offset is calculated only twice,"
>  		"                                                         at the beginnig and at the end of the trace\n"
>  		"          --poll don't block while reading from the trace buffer\n"
> -		"          --verbose 'level' Set the desired log level\n"
> +		"          --file-version set the desired trace file version\n"
>  	},
>  	{
>  		"set",

Re: [PATCH v7 00/25] Trace file version 7 - sections

[Steven Rostedt]

On Fri, 10 Dec 2021 12:54:23 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Introduce the next version of the trace-cmd trace file - 7. Major changes
> since version 6 is more flexible extendable structure of the file, based on
> independent sections.
> 
> v7 changes:
>  - Rebased on top of the latest master.
>  - Introduced new metadata strings section.
>  - Use 8 bytes for section size. 

I finished my review of this series. Looks good!

I'm hoping that v8 will be the final one.

Thanks Tzvetomir,

-- Steve

Re: [PATCH v7 00/25] Trace file version 7 - sections

[Tzvetomir Stoyanov]

On Sat, Jan 15, 2022 at 6:21 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 10 Dec 2021 12:54:23 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > Introduce the next version of the trace-cmd trace file - 7. Major changes
> > since version 6 is more flexible extendable structure of the file, based on
> > independent sections.
> >
> > v7 changes:
> >  - Rebased on top of the latest master.
> >  - Introduced new metadata strings section.
> >  - Use 8 bytes for section size.
>
> I finished my review of this series. Looks good!
>
> I'm hoping that v8 will be the final one.
>
> Thanks Tzvetomir,
>
> -- Steve

Thanks Steven!
I'll submit the next version this week.

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