[PATCH perf/core v2 0/8] perf tools: Update strbuf to remove xrealloc

[PATCH perf/core v2 0/8] perf tools: Update strbuf to remove xrealloc

[Masami Hiramatsu]

Hi Arnaldo, 

Here is the 2nd version of the strbuf update, and just rebased on
the latest perf/core :) In order to avoid confusion, I've added
v2 on tag.

This patch series does refactoring strbuf and xrealloc related code
to remove xrealloc since it can call die() to exit immediately when
it hits any error. Instead of that, it should return error code to
the caller so that the caller can handle its error.

Thus, at first, this changes the strbuf APIs to return error code
instead of die() immediately. And then changing API callers according
to the following rules.
 - Check the return value of strbuf APIs and handle errors and,
   - If the caller returns an error code (errno), it returns
     the return value of strbuf APIs.
   - If the caller just return -1 or NULl in error case, it also
     returns -1 or NULL in case of strbuf error. 
   - If the caller can call die() directly, it also call die()
     in case of strbuf error.
 - Error checking patches are splitted for each subcommand, since
   it will help review.

This also removes xrealloc and ALLOC_GROW from libperf, so that no
one use it anymore.

Thank you,

---

Masami Hiramatsu (8):
      perf: Rewrite strbuf not to die
      perf probe: Check the return value of strbuf APIs
      perf help: Make check_emacsclient_version to check strbuf APIs
      perf: Make alias handler to check return value of strbuf
      perf header: Make topology checkers to check return value of strbuf
      perf pmu: Make pmu_formats_string to check return value of strbuf
      perf help: Do not use ALLOC_GROW in add_cmd_list
      perf tools: Remove xrealloc and ALLOC_GROW


 tools/perf/builtin-help.c          |   18 ++--
 tools/perf/perf.c                  |    8 +-
 tools/perf/util/Build              |    1 
 tools/perf/util/cache.h            |   19 -----
 tools/perf/util/dwarf-aux.c        |   50 +++++-------
 tools/perf/util/header.c           |   31 +++++---
 tools/perf/util/help-unknown-cmd.c |   30 +++++--
 tools/perf/util/pmu.c              |   10 +-
 tools/perf/util/probe-event.c      |  146 +++++++++++++++++++++---------------
 tools/perf/util/probe-finder.c     |   30 ++++---
 tools/perf/util/quote.c            |   36 +++++----
 tools/perf/util/quote.h            |    2 
 tools/perf/util/strbuf.c           |   93 +++++++++++++++++------
 tools/perf/util/strbuf.h           |   25 ++++--
 tools/perf/util/util.h             |    6 -
 tools/perf/util/wrapper.c          |   29 -------
 16 files changed, 291 insertions(+), 243 deletions(-)
 delete mode 100644 tools/perf/util/wrapper.c

--
Masami Hiramatsu

[PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die

[Masami Hiramatsu]

Rewrite strbuf implementation not to use die() nor xrealloc().
Instead of die, now most of the API returns error code or 0 if
succeeded.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/strbuf.c |   93 +++++++++++++++++++++++++++++++++-------------
 tools/perf/util/strbuf.h |   25 +++++++-----
 2 files changed, 82 insertions(+), 36 deletions(-)

diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
index 8fb7329..a98bb60 100644
--- a/tools/perf/util/strbuf.c
+++ b/tools/perf/util/strbuf.c
@@ -1,3 +1,4 @@
+#include "debug.h"
 #include "cache.h"
 #include <linux/kernel.h>
 
@@ -17,12 +18,13 @@ int prefixcmp(const char *str, const char *prefix)
  */
 char strbuf_slopbuf[1];
 
-void strbuf_init(struct strbuf *sb, ssize_t hint)
+int strbuf_init(struct strbuf *sb, ssize_t hint)
 {
 	sb->alloc = sb->len = 0;
 	sb->buf = strbuf_slopbuf;
 	if (hint)
-		strbuf_grow(sb, hint);
+		return strbuf_grow(sb, hint);
+	return 0;
 }
 
 void strbuf_release(struct strbuf *sb)
@@ -42,67 +44,104 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
 	return res;
 }
 
-void strbuf_grow(struct strbuf *sb, size_t extra)
+int strbuf_grow(struct strbuf *sb, size_t extra)
 {
-	if (sb->len + extra + 1 <= sb->len)
-		die("you want to use way too much memory");
-	if (!sb->alloc)
-		sb->buf = NULL;
-	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
+	char *buf;
+	size_t nr = sb->len + extra + 1;
+
+	if (nr < sb->alloc)
+		return 0;
+
+	if (nr <= sb->len)
+		return -E2BIG;
+
+	if (alloc_nr(sb->alloc) > nr)
+		nr = alloc_nr(sb->alloc);
+
+	buf = malloc(nr * sizeof(*buf));
+	if (!buf)
+		return -ENOMEM;
+
+	if (sb->alloc) {
+		memcpy(buf, sb->buf, sb->alloc);
+		free(sb->buf);
+	}
+	sb->buf = buf;
+	sb->alloc = nr;
+	return 0;
 }
 
-void strbuf_addch(struct strbuf *sb, int c)
+int strbuf_addch(struct strbuf *sb, int c)
 {
-	strbuf_grow(sb, 1);
+	int ret = strbuf_grow(sb, 1);
+	if (ret)
+		return ret;
+
 	sb->buf[sb->len++] = c;
 	sb->buf[sb->len] = '\0';
+	return 0;
 }
 
-void strbuf_add(struct strbuf *sb, const void *data, size_t len)
+int strbuf_add(struct strbuf *sb, const void *data, size_t len)
 {
-	strbuf_grow(sb, len);
+	int ret = strbuf_grow(sb, len);
+	if (ret)
+		return ret;
+
 	memcpy(sb->buf + sb->len, data, len);
-	strbuf_setlen(sb, sb->len + len);
+	return strbuf_setlen(sb, sb->len + len);
 }
 
-static void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
+static int strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
 {
-	int len;
+	int len, ret;
 	va_list ap_saved;
 
-	if (!strbuf_avail(sb))
-		strbuf_grow(sb, 64);
+	if (!strbuf_avail(sb)) {
+		ret = strbuf_grow(sb, 64);
+		if (ret)
+			return ret;
+	}
 
 	va_copy(ap_saved, ap);
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
 	if (len < 0)
-		die("your vsnprintf is broken");
+		return len;
 	if (len > strbuf_avail(sb)) {
-		strbuf_grow(sb, len);
+		ret = strbuf_grow(sb, len);
+		if (ret)
+			return ret;
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap_saved);
 		va_end(ap_saved);
 		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your vsnprintf is broken");
+			pr_debug("this should not happen, your vsnprintf is broken");
+			return -EINVAL;
 		}
 	}
-	strbuf_setlen(sb, sb->len + len);
+	return strbuf_setlen(sb, sb->len + len);
 }
 
-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+int strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 {
 	va_list ap;
+	int ret;
 
 	va_start(ap, fmt);
-	strbuf_addv(sb, fmt, ap);
+	ret = strbuf_addv(sb, fmt, ap);
 	va_end(ap);
+	return ret;
 }
 
 ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
 {
 	size_t oldlen = sb->len;
 	size_t oldalloc = sb->alloc;
+	int ret;
+
+	ret = strbuf_grow(sb, hint ? hint : 8192);
+	if (ret)
+		return ret;
 
-	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
 		ssize_t cnt;
 
@@ -112,12 +151,14 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
 				strbuf_release(sb);
 			else
 				strbuf_setlen(sb, oldlen);
-			return -1;
+			return cnt;
 		}
 		if (!cnt)
 			break;
 		sb->len += cnt;
-		strbuf_grow(sb, 8192);
+		ret = strbuf_grow(sb, 8192);
+		if (ret)
+			return ret;
 	}
 
 	sb->buf[sb->len] = '\0';
diff --git a/tools/perf/util/strbuf.h b/tools/perf/util/strbuf.h
index ab9be0fb..54b4092 100644
--- a/tools/perf/util/strbuf.h
+++ b/tools/perf/util/strbuf.h
@@ -51,7 +51,7 @@ struct strbuf {
 #define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
 
 /*----- strbuf life cycle -----*/
-void strbuf_init(struct strbuf *buf, ssize_t hint);
+int strbuf_init(struct strbuf *buf, ssize_t hint);
 void strbuf_release(struct strbuf *buf);
 char *strbuf_detach(struct strbuf *buf, size_t *);
 
@@ -60,26 +60,31 @@ static inline ssize_t strbuf_avail(const struct strbuf *sb) {
 	return sb->alloc ? sb->alloc - sb->len - 1 : 0;
 }
 
-void strbuf_grow(struct strbuf *buf, size_t);
+int strbuf_grow(struct strbuf *buf, size_t);
 
-static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-	if (!sb->alloc)
-		strbuf_grow(sb, 0);
+static inline int strbuf_setlen(struct strbuf *sb, size_t len) {
+	int ret;
+	if (!sb->alloc) {
+		ret = strbuf_grow(sb, 0);
+		if (ret)
+			return ret;
+	}
 	assert(len < sb->alloc);
 	sb->len = len;
 	sb->buf[len] = '\0';
+	return 0;
 }
 
 /*----- add data in your buffer -----*/
-void strbuf_addch(struct strbuf *sb, int c);
+int strbuf_addch(struct strbuf *sb, int c);
 
-void strbuf_add(struct strbuf *buf, const void *, size_t);
-static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
-	strbuf_add(sb, s, strlen(s));
+int strbuf_add(struct strbuf *buf, const void *, size_t);
+static inline int strbuf_addstr(struct strbuf *sb, const char *s) {
+	return strbuf_add(sb, s, strlen(s));
 }
 
 __attribute__((format(printf,2,3)))
-void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+int strbuf_addf(struct strbuf *sb, const char *fmt, ...);
 
 /* XXX: if read fails, any partial read is undone */
 ssize_t strbuf_read(struct strbuf *, int fd, ssize_t hint);

[PATCH perf/core v2 2/8] perf probe: Check the return value of strbuf APIs

[Masami Hiramatsu]

Check the return value of strbuf APIs in perf-probe
related code, so that it can handle errors in strbuf.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
  Changes in V2:
   - Rebased on the latest perf/core.
---
 tools/perf/util/dwarf-aux.c    |   50 ++++++--------
 tools/perf/util/probe-event.c  |  146 ++++++++++++++++++++++++----------------
 tools/perf/util/probe-finder.c |   30 +++++---
 3 files changed, 126 insertions(+), 100 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index aea189b..8c8b4c1 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -915,8 +915,7 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
 		tmp = "*";
 	else if (tag == DW_TAG_subroutine_type) {
 		/* Function pointer */
-		strbuf_add(buf, "(function_type)", 15);
-		return 0;
+		return strbuf_add(buf, "(function_type)", 15);
 	} else {
 		if (!dwarf_diename(&type))
 			return -ENOENT;
@@ -927,14 +926,10 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
 		else if (tag == DW_TAG_enumeration_type)
 			tmp = "enum ";
 		/* Write a base name */
-		strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
-		return 0;
+		return strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
 	}
 	ret = die_get_typename(&type, buf);
-	if (ret == 0)
-		strbuf_addstr(buf, tmp);
-
-	return ret;
+	return ret ? ret : strbuf_addstr(buf, tmp);
 }
 
 /**
@@ -951,12 +946,10 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
 	ret = die_get_typename(vr_die, buf);
 	if (ret < 0) {
 		pr_debug("Failed to get type, make it unknown.\n");
-		strbuf_add(buf, " (unknown_type)", 14);
+		ret = strbuf_add(buf, " (unknown_type)", 14);
 	}
 
-	strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
-
-	return 0;
+	return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
 }
 
 #ifdef HAVE_DWARF_GETLOCATIONS
@@ -998,23 +991,23 @@ static int die_get_var_innermost_scope(Dwarf_Die *sp_die, Dwarf_Die *vr_die,
 		goto out;
 	}
 
-	while ((offset = dwarf_ranges(&scopes[1], offset, &base,
-				&start, &end)) > 0) {
+	while (!ret && (offset = dwarf_ranges(&scopes[1], offset, &base,
+					&start, &end)) > 0) {
 		start -= entry;
 		end -= entry;
 
 		if (first) {
-			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
-				name, start, end);
+			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
+					  name, start, end);
 			first = false;
 		} else {
-			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
-				start, end);
+			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
+					  start, end);
 		}
 	}
 
-	if (!first)
-		strbuf_add(buf, "]>", 2);
+	if (!ret && !first)
+		ret = strbuf_add(buf, "]>", 2);
 
 out:
 	free(scopes);
@@ -1054,9 +1047,8 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
 	if (dwarf_attr(vr_die, DW_AT_location, &attr) == NULL)
 		return -EINVAL;
 
-	while ((offset = dwarf_getlocations(
-				&attr, offset, &base,
-				&start, &end, &op, &nops)) > 0) {
+	while (!ret && (offset = dwarf_getlocations(&attr, offset, &base,
+					&start, &end, &op, &nops)) > 0) {
 		if (start == 0) {
 			/* Single Location Descriptions */
 			ret = die_get_var_innermost_scope(sp_die, vr_die, buf);
