linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Save tracee memory map into trace.dat file
@ 2019-06-17 11:52 tz.stoyanov
  2019-06-17 11:52 ` [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v() tz.stoyanov
  2019-06-17 11:52 ` [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file tz.stoyanov
  0 siblings, 2 replies; 8+ messages in thread
From: tz.stoyanov @ 2019-06-17 11:52 UTC (permalink / raw)
  To: rostedt, y.karadz; +Cc: linux-trace-devel

From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>

New trace-cmd option is introduced, "--mmap". When it is used together
with -F or -P options, the memory map of the traced applications is
stored into the trace.dat file. The new API tracecmd_search_tracee_mmap()
can be used to do lookups into those memory maps.


Tzvetomir Stoyanov (1):
  trace-cmd: Implemented new API tracecmd_add_option_v()

Tzvetomir Stoyanov (VMware) (1):
  trace-cmd: Save the tracee memory map into the trace.dat file.

 include/trace-cmd/trace-cmd.h    |   9 +++
 include/traceevent/event-parse.h |   1 +
 lib/trace-cmd/trace-input.c      |  81 ++++++++++++++++++++
 tracecmd/include/trace-local.h   |  16 ++++
 tracecmd/trace-output.c          | 117 ++++++++++++++++++++++++-----
 tracecmd/trace-record.c          | 123 +++++++++++++++++++++++++++++++
 6 files changed, 330 insertions(+), 17 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v()
  2019-06-17 11:52 [PATCH 0/2] Save tracee memory map into trace.dat file tz.stoyanov
@ 2019-06-17 11:52 ` tz.stoyanov
  2019-06-17 13:06   ` Steven Rostedt
  2019-06-17 11:52 ` [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file tz.stoyanov
  1 sibling, 1 reply; 8+ messages in thread
From: tz.stoyanov @ 2019-06-17 11:52 UTC (permalink / raw)
  To: rostedt, y.karadz; +Cc: linux-trace-devel

From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>

From: Tzvetomir Stoyanov <tstoyanov@vmware.com>

This patch implements a new tracecmd API, tracecmd_add_option_v()
It adds new option in trace.dat, similar to tracecmd_add_option(),
but the option's data is passed as list of buffers. The standard
struct iovec is used as input parameter, containing the option's
data buffers.

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 include/trace-cmd/trace-cmd.h    |   5 ++
 include/traceevent/event-parse.h |   1 +
 tracecmd/trace-output.c          | 117 ++++++++++++++++++++++++++-----
 3 files changed, 106 insertions(+), 17 deletions(-)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index ceb03f4..3919673 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -245,11 +245,16 @@ struct tracecmd_output *tracecmd_create_init_file_override(const char *output_fi
 struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
 					    unsigned short id, int size,
 					    const void *data);
+struct tracecmd_option *
+tracecmd_add_option_v(struct tracecmd_output *handle,
+		    unsigned short id, const struct iovec *vector, int count);
+
 struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle,
 						   const char *name, int cpus);
 
 int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus);
 int tracecmd_write_options(struct tracecmd_output *handle);
+int tracecmd_append_options(struct tracecmd_output *handle);
 int tracecmd_update_option(struct tracecmd_output *handle,
 			   struct tracecmd_option *option, int size,
 			   const void *data);
diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h
index 5e0fd19..62057b3 100644
--- a/include/traceevent/event-parse.h
+++ b/include/traceevent/event-parse.h
@@ -11,6 +11,7 @@
 #include <stdio.h>
 #include <regex.h>
 #include <string.h>
+#include <sys/uio.h>
 
 #include "trace-seq.h"
 
diff --git a/tracecmd/trace-output.c b/tracecmd/trace-output.c
index 33d6ce3..f7a2791 100644
--- a/tracecmd/trace-output.c
+++ b/tracecmd/trace-output.c
@@ -883,21 +883,23 @@ static struct tracecmd_output *create_file(const char *output_file,
 }
 
 /**
- * tracecmd_add_option - add options to the file
+ * tracecmd_add_option_v - add options to the file
  * @handle: the output file handle name
  * @id: the id of the option
- * @size: the size of the option data
- * @data: the data to write to the file.
+ * @vector: array of vectors, pointing to the data to write in the file
+ * @count: number of items in the vector array
  *
  * Returns handle to update option if needed.
  *  Just the content can be updated, with smaller or equal to
  *  content than the specified size.
  */
 struct tracecmd_option *
-tracecmd_add_option(struct tracecmd_output *handle,
-		    unsigned short id, int size, const void *data)
+tracecmd_add_option_v(struct tracecmd_output *handle,
+		    unsigned short id, const struct iovec *vector, int count)
 {
 	struct tracecmd_option *option;
+	char *data = NULL;
+	int i, size = 0;
 
 	/*
 	 * We can only add options before they were written.
@@ -906,32 +908,63 @@ tracecmd_add_option(struct tracecmd_output *handle,
 	if (handle->options_written)
 		return NULL;
 
-	handle->nr_options++;
+	for (i = 0; i < count; i++)
+		size += vector[i].iov_len;
+
+	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass vector with 0 / NULL data */
+	if (size) {
+		data = malloc(size);
+		if (!data) {
+			warning("Insufficient memory");
+			return NULL;
+		}
+	}
 
 	option = malloc(sizeof(*option));
 	if (!option) {
 		warning("Could not allocate space for option");
+		free(data);
 		return NULL;
 	}
 
-	option->id = id;
-	option->size = size;
-	option->data = malloc(size);
-	if (!option->data) {
-		warning("Insufficient memory");
-		free(option);
-		return NULL;
+	handle->nr_options++;
+	option->data = data;
+	for (i = 0; i < count; i++) {
+		if (vector[i].iov_base && vector[i].iov_len) {
+			memcpy(data, vector[i].iov_base, vector[i].iov_len);
+			data += vector[i].iov_len;
+		}
 	}
-
-	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass 0 / NULL data */
-	if (size)
-		memcpy(option->data, data, size);
+	option->size = size;
+	option->id = id;
 
 	list_add_tail(&option->list, &handle->options);
 
 	return option;
 }
 
+/**
+ * tracecmd_add_option - add options to the file
+ * @handle: the output file handle name
+ * @id: the id of the option
+ * @size: the size of the option data
+ * @data: the data to write to the file.
+ *
+ * Returns handle to update option if needed.
+ *  Just the content can be updated, with smaller or equal to
+ *  content than the specified size.
+ */
+struct tracecmd_option *
+tracecmd_add_option(struct tracecmd_output *handle,
+		    unsigned short id, int size, const void *data)
+{
+	struct iovec vect;
+
+	vect.iov_base = (void *) data;
+	vect.iov_len = size;
+	return tracecmd_add_option_v(handle, id, &vect, 1);
+}
+
 int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
 {
 	cpus = convert_endian_4(handle, cpus);
@@ -979,6 +1012,56 @@ int tracecmd_write_options(struct tracecmd_output *handle)
 	return 0;
 }
 
+int tracecmd_append_options(struct tracecmd_output *handle)
+{
+	struct tracecmd_option *options;
+	unsigned short option;
+	unsigned short endian2;
+	unsigned int endian4;
+	off_t offset;
+	int r;
+
+	/* If already written, ignore */
+	if (handle->options_written)
+		return 0;
+
+	if (lseek64(handle->fd, 0, SEEK_END) == (off_t)-1)
+		return -1;
+	offset = lseek64(handle->fd, -2, SEEK_CUR);
+	if (offset == (off_t)-1)
+		return -1;
+
+	r = pread(handle->fd, &option, 2, offset);
+	if (r != 2 || option != TRACECMD_OPTION_DONE)
+		return -1;
+
+	list_for_each_entry(options, &handle->options, list) {
+		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 in case it needs to be updated */
+		options->offset = lseek64(handle->fd, 0, SEEK_CUR);
+
+		if (do_write_check(handle, options->data,
+				   options->size))
+			return -1;
+	}
+
+	option = TRACECMD_OPTION_DONE;
+
+	if (do_write_check(handle, &option, 2))
+		return -1;
+
+	handle->options_written = 1;
+
+	return 0;
+}
+
 int tracecmd_update_option(struct tracecmd_output *handle,
 			   struct tracecmd_option *option, int size,
 			   const void *data)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file.
  2019-06-17 11:52 [PATCH 0/2] Save tracee memory map into trace.dat file tz.stoyanov
  2019-06-17 11:52 ` [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v() tz.stoyanov
@ 2019-06-17 11:52 ` tz.stoyanov
  2019-06-17 13:37   ` Steven Rostedt
  1 sibling, 1 reply; 8+ messages in thread
From: tz.stoyanov @ 2019-06-17 11:52 UTC (permalink / raw)
  To: rostedt, y.karadz; +Cc: linux-trace-devel

From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>

A new trace-cmd record option is added: "--mmap". When it is set with
combination of -F or -P options, the memory map of the traced applications
is stored in the trace.dat file. A new API tracecmd_search_tracee_mmap()
can be used to look up into stored memory maps. The map is retrieved from
/proc/<pid>/maps file.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/trace-cmd/trace-cmd.h  |   4 ++
 lib/trace-cmd/trace-input.c    |  81 ++++++++++++++++++++++
 tracecmd/include/trace-local.h |  16 +++++
 tracecmd/trace-record.c        | 123 +++++++++++++++++++++++++++++++++
 4 files changed, 224 insertions(+)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 3919673..868e571 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -81,6 +81,7 @@ enum {
 	TRACECMD_OPTION_HOOK,
 	TRACECMD_OPTION_OFFSET,
 	TRACECMD_OPTION_CPUCOUNT,
+	TRACECMD_OPTION_PIDMMAPS,
 };
 
 enum {
@@ -206,6 +207,9 @@ unsigned long long tracecmd_page_ts(struct tracecmd_input *handle,
 unsigned int tracecmd_record_ts_delta(struct tracecmd_input *handle,
 				      struct tep_record *record);
 
+char *tracecmd_get_tracee_lib(struct tracecmd_input *handle,
+			      int pid, unsigned long addr);
+
 #ifndef SWIG
 /* hack for function graph work around */
 extern __thread struct tracecmd_input *tracecmd_curr_thread_handle;
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 264e3c3..aaf3c56 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -100,6 +100,7 @@ struct tracecmd_input {
 	struct tracecmd_ftrace	finfo;
 
 	struct hook_list	*hooks;
+	struct pid_mem_maps	*pid_mmaps;
 	/* file information */
 	size_t			header_files_start;
 	size_t			ftrace_files_start;
@@ -2133,6 +2134,83 @@ void tracecmd_set_ts2secs(struct tracecmd_input *handle,
 	handle->use_trace_clock = false;
 }
 
+static void trace_pid_mmap_load(struct tracecmd_input *handle, char *buf)
+{
+	struct pid_mem_maps *maps;
+	char mapname[PATH_MAX+1];
+	char *line;
+	int ret;
+	int i;
+
+	maps = calloc(1, sizeof(*maps));
+	if (!maps)
+		return;
+
+	maps->nr_lib_maps = tep_read_number(handle->pevent, buf, 4);
+	maps->lib_maps = calloc(maps->nr_lib_maps, sizeof(struct lib_mem_map));
+	if (!maps->lib_maps) {
+		free(maps);
+		return;
+	}
+	maps->next = handle->pid_mmaps;
+	handle->pid_mmaps = maps;
+
+	ret = sscanf(buf+4, "%d %s", &maps->pid, mapname);
+	if (ret == 2)
+		maps->proc_name = strdup(mapname);
+	line = strchr(buf+4, '\n');
+	for (i = 0; i < maps->nr_lib_maps; i++) {
+		if (!line)
+			break;
+		ret = sscanf(line+1, "%llx %llx %s", &maps->lib_maps[i].start,
+			     &maps->lib_maps[i].end, mapname);
+		if (ret == 3)
+			maps->lib_maps[i].lib_name = strdup(mapname);
+		line = strchr(line+1, '\n');
+	}
+}
+
+/**
+ * tracecmd_search_tracee_mmap - Search tracee memory address map
+ * @handle: input handle to the trace.dat file
+ * @pid: pid of the tracee
+ * @addr: address from the tracee memory space.
+ *
+ * Map of the tracee memory can be saved in the trace.dat file, using the option
+ * "--mmap". If there is such information, this API can be used to look up into
+ * this memory map to find library is loaded at the given @addr.
+ *
+ * The name of the library at given tracee @addr is returned.
+ */
+char *tracecmd_search_tracee_mmap(struct tracecmd_input *handle,
+				  int pid, unsigned long long addr)
+{
+	struct pid_mem_maps *maps;
+	int i;
+
+	if (!handle || !handle->pid_mmaps)
+		return NULL;
+
+	maps = handle->pid_mmaps;
+	while (maps) {
+		if (maps->pid == pid)
+			break;
+		maps = maps->next;
+	}
+	if (!maps || !maps->nr_lib_maps || !maps->lib_maps)
+		return NULL;
+
+	for (i = 0; i < maps->nr_lib_maps; i++)
+		if (maps->lib_maps[i].start <= addr &&
+		    maps->lib_maps[i].end > addr)
+			break;
+
+	if (i < maps->nr_lib_maps)
+		return maps->lib_maps[i].lib_name;
+
+	return NULL;
+}
+
 static int handle_options(struct tracecmd_input *handle)
 {
 	unsigned long long offset;
@@ -2229,6 +2307,9 @@ static int handle_options(struct tracecmd_input *handle)
 			cpus = *(int *)buf;
 			handle->cpus = tep_read_number(handle->pevent, &cpus, 4);
 			break;
+		case TRACECMD_OPTION_PIDMMAPS:
+			trace_pid_mmap_load(handle, buf);
+			break;
 		default:
 			warning("unknown option %d", option);
 			break;
diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
index a1a06e9..5c623ec 100644
--- a/tracecmd/include/trace-local.h
+++ b/tracecmd/include/trace-local.h
@@ -157,6 +157,20 @@ struct func_list {
 	const char *mod;
 };
 
+struct lib_mem_map {
+	unsigned long long start;
+	unsigned long long end;
+	char		*lib_name;
+};
+
+struct pid_mem_maps {
+	struct pid_mem_maps	*next;
+	struct lib_mem_map	*lib_maps;
+	unsigned int		nr_lib_maps;
+	char			*proc_name;
+	int			pid;
+};
+
 struct buffer_instance {
 	struct buffer_instance	*next;
 	const char		*name;
@@ -183,6 +197,8 @@ struct buffer_instance {
 	struct tracecmd_msg_handle *msg_handle;
 	struct tracecmd_output *network_handle;
 
+	struct pid_mem_maps	*mem_maps;
+
 	int			flags;
 	int			tracing_on_init_val;
 	int			tracing_on_fd;
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 2f5fbd9..2147554 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -83,6 +83,7 @@ static int max_kb;
 static bool use_tcp;
 
 static int do_ptrace;
+static int get_mmap;
 
 static int filter_task;
 static int filter_pid = -1;
@@ -1060,6 +1061,82 @@ static char *make_pid_filter(char *curr_filter, const char *field)
 	return filter;
 }
 
+static void get_pid_mmaps(int pid)
+{
+	char buf[PATH_MAX+100], perm[5], dev[6], mapname[PATH_MAX+1];
+	struct buffer_instance *instance = &top_instance;
+	struct pid_mem_maps *maps = instance->mem_maps;
+	unsigned long long begin, end, inode, foo;
+	struct lib_mem_map *map;
+	char fname[PATH_MAX+1];
+	FILE *f;
+	int ret;
+	int i;
+
+	while (maps) {
+		if (pid == maps->pid)
+			break;
+		maps = maps->next;
+	}
+
+	sprintf(fname, "/proc/%d/exe", pid);
+	ret = readlink(fname, mapname, PATH_MAX);
+	if (ret >= PATH_MAX || ret < 0)
+		return;
+
+	sprintf(fname, "/proc/%d/maps", pid);
+	f = fopen(fname, "r");
+	if (!f)
+		return;
+
+	if (!maps) {
+		maps = calloc(1, sizeof(*maps));
+		if (!maps)
+			return;
+		maps->pid = pid;
+		maps->next = instance->mem_maps;
+		instance->mem_maps = maps;
+	} else {
+		for (i = 0; i < maps->nr_lib_maps; i++)
+			free(maps->lib_maps[i].lib_name);
+		free(maps->lib_maps);
+		maps->lib_maps = NULL;
+		maps->nr_lib_maps = 0;
+		free(maps->proc_name);
+	}
+
+	maps->proc_name = strdup(mapname);
+
+	while (fgets(buf, sizeof(buf), f)) {
+		mapname[0] = '\0';
+		ret = sscanf(buf, "%llx-%llx %4s %llx %5s %lld %s",
+			     &begin, &end, perm, &foo, dev, &inode, mapname);
+		if (ret == 7 && mapname[0] != '\0') {
+			map = realloc(maps->lib_maps,
+				      (maps->nr_lib_maps+1)*sizeof(*map));
+			if (!map)
+				break;
+			map[maps->nr_lib_maps].end = end;
+			map[maps->nr_lib_maps].start = begin;
+			map[maps->nr_lib_maps].lib_name = strdup(mapname);
+			maps->lib_maps = map;
+			maps->nr_lib_maps++;
+		}
+	}
+	fclose(f);
+}
+
+static void get_filter_pid_mmaps(void)
+{
+	struct filter_pids *p;
+
+	for (p = filter_pids; p; p = p->next) {
+		if (p->exclude)
+			continue;
+		get_pid_mmaps(p->pid);
+	}
+}
+
 static void update_task_filter(void)
 {
 	struct buffer_instance *instance;
@@ -1068,6 +1145,9 @@ static void update_task_filter(void)
 	if (no_filter)
 		return;
 
+	if (get_mmap && filter_pids)
+		get_filter_pid_mmaps();
+
 	if (filter_task)
 		add_filter_pid(pid, 0);
 
@@ -1262,6 +1342,8 @@ static void ptrace_wait(enum trace_type type, int main_pid)
 				break;
 
 			case PTRACE_EVENT_EXIT:
+				if (get_mmap)
+					get_pid_mmaps(main_pid);
 				ptrace(PTRACE_GETEVENTMSG, pid, NULL, &cstatus);
 				ptrace(PTRACE_DETACH, pid, NULL, NULL);
 				break;
@@ -3092,6 +3174,35 @@ static void append_buffer(struct tracecmd_output *handle,
 	}
 }
 
+static void
+add_pid_mem_maps(struct tracecmd_output *handle, struct buffer_instance *instance)
+{
+	struct pid_mem_maps *maps = instance->mem_maps;
+	struct iovec vector[2];
+	struct trace_seq s;
+	int i;
+
+	trace_seq_init(&s);
+	while (maps) {
+		if (!maps->nr_lib_maps)
+			continue;
+		trace_seq_reset(&s);
+		trace_seq_printf(&s, "%d %s\n", maps->pid, maps->proc_name);
+		for (i = 0; i < maps->nr_lib_maps; i++)
+			trace_seq_printf(&s, "%llx %llx %s\n",
+					maps->lib_maps[i].start,
+					maps->lib_maps[i].end,
+					maps->lib_maps[i].lib_name);
+		vector[0].iov_len = 4;
+		vector[0].iov_base = &maps->nr_lib_maps;
+		vector[1].iov_len = s.len+1;
+		vector[1].iov_base = s.buffer;
+		tracecmd_add_option_v(handle, TRACECMD_OPTION_PIDMMAPS, vector, 2);
+		maps = maps->next;
+	}
+	trace_seq_destroy(&s);
+}
+
 static void
 add_buffer_stat(struct tracecmd_output *handle, struct buffer_instance *instance)
 {
@@ -3272,6 +3383,10 @@ static void record_data(struct common_record_context *ctx)
 		if (!no_top_instance() && !top_instance.msg_handle)
 			print_stat(&top_instance);
 
+		for_all_instances(instance) {
+			add_pid_mem_maps(handle, instance);
+		}
+
 		tracecmd_append_cpu_data(handle, local_cpu_count, temp_files);
 
 		for (i = 0; i < max_cpu_count; i++)
@@ -4382,6 +4497,7 @@ void update_first_instance(struct buffer_instance *instance, int topt)
 }
 
 enum {
+	OPT_mmap		= 244,
 	OPT_quiet		= 245,
 	OPT_debug		= 246,
 	OPT_no_filter		= 247,
@@ -4612,6 +4728,7 @@ static void parse_record_options(int argc,
 			{"debug", no_argument, NULL, OPT_debug},
 			{"quiet", no_argument, NULL, OPT_quiet},
 			{"help", no_argument, NULL, '?'},
+			{"mmap", no_argument, NULL, OPT_mmap},
 			{"module", required_argument, NULL, OPT_module},
 			{NULL, 0, NULL, 0}
 		};
@@ -4843,6 +4960,9 @@ static void parse_record_options(int argc,
 		case 'i':
 			ignore_event_not_found = 1;
 			break;
+		case OPT_mmap:
+			get_mmap = 1;
+			break;
 		case OPT_date:
 			ctx->date = 1;
 			if (ctx->data_flags & DATA_FL_OFFSET)
@@ -4909,6 +5029,9 @@ static void parse_record_options(int argc,
 		add_func(&ctx->instance->filter_funcs,
 			 ctx->instance->filter_mod, "*");
 
+	if (filter_task && get_mmap)
+		do_ptrace = 1;
+
 	if (do_ptrace && !filter_task && (filter_pid < 0))
 		die(" -c can only be used with -F (or -P with event-fork support)");
 	if (ctx->do_child && !filter_task &&! filter_pid)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v()
  2019-06-17 11:52 ` [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v() tz.stoyanov
@ 2019-06-17 13:06   ` Steven Rostedt
  2019-06-17 13:18     ` Ceco
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2019-06-17 13:06 UTC (permalink / raw)
  To: tz.stoyanov; +Cc: y.karadz, linux-trace-devel

On Mon, 17 Jun 2019 14:52:17 +0300
tz.stoyanov@gmail.com wrote:

> From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
> 
> From: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> 
> This patch implements a new tracecmd API, tracecmd_add_option_v()
> It adds new option in trace.dat, similar to tracecmd_add_option(),
> but the option's data is passed as list of buffers. The standard
> struct iovec is used as input parameter, containing the option's
> data buffers.
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
>  include/trace-cmd/trace-cmd.h    |   5 ++
>  include/traceevent/event-parse.h |   1 +
>  tracecmd/trace-output.c          | 117 ++++++++++++++++++++++++++-----
>  3 files changed, 106 insertions(+), 17 deletions(-)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index ceb03f4..3919673 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -245,11 +245,16 @@ struct tracecmd_output *tracecmd_create_init_file_override(const char *output_fi
>  struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
>  					    unsigned short id, int size,
>  					    const void *data);
> +struct tracecmd_option *
> +tracecmd_add_option_v(struct tracecmd_output *handle,
> +		    unsigned short id, const struct iovec *vector, int count);
> +
>  struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle,
>  						   const char *name, int cpus);
>  
>  int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus);
>  int tracecmd_write_options(struct tracecmd_output *handle);
> +int tracecmd_append_options(struct tracecmd_output *handle);

The change log doesn't mention anything about tracecmd_append_options()?

>  int tracecmd_update_option(struct tracecmd_output *handle,
>  			   struct tracecmd_option *option, int size,
>  			   const void *data);
> diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h
> index 5e0fd19..62057b3 100644
> --- a/include/traceevent/event-parse.h
> +++ b/include/traceevent/event-parse.h
> @@ -11,6 +11,7 @@
>  #include <stdio.h>
>  #include <regex.h>
>  #include <string.h>
> +#include <sys/uio.h>
>  
>  #include "trace-seq.h"
>  
> diff --git a/tracecmd/trace-output.c b/tracecmd/trace-output.c
> index 33d6ce3..f7a2791 100644
> --- a/tracecmd/trace-output.c
> +++ b/tracecmd/trace-output.c
> @@ -883,21 +883,23 @@ static struct tracecmd_output *create_file(const char *output_file,
>  }
>  
>  /**
> - * tracecmd_add_option - add options to the file
> + * tracecmd_add_option_v - add options to the file
>   * @handle: the output file handle name
>   * @id: the id of the option
> - * @size: the size of the option data
> - * @data: the data to write to the file.
> + * @vector: array of vectors, pointing to the data to write in the file
> + * @count: number of items in the vector array
>   *
>   * Returns handle to update option if needed.
>   *  Just the content can be updated, with smaller or equal to
>   *  content than the specified size.
>   */
>  struct tracecmd_option *
> -tracecmd_add_option(struct tracecmd_output *handle,
> -		    unsigned short id, int size, const void *data)
> +tracecmd_add_option_v(struct tracecmd_output *handle,
> +		    unsigned short id, const struct iovec *vector, int count)

Hmm, I think this is a bit overkill. I don't really see anything using
more than one or two data vectors. All I see would be at most a "count"
followed by a list of data, which is what I think you are using this
for.

I rather wait to implement something like this when there's more of a
need for it. I don't believe this change really requires it.

-- Steve


>  {
>  	struct tracecmd_option *option;
> +	char *data = NULL;
> +	int i, size = 0;
>  
>  	/*
>  	 * We can only add options before they were written.
> @@ -906,32 +908,63 @@ tracecmd_add_option(struct tracecmd_output *handle,
>  	if (handle->options_written)
>  		return NULL;
>  
> -	handle->nr_options++;
> +	for (i = 0; i < count; i++)
> +		size += vector[i].iov_len;
> +
> +	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass vector with 0 / NULL data */
> +	if (size) {
> +		data = malloc(size);
> +		if (!data) {
> +			warning("Insufficient memory");
> +			return NULL;
> +		}
> +	}
>  
>  	option = malloc(sizeof(*option));
>  	if (!option) {
>  		warning("Could not allocate space for option");
> +		free(data);
>  		return NULL;
>  	}
>  
> -	option->id = id;
> -	option->size = size;
> -	option->data = malloc(size);
> -	if (!option->data) {
> -		warning("Insufficient memory");
> -		free(option);
> -		return NULL;
> +	handle->nr_options++;
> +	option->data = data;
> +	for (i = 0; i < count; i++) {
> +		if (vector[i].iov_base && vector[i].iov_len) {
> +			memcpy(data, vector[i].iov_base, vector[i].iov_len);
> +			data += vector[i].iov_len;
> +		}
>  	}
> -
> -	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass 0 / NULL data */
> -	if (size)
> -		memcpy(option->data, data, size);
> +	option->size = size;
> +	option->id = id;
>  
>  	list_add_tail(&option->list, &handle->options);
>  
>  	return option;
>  }
>  
> +/**
> + * tracecmd_add_option - add options to the file
> + * @handle: the output file handle name
> + * @id: the id of the option
> + * @size: the size of the option data
> + * @data: the data to write to the file.
> + *
> + * Returns handle to update option if needed.
> + *  Just the content can be updated, with smaller or equal to
> + *  content than the specified size.
> + */
> +struct tracecmd_option *
> +tracecmd_add_option(struct tracecmd_output *handle,
> +		    unsigned short id, int size, const void *data)
> +{
> +	struct iovec vect;
> +
> +	vect.iov_base = (void *) data;
> +	vect.iov_len = size;
> +	return tracecmd_add_option_v(handle, id, &vect, 1);
> +}
> +
>  int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus)
>  {
>  	cpus = convert_endian_4(handle, cpus);
> @@ -979,6 +1012,56 @@ int tracecmd_write_options(struct tracecmd_output *handle)
>  	return 0;
>  }
>  
> +int tracecmd_append_options(struct tracecmd_output *handle)
> +{
> +	struct tracecmd_option *options;
> +	unsigned short option;
> +	unsigned short endian2;
> +	unsigned int endian4;
> +	off_t offset;
> +	int r;
> +
> +	/* If already written, ignore */
> +	if (handle->options_written)
> +		return 0;
> +
> +	if (lseek64(handle->fd, 0, SEEK_END) == (off_t)-1)
> +		return -1;
> +	offset = lseek64(handle->fd, -2, SEEK_CUR);
> +	if (offset == (off_t)-1)
> +		return -1;
> +
> +	r = pread(handle->fd, &option, 2, offset);
> +	if (r != 2 || option != TRACECMD_OPTION_DONE)
> +		return -1;
> +
> +	list_for_each_entry(options, &handle->options, list) {
> +		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 in case it needs to be updated */
> +		options->offset = lseek64(handle->fd, 0, SEEK_CUR);
> +
> +		if (do_write_check(handle, options->data,
> +				   options->size))
> +			return -1;
> +	}
> +
> +	option = TRACECMD_OPTION_DONE;
> +
> +	if (do_write_check(handle, &option, 2))
> +		return -1;
> +
> +	handle->options_written = 1;
> +
> +	return 0;
> +}
> +
>  int tracecmd_update_option(struct tracecmd_output *handle,
>  			   struct tracecmd_option *option, int size,
>  			   const void *data)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v()
  2019-06-17 13:06   ` Steven Rostedt
@ 2019-06-17 13:18     ` Ceco
  2019-06-17 13:39       ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Ceco @ 2019-06-17 13:18 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Yordan Karadzhov, linux-trace-devel

Hi Steven

On Mon, Jun 17, 2019 at 4:06 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
...
>
> Hmm, I think this is a bit overkill. I don't really see anything using
> more than one or two data vectors. All I see would be at most a "count"
> followed by a list of data, which is what I think you are using this
> for.
>
> I rather wait to implement something like this when there's more of a
> need for it. I don't believe this change really requires it.
>
> -- Steve
>

Actually, this patch is from the patch set (N 7):
 "trace-cmd: Timetamps sync between host and guest machines, relying
on vsock events. "
and I took it as-is. In timesync changes tracecmd_add_option_v() is
used in similar way.
That explains the tracecmd_append_options() API, which is used there.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file.
  2019-06-17 11:52 ` [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file tz.stoyanov
@ 2019-06-17 13:37   ` Steven Rostedt
  2019-06-17 14:12     ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2019-06-17 13:37 UTC (permalink / raw)
  To: tz.stoyanov; +Cc: y.karadz, linux-trace-devel

On Mon, 17 Jun 2019 14:52:18 +0300
tz.stoyanov@gmail.com wrote:

> From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
> 
> A new trace-cmd record option is added: "--mmap". When it is set with
> combination of -F or -P options, the memory map of the traced applications
> is stored in the trace.dat file. A new API tracecmd_search_tracee_mmap()
> can be used to look up into stored memory maps. The map is retrieved from
> /proc/<pid>/maps file.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/trace-cmd/trace-cmd.h  |   4 ++
>  lib/trace-cmd/trace-input.c    |  81 ++++++++++++++++++++++
>  tracecmd/include/trace-local.h |  16 +++++
>  tracecmd/trace-record.c        | 123 +++++++++++++++++++++++++++++++++
>  4 files changed, 224 insertions(+)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index 3919673..868e571 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -81,6 +81,7 @@ enum {
>  	TRACECMD_OPTION_HOOK,
>  	TRACECMD_OPTION_OFFSET,
>  	TRACECMD_OPTION_CPUCOUNT,
> +	TRACECMD_OPTION_PIDMMAPS,
>  };
>  
>  enum {
> @@ -206,6 +207,9 @@ unsigned long long tracecmd_page_ts(struct tracecmd_input *handle,
>  unsigned int tracecmd_record_ts_delta(struct tracecmd_input *handle,
>  				      struct tep_record *record);
>  
> +char *tracecmd_get_tracee_lib(struct tracecmd_input *handle,
> +			      int pid, unsigned long addr);
> +
>  #ifndef SWIG
>  /* hack for function graph work around */
>  extern __thread struct tracecmd_input *tracecmd_curr_thread_handle;
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 264e3c3..aaf3c56 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -100,6 +100,7 @@ struct tracecmd_input {
>  	struct tracecmd_ftrace	finfo;
>  
>  	struct hook_list	*hooks;
> +	struct pid_mem_maps	*pid_mmaps;
>  	/* file information */
>  	size_t			header_files_start;
>  	size_t			ftrace_files_start;
> @@ -2133,6 +2134,83 @@ void tracecmd_set_ts2secs(struct tracecmd_input *handle,
>  	handle->use_trace_clock = false;
>  }
>  
> +static void trace_pid_mmap_load(struct tracecmd_input *handle, char *buf)

Probably should have a return value for this function.

> +{
> +	struct pid_mem_maps *maps;
> +	char mapname[PATH_MAX+1];
> +	char *line;
> +	int ret;
> +	int i;
> +
> +	maps = calloc(1, sizeof(*maps));
> +	if (!maps)

We can return -ENOMEM

> +		return;
> +
> +	maps->nr_lib_maps = tep_read_number(handle->pevent, buf, 4);
> +	maps->lib_maps = calloc(maps->nr_lib_maps, sizeof(struct lib_mem_map));
> +	if (!maps->lib_maps) {
> +		free(maps);
> +		return;
> +	}
> +	maps->next = handle->pid_mmaps;
> +	handle->pid_mmaps = maps;

Probably want to hold off to adding maps till the end. That way we can
check for more errors below.

> +
> +	ret = sscanf(buf+4, "%d %s", &maps->pid, mapname);

Hmm, this is very dangerous. Using a %s here, where we don't know the
length of that string can cause mapname to overflow. This is run as
root, and I don't want someone to send a trace.dat file that can cause
a security issue when read.

What you can do is something like this:

	char tempbuf[PATH_MAX+1];

	tempbuf[PATH_MAX] = '\0';
	strncpy(tempbuf, buf+4, PATH_MAX);

	ret = sscanf(tempbuf, "%d %s", &maps->pid, mapname);

Which would make sure that buf wont overrun mapname.

> +	if (ret == 2)
> +		maps->proc_name = strdup(mapname);

Note, strdup() can fail. We need to check for that.

> +	line = strchr(buf+4, '\n');
> +	for (i = 0; i < maps->nr_lib_maps; i++) {
> +		if (!line)
> +			break;
> +		ret = sscanf(line+1, "%llx %llx %s", &maps->lib_maps[i].start,
> +			     &maps->lib_maps[i].end, mapname);

We should make sure that line length is not greater than PATH_MAX and
warn on it if it is. Otherwise it can overflow mapname.


> +		if (ret == 3)
> +			maps->lib_maps[i].lib_name = strdup(mapname);

Need to check the return of mapname.

> +		line = strchr(line+1, '\n');
> +	}
> +}
> +
> +/**
> + * tracecmd_search_tracee_mmap - Search tracee memory address map
> + * @handle: input handle to the trace.dat file
> + * @pid: pid of the tracee
> + * @addr: address from the tracee memory space.
> + *
> + * Map of the tracee memory can be saved in the trace.dat file, using the option
> + * "--mmap". If there is such information, this API can be used to look up into
> + * this memory map to find library is loaded at the given @addr.
> + *
> + * The name of the library at given tracee @addr is returned.
> + */
> +char *tracecmd_search_tracee_mmap(struct tracecmd_input *handle,
> +				  int pid, unsigned long long addr)
> +{
> +	struct pid_mem_maps *maps;
> +	int i;
> +
> +	if (!handle || !handle->pid_mmaps)
> +		return NULL;
> +
> +	maps = handle->pid_mmaps;
> +	while (maps) {

I wonder if we should sort the maps after they are loaded, and then we
can do a binary search. Which would be much faster.

> +		if (maps->pid == pid)
> +			break;
> +		maps = maps->next;
> +	}
> +	if (!maps || !maps->nr_lib_maps || !maps->lib_maps)
> +		return NULL;
> +
> +	for (i = 0; i < maps->nr_lib_maps; i++)
> +		if (maps->lib_maps[i].start <= addr &&
> +		    maps->lib_maps[i].end > addr)

Probably could make this into a binary search too.

 sort() and bsearch() are your friends ;-)

> +			break;
> +
> +	if (i < maps->nr_lib_maps)
> +		return maps->lib_maps[i].lib_name;
> +
> +	return NULL;
> +}
> +
>  static int handle_options(struct tracecmd_input *handle)
>  {
>  	unsigned long long offset;
> @@ -2229,6 +2307,9 @@ static int handle_options(struct tracecmd_input *handle)
>  			cpus = *(int *)buf;
>  			handle->cpus = tep_read_number(handle->pevent, &cpus, 4);
>  			break;
> +		case TRACECMD_OPTION_PIDMMAPS:
> +			trace_pid_mmap_load(handle, buf);
> +			break;
>  		default:
>  			warning("unknown option %d", option);
>  			break;
> diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
> index a1a06e9..5c623ec 100644
> --- a/tracecmd/include/trace-local.h
> +++ b/tracecmd/include/trace-local.h
> @@ -157,6 +157,20 @@ struct func_list {
>  	const char *mod;
>  };
>  
> +struct lib_mem_map {
> +	unsigned long long start;
> +	unsigned long long end;
> +	char		*lib_name;

To make this easier to read, lets add another tab between the type and
name. Although, I haven't looked at this applied, so it may not be as
bad as the patch looks.

> +};
> +
> +struct pid_mem_maps {
> +	struct pid_mem_maps	*next;
> +	struct lib_mem_map	*lib_maps;
> +	unsigned int		nr_lib_maps;
> +	char			*proc_name;
> +	int			pid;
> +};
> +
>  struct buffer_instance {
>  	struct buffer_instance	*next;
>  	const char		*name;
> @@ -183,6 +197,8 @@ struct buffer_instance {
>  	struct tracecmd_msg_handle *msg_handle;
>  	struct tracecmd_output *network_handle;
>  
> +	struct pid_mem_maps	*mem_maps;
> +
>  	int			flags;
>  	int			tracing_on_init_val;
>  	int			tracing_on_fd;
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 2f5fbd9..2147554 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -83,6 +83,7 @@ static int max_kb;
>  static bool use_tcp;
>  
>  static int do_ptrace;
> +static int get_mmap;
>  
>  static int filter_task;
>  static int filter_pid = -1;
> @@ -1060,6 +1061,82 @@ static char *make_pid_filter(char *curr_filter, const char *field)
>  	return filter;
>  }
>  
> +static void get_pid_mmaps(int pid)

Although it's not tested, perhaps we should return an error if we come
across one.

> +{
> +	char buf[PATH_MAX+100], perm[5], dev[6], mapname[PATH_MAX+1];

BTW, it's better not to put so many types on one line. It makes warning
messages like "variable not used" easier to find. I personally prefer
not to have any multiple declarations on a single line except for
counters: int i, j, k. All else really should be separate.

I know this is more of a personal preference, but I do think it makes
the code easier to read.

> +	struct buffer_instance *instance = &top_instance;
> +	struct pid_mem_maps *maps = instance->mem_maps;
> +	unsigned long long begin, end, inode, foo;

This isn't so bad to be on the same line, as they are related.

You could argue to say the char's above are related, but because they
all have their own defined sizes, I find it to be a bit more messy.

> +	struct lib_mem_map *map;
> +	char fname[PATH_MAX+1];
> +	FILE *f;
> +	int ret;
> +	int i;
> +
> +	while (maps) {
> +		if (pid == maps->pid)
> +			break;
> +		maps = maps->next;
> +	}
> +
> +	sprintf(fname, "/proc/%d/exe", pid);
> +	ret = readlink(fname, mapname, PATH_MAX);
> +	if (ret >= PATH_MAX || ret < 0)
> +		return;

Probably should return error types, or at least do some kind of warning.

> +
> +	sprintf(fname, "/proc/%d/maps", pid);
> +	f = fopen(fname, "r");
> +	if (!f)
> +		return;
> +
> +	if (!maps) {
> +		maps = calloc(1, sizeof(*maps));
> +		if (!maps)
> +			return;
> +		maps->pid = pid;
> +		maps->next = instance->mem_maps;
> +		instance->mem_maps = maps;
> +	} else {
> +		for (i = 0; i < maps->nr_lib_maps; i++)
> +			free(maps->lib_maps[i].lib_name);
> +		free(maps->lib_maps);
> +		maps->lib_maps = NULL;
> +		maps->nr_lib_maps = 0;
> +		free(maps->proc_name);
> +	}
> +
> +	maps->proc_name = strdup(mapname);

Check for strdup() return.

> +
> +	while (fgets(buf, sizeof(buf), f)) {
> +		mapname[0] = '\0';
> +		ret = sscanf(buf, "%llx-%llx %4s %llx %5s %lld %s",
> +			     &begin, &end, perm, &foo, dev, &inode, mapname);

Probably should test the size of buf to make sure mapname isn't
overflow.


> +		if (ret == 7 && mapname[0] != '\0') {
> +			map = realloc(maps->lib_maps,
> +				      (maps->nr_lib_maps+1)*sizeof(*map));
> +			if (!map)
> +				break;
> +			map[maps->nr_lib_maps].end = end;
> +			map[maps->nr_lib_maps].start = begin;
> +			map[maps->nr_lib_maps].lib_name = strdup(mapname);
> +			maps->lib_maps = map;
> +			maps->nr_lib_maps++;
> +		}
> +	}
> +	fclose(f);
> +}
> +
> +static void get_filter_pid_mmaps(void)
> +{
> +	struct filter_pids *p;
> +
> +	for (p = filter_pids; p; p = p->next) {
> +		if (p->exclude)
> +			continue;
> +		get_pid_mmaps(p->pid);
> +	}
> +}
> +
>  static void update_task_filter(void)
>  {
>  	struct buffer_instance *instance;
> @@ -1068,6 +1145,9 @@ static void update_task_filter(void)
>  	if (no_filter)
>  		return;
>  
> +	if (get_mmap && filter_pids)
> +		get_filter_pid_mmaps();
> +
>  	if (filter_task)
>  		add_filter_pid(pid, 0);
>  
> @@ -1262,6 +1342,8 @@ static void ptrace_wait(enum trace_type type, int main_pid)
>  				break;
>  
>  			case PTRACE_EVENT_EXIT:
> +				if (get_mmap)
> +					get_pid_mmaps(main_pid);

I'll have to look at this a bit more. I want to make sure that we are
not doing the full ptrace, and just tracing exit.

>  				ptrace(PTRACE_GETEVENTMSG, pid, NULL, &cstatus);
>  				ptrace(PTRACE_DETACH, pid, NULL, NULL);
>  				break;
> @@ -3092,6 +3174,35 @@ static void append_buffer(struct tracecmd_output *handle,
>  	}
>  }
>  
> +static void
> +add_pid_mem_maps(struct tracecmd_output *handle, struct buffer_instance *instance)
> +{
> +	struct pid_mem_maps *maps = instance->mem_maps;
> +	struct iovec vector[2];
> +	struct trace_seq s;
> +	int i;
> +
> +	trace_seq_init(&s);
> +	while (maps) {
> +		if (!maps->nr_lib_maps)
> +			continue;
> +		trace_seq_reset(&s);
> +		trace_seq_printf(&s, "%d %s\n", maps->pid, maps->proc_name);
> +		for (i = 0; i < maps->nr_lib_maps; i++)
> +			trace_seq_printf(&s, "%llx %llx %s\n",
> +					maps->lib_maps[i].start,
> +					maps->lib_maps[i].end,
> +					maps->lib_maps[i].lib_name);
> +		vector[0].iov_len = 4;
> +		vector[0].iov_base = &maps->nr_lib_maps;
> +		vector[1].iov_len = s.len+1;
> +		vector[1].iov_base = s.buffer;
> +		tracecmd_add_option_v(handle, TRACECMD_OPTION_PIDMMAPS, vector, 2);

As stated in the last email. The vector size is hard coded as 2, which
is why I'm reluctant to add a vector version now. If it was dynamic,
then that would be more of a reason. Here we can just create our own
structure and use that:

	struct tracecmd_map_file {
		int		nr_maps;
		int		maps_len;
		char		*maps;
	}

Or something like that.

-- Steve

> +		maps = maps->next;
> +	}
> +	trace_seq_destroy(&s);
> +}
> +
>  static void
>  add_buffer_stat(struct tracecmd_output *handle, struct buffer_instance *instance)
>  {
> @@ -3272,6 +3383,10 @@ static void record_data(struct common_record_context *ctx)
>  		if (!no_top_instance() && !top_instance.msg_handle)
>  			print_stat(&top_instance);
>  
> +		for_all_instances(instance) {
> +			add_pid_mem_maps(handle, instance);
> +		}
> +
>  		tracecmd_append_cpu_data(handle, local_cpu_count, temp_files);
>  
>  		for (i = 0; i < max_cpu_count; i++)
> @@ -4382,6 +4497,7 @@ void update_first_instance(struct buffer_instance *instance, int topt)
>  }
>  
>  enum {
> +	OPT_mmap		= 244,
>  	OPT_quiet		= 245,
>  	OPT_debug		= 246,
>  	OPT_no_filter		= 247,
> @@ -4612,6 +4728,7 @@ static void parse_record_options(int argc,
>  			{"debug", no_argument, NULL, OPT_debug},
>  			{"quiet", no_argument, NULL, OPT_quiet},
>  			{"help", no_argument, NULL, '?'},
> +			{"mmap", no_argument, NULL, OPT_mmap},
>  			{"module", required_argument, NULL, OPT_module},
>  			{NULL, 0, NULL, 0}
>  		};
> @@ -4843,6 +4960,9 @@ static void parse_record_options(int argc,
>  		case 'i':
>  			ignore_event_not_found = 1;
>  			break;
> +		case OPT_mmap:
> +			get_mmap = 1;
> +			break;
>  		case OPT_date:
>  			ctx->date = 1;
>  			if (ctx->data_flags & DATA_FL_OFFSET)
> @@ -4909,6 +5029,9 @@ static void parse_record_options(int argc,
>  		add_func(&ctx->instance->filter_funcs,
>  			 ctx->instance->filter_mod, "*");
>  
> +	if (filter_task && get_mmap)
> +		do_ptrace = 1;
> +
>  	if (do_ptrace && !filter_task && (filter_pid < 0))
>  		die(" -c can only be used with -F (or -P with event-fork support)");
>  	if (ctx->do_child && !filter_task &&! filter_pid)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v()
  2019-06-17 13:18     ` Ceco
@ 2019-06-17 13:39       ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2019-06-17 13:39 UTC (permalink / raw)
  To: Ceco; +Cc: Yordan Karadzhov, linux-trace-devel

On Mon, 17 Jun 2019 16:18:38 +0300
Ceco <tz.stoyanov@gmail.com> wrote:

> Hi Steven
> 
> On Mon, Jun 17, 2019 at 4:06 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >  
> ...
> >
> > Hmm, I think this is a bit overkill. I don't really see anything using
> > more than one or two data vectors. All I see would be at most a "count"
> > followed by a list of data, which is what I think you are using this
> > for.
> >
> > I rather wait to implement something like this when there's more of a
> > need for it. I don't believe this change really requires it.
> >
> > -- Steve
> >  
> 
> Actually, this patch is from the patch set (N 7):
>  "trace-cmd: Timetamps sync between host and guest machines, relying
> on vsock events. "
> and I took it as-is. In timesync changes tracecmd_add_option_v() is
> used in similar way.
> That explains the tracecmd_append_options() API, which is used there.
> 
> 

Ah, that needed to be stated. Yeah, when taking a patch from another
series, make sure that the change log is updated too. Never expect that
the reviewer will know about any other series ;-)

-- Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file.
  2019-06-17 13:37   ` Steven Rostedt
@ 2019-06-17 14:12     ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2019-06-17 14:12 UTC (permalink / raw)
  To: tz.stoyanov; +Cc: y.karadz, linux-trace-devel

On Mon, 17 Jun 2019 09:37:22 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> As stated in the last email. The vector size is hard coded as 2, which
> is why I'm reluctant to add a vector version now. If it was dynamic,
> then that would be more of a reason. Here we can just create our own
> structure and use that:
> 
> 	struct tracecmd_map_file {
> 		int		nr_maps;
> 		int		maps_len;
> 		char		*maps;
> 	}
> 
> Or something like that.

As you can't send that to the tracecmd_add_option, you would need
something more like this:

	struct tracecmd_map_file {
		int		nr_maps;
		int		maps_len;
		char		maps[0];
	};

And then you would need to copy it first before sending it:


	struct tracecmd_map_file *tmap;

	tmap = malloc(sizeof(*tmap) + s.len + 1);
	
	and copy the buffer:

	memcpy(&tmap->maps[0], s.buffer, s.len + 1);

before sending it to tracecmd_add_option();

-- Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-06-17 14:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 11:52 [PATCH 0/2] Save tracee memory map into trace.dat file tz.stoyanov
2019-06-17 11:52 ` [PATCH 1/2] trace-cmd: Implemented new API tracecmd_add_option_v() tz.stoyanov
2019-06-17 13:06   ` Steven Rostedt
2019-06-17 13:18     ` Ceco
2019-06-17 13:39       ` Steven Rostedt
2019-06-17 11:52 ` [PATCH 2/2] trace-cmd: Save the tracee memory map into the trace.dat file tz.stoyanov
2019-06-17 13:37   ` Steven Rostedt
2019-06-17 14:12     ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).