[PATCHSET 0/9] perf util: Cleanup die() and its friends

[PATCHSET 0/9] perf util: Cleanup die() and its friends

[Namhyung Kim]

Hi,

I've done this painful cleanup of *die() removal. ;-) It now can
return error code rather than calling die() when something bad happen.
It only touches tracepoint related code and I tested it's working
normally but lacks thorough testing in various environment.

Please help testing more!

Thanks,
Namhyung


Namhyung Kim (9):
  perf util: Let get_tracing_file() can return NULL
  perf util: Get rid of malloc_or_die() in trace-event-info.c
  perf util: Get rid of write_or_die() from trace-event-info.c
  perf util: Get rid of die() calls from trace-event-info.c
  perf util: Handle failure case in trace_report()
  perf util: Get rid of malloc_or_die() in trace-event-read.c
  perf util: Get rid of read_or_die() in trace-event-read.c
  perf util: Get rid of die() calls in trace-data-read.c
  perf util: Cleanup calc_data_size logic

 tools/perf/util/header.c           |  11 +-
 tools/perf/util/trace-event-info.c | 336 +++++++++++++++++++++++++------------
 tools/perf/util/trace-event-read.c | 256 ++++++++++++++++++----------
 tools/perf/util/trace-event.h      |   2 +-
 4 files changed, 403 insertions(+), 202 deletions(-)

-- 
1.7.11.7

[PATCH 1/9] perf util: Let get_tracing_file() can return NULL

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

So that it can be used by other places.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-info.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 5729f434c5b1..f241343b7d48 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -62,7 +62,7 @@ static const char *find_debugfs(void)
 	const char *path = perf_debugfs_mount(NULL);
 
 	if (!path)
-		die("Your kernel not support debugfs filesystem");
+		pr_err("Your kernel not support debugfs filesystem");
 
 	return path;
 }
@@ -81,8 +81,12 @@ static const char *find_tracing_dir(void)
 		return tracing;
 
 	debugfs = find_debugfs();
+	if (!debugfs)
+		return NULL;
 
-	tracing = malloc_or_die(strlen(debugfs) + 9);
+	tracing = malloc(strlen(debugfs) + 9);
+	if (!tracing)
+		return NULL;
 
 	sprintf(tracing, "%s/tracing", debugfs);
 
@@ -99,7 +103,9 @@ static char *get_tracing_file(const char *name)
 	if (!tracing)
 		return NULL;
 
-	file = malloc_or_die(strlen(tracing) + strlen(name) + 2);
+	file = malloc(strlen(tracing) + strlen(name) + 2);
+	if (!file)
+		return NULL;
 
 	sprintf(file, "%s/%s", tracing, name);
 	return file;
@@ -170,6 +176,9 @@ static void read_header_files(void)
 	struct stat st;
 
 	path = get_tracing_file("events/header_page");
+	if (!path)
+		die("can't get tracing/events/header_page");
+
 	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
@@ -178,6 +187,9 @@ static void read_header_files(void)
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
+	if (!path)
+		die("can't get tracing/events/header_event");
+
 	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
@@ -251,6 +263,8 @@ static void read_ftrace_files(struct tracepoint_path *tps)
 	char *path;
 
 	path = get_tracing_file("events/ftrace");
+	if (!path)
+		die("can't get tracing/events/ftrace");
 
 	copy_event_system(path, tps);
 
@@ -279,6 +293,8 @@ static void read_event_files(struct tracepoint_path *tps)
 	int ret;
 
 	path = get_tracing_file("events");
+	if (!path)
+		die("can't get tracing/events");
 
 	dir = opendir(path);
 	if (!dir)
@@ -343,6 +359,9 @@ static void read_ftrace_printk(void)
 	int ret;
 
 	path = get_tracing_file("printk_formats");
+	if (!path)
+		die("can't get tracing/printk_formats");
+
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
-- 
1.7.11.7

[PATCH 2/9] perf util: Get rid of malloc_or_die() in trace-event-info.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Check return value of malloc and fail if NULL.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-info.c | 48 ++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index f241343b7d48..f04c8fd190dc 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -47,16 +47,6 @@ static const char *output_file = "trace.info";
 static int output_fd;
 
 
-static void *malloc_or_die(unsigned int size)
-{
-	void *data;
-
-	data = malloc(size);
-	if (!data)
-		die("malloc");
-	return data;
-}
-
 static const char *find_debugfs(void)
 {
 	const char *path = perf_debugfs_mount(NULL);
@@ -209,7 +199,7 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
 	return false;
 }
 
-static void copy_event_system(const char *sys, struct tracepoint_path *tps)
+static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
 	struct dirent *dent;
 	struct stat st;
@@ -217,6 +207,7 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 	DIR *dir;
 	int count = 0;
 	int ret;
+	int err;
 
 	dir = opendir(sys);
 	if (!dir)
@@ -228,7 +219,11 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 		    strcmp(dent->d_name, "..") == 0 ||
 		    !name_in_tp_list(dent->d_name, tps))
 			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
+		format = malloc(strlen(sys) + strlen(dent->d_name) + 10);
+		if (!format) {
+			err = -ENOMEM;
+			goto out;
+		}
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 		free(format);
@@ -246,16 +241,22 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 		    strcmp(dent->d_name, "..") == 0 ||
 		    !name_in_tp_list(dent->d_name, tps))
 			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
+		format = malloc(strlen(sys) + strlen(dent->d_name) + 10);
+		if (!format) {
+			err = -ENOMEM;
+			goto out;
+		}
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
 		if (ret >= 0)
 			record_file(format, 8);
-
 		free(format);
 	}
+	err = 0;
+out:
 	closedir(dir);
+	return err;
 }
 
 static void read_ftrace_files(struct tracepoint_path *tps)
@@ -282,7 +283,7 @@ static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
 	return false;
 }
 
-static void read_event_files(struct tracepoint_path *tps)
+static int read_event_files(struct tracepoint_path *tps)
 {
 	struct dirent *dent;
 	struct stat st;
@@ -291,6 +292,7 @@ static void read_event_files(struct tracepoint_path *tps)
 	DIR *dir;
 	int count = 0;
 	int ret;
+	int err;
 
 	path = get_tracing_file("events");
 	if (!path)
@@ -320,7 +322,11 @@ static void read_event_files(struct tracepoint_path *tps)
 		    strcmp(dent->d_name, "ftrace") == 0 ||
 		    !system_in_tp_list(dent->d_name, tps))
 			continue;
-		sys = malloc_or_die(strlen(path) + strlen(dent->d_name) + 2);
+		sys = malloc(strlen(path) + strlen(dent->d_name) + 2);
+		if (!sys) {
+			err = -ENOMEM;
+			goto out;
+		}
 		sprintf(sys, "%s/%s", path, dent->d_name);
 		ret = stat(sys, &st);
 		if (ret >= 0) {
@@ -329,9 +335,12 @@ static void read_event_files(struct tracepoint_path *tps)
 		}
 		free(sys);
 	}
-
+	err = 0;
+out:
 	closedir(dir);
 	put_tracing_file(path);
+
+	return err;
 }
 
 static void read_proc_kallsyms(void)
@@ -463,7 +472,10 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
 	if (!tps)
 		return NULL;
 
-	tdata = malloc_or_die(sizeof(*tdata));
+	tdata = malloc(sizeof(*tdata));
+	if (!tdata)
+		return NULL;
+
 	tdata->temp = temp;
 	tdata->size = 0;
 
-- 
1.7.11.7

[PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Check return value of write and fail if error.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-info.c | 169 +++++++++++++++++++++++++------------
 tools/perf/util/trace-event.h      |   2 +-
 2 files changed, 116 insertions(+), 55 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index f04c8fd190dc..05cf94ef57e4 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -106,17 +106,6 @@ static void put_tracing_file(char *file)
 	free(file);
 }
 
-static ssize_t write_or_die(const void *buf, size_t len)
-{
-	int ret;
-
-	ret = write(output_fd, buf, len);
-	if (ret < 0)
-		die("writing to '%s'", output_file);
-
-	return ret;
-}
-
 int bigendian(void)
 {
 	unsigned char str[] = { 0x1, 0x2, 0x3, 0x4, 0x0, 0x0, 0x0, 0x0};
@@ -127,29 +116,32 @@ int bigendian(void)
 }
 
 /* unfortunately, you can not stat debugfs or proc files for size */
-static void record_file(const char *file, size_t hdr_sz)
+static int record_file(const char *file, ssize_t hdr_sz)
 {
 	unsigned long long size = 0;
 	char buf[BUFSIZ], *sizep;
 	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
 	int r, fd;
+	int err = -EIO;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
 
 	/* put in zeros for file size, then fill true size later */
-	if (hdr_sz)
-		write_or_die(&size, hdr_sz);
+	if (hdr_sz) {
+		if (write(output_fd, &size, hdr_sz) != hdr_sz)
+			goto out;
+	}
 
 	do {
 		r = read(fd, buf, BUFSIZ);
 		if (r > 0) {
 			size += r;
-			write_or_die(buf, r);
+			if (write(output_fd, buf, r) != r)
+				goto out;
 		}
 	} while (r > 0);
-	close(fd);
 
 	/* ugh, handle big-endian hdr_size == 4 */
 	sizep = (char*)&size;
@@ -158,12 +150,18 @@ static void record_file(const char *file, size_t hdr_sz)
 
 	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
 		die("writing to %s", output_file);
+
+	err = 0;
+out:
+	close(fd);
+	return err;
 }
 