@@ -1067,17 +1059,17 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
 		start -= entry;
 		end -= entry;
 		if (first) {
-			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
-				name, start, end);
+			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
+					  name, start, end);
 			first = false;
 		} else {
-			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
-				start, end);
+			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
+					  start, end);
 		}
 	}
 
-	if (!first)
-		strbuf_add(buf, "]>", 2);
+	if (!ret && !first)
+		ret = strbuf_add(buf, "]>", 2);
 
 	return ret;
 }
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 85d82f4..ff9c5c1 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1677,28 +1677,36 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
 {
 	struct perf_probe_arg_field *field = pa->field;
 	struct strbuf buf;
-	char *ret;
+	char *ret = NULL;
+	int err;
+
+	if (strbuf_init(&buf, 64) < 0)
+		return NULL;
 
-	strbuf_init(&buf, 64);
 	if (pa->name && pa->var)
-		strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
+		err = strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
 	else
-		strbuf_addstr(&buf, pa->name ?: pa->var);
-
-	while (field) {
+		err = strbuf_addstr(&buf, pa->name ?: pa->var);
+	if (err)
+		goto out;
+	while (field && !err) {
 		if (field->name[0] == '[')
-			strbuf_addstr(&buf, field->name);
+			err = strbuf_addstr(&buf, field->name);
 		else
-			strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
-				    field->name);
+			err = strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
+					  field->name);
 		field = field->next;
 	}
+	if (err)
+		goto out;
 
 	if (pa->type)
-		strbuf_addf(&buf, ":%s", pa->type);
+		if (strbuf_addf(&buf, ":%s", pa->type) < 0)
+			goto out;
 
 	ret = strbuf_detach(&buf, NULL);
-
+out:
+	strbuf_release(&buf);
 	return ret;
 }
 
@@ -1706,18 +1714,23 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
 static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
 {
 	struct strbuf buf;
-	char *tmp;
-	int len;
+	char *tmp, *ret = NULL;
+	int len, err = 0;
+
+	if (strbuf_init(&buf, 64) < 0)
+		return NULL;
 
-	strbuf_init(&buf, 64);
 	if (pp->function) {
-		strbuf_addstr(&buf, pp->function);
+		if (strbuf_addstr(&buf, pp->function) < 0)
+			goto out;
 		if (pp->offset)
-			strbuf_addf(&buf, "+%lu", pp->offset);
+			err = strbuf_addf(&buf, "+%lu", pp->offset);
 		else if (pp->line)
-			strbuf_addf(&buf, ":%d", pp->line);
+			err = strbuf_addf(&buf, ":%d", pp->line);
 		else if (pp->retprobe)
-			strbuf_addstr(&buf, "%return");
+			err = strbuf_addstr(&buf, "%return");
+		if (err)
+			goto out;
 	}
 	if (pp->file) {
 		tmp = pp->file;
@@ -1726,12 +1739,15 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
 			tmp = strchr(pp->file + len - 30, '/');
 			tmp = tmp ? tmp + 1 : pp->file + len - 30;
 		}
-		strbuf_addf(&buf, "@%s", tmp);
-		if (!pp->function && pp->line)
-			strbuf_addf(&buf, ":%d", pp->line);
+		err = strbuf_addf(&buf, "@%s", tmp);
+		if (!err && !pp->function && pp->line)
+			err = strbuf_addf(&buf, ":%d", pp->line);
 	}
-
-	return strbuf_detach(&buf, NULL);
+	if (!err)
+		ret = strbuf_detach(&buf, NULL);
+out:
+	strbuf_release(&buf);
+	return ret;
 }
 
 #if 0
@@ -1762,28 +1778,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
 static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
 					    struct strbuf *buf, int depth)
 {
+	int err;
 	if (ref->next) {
 		depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
 							 depth + 1);
 		if (depth < 0)
-			goto out;
+			return depth;
 	}
-	strbuf_addf(buf, "%+ld(", ref->offset);
-out:
-	return depth;
+	err = strbuf_addf(buf, "%+ld(", ref->offset);
+	return (err < 0) ? err : depth;
 }
 
 static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
 				      struct strbuf *buf)
 {
 	struct probe_trace_arg_ref *ref = arg->ref;
-	int depth = 0;
+	int depth = 0, err;
 
 	/* Argument name or separator */
 	if (arg->name)
-		strbuf_addf(buf, " %s=", arg->name);
+		err = strbuf_addf(buf, " %s=", arg->name);
 	else
-		strbuf_addch(buf, ' ');
+		err = strbuf_addch(buf, ' ');
+	if (err)
+		return err;
 
 	/* Special case: @XXX */
 	if (arg->value[0] == '@' && arg->ref)
@@ -1798,18 +1816,19 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
 
 	/* Print argument value */
 	if (arg->value[0] == '@' && arg->ref)
-		strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
+		err = strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
 	else
-		strbuf_addstr(buf, arg->value);
+		err = strbuf_addstr(buf, arg->value);
 
 	/* Closing */
-	while (depth--)
-		strbuf_addch(buf, ')');
+	while (depth-- && !err)
+		err = strbuf_addch(buf, ')');
+
 	/* Print argument type */
-	if (arg->type)
-		strbuf_addf(buf, ":%s", arg->type);
+	if (arg->type && !err)
+		err = strbuf_addf(buf, ":%s", arg->type);
 
-	return 0;
+	return err;
 }
 
 char *synthesize_probe_trace_command(struct probe_trace_event *tev)
@@ -1817,15 +1836,18 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
 	struct probe_trace_point *tp = &tev->point;
 	struct strbuf buf;
 	char *ret = NULL;
-	int i;
+	int i, err;
 
 	/* Uprobes must have tp->module */
 	if (tev->uprobes && !tp->module)
 		return NULL;
 
-	strbuf_init(&buf, 32);
-	strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
-		    tev->group, tev->event);
+	if (strbuf_init(&buf, 32) < 0)
+		return NULL;
+
+	if (strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
+			tev->group, tev->event) < 0)
+		goto error;
 	/*
 	 * If tp->address == 0, then this point must be a
 	 * absolute address uprobe.
@@ -1839,14 +1861,16 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
 
 	/* Use the tp->address for uprobes */
 	if (tev->uprobes)
-		strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
+		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
 	else if (!strncmp(tp->symbol, "0x", 2))
 		/* Absolute address. See try_to_find_absolute_address() */
-		strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
-			    tp->module ? ":" : "", tp->address);
+		err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
+				  tp->module ? ":" : "", tp->address);
 	else
-		strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
-			    tp->module ? ":" : "", tp->symbol, tp->offset);
+		err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
+				tp->module ? ":" : "", tp->symbol, tp->offset);
+	if (err)
+		goto error;
 
 	for (i = 0; i < tev->nargs; i++)
 		if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
@@ -1960,14 +1984,15 @@ static int convert_to_perf_probe_event(struct probe_trace_event *tev,
 		if (tev->args[i].name)
 			pev->args[i].name = strdup(tev->args[i].name);
 		else {
-			strbuf_init(&buf, 32);
+			if ((ret = strbuf_init(&buf, 32)) < 0)
+				goto error;
 			ret = synthesize_probe_trace_arg(&tev->args[i], &buf);
 			pev->args[i].name = strbuf_detach(&buf, NULL);
 		}
 		if (pev->args[i].name == NULL && ret >= 0)
 			ret = -ENOMEM;
 	}
-
+error:
 	if (ret < 0)
 		clear_perf_probe_event(pev);
 
@@ -2140,37 +2165,40 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
 				     const char *module,
 				     struct strbuf *result)
 {
-	int i;
+	int i, ret;
 	char *buf;
 
 	if (asprintf(&buf, "%s:%s", group, event) < 0)
 		return -errno;
-	strbuf_addf(result, "  %-20s (on ", buf);
+	ret = strbuf_addf(result, "  %-20s (on ", buf);
 	free(buf);
+	if (ret)
+		return ret;
 
 	/* Synthesize only event probe point */
 	buf = synthesize_perf_probe_point(&pev->point);
 	if (!buf)
 		return -ENOMEM;
-	strbuf_addstr(result, buf);
+	ret = strbuf_addstr(result, buf);
 	free(buf);
 
-	if (module)
-		strbuf_addf(result, " in %s", module);
+	if (!ret && module)
+		ret = strbuf_addf(result, " in %s", module);
 
-	if (pev->nargs > 0) {
-		strbuf_add(result, " with", 5);
-		for (i = 0; i < pev->nargs; i++) {
+	if (!ret && pev->nargs > 0) {
+		ret = strbuf_add(result, " with", 5);
+		for (i = 0; !ret && i < pev->nargs; i++) {
 			buf = synthesize_perf_probe_arg(&pev->args[i]);
 			if (!buf)
 				return -ENOMEM;
-			strbuf_addf(result, " %s", buf);
+			ret = strbuf_addf(result, " %s", buf);
 			free(buf);
 		}
 	}
-	strbuf_addch(result, ')');
+	if (!ret)
+		ret = strbuf_addch(result, ')');
 
-	return 0;
+	return ret;
 }
 
 /* Show an event */
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 9f68875..1259839 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1294,6 +1294,7 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
 {
 	struct available_var_finder *af = data;
 	struct variable_list *vl;
+	struct strbuf buf = STRBUF_INIT;
 	int tag, ret;
 
 	vl = &af->vls[af->nvls - 1];
@@ -1307,25 +1308,26 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
 		if (ret == 0 || ret == -ERANGE) {
 			int ret2;
 			bool externs = !af->child;
-			struct strbuf buf;
 
-			strbuf_init(&buf, 64);
+			if (strbuf_init(&buf, 64) < 0)
+				goto error;
 
 			if (probe_conf.show_location_range) {
-				if (!externs) {
-					if (ret)
-						strbuf_add(&buf, "[INV]\t", 6);
-					else
-						strbuf_add(&buf, "[VAL]\t", 6);
-				} else
-					strbuf_add(&buf, "[EXT]\t", 6);
+				if (!externs)
+					ret2 = strbuf_add(&buf,
+						ret ? "[INV]\t" : "[VAL]\t", 6);
+				else
+					ret2 = strbuf_add(&buf, "[EXT]\t", 6);
+				if (ret2)
+					goto error;
 			}
 
 			ret2 = die_get_varname(die_mem, &buf);
 
 			if (!ret2 && probe_conf.show_location_range &&
 				!externs) {
-				strbuf_addch(&buf, '\t');
+				if (strbuf_addch(&buf, '\t') < 0)
+					goto error;
 				ret2 = die_get_var_range(&af->pf.sp_die,
 							die_mem, &buf);
 			}
@@ -1334,8 +1336,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
 			if (ret2 == 0) {
 				strlist__add(vl->vars,
 					strbuf_detach(&buf, NULL));
-			} else
-				strbuf_release(&buf);
+			}
+			strbuf_release(&buf);
 		}
 	}
 
@@ -1343,6 +1345,10 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
 		return DIE_FIND_CB_CONTINUE;
 	else
 		return DIE_FIND_CB_SIBLING;
+error:
+	strbuf_release(&buf);
+	pr_debug("Error in strbuf\n");
+	return DIE_FIND_CB_END;
 }
 
 /* Add a found vars into available variables list */

[PATCH perf/core v2 3/8] perf help: Make check_emacsclient_version to check strbuf APIs

[Masami Hiramatsu]

Make check_emacsclient_version() to check the return
value of strbuf APIs so that it can handle errors in
strbuf.
---
 tools/perf/builtin-help.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
index bc1de9b..f9830c9 100644
--- a/tools/perf/builtin-help.c
+++ b/tools/perf/builtin-help.c
@@ -61,6 +61,7 @@ static int check_emacsclient_version(void)
 	struct child_process ec_process;
 	const char *argv_ec[] = { "emacsclient", "--version", NULL };
 	int version;
+	int ret = -1;
 
 	/* emacsclient prints its version number on stderr */
 	memset(&ec_process, 0, sizeof(ec_process));
@@ -71,7 +72,10 @@ static int check_emacsclient_version(void)
 		fprintf(stderr, "Failed to start emacsclient.\n");
 		return -1;
 	}
