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

[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

[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. In
order to do this, introduce parse_options_subcommand() for handling
subcommands as a special case in the parse-options machinery.

Cc: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Does this example justify the creation of parse_options_subcommand()?
 Initializing the usagestr to { NULL, NULL } and passing it to
 parse_options_subcommand() isn't intuitive -- what can we do about
 that?

 tools/perf/builtin-kvm.c        | 12 +++++-------
 tools/perf/perf-completion.sh   |  2 +-
 tools/perf/util/parse-options.c | 37 +++++++++++++++++++++++++++++++++----
 tools/perf/util/parse-options.h |  8 +++++++-
 4 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index a735051..21c164b 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1691,17 +1691,15 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 		OPT_END()
 	};
 
-
-	const char * const kvm_usage[] = {
-		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
-		NULL
-	};
+	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
+						"buildid-list", "stat", NULL };
+	const char *kvm_usage[] = { NULL, NULL };
 
 	perf_host  = 0;
 	perf_guest = 1;
 
-	argc = parse_options(argc, argv, kvm_options, kvm_usage,
-			PARSE_OPT_STOP_AT_NON_OPTION);
+	argc = parse_options_subcommand(argc, argv, kvm_options, kvm_subcommands, kvm_usage,
+					PARSE_OPT_STOP_AT_NON_OPTION);
 	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..ae3a576 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 $prev --list-cmds)
 		__perfcomp_colon "$subcmds" "$cur"
 	# List long option names
 	elif [[ $cur == --* ]];  then
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index d22e3f8..bf48092 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -407,7 +407,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		if (internal_help && !strcmp(arg + 2, "help"))
 			return usage_with_options_internal(usagestr, options, 0);
 		if (!strcmp(arg + 2, "list-opts"))
-			return PARSE_OPT_LIST;
+			return PARSE_OPT_LIST_OPTS;
+		if (!strcmp(arg + 2, "list-cmds"))
+			return PARSE_OPT_LIST_SUBCMDS;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
 		case -1:
 			return parse_options_usage(usagestr, options, arg + 2, 0);
