Re: [PATCH perf/core 1/4] [BUGFIX] perf probe: List probes in stdout

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

Em Fri, Jun 12, 2015 at 02:08:13PM +0900, Masami Hiramatsu escreveu:
> Since commit 5e17b28f1e24 ("perf probe: Add --quiet option to
> suppress output result message") have replaced printf with pr_info,
> perf probe -l outputs its result in stderr. However, that is not
> what the commit expected.
> e.g.
>   -----
>   # perf probe -l > /dev/null
>     probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
>   -----
> With this fix,
>   -----
>   # perf probe -l > list
>   # cat list
>     probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)

Nope, with this fix just applied:

[acme@zoo linux]$ rm -f ~/bin/perf;hash -r;type perf;rm -rf /tmp/build/perf ; mkdir -p /tmp/build/perf ; make -C tools/perf O=/tmp/build/perf install-bin > /dev/null; type perf
bash: type: perf: not found
  PERF_VERSION = 4.1.rc5.g3f046e
perf is /home/acme/bin/perf
[acme@zoo linux]$ sudo ~acme/bin/perf probe -l > /tmp/bla
  probe:SYSC_perf_event_open (on SYSC_perf_event_open@linux/kernel/events/core.c)
[acme@zoo linux]$ cat /tmp/bla
[acme@zoo linux]$ git log --oneline | head -1
3f046ed885e1 perf probe: List probes in stdout
acme@zoo linux]$

I see there is one place where this 'use_stdout' thing is always 'false', is
that the problem? Do we really need to have this complexity? Why not plain use
fprintf(stdout)? This is not a informative message, this is the command output,
right?

- Arnaldo

>   -----
> Of course, --quiet(-q) still works on --add/--del.
>   -----
>   # perf probe -q vfs_write
>   # perf probe -l
>     probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
>     probe:vfs_write      (on vfs_write@ksrc/linux-3/fs/read_write.c)
>   -----
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reported-by: Naohiro Aota <naota@elisp.net>
> ---
>  tools/perf/util/probe-event.c |   49 +++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index d4cf50b..710830c 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2126,9 +2126,9 @@ kprobe_blacklist__find_by_address(struct list_head *blacklist,
>  	return NULL;
>  }
>  
> -/* Show an event */
> -static int show_perf_probe_event(struct perf_probe_event *pev,
> -				 const char *module)
> +static int perf_probe_event__sprintf(struct perf_probe_event *pev,
> +				     const char *module,
> +				     struct strbuf *result)
>  {
>  	int i, ret;
>  	char buf[128];
> @@ -2141,27 +2141,47 @@ static int show_perf_probe_event(struct perf_probe_event *pev,
>  
>  	ret = e_snprintf(buf, 128, "%s:%s", pev->group, pev->event);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
> -	pr_info("  %-20s (on %s", buf, place);
> +	strbuf_addf(result, "  %-20s (on %s", buf, place);
>  	if (module)
> -		pr_info(" in %s", module);
> +		strbuf_addf(result, " in %s", module);
>  
>  	if (pev->nargs > 0) {
> -		pr_info(" with");
> +		strbuf_addstr(result, " with");
>  		for (i = 0; i < pev->nargs; i++) {
>  			ret = synthesize_perf_probe_arg(&pev->args[i],
>  							buf, 128);
>  			if (ret < 0)
> -				break;
> -			pr_info(" %s", buf);
> +				goto out;
> +			strbuf_addf(result, " %s", buf);
>  		}
>  	}
> -	pr_info(")\n");
> +	strbuf_addch(result, ')');
> +out:
>  	free(place);
>  	return ret;
>  }
>  
> +/* Show an event */
> +static int show_perf_probe_event(struct perf_probe_event *pev,
> +				 const char *module, bool use_stdout)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int ret;
> +
> +	ret = perf_probe_event__sprintf(pev, module, &buf);
> +	if (ret >= 0) {
> +		if (use_stdout)
> +			printf("%s\n", buf.buf);
> +		else
> +			pr_info("%s\n", buf.buf);
> +	}
> +	strbuf_release(&buf);
> +
> +	return ret;
> +}
> +
>  static bool filter_probe_trace_event(struct probe_trace_event *tev,
>  				     struct strfilter *filter)
>  {
> @@ -2200,9 +2220,10 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
>  				goto next;
>  			ret = convert_to_perf_probe_event(&tev, &pev,
>  								is_kprobe);
> -			if (ret >= 0)
> -				ret = show_perf_probe_event(&pev,
> -							    tev.point.module);
> +			if (ret < 0)
> +				goto next;
> +			ret = show_perf_probe_event(&pev, tev.point.module,
> +						    false);
>  		}
>  next:
>  		clear_perf_probe_event(&pev);
> @@ -2463,7 +2484,7 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  		group = pev->group;
>  		pev->event = tev->event;
>  		pev->group = tev->group;
> -		show_perf_probe_event(pev, tev->point.module);
> +		show_perf_probe_event(pev, tev->point.module, true);
>  		/* Trick here - restore current event/group */
>  		pev->event = (char *)event;
>  		pev->group = (char *)group;
>