-	strbuf_read(&buffer, ec_process.err, 20);
+	if (strbuf_read(&buffer, ec_process.err, 20) < 0) {
+		fprintf(stderr, "Failed to read emacsclient version\n");
+		goto out;
+	}
 	close(ec_process.err);
 
 	/*
@@ -82,8 +86,7 @@ static int check_emacsclient_version(void)
 
 	if (prefixcmp(buffer.buf, "emacsclient")) {
 		fprintf(stderr, "Failed to parse emacsclient version.\n");
-		strbuf_release(&buffer);
-		return -1;
+		goto out;
 	}
 
 	version = atoi(buffer.buf + strlen("emacsclient"));
@@ -92,12 +95,11 @@ static int check_emacsclient_version(void)
 		fprintf(stderr,
 			"emacsclient version '%d' too old (< 22).\n",
 			version);
-		strbuf_release(&buffer);
-		return -1;
-	}
-
+	} else
+		ret = 0;
+out:
 	strbuf_release(&buffer);
-	return 0;
+	return ret;
 }
 
 static void exec_woman_emacs(const char *path, const char *page)

[PATCH perf/core v2 4/8] perf: Make alias handler to check return value of strbuf

[Masami Hiramatsu]

Make alias handler and sq_quote_argv to check the return
value of strbuf APIs.
In sq_quote_argv() calls die(), but this fix handles strbuf
failure as a special case and returns to caller, since
the caller - handle_alias() also has to check the return
value of other strbuf APIs and those checks can be merged
to one if() statement.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/perf.c       |    8 +++++---
 tools/perf/util/quote.c |   36 ++++++++++++++++++++----------------
 tools/perf/util/quote.h |    2 +-
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 83ffe7c..7970008 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -309,9 +309,11 @@ static int handle_alias(int *argcp, const char ***argv)
 			if (*argcp > 1) {
 				struct strbuf buf;
 
-				strbuf_init(&buf, PATH_MAX);
-				strbuf_addstr(&buf, alias_string);
-				sq_quote_argv(&buf, (*argv) + 1, PATH_MAX);
+				if (strbuf_init(&buf, PATH_MAX) < 0 ||
+				    strbuf_addstr(&buf, alias_string) < 0 ||
+				    sq_quote_argv(&buf, (*argv) + 1,
+						  PATH_MAX) < 0)
+					die("Failed to allocate memory.");
 				free(alias_string);
 				alias_string = buf.buf;
 			}
diff --git a/tools/perf/util/quote.c b/tools/perf/util/quote.c
index 01f0324..c6d4ee2 100644
--- a/tools/perf/util/quote.c
+++ b/tools/perf/util/quote.c
@@ -17,38 +17,42 @@ static inline int need_bs_quote(char c)
 	return (c == '\'' || c == '!');
 }
 
-static void sq_quote_buf(struct strbuf *dst, const char *src)
+static int sq_quote_buf(struct strbuf *dst, const char *src)
 {
 	char *to_free = NULL;
+	int ret;
 
 	if (dst->buf == src)
 		to_free = strbuf_detach(dst, NULL);
 
-	strbuf_addch(dst, '\'');
-	while (*src) {
+	ret = strbuf_addch(dst, '\'');
+	while (!ret && *src) {
 		size_t len = strcspn(src, "'!");
-		strbuf_add(dst, src, len);
+		ret = strbuf_add(dst, src, len);
 		src += len;
-		while (need_bs_quote(*src)) {
-			strbuf_addstr(dst, "'\\");
-			strbuf_addch(dst, *src++);
-			strbuf_addch(dst, '\'');
-		}
+		while (!ret && need_bs_quote(*src))
+			ret = strbuf_addf(dst, "'\\%c\'", *src++);
 	}
-	strbuf_addch(dst, '\'');
+	if (!ret)
+		ret = strbuf_addch(dst, '\'');
 	free(to_free);
+
+	return ret;
 }
 
-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
+int sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
 {
-	int i;
+	int i, ret;
 
 	/* Copy into destination buffer. */
-	strbuf_grow(dst, 255);
-	for (i = 0; argv[i]; ++i) {
-		strbuf_addch(dst, ' ');
-		sq_quote_buf(dst, argv[i]);
+	ret = strbuf_grow(dst, 255);
+	for (i = 0; !ret && argv[i]; ++i) {
+		ret = strbuf_addch(dst, ' ');
+		if (ret)
+			break;
+		ret = sq_quote_buf(dst, argv[i]);
 		if (maxlen && dst->len > maxlen)
 			die("Too many or long arguments");
 	}
+	return ret;
 }
diff --git a/tools/perf/util/quote.h b/tools/perf/util/quote.h
index 3340c9c..e1ec191 100644
--- a/tools/perf/util/quote.h
+++ b/tools/perf/util/quote.h
@@ -24,6 +24,6 @@
  * sq_quote() in a real application.
  */
 
-void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
+int sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
 
 #endif /* __PERF_QUOTE_H */

[PATCH perf/core v2 5/8] perf header: Make topology checkers to check return value of strbuf

[Masami Hiramatsu]

