Re: [PATCH perf/core v4 01/19] perf probe: Use strbuf for making strings

From: Arnaldo Carvalho de Melo
Date: Tue Apr 26 2016 - 09:37:12 EST

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH perf/core v4 04/19] perf: Add lsdir to read a directory"
Previous message: Rob Herring: "Re: [PATCH 15/41] Documentation: dt: mfd: fix spelling mistakes"
In reply to: Masami Hiramatsu: "[PATCH perf/core v4 01/19] perf probe: Use strbuf for making strings"
Next in thread: Masami Hiramatsu: "Re: [PATCH perf/core v4 01/19] perf probe: Use strbuf for making strings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu:
> From: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>
> Replace many fixed-length char array with strbuf to
> stringify perf_probe_event and probe_trace_event etc.

Sure you want to do that? From time to time I try to reduce the strbuf
usage, not grow it :-\

That is one of the last users of xrealloc(), via that ALLOC_GROW()
thing.

- Arnaldo

> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>
> ---
> Changes in v4:
> - Update for the latest kernel.
> - Remove "probe-event.c" from title and body, because
> this also changes probe-finder.c too :)
>
> Changes in v3:
> - Remove unneeded strbuf_release(). (Thanks Namhyung!)
>
> Changes in v2:
> - Make perf_probe_event__sprintf() simpler.
> ---
> tools/perf/util/probe-event.c | 246 ++++++++++++++--------------------------
> tools/perf/util/probe-event.h | 2
> tools/perf/util/probe-finder.c | 14 +-
> 3 files changed, 93 insertions(+), 169 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 8319fbb..148af81 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1618,69 +1618,51 @@ out:
> }
>
> /* Compose only probe arg */
> -int synthesize_perf_probe_arg(struct perf_probe_arg *pa, char *buf, size_t len)
> +char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> {
> struct perf_probe_arg_field *field = pa->field;
> - int ret;
> - char *tmp = buf;
> + struct strbuf buf;
> + char *ret;
>
> + strbuf_init(&buf, 64);
> if (pa->name && pa->var)
> - ret = e_snprintf(tmp, len, "%s=%s", pa->name, pa->var);
> + strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> else
> - ret = e_snprintf(tmp, len, "%s", pa->name ? pa->name : pa->var);
> - if (ret <= 0)
> - goto error;
> - tmp += ret;
> - len -= ret;
> + strbuf_addstr(&buf, pa->name ?: pa->var);
>
> while (field) {
> if (field->name[0] == '[')
> - ret = e_snprintf(tmp, len, "%s", field->name);
> + strbuf_addstr(&buf, field->name);
> else
> - ret = e_snprintf(tmp, len, "%s%s",
> - field->ref ? "->" : ".", field->name);
> - if (ret <= 0)
> - goto error;
> - tmp += ret;
> - len -= ret;
> + strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> + field->name);
> field = field->next;
> }
>
> - if (pa->type) {
> - ret = e_snprintf(tmp, len, ":%s", pa->type);
> - if (ret <= 0)
> - goto error;
> - tmp += ret;
> - len -= ret;
> - }
> + if (pa->type)
> + strbuf_addf(&buf, ":%s", pa->type);
> +
> + ret = strbuf_detach(&buf, NULL);
>
> - return tmp - buf;
> -error:
> - pr_debug("Failed to synthesize perf probe argument: %d\n", ret);
> return ret;
> }
>
> /* Compose only probe point (not argument) */
> static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> {
> - char *buf, *tmp;
> - char offs[32] = "", line[32] = "", file[32] = "";
> - int ret, len;
> -
> - buf = zalloc(MAX_CMDLEN);
> - if (buf == NULL) {
> - ret = -ENOMEM;
> - goto error;
> - }
> - if (pp->offset) {
> - ret = e_snprintf(offs, 32, "+%lu", pp->offset);
> - if (ret <= 0)
> - goto error;
> - }
> - if (pp->line) {
> - ret = e_snprintf(line, 32, ":%d", pp->line);
> - if (ret <= 0)
> - goto error;
> + struct strbuf buf;
> + char *tmp;
> + int len;
> +
> + strbuf_init(&buf, 64);
> + if (pp->function) {
> + strbuf_addstr(&buf, pp->function);
> + if (pp->offset)
> + strbuf_addf(&buf, "+%lu", pp->offset);
> + else if (pp->line)
> + strbuf_addf(&buf, ":%d", pp->line);
> + else if (pp->retprobe)
> + strbuf_addstr(&buf, "%return");
> }
> if (pp->file) {
> tmp = pp->file;
> @@ -1689,25 +1671,12 @@ 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;
> }
> - ret = e_snprintf(file, 32, "@%s", tmp);
> - if (ret <= 0)
> - goto error;
> + strbuf_addf(&buf, "@%s", tmp);
> + if (!pp->function && pp->line)
> + strbuf_addf(&buf, ":%d", pp->line);
> }
>
> - if (pp->function)
> - ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s%s", pp->function,
> - offs, pp->retprobe ? "%return" : "", line,
> - file);
> - else
> - ret = e_snprintf(buf, MAX_CMDLEN, "%s%s", file, line);
> - if (ret <= 0)
> - goto error;
> -
> - return buf;
> -error:
> - pr_debug("Failed to synthesize perf probe point: %d\n", ret);
> - free(buf);
> - return NULL;
> + return strbuf_detach(&buf, NULL);
> }
>
> #if 0
> @@ -1736,45 +1705,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
> #endif
>
> static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
> - char **buf, size_t *buflen,
> - int depth)
> + struct strbuf *buf, int depth)
> {
> - int ret;
> if (ref->next) {
> depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
> - buflen, depth + 1);
> + depth + 1);
> if (depth < 0)
> goto out;
> }
> -
> - ret = e_snprintf(*buf, *buflen, "%+ld(", ref->offset);
> - if (ret < 0)
> - depth = ret;
> - else {
> - *buf += ret;
> - *buflen -= ret;
> - }
> + strbuf_addf(buf, "%+ld(", ref->offset);
> out:
> return depth;
> -
> }
>
> static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> - char *buf, size_t buflen)
> + struct strbuf *buf)
> {
> struct probe_trace_arg_ref *ref = arg->ref;
> - int ret, depth = 0;
> - char *tmp = buf;
> + int depth = 0;
>
> /* Argument name or separator */
> if (arg->name)
> - ret = e_snprintf(buf, buflen, " %s=", arg->name);
> + strbuf_addf(buf, " %s=", arg->name);
> else
> - ret = e_snprintf(buf, buflen, " ");
> - if (ret < 0)
> - return ret;
> - buf += ret;
> - buflen -= ret;
> + strbuf_addch(buf, ' ');
>
> /* Special case: @XXX */
> if (arg->value[0] == '@' && arg->ref)
> @@ -1782,60 +1736,41 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
>
> /* Dereferencing arguments */
> if (ref) {
> - depth = __synthesize_probe_trace_arg_ref(ref, &buf,
> - &buflen, 1);
> + depth = __synthesize_probe_trace_arg_ref(ref, buf, 1);
> if (depth < 0)
> return depth;
> }
>
> /* Print argument value */
> if (arg->value[0] == '@' && arg->ref)
> - ret = e_snprintf(buf, buflen, "%s%+ld", arg->value,
> - arg->ref->offset);
> + strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> else
> - ret = e_snprintf(buf, buflen, "%s", arg->value);
> - if (ret < 0)
> - return ret;
> - buf += ret;
> - buflen -= ret;
> + strbuf_addstr(buf, arg->value);
>
> /* Closing */
> - while (depth--) {
> - ret = e_snprintf(buf, buflen, ")");
> - if (ret < 0)
> - return ret;
> - buf += ret;
> - buflen -= ret;
> - }
> + while (depth--)
> + strbuf_addch(buf, ')');
> /* Print argument type */
> - if (arg->type) {
> - ret = e_snprintf(buf, buflen, ":%s", arg->type);
> - if (ret <= 0)
> - return ret;
> - buf += ret;
> - }
> + if (arg->type)
> + strbuf_addf(buf, ":%s", arg->type);
>
> - return buf - tmp;
> + return 0;
> }
>
> char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> {
> struct probe_trace_point *tp = &tev->point;
> - char *buf;
> - int i, len, ret;
> -
> - buf = zalloc(MAX_CMDLEN);
> - if (buf == NULL)
> - return NULL;
> -
> - len = e_snprintf(buf, MAX_CMDLEN, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> - tev->group, tev->event);
> - if (len <= 0)
> - goto error;
> + struct strbuf buf;
> + char *ret = NULL;
> + int i;
>
> /* Uprobes must have tp->module */
> if (tev->uprobes && !tp->module)
> - goto error;
> + return NULL;
> +
> + strbuf_init(&buf, 32);
> + strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> + tev->group, tev->event);
> /*
> * If tp->address == 0, then this point must be a
> * absolute address uprobe.
> @@ -1849,34 +1784,23 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>
> /* Use the tp->address for uprobes */
> if (tev->uprobes)
> - ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s:0x%lx",
> - tp->module, tp->address);
> + 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() */
> - ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s%s0x%lx",
> - tp->module ?: "", tp->module ? ":" : "",
> - tp->address);
> + strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> + tp->module ? ":" : "", tp->address);
> else
> - ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s%s%s+%lu",
> - tp->module ?: "", tp->module ? ":" : "",
> - tp->symbol, tp->offset);
> -
> - if (ret <= 0)
> - goto error;
> - len += ret;
> + strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> + tp->module ? ":" : "", tp->symbol, tp->offset);
>
> - for (i = 0; i < tev->nargs; i++) {
> - ret = synthesize_probe_trace_arg(&tev->args[i], buf + len,
> - MAX_CMDLEN - len);
> - if (ret <= 0)
> + for (i = 0; i < tev->nargs; i++)
> + if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
> goto error;
> - len += ret;
> - }
>
> - return buf;
> + ret = strbuf_detach(&buf, NULL);
> error:
> - free(buf);
> - return NULL;
> + strbuf_release(&buf);
> + return ret;
> }
>
> static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
> @@ -1958,7 +1882,7 @@ static int convert_to_perf_probe_point(struct probe_trace_point *tp,
> static int convert_to_perf_probe_event(struct probe_trace_event *tev,
> struct perf_probe_event *pev, bool is_kprobe)
> {
> - char buf[64] = "";
> + struct strbuf buf = STRBUF_INIT;
> int i, ret;
>
> /* Convert event/group name */
> @@ -1981,9 +1905,9 @@ 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 {
> - ret = synthesize_probe_trace_arg(&tev->args[i],
> - buf, 64);
> - pev->args[i].name = strdup(buf);
> + strbuf_init(&buf, 32);
> + 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;
> @@ -2161,37 +2085,37 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
> const char *module,
> struct strbuf *result)
> {
> - int i, ret;
> - char buf[128];
> - char *place;
> + int i;
> + char *buf;
>
> - /* Synthesize only event probe point */
> - place = synthesize_perf_probe_point(&pev->point);
> - if (!place)
> - return -EINVAL;
> + if (asprintf(&buf, "%s:%s", group, event) < 0)
> + return -errno;
> + strbuf_addf(result, " %-20s (on ", buf);
> + free(buf);
>
> - ret = e_snprintf(buf, 128, "%s:%s", group, event);
> - if (ret < 0)
> - goto out;
> + /* Synthesize only event probe point */
> + buf = synthesize_perf_probe_point(&pev->point);
> + if (!buf)
> + return -ENOMEM;
> + strbuf_addstr(result, buf);
> + free(buf);
>
> - strbuf_addf(result, " %-20s (on %s", buf, place);
> if (module)
> strbuf_addf(result, " in %s", module);
>
> if (pev->nargs > 0) {
> strbuf_add(result, " with", 5);
> for (i = 0; i < pev->nargs; i++) {
> - ret = synthesize_perf_probe_arg(&pev->args[i],
> - buf, 128);
> - if (ret < 0)
> - goto out;
> + buf = synthesize_perf_probe_arg(&pev->args[i]);
> + if (!buf)
> + return -ENOMEM;
> strbuf_addf(result, " %s", buf);
> + free(buf);
> }
> }
> strbuf_addch(result, ')');
> -out:
> - free(place);
> - return ret;
> +
> + return 0;
> }
>
> /* Show an event */
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index e54e7b0..e220962 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -120,7 +120,7 @@ int parse_probe_trace_command(const char *cmd, struct probe_trace_event *tev);
> /* Events to command string */
> char *synthesize_perf_probe_command(struct perf_probe_event *pev);
> char *synthesize_probe_trace_command(struct probe_trace_event *tev);
> -int synthesize_perf_probe_arg(struct perf_probe_arg *pa, char *buf, size_t len);
> +char *synthesize_perf_probe_arg(struct perf_probe_arg *pa);
>
> /* Check the perf_probe_event needs debuginfo */
> bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index b3bd0fb..9f68875 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -553,7 +553,7 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
> static int find_variable(Dwarf_Die *sc_die, struct probe_finder *pf)
> {
> Dwarf_Die vr_die;
> - char buf[32], *ptr;
> + char *buf, *ptr;
> int ret = 0;
>
> /* Copy raw parameters */
> @@ -563,13 +563,13 @@ static int find_variable(Dwarf_Die *sc_die, struct probe_finder *pf)
> if (pf->pvar->name)
> pf->tvar->name = strdup(pf->pvar->name);
> else {
> - ret = synthesize_perf_probe_arg(pf->pvar, buf, 32);
> - if (ret < 0)
> - return ret;
> + buf = synthesize_perf_probe_arg(pf->pvar);
> + if (!buf)
> + return -ENOMEM;
> ptr = strchr(buf, ':'); /* Change type separator to _ */
> if (ptr)
> *ptr = '_';
> - pf->tvar->name = strdup(buf);
> + pf->tvar->name = buf;
> }
> if (pf->tvar->name == NULL)
> return -ENOMEM;
> @@ -1334,8 +1334,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> if (ret2 == 0) {
> strlist__add(vl->vars,
> strbuf_detach(&buf, NULL));
> - }
> - strbuf_release(&buf);
> + } else
> + strbuf_release(&buf);
> }
> }
>

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH perf/core v4 04/19] perf: Add lsdir to read a directory"
Previous message: Rob Herring: "Re: [PATCH 15/41] Documentation: dt: mfd: fix spelling mistakes"
In reply to: Masami Hiramatsu: "[PATCH perf/core v4 01/19] perf probe: Use strbuf for making strings"
Next in thread: Masami Hiramatsu: "Re: [PATCH perf/core v4 01/19] perf probe: Use strbuf for making strings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]