[PATCH 1/4] perf tools: Improve ambiguous option help message

[PATCH 1/4] perf tools: Improve ambiguous option help message

[Namhyung Kim]

Currently if an option name is ambiguous it only prints first two
matched option names but no help.  It'd be better it could show all
possible names and help messages too.

Before:
  $ perf report --show
    Error: Ambiguous option: show (could be --show-total-period or
                                            --show-ref-call-graph)
   Usage: perf report [<options>]

After:
  $ perf report --show
    Error: Ambiguous option: show (could be --show-total-period or
                                            --show-ref-call-graph)
   Usage: perf report [<options>]

      -n, --show-nr-samples
                              Show a column with the number of samples
          --showcpuutilization
                              Show sample percentage for different cpu modes
      -I, --show-info         Display extended information about perf.data file
          --show-total-period
                              Show a column with the sum of periods
          --show-ref-call-graph
                              Show callgraph from reference event

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/parse-options.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 22c2806bda98..b8d98229a8af 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -770,24 +770,23 @@ int parse_options_usage(const char * const *usagestr,
 opt:
 	for (  ; opts->type != OPTION_END; opts++) {
 		if (short_opt) {
-			if (opts->short_name == *optstr)
+			if (opts->short_name == *optstr) {
+				print_option_help(opts, 0);
 				break;
+			}
 			continue;
 		}
 
 		if (opts->long_name == NULL)
 			continue;
 
-		if (!prefixcmp(optstr, opts->long_name))
-			break;
-		if (!prefixcmp(optstr, "no-") &&
-		    !prefixcmp(optstr + 3, opts->long_name))
-			break;
+		if (!prefixcmp(opts->long_name, optstr))
+			print_option_help(opts, 0);
+		if (!prefixcmp("no-", optstr) &&
+		    !prefixcmp(opts->long_name, optstr + 3))
+			print_option_help(opts, 0);
 	}
 
-	if (opts->type != OPTION_END)
-		print_option_help(opts, 0);
-
 	return PARSE_OPT_HELP;
 }
 
-- 
2.6.0

[PATCH 2/4] perf report: Rename to --show-cpu-utilization

[Namhyung Kim]

So that it can be more consistent with other --show-* options.  The old
name (--showcpuutilization) is provided only for compatibility.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt | 2 +-
 tools/perf/builtin-report.c              | 4 +++-
 tools/perf/util/parse-options.h          | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index ab1fd64e3627..5ce8da1e1256 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -29,7 +29,7 @@ OPTIONS
 --show-nr-samples::
 	Show the number of samples for each symbol
 
---showcpuutilization::
+--show-cpu-utilization::
         Show sample percentage for different cpu modes.
 
 -T::
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 50dd4d3d8667..2853ad2bd435 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -699,8 +699,10 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		   " Please refer the man page for the complete list."),
 	OPT_STRING('F', "fields", &field_order, "key[,keys...]",
 		   "output field(s): overhead, period, sample plus all of sort keys"),
-	OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
+	OPT_BOOLEAN(0, "show-cpu-utilization", &symbol_conf.show_cpu_utilization,
 		    "Show sample percentage for different cpu modes"),
