[PATCHSET 0/7] perf tools: Remove browser dependency from usage_with_options()

[PATCHSET 0/7] perf tools: Remove browser dependency from usage_with_options()

[Namhyung Kim]

Hello,

This patchset removes the UI browser dependency (specifically
exit_browser function) from option parser code.  It'll help to
separate out the common code into a library.

Now existing users of usage_with_options() were converted to call it
before setup_browser().  I think future users can notice the
difference when they test their code and will call it properly.

It's available on 'perf/option-dependency-v1' branch on my tree

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks
Namhyung


Namhyung Kim (7):
  perf annotate: Check argument before calling setup_browser()
  perf annotate: Delay UI browser setup after initialization is done
  perf kvm: Remove invocation of setup/exit_browser()
  perf report: Check argument before calling setup_browser()
  perf top: Delay UI browser setup after initialization is done
  perf tools: Free strlist on error path
  perf tools: Get rid of exit_browser() from usage_with_options()

 tools/perf/builtin-annotate.c   | 33 ++++++++++++++++-----------------
 tools/perf/builtin-kvm.c        |  3 ---
 tools/perf/builtin-report.c     | 21 ++++++++++-----------
 tools/perf/builtin-top.c        | 14 +++++++-------
 tools/perf/util/parse-options.c |  3 ---
 tools/perf/util/thread_map.c    |  1 +
 6 files changed, 34 insertions(+), 41 deletions(-)

-- 
2.6.2

[PATCH 1/7] perf annotate: Check argument before calling setup_browser()

[Namhyung Kim]

This is necessary to get rid of the browser dependency from
usage_with_options() and its friends.  Because there's no code
changing the argc and argv, it'd be ok to check it early.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 2bf9b3fd9e61..55f6f8dab5d4 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -343,6 +343,16 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		return ret;
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(annotate_usage, options);
+
+		annotate.sym_hist_filter = argv[0];
+	}
 
 	if (annotate.use_stdio)
 		use_browser = 0;
@@ -369,17 +379,6 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(annotate_usage, options);
-
-		annotate.sym_hist_filter = argv[0];
-	}
-
 	ret = __cmd_annotate(&annotate);
 
 out_delete:
-- 
2.6.2

[tip:perf/core] perf annotate: Check argument before calling setup_browser()

[tip-bot for Namhyung Kim]

Commit-ID:  50e19ef978158a3d1f790568eccd8e4a802190c2
Gitweb:     http://git.kernel.org/tip/50e19ef978158a3d1f790568eccd8e4a802190c2
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:53 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 14:32:33 -0300

perf annotate: Check argument before calling setup_browser()

This is necessary to get rid of the browser dependency from
usage_with_options() and its friends.  Because there's no code changing
the argc and argv, it'd be ok to check it early.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
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/1449716459-23004-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-annotate.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 2bf9b3f..55f6f8d 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -343,6 +343,16 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		return ret;
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(annotate_usage, options);
+
+		annotate.sym_hist_filter = argv[0];
+	}
 
 	if (annotate.use_stdio)
 		use_browser = 0;
@@ -369,17 +379,6 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(annotate_usage, options);
-
-		annotate.sym_hist_filter = argv[0];
-	}
-
 	ret = __cmd_annotate(&annotate);
 
 out_delete:

[PATCH 2/7] perf annotate: Delay UI browser setup after initialization is done

[Namhyung Kim]

Move setup_browser after all necessary initialization is done.  This
is to remove the browser dependency from usage_with_options and
friends.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 55f6f8dab5d4..1f00dc7cecba 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -354,17 +354,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		annotate.sym_hist_filter = argv[0];
 	}
 
-	if (annotate.use_stdio)
-		use_browser = 0;
-	else if (annotate.use_tui)
-		use_browser = 1;
-	else if (annotate.use_gtk)
-		use_browser = 2;
-
 	file.path  = input_name;
 
-	setup_browser(true);
-
 	annotate.session = perf_session__new(&file, false, &annotate.tool);
 	if (annotate.session == NULL)
 		return -1;
