[PATCH 0/2] Completion for 'perf kvm'

[PATCH 0/2] Completion for 'perf kvm'

[Ramkumar Ramachandra]

Hi,

These are based on tip/master.

Thanks.

Ramkumar Ramachandra (2):
  perf completion: complete 'perf kvm'
  perf tools: ignore files generated by 'perf kvm'

 tools/perf/.gitignore         | 2 ++
 tools/perf/perf-completion.sh | 4 ++++
 2 files changed, 6 insertions(+)

-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH 1/2] perf completion: complete 'perf kvm'

[Ramkumar Ramachandra]

Currently, there is no way to enumerate the subcommands under 'perf
kvm', so hardcode them.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/perf-completion.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/perf-completion.sh b/tools/perf/perf-completion.sh
index 4949488..496e2ab 100644
--- a/tools/perf/perf-completion.sh
+++ b/tools/perf/perf-completion.sh
@@ -121,6 +121,10 @@ __perf_main ()
 	elif [[ $prev == "-e" && "${words[1]}" == @(record|stat|top) ]]; then
 		evts=$($cmd list --raw-dump)
 		__perfcomp_colon "$evts" "$cur"
+	# List subcommands for 'perf kvm'
+	elif [[ $prev == "kvm" ]]; then
+		subcmds="top record report diff buildid-list stat"
+		__perfcomp_colon "$subcmds" "$cur"
 	# List long option names
 	elif [[ $cur == --* ]];  then
 		subcmd=${words[1]}
-- 
1.8.5.rc0.5.g70ebc73.dirty

[PATCH 2/2] perf tools: ignore files generated by 'perf kvm'

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 tools/perf/.gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore
index 782d86e..28264a9 100644
--- a/tools/perf/.gitignore
+++ b/tools/perf/.gitignore
@@ -13,6 +13,8 @@ perf*.html
 common-cmds.h
 perf.data
 perf.data.old
+perf.data.guest
+perf.data.guest.old
 output.svg
 perf-archive
 tags
-- 
1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH 1/2] perf completion: complete 'perf kvm'

[Arnaldo Carvalho de Melo]

Em Wed, Dec 11, 2013 at 04:04:15PM +0530, Ramkumar Ramachandra escreveu:
> Currently, there is no way to enumerate the subcommands under 'perf
> kvm', so hardcode them.

[acme@ssdandy linux]$ perf kvm -h 2>&1 | grep usage
 usage: perf kvm [<options>] {top|record|report|diff|buildid-list|stat}

Can't you use this somehow?

I'm applying it tho as it is better than the current situation.

- Arnaldo
 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  tools/perf/perf-completion.sh | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/perf-completion.sh b/tools/perf/perf-completion.sh
> index 4949488..496e2ab 100644
> --- a/tools/perf/perf-completion.sh
> +++ b/tools/perf/perf-completion.sh
> @@ -121,6 +121,10 @@ __perf_main ()
>  	elif [[ $prev == "-e" && "${words[1]}" == @(record|stat|top) ]]; then
>  		evts=$($cmd list --raw-dump)
>  		__perfcomp_colon "$evts" "$cur"
> +	# List subcommands for 'perf kvm'
> +	elif [[ $prev == "kvm" ]]; then
> +		subcmds="top record report diff buildid-list stat"
> +		__perfcomp_colon "$subcmds" "$cur"
>  	# List long option names
>  	elif [[ $cur == --* ]];  then
>  		subcmd=${words[1]}
> -- 
> 1.8.5.rc0.5.g70ebc73.dirty

Re: [PATCH 1/2] perf completion: complete 'perf kvm'

[David Ahern]

On 12/11/13, 12:50 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 11, 2013 at 04:04:15PM +0530, Ramkumar Ramachandra escreveu:
>> Currently, there is no way to enumerate the subcommands under 'perf
>> kvm', so hardcode them.
>
> [acme@ssdandy linux]$ perf kvm -h 2>&1 | grep usage
>   usage: perf kvm [<options>] {top|record|report|diff|buildid-list|stat}
>
> Can't you use this somehow?
>
> I'm applying it tho as it is better than the current situation.

I don't like the command list being embedded in the completion script. 
It will get outdated quickly.

David

Re: [PATCH 2/2] perf tools: ignore files generated by 'perf kvm'

[Arnaldo Carvalho de Melo]

Em Wed, Dec 11, 2013 at 04:04:16PM +0530, Ramkumar Ramachandra escreveu:
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  tools/perf/.gitignore | 2 ++
>  1 file changed, 2 insertions(+)

