Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757617AbbGQKRH (ORCPT ); Fri, 17 Jul 2015 06:17:07 -0400 Received: from mail9.hitachi.co.jp ([133.145.228.44]:46168 "EHLO mail9.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751706AbbGQKRF (ORCPT ); Fri, 17 Jul 2015 06:17:05 -0400 Message-ID: <55A8D617.2050803@hitachi.com> Date: Fri, 17 Jul 2015 19:16:55 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Namhyung Kim CC: Arnaldo Carvalho de Melo , Peter Zijlstra , linux-kernel@vger.kernel.org, Adrian Hunter , Ingo Molnar , Paul Mackerras , Jiri Olsa , Borislav Petkov , Hemant Kumar Subject: Re: Re: [RFC PATCH perf/core v2 03/16] perf probe: Use strbuf for making strings in probe-event.c References: <20150715091352.8915.87480.stgit@localhost.localdomain> <20150715091414.8915.15879.stgit@localhost.localdomain> <20150717074205.GB25386@sejong> In-Reply-To: <20150717074205.GB25386@sejong> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6164 Lines: 219 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 >> --- > > [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 -- 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/