Make topology checkers to check the return value of strbuf
APIs so that it can detect errors in it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/header.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 90680ec..5ab74e4 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1819,7 +1819,8 @@ static int process_cpu_topology(struct perf_file_section *section,
 
 	ph->env.nr_sibling_cores = nr;
 	size += sizeof(u32);
-	strbuf_init(&sb, 128);
+	if (strbuf_init(&sb, 128) < 0)
+		goto free_cpu;
 
 	for (i = 0; i < nr; i++) {
 		str = do_read_string(fd, ph);
@@ -1827,7 +1828,8 @@ static int process_cpu_topology(struct perf_file_section *section,
 			goto error;
 
 		/* include a NULL character at the end */
-		strbuf_add(&sb, str, strlen(str) + 1);
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+			goto error;
 		size += string_size(str);
 		free(str);
 	}
@@ -1849,7 +1851,8 @@ static int process_cpu_topology(struct perf_file_section *section,
 			goto error;
 
 		/* include a NULL character at the end */
-		strbuf_add(&sb, str, strlen(str) + 1);
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+			goto error;
 		size += string_size(str);
 		free(str);
 	}
@@ -1907,7 +1910,7 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 	u32 nr, node, i;
 	char *str;
 	uint64_t mem_total, mem_free;
-	struct strbuf sb;
+	struct strbuf sb = STRBUF_INIT;
 
 	/* nr nodes */
 	ret = readn(fd, &nr, sizeof(nr));
@@ -1918,7 +1921,8 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 		nr = bswap_32(nr);
 
 	ph->env.nr_numa_nodes = nr;
-	strbuf_init(&sb, 256);
+	if (strbuf_init(&sb, 256) < 0)
+		goto error;
 
 	for (i = 0; i < nr; i++) {
 		/* node number */
@@ -1940,15 +1944,17 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 			mem_free = bswap_64(mem_free);
 		}
 
-		strbuf_addf(&sb, "%u:%"PRIu64":%"PRIu64":",
-			    node, mem_total, mem_free);
+		if (strbuf_addf(&sb, "%u:%"PRIu64":%"PRIu64":",
+				node, mem_total, mem_free) < 0)
+			goto error;
 
 		str = do_read_string(fd, ph);
 		if (!str)
 			goto error;
 
 		/* include a NULL character at the end */
-		strbuf_add(&sb, str, strlen(str) + 1);
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+			goto error;
 		free(str);
 	}
 	ph->env.numa_nodes = strbuf_detach(&sb, NULL);
@@ -1982,7 +1988,8 @@ static int process_pmu_mappings(struct perf_file_section *section __maybe_unused
 	}
 
 	ph->env.nr_pmu_mappings = pmu_num;
-	strbuf_init(&sb, 128);
+	if (strbuf_init(&sb, 128) < 0)
+		return -1;
 
 	while (pmu_num) {
 		if (readn(fd, &type, sizeof(type)) != sizeof(type))
@@ -1994,9 +2001,11 @@ static int process_pmu_mappings(struct perf_file_section *section __maybe_unused
 		if (!name)
 			goto error;
 
-		strbuf_addf(&sb, "%u:%s", type, name);
+		if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
+			goto error;
 		/* include a NULL character at the end */
-		strbuf_add(&sb, "", 1);
+		if (strbuf_add(&sb, "", 1) < 0)
+			goto error;
 
 		if (!strcmp(name, "msr"))
 			ph->env.msr_pmu_type = type;

[PATCH perf/core v2 6/8] perf pmu: Make pmu_formats_string to check return value of strbuf

[Masami Hiramatsu]

Make pmu_formats_string() to check return value of
strbuf APIs so that it can detect errors in it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/pmu.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index bf34468..ddb0261 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -643,20 +643,20 @@ static int pmu_resolve_param_term(struct parse_events_term *term,
 static char *pmu_formats_string(struct list_head *formats)
 {
 	struct perf_pmu_format *format;
-	char *str;
-	struct strbuf buf;
+	char *str = NULL;
+	struct strbuf buf = STRBUF_INIT;
 	unsigned i = 0;
 
 	if (!formats)
 		return NULL;
 
-	strbuf_init(&buf, 0);
 	/* sysfs exported terms */
 	list_for_each_entry(format, formats, list)
-		strbuf_addf(&buf, i++ ? ",%s" : "%s",
-			    format->name);
+		if (strbuf_addf(&buf, i++ ? ",%s" : "%s", format->name) < 0)
+			goto error;
 
 	str = strbuf_detach(&buf, NULL);
+error:
 	strbuf_release(&buf);
 
 	return str;

[PATCH perf/core v2 7/8] perf help: Do not use ALLOC_GROW in add_cmd_list

[Masami Hiramatsu]

Replace ALLOC_GROW with normal realloc code in add_cmd_list()
so that it can handle errors directly.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/help-unknown-cmd.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/help-unknown-cmd.c b/tools/perf/util/help-unknown-cmd.c
index 43a98a4..d62ccae 100644
--- a/tools/perf/util/help-unknown-cmd.c
+++ b/tools/perf/util/help-unknown-cmd.c
@@ -27,16 +27,27 @@ static int levenshtein_compare(const void *p1, const void *p2)
 	return l1 != l2 ? l1 - l2 : strcmp(s1, s2);
 }
 
-static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
+static int add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 {
-	unsigned int i;
-
-	ALLOC_GROW(cmds->names, cmds->cnt + old->cnt, cmds->alloc);
-
+	unsigned int i, nr = cmds->cnt + old->cnt;
+	void *tmp;
+
+	if (nr > cmds->alloc) {
+		/* Choose bigger one to alloc */
+		if (alloc_nr(cmds->alloc) < nr)
+			cmds->alloc = nr;
+		else
+			cmds->alloc = alloc_nr(cmds->alloc);
+		tmp = realloc(cmds->names, cmds->alloc * sizeof(*cmds->names));
+		if (!tmp)
+			return -1;
+		cmds->names = tmp;
+	}
 	for (i = 0; i < old->cnt; i++)
 		cmds->names[cmds->cnt++] = old->names[i];
 	zfree(&old->names);
 	old->cnt = 0;
+	return 0;
 }
 
 const char *help_unknown_cmd(const char *cmd)
@@ -52,8 +63,11 @@ const char *help_unknown_cmd(const char *cmd)
 
 	load_command_list("perf-", &main_cmds, &other_cmds);
 
-	add_cmd_list(&main_cmds, &aliases);
-	add_cmd_list(&main_cmds, &other_cmds);
+	if (add_cmd_list(&main_cmds, &aliases) < 0 ||
+	    add_cmd_list(&main_cmds, &other_cmds) < 0) {
+		fprintf(stderr, "ERROR: Failed to allocate command list for unknown command.\n");
+		goto end;
+	}
 	qsort(main_cmds.names, main_cmds.cnt,
 	      sizeof(main_cmds.names), cmdname_compare);
 	uniq(&main_cmds);
@@ -99,6 +113,6 @@ const char *help_unknown_cmd(const char *cmd)
 		for (i = 0; i < n; i++)
 			fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
 	}
-
+end:
 	exit(1);
 }

[PATCH perf/core v2 8/8] perf tools: Remove xrealloc and ALLOC_GROW

[Masami Hiramatsu]

Remove unused xrealloc() and ALLOC_GROW() from libperf.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/Build     |    1 -
 tools/perf/util/cache.h   |   19 -------------------
 tools/perf/util/util.h    |    6 ------
 tools/perf/util/wrapper.c |   29 -----------------------------
 4 files changed, 55 deletions(-)
 delete mode 100644 tools/perf/util/wrapper.c

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 90229a8..b7768ea 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -27,7 +27,6 @@ libperf-y += strlist.o
 libperf-y += strfilter.o
 libperf-y += top.o
 libperf-y += usage.o
-libperf-y += wrapper.o
 libperf-y += dso.o
 libperf-y += symbol.o
 libperf-y += symbol_fprintf.o
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 1f5a93c..0d814bb 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -40,25 +40,6 @@ int split_cmdline(char *cmdline, const char ***argv);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
-/*
- * Realloc the buffer pointed at by variable 'x' so that it can hold
- * at least 'nr' entries; the number of entries currently allocated
- * is 'alloc', using the standard growing factor alloc_nr() macro.
- *
- * DO NOT USE any expression with side-effect for 'x' or 'alloc'.
- */
-#define ALLOC_GROW(x, nr, alloc) \
-	do { \
-		if ((nr) > alloc) { \
-			if (alloc_nr(alloc) < (nr)) \
-				alloc = (nr); \
-			else \
-				alloc = alloc_nr(alloc); \
-			x = xrealloc((x), alloc * sizeof(*(x))); \
-		} \
-	} while(0)
-
-
 static inline int is_absolute_path(const char *path)
 {
 	return path[0] == '/';
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 88f607a..7651633 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -160,12 +160,6 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
-/*
- * Wrappers:
- */
-void *xrealloc(void *ptr, size_t size) __attribute__((weak));
-
-
 static inline void *zalloc(size_t size)
 {
 	return calloc(1, size);
diff --git a/tools/perf/util/wrapper.c b/tools/perf/util/wrapper.c
deleted file mode 100644
index 5f1a07c..0000000
--- a/tools/perf/util/wrapper.c
+++ /dev/null
@@ -1,29 +0,0 @@
-/*
- * Various trivial helper wrappers around standard functions
- */
-#include "cache.h"
-
-/*
- * There's no pack memory to release - but stay close to the Git
- * version so wrap this away:
- */
-static inline void release_pack_memory(size_t size __maybe_unused,
-				       int flag __maybe_unused)
-{
-}
-
-void *xrealloc(void *ptr, size_t size)
-{
-	void *ret = realloc(ptr, size);
-	if (!ret && !size)
-		ret = realloc(ptr, 1);
-	if (!ret) {
-		release_pack_memory(size, -1);
-		ret = realloc(ptr, size);
-		if (!ret && !size)
-			ret = realloc(ptr, 1);
-		if (!ret)
-			die("Out of memory, realloc failed");
-	}
-	return ret;
-}

Re: [PATCH perf/core v2 0/8] perf tools: Update strbuf to remove xrealloc

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:09:41AM +0900, Masami Hiramatsu escreveu:
> Hi Arnaldo, 
> 
> Here is the 2nd version of the strbuf update, and just rebased on
> the latest perf/core :) In order to avoid confusion, I've added
> v2 on tag.
> 
> This patch series does refactoring strbuf and xrealloc related code
> to remove xrealloc since it can call die() to exit immediately when
> it hits any error. Instead of that, it should return error code to
> the caller so that the caller can handle its error.
> 
> Thus, at first, this changes the strbuf APIs to return error code
> instead of die() immediately. And then changing API callers according
> to the following rules.
>  - Check the return value of strbuf APIs and handle errors and,
>    - If the caller returns an error code (errno), it returns
>      the return value of strbuf APIs.
>    - If the caller just return -1 or NULl in error case, it also
>      returns -1 or NULL in case of strbuf error. 
>    - If the caller can call die() directly, it also call die()
>      in case of strbuf error.
>  - Error checking patches are splitted for each subcommand, since
>    it will help review.
> 
> This also removes xrealloc and ALLOC_GROW from libperf, so that no
> one use it anymore.

Thanks a lot, I'll try reviewing this this weekend,

- Arnaldo
 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (8):
>       perf: Rewrite strbuf not to die
>       perf probe: Check the return value of strbuf APIs
>       perf help: Make check_emacsclient_version to check strbuf APIs
>       perf: Make alias handler to check return value of strbuf
>       perf header: Make topology checkers to check return value of strbuf
>       perf pmu: Make pmu_formats_string to check return value of strbuf
>       perf help: Do not use ALLOC_GROW in add_cmd_list
>       perf tools: Remove xrealloc and ALLOC_GROW
> 
> 
>  tools/perf/builtin-help.c          |   18 ++--
>  tools/perf/perf.c                  |    8 +-
>  tools/perf/util/Build              |    1 
>  tools/perf/util/cache.h            |   19 -----
>  tools/perf/util/dwarf-aux.c        |   50 +++++-------
>  tools/perf/util/header.c           |   31 +++++---
>  tools/perf/util/help-unknown-cmd.c |   30 +++++--
>  tools/perf/util/pmu.c              |   10 +-
>  tools/perf/util/probe-event.c      |  146 +++++++++++++++++++++---------------
>  tools/perf/util/probe-finder.c     |   30 ++++---
>  tools/perf/util/quote.c            |   36 +++++----
>  tools/perf/util/quote.h            |    2 
>  tools/perf/util/strbuf.c           |   93 +++++++++++++++++------
>  tools/perf/util/strbuf.h           |   25 ++++--
>  tools/perf/util/util.h             |    6 -
>  tools/perf/util/wrapper.c          |   29 -------
>  16 files changed, 291 insertions(+), 243 deletions(-)
>  delete mode 100644 tools/perf/util/wrapper.c
> 
> --
> Masami Hiramatsu

Re: [PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:09:50AM +0900, Masami Hiramatsu escreveu:
> Rewrite strbuf implementation not to use die() nor xrealloc().
> Instead of die, now most of the API returns error code or 0 if
> succeeded.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/strbuf.c |   93 +++++++++++++++++++++++++++++++++-------------
>  tools/perf/util/strbuf.h |   25 +++++++-----
>  2 files changed, 82 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
> index 8fb7329..a98bb60 100644
> --- a/tools/perf/util/strbuf.c
> +++ b/tools/perf/util/strbuf.c
> @@ -1,3 +1,4 @@
> +#include "debug.h"
>  #include "cache.h"
>  #include <linux/kernel.h>
>  
> @@ -17,12 +18,13 @@ int prefixcmp(const char *str, const char *prefix)
>   */
>  char strbuf_slopbuf[1];
>  
> -void strbuf_init(struct strbuf *sb, ssize_t hint)
> +int strbuf_init(struct strbuf *sb, ssize_t hint)
>  {
>  	sb->alloc = sb->len = 0;
>  	sb->buf = strbuf_slopbuf;
>  	if (hint)
> -		strbuf_grow(sb, hint);
> +		return strbuf_grow(sb, hint);
> +	return 0;
>  }
>  
>  void strbuf_release(struct strbuf *sb)
> @@ -42,67 +44,104 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
>  	return res;
>  }
>  
> -void strbuf_grow(struct strbuf *sb, size_t extra)
> +int strbuf_grow(struct strbuf *sb, size_t extra)
>  {
> -	if (sb->len + extra + 1 <= sb->len)
> -		die("you want to use way too much memory");
> -	if (!sb->alloc)
> -		sb->buf = NULL;
> -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> +	char *buf;
> +	size_t nr = sb->len + extra + 1;
> +
> +	if (nr < sb->alloc)
> +		return 0;
> +
> +	if (nr <= sb->len)
> +		return -E2BIG;
> +
> +	if (alloc_nr(sb->alloc) > nr)
> +		nr = alloc_nr(sb->alloc);
> +
> +	buf = malloc(nr * sizeof(*buf));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (sb->alloc) {
> +		memcpy(buf, sb->buf, sb->alloc);
> +		free(sb->buf);
> +	}

Why not use realloc? I.e. as the old code did, the problem was not that,
was just the panic when the realloc operation fails, that you are
removing here.

I.e. the above would be:

	buf = realloc(sb->buf, nr * sizeof(*buf));
	if (!buf)
		return -ENOMEM;

> +	sb->buf = buf;
> +	sb->alloc = nr;
> +	return 0;

I.e. no need to do the memcpy() nor the free(), no?

>  ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
>  {
>  	size_t oldlen = sb->len;
>  	size_t oldalloc = sb->alloc;
> +	int ret;
> +
> +	ret = strbuf_grow(sb, hint ? hint : 8192);
> +	if (ret)
> +		return ret;
>  
> -	strbuf_grow(sb, hint ? hint : 8192);
>  	for (;;) {
>  		ssize_t cnt;
>  
> @@ -112,12 +151,14 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
>  				strbuf_release(sb);
>  			else
>  				strbuf_setlen(sb, oldlen);
> -			return -1;
> +			return cnt;

This is unrelated, no?

I.e. this _was_ already returning a failure code, but then you are
propagating the read() return, which may even be a good idea, haven't
thought about that, but is unrelated to what this patch is doing, please
put it in a separate patch if you think it is a good idea.

All the rest seems ok, going over the other patches now.

- Arnaldo

Re: [PATCH perf/core v2 2/8] perf probe: Check the return value of strbuf APIs

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:00AM +0900, Masami Hiramatsu escreveu:
> Check the return value of strbuf APIs in perf-probe
> related code, so that it can handle errors in strbuf.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>   Changes in V2:
>    - Rebased on the latest perf/core.
> ---
>  tools/perf/util/dwarf-aux.c    |   50 ++++++--------
>  tools/perf/util/probe-event.c  |  146 ++++++++++++++++++++++++----------------
>  tools/perf/util/probe-finder.c |   30 +++++---
>  3 files changed, 126 insertions(+), 100 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index aea189b..8c8b4c1 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -915,8 +915,7 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
>  		tmp = "*";
>  	else if (tag == DW_TAG_subroutine_type) {
>  		/* Function pointer */
> -		strbuf_add(buf, "(function_type)", 15);
> -		return 0;
> +		return strbuf_add(buf, "(function_type)", 15);
>  	} else {
>  		if (!dwarf_diename(&type))
>  			return -ENOENT;
> @@ -927,14 +926,10 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
>  		else if (tag == DW_TAG_enumeration_type)
>  			tmp = "enum ";
>  		/* Write a base name */
> -		strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> -		return 0;
> +		return strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
>  	}
>  	ret = die_get_typename(&type, buf);
> -	if (ret == 0)
> -		strbuf_addstr(buf, tmp);
> -
> -	return ret;
> +	return ret ? ret : strbuf_addstr(buf, tmp);
>  }
>  
>  /**
> @@ -951,12 +946,10 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
>  	ret = die_get_typename(vr_die, buf);
>  	if (ret < 0) {
>  		pr_debug("Failed to get type, make it unknown.\n");
> -		strbuf_add(buf, " (unknown_type)", 14);
> +		ret = strbuf_add(buf, " (unknown_type)", 14);
>  	}
>  
> -	strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> -
> -	return 0;
> +	return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
>  }
>  
>  #ifdef HAVE_DWARF_GETLOCATIONS
> @@ -998,23 +991,23 @@ static int die_get_var_innermost_scope(Dwarf_Die *sp_die, Dwarf_Die *vr_die,
>  		goto out;
>  	}
>  
> -	while ((offset = dwarf_ranges(&scopes[1], offset, &base,
> -				&start, &end)) > 0) {
> +	while (!ret && (offset = dwarf_ranges(&scopes[1], offset, &base,
> +					&start, &end)) > 0) {

Why not leave the above as is, i.e. no checking for ret that will always
fail the first time, and...

>  		start -= entry;
>  		end -= entry;
>  
>  		if (first) {
> -			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> -				name, start, end);
> +			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> +					  name, start, end);
>  			first = false;
>  		} else {
> -			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> -				start, end);
> +			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> +					  start, end);
>  		}

Add:

		if (ret)
			goto out;

Here?

>  	}
>  
> -	if (!first)
> -		strbuf_add(buf, "]>", 2);
> +	if (!ret && !first)
> +		ret = strbuf_add(buf, "]>", 2);

No need for the above hunk? ditto for the following set of hunks.

>  out:
>  	free(scopes);
> @@ -1054,9 +1047,8 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
>  	if (dwarf_attr(vr_die, DW_AT_location, &attr) == NULL)
>  		return -EINVAL;
>  
> -	while ((offset = dwarf_getlocations(
> -				&attr, offset, &base,
> -				&start, &end, &op, &nops)) > 0) {
> +	while (!ret && (offset = dwarf_getlocations(&attr, offset, &base,
> +					&start, &end, &op, &nops)) > 0) {
>  		if (start == 0) {
>  			/* Single Location Descriptions */
>  			ret = die_get_var_innermost_scope(sp_die, vr_die, buf);
> @@ -1067,17 +1059,17 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
>  		start -= entry;
>  		end -= entry;
>  		if (first) {
> -			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> -				name, start, end);
> +			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> +					  name, start, end);
>  			first = false;
>  		} else {
> -			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> -				start, end);
> +			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> +					  start, end);
>  		}
>  	}
>  
> -	if (!first)
> -		strbuf_add(buf, "]>", 2);
> +	if (!ret && !first)
> +		ret = strbuf_add(buf, "]>", 2);
>  
>  	return ret;
>  }
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 85d82f4..ff9c5c1 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1677,28 +1677,36 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
>  {
>  	struct perf_probe_arg_field *field = pa->field;
>  	struct strbuf buf;
> -	char *ret;
> +	char *ret = NULL;
> +	int err;
> +
> +	if (strbuf_init(&buf, 64) < 0)
> +		return NULL;
>  
> -	strbuf_init(&buf, 64);
>  	if (pa->name && pa->var)
> -		strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> +		err = strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
>  	else
> -		strbuf_addstr(&buf, pa->name ?: pa->var);
> -
> -	while (field) {
> +		err = strbuf_addstr(&buf, pa->name ?: pa->var);
> +	if (err)
> +		goto out;

Why test err twice? Leave the 'while (field)' and...

> +	while (field && !err) {
>  		if (field->name[0] == '[')
> -			strbuf_addstr(&buf, field->name);
> +			err = strbuf_addstr(&buf, field->name);
>  		else
> -			strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> -				    field->name);
> +			err = strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> +					  field->name);
>  		field = field->next;

		if (err)
			goto out;
>  	}

