perf tools: Improve ambiguous option help message

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

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.

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 <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 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(-)

tools/perf/builtin-evlist.c +2 -2

index 695ec5a50cf2..f4d62510acbb 100644
@@ -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);

tools/perf/builtin-probe.c +12 -8

index 530c3a28a58c..132afc97676c 100644
@@ -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);

tools/perf/builtin-record.c +6 -5

index 2740d7a82ae8..de02267c73d8 100644
@@ -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) {

tools/perf/builtin-sched.c +2 -2

index 33962612a5e9..0ee6d900e100 100644
@@ -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);
 		}
 	}
 

tools/perf/builtin-script.c +4 -4

index 2653c0273b89..278acb22f029 100644
@@ -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);
 			}
 		}
 

tools/perf/util/parse-options.c +15 -0

index eeeed98eb26d..230e771407a3 100644
@@ -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)

tools/perf/util/parse-options.h +4 -0

index 182c86099330..a8e407bc251e 100644
@@ -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 -----*/
 

tools/perf/util/strbuf.c +15 -7

index 4abe23550c73..25671fa16618 100644
@@ -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;

tools/perf/util/strbuf.h +2 -0

index 436ac319f6c7..529f2f035249 100644
@@ -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);

Signature · 1 line

2.6.0

From: Namhyung Kim [mailto:[email protected]] 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.

Cc Masami Hiramatsu <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---

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(-)

tools/perf/builtin-evlist.c +2 -2

index 695ec5a50cf2..f4d62510acbb 100644
@@ -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);

tools/perf/builtin-probe.c +12 -8

index 530c3a28a58c..132afc97676c 100644
@@ -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);

tools/perf/builtin-record.c +6 -5

index 2740d7a82ae8..de02267c73d8 100644
@@ -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) {

tools/perf/builtin-sched.c +2 -2

index 33962612a5e9..0ee6d900e100 100644
@@ -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);
 		}
 	}
 

tools/perf/builtin-script.c +4 -4

index 2653c0273b89..278acb22f029 100644
@@ -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);
 			}
 		}
 

tools/perf/util/parse-options.c +15 -0

index eeeed98eb26d..230e771407a3 100644
@@ -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)

tools/perf/util/parse-options.h +4 -0

index 182c86099330..a8e407bc251e 100644
@@ -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 -----*/
 

tools/perf/util/strbuf.c +15 -7

index 4abe23550c73..25671fa16618 100644
@@ -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;

tools/perf/util/strbuf.h +2 -1

index 436ac319f6c7..529f2f035249 100644
@@ -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

Quoted 5 lines

From: Namhyung Kim [mailto:[email protected]] 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 <[email protected]>

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