RE: [PATCH 4/4] perf tools: Introduce usage_with_options_msg()
From: åæéå / HIRAMATUïMASAMI
Date: Mon Oct 26 2015 - 19:13:54 EST
>From: Namhyung Kim [mailto:namhyung@xxxxxxxxx] 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@xxxxxxxxxxx>
>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>---
> 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‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~††Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ 0¶ìå