Re: [RFC PATCH perf/core v3 01/17] perf probe: Use strbuf for making strings in probe-event.c

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Sat, Aug 15, 2015 at 08:42:55PM +0900, Masami Hiramatsu escreveu:
> Replace many fixed-length char array with strbuf to
> stringify perf_probe_event and probe_trace_event etc. in
> util/probe-event.c.

Doesn't apply to perf/core.

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> Changes in v3:
>  - Remove unneeded strbuf_release(). (Thanks Namhyung!)
> 
> Changes in v2:
>  - Make perf_probe_event__sprintf() simpler.
> ---
>  tools/perf/util/probe-event.c  |  238 ++++++++++++++--------------------------
>  tools/perf/util/probe-event.h  |    3 -
>  tools/perf/util/probe-finder.c |   14 +-
>  3 files changed, 90 insertions(+), 165 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index fe4941a..be417ee 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1562,69 +1562,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;
> @@ -1633,25 +1615,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
> @@ -1680,45 +1649,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)
> @@ -1726,86 +1680,58 @@ 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->address and tp->module */
>  	if (tev->uprobes && (!tp->address || !tp->module))
> -		goto error;
> +		return NULL;
> +
> +	strbuf_init(&buf, 32);
> +	strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> +		    tev->group, tev->event);
>  
>  	/* 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
> -		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)
> +		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,
> @@ -1883,7 +1809,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 */
> @@ -1906,9 +1832,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;
> @@ -2086,37 +2012,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_addstr(result, " with");
>  		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 83ee95e..50216ff 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -117,8 +117,7 @@ extern int parse_probe_trace_command(const char *cmd,
>  /* Events to command string */
>  extern char *synthesize_perf_probe_command(struct perf_probe_event *pev);
>  extern char *synthesize_probe_trace_command(struct probe_trace_event *tev);
> -extern int synthesize_perf_probe_arg(struct perf_probe_arg *pa, char *buf,
> -				     size_t len);
> +extern char *synthesize_perf_probe_arg(struct perf_probe_arg *pa);
>  
>  /* Check the perf_probe_event needs debuginfo */
>  extern 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 7b80f8c..0c5168d 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -550,7 +550,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;
>  
>  	if (!is_c_varname(pf->pvar->var)) {
> @@ -575,13 +575,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;
> @@ -1318,8 +1318,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);
>  		}
>  	}
>