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

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/07/17 16:42, Namhyung Kim wrote:
> 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().

Ah, right! I'll remove those redundant strbuf_release.

Thanks!

> 
> 
>>  
>> -	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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com