[cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[Dominik Brodowski]

Currently, one needs to set the cpulist _before_ the subcommand, i.e.

	cpupower [ -c cpulist ] subcommand [ARGS]

However, cpufrequtils used to allow to have "-c CPU" as a parameter to
cpufreq-set(1) or cpufreq-info(1). Therefore, it may make sense to also
support

	cpupower subcommand [ARGS#1] [ -c cpulist ] [ARGS#2]

The attached patch attempts to do so, but I'm not yet 100% convinced myself
that a) we should do this at all, and b) that I chose the right approach.

Best,
	Dominik

diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c
index 5844ae0..1bb02df 100644
--- a/tools/power/cpupower/utils/cpupower.c
+++ b/tools/power/cpupower/utils/cpupower.c
@@ -101,20 +101,26 @@ static void print_version(void)
 	printf(_("Report errors and bugs to %s, please.\n"), PACKAGE_BUGREPORT);
 }
 
-static void handle_options(int *argc, const char ***argv)
+static char **handle_options(int *argc, const char ***argv)
 {
 	int ret, x, new_argc = 0;
+	int before_command = 1;
+	char **new_argv;
 
 	if (*argc < 1)
-		return;
+		return NULL;
 
-	for (x = 0;  x < *argc && ((*argv)[x])[0] == '-'; x++) {
+	new_argv = calloc(*argc, sizeof(unsigned long));
+	for (x = 0;  x < *argc; x++) {
 		const char *param = (*argv)[x];
-		if (!strcmp(param, "-h") || !strcmp(param, "--help")) {
+		if (param[0] != '-')
+			before_command = 0;
+		if ((!strcmp(param, "-h") || !strcmp(param, "--help"))
+			&& before_command) {
 			print_help();
 			exit(EXIT_SUCCESS);
 		} else if (!strcmp(param, "-c") || !strcmp(param, "--cpu")) {
-			if (*argc < 2) {
+			if ((*argc < 2) || (*argc == x + 1)) {
 				print_help();
 				exit(EXIT_FAILURE);
 			}
@@ -129,9 +135,8 @@ static void handle_options(int *argc, const char ***argv)
 					exit(EXIT_FAILURE);
 				}
 			}
+			/* Cut out the next param as well */
 			x += 1;
-			/* Cut out param: cpupower -c 1 info -> cpupower info */
-			new_argc += 2;
 			continue;
 		} else if (!strcmp(param, "-v") ||
 			!strcmp(param, "--version")) {
@@ -140,30 +145,31 @@ static void handle_options(int *argc, const char ***argv)
 #ifdef DEBUG
 		} else if (!strcmp(param, "-d") || !strcmp(param, "--debug")) {
 			be_verbose = 1;
-			new_argc++;
-			continue;
 #endif
-		} else {
-			fprintf(stderr, "Unknown option: %s\n", param);
+		} else if (before_command) {
+			fprintf(stderr, "1 Unknown option: %s\n", param);
 			print_help();
 			exit(EXIT_FAILURE);
 		}
+		new_argv[new_argc] = strdup(param);
+		new_argc += 1;
 	}
-	*argc -= new_argc;
-	*argv += new_argc;
+	*argc = new_argc;
+	return new_argv;
 }
 
-int main(int argc, const char *argv[])
+int main(int argc, const char *i_argv[])
 {
-	const char *cmd;
+	char *cmd;
 	unsigned int i, ret;
+	char **argv;
 
 	cpus_chosen = bitmask_alloc(sysconf(_SC_NPROCESSORS_CONF));
 
 	argc--;
-	argv += 1;
+	i_argv += 1;
 
-	handle_options(&argc, &argv);
+	argv = handle_options(&argc, &i_argv);
 
 	cmd = argv[0];
 
@@ -193,7 +199,7 @@ int main(int argc, const char *argv[])
 					  "privileges\n"), cmd);
 			return EXIT_FAILURE;
 		}
-		ret = p->main(argc, argv);
+		ret = p->main(argc, (const char **) argv);
 		if (cpus_chosen)
 			bitmask_free(cpus_chosen);
 		return ret;