Some more are missing, from 'perf kvm --help':

             Default('')         ->  perf.data.guest
             --host              ->  perf.data.kvm
             --guest             ->  perf.data.guest
             --host --guest      ->  perf.data.kvm
             --host --no-guest   ->  perf.data.host

Can you respin, please?

- Arnaldo
 
> diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore
> index 782d86e..28264a9 100644
> --- a/tools/perf/.gitignore
> +++ b/tools/perf/.gitignore
> @@ -13,6 +13,8 @@ perf*.html
>  common-cmds.h
>  perf.data
>  perf.data.old
> +perf.data.guest
> +perf.data.guest.old
>  output.svg
>  perf-archive
>  tags
> -- 
> 1.8.5.rc0.5.g70ebc73.dirty

[PATCH v2] perf tools: ignore files generated by 'perf kvm'

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Include .kvm[.old], .host[.old] files, as suggested by Arnaldo.

 tools/perf/.gitignore | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore
index 782d86e..9fea0d9 100644
--- a/tools/perf/.gitignore
+++ b/tools/perf/.gitignore
@@ -13,6 +13,12 @@ perf*.html
 common-cmds.h
 perf.data
 perf.data.old
+perf.data.guest
+perf.data.guest.old
+perf.data.kvm
+perf.data.kvm.old
+perf.data.host
+perf.data.host.old
 output.svg
 perf-archive
 tags
-- 
1.8.5.1.113.g8cb5bef.dirty

Re: [PATCH 1/2] perf completion: complete 'perf kvm'

[Ramkumar Ramachandra]

David Ahern wrote:
> I don't like the command list being embedded in the completion script. It
> will get outdated quickly.

I could introduce a

  $ perf kvm --list-cmds

but that would mean a

  printf("record report stat ..")

in builtin-kvm.c, because the commands aren't in any sort of array.

Re: [PATCH 1/2] perf completion: complete 'perf kvm'

[David Ahern]

On 12/12/13, 2:09 AM, Ramkumar Ramachandra wrote:
> David Ahern wrote:
>> I don't like the command list being embedded in the completion script. It
>> will get outdated quickly.
>
> I could introduce a
>
>    $ perf kvm --list-cmds
>
> but that would mean a
>
>    printf("record report stat ..")
>
> in builtin-kvm.c, because the commands aren't in any sort of array.
>

That would work -- perhaps a #define or string near

    const char * const kvm_usage[] = {
         "perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
         NULL
     };

Building kvm_usage from the string would better - only 1 place listing 
the commands.

David

[PATCH] perf kvm: introduce --list-cmds for use by scripts

[Ramkumar Ramachandra]

Introduce

  $ perf kvm --list-cmds

to dump a raw list of commands for use by the completion script. While
at it, refactor kvm_usage so that there's only one copy of the command
listing.

Cc: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 David Ahern wrote:
 > That would work -- perhaps a #define or string near
 >
 >    const char * const kvm_usage[] = {
 >         "perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
 >         NULL
 >     };
 >
 > Building kvm_usage from the string would better - only 1 place listing the
 > commands.

 Something like this, perhaps? It's not too pretty though: do you have
 suggestions to prettify it?

 tools/perf/builtin-kvm.c      | 25 +++++++++++++++++++++----
 tools/perf/perf-completion.sh |  2 +-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index c6fa3cb..ce44a9b 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1672,6 +1672,7 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	const char *file_name = NULL;
+	bool list_cmds = false;
 	const struct option kvm_options[] = {
 		OPT_STRING('i', "input", &file_name, "file",
 			   "Input file name"),
@@ -1692,20 +1693,36 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 			   "file", "file saving guest os /proc/modules"),
 		OPT_INCR('v', "verbose", &verbose,
 			    "be more verbose (show counter open errors, etc)"),
+		OPT_BOOLEAN(0, "list-cmds", &list_cmds,
+			"list commands raw for use by scripts"),
 		OPT_END()
 	};
 
+	const char *const commands[] = { "top", "record", "report", "diff",
+					 "buildid-list", "stat", NULL };
+	char kvm_usage_str[80];
+	const char *kvm_usage[] = { NULL, NULL };
 
-	const char * const kvm_usage[] = {
-		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
-		NULL
-	};
+	sprintf(kvm_usage_str, "%s", "perf kvm [<options>] {");
+	for (int i = 0; commands[i]; i++) {
+		if (i)
+			strcat(kvm_usage_str, "|");
+		strcat(kvm_usage_str, commands[i]);
+	}
+	strcat(kvm_usage_str, "}");
+
+	kvm_usage[0] = kvm_usage_str;
 
 	perf_host  = 0;
 	perf_guest = 1;
 
 	argc = parse_options(argc, argv, kvm_options, kvm_usage,
 			PARSE_OPT_STOP_AT_NON_OPTION);
