Re: [PATCH perf/core ] perf-probe: Output the result of adding/deleting probe in buildin-probe

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Sep 03, 2015 at 12:18:28PM +0000, 平松雅巳 / HIRAMATU，MASAMI wrote:
> Hi Namhyung,
> 
> So, I hope this would be what you've suggested.
> 
> Thank you,

Hi Masami,

I think something different, but this can be ok.  Anyway, I'll send my
idea soon..

Thanks,
Namhyung


> 
> -- 
> 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
> 
> 
> > -----Original Message-----
> > From: Masami Hiramatsu [mailto:masami.hiramatsu.pt@hitachi.com]
> > Sent: Thursday, September 03, 2015 9:11 PM
> > To: Namhyung Kim; Arnaldo Carvalho de Melo
> > Cc: Wang Nan; Kaixu Xia; Peter Zijlstra; Daniel Borkmann; linux-kernel@vger.kernel.org; He Kuang; lizefan@huawei.com;
> > Jiri Olsa; David Ahern; Brendan Gregg; mingo@kernel.org; ast@plumgrid.com
> > Subject: [PATCH perf/core ] perf-probe: Output the result of adding/deleting probe in buildin-probe
> > 
> > Output the normal result of adding/deleting probe in buildin-probe
> > instead of showing it by add/del_perf_probe_events.
> > All the result string is stored into "result" strbuf parameter.
> > If you want to ignore the result string, pass a NULL to the "result".
> > Note that all warning/debug strings are still in the
> > add/del_perf_probe_events.
> > 
> > Suggested-by: Namhyung Kim <namhyung@gmail.com>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > ---
> >  tools/perf/builtin-probe.c    |    9 +++++++--
> >  tools/perf/util/probe-event.c |   33 ++++++++++++++++++++-------------
> >  tools/perf/util/probe-event.h |    6 ++++--
> >  tools/perf/util/probe-file.c  |    5 +++--
> >  tools/perf/util/probe-file.h  |    4 +++-
> >  5 files changed, 37 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> > index b81cec3..d11ad21 100644
> > --- a/tools/perf/builtin-probe.c
> > +++ b/tools/perf/builtin-probe.c
> > @@ -402,6 +402,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		    "Enable kernel symbol demangling"),
> >  	OPT_END()
> >  	};
> > +	struct strbuf buf = STRBUF_INIT;
> >  	int ret;
> > 
> >  	set_option_flag(options, 'a', "add", PARSE_OPT_EXCLUSIVE);
> > @@ -483,7 +484,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		return ret;
> >  #endif
> >  	case 'd':
> > -		ret = del_perf_probe_events(params.filter);
> > +		ret = del_perf_probe_events(params.filter, &buf);
> > +		/* Even if failed, we should show the result first */
> > +		pr_info("%s", buf.buf);
> >  		if (ret < 0) {
> >  			pr_err_with_code("  Error: Failed to delete events.", ret);
> >  			return ret;
> > @@ -496,7 +499,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  			usage_with_options(probe_usage, options);
> >  		}
> > 
> > -		ret = add_perf_probe_events(params.events, params.nevents);
> > +		ret = add_perf_probe_events(params.events, params.nevents, &buf);
> > +		/* Even if failed, we should show the result first */
> > +		pr_info("%s", buf.buf);
> >  		if (ret < 0) {
> >  			pr_err_with_code("  Error: Failed to add events.", ret);
> >  			return ret;
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index eb5f18b..1a3ed7c 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2395,7 +2395,8 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
> > 
> >  static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  				     struct probe_trace_event *tevs,
> > -				     int ntevs, bool allow_suffix)
> > +				     int ntevs, bool allow_suffix,
> > +				     struct strbuf *buf)
> >  {
> >  	int i, fd, ret;
> >  	struct probe_trace_event *tev = NULL;
> > @@ -2415,7 +2416,9 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  	}
> > 
> >  	ret = 0;
> > -	pr_info("Added new event%s\n", (ntevs > 1) ? "s:" : ":");
> > +	if (buf)
> > +		strbuf_addf(buf, "Added new event%s\n",
> > +			    (ntevs > 1) ? "s:" : ":");
> >  	for (i = 0; i < ntevs; i++) {
> >  		tev = &tevs[i];
> >  		/* Skip if the symbol is out of .text or blacklisted */
> > @@ -2432,9 +2435,12 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  		if (ret < 0)
> >  			break;
> > 
> > -		/* We use tev's name for showing new events */
> > -		show_perf_probe_event(tev->group, tev->event, pev,
> > -				      tev->point.module, false);
> > +		if (buf) {
> > +			/* We use tev's name for showing new events */
> > +			perf_probe_event__sprintf(tev->group, tev->event,
> > +						  pev, tev->point.module, buf);
> > +			strbuf_addch(buf, '\n');
> > +		}
> >  		/* Save the last valid name */
> >  		event = tev->event;
> >  		group = tev->group;
> > @@ -2451,10 +2457,10 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  		warn_uprobe_event_compat(tev);
> > 
> >  	/* Note that it is possible to skip all events because of blacklist */
> > -	if (ret >= 0 && event) {
> > +	if (ret >= 0 && event && buf) {
> >  		/* Show how to use the event. */
> > -		pr_info("\nYou can now use it in all perf tools, such as:\n\n");
> > -		pr_info("\tperf record -e %s:%s -aR sleep 1\n\n", group, event);
> > +		strbuf_addf(buf, "\nYou can now use it in all perf tools, such as:\n\n");
> > +		strbuf_addf(buf, "\tperf record -e %s:%s -aR sleep 1\n\n", group, event);
> >  	}
> > 
> >  	strlist__delete(namelist);
> > @@ -2765,7 +2771,8 @@ struct __event_package {
> >  	int				ntevs;
> >  };
> > 
> > -int add_perf_probe_events(struct perf_probe_event *pevs, int npevs)
> > +int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
> > +			  struct strbuf *result)
> >  {
> >  	int i, j, ret;
> >  	struct __event_package *pkgs;
> > @@ -2802,7 +2809,7 @@ int add_perf_probe_events(struct perf_probe_event *pevs, int npevs)
> >  	for (i = 0; i < npevs; i++) {
> >  		ret = __add_probe_trace_events(pkgs[i].pev, pkgs[i].tevs,
> >  					       pkgs[i].ntevs,
> > -					       probe_conf.force_add);
> > +					       probe_conf.force_add, result);
> >  		if (ret < 0)
> >  			break;
> >  	}
> > @@ -2819,7 +2826,7 @@ end:
> >  	return ret;
> >  }
> > 
> > -int del_perf_probe_events(struct strfilter *filter)
> > +int del_perf_probe_events(struct strfilter *filter, struct strbuf *result)
> >  {
> >  	int ret, ret2, ufd = -1, kfd = -1;
> >  	char *str = strfilter__string(filter);
> > @@ -2834,11 +2841,11 @@ int del_perf_probe_events(struct strfilter *filter)
> >  	if (ret < 0)
> >  		goto out;
> > 
> > -	ret = probe_file__del_events(kfd, filter);
> > +	ret = probe_file__del_events(kfd, filter, result);
> >  	if (ret < 0 && ret != -ENOENT)
> >  		goto error;
> > 
> > -	ret2 = probe_file__del_events(ufd, filter);
> > +	ret2 = probe_file__del_events(ufd, filter, result);
> >  	if (ret2 < 0 && ret2 != -ENOENT) {
> >  		ret = ret2;
> >  		goto error;
> > diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> > index 6e7ec68..9855dbf 100644
> > --- a/tools/perf/util/probe-event.h
> > +++ b/tools/perf/util/probe-event.h
> > @@ -137,8 +137,10 @@ extern void line_range__clear(struct line_range *lr);
> >  /* Initialize line range */
> >  extern int line_range__init(struct line_range *lr);
> > 
> > -extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs);
> > -extern int del_perf_probe_events(struct strfilter *filter);
> > +extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
> > +				 struct strbuf *result);
> > +extern int del_perf_probe_events(struct strfilter *filter,
> > +				 struct strbuf *result);
> >  extern int show_perf_probe_events(struct strfilter *filter);
> >  extern int show_line_range(struct line_range *lr, const char *module,
> >  			   bool user);
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index bbb2437..e22fa12 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -267,7 +267,6 @@ static int __del_trace_probe_event(int fd, struct str_node *ent)
> >  		goto error;
> >  	}
> > 
> > -	pr_info("Removed event: %s\n", ent->s);
> >  	return 0;
> >  error:
> >  	pr_warning("Failed to delete event: %s\n",
> > @@ -275,7 +274,7 @@ error:
> >  	return ret;
> >  }
> > 
> > -int probe_file__del_events(int fd, struct strfilter *filter)
> > +int probe_file__del_events(int fd, struct strfilter *filter, struct strbuf *buf)
> >  {
> >  	struct strlist *namelist;
> >  	struct str_node *ent;
> > @@ -293,6 +292,8 @@ int probe_file__del_events(int fd, struct strfilter *filter)
> >  			ret = __del_trace_probe_event(fd, ent);
> >  			if (ret < 0)
> >  				break;
> > +			if (buf)
> > +				strbuf_addf(buf, "Removed event: %s\n", ent->s);
> >  		}
> >  	}
> >  	strlist__delete(namelist);
> > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> > index ada94a2..ee89ef0 100644
> > --- a/tools/perf/util/probe-file.h
> > +++ b/tools/perf/util/probe-file.h
> > @@ -1,6 +1,7 @@
> >  #ifndef __PROBE_FILE_H
> >  #define __PROBE_FILE_H
> > 
> > +#include "strbuf.h"
> >  #include "strlist.h"
> >  #include "strfilter.h"
> >  #include "probe-event.h"
> > @@ -13,6 +14,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag);
> >  struct strlist *probe_file__get_namelist(int fd);
> >  struct strlist *probe_file__get_rawlist(int fd);
> >  int probe_file__add_event(int fd, struct probe_trace_event *tev);
> > -int probe_file__del_events(int fd, struct strfilter *filter);
> > +int probe_file__del_events(int fd, struct strfilter *filter,
> > +			   struct strbuf *buf);
> > 
> >  #endif
> > 
>