+	OPT_BOOLEAN_FLAG(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
+		    "Show sample percentage for different cpu modes", PARSE_OPT_HIDDEN),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
 		   "regex filter to identify parent, see: '--sort parent'"),
 	OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 367d8b816cc7..182c86099330 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -111,6 +111,7 @@ struct option {
 #define OPT_GROUP(h)                { .type = OPTION_GROUP, .help = (h) }
 #define OPT_BIT(s, l, v, h, b)      { .type = OPTION_BIT, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h), .defval = (b) }
 #define OPT_BOOLEAN(s, l, v, h)     { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h) }
+#define OPT_BOOLEAN_FLAG(s, l, v, h, f)     { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h), .flags = (f) }
 #define OPT_BOOLEAN_SET(s, l, v, os, h) \
 	{ .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), \
 	.value = check_vtype(v, bool *), .help = (h), \
-- 
2.6.0

[PATCH 3/4] perf tools: Setup pager when printing usage and help

[Namhyung Kim]

It's annoying to see error or help message when command has many options
like in perf record, report or top.  So setup pager when print parser
error or help message - it should be OK since no UI is enabled at the
parsing time.  The usage_with_options() already disables it by calling
exit_browser() anyway.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/parse-options.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index b8d98229a8af..eeeed98eb26d 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -7,6 +7,8 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
+static struct strbuf error_buf = STRBUF_INIT;
+
 static int opterror(const struct option *opt, const char *reason, int flags)
 {
 	if (flags & OPT_SHORT)
@@ -540,9 +542,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 		exit(130);
 	default: /* PARSE_OPT_UNKNOWN */
 		if (ctx.argv[0][1] == '-') {
-			error("unknown option `%s'", ctx.argv[0] + 2);
+			strbuf_addf(&error_buf, "unknown option `%s'",
+				    ctx.argv[0] + 2);
 		} else {
-			error("unknown switch `%c'", *ctx.opt);
+			strbuf_addf(&error_buf, "unknown switch `%c'",
+				    *ctx.opt);
 		}
 		usage_with_options(usagestr, options);
 	}
@@ -711,6 +715,13 @@ int usage_with_options_internal(const char * const *usagestr,
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
+	setup_pager();
+
+	if (strbuf_avail(&error_buf)) {
+		fprintf(stderr, "  Error: %s\n", error_buf.buf);
+		strbuf_release(&error_buf);
+	}
+
 	fprintf(stderr, "\n Usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
 		fprintf(stderr, "    or: %s\n", *usagestr++);
-- 
2.6.0

[PATCH 4/4] perf tools: Introduce usage_with_options_msg()

[Namhyung Kim]

Now usage_with_options() setup a pager before printing message so normal
printf() or pr_err() will not be shown.  The usage_with_options_msg()
can be used to print some help message before usage strings.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-evlist.c     |  4 ++--
 tools/perf/builtin-probe.c      | 20 ++++++++++++--------
 tools/perf/builtin-record.c     | 11 ++++++-----
 tools/perf/builtin-sched.c      |  4 ++--
 tools/perf/builtin-script.c     |  8 ++++----
 tools/perf/util/parse-options.c | 15 +++++++++++++++
 tools/perf/util/parse-options.h |  4 ++++
 tools/perf/util/strbuf.c        | 22 +++++++++++++++-------
 tools/perf/util/strbuf.h        |  2 ++
 9 files changed, 62 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 695ec5a50cf2..f4d62510acbb 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -61,8 +61,8 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(evlist_usage, options);
 
 	if (details.event_group && (details.verbose || details.freq)) {
-		pr_err("--group option is not compatible with other options\n");
-		usage_with_options(evlist_usage, options);
+		usage_with_options_msg(evlist_usage, options,
+			"--group option is not compatible with other options\n");
 	}
 
 	return __cmd_evlist(input_name, &details);
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 530c3a28a58c..132afc97676c 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -528,12 +528,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	if (argc > 0) {
 		if (strcmp(argv[0], "-") == 0) {
-			pr_warning("  Error: '-' is not supported.\n");
-			usage_with_options(probe_usage, options);
+			usage_with_options_msg(probe_usage, options,
+				"'-' is not supported.\n");
 		}
 		if (params.command && params.command != 'a') {
-			pr_warning("  Error: another command except --add is set.\n");
-			usage_with_options(probe_usage, options);
+			usage_with_options_msg(probe_usage, options,
+				"another command except --add is set.\n");
 		}
 		ret = parse_probe_event_argv(argc, argv);
 		if (ret < 0) {
@@ -562,8 +562,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	switch (params.command) {
 	case 'l':
 		if (params.uprobes) {
-			pr_warning("  Error: Don't use --list with --exec.\n");
-			usage_with_options(probe_usage, options);
+			pr_err("  Error: Don't use --list with --exec.\n");
+			parse_options_usage(probe_usage, options, "l", true);
+			parse_options_usage(NULL, options, "x", true);
+			return -EINVAL;
 		}
 		ret = show_perf_probe_events(params.filter);
 		if (ret < 0)
@@ -603,8 +605,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	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");
-			usage_with_options(probe_usage, options);
+			pr_err("  Error: -x/-m must follow the probe definitions.\n");
+			parse_options_usage(probe_usage, options, "m", true);
+			parse_options_usage(NULL, options, "x", true);
+			return -EINVAL;
 		}
 
 		ret = perf_add_probe_events(params.events, params.nevents);
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2740d7a82ae8..de02267c73d8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1135,14 +1135,15 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(record_usage, record_options);
 
 	if (nr_cgroups && !rec->opts.target.system_wide) {
-		ui__error("cgroup monitoring only available in"
-			  " system-wide mode\n");
-		usage_with_options(record_usage, record_options);
+		usage_with_options_msg(record_usage, record_options,
+			"cgroup monitoring only available in system-wide mode");
+
 	}
 	if (rec->opts.record_switch_events &&
 	    !perf_can_record_switch_events()) {
-		ui__error("kernel does not support recording context switch events (--switch-events option)\n");
-		usage_with_options(record_usage, record_options);
+		ui__error("kernel does not support recording context switch events\n");
+		parse_options_usage(record_usage, record_options, "switch-events", 0);
+		return -EINVAL;
 	}
 
 	if (!rec->itr) {
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 33962612a5e9..0ee6d900e100 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1728,8 +1728,8 @@ static void setup_sorting(struct perf_sched *sched, const struct option *options
 	for (tok = strtok_r(str, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		if (sort_dimension__add(tok, &sched->sort_list) < 0) {
-			error("Unknown --sort key: `%s'", tok);
-			usage_with_options(usage_msg, options);
+			usage_with_options_msg(usage_msg, options,
+					"Unknown --sort key: `%s'", tok);
 		}
 	}
 
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 2653c0273b89..278acb22f029 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1767,9 +1767,9 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
 		rep_script_path = get_script_path(argv[0], REPORT_SUFFIX);
 
 		if (!rec_script_path && !rep_script_path) {
-			fprintf(stderr, " Couldn't find script %s\n\n See perf"
+			usage_with_options_msg(script_usage, options,
+				"Couldn't find script `%s'\n\n See perf"
 				" script -l for available scripts.\n", argv[0]);
-			usage_with_options(script_usage, options);
 		}
 
 		if (is_top_script(argv[0])) {
@@ -1780,10 +1780,10 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
 			rep_args = has_required_arg(rep_script_path);
 			rec_args = (argc - 1) - rep_args;
 			if (rec_args < 0) {
-				fprintf(stderr, " %s script requires options."
+				usage_with_options_msg(script_usage, options,
+					"`%s' script requires options."
 					"\n\n See perf script -l for available "
 					"scripts and options.\n", argv[0]);
-				usage_with_options(script_usage, options);
 			}
 		}
 
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index eeeed98eb26d..230e771407a3 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -760,6 +760,21 @@ void usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void usage_with_options_msg(const char * const *usagestr,
+			    const struct option *opts, const char *fmt, ...)
+{
+	va_list ap;
+
+	exit_browser(false);
+
+	va_start(ap, fmt);
+	strbuf_addv(&error_buf, fmt, ap);
+	va_end(ap);
+
+	usage_with_options_internal(usagestr, opts, 0, NULL);
+	exit(129);
+}
+
 int parse_options_usage(const char * const *usagestr,
 			const struct option *opts,
 			const char *optstr, bool short_opt)
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 182c86099330..a8e407bc251e 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -161,6 +161,10 @@ extern int parse_options_subcommand(int argc, const char **argv,
 
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
+extern NORETURN __attribute__((format(printf,3,4)))
+void usage_with_options_msg(const char * const *usagestr,
+			    const struct option *options,
+			    const char *fmt, ...);
 
 /*----- incremantal advanced APIs -----*/
 
diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
index 4abe23550c73..25671fa16618 100644
--- a/tools/perf/util/strbuf.c
+++ b/tools/perf/util/strbuf.c
@@ -82,23 +82,22 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }
 
-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
 {
 	int len;
-	va_list ap;
+	va_list ap_saved;
 
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
-	va_start(ap, fmt);
+
+	va_copy(ap_saved, ap);
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-	va_end(ap);
 	if (len < 0)
 		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
-		va_start(ap, fmt);
-		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-		va_end(ap);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap_saved);
+		va_end(ap_saved);
 		if (len > strbuf_avail(sb)) {
 			die("this should not happen, your vsnprintf is broken");
 		}
@@ -106,6 +105,15 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	strbuf_addv(sb, fmt, ap);
+	va_end(ap);
+}
+
 ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
 {
 	size_t oldlen = sb->len;
diff --git a/tools/perf/util/strbuf.h b/tools/perf/util/strbuf.h
index 436ac319f6c7..529f2f035249 100644
--- a/tools/perf/util/strbuf.h
+++ b/tools/perf/util/strbuf.h
@@ -39,6 +39,7 @@
  */
 
 #include <assert.h>
+#include <stdarg.h>
 
 extern char strbuf_slopbuf[];
 struct strbuf {
@@ -85,6 +86,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);
 
 /* XXX: if read fails, any partial read is undone */
 extern ssize_t strbuf_read(struct strbuf *, int fd, ssize_t hint);
-- 
2.6.0

Re: [PATCH 1/4] perf tools: Improve ambiguous option help message

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Currently if an option name is ambiguous it only prints first two
> matched option names but no help.  It'd be better it could show all
> possible names and help messages too.
> 
> Before:
>   $ perf report --show
>     Error: Ambiguous option: show (could be --show-total-period or
>                                             --show-ref-call-graph)
>    Usage: perf report [<options>]
> 
> After:
>   $ perf report --show
>     Error: Ambiguous option: show (could be --show-total-period or
>                                             --show-ref-call-graph)
>    Usage: perf report [<options>]
> 
>       -n, --show-nr-samples
>                               Show a column with the number of samples
>           --showcpuutilization
>                               Show sample percentage for different cpu modes
>       -I, --show-info         Display extended information about perf.data file
>           --show-total-period
>                               Show a column with the sum of periods
>           --show-ref-call-graph
>                               Show callgraph from reference event

Very nice touch!

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 2/4] perf report: Rename to --show-cpu-utilization

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> So that it can be more consistent with other --show-* options.  The old
> name (--showcpuutilization) is provided only for compatibility.

1)

Btw., maybe we could enhance the option parser to strip all (non-leading) dashes 
from long-form option names? That way both variants would work naturally, and if 
someone thinks it's called '--show-cpuutilization' that would work as well.

2)

Also, another enhancement would be to allow partial matches, so that --showcpu 
would match on the first long-form option name that matches.

Right now we do:

  triton:~/tip> perf report -h --showcpu

   Usage: perf report [<options>]


  triton:~/tip> 

Which arguably isn't very helpful! :-)

3)

The only drawback of doing partial matches would be if we introduce new variants - 
but we'd still 'break' safely: if for example --show-cpu-usage is introduced in a 
couple of years, then previous usage of:

   perf report --showcpu

would emit your ambiguous-options warning that you improved in this series. We'd 
output the two options that match and the user could adjust the parameter to 
whichever he meant.

It would still very smooth behavior from a UI ergonomy POV IMHO.

4)

Another possible tweak would be to print a non-fatal info line when a user didn't 
use the canonical form of the option. So writing:

   perf report --showcpu

would result in (stdout) output like this:

   # Option parser: '--showcpu' matched on '--show-cpu-utilization'

The disadvantage of this would be the extra nuisance factor, so I'm not sure we 
want this aspect.

Thanks,

	Ingo

Re: [PATCH 3/4] perf tools: Setup pager when printing usage and help

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> It's annoying to see error or help message when command has many options like in 
> perf record, report or top. [...]

That's indeed so.

> [...]  So setup pager when print parser error or help message - it should be OK 
> since no UI is enabled at the parsing time.  The usage_with_options() already 
> disables it by calling exit_browser() anyway.

So I wanted to write that this has a disadvantage as well, that the pager disrupts 
the regular 'timeline', 'append-only' visual flow of a regular shell workflow.

But then I tried your patches, and for short outputs (such as 'perf stat -h') the 
pager is not activated, while for longer output (such as 'perf report -h') it's 
activated.

That's very intuitive:

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 4/4] perf tools: Introduce usage_with_options_msg()

[Arnaldo Carvalho de Melo]

Em Sun, Oct 25, 2015 at 12:49:27AM +0900, Namhyung Kim escreveu:
> Now usage_with_options() setup a pager before printing message so normal
> printf() or pr_err() will not be shown.  The usage_with_options_msg()
> can be used to print some help message before usage strings.

Haven't tested, but intent looks good, Masami, could you please check if
all is ok and provide an Acked-by and/or Tested-by?

- Arnaldo
 
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-evlist.c     |  4 ++--
>  tools/perf/builtin-probe.c      | 20 ++++++++++++--------
>  tools/perf/builtin-record.c     | 11 ++++++-----
>  tools/perf/builtin-sched.c      |  4 ++--
>  tools/perf/builtin-script.c     |  8 ++++----
>  tools/perf/util/parse-options.c | 15 +++++++++++++++
>  tools/perf/util/parse-options.h |  4 ++++
>  tools/perf/util/strbuf.c        | 22 +++++++++++++++-------
>  tools/perf/util/strbuf.h        |  2 ++
>  9 files changed, 62 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> index 695ec5a50cf2..f4d62510acbb 100644
> --- a/tools/perf/builtin-evlist.c
> +++ b/tools/perf/builtin-evlist.c
> @@ -61,8 +61,8 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
>  		usage_with_options(evlist_usage, options);
>  
>  	if (details.event_group && (details.verbose || details.freq)) {
> -		pr_err("--group option is not compatible with other options\n");
> -		usage_with_options(evlist_usage, options);
> +		usage_with_options_msg(evlist_usage, options,
> +			"--group option is not compatible with other options\n");
>  	}
>  
>  	return __cmd_evlist(input_name, &details);
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 530c3a28a58c..132afc97676c 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -528,12 +528,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
>  	if (argc > 0) {
>  		if (strcmp(argv[0], "-") == 0) {
> -			pr_warning("  Error: '-' is not supported.\n");
> -			usage_with_options(probe_usage, options);
> +			usage_with_options_msg(probe_usage, options,
> +				"'-' is not supported.\n");
>  		}
>  		if (params.command && params.command != 'a') {
> -			pr_warning("  Error: another command except --add is set.\n");
> -			usage_with_options(probe_usage, options);
> +			usage_with_options_msg(probe_usage, options,
> +				"another command except --add is set.\n");
>  		}
>  		ret = parse_probe_event_argv(argc, argv);
>  		if (ret < 0) {
> @@ -562,8 +562,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>  	switch (params.command) {
>  	case 'l':
>  		if (params.uprobes) {
> -			pr_warning("  Error: Don't use --list with --exec.\n");
> -			usage_with_options(probe_usage, options);
> +			pr_err("  Error: Don't use --list with --exec.\n");
> +			parse_options_usage(probe_usage, options, "l", true);
> +			parse_options_usage(NULL, options, "x", true);
> +			return -EINVAL;
>  		}
>  		ret = show_perf_probe_events(params.filter);
>  		if (ret < 0)
> @@ -603,8 +605,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>  	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");
> -			usage_with_options(probe_usage, options);
> +			pr_err("  Error: -x/-m must follow the probe definitions.\n");
> +			parse_options_usage(probe_usage, options, "m", true);
> +			parse_options_usage(NULL, options, "x", true);
> +			return -EINVAL;
>  		}
>  
>  		ret = perf_add_probe_events(params.events, params.nevents);
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 2740d7a82ae8..de02267c73d8 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1135,14 +1135,15 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  		usage_with_options(record_usage, record_options);
>  
>  	if (nr_cgroups && !rec->opts.target.system_wide) {
> -		ui__error("cgroup monitoring only available in"
> -			  " system-wide mode\n");
> -		usage_with_options(record_usage, record_options);
> +		usage_with_options_msg(record_usage, record_options,
> +			"cgroup monitoring only available in system-wide mode");
> +
>  	}
>  	if (rec->opts.record_switch_events &&
>  	    !perf_can_record_switch_events()) {
> -		ui__error("kernel does not support recording context switch events (--switch-events option)\n");
> -		usage_with_options(record_usage, record_options);
> +		ui__error("kernel does not support recording context switch events\n");
> +		parse_options_usage(record_usage, record_options, "switch-events", 0);
> +		return -EINVAL;
>  	}
>  
>  	if (!rec->itr) {
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 33962612a5e9..0ee6d900e100 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1728,8 +1728,8 @@ static void setup_sorting(struct perf_sched *sched, const struct option *options
>  	for (tok = strtok_r(str, ", ", &tmp);
>  			tok; tok = strtok_r(NULL, ", ", &tmp)) {
>  		if (sort_dimension__add(tok, &sched->sort_list) < 0) {
> -			error("Unknown --sort key: `%s'", tok);
> -			usage_with_options(usage_msg, options);
> +			usage_with_options_msg(usage_msg, options,
> +					"Unknown --sort key: `%s'", tok);
>  		}
>  	}
>  
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 2653c0273b89..278acb22f029 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1767,9 +1767,9 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
>  		rep_script_path = get_script_path(argv[0], REPORT_SUFFIX);
>  
>  		if (!rec_script_path && !rep_script_path) {
> -			fprintf(stderr, " Couldn't find script %s\n\n See perf"
> +			usage_with_options_msg(script_usage, options,
> +				"Couldn't find script `%s'\n\n See perf"
>  				" script -l for available scripts.\n", argv[0]);
> -			usage_with_options(script_usage, options);
>  		}
>  
>  		if (is_top_script(argv[0])) {
> @@ -1780,10 +1780,10 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
>  			rep_args = has_required_arg(rep_script_path);
>  			rec_args = (argc - 1) - rep_args;
>  			if (rec_args < 0) {
> -				fprintf(stderr, " %s script requires options."
> +				usage_with_options_msg(script_usage, options,
> +					"`%s' script requires options."
>  					"\n\n See perf script -l for available "
>  					"scripts and options.\n", argv[0]);
> -				usage_with_options(script_usage, options);
>  			}
>  		}
>  
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index eeeed98eb26d..230e771407a3 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -760,6 +760,21 @@ void usage_with_options(const char * const *usagestr,
>  	exit(129);
>  }
>  
> +void usage_with_options_msg(const char * const *usagestr,
> +			    const struct option *opts, const char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	exit_browser(false);
> +
> +	va_start(ap, fmt);
> +	strbuf_addv(&error_buf, fmt, ap);
> +	va_end(ap);
> +
> +	usage_with_options_internal(usagestr, opts, 0, NULL);
> +	exit(129);
> +}
> +
>  int parse_options_usage(const char * const *usagestr,
>  			const struct option *opts,
>  			const char *optstr, bool short_opt)
> diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> index 182c86099330..a8e407bc251e 100644
> --- a/tools/perf/util/parse-options.h
> +++ b/tools/perf/util/parse-options.h
> @@ -161,6 +161,10 @@ extern int parse_options_subcommand(int argc, const char **argv,
>  
>  extern NORETURN void usage_with_options(const char * const *usagestr,
>                                          const struct option *options);
> +extern NORETURN __attribute__((format(printf,3,4)))
> +void usage_with_options_msg(const char * const *usagestr,
> +			    const struct option *options,
> +			    const char *fmt, ...);
>  
>  /*----- incremantal advanced APIs -----*/
>  
> diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
> index 4abe23550c73..25671fa16618 100644
> --- a/tools/perf/util/strbuf.c
> +++ b/tools/perf/util/strbuf.c
> @@ -82,23 +82,22 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
>  	strbuf_setlen(sb, sb->len + len);
>  }
>  
> -void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> +void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
>  {
>  	int len;
> -	va_list ap;
> +	va_list ap_saved;
>  
>  	if (!strbuf_avail(sb))
>  		strbuf_grow(sb, 64);
> -	va_start(ap, fmt);
> +
> +	va_copy(ap_saved, ap);
>  	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> -	va_end(ap);
>  	if (len < 0)
>  		die("your vsnprintf is broken");
>  	if (len > strbuf_avail(sb)) {
>  		strbuf_grow(sb, len);
> -		va_start(ap, fmt);
> -		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> -		va_end(ap);
> +		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap_saved);
> +		va_end(ap_saved);
>  		if (len > strbuf_avail(sb)) {
>  			die("this should not happen, your vsnprintf is broken");
>  		}
> @@ -106,6 +105,15 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
>  	strbuf_setlen(sb, sb->len + len);
>  }
>  
> +void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	va_start(ap, fmt);
> +	strbuf_addv(sb, fmt, ap);
> +	va_end(ap);
> +}
> +
>  ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
>  {
>  	size_t oldlen = sb->len;
> diff --git a/tools/perf/util/strbuf.h b/tools/perf/util/strbuf.h
> index 436ac319f6c7..529f2f035249 100644
> --- a/tools/perf/util/strbuf.h
> +++ b/tools/perf/util/strbuf.h
> @@ -39,6 +39,7 @@
>   */
>  
>  #include <assert.h>
> +#include <stdarg.h>
>  
>  extern char strbuf_slopbuf[];
>  struct strbuf {
> @@ -85,6 +86,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
>  
>  __attribute__((format(printf,2,3)))
>  extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
> +extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);
>  
>  /* XXX: if read fails, any partial read is undone */
>  extern ssize_t strbuf_read(struct strbuf *, int fd, ssize_t hint);
> -- 
> 2.6.0

RE: [PATCH 4/4] perf tools: Introduce usage_with_options_msg()

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 10383 bytes --]

>From: Namhyung Kim [mailto:namhyung@gmail.com] On Behalf Of Namhyung Kim
>
>Now usage_with_options() setup a pager before printing message so normal
>printf() or pr_err() will not be shown.  The usage_with_options_msg()
>can be used to print some help message before usage strings.

Thanks! looks good to me  :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>---
> tools/perf/builtin-evlist.c     |  4 ++--
> tools/perf/builtin-probe.c      | 20 ++++++++++++--------
> tools/perf/builtin-record.c     | 11 ++++++-----
> tools/perf/builtin-sched.c      |  4 ++--
> tools/perf/builtin-script.c     |  8 ++++----
> tools/perf/util/parse-options.c | 15 +++++++++++++++
> tools/perf/util/parse-options.h |  4 ++++
> tools/perf/util/strbuf.c        | 22 +++++++++++++++-------
> tools/perf/util/strbuf.h        |  2 ++
> 9 files changed, 62 insertions(+), 28 deletions(-)
>
>diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
>index 695ec5a50cf2..f4d62510acbb 100644
>--- a/tools/perf/builtin-evlist.c
>+++ b/tools/perf/builtin-evlist.c
>@@ -61,8 +61,8 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
> 		usage_with_options(evlist_usage, options);
>
> 	if (details.event_group && (details.verbose || details.freq)) {
>-		pr_err("--group option is not compatible with other options\n");
>-		usage_with_options(evlist_usage, options);
>+		usage_with_options_msg(evlist_usage, options,
>+			"--group option is not compatible with other options\n");
> 	}
>
> 	return __cmd_evlist(input_name, &details);
>diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>index 530c3a28a58c..132afc97676c 100644
>--- a/tools/perf/builtin-probe.c
>+++ b/tools/perf/builtin-probe.c
>@@ -528,12 +528,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> 			     PARSE_OPT_STOP_AT_NON_OPTION);
> 	if (argc > 0) {
> 		if (strcmp(argv[0], "-") == 0) {
>-			pr_warning("  Error: '-' is not supported.\n");
>-			usage_with_options(probe_usage, options);
>+			usage_with_options_msg(probe_usage, options,
>+				"'-' is not supported.\n");
> 		}
> 		if (params.command && params.command != 'a') {
>-			pr_warning("  Error: another command except --add is set.\n");
>-			usage_with_options(probe_usage, options);
>+			usage_with_options_msg(probe_usage, options,
>+				"another command except --add is set.\n");
> 		}
> 		ret = parse_probe_event_argv(argc, argv);
> 		if (ret < 0) {
>@@ -562,8 +562,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> 	switch (params.command) {
> 	case 'l':
> 		if (params.uprobes) {
>-			pr_warning("  Error: Don't use --list with --exec.\n");
>-			usage_with_options(probe_usage, options);
>+			pr_err("  Error: Don't use --list with --exec.\n");
>+			parse_options_usage(probe_usage, options, "l", true);
>+			parse_options_usage(NULL, options, "x", true);
>+			return -EINVAL;
> 		}
> 		ret = show_perf_probe_events(params.filter);
> 		if (ret < 0)
>@@ -603,8 +605,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> 	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");
>-			usage_with_options(probe_usage, options);
>+			pr_err("  Error: -x/-m must follow the probe definitions.\n");
>+			parse_options_usage(probe_usage, options, "m", true);
>+			parse_options_usage(NULL, options, "x", true);
>+			return -EINVAL;
> 		}
>
> 		ret = perf_add_probe_events(params.events, params.nevents);
>diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>index 2740d7a82ae8..de02267c73d8 100644
>--- a/tools/perf/builtin-record.c
>+++ b/tools/perf/builtin-record.c
>@@ -1135,14 +1135,15 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
> 		usage_with_options(record_usage, record_options);
>
> 	if (nr_cgroups && !rec->opts.target.system_wide) {
>-		ui__error("cgroup monitoring only available in"
>-			  " system-wide mode\n");
>-		usage_with_options(record_usage, record_options);
>+		usage_with_options_msg(record_usage, record_options,
>+			"cgroup monitoring only available in system-wide mode");
>+
> 	}
> 	if (rec->opts.record_switch_events &&
> 	    !perf_can_record_switch_events()) {
>-		ui__error("kernel does not support recording context switch events (--switch-events option)\n");
>-		usage_with_options(record_usage, record_options);
>+		ui__error("kernel does not support recording context switch events\n");
>+		parse_options_usage(record_usage, record_options, "switch-events", 0);
>+		return -EINVAL;
> 	}
>
> 	if (!rec->itr) {
>diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>index 33962612a5e9..0ee6d900e100 100644
>--- a/tools/perf/builtin-sched.c
>+++ b/tools/perf/builtin-sched.c
>@@ -1728,8 +1728,8 @@ static void setup_sorting(struct perf_sched *sched, const struct option *options
> 	for (tok = strtok_r(str, ", ", &tmp);
> 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
> 		if (sort_dimension__add(tok, &sched->sort_list) < 0) {
>-			error("Unknown --sort key: `%s'", tok);
>-			usage_with_options(usage_msg, options);
>+			usage_with_options_msg(usage_msg, options,
>+					"Unknown --sort key: `%s'", tok);
> 		}
> 	}
>
>diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>index 2653c0273b89..278acb22f029 100644
>--- a/tools/perf/builtin-script.c
>+++ b/tools/perf/builtin-script.c
>@@ -1767,9 +1767,9 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
> 		rep_script_path = get_script_path(argv[0], REPORT_SUFFIX);
>
> 		if (!rec_script_path && !rep_script_path) {
>-			fprintf(stderr, " Couldn't find script %s\n\n See perf"
>+			usage_with_options_msg(script_usage, options,
>+				"Couldn't find script `%s'\n\n See perf"
> 				" script -l for available scripts.\n", argv[0]);
>-			usage_with_options(script_usage, options);
> 		}
>
> 		if (is_top_script(argv[0])) {
>@@ -1780,10 +1780,10 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
> 			rep_args = has_required_arg(rep_script_path);
> 			rec_args = (argc - 1) - rep_args;
> 			if (rec_args < 0) {
>-				fprintf(stderr, " %s script requires options."
>+				usage_with_options_msg(script_usage, options,
>+					"`%s' script requires options."
> 					"\n\n See perf script -l for available "
> 					"scripts and options.\n", argv[0]);
>-				usage_with_options(script_usage, options);
> 			}
> 		}
>
>diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
>index eeeed98eb26d..230e771407a3 100644
>--- a/tools/perf/util/parse-options.c
>+++ b/tools/perf/util/parse-options.c
>@@ -760,6 +760,21 @@ void usage_with_options(const char * const *usagestr,
> 	exit(129);
> }
>
>+void usage_with_options_msg(const char * const *usagestr,
>+			    const struct option *opts, const char *fmt, ...)
>+{
>+	va_list ap;
>+
>+	exit_browser(false);
>+
>+	va_start(ap, fmt);
>+	strbuf_addv(&error_buf, fmt, ap);
>+	va_end(ap);
>+
>+	usage_with_options_internal(usagestr, opts, 0, NULL);
>+	exit(129);
>+}
>+
> int parse_options_usage(const char * const *usagestr,
> 			const struct option *opts,
> 			const char *optstr, bool short_opt)
>diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
>index 182c86099330..a8e407bc251e 100644
>--- a/tools/perf/util/parse-options.h
>+++ b/tools/perf/util/parse-options.h
>@@ -161,6 +161,10 @@ extern int parse_options_subcommand(int argc, const char **argv,
>
> extern NORETURN void usage_with_options(const char * const *usagestr,
>                                         const struct option *options);
>+extern NORETURN __attribute__((format(printf,3,4)))
>+void usage_with_options_msg(const char * const *usagestr,
>+			    const struct option *options,
>+			    const char *fmt, ...);
>
> /*----- incremantal advanced APIs -----*/
>
>diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
>index 4abe23550c73..25671fa16618 100644
>--- a/tools/perf/util/strbuf.c
>+++ b/tools/perf/util/strbuf.c
>@@ -82,23 +82,22 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
> 	strbuf_setlen(sb, sb->len + len);
> }
>
>-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
>+void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
> {
> 	int len;
>-	va_list ap;
>+	va_list ap_saved;
>
> 	if (!strbuf_avail(sb))
> 		strbuf_grow(sb, 64);
>-	va_start(ap, fmt);
>+
>+	va_copy(ap_saved, ap);
> 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>-	va_end(ap);
> 	if (len < 0)
> 		die("your vsnprintf is broken");
> 	if (len > strbuf_avail(sb)) {
> 		strbuf_grow(sb, len);
>-		va_start(ap, fmt);
>-		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>-		va_end(ap);
>+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap_saved);
>+		va_end(ap_saved);
> 		if (len > strbuf_avail(sb)) {
> 			die("this should not happen, your vsnprintf is broken");
> 		}
>@@ -106,6 +105,15 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> 	strbuf_setlen(sb, sb->len + len);
> }
>
>+void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
>+{
>+	va_list ap;
>+
>+	va_start(ap, fmt);
>+	strbuf_addv(sb, fmt, ap);
>+	va_end(ap);
>+}
>+
> ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
> {
> 	size_t oldlen = sb->len;
>diff --git a/tools/perf/util/strbuf.h b/tools/perf/util/strbuf.h
>index 436ac319f6c7..529f2f035249 100644
>--- a/tools/perf/util/strbuf.h
>+++ b/tools/perf/util/strbuf.h
>@@ -39,6 +39,7 @@
>  */
>
> #include <assert.h>
>+#include <stdarg.h>
>
> extern char strbuf_slopbuf[];
> struct strbuf {
>@@ -85,6 +86,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
>
> __attribute__((format(printf,2,3)))
> extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
>+extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);
>
> /* XXX: if read fails, any partial read is undone */
> extern ssize_t strbuf_read(struct strbuf *, int fd, ssize_t hint);
>--
>2.6.0

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/4] perf tools: Introduce usage_with_options_msg()

[Arnaldo Carvalho de Melo]

Em Mon, Oct 26, 2015 at 11:13:32PM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> >From: Namhyung Kim [mailto:namhyung@gmail.com] On Behalf Of Namhyung Kim
> >
> >Now usage_with_options() setup a pager before printing message so normal
> >printf() or pr_err() will not be shown.  The usage_with_options_msg()
> >can be used to print some help message before usage strings.
> 
> Thanks! looks good to me  :)
> 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks, applied.

- Arnaldo

[tip:perf/core] perf tools: Improve ambiguous option help message

[tip-bot for Namhyung Kim]

Commit-ID:  a5f4a6932ec2e1a53642e97a1be64bc7b169942f
Gitweb:     http://git.kernel.org/tip/a5f4a6932ec2e1a53642e97a1be64bc7b169942f
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 25 Oct 2015 00:49:24 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 26 Oct 2015 13:59:06 -0300

perf tools: Improve ambiguous option help message

Currently if an option name is ambiguous it only prints first two
matched option names but no help.  It'd be better it could show all
possible names and help messages too.

Before:
  $ perf report --show
    Error: Ambiguous option: show (could be --show-total-period or
                                            --show-ref-call-graph)
   Usage: perf report [<options>]

After:
  $ perf report --show
    Error: Ambiguous option: show (could be --show-total-period or
                                            --show-ref-call-graph)
   Usage: perf report [<options>]

      -n, --show-nr-samples
                              Show a column with the number of samples
          --showcpuutilization
                              Show sample percentage for different cpu modes
      -I, --show-info         Display extended information about perf.data file
          --show-total-period
                              Show a column with the sum of periods
          --show-ref-call-graph
                              Show callgraph from reference event

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1445701767-12731-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-options.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 22c2806..b8d9822 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -770,24 +770,23 @@ int parse_options_usage(const char * const *usagestr,
 opt:
 	for (  ; opts->type != OPTION_END; opts++) {
 		if (short_opt) {
-			if (opts->short_name == *optstr)
+			if (opts->short_name == *optstr) {
+				print_option_help(opts, 0);
 				break;
+			}
 			continue;
 		}
 
 		if (opts->long_name == NULL)
 			continue;
 
-		if (!prefixcmp(optstr, opts->long_name))
-			break;
-		if (!prefixcmp(optstr, "no-") &&
-		    !prefixcmp(optstr + 3, opts->long_name))
-			break;
+		if (!prefixcmp(opts->long_name, optstr))
+			print_option_help(opts, 0);
+		if (!prefixcmp("no-", optstr) &&
+		    !prefixcmp(opts->long_name, optstr + 3))
+			print_option_help(opts, 0);
 	}
 
-	if (opts->type != OPTION_END)
-		print_option_help(opts, 0);
-
 	return PARSE_OPT_HELP;
 }
 

[tip:perf/core] perf report: Rename to --show-cpu-utilization

[tip-bot for Namhyung Kim]

Commit-ID:  b272a59d835cd8ca6b45f41c66c61b473996c759
Gitweb:     http://git.kernel.org/tip/b272a59d835cd8ca6b45f41c66c61b473996c759
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 25 Oct 2015 00:49:25 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 26 Oct 2015 14:06:04 -0300

perf report: Rename to --show-cpu-utilization

So that it can be more consistent with other --show-* options.  The old
name (--showcpuutilization) is provided only for compatibility.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1445701767-12731-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-report.txt | 2 +-
 tools/perf/builtin-report.c              | 4 +++-
 tools/perf/util/parse-options.h          | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index ab1fd64..5ce8da1 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -29,7 +29,7 @@ OPTIONS
 --show-nr-samples::
 	Show the number of samples for each symbol
 
---showcpuutilization::
+--show-cpu-utilization::
         Show sample percentage for different cpu modes.
 
 -T::
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 50dd4d3..2853ad2 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -699,8 +699,10 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		   " Please refer the man page for the complete list."),
 	OPT_STRING('F', "fields", &field_order, "key[,keys...]",
 		   "output field(s): overhead, period, sample plus all of sort keys"),
-	OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
+	OPT_BOOLEAN(0, "show-cpu-utilization", &symbol_conf.show_cpu_utilization,
 		    "Show sample percentage for different cpu modes"),
+	OPT_BOOLEAN_FLAG(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
+		    "Show sample percentage for different cpu modes", PARSE_OPT_HIDDEN),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
 		   "regex filter to identify parent, see: '--sort parent'"),
 	OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 367d8b8..182c860 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -111,6 +111,7 @@ struct option {
 #define OPT_GROUP(h)                { .type = OPTION_GROUP, .help = (h) }
 #define OPT_BIT(s, l, v, h, b)      { .type = OPTION_BIT, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h), .defval = (b) }
 #define OPT_BOOLEAN(s, l, v, h)     { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h) }
+#define OPT_BOOLEAN_FLAG(s, l, v, h, f)     { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h), .flags = (f) }
 #define OPT_BOOLEAN_SET(s, l, v, os, h) \
 	{ .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), \
 	.value = check_vtype(v, bool *), .help = (h), \

[tip:perf/core] perf tools: Setup pager when printing usage and help

[tip-bot for Namhyung Kim]

Commit-ID:  01b19455c08cc37d1c3ef174524278e84c92fec1
Gitweb:     http://git.kernel.org/tip/01b19455c08cc37d1c3ef174524278e84c92fec1
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 25 Oct 2015 00:49:26 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 26 Oct 2015 14:08:48 -0300

perf tools: Setup pager when printing usage and help

It's annoying to see error or help message when command has many options
like in perf record, report or top.  So setup pager when print parser
error or help message - it should be OK since no UI is enabled at the
parsing time.  The usage_with_options() already disables it by calling
exit_browser() anyway.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1445701767-12731-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-options.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index b8d9822..eeeed98 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -7,6 +7,8 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
+static struct strbuf error_buf = STRBUF_INIT;
+
 static int opterror(const struct option *opt, const char *reason, int flags)
 {
 	if (flags & OPT_SHORT)
@@ -540,9 +542,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 		exit(130);
 	default: /* PARSE_OPT_UNKNOWN */
 		if (ctx.argv[0][1] == '-') {
-			error("unknown option `%s'", ctx.argv[0] + 2);
+			strbuf_addf(&error_buf, "unknown option `%s'",
+				    ctx.argv[0] + 2);
 		} else {
-			error("unknown switch `%c'", *ctx.opt);
+			strbuf_addf(&error_buf, "unknown switch `%c'",
+				    *ctx.opt);
 		}
 		usage_with_options(usagestr, options);
 	}
@@ -711,6 +715,13 @@ int usage_with_options_internal(const char * const *usagestr,
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
+	setup_pager();
+
+	if (strbuf_avail(&error_buf)) {
+		fprintf(stderr, "  Error: %s\n", error_buf.buf);
+		strbuf_release(&error_buf);
+	}
+
 	fprintf(stderr, "\n Usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
 		fprintf(stderr, "    or: %s\n", *usagestr++);

[tip:perf/core] perf tools: Introduce usage_with_options_msg()

[tip-bot for Namhyung Kim]

Commit-ID:  c71183697250b356be6c7c1abc2e9a74073e1dca
Gitweb:     http://git.kernel.org/tip/c71183697250b356be6c7c1abc2e9a74073e1dca
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 25 Oct 2015 00:49:27 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 27 Oct 2015 09:28:44 -0300

perf tools: Introduce usage_with_options_msg()

Now usage_with_options() setup a pager before printing message so normal
printf() or pr_err() will not be shown.  The usage_with_options_msg()
can be used to print some help message before usage strings.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1445701767-12731-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-evlist.c     |  4 ++--
 tools/perf/builtin-probe.c      | 20 ++++++++++++--------
 tools/perf/builtin-record.c     | 11 ++++++-----
 tools/perf/builtin-sched.c      |  4 ++--
 tools/perf/builtin-script.c     |  8 ++++----
 tools/perf/util/parse-options.c | 15 +++++++++++++++
 tools/perf/util/parse-options.h |  4 ++++
 tools/perf/util/strbuf.c        | 22 +++++++++++++++-------
 tools/perf/util/strbuf.h        |  2 ++
 9 files changed, 62 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 695ec5a..f4d6251 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -61,8 +61,8 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(evlist_usage, options);
 
 	if (details.event_group && (details.verbose || details.freq)) {
-		pr_err("--group option is not compatible with other options\n");
-		usage_with_options(evlist_usage, options);
+		usage_with_options_msg(evlist_usage, options,
+			"--group option is not compatible with other options\n");
 	}
 
 	return __cmd_evlist(input_name, &details);
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 530c3a2..132afc9 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -528,12 +528,12 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	if (argc > 0) {
 		if (strcmp(argv[0], "-") == 0) {
-			pr_warning("  Error: '-' is not supported.\n");
-			usage_with_options(probe_usage, options);
+			usage_with_options_msg(probe_usage, options,
+				"'-' is not supported.\n");
 		}
 		if (params.command && params.command != 'a') {
-			pr_warning("  Error: another command except --add is set.\n");
-			usage_with_options(probe_usage, options);
+			usage_with_options_msg(probe_usage, options,
+				"another command except --add is set.\n");
 		}
 		ret = parse_probe_event_argv(argc, argv);
 		if (ret < 0) {
@@ -562,8 +562,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	switch (params.command) {
 	case 'l':
 		if (params.uprobes) {
-			pr_warning("  Error: Don't use --list with --exec.\n");
-			usage_with_options(probe_usage, options);
+			pr_err("  Error: Don't use --list with --exec.\n");
+			parse_options_usage(probe_usage, options, "l", true);
+			parse_options_usage(NULL, options, "x", true);
+			return -EINVAL;
 		}
 		ret = show_perf_probe_events(params.filter);
 		if (ret < 0)
@@ -603,8 +605,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	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");
-			usage_with_options(probe_usage, options);
+			pr_err("  Error: -x/-m must follow the probe definitions.\n");
+			parse_options_usage(probe_usage, options, "m", true);
+			parse_options_usage(NULL, options, "x", true);
+			return -EINVAL;
 		}
 
 		ret = perf_add_probe_events(params.events, params.nevents);
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2740d7a..de02267 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1135,14 +1135,15 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 		usage_with_options(record_usage, record_options);
 
 	if (nr_cgroups && !rec->opts.target.system_wide) {
-		ui__error("cgroup monitoring only available in"
-			  " system-wide mode\n");
-		usage_with_options(record_usage, record_options);
+		usage_with_options_msg(record_usage, record_options,
+			"cgroup monitoring only available in system-wide mode");
+
 	}
 	if (rec->opts.record_switch_events &&
 	    !perf_can_record_switch_events()) {
-		ui__error("kernel does not support recording context switch events (--switch-events option)\n");
-		usage_with_options(record_usage, record_options);
+		ui__error("kernel does not support recording context switch events\n");
+		parse_options_usage(record_usage, record_options, "switch-events", 0);
+		return -EINVAL;
 	}
 
 	if (!rec->itr) {
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 3396261..0ee6d90 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1728,8 +1728,8 @@ static void setup_sorting(struct perf_sched *sched, const struct option *options
 	for (tok = strtok_r(str, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		if (sort_dimension__add(tok, &sched->sort_list) < 0) {
-			error("Unknown --sort key: `%s'", tok);
-			usage_with_options(usage_msg, options);
+			usage_with_options_msg(usage_msg, options,
+					"Unknown --sort key: `%s'", tok);
 		}
 	}
 
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 2653c02..278acb2 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1767,9 +1767,9 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
 		rep_script_path = get_script_path(argv[0], REPORT_SUFFIX);
 
 		if (!rec_script_path && !rep_script_path) {
-			fprintf(stderr, " Couldn't find script %s\n\n See perf"
+			usage_with_options_msg(script_usage, options,
+				"Couldn't find script `%s'\n\n See perf"
 				" script -l for available scripts.\n", argv[0]);
-			usage_with_options(script_usage, options);
 		}
 
 		if (is_top_script(argv[0])) {
@@ -1780,10 +1780,10 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
 			rep_args = has_required_arg(rep_script_path);
 			rec_args = (argc - 1) - rep_args;
 			if (rec_args < 0) {
-				fprintf(stderr, " %s script requires options."
+				usage_with_options_msg(script_usage, options,
+					"`%s' script requires options."
 					"\n\n See perf script -l for available "
 					"scripts and options.\n", argv[0]);
-				usage_with_options(script_usage, options);
 			}
 		}
 
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index eeeed98..230e771 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -760,6 +760,21 @@ void usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void usage_with_options_msg(const char * const *usagestr,
+			    const struct option *opts, const char *fmt, ...)
+{
+	va_list ap;
+
+	exit_browser(false);
+
+	va_start(ap, fmt);
+	strbuf_addv(&error_buf, fmt, ap);
+	va_end(ap);
+
+	usage_with_options_internal(usagestr, opts, 0, NULL);
+	exit(129);
+}
+
 int parse_options_usage(const char * const *usagestr,
 			const struct option *opts,
 			const char *optstr, bool short_opt)
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index 182c860..a8e407b 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -161,6 +161,10 @@ extern int parse_options_subcommand(int argc, const char **argv,
 
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
+extern NORETURN __attribute__((format(printf,3,4)))
+void usage_with_options_msg(const char * const *usagestr,
+			    const struct option *options,
+			    const char *fmt, ...);
 
 /*----- incremantal advanced APIs -----*/
 
diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
index 4abe235..25671fa 100644
--- a/tools/perf/util/strbuf.c
+++ b/tools/perf/util/strbuf.c
@@ -82,23 +82,22 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }
 
-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)
 {
 	int len;
-	va_list ap;
+	va_list ap_saved;
 
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
-	va_start(ap, fmt);
+
+	va_copy(ap_saved, ap);
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-	va_end(ap);
 	if (len < 0)
 		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
-		va_start(ap, fmt);
-		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-		va_end(ap);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap_saved);
+		va_end(ap_saved);
 		if (len > strbuf_avail(sb)) {
 			die("this should not happen, your vsnprintf is broken");
 		}
@@ -106,6 +105,15 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	strbuf_addv(sb, fmt, ap);
+	va_end(ap);
+}
+
 ssize_t strbuf_read(struct strbuf *sb, int fd, ssize_t hint)
 {
 	size_t oldlen = sb->len;
diff --git a/tools/perf/util/strbuf.h b/tools/perf/util/strbuf.h
index 436ac31..529f2f0 100644
--- a/tools/perf/util/strbuf.h
+++ b/tools/perf/util/strbuf.h
@@ -39,6 +39,7 @@
  */
 
 #include <assert.h>
+#include <stdarg.h>
 
 extern char strbuf_slopbuf[];
 struct strbuf {
@@ -85,6 +86,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);
 
 /* XXX: if read fails, any partial read is undone */
 extern ssize_t strbuf_read(struct strbuf *, int fd, ssize_t hint);