Move the following check + goto to inside the loop, as the last thing,
otherwise you'll be testing it twice for no reason.

> +	if (err)
> +		goto out;
>  
>  	if (pa->type)
> -		strbuf_addf(&buf, ":%s", pa->type);
> +		if (strbuf_addf(&buf, ":%s", pa->type) < 0)
> +			goto out;

The bove is wrong, it is not propagating the error, should be:


	if (pa->type) {
		ret = strbuf_addf(&buf, ":%s", pa->type);
		if (ret < 0)
			goto out;
	}

>  
>  	ret = strbuf_detach(&buf, NULL);
> -
> +out:
> +	strbuf_release(&buf);
>  	return ret;
>  }
>  
> @@ -1706,18 +1714,23 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
>  static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
>  {
>  	struct strbuf buf;
> -	char *tmp;
> -	int len;
> +	char *tmp, *ret = NULL;
> +	int len, err = 0;
> +
> +	if (strbuf_init(&buf, 64) < 0)
> +		return NULL;
>  
> -	strbuf_init(&buf, 64);
>  	if (pp->function) {
> -		strbuf_addstr(&buf, pp->function);
> +		if (strbuf_addstr(&buf, pp->function) < 0)
> +			goto out;
>  		if (pp->offset)
> -			strbuf_addf(&buf, "+%lu", pp->offset);
> +			err = strbuf_addf(&buf, "+%lu", pp->offset);
>  		else if (pp->line)
> -			strbuf_addf(&buf, ":%d", pp->line);
> +			err = strbuf_addf(&buf, ":%d", pp->line);
>  		else if (pp->retprobe)
> -			strbuf_addstr(&buf, "%return");
> +			err = strbuf_addstr(&buf, "%return");
> +		if (err)
> +			goto out;
>  	}
>  	if (pp->file) {
>  		tmp = pp->file;
> @@ -1726,12 +1739,15 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
>  			tmp = strchr(pp->file + len - 30, '/');
>  			tmp = tmp ? tmp + 1 : pp->file + len - 30;
>  		}
> -		strbuf_addf(&buf, "@%s", tmp);
> -		if (!pp->function && pp->line)
> -			strbuf_addf(&buf, ":%d", pp->line);
> +		err = strbuf_addf(&buf, "@%s", tmp);
> +		if (!err && !pp->function && pp->line)
> +			err = strbuf_addf(&buf, ":%d", pp->line);
>  	}
> -
> -	return strbuf_detach(&buf, NULL);
> +	if (!err)
> +		ret = strbuf_detach(&buf, NULL);
> +out:
> +	strbuf_release(&buf);
> +	return ret;
>  }
>  
>  #if 0
> @@ -1762,28 +1778,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
>  static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
>  					    struct strbuf *buf, int depth)
>  {
> +	int err;
>  	if (ref->next) {
>  		depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
>  							 depth + 1);
>  		if (depth < 0)
> -			goto out;
> +			return depth;
>  	}
> -	strbuf_addf(buf, "%+ld(", ref->offset);
> -out:
> -	return depth;
> +	err = strbuf_addf(buf, "%+ld(", ref->offset);
> +	return (err < 0) ? err : depth;
>  }

The above would be more compact as:

	return err ?: depth;

And would at a glance state that the return of addf() is 0 for Ok and -1
for error, which I checked and is the case, minor detail tho. :-)

>  
>  static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
>  				      struct strbuf *buf)
>  {
>  	struct probe_trace_arg_ref *ref = arg->ref;
> -	int depth = 0;
> +	int depth = 0, err;
>  
>  	/* Argument name or separator */
>  	if (arg->name)
> -		strbuf_addf(buf, " %s=", arg->name);
> +		err = strbuf_addf(buf, " %s=", arg->name);
>  	else
> -		strbuf_addch(buf, ' ');
> +		err = strbuf_addch(buf, ' ');
> +	if (err)
> +		return err;
>  
>  	/* Special case: @XXX */
>  	if (arg->value[0] == '@' && arg->ref)
> @@ -1798,18 +1816,19 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
>  
>  	/* Print argument value */
>  	if (arg->value[0] == '@' && arg->ref)
> -		strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> +		err = strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
>  	else
> -		strbuf_addstr(buf, arg->value);
> +		err = strbuf_addstr(buf, arg->value);

Missing check:

	if (err)
		return err;

Ahh, ok, caught in the following loop, but then there the !err would be
better as the first thing, as it checks what comes immediately before
it, i.e. no sense in decrementing depth if there was an error before
that loop. Confusing, but harmless.

>  
>  	/* Closing */
> -	while (depth--)
> -		strbuf_addch(buf, ')');
> +	while (depth-- && !err)
> +		err = strbuf_addch(buf, ')');
> +
>  	/* Print argument type */
> -	if (arg->type)
> -		strbuf_addf(buf, ":%s", arg->type);

One more, its more clear as:

	if (!err && arg->type)

But harmless...

> +	if (arg->type && !err)
> +		err = strbuf_addf(buf, ":%s", arg->type);
>  
> -	return 0;
> +	return err;
>  }
>  
>  char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> @@ -1817,15 +1836,18 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>  	struct probe_trace_point *tp = &tev->point;
>  	struct strbuf buf;
>  	char *ret = NULL;
> -	int i;
> +	int i, err;
>  
>  	/* Uprobes must have tp->module */
>  	if (tev->uprobes && !tp->module)
>  		return NULL;
>  
> -	strbuf_init(&buf, 32);
> -	strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> -		    tev->group, tev->event);
> +	if (strbuf_init(&buf, 32) < 0)
> +		return NULL;
> +
> +	if (strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> +			tev->group, tev->event) < 0)
> +		goto error;
>  	/*
>  	 * If tp->address == 0, then this point must be a
>  	 * absolute address uprobe.
> @@ -1839,14 +1861,16 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>  
>  	/* Use the tp->address for uprobes */
>  	if (tev->uprobes)
> -		strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> +		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
>  	else if (!strncmp(tp->symbol, "0x", 2))
>  		/* Absolute address. See try_to_find_absolute_address() */
> -		strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> -			    tp->module ? ":" : "", tp->address);
> +		err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> +				  tp->module ? ":" : "", tp->address);
>  	else
> -		strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> -			    tp->module ? ":" : "", tp->symbol, tp->offset);
> +		err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> +				tp->module ? ":" : "", tp->symbol, tp->offset);
> +	if (err)
> +		goto error;

Good! I thought you would do it as:

  	for (i = 0; !err && i < tev->nargs; i++)

;-) :-)