@@ -379,6 +370,15 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
+	if (annotate.use_stdio)
+		use_browser = 0;
+	else if (annotate.use_tui)
+		use_browser = 1;
+	else if (annotate.use_gtk)
+		use_browser = 2;
+
+	setup_browser(true);
+
 	ret = __cmd_annotate(&annotate);
 
 out_delete:
-- 
2.6.2

[tip:perf/core] perf annotate: Delay UI browser setup after initialization is done

[tip-bot for Namhyung Kim]

Commit-ID:  3df668e74a5bc60d74c2ce0b3498af2d77b4b556
Gitweb:     http://git.kernel.org/tip/3df668e74a5bc60d74c2ce0b3498af2d77b4b556
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:54 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 14:33:54 -0300

perf annotate: Delay UI browser setup after initialization is done

Move setup_browser after all necessary initialization is done.  This is
to remove the browser dependency from usage_with_options and friends.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
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/1449716459-23004-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-annotate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 55f6f8d..1f00dc7 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -354,17 +354,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		annotate.sym_hist_filter = argv[0];
 	}
 
-	if (annotate.use_stdio)
-		use_browser = 0;
-	else if (annotate.use_tui)
-		use_browser = 1;
-	else if (annotate.use_gtk)
-		use_browser = 2;
-
 	file.path  = input_name;
 
-	setup_browser(true);
-
 	annotate.session = perf_session__new(&file, false, &annotate.tool);
 	if (annotate.session == NULL)
 		return -1;
@@ -379,6 +370,15 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(annotate_usage, options);
 
+	if (annotate.use_stdio)
+		use_browser = 0;
+	else if (annotate.use_tui)
+		use_browser = 1;
+	else if (annotate.use_gtk)
+		use_browser = 2;
+
+	setup_browser(true);
+
 	ret = __cmd_annotate(&annotate);
 
 out_delete:

[PATCH 3/7] perf kvm: Remove invocation of setup/exit_browser()

[Namhyung Kim]

Calling setup_browser(false) with use_browser = 0 is meaningless.
Just get rid of it.  This is necessary to remove the browser
dependency from usage_with_options() and friends.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index dd94b4ca2213..031f9f55c281 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1351,7 +1351,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	disable_buildid_cache();
 
 	use_browser = 0;
