Re: [PATCH perf/core v4 4/4] perf probe: Cleanup and consolidate command parsers

[Arnaldo Carvalho de Melo]

Em Wed, May 06, 2015 at 12:06:16AM +0900, Masami Hiramatsu escreveu:
> On 2015/05/05 11:29, Masami Hiramatsu wrote:
> > To simplify the perf-probe command code, consolidate some
> > similar functions and use command short-name for command
> > classification, instead of separated booleans.
> 
> Oops, I've found my mistake on this patch. perf-probe without
> any command but probe defs should imply "--add", but this patch
> drops that.
> I'll update this for v5 patch.

Humm, ok, so I'll drop this one from my perf/core branch now, to keep it
bisectable, ok?

- Arnaldo
 
> Thank you,
> 
> > 
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> > ---
> >  tools/perf/builtin-probe.c |  105 ++++++++++++++++----------------------------
> >  1 file changed, 37 insertions(+), 68 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> > index 08c9481..17870b8 100644
> > --- a/tools/perf/builtin-probe.c
> > +++ b/tools/perf/builtin-probe.c
> > @@ -48,14 +48,10 @@
> >  
> >  /* Session management structure */
> >  static struct {
> > +	int command;	/* Command short_name */
> >  	bool list_events;
> >  	bool force_add;
> > -	bool show_lines;
> > -	bool show_vars;
> >  	bool show_ext_vars;
> > -	bool show_funcs;
> > -	bool mod_events;
> > -	bool del_events;
> >  	bool uprobes;
> >  	bool quiet;
> >  	bool target_used;
> > @@ -175,44 +171,11 @@ static int parse_probe_event_argv(int argc, const char **argv)
> >  
> >  		len += sprintf(&buf[len], "%s ", argv[i]);
> >  	}
> > -	params.mod_events = true;
> >  	ret = parse_probe_event(buf);
> >  	free(buf);
> >  	return ret;
> >  }
> >  
> > -static int opt_add_probe_event(const struct option *opt __maybe_unused,
> > -			      const char *str, int unset __maybe_unused)
> > -{
> > -	if (str) {
> > -		params.mod_events = true;
> > -		return parse_probe_event(str);
> > -	} else
> > -		return 0;
> > -}
> > -
> > -static int opt_del_probe_event(const struct option *opt __maybe_unused,
> > -			       const char *str, int unset __maybe_unused)
> > -{
> > -	if (str) {
> > -		params.del_events = true;
> > -		return params_add_filter(str);
> > -	}
> > -	return 0;
> > -}
> > -
> > -static int opt_list_probe_event(const struct option *opt __maybe_unused,
> > -				const char *str, int unset)
> > -{
> > -	if (!unset)
> > -		params.list_events = true;
> > -
> > -	if (str)
> > -		return params_add_filter(str);
> > -
> > -	return 0;
> > -}
> > -
> >  static int opt_set_target(const struct option *opt, const char *str,
> >  			int unset __maybe_unused)
> >  {
> > @@ -250,8 +213,10 @@ static int opt_set_target(const struct option *opt, const char *str,
> >  	return ret;
> >  }
> >  
> > +/* Command option callbacks */
> > +
> >  #ifdef HAVE_DWARF_SUPPORT
> > -static int opt_show_lines(const struct option *opt __maybe_unused,
> > +static int opt_show_lines(const struct option *opt,
> >  			  const char *str, int unset __maybe_unused)
> >  {
> >  	int ret = 0;
> > @@ -259,19 +224,19 @@ static int opt_show_lines(const struct option *opt __maybe_unused,
> >  	if (!str)
> >  		return 0;
> >  
> > -	if (params.show_lines) {
> > +	if (params.command == 'L') {
> >  		pr_warning("Warning: more than one --line options are"
> >  			   " detected. Only the first one is valid.\n");
> >  		return 0;
> >  	}
> >  
> > -	params.show_lines = true;
> > +	params.command = opt->short_name;
> >  	ret = parse_line_range_desc(str, &params.line_range);
> >  
> >  	return ret;
> >  }
> >  
> > -static int opt_show_vars(const struct option *opt __maybe_unused,
> > +static int opt_show_vars(const struct option *opt,
> >  			 const char *str, int unset __maybe_unused)
> >  {
> >  	struct perf_probe_event *pev = &params.events[params.nevents];
> > @@ -285,16 +250,27 @@ static int opt_show_vars(const struct option *opt __maybe_unused,
> >  		pr_err("  Error: '--vars' doesn't accept arguments.\n");
> >  		return -EINVAL;
> >  	}
> > -	params.show_vars = true;
> > +	params.command = opt->short_name;
> >  
> >  	return ret;
> >  }
> >  #endif
> > -static int opt_show_funcs(const struct option *opt __maybe_unused,
> > -			  const char *str, int unset)
> > +static int opt_add_probe_event(const struct option *opt,
> > +			      const char *str, int unset __maybe_unused)
> > +{
> > +	if (str) {
> > +		params.command = opt->short_name;
> > +		return parse_probe_event(str);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int opt_set_filter_with_command(const struct option *opt,
> > +				       const char *str, int unset)
> >  {
> >  	if (!unset)
> > -		params.show_funcs = true;
> > +		params.command = opt->short_name;
> >  
> >  	if (str)
> >  		return params_add_filter(str);
> > @@ -360,10 +336,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	OPT_BOOLEAN('q', "quiet", &params.quiet,
> >  		    "be quiet (do not show any mesages)"),
> >  	OPT_CALLBACK_DEFAULT('l', "list", NULL, "[GROUP:]EVENT",
> > -			     "list up probe events", opt_list_probe_event,
> > -			     DEFAULT_LIST_FILTER),
> > +			     "list up probe events",
> > +			     opt_set_filter_with_command, DEFAULT_LIST_FILTER),
> >  	OPT_CALLBACK('d', "del", NULL, "[GROUP:]EVENT", "delete a probe event.",
> > -		opt_del_probe_event),
> > +		     opt_set_filter_with_command),
> >  	OPT_CALLBACK('a', "add", NULL,
> >  #ifdef HAVE_DWARF_SUPPORT
> >  		"[EVENT=]FUNC[@SRC][+OFF|%return|:RL|;PT]|SRC:AL|SRC;PT"
> > @@ -412,7 +388,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		 "Set how many probe points can be found for a probe."),
> >  	OPT_CALLBACK_DEFAULT('F', "funcs", NULL, "[FILTER]",
> >  			     "Show potential probe-able functions.",
> > -			     opt_show_funcs, DEFAULT_FUNC_FILTER),
> > +			     opt_set_filter_with_command, DEFAULT_FUNC_FILTER),
> >  	OPT_CALLBACK('\0', "filter", NULL,
> >  		     "[!]FILTER", "Set a filter (with --vars/funcs only)\n"
> >  		     "\t\t\t(default: \"" DEFAULT_VAR_FILTER "\" for --vars,\n"
> > @@ -462,16 +438,13 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	if (params.max_probe_points == 0)
> >  		params.max_probe_points = MAX_PROBES;
> >  
> > -	if ((!params.nevents && !params.del_events && !params.list_events &&
> > -	     !params.show_lines && !params.show_funcs))
> > -		usage_with_options(probe_usage, options);
> > -
> >  	/*
> >  	 * Only consider the user's kernel image path if given.
> >  	 */
> >  	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
> >  
> > -	if (params.list_events) {
> > +	switch (params.command) {
> > +	case 'l':
> >  		if (params.uprobes) {
> >  			pr_warning("  Error: Don't use --list with --exec.\n");
> >  			usage_with_options(probe_usage, options);
> > @@ -480,24 +453,20 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		if (ret < 0)
> >  			pr_err_with_code("  Error: Failed to show event list.", ret);
> >  		return ret;
> > -	}
> > -	if (params.show_funcs) {
> > +	case 'F':
> >  		ret = show_available_funcs(params.target, params.filter,
> >  					params.uprobes);
> >  		if (ret < 0)
> >  			pr_err_with_code("  Error: Failed to show functions.", ret);
> >  		return ret;
> > -	}
> > -
> >  #ifdef HAVE_DWARF_SUPPORT
> > -	if (params.show_lines) {
> > +	case 'L':
> >  		ret = show_line_range(&params.line_range, params.target,
> >  				      params.uprobes);
> >  		if (ret < 0)
> >  			pr_err_with_code("  Error: Failed to show lines.", ret);
> >  		return ret;
> > -	}
> > -	if (params.show_vars) {
> > +	case 'V':
> >  		if (!params.filter)
> >  			params.filter = strfilter__new(DEFAULT_VAR_FILTER,
> >  						       NULL);
> > @@ -510,18 +479,15 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		if (ret < 0)
> >  			pr_err_with_code("  Error: Failed to show vars.", ret);
> >  		return ret;
> > -	}
> >  #endif
> > -
> > -	if (params.del_events) {
> > +	case 'd':
> >  		ret = del_perf_probe_events(params.filter);
> >  		if (ret < 0) {
> >  			pr_err_with_code("  Error: Failed to delete events.", ret);
> >  			return ret;
> >  		}
> > -	}
> > -
> > -	if (params.nevents) {
> > +		break;
> > +	case 'a':
> >  		/* Ensure the last given target is used */
> >  		if (params.target && !params.target_used) {
> >  			pr_warning("  Error: -x/-m must follow the probe definitions.\n");
> > @@ -535,6 +501,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  			pr_err_with_code("  Error: Failed to add events.", ret);
> >  			return ret;
> >  		}
> > +		break;
> > +	default:
> > +		usage_with_options(probe_usage, options);
> >  	}
> >  	return 0;
> >  }
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/