>  
>  	for (i = 0; i < tev->nargs; i++)
>  		if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
> @@ -1960,14 +1984,15 @@ static int convert_to_perf_probe_event(struct probe_trace_event *tev,
>  		if (tev->args[i].name)
>  			pev->args[i].name = strdup(tev->args[i].name);
>  		else {
> -			strbuf_init(&buf, 32);
> +			if ((ret = strbuf_init(&buf, 32)) < 0)
> +				goto error;
>  			ret = synthesize_probe_trace_arg(&tev->args[i], &buf);
>  			pev->args[i].name = strbuf_detach(&buf, NULL);
>  		}
>  		if (pev->args[i].name == NULL && ret >= 0)
>  			ret = -ENOMEM;
>  	}
> -
> +error:
>  	if (ret < 0)
>  		clear_perf_probe_event(pev);
>  
> @@ -2140,37 +2165,40 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
>  				     const char *module,
>  				     struct strbuf *result)
>  {
> -	int i;
> +	int i, ret;
>  	char *buf;
>  
>  	if (asprintf(&buf, "%s:%s", group, event) < 0)
>  		return -errno;
> -	strbuf_addf(result, "  %-20s (on ", buf);
> +	ret = strbuf_addf(result, "  %-20s (on ", buf);
>  	free(buf);
> +	if (ret)
> +		return ret;
>  
>  	/* Synthesize only event probe point */
>  	buf = synthesize_perf_probe_point(&pev->point);
>  	if (!buf)
>  		return -ENOMEM;
> -	strbuf_addstr(result, buf);
> +	ret = strbuf_addstr(result, buf);
>  	free(buf);
>  
> -	if (module)
> -		strbuf_addf(result, " in %s", module);

the next ones match what I suggested elsewhere: first check the result
of the previous step before reusing the existing if to do something,
as a way of saving source code lines, ok.

> +	if (!ret && module)
> +		ret = strbuf_addf(result, " in %s", module);
>  
> -	if (pev->nargs > 0) {
> -		strbuf_add(result, " with", 5);
> -		for (i = 0; i < pev->nargs; i++) {
> +	if (!ret && pev->nargs > 0) {
> +		ret = strbuf_add(result, " with", 5);
> +		for (i = 0; !ret && i < pev->nargs; i++) {
>  			buf = synthesize_perf_probe_arg(&pev->args[i]);
>  			if (!buf)
>  				return -ENOMEM;
> -			strbuf_addf(result, " %s", buf);
> +			ret = strbuf_addf(result, " %s", buf);
>  			free(buf);
>  		}
>  	}
> -	strbuf_addch(result, ')');
> +	if (!ret)
> +		ret = strbuf_addch(result, ')');
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* Show an event */
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 9f68875..1259839 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1294,6 +1294,7 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  {
>  	struct available_var_finder *af = data;
>  	struct variable_list *vl;
> +	struct strbuf buf = STRBUF_INIT;
>  	int tag, ret;
>  
>  	vl = &af->vls[af->nvls - 1];
> @@ -1307,25 +1308,26 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  		if (ret == 0 || ret == -ERANGE) {
>  			int ret2;
>  			bool externs = !af->child;
> -			struct strbuf buf;
>  
> -			strbuf_init(&buf, 64);
> +			if (strbuf_init(&buf, 64) < 0)
> +				goto error;
>  
>  			if (probe_conf.show_location_range) {
> -				if (!externs) {
> -					if (ret)
> -						strbuf_add(&buf, "[INV]\t", 6);
> -					else
> -						strbuf_add(&buf, "[VAL]\t", 6);
> -				} else
> -					strbuf_add(&buf, "[EXT]\t", 6);
> +				if (!externs)
> +					ret2 = strbuf_add(&buf,
> +						ret ? "[INV]\t" : "[VAL]\t", 6);
> +				else
> +					ret2 = strbuf_add(&buf, "[EXT]\t", 6);
> +				if (ret2)
> +					goto error;
>  			}
>  
>  			ret2 = die_get_varname(die_mem, &buf);
>  
>  			if (!ret2 && probe_conf.show_location_range &&
>  				!externs) {
> -				strbuf_addch(&buf, '\t');
> +				if (strbuf_addch(&buf, '\t') < 0)
> +					goto error;
>  				ret2 = die_get_var_range(&af->pf.sp_die,
>  							die_mem, &buf);
>  			}
> @@ -1334,8 +1336,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  			if (ret2 == 0) {
>  				strlist__add(vl->vars,
>  					strbuf_detach(&buf, NULL));
> -			} else
> -				strbuf_release(&buf);
> +			}
> +			strbuf_release(&buf);
>  		}
>  	}
>  
> @@ -1343,6 +1345,10 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
>  		return DIE_FIND_CB_CONTINUE;
>  	else
>  		return DIE_FIND_CB_SIBLING;
> +error:
> +	strbuf_release(&buf);
> +	pr_debug("Error in strbuf\n");
> +	return DIE_FIND_CB_END;
>  }
>  
>  /* Add a found vars into available variables list */

Re: [PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die

[Arnaldo Carvalho de Melo]

Em Thu, May 05, 2016 at 08:25:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, Apr 30, 2016 at 12:09:50AM +0900, Masami Hiramatsu escreveu:
> > @@ -112,12 +151,14 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
> >  				strbuf_release(sb);
> >  			else
> >  				strbuf_setlen(sb, oldlen);
> > -			return -1;
> > +			return cnt;
> 
> This is unrelated, no?
> 
> I.e. this _was_ already returning a failure code, but then you are
> propagating the read() return, which may even be a good idea, haven't
> thought about that, but is unrelated to what this patch is doing, please
> put it in a separate patch if you think it is a good idea.
> 
> All the rest seems ok, going over the other patches now.

Just a bit annoying, but this is done when checking if cnt < 0, which
can only be -1 as per read's man page, so its ok, nevermind...

- Arnaldo

Re: [PATCH perf/core v2 3/8] perf help: Make check_emacsclient_version to check strbuf APIs

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:14AM +0900, Masami Hiramatsu escreveu:
> Make check_emacsclient_version() to check the return
> value of strbuf APIs so that it can handle errors in
> strbuf.

Looks ok.

> ---
>  tools/perf/builtin-help.c |   18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
> index bc1de9b..f9830c9 100644
> --- a/tools/perf/builtin-help.c
> +++ b/tools/perf/builtin-help.c
> @@ -61,6 +61,7 @@ static int check_emacsclient_version(void)
>  	struct child_process ec_process;
>  	const char *argv_ec[] = { "emacsclient", "--version", NULL };
>  	int version;
> +	int ret = -1;
>  
>  	/* emacsclient prints its version number on stderr */
>  	memset(&ec_process, 0, sizeof(ec_process));
> @@ -71,7 +72,10 @@ static int check_emacsclient_version(void)
>  		fprintf(stderr, "Failed to start emacsclient.\n");
>  		return -1;
>  	}
> -	strbuf_read(&buffer, ec_process.err, 20);
> +	if (strbuf_read(&buffer, ec_process.err, 20) < 0) {
> +		fprintf(stderr, "Failed to read emacsclient version\n");
> +		goto out;
> +	}
>  	close(ec_process.err);
>  
>  	/*
> @@ -82,8 +86,7 @@ static int check_emacsclient_version(void)
>  
>  	if (prefixcmp(buffer.buf, "emacsclient")) {
>  		fprintf(stderr, "Failed to parse emacsclient version.\n");
> -		strbuf_release(&buffer);
> -		return -1;
> +		goto out;
>  	}
>  
>  	version = atoi(buffer.buf + strlen("emacsclient"));
> @@ -92,12 +95,11 @@ static int check_emacsclient_version(void)
>  		fprintf(stderr,
>  			"emacsclient version '%d' too old (< 22).\n",
>  			version);
> -		strbuf_release(&buffer);
> -		return -1;
> -	}
> -
> +	} else
> +		ret = 0;
> +out:
>  	strbuf_release(&buffer);
> -	return 0;
> +	return ret;
>  }
>  
>  static void exec_woman_emacs(const char *path, const char *page)

Re: [PATCH perf/core v2 4/8] perf: Make alias handler to check return value of strbuf

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:23AM +0900, Masami Hiramatsu escreveu:
> Make alias handler and sq_quote_argv to check the return
> value of strbuf APIs.
> In sq_quote_argv() calls die(), but this fix handles strbuf
> failure as a special case and returns to caller, since
> the caller - handle_alias() also has to check the return
> value of other strbuf APIs and those checks can be merged
> to one if() statement.

Fair enough, looks ok.
 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/perf.c       |    8 +++++---
>  tools/perf/util/quote.c |   36 ++++++++++++++++++++----------------
>  tools/perf/util/quote.h |    2 +-
>  3 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 83ffe7c..7970008 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -309,9 +309,11 @@ static int handle_alias(int *argcp, const char ***argv)
>  			if (*argcp > 1) {
>  				struct strbuf buf;
>  
> -				strbuf_init(&buf, PATH_MAX);
> -				strbuf_addstr(&buf, alias_string);
> -				sq_quote_argv(&buf, (*argv) + 1, PATH_MAX);
> +				if (strbuf_init(&buf, PATH_MAX) < 0 ||
> +				    strbuf_addstr(&buf, alias_string) < 0 ||
> +				    sq_quote_argv(&buf, (*argv) + 1,
> +						  PATH_MAX) < 0)
> +					die("Failed to allocate memory.");
>  				free(alias_string);
>  				alias_string = buf.buf;
>  			}
> diff --git a/tools/perf/util/quote.c b/tools/perf/util/quote.c
> index 01f0324..c6d4ee2 100644
> --- a/tools/perf/util/quote.c
> +++ b/tools/perf/util/quote.c
> @@ -17,38 +17,42 @@ static inline int need_bs_quote(char c)
>  	return (c == '\'' || c == '!');
>  }
>  
> -static void sq_quote_buf(struct strbuf *dst, const char *src)
> +static int sq_quote_buf(struct strbuf *dst, const char *src)
>  {
>  	char *to_free = NULL;
> +	int ret;
>  
>  	if (dst->buf == src)
>  		to_free = strbuf_detach(dst, NULL);
>  
> -	strbuf_addch(dst, '\'');
> -	while (*src) {
> +	ret = strbuf_addch(dst, '\'');
> +	while (!ret && *src) {
>  		size_t len = strcspn(src, "'!");
> -		strbuf_add(dst, src, len);
> +		ret = strbuf_add(dst, src, len);
>  		src += len;
> -		while (need_bs_quote(*src)) {
> -			strbuf_addstr(dst, "'\\");
> -			strbuf_addch(dst, *src++);
> -			strbuf_addch(dst, '\'');
> -		}
> +		while (!ret && need_bs_quote(*src))
> +			ret = strbuf_addf(dst, "'\\%c\'", *src++);
>  	}
> -	strbuf_addch(dst, '\'');
> +	if (!ret)
> +		ret = strbuf_addch(dst, '\'');
>  	free(to_free);
> +
> +	return ret;
>  }
>  
> -void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
> +int sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
>  {
> -	int i;
> +	int i, ret;
>  
>  	/* Copy into destination buffer. */
> -	strbuf_grow(dst, 255);
> -	for (i = 0; argv[i]; ++i) {
> -		strbuf_addch(dst, ' ');
> -		sq_quote_buf(dst, argv[i]);
> +	ret = strbuf_grow(dst, 255);
> +	for (i = 0; !ret && argv[i]; ++i) {
> +		ret = strbuf_addch(dst, ' ');
> +		if (ret)
> +			break;
> +		ret = sq_quote_buf(dst, argv[i]);
>  		if (maxlen && dst->len > maxlen)
>  			die("Too many or long arguments");
>  	}
> +	return ret;
>  }
> diff --git a/tools/perf/util/quote.h b/tools/perf/util/quote.h
> index 3340c9c..e1ec191 100644
> --- a/tools/perf/util/quote.h
> +++ b/tools/perf/util/quote.h
> @@ -24,6 +24,6 @@
>   * sq_quote() in a real application.
>   */
>  
> -void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
> +int sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
>  
>  #endif /* __PERF_QUOTE_H */

Re: [PATCH perf/core v2 5/8] perf header: Make topology checkers to check return value of strbuf

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:33AM +0900, Masami Hiramatsu escreveu:
> Make topology checkers to check the return value of strbuf
> APIs so that it can detect errors in it.

>  	}
> @@ -1907,7 +1910,7 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  	u32 nr, node, i;
>  	char *str;
>  	uint64_t mem_total, mem_free;
> -	struct strbuf sb;
> +	struct strbuf sb = STRBUF_INIT;

Since you're going to call strbuf_init() later, is the above really
needed?
  
>  	/* nr nodes */
>  	ret = readn(fd, &nr, sizeof(nr));
> @@ -1918,7 +1921,8 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  		nr = bswap_32(nr);
>  
>  	ph->env.nr_numa_nodes = nr;
> -	strbuf_init(&sb, 256);
> +	if (strbuf_init(&sb, 256) < 0)
> +		goto error;
>  

Re: [PATCH perf/core v2 6/8] perf pmu: Make pmu_formats_string to check return value of strbuf

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:42AM +0900, Masami Hiramatsu escreveu:
> Make pmu_formats_string() to check return value of
> strbuf APIs so that it can detect errors in it.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/pmu.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index bf34468..ddb0261 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -643,20 +643,20 @@ static int pmu_resolve_param_term(struct parse_events_term *term,
>  static char *pmu_formats_string(struct list_head *formats)
>  {
>  	struct perf_pmu_format *format;
> -	char *str;
> -	struct strbuf buf;
> +	char *str = NULL;
> +	struct strbuf buf = STRBUF_INIT;
>  	unsigned i = 0;

Ok, here you use STRBUF_INIT and you removed the equivalent (I guess)
call to strbuf_init(&buf, 0) below, i.e. there will be no alloc the
check.

Looks ok.
  
>  	if (!formats)
>  		return NULL;
>  
> -	strbuf_init(&buf, 0);
>  	/* sysfs exported terms */
>  	list_for_each_entry(format, formats, list)
> -		strbuf_addf(&buf, i++ ? ",%s" : "%s",
> -			    format->name);
> +		if (strbuf_addf(&buf, i++ ? ",%s" : "%s", format->name) < 0)
> +			goto error;
>  
>  	str = strbuf_detach(&buf, NULL);
> +error:
>  	strbuf_release(&buf);
>  
>  	return str;

Re: [PATCH perf/core v2 7/8] perf help: Do not use ALLOC_GROW in add_cmd_list

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:10:51AM +0900, Masami Hiramatsu escreveu:
> Replace ALLOC_GROW with normal realloc code in add_cmd_list()
> so that it can handle errors directly.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/help-unknown-cmd.c |   30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/help-unknown-cmd.c b/tools/perf/util/help-unknown-cmd.c
> index 43a98a4..d62ccae 100644
> --- a/tools/perf/util/help-unknown-cmd.c
> +++ b/tools/perf/util/help-unknown-cmd.c
> @@ -27,16 +27,27 @@ static int levenshtein_compare(const void *p1, const void *p2)
>  	return l1 != l2 ? l1 - l2 : strcmp(s1, s2);
>  }
>  
> -static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
> +static int add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
>  {
> -	unsigned int i;
> -
> -	ALLOC_GROW(cmds->names, cmds->cnt + old->cnt, cmds->alloc);
> -
> +	unsigned int i, nr = cmds->cnt + old->cnt;
> +	void *tmp;
> +
> +	if (nr > cmds->alloc) {
> +		/* Choose bigger one to alloc */
> +		if (alloc_nr(cmds->alloc) < nr)
> +			cmds->alloc = nr;
> +		else
> +			cmds->alloc = alloc_nr(cmds->alloc);
> +		tmp = realloc(cmds->names, cmds->alloc * sizeof(*cmds->names));
> +		if (!tmp)
> +			return -1;

Cool, here you use replace that big realloc-that-fails (ALLOC_GROW) but
still uses realloc, checking its return, great.

Looks ok.

> +		cmds->names = tmp;
> +	}
>  	for (i = 0; i < old->cnt; i++)
>  		cmds->names[cmds->cnt++] = old->names[i];
>  	zfree(&old->names);
>  	old->cnt = 0;
> +	return 0;
>  }
>  
>  const char *help_unknown_cmd(const char *cmd)
> @@ -52,8 +63,11 @@ const char *help_unknown_cmd(const char *cmd)
>  
>  	load_command_list("perf-", &main_cmds, &other_cmds);
>  
> -	add_cmd_list(&main_cmds, &aliases);
> -	add_cmd_list(&main_cmds, &other_cmds);
> +	if (add_cmd_list(&main_cmds, &aliases) < 0 ||
> +	    add_cmd_list(&main_cmds, &other_cmds) < 0) {
> +		fprintf(stderr, "ERROR: Failed to allocate command list for unknown command.\n");
> +		goto end;
> +	}
>  	qsort(main_cmds.names, main_cmds.cnt,
>  	      sizeof(main_cmds.names), cmdname_compare);
>  	uniq(&main_cmds);
> @@ -99,6 +113,6 @@ const char *help_unknown_cmd(const char *cmd)
>  		for (i = 0; i < n; i++)
>  			fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
>  	}
> -
> +end:
>  	exit(1);
>  }

Re: [PATCH perf/core v2 8/8] perf tools: Remove xrealloc and ALLOC_GROW

[Arnaldo Carvalho de Melo]

Em Sat, Apr 30, 2016 at 12:11:00AM +0900, Masami Hiramatsu escreveu:
> Remove unused xrealloc() and ALLOC_GROW() from libperf.