@@ -433,25 +435,45 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-int parse_options(int argc, const char **argv, const struct option *options,
-		  const char * const usagestr[], int flags)
+int parse_options_subcommand(int argc, const char **argv, const struct option *options,
+			const char *const subcommands[], const char *usagestr[], int flags)
 {
 	struct parse_opt_ctx_t ctx;
 
 	perf_header__set_cmdline(argc, argv);
 
+	/* build usage string if it's not provided */
+	if (subcommands && !usagestr[0]) {
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_addf(&buf, "perf %s [<options>] {", argv[0]);
+		for (int i = 0; subcommands[i]; i++) {
+			if (i)
+				strbuf_addstr(&buf, "|");
+			strbuf_addstr(&buf, subcommands[i]);
+		}
+		strbuf_addstr(&buf, "}");
+
+		usagestr[0] = strdup(buf.buf);
+		strbuf_release(&buf);
+	}
+
 	parse_options_start(&ctx, argc, argv, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
 	case PARSE_OPT_DONE:
 		break;
-	case PARSE_OPT_LIST:
+	case PARSE_OPT_LIST_OPTS:
 		while (options->type != OPTION_END) {
 			printf("--%s ", options->long_name);
 			options++;
 		}
 		exit(130);
+	case PARSE_OPT_LIST_SUBCMDS:
+		for (int i = 0; subcommands[i]; i++)
+			printf("%s ", subcommands[i]);
+		exit(130);
 	default: /* PARSE_OPT_UNKNOWN */
 		if (ctx.argv[0][1] == '-') {
 			error("unknown option `%s'", ctx.argv[0] + 2);
@@ -464,6 +486,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	return parse_options_end(&ctx);
 }
 
+int parse_options(int argc, const char **argv, const struct option *options,
+		  const char * const usagestr[], int flags)
+{
+	return parse_options_subcommand(argc, argv, options, NULL,
+					(const char **) usagestr, flags);
+}
+
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index cbf0149..d8dac8a 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -140,6 +140,11 @@ extern int parse_options(int argc, const char **argv,
                          const struct option *options,
                          const char * const usagestr[], int flags);
 
+extern int parse_options_subcommand(int argc, const char **argv,
+				const struct option *options,
+				const char *const subcommands[],
+				const char *usagestr[], int flags);
+
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
@@ -148,7 +153,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
-	PARSE_OPT_LIST,
+	PARSE_OPT_LIST_OPTS,
+	PARSE_OPT_LIST_SUBCMDS,
 	PARSE_OPT_UNKNOWN,
 };
 
-- 
1.9.rc0.1.g9d22d25

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

[David Ahern]

On 3/3/14, 6:26 PM, Ramkumar Ramachandra wrote:
> Introduce
>
>    $ perf kvm --list-cmds
>
> to dump a raw list of commands for use by the completion script. In
> order to do this, introduce parse_options_subcommand() for handling
> subcommands as a special case in the parse-options machinery.
>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Looks ok to me. Jiri?

David

> ---
>   Does this example justify the creation of parse_options_subcommand()?
>   Initializing the usagestr to { NULL, NULL } and passing it to
>   parse_options_subcommand() isn't intuitive -- what can we do about
>   that?
>
>   tools/perf/builtin-kvm.c        | 12 +++++-------
>   tools/perf/perf-completion.sh   |  2 +-
>   tools/perf/util/parse-options.c | 37 +++++++++++++++++++++++++++++++++----
>   tools/perf/util/parse-options.h |  8 +++++++-
>   4 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index a735051..21c164b 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1691,17 +1691,15 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>   		OPT_END()
>   	};
>
> -
> -	const char * const kvm_usage[] = {
> -		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
> -		NULL
> -	};
> +	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
> +						"buildid-list", "stat", NULL };
> +	const char *kvm_usage[] = { NULL, NULL };
>
>   	perf_host  = 0;
>   	perf_guest = 1;
>
> -	argc = parse_options(argc, argv, kvm_options, kvm_usage,
> -			PARSE_OPT_STOP_AT_NON_OPTION);
> +	argc = parse_options_subcommand(argc, argv, kvm_options, kvm_subcommands, kvm_usage,
> +					PARSE_OPT_STOP_AT_NON_OPTION);
>   	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..ae3a576 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 $prev --list-cmds)
>   		__perfcomp_colon "$subcmds" "$cur"
>   	# List long option names
>   	elif [[ $cur == --* ]];  then
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index d22e3f8..bf48092 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -407,7 +407,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>   		if (internal_help && !strcmp(arg + 2, "help"))
>   			return usage_with_options_internal(usagestr, options, 0);
>   		if (!strcmp(arg + 2, "list-opts"))
> -			return PARSE_OPT_LIST;
> +			return PARSE_OPT_LIST_OPTS;
> +		if (!strcmp(arg + 2, "list-cmds"))
> +			return PARSE_OPT_LIST_SUBCMDS;
>   		switch (parse_long_opt(ctx, arg + 2, options)) {
>   		case -1:
>   			return parse_options_usage(usagestr, options, arg + 2, 0);
> @@ -433,25 +435,45 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
>   	return ctx->cpidx + ctx->argc;
>   }
>
> -int parse_options(int argc, const char **argv, const struct option *options,
> -		  const char * const usagestr[], int flags)
> +int parse_options_subcommand(int argc, const char **argv, const struct option *options,
> +			const char *const subcommands[], const char *usagestr[], int flags)
>   {
>   	struct parse_opt_ctx_t ctx;
>
>   	perf_header__set_cmdline(argc, argv);
>
> +	/* build usage string if it's not provided */
> +	if (subcommands && !usagestr[0]) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_addf(&buf, "perf %s [<options>] {", argv[0]);
> +		for (int i = 0; subcommands[i]; i++) {
> +			if (i)
> +				strbuf_addstr(&buf, "|");
> +			strbuf_addstr(&buf, subcommands[i]);
> +		}
> +		strbuf_addstr(&buf, "}");
> +
> +		usagestr[0] = strdup(buf.buf);
> +		strbuf_release(&buf);
> +	}
> +
>   	parse_options_start(&ctx, argc, argv, flags);
>   	switch (parse_options_step(&ctx, options, usagestr)) {
>   	case PARSE_OPT_HELP:
>   		exit(129);
>   	case PARSE_OPT_DONE:
>   		break;
> -	case PARSE_OPT_LIST:
> +	case PARSE_OPT_LIST_OPTS:
>   		while (options->type != OPTION_END) {
>   			printf("--%s ", options->long_name);
>   			options++;
>   		}
>   		exit(130);
> +	case PARSE_OPT_LIST_SUBCMDS:
> +		for (int i = 0; subcommands[i]; i++)
> +			printf("%s ", subcommands[i]);
> +		exit(130);
>   	default: /* PARSE_OPT_UNKNOWN */
>   		if (ctx.argv[0][1] == '-') {
>   			error("unknown option `%s'", ctx.argv[0] + 2);
> @@ -464,6 +486,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
>   	return parse_options_end(&ctx);
>   }
>
> +int parse_options(int argc, const char **argv, const struct option *options,
> +		  const char * const usagestr[], int flags)
> +{
> +	return parse_options_subcommand(argc, argv, options, NULL,
> +					(const char **) usagestr, flags);
> +}
> +
>   #define USAGE_OPTS_WIDTH 24
>   #define USAGE_GAP         2
>
> diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> index cbf0149..d8dac8a 100644
> --- a/tools/perf/util/parse-options.h
> +++ b/tools/perf/util/parse-options.h
> @@ -140,6 +140,11 @@ extern int parse_options(int argc, const char **argv,
>                            const struct option *options,
>                            const char * const usagestr[], int flags);
>
> +extern int parse_options_subcommand(int argc, const char **argv,
> +				const struct option *options,
> +				const char *const subcommands[],
> +				const char *usagestr[], int flags);
> +
>   extern NORETURN void usage_with_options(const char * const *usagestr,
>                                           const struct option *options);
>
> @@ -148,7 +153,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
>   enum {
>   	PARSE_OPT_HELP = -1,
>   	PARSE_OPT_DONE,
> -	PARSE_OPT_LIST,
> +	PARSE_OPT_LIST_OPTS,
> +	PARSE_OPT_LIST_SUBCMDS,
>   	PARSE_OPT_UNKNOWN,
>   };
>
>

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

[Jiri Olsa]

On Mon, Mar 03, 2014 at 08:26:36PM -0500, Ramkumar Ramachandra wrote:
> Introduce
> 
>   $ perf kvm --list-cmds
> 
> to dump a raw list of commands for use by the completion script. In
> order to do this, introduce parse_options_subcommand() for handling
> subcommands as a special case in the parse-options machinery.

so this doesn't change the behaviour at all, right?

SNIP

>  
> -	argc = parse_options(argc, argv, kvm_options, kvm_usage,
> -			PARSE_OPT_STOP_AT_NON_OPTION);
> +	argc = parse_options_subcommand(argc, argv, kvm_options, kvm_subcommands, kvm_usage,
> +					PARSE_OPT_STOP_AT_NON_OPTION);
>  	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..ae3a576 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 $prev --list-cmds)

