Re: [RFC PATCH perf/core v2 03/16] perf probe: Use strbuf for making strings in probe-event.c

[Namhyung Kim <namhyung@kernel.org>]

Hi Masami,

On Wed, Jul 15, 2015 at 06:14:14PM +0900, Masami Hiramatsu wrote:
> Replace many fixed-length char array with strbuf to
> stringify perf_probe_event and probe_trace_event etc. in
> util/probe-event.c.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---

[SNIP]

> +	ret = strbuf_detach(&buf, NULL);
> +	strbuf_release(&buf);

It seems that strbuf_release() is meaningless after strbuf_detach().


>  
> -	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 +1616,14 @@ 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;
> +	tmp = strbuf_detach(&buf, NULL);
> +	strbuf_release(&buf);

Ditto.


> +	return tmp;
>  }


[SNIP]
>  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);

This is necessary for the error path.


> +	return ret;
>  }
>  
>  static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
> @@ -1883,7 +1812,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 +1835,10 @@ 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);
> +			strbuf_release(&buf);

Ditto.

Thanks,
Namhyung

>  		}
>  		if (pev->args[i].name == NULL && ret >= 0)
>  			ret = -ENOMEM;