Whee! Thank you!

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/Build     |    1 -
>  tools/perf/util/cache.h   |   19 -------------------
>  tools/perf/util/util.h    |    6 ------
>  tools/perf/util/wrapper.c |   29 -----------------------------
>  4 files changed, 55 deletions(-)
>  delete mode 100644 tools/perf/util/wrapper.c
> 
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 90229a8..b7768ea 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -27,7 +27,6 @@ libperf-y += strlist.o
>  libperf-y += strfilter.o
>  libperf-y += top.o
>  libperf-y += usage.o
> -libperf-y += wrapper.o
>  libperf-y += dso.o
>  libperf-y += symbol.o
>  libperf-y += symbol_fprintf.o
> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
> index 1f5a93c..0d814bb 100644
> --- a/tools/perf/util/cache.h
> +++ b/tools/perf/util/cache.h
> @@ -40,25 +40,6 @@ int split_cmdline(char *cmdline, const char ***argv);
>  
>  #define alloc_nr(x) (((x)+16)*3/2)
>  
> -/*
> - * Realloc the buffer pointed at by variable 'x' so that it can hold
> - * at least 'nr' entries; the number of entries currently allocated
> - * is 'alloc', using the standard growing factor alloc_nr() macro.
> - *
> - * DO NOT USE any expression with side-effect for 'x' or 'alloc'.
> - */
> -#define ALLOC_GROW(x, nr, alloc) \
> -	do { \
> -		if ((nr) > alloc) { \
> -			if (alloc_nr(alloc) < (nr)) \
> -				alloc = (nr); \
> -			else \
> -				alloc = alloc_nr(alloc); \
> -			x = xrealloc((x), alloc * sizeof(*(x))); \
> -		} \
> -	} while(0)
> -
> -
>  static inline int is_absolute_path(const char *path)
>  {
>  	return path[0] == '/';
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 88f607a..7651633 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -160,12 +160,6 @@ static inline char *gitstrchrnul(const char *s, int c)
>  }
>  #endif
>  
> -/*
> - * Wrappers:
> - */
> -void *xrealloc(void *ptr, size_t size) __attribute__((weak));
> -
> -
>  static inline void *zalloc(size_t size)
>  {
>  	return calloc(1, size);
> diff --git a/tools/perf/util/wrapper.c b/tools/perf/util/wrapper.c
> deleted file mode 100644
> index 5f1a07c..0000000
> --- a/tools/perf/util/wrapper.c
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -/*
> - * Various trivial helper wrappers around standard functions
> - */
> -#include "cache.h"
> -
> -/*
> - * There's no pack memory to release - but stay close to the Git
> - * version so wrap this away:
> - */
> -static inline void release_pack_memory(size_t size __maybe_unused,
> -				       int flag __maybe_unused)
> -{
> -}
> -
> -void *xrealloc(void *ptr, size_t size)
> -{
> -	void *ret = realloc(ptr, size);
> -	if (!ret && !size)
> -		ret = realloc(ptr, 1);
> -	if (!ret) {
> -		release_pack_memory(size, -1);
> -		ret = realloc(ptr, size);
> -		if (!ret && !size)
> -			ret = realloc(ptr, 1);
> -		if (!ret)
> -			die("Out of memory, realloc failed");
> -	}
> -	return ret;
> -}

Re: [PATCH perf/core v2 1/8] perf: Rewrite strbuf not to die

[Masami Hiramatsu]

On Thu, 5 May 2016 20:25:38 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Sat, Apr 30, 2016 at 12:09:50AM +0900, Masami Hiramatsu escreveu:
> > Rewrite strbuf implementation not to use die() nor xrealloc().
> > Instead of die, now most of the API returns error code or 0 if
> > succeeded.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/strbuf.c |   93 +++++++++++++++++++++++++++++++++-------------
> >  tools/perf/util/strbuf.h |   25 +++++++-----
> >  2 files changed, 82 insertions(+), 36 deletions(-)
> > 
> > diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
> > index 8fb7329..a98bb60 100644
> > --- a/tools/perf/util/strbuf.c
> > +++ b/tools/perf/util/strbuf.c
> > @@ -1,3 +1,4 @@
> > +#include "debug.h"
> >  #include "cache.h"
> >  #include <linux/kernel.h>
> >  
> > @@ -17,12 +18,13 @@ int prefixcmp(const char *str, const char *prefix)
> >   */
> >  char strbuf_slopbuf[1];
> >  
> > -void strbuf_init(struct strbuf *sb, ssize_t hint)
> > +int strbuf_init(struct strbuf *sb, ssize_t hint)
> >  {
> >  	sb->alloc = sb->len = 0;
> >  	sb->buf = strbuf_slopbuf;
> >  	if (hint)
> > -		strbuf_grow(sb, hint);
> > +		return strbuf_grow(sb, hint);
> > +	return 0;
> >  }
> >  
> >  void strbuf_release(struct strbuf *sb)
> > @@ -42,67 +44,104 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
> >  	return res;
> >  }
> >  
> > -void strbuf_grow(struct strbuf *sb, size_t extra)
> > +int strbuf_grow(struct strbuf *sb, size_t extra)
> >  {
> > -	if (sb->len + extra + 1 <= sb->len)
> > -		die("you want to use way too much memory");
> > -	if (!sb->alloc)
> > -		sb->buf = NULL;
> > -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> > +	char *buf;
> > +	size_t nr = sb->len + extra + 1;
> > +
> > +	if (nr < sb->alloc)
> > +		return 0;
> > +
> > +	if (nr <= sb->len)
> > +		return -E2BIG;
> > +
> > +	if (alloc_nr(sb->alloc) > nr)
> > +		nr = alloc_nr(sb->alloc);
> > +
> > +	buf = malloc(nr * sizeof(*buf));
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	if (sb->alloc) {
> > +		memcpy(buf, sb->buf, sb->alloc);
> > +		free(sb->buf);
> > +	}
> 
> Why not use realloc? I.e. as the old code did, the problem was not that,
> was just the panic when the realloc operation fails, that you are
> removing here.

Oops, right. it seems I misunderstood the return value of realloc.
> 
> I.e. the above would be:
> 
> 	buf = realloc(sb->buf, nr * sizeof(*buf));
> 	if (!buf)
> 		return -ENOMEM;
> 
> > +	sb->buf = buf;
> > +	sb->alloc = nr;
> > +	return 0;
> 
> I.e. no need to do the memcpy() nor the free(), no?

Yes, agreed.
I'll update to do so.

Thanks!




-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH perf/core v2 2/8] perf probe: Check the return value of strbuf APIs

[Masami Hiramatsu]

Hi Arnaldo,

Thank you for reviewing!

On Thu, 5 May 2016 20:46:11 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Sat, Apr 30, 2016 at 12:10:00AM +0900, Masami Hiramatsu escreveu:
> > Check the return value of strbuf APIs in perf-probe
> > related code, so that it can handle errors in strbuf.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >   Changes in V2:
> >    - Rebased on the latest perf/core.
> > ---
> >  tools/perf/util/dwarf-aux.c    |   50 ++++++--------
> >  tools/perf/util/probe-event.c  |  146 ++++++++++++++++++++++++----------------
> >  tools/perf/util/probe-finder.c |   30 +++++---
> >  3 files changed, 126 insertions(+), 100 deletions(-)
> > 
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index aea189b..8c8b4c1 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -915,8 +915,7 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
> >  		tmp = "*";
> >  	else if (tag == DW_TAG_subroutine_type) {
> >  		/* Function pointer */
> > -		strbuf_add(buf, "(function_type)", 15);
> > -		return 0;
> > +		return strbuf_add(buf, "(function_type)", 15);
> >  	} else {
> >  		if (!dwarf_diename(&type))
> >  			return -ENOENT;
> > @@ -927,14 +926,10 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
> >  		else if (tag == DW_TAG_enumeration_type)
> >  			tmp = "enum ";
> >  		/* Write a base name */
> > -		strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> > -		return 0;
> > +		return strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> >  	}
> >  	ret = die_get_typename(&type, buf);
> > -	if (ret == 0)
> > -		strbuf_addstr(buf, tmp);
> > -
> > -	return ret;
> > +	return ret ? ret : strbuf_addstr(buf, tmp);
> >  }
> >  
> >  /**
> > @@ -951,12 +946,10 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
> >  	ret = die_get_typename(vr_die, buf);
> >  	if (ret < 0) {
> >  		pr_debug("Failed to get type, make it unknown.\n");
> > -		strbuf_add(buf, " (unknown_type)", 14);
> > +		ret = strbuf_add(buf, " (unknown_type)", 14);
> >  	}
> >  
> > -	strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> > -
> > -	return 0;
> > +	return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> >  }
> >  
> >  #ifdef HAVE_DWARF_GETLOCATIONS
> > @@ -998,23 +991,23 @@ static int die_get_var_innermost_scope(Dwarf_Die *sp_die, Dwarf_Die *vr_die,
> >  		goto out;
> >  	}
> >  
> > -	while ((offset = dwarf_ranges(&scopes[1], offset, &base,
> > -				&start, &end)) > 0) {
> > +	while (!ret && (offset = dwarf_ranges(&scopes[1], offset, &base,
> > +					&start, &end)) > 0) {
> 
> Why not leave the above as is, i.e. no checking for ret that will always
> fail the first time, and...
> 
> >  		start -= entry;
> >  		end -= entry;
> >  
> >  		if (first) {
> > -			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > -				name, start, end);
> > +			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > +					  name, start, end);
> >  			first = false;
> >  		} else {
> > -			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > -				start, end);
> > +			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > +					  start, end);
> >  		}
> 
> Add:
> 
> 		if (ret)
> 			goto out;
> 
> Here?

I just reused the while() condition :) 

> 
> >  	}
> >  
> > -	if (!first)
> > -		strbuf_add(buf, "]>", 2);
> > +	if (!ret && !first)
> > +		ret = strbuf_add(buf, "]>", 2);
> 
> No need for the above hunk? ditto for the following set of hunks.

Yeah, just for avoiding goto. But agreed, it seems not straightforward.
I'll fix that. (btw, anyway we still have to check the retval of the
last strbuf__add())

> 
> >  out:
> >  	free(scopes);
> > @@ -1054,9 +1047,8 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
> >  	if (dwarf_attr(vr_die, DW_AT_location, &attr) == NULL)
> >  		return -EINVAL;
> >  
> > -	while ((offset = dwarf_getlocations(
> > -				&attr, offset, &base,
> > -				&start, &end, &op, &nops)) > 0) {
> > +	while (!ret && (offset = dwarf_getlocations(&attr, offset, &base,
> > +					&start, &end, &op, &nops)) > 0) {
> >  		if (start == 0) {
> >  			/* Single Location Descriptions */
> >  			ret = die_get_var_innermost_scope(sp_die, vr_die, buf);
> > @@ -1067,17 +1059,17 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
> >  		start -= entry;
> >  		end -= entry;
> >  		if (first) {
> > -			strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > -				name, start, end);
> > +			ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > +					  name, start, end);
> >  			first = false;
> >  		} else {
> > -			strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > -				start, end);
> > +			ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > +					  start, end);
> >  		}
> >  	}
> >  
> > -	if (!first)
> > -		strbuf_add(buf, "]>", 2);
> > +	if (!ret && !first)
> > +		ret = strbuf_add(buf, "]>", 2);

fix this too.

> >  
> >  	return ret;
> >  }
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 85d82f4..ff9c5c1 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -1677,28 +1677,36 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> >  {
> >  	struct perf_probe_arg_field *field = pa->field;
> >  	struct strbuf buf;
> > -	char *ret;
> > +	char *ret = NULL;
> > +	int err;
> > +
> > +	if (strbuf_init(&buf, 64) < 0)
> > +		return NULL;
> >  
> > -	strbuf_init(&buf, 64);
> >  	if (pa->name && pa->var)
> > -		strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> > +		err = strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> >  	else
> > -		strbuf_addstr(&buf, pa->name ?: pa->var);
> > -
> > -	while (field) {
> > +		err = strbuf_addstr(&buf, pa->name ?: pa->var);
> > +	if (err)
> > +		goto out;
> 
> Why test err twice? Leave the 'while (field)' and...
> 
> > +	while (field && !err) {
> >  		if (field->name[0] == '[')
> > -			strbuf_addstr(&buf, field->name);
> > +			err = strbuf_addstr(&buf, field->name);
> >  		else
> > -			strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> > -				    field->name);
> > +			err = strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> > +					  field->name);
> >  		field = field->next;
> 
> 		if (err)
> 			goto out;
> >  	}
> 
> Move the following check + goto to inside the loop, as the last thing,
> otherwise you'll be testing it twice for no reason.

OK, I'll fix this.

> 
> > +	if (err)
> > +		goto out;
> >  
> >  	if (pa->type)
> > -		strbuf_addf(&buf, ":%s", pa->type);
> > +		if (strbuf_addf(&buf, ":%s", pa->type) < 0)
> > +			goto out;
> 
> The bove is wrong, it is not propagating the error, should be:

No need to propagate. This function returns allocated string.
If an error happens it just returns NULL.