Do we want some generic approach here.. if we're
adding generic --list-cmds option anyway.. hm?

SNIP

> -int parse_options(int argc, const char **argv, const struct option *options,
> -		  const char * const usagestr[], int flags)
> +int parse_options_subcommand(int argc, const char **argv, const struct option *options,
> +			const char *const subcommands[], const char *usagestr[], int flags)
>  {
>  	struct parse_opt_ctx_t ctx;
>  
>  	perf_header__set_cmdline(argc, argv);
>  
> +	/* build usage string if it's not provided */
> +	if (subcommands && !usagestr[0]) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_addf(&buf, "perf %s [<options>] {", argv[0]);
> +		for (int i = 0; subcommands[i]; i++) {
> +			if (i)
> +				strbuf_addstr(&buf, "|");
> +			strbuf_addstr(&buf, subcommands[i]);
> +		}
> +		strbuf_addstr(&buf, "}");
> +
> +		usagestr[0] = strdup(buf.buf);
> +		strbuf_release(&buf);
> +	}

is above code ever used now?

jirka

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

[Ramkumar Ramachandra]

Jiri Olsa wrote:
> On Mon, Mar 03, 2014 at 08:26:36PM -0500, Ramkumar Ramachandra wrote:
>> Introduce
>>
>>   $ perf kvm --list-cmds
>>
>> to dump a raw list of commands for use by the completion script. In
>> order to do this, introduce parse_options_subcommand() for handling
>> subcommands as a special case in the parse-options machinery.
>
> so this doesn't change the behaviour at all, right?