+	if (list_cmds) {
+		for (int i = 0; commands[i]; i++)
+			printf("%s ", commands[i]);
+		return 0;
+	}
 	if (!argc)
 		usage_with_options(kvm_usage, kvm_options);
 
diff --git a/tools/perf/perf-completion.sh b/tools/perf/perf-completion.sh
index 496e2ab..d8bfa43 100644
--- a/tools/perf/perf-completion.sh
+++ b/tools/perf/perf-completion.sh
@@ -123,7 +123,7 @@ __perf_main ()
 		__perfcomp_colon "$evts" "$cur"
 	# List subcommands for 'perf kvm'
 	elif [[ $prev == "kvm" ]]; then
-		subcmds="top record report diff buildid-list stat"
+		subcmds=$($cmd kvm --list-cmds)
 		__perfcomp_colon "$subcmds" "$cur"
 	# List long option names
 	elif [[ $cur == --* ]];  then
-- 
1.8.5.1.113.g8cb5bef.dirty

Re: [PATCH] perf kvm: introduce --list-cmds for use by scripts

[David Ahern]

On 12/12/13, 10:26 AM, Ramkumar Ramachandra wrote:
> Introduce
>
>    $ perf kvm --list-cmds
>
> to dump a raw list of commands for use by the completion script. While
> at it, refactor kvm_usage so that there's only one copy of the command
> listing.
>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>   David Ahern wrote:
>   > That would work -- perhaps a #define or string near
>   >
>   >    const char * const kvm_usage[] = {
>   >         "perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
>   >         NULL
>   >     };
>   >
>   > Building kvm_usage from the string would better - only 1 place listing the
>   > commands.
>
>   Something like this, perhaps? It's not too pretty though: do you have
>   suggestions to prettify it?
>
>   tools/perf/builtin-kvm.c      | 25 +++++++++++++++++++++----
>   tools/perf/perf-completion.sh |  2 +-
>   2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index c6fa3cb..ce44a9b 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1672,6 +1672,7 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>   int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>   {
>   	const char *file_name = NULL;
> +	bool list_cmds = false;
>   	const struct option kvm_options[] = {
>   		OPT_STRING('i', "input", &file_name, "file",
>   			   "Input file name"),
> @@ -1692,20 +1693,36 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>   			   "file", "file saving guest os /proc/modules"),
>   		OPT_INCR('v', "verbose", &verbose,
>   			    "be more verbose (show counter open errors, etc)"),
> +		OPT_BOOLEAN(0, "list-cmds", &list_cmds,
> +			"list commands raw for use by scripts"),
>   		OPT_END()
>   	};
>
> +	const char *const commands[] = { "top", "record", "report", "diff",
> +					 "buildid-list", "stat", NULL };

Building it is kind of ugly looking.

Arnaldo: what about this:
#define KVM_CMDS  "top record report diff buildid-list stat"