-	setup_browser(false);
 
 	if (argc) {
 		argc = parse_options(argc, argv, live_options,
@@ -1409,8 +1408,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	err = kvm_events_live_report(kvm);
 
 out:
-	exit_browser(0);
-
 	if (kvm->session)
 		perf_session__delete(kvm->session);
 	kvm->session = NULL;
-- 
2.6.2

[tip:perf/core] perf kvm: Remove invocation of setup/exit_browser ()

[tip-bot for Namhyung Kim]

Commit-ID:  1b0344e64d7b4e512a8e5d2bc88b022fbb7a9ee6
Gitweb:     http://git.kernel.org/tip/1b0344e64d7b4e512a8e5d2bc88b022fbb7a9ee6
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:55 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 14:35:35 -0300

perf kvm: Remove invocation of setup/exit_browser()

Calling setup_browser(false) with use_browser = 0 is meaningless.
Just get rid of it.  This is necessary to remove the browser
dependency from usage_with_options() and friends.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@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/1449716459-23004-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-kvm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index dd94b4c..031f9f5 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1351,7 +1351,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	disable_buildid_cache();
 
 	use_browser = 0;
-	setup_browser(false);
 
 	if (argc) {
 		argc = parse_options(argc, argv, live_options,
@@ -1409,8 +1408,6 @@ static int kvm_events_live(struct perf_kvm_stat *kvm,
 	err = kvm_events_live_report(kvm);
 
 out:
-	exit_browser(0);
-
 	if (kvm->session)
 		perf_session__delete(kvm->session);
 	kvm->session = NULL;

[PATCH 4/7] perf report: Check argument before calling setup_browser()

[Namhyung Kim]

This is necessary to get rid of the browser dependency from
usage_with_options() and its friends.  Because there's no code
changing the argc and argv, it'd be ok to check it early.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index af5db885ea9c..5a454669d075 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -801,6 +801,16 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_config(report__config, &report);
 
 	argc = parse_options(argc, argv, options, report_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(report_usage, options);
+
+		report.symbol_filter_str = argv[0];
+	}
 
 	if (symbol_conf.vmlinux_name &&
 	    access(symbol_conf.vmlinux_name, R_OK)) {
@@ -946,17 +956,6 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (symbol__init(&session->header.env) < 0)
 		goto error;
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(report_usage, options);
-
-		report.symbol_filter_str = argv[0];
-	}
-
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);
-- 
2.6.2

[tip:perf/core] perf report: Check argument before calling setup_browser()

[tip-bot for Namhyung Kim]

Commit-ID:  b3f38fc2422ace049110d1588a67b35bd15b81ce
Gitweb:     http://git.kernel.org/tip/b3f38fc2422ace049110d1588a67b35bd15b81ce
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:56 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 14:35:58 -0300

perf report: Check argument before calling setup_browser()

This is necessary to get rid of the browser dependency from
usage_with_options() and its friends.  Because there's no code
changing the argc and argv, it'd be ok to check it early.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
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/1449716459-23004-5-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index af5db88..5a45466 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -801,6 +801,16 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_config(report__config, &report);
 
 	argc = parse_options(argc, argv, options, report_usage, 0);
+	if (argc) {
+		/*
+		 * Special case: if there's an argument left then assume that
+		 * it's a symbol filter:
+		 */
+		if (argc > 1)
+			usage_with_options(report_usage, options);
+
+		report.symbol_filter_str = argv[0];
+	}
 
 	if (symbol_conf.vmlinux_name &&
 	    access(symbol_conf.vmlinux_name, R_OK)) {
@@ -946,17 +956,6 @@ repeat:
 	if (symbol__init(&session->header.env) < 0)
 		goto error;
 
-	if (argc) {
-		/*
-		 * Special case: if there's an argument left then assume that
-		 * it's a symbol filter:
-		 */
-		if (argc > 1)
-			usage_with_options(report_usage, options);
-
-		report.symbol_filter_str = argv[0];
-	}
-
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);

[PATCH 5/7] perf top: Delay UI browser setup after initialization is done

[Namhyung Kim]

Move setup_browser after all necessary initialization is done.  This
is to remove the browser dependency from usage_with_options() and
friends.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 7e2e72e6d9d1..75134e106a62 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1252,13 +1252,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 		goto out_delete_evlist;
 	}
 
-	if (top.use_stdio)
-		use_browser = 0;
-	else if (top.use_tui)
-		use_browser = 1;
-
-	setup_browser(false);
-
 	status = target__validate(target);
 	if (status) {
 		target__strerror(target, status, errbuf, BUFSIZ);
@@ -1326,6 +1319,13 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 		sigaction(SIGWINCH, &act, NULL);
 	}
 
+	if (top.use_stdio)
+		use_browser = 0;
+	else if (top.use_tui)
+		use_browser = 1;
+
+	setup_browser(false);
+
 	status = __cmd_top(&top);
 
 out_delete_evlist:
-- 
2.6.2

Re: [PATCH 5/7] perf top: Delay UI browser setup after initialization is done

[Arnaldo Carvalho de Melo]

Em Thu, Dec 10, 2015 at 12:00:57PM +0900, Namhyung Kim escreveu:
> Move setup_browser after all necessary initialization is done.  This
> is to remove the browser dependency from usage_with_options() and
> friends.

So, please try:

perf top -C 0 -p 1

So that we get a command line validation error that will cause cmd_top
to trip this:

        status = target__validate(target);
        if (status) {
                target__strerror(target, status, errbuf, BUFSIZ);
                ui__warning("%s\n", errbuf);
        }

ui__warning() will emit the warning to stdio, and this message will be
seen only after the user exits the tool:

[root@ssdandy ~]# perf top -C 0 -p 1
Warning:
PID/TID switch overriding CPU

Where without this patch the user will be warning with a popup window,
that has to be acknowledged with an enter before proceeding.

Looking at the other patches to check which can be processed now,
leaving this one for later.

- Arnaldo
 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-top.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 7e2e72e6d9d1..75134e106a62 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1252,13 +1252,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  		goto out_delete_evlist;
>  	}
>  
> -	if (top.use_stdio)
> -		use_browser = 0;
> -	else if (top.use_tui)
> -		use_browser = 1;
> -
> -	setup_browser(false);
> -
>  	status = target__validate(target);
>  	if (status) {
>  		target__strerror(target, status, errbuf, BUFSIZ);
> @@ -1326,6 +1319,13 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  		sigaction(SIGWINCH, &act, NULL);
>  	}
>  
> +	if (top.use_stdio)
> +		use_browser = 0;
> +	else if (top.use_tui)
> +		use_browser = 1;
> +
> +	setup_browser(false);
> +
>  	status = __cmd_top(&top);
>  
>  out_delete_evlist:
> -- 
> 2.6.2

Re: [PATCH 5/7] perf top: Delay UI browser setup after initialization is done

[Arnaldo Carvalho de Melo]

Em Thu, Dec 10, 2015 at 02:43:32PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Dec 10, 2015 at 12:00:57PM +0900, Namhyung Kim escreveu:
> > Move setup_browser after all necessary initialization is done.  This
> > is to remove the browser dependency from usage_with_options() and
> > friends.
> 
> So, please try:
> 
> perf top -C 0 -p 1
> 
> So that we get a command line validation error that will cause cmd_top
> to trip this:
> 
>         status = target__validate(target);
>         if (status) {
>                 target__strerror(target, status, errbuf, BUFSIZ);
>                 ui__warning("%s\n", errbuf);
>         }
> 
> ui__warning() will emit the warning to stdio, and this message will be
> seen only after the user exits the tool:

So, this one should be enough, ack?

>From 83c5eb5124210a579f174d37af8f0ce4f21264ff Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 10 Dec 2015 14:48:45 -0300
Subject: [PATCH] perf report: Do show usage message when failing to create
 cpu/thread maps

This is necessary to get rid of the browser dependency from
usage_with_options() and its friends.  Because we validate the targets
which are used to create the cpu/thread maps and inform the user about
any override performed via the chosen UI, we don't need to call the
usage routine for that.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@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/n/tip-slu7lj7buzpwgop1vo9la8ma@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 7e2e72e6d9d1..4eb9c6906219 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1279,8 +1279,10 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (target__none(target))
 		target->system_wide = true;
 
-	if (perf_evlist__create_maps(top.evlist, target) < 0)
-		usage_with_options(top_usage, options);
+	if (perf_evlist__create_maps(top.evlist, target) < 0) {
+		ui__error("Not enough memory to create thread/cpu maps\n");
+		goto out_delete_evlist;
+	}
 
 	if (!top.evlist->nr_entries &&
 	    perf_evlist__add_default(top.evlist) < 0) {
-- 
1.8.3.1

Re: [PATCH 5/7] perf top: Delay UI browser setup after initialization is done

[Arnaldo Carvalho de Melo]

Em Thu, Dec 10, 2015 at 02:52:05PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Dec 10, 2015 at 02:43:32PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Dec 10, 2015 at 12:00:57PM +0900, Namhyung Kim escreveu:
> > > Move setup_browser after all necessary initialization is done.  This
> > > is to remove the browser dependency from usage_with_options() and
> > > friends.
> > 
> > So, please try:
> > 
> > perf top -C 0 -p 1
> > 
> > So that we get a command line validation error that will cause cmd_top
> > to trip this:
> > 
> >         status = target__validate(target);
> >         if (status) {
> >                 target__strerror(target, status, errbuf, BUFSIZ);
> >                 ui__warning("%s\n", errbuf);
> >         }
> > 
> > ui__warning() will emit the warning to stdio, and this message will be
> > seen only after the user exits the tool:
> 
> So, this one should be enough, ack?

        if (perf_evlist__create_maps(top.evlist, target) < 0) {
-               ui__error("Not enough memory to create thread/cpu maps\n");
+               ui__error("Couldn't create thread/CPU maps: %s\n",
+                         errno == ENOENT ? "No such process" : strerror_r(errno, errbuf, sizeof(errbuf)));
                goto out_delete_evlist;
        }
 
I'm adding "Introduce perf_evlist__strerror_create_maps()" to my TODO list
at https://perf.wiki.kernel.org/index.php/Todo
 
> From 83c5eb5124210a579f174d37af8f0ce4f21264ff Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Thu, 10 Dec 2015 14:48:45 -0300
> Subject: [PATCH] perf report: Do show usage message when failing to create
>  cpu/thread maps
> 
> This is necessary to get rid of the browser dependency from
> usage_with_options() and its friends.  Because we validate the targets
> which are used to create the cpu/thread maps and inform the user about
> any override performed via the chosen UI, we don't need to call the
> usage routine for that.


> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Josh Poimboeuf <jpoimboe@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/n/tip-slu7lj7buzpwgop1vo9la8ma@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-top.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 7e2e72e6d9d1..4eb9c6906219 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1279,8 +1279,10 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (target__none(target))
>  		target->system_wide = true;
>  
> -	if (perf_evlist__create_maps(top.evlist, target) < 0)
> -		usage_with_options(top_usage, options);
> +	if (perf_evlist__create_maps(top.evlist, target) < 0) {
> +		ui__error("Not enough memory to create thread/cpu maps\n");
> +		goto out_delete_evlist;
> +	}
>  
>  	if (!top.evlist->nr_entries &&
>  	    perf_evlist__add_default(top.evlist) < 0) {
> -- 
> 1.8.3.1
> 

[PATCH 6/7] perf tools: Free strlist on error path

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/thread_map.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 6ec3c5ca438f..371fb28fe5b1 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -304,6 +304,7 @@ static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
 
 out_free_threads:
 	zfree(&threads);
+	strlist__delete(slist);
 	goto out;
 }
 
-- 
2.6.2

[tip:perf/core] perf thread_map: Free strlist on constructor error path

[tip-bot for Namhyung Kim]

Commit-ID:  7ecb48fde39e1d61ab8aff95581dcdfb572bcc28
Gitweb:     http://git.kernel.org/tip/7ecb48fde39e1d61ab8aff95581dcdfb572bcc28
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:58 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 15:47:51 -0300

perf thread_map: Free strlist on constructor error path

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@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/1449716459-23004-7-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/thread_map.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 6ec3c5c..371fb28 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -304,6 +304,7 @@ out:
 
 out_free_threads:
 	zfree(&threads);
+	strlist__delete(slist);
 	goto out;
 }
 

[PATCH 7/7] perf tools: Get rid of exit_browser() from usage_with_options()

[Namhyung Kim]

Since all of its users call before setup_browser(), there's no need to
call exit_browser() inside of the function.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/parse-options.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index d09aff983581..de3290b47db1 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -766,7 +766,6 @@ int usage_with_options_internal(const char * const *usagestr,
 void usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	exit_browser(false);
 	usage_with_options_internal(usagestr, opts, 0, NULL);
 	exit(129);
 }
@@ -776,8 +775,6 @@ void usage_with_options_msg(const char * const *usagestr,
 {
 	va_list ap;
 
-	exit_browser(false);
-
 	va_start(ap, fmt);
 	strbuf_addv(&error_buf, fmt, ap);
 	va_end(ap);
-- 
2.6.2

[tip:perf/core] perf tools: Get rid of exit_browser() from usage_with_options()

[tip-bot for Namhyung Kim]

Commit-ID:  3f86eb6b0771d785099c91354838d3f8d8126630
Gitweb:     http://git.kernel.org/tip/3f86eb6b0771d785099c91354838d3f8d8126630
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 10 Dec 2015 12:00:59 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 10 Dec 2015 15:47:52 -0300

perf tools: Get rid of exit_browser() from usage_with_options()

Since all of its users call before setup_browser(), there's no need to
call exit_browser() inside of the function.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
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/1449716459-23004-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-options.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index d09aff9..de3290b 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -766,7 +766,6 @@ int usage_with_options_internal(const char * const *usagestr,
 void usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	exit_browser(false);
 	usage_with_options_internal(usagestr, opts, 0, NULL);
 	exit(129);
 }
@@ -776,8 +775,6 @@ void usage_with_options_msg(const char * const *usagestr,
 {
 	va_list ap;
 
-	exit_browser(false);
-
 	va_start(ap, fmt);
 	strbuf_addv(&error_buf, fmt, ap);
 	va_end(ap);

Re: [PATCHSET 0/7] perf tools: Remove browser dependency from usage_with_options()

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 12:00:52PM +0900, Namhyung Kim wrote:
> Hello,
> 
> This patchset removes the UI browser dependency (specifically
> exit_browser function) from option parser code.  It'll help to
> separate out the common code into a library.
> 
> Now existing users of usage_with_options() were converted to call it
> before setup_browser().  I think future users can notice the
> difference when they test their code and will call it properly.
> 
> It's available on 'perf/option-dependency-v1' branch on my tree
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Thanks
> Namhyung
> 
> 
> Namhyung Kim (7):
>   perf annotate: Check argument before calling setup_browser()
>   perf annotate: Delay UI browser setup after initialization is done
>   perf kvm: Remove invocation of setup/exit_browser()
>   perf report: Check argument before calling setup_browser()
>   perf top: Delay UI browser setup after initialization is done
>   perf tools: Free strlist on error path
>   perf tools: Get rid of exit_browser() from usage_with_options()
> 
>  tools/perf/builtin-annotate.c   | 33 ++++++++++++++++-----------------
>  tools/perf/builtin-kvm.c        |  3 ---
>  tools/perf/builtin-report.c     | 21 ++++++++++-----------
>  tools/perf/builtin-top.c        | 14 +++++++-------
>  tools/perf/util/parse-options.c |  3 ---
>  tools/perf/util/thread_map.c    |  1 +
>  6 files changed, 34 insertions(+), 41 deletions(-)

Thanks a lot Namhyung!

For the series,

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCHSET 0/7] perf tools: Remove browser dependency from usage_with_options()

[Arnaldo Carvalho de Melo]

Em Thu, Dec 10, 2015 at 09:10:21AM -0600, Josh Poimboeuf escreveu:
> On Thu, Dec 10, 2015 at 12:00:52PM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > This patchset removes the UI browser dependency (specifically
> > exit_browser function) from option parser code.  It'll help to
> > separate out the common code into a library.
> > 
> > Now existing users of usage_with_options() were converted to call it
> > before setup_browser().  I think future users can notice the
> > difference when they test their code and will call it properly.
> > 
> > It's available on 'perf/option-dependency-v1' branch on my tree
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

So, applied all except for the top one, that I rewrote as reported,
pushed to perf/core, the replaced patch is this one:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=f8a5c0b24b8b1e77a0812b0c8251db0afc0524b7
 
> > 
> > Namhyung Kim (7):
> >   perf annotate: Check argument before calling setup_browser()
> >   perf annotate: Delay UI browser setup after initialization is done
> >   perf kvm: Remove invocation of setup/exit_browser()
> >   perf report: Check argument before calling setup_browser()
> >   perf top: Delay UI browser setup after initialization is done
> >   perf tools: Free strlist on error path
> >   perf tools: Get rid of exit_browser() from usage_with_options()
> > 
> >  tools/perf/builtin-annotate.c   | 33 ++++++++++++++++-----------------
> >  tools/perf/builtin-kvm.c        |  3 ---
> >  tools/perf/builtin-report.c     | 21 ++++++++++-----------
> >  tools/perf/builtin-top.c        | 14 +++++++-------
> >  tools/perf/util/parse-options.c |  3 ---
> >  tools/perf/util/thread_map.c    |  1 +
> >  6 files changed, 34 insertions(+), 41 deletions(-)
> 
> Thanks a lot Namhyung!
> 
> For the series,
> 
> Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> -- 
> Josh