It just introduces --list-cmds; no, it doesn't change behavior otherwise.

>>
>> -     argc = parse_options(argc, argv, kvm_options, kvm_usage,
>> -                     PARSE_OPT_STOP_AT_NON_OPTION);
>> +     argc = parse_options_subcommand(argc, argv, kvm_options, kvm_subcommands, kvm_usage,
>> +                                     PARSE_OPT_STOP_AT_NON_OPTION);
>>       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..ae3a576 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 $prev --list-cmds)
>
> Do we want some generic approach here.. if we're
> adding generic --list-cmds option anyway.. hm?

Currently, we're using $cmd $prev --list-cmds instead of hard-coding a
list for 'perf kvm'. Once I submit future patches to convert other
commands to use parse_options_subcommand(), we can use the same line
for completions.

Or did you mean something else when you said generic approach?

>> -int parse_options(int argc, const char **argv, const struct option *options,
>> -               const char * const usagestr[], int flags)
>> +int parse_options_subcommand(int argc, const char **argv, const struct option *options,
>> +                     const char *const subcommands[], const char *usagestr[], int flags)
>>  {
>>       struct parse_opt_ctx_t ctx;
>>
>>       perf_header__set_cmdline(argc, argv);
>>
>> +     /* build usage string if it's not provided */
>> +     if (subcommands && !usagestr[0]) {
>> +             struct strbuf buf = STRBUF_INIT;
>> +
>> +             strbuf_addf(&buf, "perf %s [<options>] {", argv[0]);
>> +             for (int i = 0; subcommands[i]; i++) {
>> +                     if (i)
>> +                             strbuf_addstr(&buf, "|");
>> +                     strbuf_addstr(&buf, subcommands[i]);
>> +             }
>> +             strbuf_addstr(&buf, "}");
>> +
>> +             usagestr[0] = strdup(buf.buf);
>> +             strbuf_release(&buf);
>> +     }
>
> is above code ever used now?

Yeah; note that kvm_usage is initialized to { NULL, NULL } and passed
-- this code fills in the usage string then.

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

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Jiri Olsa wrote:
>> On Mon, Mar 03, 2014 at 08:26:36PM -0500, Ramkumar Ramachandra wrote:
>>> Introduce
>>>
>>>   $ perf kvm --list-cmds
>>>
>>> to dump a raw list of commands for use by the completion script. In
>>> order to do this, introduce parse_options_subcommand() for handling
>>> subcommands as a special case in the parse-options machinery.

Jiri? How do we proceed with this?

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

[Jiri Olsa]

On Thu, Mar 13, 2014 at 11:52:12AM -0400, Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra wrote:
> > Jiri Olsa wrote:
> >> On Mon, Mar 03, 2014 at 08:26:36PM -0500, Ramkumar Ramachandra wrote:
> >>> Introduce
> >>>
> >>>   $ perf kvm --list-cmds
> >>>
> >>> to dump a raw list of commands for use by the completion script. In
> >>> order to do this, introduce parse_options_subcommand() for handling
> >>> subcommands as a special case in the parse-options machinery.
> 
> Jiri? How do we proceed with this?

I think Arnaldo will process it eventually ;-)
I dont have any objections..

jirka

[tip:perf/core] perf kvm: introduce --list-cmds for use by scripts

[tip-bot for Ramkumar Ramachandra]

Commit-ID:  09a71b97cce70551356b13b668aa1d7d6da84457
Gitweb:     http://git.kernel.org/tip/09a71b97cce70551356b13b668aa1d7d6da84457
Author:     Ramkumar Ramachandra <artagnon@gmail.com>
AuthorDate: Mon, 3 Mar 2014 20:26:36 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 14 Mar 2014 18:08:41 -0300

perf kvm: introduce --list-cmds for use by scripts

Introduce

  $ perf kvm --list-cmds

to dump a raw list of commands for use by the completion script. In
order to do this, introduce parse_options_subcommand() for handling
subcommands as a special case in the parse-options machinery.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Acked-by: David Ahern <dsahern@gmail.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1393896396-10427-1-git-send-email-artagnon@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-kvm.c        | 12 +++++-------
 tools/perf/perf-completion.sh   |  2 +-
 tools/perf/util/parse-options.c | 37 +++++++++++++++++++++++++++++++++----
 tools/perf/util/parse-options.h |  8 +++++++-
 4 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index a735051..21c164b 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1691,17 +1691,15 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 		OPT_END()
 	};
 
