Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2993000AbbEEQJP (ORCPT ); Tue, 5 May 2015 12:09:15 -0400 Received: from mail.kernel.org ([198.145.29.136]:35209 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2993327AbbEEPMs (ORCPT ); Tue, 5 May 2015 11:12:48 -0400 Date: Tue, 5 May 2015 12:12:42 -0300 From: Arnaldo Carvalho de Melo To: Masami Hiramatsu Cc: Peter Zijlstra , Linux Kernel Mailing List , David Ahern , namhyung@kernel.org, Jiri Olsa , Ingo Molnar Subject: Re: [PATCH perf/core v4 4/4] perf probe: Cleanup and consolidate command parsers Message-ID: <20150505151242.GT10475@kernel.org> References: <20150505022945.23399.993.stgit@localhost.localdomain> <20150505022954.23399.61078.stgit@localhost.localdomain> <5548DC68.5020808@hitachi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5548DC68.5020808@hitachi.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9454 Lines: 278 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 > > --- > > 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, ¶ms.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 = ¶ms.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", ¶ms.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(¶ms.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@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/