-static void read_header_files(void)
+static int read_header_files(void)
 {
 	char *path;
 	struct stat st;
+	int err = -EIO;
 
 	path = get_tracing_file("events/header_page");
 	if (!path)
@@ -172,8 +170,16 @@ static void read_header_files(void)
 	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	write_or_die("header_page", 12);
-	record_file(path, 8);
+	if (write(output_fd, "header_page", 12) != 12) {
+		pr_err("can't write header_page\n");
+		goto out;
+	}
+
+	if (record_file(path, 8) < 0) {
+		pr_err("can't record header_page file\n");
+		goto out;
+	}
+
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
@@ -183,9 +189,20 @@ static void read_header_files(void)
 	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	write_or_die("header_event", 13);
-	record_file(path, 8);
+	if (write(output_fd, "header_event", 13) != 13) {
+		pr_err("can't write header_event\n");
+		goto out;
+	}
+
+	if (record_file(path, 8) < 0) {
+		pr_err("can't record header_event file\n");
+		goto out;
+	}
+
+	err = 0;
+out:
 	put_tracing_file(path);
+	return err;
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -232,7 +249,11 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 		count++;
 	}
 
-	write_or_die(&count, 4);
+	if (write(output_fd, &count, 4) != 4) {
+		err = -EIO;
+		pr_err("can't write count\n");
+		goto out;
+	}
 
 	rewinddir(dir);
 	while ((dent = readdir(dir))) {
@@ -249,8 +270,13 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0)
-			record_file(format, 8);
+		if (ret >= 0) {
+			err = record_file(format, 8);
+			if (err) {
+				free(format);
+				goto out;
+			}
+		}
 		free(format);
 	}
 	err = 0;
@@ -259,17 +285,20 @@ out:
 	return err;
 }
 
-static void read_ftrace_files(struct tracepoint_path *tps)
+static int read_ftrace_files(struct tracepoint_path *tps)
 {
 	char *path;
+	int ret;
 
 	path = get_tracing_file("events/ftrace");
 	if (!path)
 		die("can't get tracing/events/ftrace");
 
-	copy_event_system(path, tps);
+	ret = copy_event_system(path, tps);
 
 	put_tracing_file(path);
+
+	return ret;
 }
 
 static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -312,7 +341,11 @@ static int read_event_files(struct tracepoint_path *tps)
 		count++;
 	}
 
-	write_or_die(&count, 4);
+	if (write(output_fd, &count, 4) != 4) {
+		err = -EIO;
+		pr_err("can't write count\n");
+		goto out;
+	}
 
 	rewinddir(dir);
 	while ((dent = readdir(dir))) {
@@ -330,8 +363,14 @@ static int read_event_files(struct tracepoint_path *tps)
 		sprintf(sys, "%s/%s", path, dent->d_name);
 		ret = stat(sys, &st);
 		if (ret >= 0) {
-			write_or_die(dent->d_name, strlen(dent->d_name) + 1);
-			copy_event_system(sys, tps);
+			ssize_t size = strlen(dent->d_name) + 1;
+
+			if (write(output_fd, dent->d_name, size) != size ||
+			    copy_event_system(sys, tps) < 0) {
+				err = -EIO;
+				free(sys);
+				goto out;
+			}
 		}
 		free(sys);
 	}
@@ -343,29 +382,30 @@ out:
 	return err;
 }
 
-static void read_proc_kallsyms(void)
+static int read_proc_kallsyms(void)
 {
 	unsigned int size;
 	const char *path = "/proc/kallsyms";
 	struct stat st;
-	int ret;
+	int ret, err = 0;
 
 	ret = stat(path, &st);
 	if (ret < 0) {
 		/* not found */
 		size = 0;
-		write_or_die(&size, 4);
-		return;
+		if (write(output_fd, &size, 4) != 4)
+			err = -EIO;
+		return err;
 	}
-	record_file(path, 4);
+	return record_file(path, 4);
 }
 
-static void read_ftrace_printk(void)
+static int read_ftrace_printk(void)
 {
 	unsigned int size;
 	char *path;
 	struct stat st;
-	int ret;
+	int ret, err = 0;
 
 	path = get_tracing_file("printk_formats");
 	if (!path)
@@ -375,13 +415,15 @@ static void read_ftrace_printk(void)
 	if (ret < 0) {
 		/* not found */
 		size = 0;
-		write_or_die(&size, 4);
+		if (write(output_fd, &size, 4) != 4)
+			err = -EIO;
 		goto out;
 	}
-	record_file(path, 4);
+	err = record_file(path, 4);
 
 out:
 	put_tracing_file(path);
+	return err;
 }
 
 static struct tracepoint_path *