Re: [cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[Thomas Renninger]

On Saturday 06 August 2011 17:18:29 Dominik Brodowski wrote:
> Currently, one needs to set the cpulist _before_ the subcommand, i.e.
> 
> 	cpupower [ -c cpulist ] subcommand [ARGS]
> 
> However, cpufrequtils used to allow to have "-c CPU" as a parameter to
> cpufreq-set(1) or cpufreq-info(1). Therefore, it may make sense to also
> support
Why exactly would this be an enhancement?
To be able to create an alias easier?

> 	cpupower subcommand [ARGS#1] [ -c cpulist ] [ARGS#2]
> 
> The attached patch attempts to do so, but I'm not yet 100% convinced myself
> that a) we should do this at all, and b) that I chose the right approach.
I'd not try to do that.

   Thomas

Re: [cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[Dominik Brodowski]

On Wed, Aug 10, 2011 at 09:00:11AM +0200, Thomas Renninger wrote:
> On Saturday 06 August 2011 17:18:29 Dominik Brodowski wrote:
> > Currently, one needs to set the cpulist _before_ the subcommand, i.e.
> > 
> > 	cpupower [ -c cpulist ] subcommand [ARGS]
> > 
> > However, cpufrequtils used to allow to have "-c CPU" as a parameter to
> > cpufreq-set(1) or cpufreq-info(1). Therefore, it may make sense to also
> > support
> Why exactly would this be an enhancement?
> To be able to create an alias easier?

Exactly. Also, 
	$ cpufreq-set -c 0 -g powersave
would translate more intuitively to
	$ cpupower frequency-set -c 0 g powersave

> > 	cpupower subcommand [ARGS#1] [ -c cpulist ] [ARGS#2]
> > 
> > The attached patch attempts to do so, but I'm not yet 100% convinced myself
> > that a) we should do this at all, and b) that I chose the right approach.
> I'd not try to do that.

Best,
	Dominik

Re: [cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[Thomas Renninger]

On Wednesday 10 August 2011 13:27:20 Dominik Brodowski wrote:
> On Wed, Aug 10, 2011 at 09:00:11AM +0200, Thomas Renninger wrote:
> > On Saturday 06 August 2011 17:18:29 Dominik Brodowski wrote:
> > > Currently, one needs to set the cpulist _before_ the subcommand, i.e.
> > > 
> > > 	cpupower [ -c cpulist ] subcommand [ARGS]
> > > 
> > > However, cpufrequtils used to allow to have "-c CPU" as a parameter to
> > > cpufreq-set(1) or cpufreq-info(1). Therefore, it may make sense to also
> > > support
> > Why exactly would this be an enhancement?
> > To be able to create an alias easier?
> 
> Exactly.

I also tried with "being able to do an alias" without success.
One idea could be to re-implement -c evaluation in frequency_info.c
BUT be aware that default switched:
  - with cpufrequtils cpufreq-info by default showed all cpus
  - with cpupower all read commands only show cpu 0 by default

  - with cpufrequtils cpufreq-set only sets the first cpu
  - with cpupower all write commands write to all cpus
and this is on purpose and should not change.
So unfortunately an alias won't be possible.

   Thomas

Re: [cpupower] RFC: support "cpupower subcommand [ -c cpulist ]"

[Dominik Brodowski]

Hey,

On Thu, Aug 11, 2011 at 11:48:40PM +0200, Thomas Renninger wrote:
> On Wednesday 10 August 2011 13:27:20 Dominik Brodowski wrote:
> > On Wed, Aug 10, 2011 at 09:00:11AM +0200, Thomas Renninger wrote:
> > > On Saturday 06 August 2011 17:18:29 Dominik Brodowski wrote:
> > > > Currently, one needs to set the cpulist _before_ the subcommand, i.e.
> > > > 
> > > > 	cpupower [ -c cpulist ] subcommand [ARGS]
> > > > 
> > > > However, cpufrequtils used to allow to have "-c CPU" as a parameter to
> > > > cpufreq-set(1) or cpufreq-info(1). Therefore, it may make sense to also
> > > > support
> > > Why exactly would this be an enhancement?
> > > To be able to create an alias easier?
> > 
> > Exactly.
> 
> I also tried with "being able to do an alias" without success.
> One idea could be to re-implement -c evaluation in frequency_info.c
> BUT be aware that default switched:
>   - with cpufrequtils cpufreq-info by default showed all cpus
>   - with cpupower all read commands only show cpu 0 by default
> 
>   - with cpufrequtils cpufreq-set only sets the first cpu
>   - with cpupower all write commands write to all cpus
> and this is on purpose and should not change.
> So unfortunately an alias won't be possible.

Well, it wouldn't be a backwards-compatible alias. But still, might it be
worthwhile? What do you (and others reading this) think?

Best,
	Dominik