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
From: Namhyung Kim <[email protected]>
Check return value of write and fail if error.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
Convert them to pr_debug() and propagate error code.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
Check return value of malloc and fail if NULL.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
Rename it to do_read and original do_read to __do_read, and check
their return value.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
So that it can be used by other places.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
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 <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
From: Namhyung Kim <[email protected]>
Now remove all remaining die() calls and convert them to check return
value and propagate it.
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <[email protected]>
>
> So that it can be used by other places.
>
> Cc: Steven Rostedt <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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 */
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;
> }
>
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;
> }
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;
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.
On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <[email protected]>
>
> Rename it to do_read and original do_read to __do_read, and check
> their return value.
>
> Cc: Steven Rostedt <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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
On Tue, 2013-03-19 at 17:53 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <[email protected]>
>
> 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 <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
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
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 :-)
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 <[email protected]>
>>
>> So that it can be used by other places.
>>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: Frederic Weisbecker <[email protected]>
>> Signed-off-by: Namhyung Kim <[email protected]>
>> ---
>> 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
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
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
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
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 <[email protected]>
>>
>> Rename it to do_read and original do_read to __do_read, and check
>> their return value.
>>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: Frederic Weisbecker <[email protected]>
>> Signed-off-by: Namhyung Kim <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>
>> 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
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
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
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
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
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
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
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