> 
> 
> 	if (pa->type) {
> 		ret = strbuf_addf(&buf, ":%s", pa->type);
> 		if (ret < 0)
> 			goto out;
> 	}
> 
> >  
> >  	ret = strbuf_detach(&buf, NULL);
> > -
> > +out:
> > +	strbuf_release(&buf);
> >  	return ret;
> >  }
> >  
> > @@ -1706,18 +1714,23 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> >  static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> >  {
> >  	struct strbuf buf;
> > -	char *tmp;
> > -	int len;
> > +	char *tmp, *ret = NULL;
> > +	int len, err = 0;
> > +
> > +	if (strbuf_init(&buf, 64) < 0)
> > +		return NULL;
> >  
> > -	strbuf_init(&buf, 64);
> >  	if (pp->function) {
> > -		strbuf_addstr(&buf, pp->function);
> > +		if (strbuf_addstr(&buf, pp->function) < 0)
> > +			goto out;
> >  		if (pp->offset)
> > -			strbuf_addf(&buf, "+%lu", pp->offset);
> > +			err = strbuf_addf(&buf, "+%lu", pp->offset);
> >  		else if (pp->line)
> > -			strbuf_addf(&buf, ":%d", pp->line);
> > +			err = strbuf_addf(&buf, ":%d", pp->line);
> >  		else if (pp->retprobe)
> > -			strbuf_addstr(&buf, "%return");
> > +			err = strbuf_addstr(&buf, "%return");
> > +		if (err)
> > +			goto out;
> >  	}
> >  	if (pp->file) {
> >  		tmp = pp->file;
> > @@ -1726,12 +1739,15 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> >  			tmp = strchr(pp->file + len - 30, '/');
> >  			tmp = tmp ? tmp + 1 : pp->file + len - 30;
> >  		}
> > -		strbuf_addf(&buf, "@%s", tmp);
> > -		if (!pp->function && pp->line)
> > -			strbuf_addf(&buf, ":%d", pp->line);
> > +		err = strbuf_addf(&buf, "@%s", tmp);
> > +		if (!err && !pp->function && pp->line)
> > +			err = strbuf_addf(&buf, ":%d", pp->line);
> >  	}
> > -
> > -	return strbuf_detach(&buf, NULL);
> > +	if (!err)
> > +		ret = strbuf_detach(&buf, NULL);
> > +out:
> > +	strbuf_release(&buf);
> > +	return ret;
> >  }
> >  
> >  #if 0
> > @@ -1762,28 +1778,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
> >  static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
> >  					    struct strbuf *buf, int depth)
> >  {
> > +	int err;
> >  	if (ref->next) {
> >  		depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
> >  							 depth + 1);
> >  		if (depth < 0)
> > -			goto out;
> > +			return depth;
> >  	}
> > -	strbuf_addf(buf, "%+ld(", ref->offset);
> > -out:
> > -	return depth;
> > +	err = strbuf_addf(buf, "%+ld(", ref->offset);
> > +	return (err < 0) ? err : depth;
> >  }
> 
> The above would be more compact as:
> 
> 	return err ?: depth;
> 
> And would at a glance state that the return of addf() is 0 for Ok and -1
> for error, which I checked and is the case, minor detail tho. :-)

No, strbuf_addf returns the retval of strbuf_addv, which finally returns
strbuf_setlen() for OK. :(

> 
> >  
> >  static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> >  				      struct strbuf *buf)
> >  {
> >  	struct probe_trace_arg_ref *ref = arg->ref;
> > -	int depth = 0;
> > +	int depth = 0, err;
> >  
> >  	/* Argument name or separator */
> >  	if (arg->name)
> > -		strbuf_addf(buf, " %s=", arg->name);
> > +		err = strbuf_addf(buf, " %s=", arg->name);
> >  	else
> > -		strbuf_addch(buf, ' ');
> > +		err = strbuf_addch(buf, ' ');
> > +	if (err)
> > +		return err;
> >  
> >  	/* Special case: @XXX */
> >  	if (arg->value[0] == '@' && arg->ref)
> > @@ -1798,18 +1816,19 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> >  
> >  	/* Print argument value */
> >  	if (arg->value[0] == '@' && arg->ref)
> > -		strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> > +		err = strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> >  	else
> > -		strbuf_addstr(buf, arg->value);
> > +		err = strbuf_addstr(buf, arg->value);
> 
> Missing check:
> 
> 	if (err)
> 		return err;
> 
> Ahh, ok, caught in the following loop, but then there the !err would be
> better as the first thing, as it checks what comes immediately before
> it, i.e. no sense in decrementing depth if there was an error before
> that loop. Confusing, but harmless.

Yeah, I see. let me fix...

> 
> >  
> >  	/* Closing */
> > -	while (depth--)
> > -		strbuf_addch(buf, ')');
> > +	while (depth-- && !err)
> > +		err = strbuf_addch(buf, ')');
> > +
> >  	/* Print argument type */
> > -	if (arg->type)
> > -		strbuf_addf(buf, ":%s", arg->type);
> 
> One more, its more clear as:
> 
> 	if (!err && arg->type)
> 
> But harmless...

OK, 

> 
> > +	if (arg->type && !err)
> > +		err = strbuf_addf(buf, ":%s", arg->type);
> >  
> > -	return 0;
> > +	return err;
> >  }
> >  
> >  char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> > @@ -1817,15 +1836,18 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> >  	struct probe_trace_point *tp = &tev->point;
> >  	struct strbuf buf;
> >  	char *ret = NULL;
> > -	int i;
> > +	int i, err;
> >  
> >  	/* Uprobes must have tp->module */
> >  	if (tev->uprobes && !tp->module)
> >  		return NULL;
> >  
> > -	strbuf_init(&buf, 32);
> > -	strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > -		    tev->group, tev->event);
> > +	if (strbuf_init(&buf, 32) < 0)
> > +		return NULL;
> > +
> > +	if (strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > +			tev->group, tev->event) < 0)
> > +		goto error;
> >  	/*
> >  	 * If tp->address == 0, then this point must be a
> >  	 * absolute address uprobe.
> > @@ -1839,14 +1861,16 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> >  
> >  	/* Use the tp->address for uprobes */
> >  	if (tev->uprobes)
> > -		strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> > +		err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> >  	else if (!strncmp(tp->symbol, "0x", 2))
> >  		/* Absolute address. See try_to_find_absolute_address() */
> > -		strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> > -			    tp->module ? ":" : "", tp->address);
> > +		err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> > +				  tp->module ? ":" : "", tp->address);
> >  	else
> > -		strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> > -			    tp->module ? ":" : "", tp->symbol, tp->offset);
> > +		err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> > +				tp->module ? ":" : "", tp->symbol, tp->offset);
> > +	if (err)
> > +		goto error;
> 
> Good! I thought you would do it as:
> 
>   	for (i = 0; !err && i < tev->nargs; i++)
> 
> ;-) :-)

I might finally have changed my mind :)

> 
> >  
> >  	for (i = 0; i < tev->nargs; i++)
> >  		if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
> > @@ -1960,14 +1984,15 @@ static int convert_to_perf_probe_event(struct probe_trace_event *tev,
> >  		if (tev->args[i].name)
> >  			pev->args[i].name = strdup(tev->args[i].name);
> >  		else {
> > -			strbuf_init(&buf, 32);
> > +			if ((ret = strbuf_init(&buf, 32)) < 0)
> > +				goto error;
> >  			ret = synthesize_probe_trace_arg(&tev->args[i], &buf);
> >  			pev->args[i].name = strbuf_detach(&buf, NULL);
> >  		}
> >  		if (pev->args[i].name == NULL && ret >= 0)
> >  			ret = -ENOMEM;
> >  	}
> > -
> > +error:
> >  	if (ret < 0)
> >  		clear_perf_probe_event(pev);
> >  
> > @@ -2140,37 +2165,40 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
> >  				     const char *module,
> >  				     struct strbuf *result)
> >  {
> > -	int i;
> > +	int i, ret;
> >  	char *buf;
> >  
> >  	if (asprintf(&buf, "%s:%s", group, event) < 0)
> >  		return -errno;
> > -	strbuf_addf(result, "  %-20s (on ", buf);
> > +	ret = strbuf_addf(result, "  %-20s (on ", buf);
> >  	free(buf);
> > +	if (ret)
> > +		return ret;
> >  
> >  	/* Synthesize only event probe point */
> >  	buf = synthesize_perf_probe_point(&pev->point);
> >  	if (!buf)
> >  		return -ENOMEM;
> > -	strbuf_addstr(result, buf);
> > +	ret = strbuf_addstr(result, buf);
> >  	free(buf);
> >  
> > -	if (module)
> > -		strbuf_addf(result, " in %s", module);
> 
> the next ones match what I suggested elsewhere: first check the result
> of the previous step before reusing the existing if to do something,
> as a way of saving source code lines, ok.

Agreed.

Thank you!

> 
> > +	if (!ret && module)
> > +		ret = strbuf_addf(result, " in %s", module);
> >  
> > -	if (pev->nargs > 0) {
> > -		strbuf_add(result, " with", 5);
> > -		for (i = 0; i < pev->nargs; i++) {
> > +	if (!ret && pev->nargs > 0) {
> > +		ret = strbuf_add(result, " with", 5);
> > +		for (i = 0; !ret && i < pev->nargs; i++) {
> >  			buf = synthesize_perf_probe_arg(&pev->args[i]);
> >  			if (!buf)
> >  				return -ENOMEM;
> > -			strbuf_addf(result, " %s", buf);
> > +			ret = strbuf_addf(result, " %s", buf);
> >  			free(buf);
> >  		}
> >  	}
> > -	strbuf_addch(result, ')');
> > +	if (!ret)
> > +		ret = strbuf_addch(result, ')');
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  /* Show an event */
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 9f68875..1259839 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1294,6 +1294,7 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> >  {
> >  	struct available_var_finder *af = data;
> >  	struct variable_list *vl;
> > +	struct strbuf buf = STRBUF_INIT;
> >  	int tag, ret;
> >  
> >  	vl = &af->vls[af->nvls - 1];
> > @@ -1307,25 +1308,26 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> >  		if (ret == 0 || ret == -ERANGE) {
> >  			int ret2;
> >  			bool externs = !af->child;
> > -			struct strbuf buf;
> >  
> > -			strbuf_init(&buf, 64);
> > +			if (strbuf_init(&buf, 64) < 0)
> > +				goto error;
> >  
> >  			if (probe_conf.show_location_range) {
> > -				if (!externs) {
> > -					if (ret)
> > -						strbuf_add(&buf, "[INV]\t", 6);
> > -					else
> > -						strbuf_add(&buf, "[VAL]\t", 6);
> > -				} else
> > -					strbuf_add(&buf, "[EXT]\t", 6);
> > +				if (!externs)
> > +					ret2 = strbuf_add(&buf,
> > +						ret ? "[INV]\t" : "[VAL]\t", 6);
> > +				else
> > +					ret2 = strbuf_add(&buf, "[EXT]\t", 6);
> > +				if (ret2)
> > +					goto error;
> >  			}
> >  
> >  			ret2 = die_get_varname(die_mem, &buf);
> >  
> >  			if (!ret2 && probe_conf.show_location_range &&
> >  				!externs) {
> > -				strbuf_addch(&buf, '\t');
> > +				if (strbuf_addch(&buf, '\t') < 0)
> > +					goto error;
> >  				ret2 = die_get_var_range(&af->pf.sp_die,
> >  							die_mem, &buf);
> >  			}
> > @@ -1334,8 +1336,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> >  			if (ret2 == 0) {
> >  				strlist__add(vl->vars,
> >  					strbuf_detach(&buf, NULL));
> > -			} else
> > -				strbuf_release(&buf);
> > +			}
> > +			strbuf_release(&buf);
> >  		}
> >  	}
> >  
> > @@ -1343,6 +1345,10 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> >  		return DIE_FIND_CB_CONTINUE;
> >  	else
> >  		return DIE_FIND_CB_SIBLING;
> > +error:
> > +	strbuf_release(&buf);
> > +	pr_debug("Error in strbuf\n");
> > +	return DIE_FIND_CB_END;
> >  }
> >  
> >  /* Add a found vars into available variables list */


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH perf/core v2 5/8] perf header: Make topology checkers to check return value of strbuf

[Masami Hiramatsu]

On Thu, 5 May 2016 20:55:18 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Sat, Apr 30, 2016 at 12:10:33AM +0900, Masami Hiramatsu escreveu:
> > Make topology checkers to check the return value of strbuf
> > APIs so that it can detect errors in it.
> 
> >  	}
> > @@ -1907,7 +1910,7 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
> >  	u32 nr, node, i;
> >  	char *str;
> >  	uint64_t mem_total, mem_free;
> > -	struct strbuf sb;
> > +	struct strbuf sb = STRBUF_INIT;
> 
> Since you're going to call strbuf_init() later, is the above really
> needed?

Actually, this is for strbuf_release() at the error path at the
end of this function.
----
error:
        strbuf_release(&sb);
        return -1;
}
----

So, without initializing sb, strbuf_release tries to release random
address in sb.buf.
Of course we can just return -1 for such cases and avoid initializing :)

>   
> >  	/* nr nodes */
> >  	ret = readn(fd, &nr, sizeof(nr));
> > @@ -1918,7 +1921,8 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
> >  		nr = bswap_32(nr);
> >  
> >  	ph->env.nr_numa_nodes = nr;
> > -	strbuf_init(&sb, 256);
> > +	if (strbuf_init(&sb, 256) < 0)
> > +		goto error;
> >  


-- 
Masami Hiramatsu <mhiramat@kernel.org>