> +	char kvm_usage_str[80];
> +	const char *kvm_usage[] = { NULL, NULL };
>
> -	const char * const kvm_usage[] = {
> -		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
> -		NULL

	const char * const kvm_usage[] = {
		"perf kvm [<options>] <command>, command = " KVM_CMDS,
		NULL

I would even be fine with not touching kvm_usage and having the KVM_CMDS 
macro right above it -- changing one causes the other to show up in a diff.


David

Re: [PATCH] perf kvm: introduce --list-cmds for use by scripts

[Arnaldo Carvalho de Melo]

Em Thu, Dec 12, 2013 at 10:56:51PM +0530, Ramkumar Ramachandra escreveu:
> Introduce
> 
>   $ perf kvm --list-cmds
> 
> to dump a raw list of commands for use by the completion script. While
> at it, refactor kvm_usage so that there's only one copy of the command
> listing.
> 
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  David Ahern wrote:
>  > That would work -- perhaps a #define or string near
>  >
>  >    const char * const kvm_usage[] = {
>  >         "perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
>  >         NULL
>  >     };
>  >
>  > Building kvm_usage from the string would better - only 1 place listing the
>  > commands.
> 
>  Something like this, perhaps? It's not too pretty though: do you have
>  suggestions to prettify it?

Yes:

Don't do all those things open coded, introduce functions to print,
concat, etc.

The best thing tho, since we have all those sub sub commands in things
like 'perf kvm', 'perf bench', etc, we could have some
parse_options_subcmd, and make the parse options machinery aware of
this, so that it could receive an array of subcmds and when asked for
--list-cmds, would print that sublist, etc, i.e. make sub cmds a first
class citizen.

So I'd suggest that you first introduce functions for doing the concat
to pass to the current infrastructure, so that we have what your patch
provides, but prettified, then, as follow on patches, you could work on
making the options parsing machinery aware of sub cmds.

Ah, and try not using fixed sized arrays, or at least verify that space
is available, i.e. never use strcat, use strncat, better, take a look at
tools/perf/util/strbuf.h, I guess you can use it to build the string for
you in a safe way and expanding the buffer as needed, etc.

- Arnaldo
 
>  tools/perf/builtin-kvm.c      | 25 +++++++++++++++++++++----
>  tools/perf/perf-completion.sh |  2 +-
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index c6fa3cb..ce44a9b 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1672,6 +1672,7 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
>  int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	const char *file_name = NULL;
> +	bool list_cmds = false;
>  	const struct option kvm_options[] = {
>  		OPT_STRING('i', "input", &file_name, "file",
>  			   "Input file name"),
> @@ -1692,20 +1693,36 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>  			   "file", "file saving guest os /proc/modules"),
>  		OPT_INCR('v', "verbose", &verbose,
>  			    "be more verbose (show counter open errors, etc)"),
> +		OPT_BOOLEAN(0, "list-cmds", &list_cmds,
> +			"list commands raw for use by scripts"),
>  		OPT_END()
>  	};
>  
> +	const char *const commands[] = { "top", "record", "report", "diff",
> +					 "buildid-list", "stat", NULL };
> +	char kvm_usage_str[80];
> +	const char *kvm_usage[] = { NULL, NULL };
>  
> -	const char * const kvm_usage[] = {
> -		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
> -		NULL
> -	};
> +	sprintf(kvm_usage_str, "%s", "perf kvm [<options>] {");
> +	for (int i = 0; commands[i]; i++) {
> +		if (i)
> +			strcat(kvm_usage_str, "|");
> +		strcat(kvm_usage_str, commands[i]);
> +	}
> +	strcat(kvm_usage_str, "}");
> +
> +	kvm_usage[0] = kvm_usage_str;
>  
>  	perf_host  = 0;
>  	perf_guest = 1;
>  
>  	argc = parse_options(argc, argv, kvm_options, kvm_usage,
>  			PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (list_cmds) {
> +		for (int i = 0; commands[i]; i++)
> +			printf("%s ", commands[i]);
> +		return 0;
> +	}
>  	if (!argc)
>  		usage_with_options(kvm_usage, kvm_options);
>  
> diff --git a/tools/perf/perf-completion.sh b/tools/perf/perf-completion.sh
> index 496e2ab..d8bfa43 100644
> --- a/tools/perf/perf-completion.sh
> +++ b/tools/perf/perf-completion.sh
> @@ -123,7 +123,7 @@ __perf_main ()
>  		__perfcomp_colon "$evts" "$cur"
>  	# List subcommands for 'perf kvm'
>  	elif [[ $prev == "kvm" ]]; then
> -		subcmds="top record report diff buildid-list stat"
> +		subcmds=$($cmd kvm --list-cmds)
>  		__perfcomp_colon "$subcmds" "$cur"
>  	# List long option names
>  	elif [[ $cur == --* ]];  then
> -- 
> 1.8.5.1.113.g8cb5bef.dirty

[tip:perf/core] perf completion: Complete 'perf kvm'

[tip-bot for Ramkumar Ramachandra]

Commit-ID:  8f2f5ada719560954174da30ce0a67261c616e39
Gitweb:     http://git.kernel.org/tip/8f2f5ada719560954174da30ce0a67261c616e39
Author:     Ramkumar Ramachandra <artagnon@gmail.com>
AuthorDate: Wed, 11 Dec 2013 16:04:15 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 13 Dec 2013 10:30:21 -0300

perf completion: Complete 'perf kvm'

Currently, there is no way to enumerate the subcommands under 'perf
kvm', so hardcode them.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Link: http://lkml.kernel.org/r/1386758056-24618-2-git-send-email-artagnon@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/perf-completion.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/perf-completion.sh b/tools/perf/perf-completion.sh
index 4949488..496e2ab 100644
--- a/tools/perf/perf-completion.sh
+++ b/tools/perf/perf-completion.sh
@@ -121,6 +121,10 @@ __perf_main ()
 	elif [[ $prev == "-e" && "${words[1]}" == @(record|stat|top) ]]; then
 		evts=$($cmd list --raw-dump)
 		__perfcomp_colon "$evts" "$cur"
+	# List subcommands for 'perf kvm'
+	elif [[ $prev == "kvm" ]]; then
+		subcmds="top record report diff buildid-list stat"
+		__perfcomp_colon "$subcmds" "$cur"
 	# List long option names
 	elif [[ $cur == --* ]];  then
 		subcmd=${words[1]}