From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Jiri Olsa <jolsa@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH 4/4] perf tools: Introduce usage_with_options_msg()
Date: Mon, 26 Oct 2015 14:13:28 -0300 [thread overview]
Message-ID: <20151026171328.GL27006@kernel.org> (raw)
In-Reply-To: <1445701767-12731-4-git-send-email-namhyung@kernel.org>
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
next prev parent reply other threads:[~2015-10-26 17:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-24 15:49 [PATCH 1/4] perf tools: Improve ambiguous option help message Namhyung Kim
2015-10-24 15:49 ` [PATCH 2/4] perf report: Rename to --show-cpu-utilization Namhyung Kim
2015-10-25 8:59 ` Ingo Molnar
2015-10-29 9:39 ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-10-24 15:49 ` [PATCH 3/4] perf tools: Setup pager when printing usage and help Namhyung Kim
2015-10-25 9:04 ` Ingo Molnar
2015-10-29 9:40 ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-10-24 15:49 ` [PATCH 4/4] perf tools: Introduce usage_with_options_msg() Namhyung Kim
2015-10-26 17:13 ` Arnaldo Carvalho de Melo [this message]
2015-10-26 23:13 ` 平松雅巳 / HIRAMATU，MASAMI
2015-10-27 12:29 ` Arnaldo Carvalho de Melo
2015-10-29 9:40 ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-10-25 8:48 ` [PATCH 1/4] perf tools: Improve ambiguous option help message Ingo Molnar
2015-10-29 9:39 ` [tip:perf/core] " tip-bot for Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151026171328.GL27006@kernel.org \
--to=acme@kernel.org \
--cc=a.p.zijlstra@chello.nl \
--cc=dsahern@gmail.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=masami.hiramatsu.pt@hitachi.com \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.