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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).