@@ -428,9 +470,10 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
-static void tracing_data_header(void)
+static int tracing_data_header(void)
 {
 	char buf[20];
+	ssize_t size;
 
 	/* just guessing this is someone's birthday.. ;) */
 	buf[0] = 23;
@@ -438,9 +481,12 @@ static void tracing_data_header(void)
 	buf[2] = 68;
 	memcpy(buf + 3, "tracing", 7);
 
-	write_or_die(buf, 10);
+	if (write(output_fd, buf, 10) != 10)
+		return -1;
 
-	write_or_die(VERSION, strlen(VERSION) + 1);
+	size = strlen(VERSION) + 1;
+	if (write(output_fd, VERSION, size) != size)
+		return -1;
 
 	/* save endian */
 	if (bigendian())
@@ -450,14 +496,19 @@ static void tracing_data_header(void)
 
 	read_trace_init(buf[0], buf[0]);
 
-	write_or_die(buf, 1);
+	if (write(output_fd, buf, 1) != 1)
+		return -1;
 
 	/* save size of long */
 	buf[0] = sizeof(long);
-	write_or_die(buf, 1);
+	if (write(output_fd, buf, 1) != 1)
+		return -1;
 
 	/* save page_size */
-	write_or_die(&page_size, 4);
+	if (write(output_fd, &page_size, 4) != 4)
+		return -1;
+
+	return 0;
 }
 
 struct tracing_data *tracing_data_get(struct list_head *pattrs,
@@ -465,6 +516,7 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
 {
 	struct tracepoint_path *tps;
 	struct tracing_data *tdata;
+	int err1, err2, err3, err4, err5, err6;
 
 	output_fd = fd;
 
@@ -498,12 +550,12 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
 		output_fd = temp_fd;
 	}
 
-	tracing_data_header();
-	read_header_files();
-	read_ftrace_files(tps);
-	read_event_files(tps);
-	read_proc_kallsyms();
-	read_ftrace_printk();
+	err1 = tracing_data_header();
+	err2 = read_header_files();
+	err3 = read_ftrace_files(tps);
+	err4 = read_event_files(tps);
+	err5 = read_proc_kallsyms();
+	err6 = read_ftrace_printk();
 
 	/*
 	 * All tracing data are stored by now, we can restore
@@ -515,22 +567,31 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
 		output_fd = fd;
 	}
 
+	if (err1 + err2 + err3 + err4 + err5 + err6 < 0) {
+		free(tdata);
+		tdata = NULL;
+	}
+
 	put_tracepoints_path(tps);
 	return tdata;
 }
 
-void tracing_data_put(struct tracing_data *tdata)
+int tracing_data_put(struct tracing_data *tdata)
 {
+	int err = 0;
+
 	if (tdata->temp) {
-		record_file(tdata->temp_file, 0);
+		err = record_file(tdata->temp_file, 0);
 		unlink(tdata->temp_file);
 	}
 
 	free(tdata);
+	return err;
 }
 
 int read_tracing_data(int fd, struct list_head *pattrs)
 {
+	int err;
 	struct tracing_data *tdata;
 
 	/*
@@ -541,6 +602,6 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 	if (!tdata)
 		return -ENOMEM;
 
-	tracing_data_put(tdata);
-	return 0;
+	err = tracing_data_put(tdata);
+	return err;
 }
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index 28ccde8ba20f..1978c398ad87 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -68,7 +68,7 @@ struct tracing_data {
 
 struct tracing_data *tracing_data_get(struct list_head *pattrs,
 				      int fd, bool temp);
-void tracing_data_put(struct tracing_data *tdata);
+int tracing_data_put(struct tracing_data *tdata);
 
 
 struct addr_location;
-- 
1.7.11.7

[PATCH 4/9] perf util: Get rid of die() calls from trace-event-info.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Now remove all remaining die() calls and convert them to check return
value and propagate it.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-info.c | 114 +++++++++++++++++++++++--------------
 1 file changed, 72 insertions(+), 42 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 05cf94ef57e4..f006529d2284 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -43,7 +43,6 @@
 
 #define VERSION "0.5"
 
-static const char *output_file = "trace.info";
 static int output_fd;
 
 
@@ -125,8 +124,10 @@ static int record_file(const char *file, ssize_t hdr_sz)
 	int err = -EIO;
 
 	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
+	if (fd < 0) {
+		pr_debug("Can't read '%s'", file);
+		return -errno;
+	}
 
 	/* put in zeros for file size, then fill true size later */
 	if (hdr_sz) {
@@ -148,8 +149,10 @@ static int record_file(const char *file, ssize_t hdr_sz)
 	if (bigendian())
 		sizep += sizeof(u64) - hdr_sz;
 
-	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
-		die("writing to %s", output_file);
+	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0) {
+		pr_debug("writing file size failed\n");
+		goto out;
+	}
 
 	err = 0;
 out:
@@ -164,11 +167,15 @@ static int read_header_files(void)
 	int err = -EIO;
 
 	path = get_tracing_file("events/header_page");
-	if (!path)
-		die("can't get tracing/events/header_page");
+	if (!path) {
+		pr_err("can't get tracing/events/header_page");
+		return -ENOMEM;
+	}
 
-	if (stat(path, &st) < 0)
-		die("can't read '%s'", path);
+	if (stat(path, &st) < 0) {
+		pr_err("can't read '%s'", path);
+		goto out;
+	}
 
 	if (write(output_fd, "header_page", 12) != 12) {
 		pr_err("can't write header_page\n");
@@ -183,11 +190,16 @@ static int read_header_files(void)
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	if (!path)
-		die("can't get tracing/events/header_event");
+	if (!path) {
+		pr_err("can't get tracing/events/header_event");
+		err = -ENOMEM;
+		goto out;
+	}
 
-	if (stat(path, &st) < 0)
-		die("can't read '%s'", path);
+	if (stat(path, &st) < 0) {
+		pr_err("can't read '%s'", path);
+		goto out;
+	}
 
 	if (write(output_fd, "header_event", 13) != 13) {
 		pr_err("can't write header_event\n");
@@ -227,8 +239,10 @@ static int copy_event_system(const char *sys, struct tracepoint_path *tps)
 	int err;
 
 	dir = opendir(sys);
-	if (!dir)
-		die("can't read directory '%s'", sys);
+	if (!dir) {
+		pr_err("can't read directory '%s'", sys);
+		return -errno;
+	}
 
 	while ((dent = readdir(dir))) {
 		if (dent->d_type != DT_DIR ||
@@ -291,8 +305,10 @@ static int read_ftrace_files(struct tracepoint_path *tps)
 	int ret;
 
 	path = get_tracing_file("events/ftrace");
-	if (!path)
-		die("can't get tracing/events/ftrace");
+	if (!path) {
+		pr_err("can't get tracing/events/ftrace");
+		return -ENOMEM;
+	}
 
 	ret = copy_event_system(path, tps);
 
@@ -324,12 +340,17 @@ static int read_event_files(struct tracepoint_path *tps)
 	int err;
 
 	path = get_tracing_file("events");
-	if (!path)
-		die("can't get tracing/events");
+	if (!path) {
+		pr_err("can't get tracing/events");
+		return -ENOMEM;
+	}
 
 	dir = opendir(path);
-	if (!dir)
-		die("can't read directory '%s'", path);
+	if (!dir) {
+		err = -errno;
+		pr_err("can't read directory '%s'", path);
+		goto out;
+	}
 
 	while ((dent = readdir(dir))) {
 		if (dent->d_type != DT_DIR ||
@@ -408,8 +429,10 @@ static int read_ftrace_printk(void)
 	int ret, err = 0;
 
 	path = get_tracing_file("printk_formats");
-	if (!path)
-		die("can't get tracing/printk_formats");
+	if (!path) {
+		pr_err("can't get tracing/printk_formats");
+		return -ENOMEM;
+	}
 
 	ret = stat(path, &st);
 	if (ret < 0) {
@@ -426,6 +449,19 @@ out:
 	return err;
 }
 
+static void
+put_tracepoints_path(struct tracepoint_path *tps)
+{
+	while (tps) {
+		struct tracepoint_path *t = tps;
+
+		tps = tps->next;
+		free(t->name);
+		free(t->system);
+		free(t);
+	}
+}
+
 static struct tracepoint_path *
 get_tracepoints_path(struct list_head *pattrs)
 {
@@ -438,27 +474,17 @@ get_tracepoints_path(struct list_head *pattrs)
 			continue;
 		++nr_tracepoints;
 		ppath->next = tracepoint_id_to_path(pos->attr.config);
-		if (!ppath->next)
-			die("%s\n", "No memory to alloc tracepoints list");
+		if (!ppath->next) {
+			pr_err("No memory to alloc tracepoints list\n");
+			put_tracepoints_path(&path);
+			return NULL;
+		}
 		ppath = ppath->next;
 	}
 
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
-static void
-put_tracepoints_path(struct tracepoint_path *tps)
-{
-	while (tps) {
-		struct tracepoint_path *t = tps;
-
-		tps = tps->next;
-		free(t->name);
-		free(t->system);
-		free(t);
-	}
-}
-
 bool have_tracepoints(struct list_head *pattrs)
 {
 	struct perf_evsel *pos;
@@ -536,12 +562,16 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
 
 		snprintf(tdata->temp_file, sizeof(tdata->temp_file),
 			 "/tmp/perf-XXXXXX");
-		if (!mkstemp(tdata->temp_file))
-			die("Can't make temp file");
+		if (!mkstemp(tdata->temp_file)) {
+			pr_err("Can't make temp file");
+			return NULL;
+		}
 
 		temp_fd = open(tdata->temp_file, O_RDWR);
-		if (temp_fd < 0)
-			die("Can't read '%s'", tdata->temp_file);
+		if (temp_fd < 0) {
+			pr_err("Can't read '%s'", tdata->temp_file);
+			return NULL;
+		}
 
 		/*
 		 * Set the temp file the default output, so all the
-- 
1.7.11.7

[PATCH 5/9] perf util: Handle failure case in trace_report()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

If pevent allocation in read_trace_init() fails, trace_report() will
return -1 and *ppevent is set to NULL.  Its callers should check this
case and handle it properly.

This is also a preparation for the removal of *die() calls.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/header.c           | 11 +++++++---
 tools/perf/util/trace-event-read.c | 41 ++++++++++++++++++++++----------------
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index a9b7349f7c5f..f2c33c4cfc38 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1672,8 +1672,8 @@ static int process_tracing_data(struct perf_file_section *section __maybe_unused
 				struct perf_header *ph __maybe_unused,
 				int fd, void *data)
 {
-	trace_report(fd, data, false);
-	return 0;
+	ssize_t ret = trace_report(fd, data, false);
+	return ret < 0 ? -1 : 0;
 }
 
 static int process_build_id(struct perf_file_section *section,
@@ -2752,6 +2752,11 @@ static int perf_evsel__prepare_tracepoint_event(struct perf_evsel *evsel,
 	if (evsel->tp_format)
 		return 0;
 
+	if (pevent == NULL) {
+		pr_debug("broken or missing trace data\n");
+		return -1;
+	}
+
 	event = pevent_find_event(pevent, evsel->attr.config);
 	if (event == NULL)
 		return -1;
@@ -3100,7 +3105,7 @@ int perf_event__process_tracing_data(union perf_event *event,
 		}
 	}
 
-	if (size_read + padding != size) {
+	if (size_read + padding != size || session->pevent == NULL) {
 		pr_err("%s: tracing data size mismatch", __func__);
 		return -1;
 	}
diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 7cb24635adf2..129362b97ca5 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -293,7 +293,10 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 	int show_version = 0;
 	int show_funcs = 0;
 	int show_printk = 0;
-	ssize_t size;
+	ssize_t size = -1;
+	struct pevent *pevent;
+
+	*ppevent = NULL;
 
 	calc_data_size = 1;
 	repipe = __repipe;
@@ -317,34 +320,38 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 	file_bigendian = buf[0];
 	host_bigendian = bigendian();
 
-	*ppevent = read_trace_init(file_bigendian, host_bigendian);
-	if (*ppevent == NULL)
-		die("read_trace_init failed");
+	pevent = read_trace_init(file_bigendian, host_bigendian);
+	if (pevent == NULL) {
+		pr_err("read_trace_init failed");
+		goto out;
+	}
 
 	read_or_die(buf, 1);
 	long_size = buf[0];
 
-	page_size = read4(*ppevent);
-
-	read_header_files(*ppevent);
+	page_size = read4(pevent);
 
-	read_ftrace_files(*ppevent);
-	read_event_files(*ppevent);
-	read_proc_kallsyms(*ppevent);
-	read_ftrace_printk(*ppevent);
+	read_header_files(pevent);
+	read_ftrace_files(pevent);
+	read_event_files(pevent);
+	read_proc_kallsyms(pevent);
+	read_ftrace_printk(pevent);
 
 	size = calc_data_size - 1;
 	calc_data_size = 0;
 	repipe = false;
 
 	if (show_funcs) {
-		pevent_print_funcs(*ppevent);
-		return size;
-	}
-	if (show_printk) {
-		pevent_print_printk(*ppevent);
-		return size;
+		pevent_print_funcs(pevent);
+	} else if (show_printk) {
+		pevent_print_printk(pevent);
 	}
 
+	*ppevent = pevent;
+	pevent = NULL;
+
+out:
+	if (pevent)
+		pevent_free(pevent);
 	return size;
 }
-- 
1.7.11.7

[PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Check return value of malloc() and fail if error.  Now read_string()
can return NULL also check its return value and bail out.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-read.c | 90 ++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 129362b97ca5..62dd2168f4f5 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -48,16 +48,6 @@ static int long_size;
 static ssize_t calc_data_size;
 static bool repipe;
 
-static void *malloc_or_die(int size)
-{
-	void *ret;
-
-	ret = malloc(size);
-	if (!ret)
-		die("malloc");
-	return ret;
-}
-
 static int do_read(int fd, void *buf, int size)
 {
 	int rsize = size;
@@ -158,48 +148,57 @@ static char *read_string(void)
 	if (calc_data_size)
 		calc_data_size += size;
 
-	str = malloc_or_die(size);
-	memcpy(str, buf, size);
+	str = malloc(size);
+	if (str)
+		memcpy(str, buf, size);
 
 	return str;
 }
 
-static void read_proc_kallsyms(struct pevent *pevent)
+static int read_proc_kallsyms(struct pevent *pevent)
 {
 	unsigned int size;
 	char *buf;
 
 	size = read4(pevent);
 	if (!size)
-		return;
+		return 0;
+
+	buf = malloc(size + 1);
+	if (buf == NULL)
+		return -1;
 
-	buf = malloc_or_die(size + 1);
 	read_or_die(buf, size);
 	buf[size] = '\0';
 
 	parse_proc_kallsyms(pevent, buf, size);
 
 	free(buf);
+	return 0;
 }
 
-static void read_ftrace_printk(struct pevent *pevent)
+static int read_ftrace_printk(struct pevent *pevent)
 {
 	unsigned int size;
 	char *buf;
 
 	size = read4(pevent);
 	if (!size)
-		return;
+		return 0;
+
+	buf = malloc(size);
+	if (buf == NULL)
+		return -1;
 
-	buf = malloc_or_die(size);
 	read_or_die(buf, size);
 
 	parse_ftrace_printk(pevent, buf, size);
 
 	free(buf);
+	return 0;
 }
 
-static void read_header_files(struct pevent *pevent)
+static int read_header_files(struct pevent *pevent)
 {
 	unsigned long long size;
 	char *header_event;
@@ -224,65 +223,87 @@ static void read_header_files(struct pevent *pevent)
 		die("did not read header event");
 
 	size = read8(pevent);
-	header_event = malloc_or_die(size);
+	header_event = malloc(size);
+	if (header_event == NULL)
+		return -1;
+
 	read_or_die(header_event, size);
 	free(header_event);
+	return 0;
 }
 
-static void read_ftrace_file(struct pevent *pevent, unsigned long long size)
+static int read_ftrace_file(struct pevent *pevent, unsigned long long size)
 {
 	char *buf;
 
-	buf = malloc_or_die(size);
+	buf = malloc(size);
+	if (buf == NULL)
+		return -1;
+
 	read_or_die(buf, size);
 	parse_ftrace_file(pevent, buf, size);
 	free(buf);
+	return 0;
 }
 
-static void read_event_file(struct pevent *pevent, char *sys,
+static int read_event_file(struct pevent *pevent, char *sys,
 			    unsigned long long size)
 {
 	char *buf;
 
-	buf = malloc_or_die(size);
+	buf = malloc(size);
+	if (buf == NULL)
+		return -1;
+
 	read_or_die(buf, size);
 	parse_event_file(pevent, buf, size, sys);
 	free(buf);
+	return 0;
 }
 
-static void read_ftrace_files(struct pevent *pevent)
+static int read_ftrace_files(struct pevent *pevent)
 {
 	unsigned long long size;
 	int count;
 	int i;
+	int ret;
 
 	count = read4(pevent);
 
 	for (i = 0; i < count; i++) {
 		size = read8(pevent);
-		read_ftrace_file(pevent, size);
+		ret = read_ftrace_file(pevent, size);
+		if (ret)
+			return ret;
 	}
+	return 0;
 }
 
-static void read_event_files(struct pevent *pevent)
+static int read_event_files(struct pevent *pevent)
 {
 	unsigned long long size;
 	char *sys;
 	int systems;
 	int count;
 	int i,x;
+	int ret;
 
 	systems = read4(pevent);
 
 	for (i = 0; i < systems; i++) {
 		sys = read_string();
+		if (sys == NULL)
+			return -1;
 
 		count = read4(pevent);
 		for (x=0; x < count; x++) {
 			size = read8(pevent);
-			read_event_file(pevent, sys, size);
+			ret = read_event_file(pevent, sys, size);
+			if (ret)
+				return ret;
 		}
 	}
+	return 0;
 }
 
 ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
@@ -312,6 +333,8 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 		die("not a trace file (missing 'tracing' tag)");
 
 	version = read_string();
+	if (version == NULL)
+		return -1;
 	if (show_version)
 		printf("version = %s\n", version);
 	free(version);
@@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 
 	page_size = read4(pevent);
 
-	read_header_files(pevent);
-	read_ftrace_files(pevent);
-	read_event_files(pevent);
-	read_proc_kallsyms(pevent);
-	read_ftrace_printk(pevent);
+	if (read_header_files(pevent) ||
+	    read_ftrace_files(pevent) ||
+	    read_event_files(pevent) ||
+	    read_proc_kallsyms(pevent) ||
+	    read_ftrace_printk(pevent))
+		goto out;
 
 	size = calc_data_size - 1;
 	calc_data_size = 0;
-- 
1.7.11.7

[PATCH 7/9] perf util: Get rid of read_or_die() in trace-event-read.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Rename it to do_read and original do_read to __do_read, and check
their return value.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-read.c | 80 +++++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 62dd2168f4f5..87f0ccd54cdc 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -48,7 +48,7 @@ static int long_size;
 static ssize_t calc_data_size;
 static bool repipe;
 
-static int do_read(int fd, void *buf, int size)
+static int __do_read(int fd, void *buf, int size)
 {
 	int rsize = size;
 
@@ -61,8 +61,10 @@ static int do_read(int fd, void *buf, int size)
 		if (repipe) {
 			int retw = write(STDOUT_FILENO, buf, ret);
 
-			if (retw <= 0 || retw != ret)
-				die("repiping input file");
+			if (retw <= 0 || retw != ret) {
+				pr_debug("repiping input file");
+				return -1;
+			}
 		}
 
 		size -= ret;
@@ -72,14 +74,16 @@ static int do_read(int fd, void *buf, int size)
 	return rsize;
 }
 
-static int read_or_die(void *data, int size)
+static int do_read(void *data, int size)
 {
 	int r;
 
-	r = do_read(input_fd, data, size);
-	if (r <= 0)
-		die("reading input file (size expected=%d received=%d)",
-		    size, r);
+	r = __do_read(input_fd, data, size);
+	if (r <= 0) {
+		pr_debug("reading input file (size expected=%d received=%d)",
+			 size, r);
+		return -1;
+	}
 
 	if (calc_data_size)
 		calc_data_size += r;
@@ -95,7 +99,7 @@ static void skip(int size)
 
 	while (size) {
 		r = size > BUFSIZ ? BUFSIZ : size;
-		read_or_die(buf, r);
+		do_read(buf, r);
 		size -= r;
 	};
 }
@@ -104,7 +108,8 @@ static unsigned int read4(struct pevent *pevent)
 {
 	unsigned int data;
 
-	read_or_die(&data, 4);
+	if (do_read(&data, 4) < 0)
+		return 0;
 	return __data2host4(pevent, data);
 }
 
@@ -112,7 +117,8 @@ static unsigned long long read8(struct pevent *pevent)
 {
 	unsigned long long data;
 
-	read_or_die(&data, 8);
+	if (do_read(&data, 8) < 0)
+		return 0;
 	return __data2host8(pevent, data);
 }
 
@@ -168,7 +174,10 @@ static int read_proc_kallsyms(struct pevent *pevent)
 	if (buf == NULL)
 		return -1;
 
-	read_or_die(buf, size);
+	if (do_read(buf, size) < 0) {
+		free(buf);
+		return -1;
+	}
 	buf[size] = '\0';
 
 	parse_proc_kallsyms(pevent, buf, size);
@@ -182,6 +191,7 @@ static int read_ftrace_printk(struct pevent *pevent)
 	unsigned int size;
 	char *buf;
 
+	/* it can have 0 size */
 	size = read4(pevent);
 	if (!size)
 		return 0;
@@ -190,7 +200,10 @@ static int read_ftrace_printk(struct pevent *pevent)
 	if (buf == NULL)
 		return -1;
 
-	read_or_die(buf, size);
+	if (do_read(buf, size) < 0) {
+		free(buf);
+		return -1;
+	}
 
 	parse_ftrace_printk(pevent, buf, size);
 
@@ -203,8 +216,10 @@ static int read_header_files(struct pevent *pevent)
 	unsigned long long size;
 	char *header_event;
 	char buf[BUFSIZ];
+	int ret = 0;
 
-	read_or_die(buf, 12);
+	if (do_read(buf, 12) < 0)
+		return -1;
 
 	if (memcmp(buf, "header_page", 12) != 0)
 		die("did not read header page");
@@ -218,7 +233,9 @@ static int read_header_files(struct pevent *pevent)
 	 */
 	long_size = header_page_size_size;
 
-	read_or_die(buf, 13);
+	if (do_read(buf, 13) < 0)
+		return -1;
+
 	if (memcmp(buf, "header_event", 13) != 0)
 		die("did not read header event");
 
@@ -227,9 +244,11 @@ static int read_header_files(struct pevent *pevent)
 	if (header_event == NULL)
 		return -1;
 
-	read_or_die(header_event, size);
+	if (do_read(header_event, size) < 0)
+		ret = -1;
+
 	free(header_event);
-	return 0;
+	return ret;
 }
 
 static int read_ftrace_file(struct pevent *pevent, unsigned long long size)
@@ -240,7 +259,11 @@ static int read_ftrace_file(struct pevent *pevent, unsigned long long size)
 	if (buf == NULL)
 		return -1;
 
-	read_or_die(buf, size);
+	if (do_read(buf, size) < 0) {
+		free(buf);
+		return -1;
+	}
+
 	parse_ftrace_file(pevent, buf, size);
 	free(buf);
 	return 0;
@@ -255,7 +278,11 @@ static int read_event_file(struct pevent *pevent, char *sys,
 	if (buf == NULL)
 		return -1;
 
-	read_or_die(buf, size);
+	if (do_read(buf, size) < 0) {
+		free(buf);
+		return -1;
+	}
+
 	parse_event_file(pevent, buf, size, sys);
 	free(buf);
 	return 0;
@@ -296,6 +323,7 @@ static int read_event_files(struct pevent *pevent)
 			return -1;
 
 		count = read4(pevent);
+
 		for (x=0; x < count; x++) {
 			size = read8(pevent);
 			ret = read_event_file(pevent, sys, size);
@@ -324,11 +352,13 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 
 	input_fd = fd;
 
-	read_or_die(buf, 3);
+	if (do_read(buf, 3) < 0)
+		return -1;
 	if (memcmp(buf, test, 3) != 0)
 		die("no trace data in the file");
 
-	read_or_die(buf, 7);
+	if (do_read(buf, 7) < 0)
+		return -1;
 	if (memcmp(buf, "tracing", 7) != 0)
 		die("not a trace file (missing 'tracing' tag)");
 
@@ -339,7 +369,8 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 		printf("version = %s\n", version);
 	free(version);
 
-	read_or_die(buf, 1);
+	if (do_read(buf, 1) < 0)
+		return -1;
 	file_bigendian = buf[0];
 	host_bigendian = bigendian();
 
@@ -349,10 +380,13 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 		goto out;
 	}
 
-	read_or_die(buf, 1);
+	if (do_read(buf, 1) < 0)
+		goto out;
 	long_size = buf[0];
 
 	page_size = read4(pevent);
+	if (!page_size)
+		goto out;
 
 	if (read_header_files(pevent) ||
 	    read_ftrace_files(pevent) ||
-- 
1.7.11.7

[PATCH 8/9] perf util: Get rid of die() calls in trace-data-read.c

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Convert them to pr_debug() and propagate error code.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-read.c | 44 +++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 87f0ccd54cdc..7ce901643205 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -132,17 +132,23 @@ static char *read_string(void)
 
 	for (;;) {
 		r = read(input_fd, &c, 1);
-		if (r < 0)
-			die("reading input file");
+		if (r < 0) {
+			pr_debug("reading input file");
+			goto out;
+		}
 
-		if (!r)
-			die("no data");
+		if (!r) {
+			pr_debug("no data");
+			goto out;
+		}
 
 		if (repipe) {
 			int retw = write(STDOUT_FILENO, &c, 1);
 
-			if (retw <= 0 || retw != r)
-				die("repiping input file string");
+			if (retw <= 0 || retw != r) {
+				pr_debug("repiping input file string");
+				goto out;
+			}
 		}
 
 		buf[size++] = c;
@@ -157,7 +163,7 @@ static char *read_string(void)
 	str = malloc(size);
 	if (str)
 		memcpy(str, buf, size);
-
+out:
 	return str;
 }
 
@@ -221,8 +227,10 @@ static int read_header_files(struct pevent *pevent)
 	if (do_read(buf, 12) < 0)
 		return -1;
 
-	if (memcmp(buf, "header_page", 12) != 0)
-		die("did not read header page");
+	if (memcmp(buf, "header_page", 12) != 0) {
+		pr_debug("did not read header page");
+		return -1;
+	}
 
 	size = read8(pevent);
 	skip(size);
@@ -236,8 +244,10 @@ static int read_header_files(struct pevent *pevent)
 	if (do_read(buf, 13) < 0)
 		return -1;
 
-	if (memcmp(buf, "header_event", 13) != 0)
-		die("did not read header event");
+	if (memcmp(buf, "header_event", 13) != 0) {
+		pr_debug("did not read header event");
+		return -1;
+	}
 
 	size = read8(pevent);
 	header_event = malloc(size);
@@ -354,13 +364,17 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 
 	if (do_read(buf, 3) < 0)
 		return -1;
-	if (memcmp(buf, test, 3) != 0)
-		die("no trace data in the file");
+	if (memcmp(buf, test, 3) != 0) {
+		pr_debug("no trace data in the file");
+		return -1;
+	}
 
 	if (do_read(buf, 7) < 0)
 		return -1;
-	if (memcmp(buf, "tracing", 7) != 0)
-		die("not a trace file (missing 'tracing' tag)");
+	if (memcmp(buf, "tracing", 7) != 0) {
+		pr_debug("not a trace file (missing 'tracing' tag)");
+		return -1;
+	}
 
 	version = read_string();
 	if (version == NULL)
-- 
1.7.11.7

[PATCH 9/9] perf util: Cleanup calc_data_size logic

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

It's for calculating whole trace data size during reading.  However
relation functions are called only in this file, no need to
conditionalize it with tricky +1 offset and rename the variable to
more meaningful name like trace_data_size.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/trace-event-read.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 7ce901643205..6b0a2c2a4091 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -45,7 +45,7 @@ int file_bigendian;
 int host_bigendian;
 static int long_size;
 
-static ssize_t calc_data_size;
+static ssize_t trace_data_size;
 static bool repipe;
 
 static int __do_read(int fd, void *buf, int size)
@@ -85,8 +85,7 @@ static int do_read(void *data, int size)
 		return -1;
 	}
 
-	if (calc_data_size)
-		calc_data_size += r;
+	trace_data_size += r;
 
 	return r;
 }
@@ -157,8 +156,7 @@ static char *read_string(void)
 			break;
 	}
 
-	if (calc_data_size)
-		calc_data_size += size;
+	trace_data_size += size;
 
 	str = malloc(size);
 	if (str)
@@ -357,9 +355,7 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 
 	*ppevent = NULL;
 
-	calc_data_size = 1;
 	repipe = __repipe;
-
 	input_fd = fd;
 
 	if (do_read(buf, 3) < 0)
@@ -409,8 +405,7 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
 	    read_ftrace_printk(pevent))
 		goto out;
 
-	size = calc_data_size - 1;
-	calc_data_size = 0;
+	size = trace_data_size;
 	repipe = false;
 
 	if (show_funcs) {
-- 
1.7.11.7

Re: [PATCH 1/9] perf util: Let get_tracing_file() can return NULL

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> So that it can be used by other places.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/trace-event-info.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 5729f434c5b1..f241343b7d48 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -62,7 +62,7 @@ static const char *find_debugfs(void)
>  	const char *path = perf_debugfs_mount(NULL);
>  
>  	if (!path)
> -		die("Your kernel not support debugfs filesystem");
> +		pr_err("Your kernel not support debugfs filesystem");

"Your kernel does not support the debugfs filesystem"

I know you just did a s/die/pr_err/ but might as well fix the grammar
too ;-)

>  
>  	return path;
>  }
> @@ -81,8 +81,12 @@ static const char *find_tracing_dir(void)
>  		return tracing;
>  
>  	debugfs = find_debugfs();
> +	if (!debugfs)
> +		return NULL;
>  
> -	tracing = malloc_or_die(strlen(debugfs) + 9);
> +	tracing = malloc(strlen(debugfs) + 9);
> +	if (!tracing)
> +		return NULL;
>  
>  	sprintf(tracing, "%s/tracing", debugfs);
>  
> @@ -99,7 +103,9 @@ static char *get_tracing_file(const char *name)
>  	if (!tracing)
>  		return NULL;
>  
> -	file = malloc_or_die(strlen(tracing) + strlen(name) + 2);
> +	file = malloc(strlen(tracing) + strlen(name) + 2);
> +	if (!file)
> +		return NULL;

Should have clean up as well. That is, if file fails, we need to free
tracing. Otherwise there's a memory leak.

>  
>  	sprintf(file, "%s/%s", tracing, name);
>  	return file;
> @@ -170,6 +176,9 @@ static void read_header_files(void)
>  	struct stat st;
>  
>  	path = get_tracing_file("events/header_page");
> +	if (!path)
> +		die("can't get tracing/events/header_page");
> +
>  	if (stat(path, &st) < 0)
>  		die("can't read '%s'", path);
>  
> @@ -178,6 +187,9 @@ static void read_header_files(void)
>  	put_tracing_file(path);
>  
>  	path = get_tracing_file("events/header_event");
> +	if (!path)
> +		die("can't get tracing/events/header_event");
> +
>  	if (stat(path, &st) < 0)
>  		die("can't read '%s'", path);
>  
> @@ -251,6 +263,8 @@ static void read_ftrace_files(struct tracepoint_path *tps)
>  	char *path;
>  
>  	path = get_tracing_file("events/ftrace");
> +	if (!path)
> +		die("can't get tracing/events/ftrace");
>  
>  	copy_event_system(path, tps);
>  
> @@ -279,6 +293,8 @@ static void read_event_files(struct tracepoint_path *tps)
>  	int ret;
>  
>  	path = get_tracing_file("events");
> +	if (!path)
> +		die("can't get tracing/events");
>  
>  	dir = opendir(path);
>  	if (!dir)
> @@ -343,6 +359,9 @@ static void read_ftrace_printk(void)
>  	int ret;
>  
>  	path = get_tracing_file("printk_formats");
> +	if (!path)
> +		die("can't get tracing/printk_formats");
> +

OK, so we are just moving die to the caller? I'm fine with that. If we
can get the utilities to remove die, that's a big step.

-- Steve

>  	ret = stat(path, &st);
>  	if (ret < 0) {
>  		/* not found */

Re: [PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:

>  struct tracing_data *tracing_data_get(struct list_head *pattrs,
> @@ -465,6 +516,7 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
>  {
>  	struct tracepoint_path *tps;
>  	struct tracing_data *tdata;
> +	int err1, err2, err3, err4, err5, err6;
>  
>  	output_fd = fd;
>  
> @@ -498,12 +550,12 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
>  		output_fd = temp_fd;
>  	}
>  
> -	tracing_data_header();
> -	read_header_files();
> -	read_ftrace_files(tps);
> -	read_event_files(tps);
> -	read_proc_kallsyms();
> -	read_ftrace_printk();
> +	err1 = tracing_data_header();
> +	err2 = read_header_files();
> +	err3 = read_ftrace_files(tps);
> +	err4 = read_event_files(tps);
> +	err5 = read_proc_kallsyms();
> +	err6 = read_ftrace_printk();

ugly ugly ugly

What about:
	int err = 0;

	err += tracing_data_header();
	err += read_header_files();
	[...]

	if (err < 0) {
		free(tdata);
		tdata = NULL;
	}

Also, is the only clean up needed be freeing tdata?

-- Steve


>  
>  	/*
>  	 * All tracing data are stored by now, we can restore
> @@ -515,22 +567,31 @@ struct tracing_data *tracing_data_get(struct list_head *pattrs,
>  		output_fd = fd;
>  	}
>  
> +	if (err1 + err2 + err3 + err4 + err5 + err6 < 0) {
> +		free(tdata);
> +		tdata = NULL;
> +	}
> +
>  	put_tracepoints_path(tps);
>  	return tdata;
>  }
>  

Re: [PATCH 5/9] perf util: Handle failure case in trace_report()

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:

> @@ -3100,7 +3105,7 @@ int perf_event__process_tracing_data(union perf_event *event,
>  		}
>  	}
>  
> -	if (size_read + padding != size) {
> +	if (size_read + padding != size || session->pevent == NULL) {
>  		pr_err("%s: tracing data size mismatch", __func__);

This is a strange error to give when pevent is NULL. Could have been a
malloc failure.


>  		return -1;
>  	}
> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index 7cb24635adf2..129362b97ca5 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -293,7 +293,10 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>  	int show_version = 0;
>  	int show_funcs = 0;
>  	int show_printk = 0;
> -	ssize_t size;
> +	ssize_t size = -1;
> +	struct pevent *pevent;
> +
> +	*ppevent = NULL;
>  
>  	calc_data_size = 1;
>  	repipe = __repipe;
> @@ -317,34 +320,38 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>  	file_bigendian = buf[0];
>  	host_bigendian = bigendian();
>  
> -	*ppevent = read_trace_init(file_bigendian, host_bigendian);
> -	if (*ppevent == NULL)
> -		die("read_trace_init failed");
> +	pevent = read_trace_init(file_bigendian, host_bigendian);
> +	if (pevent == NULL) {

Shouldn't we still set *ppevent to NULL? Or will the user now need to
make sure that its pevent is NULL and always check for error?

-- Steve


> +		pr_err("read_trace_init failed");
> +		goto out;
> +	}
>  
>  	read_or_die(buf, 1);
>  	long_size = buf[0];
>  
> -	page_size = read4(*ppevent);
> -
> -	read_header_files(*ppevent);
> +	page_size = read4(pevent);
>  
> -	read_ftrace_files(*ppevent);
> -	read_event_files(*ppevent);
> -	read_proc_kallsyms(*ppevent);
> -	read_ftrace_printk(*ppevent);
> +	read_header_files(pevent);
> +	read_ftrace_files(pevent);
> +	read_event_files(pevent);
> +	read_proc_kallsyms(pevent);
> +	read_ftrace_printk(pevent);
>  
>  	size = calc_data_size - 1;
>  	calc_data_size = 0;
>  	repipe = false;
>  
>  	if (show_funcs) {
> -		pevent_print_funcs(*ppevent);
> -		return size;
> -	}
> -	if (show_printk) {
> -		pevent_print_printk(*ppevent);
> -		return size;
> +		pevent_print_funcs(pevent);
> +	} else if (show_printk) {
> +		pevent_print_printk(pevent);
>  	}
>  
> +	*ppevent = pevent;
> +	pevent = NULL;
> +
> +out:
> +	if (pevent)
> +		pevent_free(pevent);
>  	return size;
>  }

Re: [PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Peter Zijlstra]

On Tue, 2013-03-19 at 10:35 -0400, Steven Rostedt wrote:
> What about:
>         int err = 0;
> 
>         err += tracing_data_header();
>         err += read_header_files();
>         [...]
> 
>         if (err < 0) {
>                 free(tdata);
>                 tdata = NULL;
>         }
> 
> Also, is the only clean up needed be freeing tdata?

I always use err |= foo() and if (err) but I suppose it doesn't matter
the original error codes are lost both ways which doesn't seem to be a
problem here.

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>  	free(version);
> @@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>  
>  	page_size = read4(pevent);
>  
> -	read_header_files(pevent);
> -	read_ftrace_files(pevent);
> -	read_event_files(pevent);
> -	read_proc_kallsyms(pevent);
> -	read_ftrace_printk(pevent);
> +	if (read_header_files(pevent) ||
> +	    read_ftrace_files(pevent) ||
> +	    read_event_files(pevent) ||
> +	    read_proc_kallsyms(pevent) ||
> +	    read_ftrace_printk(pevent))
> +		goto out;

I think I like the err += func() and check for err < 0, better.

-- Steve

>  
>  	size = calc_data_size - 1;
>  	calc_data_size = 0;

Re: [PATCH 7/9] perf util: Get rid of read_or_die() in trace-event-read.c

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Rename it to do_read and original do_read to __do_read, and check
> their return value.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/trace-event-read.c | 80 +++++++++++++++++++++++++++-----------
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index 62dd2168f4f5..87f0ccd54cdc 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -48,7 +48,7 @@ static int long_size;
>  static ssize_t calc_data_size;
>  static bool repipe;
>  
> -static int do_read(int fd, void *buf, int size)
> +static int __do_read(int fd, void *buf, int size)
>  {
>  	int rsize = size;
>  
> @@ -61,8 +61,10 @@ static int do_read(int fd, void *buf, int size)
>  		if (repipe) {
>  			int retw = write(STDOUT_FILENO, buf, ret);
>  
> -			if (retw <= 0 || retw != ret)
> -				die("repiping input file");
> +			if (retw <= 0 || retw != ret) {
> +				pr_debug("repiping input file");

Again, why debug and not err?

> +				return -1;
> +			}
>  		}
>  
>  		size -= ret;
> @@ -72,14 +74,16 @@ static int do_read(int fd, void *buf, int size)
>  	return rsize;
>  }
>  
> -static int read_or_die(void *data, int size)
> +static int do_read(void *data, int size)
>  {
>  	int r;
>  
> -	r = do_read(input_fd, data, size);
> -	if (r <= 0)
> -		die("reading input file (size expected=%d received=%d)",
> -		    size, r);
> +	r = __do_read(input_fd, data, size);
> +	if (r <= 0) {
> +		pr_debug("reading input file (size expected=%d received=%d)",
> +			 size, r);
> +		return -1;
> +	}
>  
>  	if (calc_data_size)
>  		calc_data_size += r;
> @@ -95,7 +99,7 @@ static void skip(int size)
>  
>  	while (size) {
>  		r = size > BUFSIZ ? BUFSIZ : size;
> -		read_or_die(buf, r);
> +		do_read(buf, r);

Shouldn't this check the result of do_read()?

>  		size -= r;
>  	};
>  }

-- Steve

Re: [PATCH 8/9] perf util: Get rid of die() calls in trace-data-read.c

[Steven Rostedt]

On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Convert them to pr_debug() and propagate error code.

Shouldn't they be pr_err(). I mean, if the old code would kill the
process, why just keep it as a debug output?

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

Re: [PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Steven Rostedt]

On Tue, 2013-03-19 at 15:49 +0100, Peter Zijlstra wrote:
> On Tue, 2013-03-19 at 10:35 -0400, Steven Rostedt wrote:
> > What about:
> >         int err = 0;
> > 
> >         err += tracing_data_header();
> >         err += read_header_files();
> >         [...]
> > 
> >         if (err < 0) {
> >                 free(tdata);
> >                 tdata = NULL;
> >         }
> > 
> > Also, is the only clean up needed be freeing tdata?
> 
> I always use err |= foo() and if (err) but I suppose it doesn't matter
> the original error codes are lost both ways which doesn't seem to be a
> problem here.

err |= foo() is fine too. Both are better that err1, err2, err3, ...,
errN :-)

-- Steve

Re: [PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Peter Zijlstra]

On Tue, 2013-03-19 at 10:59 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 15:49 +0100, Peter Zijlstra wrote:
> > On Tue, 2013-03-19 at 10:35 -0400, Steven Rostedt wrote:
> > > What about:
> > >         int err = 0;
> > > 
> > >         err += tracing_data_header();
> > >         err += read_header_files();
> > >         [...]
> > > 
> > >         if (err < 0) {
> > >                 free(tdata);
> > >                 tdata = NULL;
> > >         }
> > > 
> > > Also, is the only clean up needed be freeing tdata?
> > 
> > I always use err |= foo() and if (err) but I suppose it doesn't matter
> > the original error codes are lost both ways which doesn't seem to be a
> > problem here.
> 
> err |= foo() is fine too. Both are better that err1, err2, err3, ...,
> errN :-)

<whinge>

The += thing has a problem where functions can return both positive and
negative values, you could get an accidental 0 (success) but coupled
with the proposed <0 test you get a much larger accident space :-)

And while totally hideous the err1..errN case preserves the actual
return codes if one would actually need those.

</whinge>

/me crawls back under his rock noaw :-)

Re: [PATCH 1/9] perf util: Let get_tracing_file() can return NULL

[Namhyung Kim]

Hi Steve,

On Tue, 19 Mar 2013 09:54:21 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> So that it can be used by other places.
>> 
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/util/trace-event-info.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
>> index 5729f434c5b1..f241343b7d48 100644
>> --- a/tools/perf/util/trace-event-info.c
>> +++ b/tools/perf/util/trace-event-info.c
>> @@ -62,7 +62,7 @@ static const char *find_debugfs(void)
>>  	const char *path = perf_debugfs_mount(NULL);
>>  
>>  	if (!path)
>> -		die("Your kernel not support debugfs filesystem");
>> +		pr_err("Your kernel not support debugfs filesystem");
>
> "Your kernel does not support the debugfs filesystem"
>
> I know you just did a s/die/pr_err/ but might as well fix the grammar
> too ;-)

Right, will do.

>
>>  
>>  	return path;
>>  }
>> @@ -81,8 +81,12 @@ static const char *find_tracing_dir(void)
>>  		return tracing;
>>  
>>  	debugfs = find_debugfs();
>> +	if (!debugfs)
>> +		return NULL;
>>  
>> -	tracing = malloc_or_die(strlen(debugfs) + 9);
>> +	tracing = malloc(strlen(debugfs) + 9);
>> +	if (!tracing)
>> +		return NULL;
>>  
>>  	sprintf(tracing, "%s/tracing", debugfs);
>>  
>> @@ -99,7 +103,9 @@ static char *get_tracing_file(const char *name)
>>  	if (!tracing)
>>  		return NULL;
>>  
>> -	file = malloc_or_die(strlen(tracing) + strlen(name) + 2);
>> +	file = malloc(strlen(tracing) + strlen(name) + 2);
>> +	if (!file)
>> +		return NULL;
>
> Should have clean up as well. That is, if file fails, we need to free
> tracing. Otherwise there's a memory leak.

Well, I'm not sure - there's a static pointer in the
find_tracing_dir().  If we free tracing here, we'll need to reset
tracing_found in the function also.  So I just kept it. :(

>
>>  
>>  	sprintf(file, "%s/%s", tracing, name);
>>  	return file;
>> @@ -170,6 +176,9 @@ static void read_header_files(void)
>>  	struct stat st;
>>  
>>  	path = get_tracing_file("events/header_page");
>> +	if (!path)
>> +		die("can't get tracing/events/header_page");
>> +
>>  	if (stat(path, &st) < 0)
>>  		die("can't read '%s'", path);
>>  
>> @@ -178,6 +187,9 @@ static void read_header_files(void)
>>  	put_tracing_file(path);
>>  
>>  	path = get_tracing_file("events/header_event");
>> +	if (!path)
>> +		die("can't get tracing/events/header_event");
>> +
>>  	if (stat(path, &st) < 0)
>>  		die("can't read '%s'", path);
>>  
>> @@ -251,6 +263,8 @@ static void read_ftrace_files(struct tracepoint_path *tps)
>>  	char *path;
>>  
>>  	path = get_tracing_file("events/ftrace");
>> +	if (!path)
>> +		die("can't get tracing/events/ftrace");
>>  
>>  	copy_event_system(path, tps);
>>  
>> @@ -279,6 +293,8 @@ static void read_event_files(struct tracepoint_path *tps)
>>  	int ret;
>>  
>>  	path = get_tracing_file("events");
>> +	if (!path)
>> +		die("can't get tracing/events");
>>  
>>  	dir = opendir(path);
>>  	if (!dir)
>> @@ -343,6 +359,9 @@ static void read_ftrace_printk(void)
>>  	int ret;
>>  
>>  	path = get_tracing_file("printk_formats");
>> +	if (!path)
>> +		die("can't get tracing/printk_formats");
>> +
>
> OK, so we are just moving die to the caller? I'm fine with that. If we
> can get the utilities to remove die, that's a big step.

They'll be removed on later patches, I moved them to callers since I
don't have proper error handling code at this time.

Thanks for review,
Namhyung

Re: [PATCH 3/9] perf util: Get rid of write_or_die() from trace-event-info.c

[Namhyung Kim]

On Tue, 19 Mar 2013 16:04:03 +0100, Peter Zijlstra wrote:
> On Tue, 2013-03-19 at 10:59 -0400, Steven Rostedt wrote:
>> On Tue, 2013-03-19 at 15:49 +0100, Peter Zijlstra wrote:
>> > On Tue, 2013-03-19 at 10:35 -0400, Steven Rostedt wrote:
>> > > What about:
>> > >         int err = 0;
>> > > 
>> > >         err += tracing_data_header();
>> > >         err += read_header_files();
>> > >         [...]
>> > > 
>> > >         if (err < 0) {
>> > >                 free(tdata);
>> > >                 tdata = NULL;
>> > >         }
>> > > 
>> > > Also, is the only clean up needed be freeing tdata?
>> > 
>> > I always use err |= foo() and if (err) but I suppose it doesn't matter
>> > the original error codes are lost both ways which doesn't seem to be a
>> > problem here.
>> 
>> err |= foo() is fine too. Both are better that err1, err2, err3, ...,
>> errN :-)
>
> <whinge>
>
> The += thing has a problem where functions can return both positive and
> negative values, you could get an accidental 0 (success) but coupled
> with the proposed <0 test you get a much larger accident space :-)
>
> And while totally hideous the err1..errN case preserves the actual
> return codes if one would actually need those.
>
> </whinge>
>
> /me crawls back under his rock noaw :-)

Sorry for the ugliness, forgot to update the code before sending ;-)

So I'll convert them to err |= foo() style as I don't care about the
actual return value at this time.

Thanks,
Namhyung

Re: [PATCH 5/9] perf util: Handle failure case in trace_report()

[Namhyung Kim]

On Tue, 19 Mar 2013 10:47:53 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>
>> @@ -3100,7 +3105,7 @@ int perf_event__process_tracing_data(union perf_event *event,
>>  		}
>>  	}
>>  
>> -	if (size_read + padding != size) {
>> +	if (size_read + padding != size || session->pevent == NULL) {
>>  		pr_err("%s: tracing data size mismatch", __func__);
>
> This is a strange error to give when pevent is NULL. Could have been a
> malloc failure.

Hmm.. right.  I was just too lazy. ;)

Will break NULL check and print "failed to process tracing data" as we
set it to NULL if something broken.

>
>
>>  		return -1;
>>  	}
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index 7cb24635adf2..129362b97ca5 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
>> @@ -293,7 +293,10 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>>  	int show_version = 0;
>>  	int show_funcs = 0;
>>  	int show_printk = 0;
>> -	ssize_t size;
>> +	ssize_t size = -1;
>> +	struct pevent *pevent;
>> +
>> +	*ppevent = NULL;
>>  
>>  	calc_data_size = 1;
>>  	repipe = __repipe;
>> @@ -317,34 +320,38 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>>  	file_bigendian = buf[0];
>>  	host_bigendian = bigendian();
>>  
>> -	*ppevent = read_trace_init(file_bigendian, host_bigendian);
>> -	if (*ppevent == NULL)
>> -		die("read_trace_init failed");
>> +	pevent = read_trace_init(file_bigendian, host_bigendian);
>> +	if (pevent == NULL) {
>
> Shouldn't we still set *ppevent to NULL? Or will the user now need to
> make sure that its pevent is NULL and always check for error?

I thought it's impossible to check return value for error since we don't
know how large a tracing data is, so I decided to init *ppevent to NULL
at the beginning and set it to a valid pevent right before return.  Thus
user need to check NULL before using it.

In the above case of perf_event__process_tracing_data(), it's a pipe
mode and we can know its size since it's saved in a temp file.

Thanks,
Namhyung

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Namhyung Kim]

On Tue, 19 Mar 2013 10:50:02 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>>  	free(version);
>> @@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>>  
>>  	page_size = read4(pevent);
>>  
>> -	read_header_files(pevent);
>> -	read_ftrace_files(pevent);
>> -	read_event_files(pevent);
>> -	read_proc_kallsyms(pevent);
>> -	read_ftrace_printk(pevent);
>> +	if (read_header_files(pevent) ||
>> +	    read_ftrace_files(pevent) ||
>> +	    read_event_files(pevent) ||
>> +	    read_proc_kallsyms(pevent) ||
>> +	    read_ftrace_printk(pevent))
>> +		goto out;
>
> I think I like the err += func() and check for err < 0, better.

Okay, I'll change them to err |= func() style if you're fine as Peter
suggested.

Thanks,
Namhyung

Re: [PATCH 7/9] perf util: Get rid of read_or_die() in trace-event-read.c

[Namhyung Kim]

On Tue, 19 Mar 2013 10:54:27 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Rename it to do_read and original do_read to __do_read, and check
>> their return value.
>> 
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/util/trace-event-read.c | 80 +++++++++++++++++++++++++++-----------
>>  1 file changed, 57 insertions(+), 23 deletions(-)
>> 
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index 62dd2168f4f5..87f0ccd54cdc 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
>> @@ -48,7 +48,7 @@ static int long_size;
>>  static ssize_t calc_data_size;
>>  static bool repipe;
>>  
>> -static int do_read(int fd, void *buf, int size)
>> +static int __do_read(int fd, void *buf, int size)
>>  {
>>  	int rsize = size;
>>  
>> @@ -61,8 +61,10 @@ static int do_read(int fd, void *buf, int size)
>>  		if (repipe) {
>>  			int retw = write(STDOUT_FILENO, buf, ret);
>>  
>> -			if (retw <= 0 || retw != ret)
>> -				die("repiping input file");
>> +			if (retw <= 0 || retw != ret) {
>> +				pr_debug("repiping input file");
>
> Again, why debug and not err?

Well, there's a pr_err() at the caller of top-level trace_report() in
case of error.  So if we use pr_err() there'll be multiple error message
for one failure and I don't think it's so helpful to normal users.  If
one really wants to know what happens inside, she will set -v to see
this low-level debug message.

Does that make sense?

>
>> +				return -1;
>> +			}
>>  		}
>>  
>>  		size -= ret;
>> @@ -72,14 +74,16 @@ static int do_read(int fd, void *buf, int size)
>>  	return rsize;
>>  }
>>  
>> -static int read_or_die(void *data, int size)
>> +static int do_read(void *data, int size)
>>  {
>>  	int r;
>>  
>> -	r = do_read(input_fd, data, size);
>> -	if (r <= 0)
>> -		die("reading input file (size expected=%d received=%d)",
>> -		    size, r);
>> +	r = __do_read(input_fd, data, size);
>> +	if (r <= 0) {
>> +		pr_debug("reading input file (size expected=%d received=%d)",
>> +			 size, r);
>> +		return -1;
>> +	}
>>  
>>  	if (calc_data_size)
>>  		calc_data_size += r;
>> @@ -95,7 +99,7 @@ static void skip(int size)
>>  
>>  	while (size) {
>>  		r = size > BUFSIZ ? BUFSIZ : size;
>> -		read_or_die(buf, r);
>> +		do_read(buf, r);
>
> Shouldn't this check the result of do_read()?

I was not so sure about this, but I skipped the check since all it does
is to "skip" and comment said "If it fails, the next read will report
it". :-)

Thanks,
Namhyung

>
>>  		size -= r;
>>  	};
>>  }
>
> -- Steve

Re: [PATCH 8/9] perf util: Get rid of die() calls in trace-data-read.c

[Namhyung Kim]

On Tue, 19 Mar 2013 10:55:28 -0400, Steven Rostedt wrote:
> On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Convert them to pr_debug() and propagate error code.
>
> Shouldn't they be pr_err(). I mean, if the old code would kill the
> process, why just keep it as a debug output?

Please see my other reply.

Arnaldo, can you give me your direction/preference?

Thanks,
Namhyung

Re: [PATCH 5/9] perf util: Handle failure case in trace_report()

[Steven Rostedt]

On Wed, 2013-03-20 at 10:12 +0900, Namhyung Kim wrote:

> >> +++ b/tools/perf/util/trace-event-read.c
> >> @@ -293,7 +293,10 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
> >>  	int show_version = 0;
> >>  	int show_funcs = 0;
> >>  	int show_printk = 0;
> >> -	ssize_t size;
> >> +	ssize_t size = -1;
> >> +	struct pevent *pevent;
> >> +
> >> +	*ppevent = NULL;
> >>  
> >>  	calc_data_size = 1;
> >>  	repipe = __repipe;
> >> @@ -317,34 +320,38 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
> >>  	file_bigendian = buf[0];
> >>  	host_bigendian = bigendian();
> >>  
> >> -	*ppevent = read_trace_init(file_bigendian, host_bigendian);
> >> -	if (*ppevent == NULL)
> >> -		die("read_trace_init failed");
> >> +	pevent = read_trace_init(file_bigendian, host_bigendian);
> >> +	if (pevent == NULL) {
> >
> > Shouldn't we still set *ppevent to NULL? Or will the user now need to
> > make sure that its pevent is NULL and always check for error?
> 
> I thought it's impossible to check return value for error since we don't
> know how large a tracing data is, so I decided to init *ppevent to NULL
> at the beginning and set it to a valid pevent right before return.  Thus
> user need to check NULL before using it.
> 
> In the above case of perf_event__process_tracing_data(), it's a pipe
> mode and we can know its size since it's saved in a temp file.
> 

My fault, I missed the addition of *ppevent = NULL at the beginning.
Ignore this comment.

-- Steve

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Steven Rostedt]

On Wed, 2013-03-20 at 10:14 +0900, Namhyung Kim wrote:
> On Tue, 19 Mar 2013 10:50:02 -0400, Steven Rostedt wrote:
> > On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> >>  	free(version);
> >> @@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
> >>  
> >>  	page_size = read4(pevent);
> >>  
> >> -	read_header_files(pevent);
> >> -	read_ftrace_files(pevent);
> >> -	read_event_files(pevent);
> >> -	read_proc_kallsyms(pevent);
> >> -	read_ftrace_printk(pevent);
> >> +	if (read_header_files(pevent) ||
> >> +	    read_ftrace_files(pevent) ||
> >> +	    read_event_files(pevent) ||
> >> +	    read_proc_kallsyms(pevent) ||
> >> +	    read_ftrace_printk(pevent))
> >> +		goto out;
> >
> > I think I like the err += func() and check for err < 0, better.
> 
> Okay, I'll change them to err |= func() style if you're fine as Peter
> suggested.

+= or |= I'm not picky ;-)

-- Steve

Re: [PATCH 7/9] perf util: Get rid of read_or_die() in trace-event-read.c

[Steven Rostedt]

On Wed, 2013-03-20 at 10:24 +0900, Namhyung Kim wrote:

> >> @@ -61,8 +61,10 @@ static int do_read(int fd, void *buf, int size)
> >>  		if (repipe) {
> >>  			int retw = write(STDOUT_FILENO, buf, ret);
> >>  
> >> -			if (retw <= 0 || retw != ret)
> >> -				die("repiping input file");
> >> +			if (retw <= 0 || retw != ret) {
> >> +				pr_debug("repiping input file");
> >
> > Again, why debug and not err?
> 
> Well, there's a pr_err() at the caller of top-level trace_report() in
> case of error.  So if we use pr_err() there'll be multiple error message
> for one failure and I don't think it's so helpful to normal users.  If
> one really wants to know what happens inside, she will set -v to see
> this low-level debug message.
> 
> Does that make sense?
> 

I haven't looked at the context of all the changes as to where they are
called from. I'm fine if we have a methodology of having pr_err() at the
top level and pr_debug() within the nested code. It looked to me that
the choices were somewhat random, but then again, I was missing context
to the code.

As long as a pr_err() that gives the user enough information to know
what went wrong is displayed, I'm fine with other errors using
pr_debug().

-- Steve

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Namhyung Kim]

On Tue, 19 Mar 2013 21:55:02 -0400, Steven Rostedt wrote:
> On Wed, 2013-03-20 at 10:14 +0900, Namhyung Kim wrote:
>> On Tue, 19 Mar 2013 10:50:02 -0400, Steven Rostedt wrote:
>> > On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
>> >>  	free(version);
>> >> @@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
>> >>  
>> >>  	page_size = read4(pevent);
>> >>  
>> >> -	read_header_files(pevent);
>> >> -	read_ftrace_files(pevent);
>> >> -	read_event_files(pevent);
>> >> -	read_proc_kallsyms(pevent);
>> >> -	read_ftrace_printk(pevent);
>> >> +	if (read_header_files(pevent) ||
>> >> +	    read_ftrace_files(pevent) ||
>> >> +	    read_event_files(pevent) ||
>> >> +	    read_proc_kallsyms(pevent) ||
>> >> +	    read_ftrace_printk(pevent))
>> >> +		goto out;
>> >
>> > I think I like the err += func() and check for err < 0, better.
>> 
>> Okay, I'll change them to err |= func() style if you're fine as Peter
>> suggested.
>
> += or |= I'm not picky ;-)

Ah, one thing I also care was the short-circuit logic.  I think we don't
need to call later functions if one fails, do we?

Thanks,
Namhyung

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Steven Rostedt]

On Wed, 2013-03-20 at 12:00 +0900, Namhyung Kim wrote:
> On Tue, 19 Mar 2013 21:55:02 -0400, Steven Rostedt wrote:
> > On Wed, 2013-03-20 at 10:14 +0900, Namhyung Kim wrote:
> >> On Tue, 19 Mar 2013 10:50:02 -0400, Steven Rostedt wrote:
> >> > On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> >> >>  	free(version);
> >> >> @@ -331,11 +354,12 @@ ssize_t trace_report(int fd, struct pevent **ppevent, bool __repipe)
> >> >>  
> >> >>  	page_size = read4(pevent);
> >> >>  
> >> >> -	read_header_files(pevent);
> >> >> -	read_ftrace_files(pevent);
> >> >> -	read_event_files(pevent);
> >> >> -	read_proc_kallsyms(pevent);
> >> >> -	read_ftrace_printk(pevent);
> >> >> +	if (read_header_files(pevent) ||
> >> >> +	    read_ftrace_files(pevent) ||
> >> >> +	    read_event_files(pevent) ||
> >> >> +	    read_proc_kallsyms(pevent) ||
> >> >> +	    read_ftrace_printk(pevent))
> >> >> +		goto out;
> >> >
> >> > I think I like the err += func() and check for err < 0, better.
> >> 
> >> Okay, I'll change them to err |= func() style if you're fine as Peter
> >> suggested.
> >
> > += or |= I'm not picky ;-)
> 
> Ah, one thing I also care was the short-circuit logic.  I think we don't
> need to call later functions if one fails, do we?

Yeah, good point. It still looks ugly, but it does make sense.

-- Steve

Re: [PATCH 6/9] perf util: Get rid of malloc_or_die() in trace-event-read.c

[Arnaldo Carvalho de Melo]

Em Tue, Mar 19, 2013 at 11:13:25PM -0400, Steven Rostedt escreveu:
> On Wed, 2013-03-20 at 12:00 +0900, Namhyung Kim wrote:
> > On Tue, 19 Mar 2013 21:55:02 -0400, Steven Rostedt wrote:
> > > On Wed, 2013-03-20 at 10:14 +0900, Namhyung Kim wrote:
> > >> On Tue, 19 Mar 2013 10:50:02 -0400, Steven Rostedt wrote:
> > >> > On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> > >> > I think I like the err += func() and check for err < 0, better.

> > >> Okay, I'll change them to err |= func() style if you're fine as Peter
> > >> suggested.

> > > += or |= I'm not picky ;-)

> > Ah, one thing I also care was the short-circuit logic.  I think we don't
> > need to call later functions if one fails, do we?

> Yeah, good point. It still looks ugly, but it does make sense.

Yes, I dislike all this += or |=, it should be normal exception
handling, just like everywhere in the kernel codebase:

	err = foo();
	if (err)
		goto out_err;

	err = bar();
	if (err)
		goto out_foo;

	err = baz();
	if (err)
		goto out_bar;

	err = new_foo();
	if (err)
		goto out_baz;

	return 0;
out_baz:
	baz_cleanup();
out_bar:
	bar_cleanup();
out_foo:
	foo_cleanup();
out_err:
	return err;

----

	That way exception handling code lies at the end of the
function, i.e. in source and binary code it has a lower chance of
polluting brain and CPU caches, and we don't need to call N functions
if we'll bail out when the one of them fails.

	I.e. nothing new here, just follow kernel coding style, move
along :-)

- Arnaldo

Re: [PATCH 8/9] perf util: Get rid of die() calls in trace-data-read.c

[Arnaldo Carvalho de Melo]

Em Wed, Mar 20, 2013 at 10:25:52AM +0900, Namhyung Kim escreveu:
> On Tue, 19 Mar 2013 10:55:28 -0400, Steven Rostedt wrote:
> > On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> >> Convert them to pr_debug() and propagate error code.

> > Shouldn't they be pr_err(). I mean, if the old code would kill the
> > process, why just keep it as a debug output?

> Please see my other reply.

Ditto.
 
> Arnaldo, can you give me your direction/preference?

Yeah, I think that lower levels should emit a debug if it helps
developers and advanced users to remedy or at least understand the
situation.

The builtin-foo.c top level and the [TG]UI routines are the ones that
must emit messages to the user.

- Arnaldo