-
-	const char * const kvm_usage[] = {
-		"perf kvm [<options>] {top|record|report|diff|buildid-list|stat}",
-		NULL
-	};
+	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
+						"buildid-list", "stat", NULL };
+	const char *kvm_usage[] = { NULL, NULL };
 
 	perf_host  = 0;
 	perf_guest = 1;
 
-	argc = parse_options(argc, argv, kvm_options, kvm_usage,
-			PARSE_OPT_STOP_AT_NON_OPTION);
+	argc = parse_options_subcommand(argc, argv, kvm_options, kvm_subcommands, kvm_usage,
+					PARSE_OPT_STOP_AT_NON_OPTION);
 	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..ae3a576 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 $prev --list-cmds)
 		__perfcomp_colon "$subcmds" "$cur"
 	# List long option names
 	elif [[ $cur == --* ]];  then
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index d22e3f8..bf48092 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -407,7 +407,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		if (internal_help && !strcmp(arg + 2, "help"))
 			return usage_with_options_internal(usagestr, options, 0);
 		if (!strcmp(arg + 2, "list-opts"))
-			return PARSE_OPT_LIST;
+			return PARSE_OPT_LIST_OPTS;
+		if (!strcmp(arg + 2, "list-cmds"))
+			return PARSE_OPT_LIST_SUBCMDS;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
 		case -1:
 			return parse_options_usage(usagestr, options, arg + 2, 0);
@@ -433,25 +435,45 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-int parse_options(int argc, const char **argv, const struct option *options,
-		  const char * const usagestr[], int flags)
+int parse_options_subcommand(int argc, const char **argv, const struct option *options,
+			const char *const subcommands[], const char *usagestr[], int flags)
 {
 	struct parse_opt_ctx_t ctx;
 
 	perf_header__set_cmdline(argc, argv);
 
+	/* build usage string if it's not provided */
+	if (subcommands && !usagestr[0]) {
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_addf(&buf, "perf %s [<options>] {", argv[0]);
+		for (int i = 0; subcommands[i]; i++) {
+			if (i)
+				strbuf_addstr(&buf, "|");
+			strbuf_addstr(&buf, subcommands[i]);
+		}
+		strbuf_addstr(&buf, "}");
+
+		usagestr[0] = strdup(buf.buf);
+		strbuf_release(&buf);
+	}
+
 	parse_options_start(&ctx, argc, argv, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
 	case PARSE_OPT_DONE:
 		break;
-	case PARSE_OPT_LIST:
+	case PARSE_OPT_LIST_OPTS:
 		while (options->type != OPTION_END) {
 			printf("--%s ", options->long_name);
 			options++;
 		}
 		exit(130);
+	case PARSE_OPT_LIST_SUBCMDS:
+		for (int i = 0; subcommands[i]; i++)
+			printf("%s ", subcommands[i]);
+		exit(130);
 	default: /* PARSE_OPT_UNKNOWN */
 		if (ctx.argv[0][1] == '-') {
 			error("unknown option `%s'", ctx.argv[0] + 2);
@@ -464,6 +486,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	return parse_options_end(&ctx);
 }
 
+int parse_options(int argc, const char **argv, const struct option *options,
+		  const char * const usagestr[], int flags)
+{
+	return parse_options_subcommand(argc, argv, options, NULL,
+					(const char **) usagestr, flags);
+}
+
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index cbf0149..d8dac8a 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -140,6 +140,11 @@ extern int parse_options(int argc, const char **argv,
                          const struct option *options,
                          const char * const usagestr[], int flags);
 
+extern int parse_options_subcommand(int argc, const char **argv,
+				const struct option *options,
+				const char *const subcommands[],
+				const char *usagestr[], int flags);
+
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
@@ -148,7 +153,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
-	PARSE_OPT_LIST,
+	PARSE_OPT_LIST_OPTS,
+	PARSE_OPT_LIST_SUBCMDS,
 	PARSE_OPT_UNKNOWN,
 };