Xen devel - Apr 2012 - [PATCH] xl: Don't require a config file for cpupools

Since the key information can be fairly simply put on the command-line,
there''s no need to require an actual config file.

Also improve the help to cross-reference the xlcpupool.cfg manpage.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 30cc13e25e01 -r 0fb728d56bae docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Tue Apr 03 19:02:19 2012 +0100
+++ b/docs/man/xl.pod.1	Tue Apr 03 19:28:04 2012 +0100
@@ -848,11 +848,13 @@ Cpu-pools can be specified either by nam
 
 =over 4
 
-=item B<cpupool-create> [I<OPTIONS>] I<ConfigFile>
[I<Variable=Value> ...]
+=item B<cpupool-create> [I<OPTIONS>] [I<ConfigFile>]
[I<Variable=Value> ...]
 
-Create a cpu pool based an I<ConfigFile>.
+Create a cpu pool based an config from a I<ConfigFile> or command-line
parameters.
 Variable settings from the I<ConfigFile> may be altered by specifying new
-or additional assignments on the command line.
+or additional assignments on the command line.  
+
+See the xlcpupool.cfg manpage for more information.
 
 B<OPTIONS>
 
diff -r 30cc13e25e01 -r 0fb728d56bae tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Apr 03 19:02:19 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Tue Apr 03 19:28:04 2012 +0100
@@ -5407,7 +5407,7 @@ int main_tmem_freeable(int argc, char **
 
 int main_cpupoolcreate(int argc, char **argv)
 {
-    const char *filename = NULL;
+    const char *filename = NULL, *config_src=NULL;
     const char *p;
     char extra_config[1024];
     int opt;
@@ -5471,23 +5471,26 @@ int main_cpupoolcreate(int argc, char **
         optind++;
     }
 
-    if (!filename) {
-        help("cpupool-create");
-        return -ERROR_FAIL;
-    }
-
-    if (libxl_read_file_contents(ctx, filename, (void **)&config_data,
&config_len)) {
-        fprintf(stderr, "Failed to read config file: %s: %s\n",
-                filename, strerror(errno));
-        return -ERROR_FAIL;
-    }
+    if (filename)
+    {
+        if (libxl_read_file_contents(ctx, filename, (void **)&config_data,
+                                     &config_len)) {
+            fprintf(stderr, "Failed to read config file: %s: %s\n",
+                    filename, strerror(errno));
+            return -ERROR_FAIL;
+        }
+        config_src=filename;
+    }
+    else
+        config_src="command line";
+
     if (strlen(extra_config)) {
         if (config_len > INT_MAX - (strlen(extra_config) + 2)) {
             fprintf(stderr, "Failed to attach extra configration\n");
             return -ERROR_FAIL;
         }
         config_data = xrealloc(config_data,
-                              config_len + strlen(extra_config) + 2);
+                               config_len + strlen(extra_config) + 2);
         if (!config_data) {
             fprintf(stderr, "Failed to realloc config_data\n");
             return -ERROR_FAIL;
@@ -5498,7 +5501,7 @@ int main_cpupoolcreate(int argc, char **
         config_len += strlen(extra_config) + 1;
     }
 
-    config = xlu_cfg_init(stderr, filename);
+    config = xlu_cfg_init(stderr, config_src);
     if (!config) {
         fprintf(stderr, "Failed to allocate for configuration\n");
         return -ERROR_FAIL;
@@ -5512,8 +5515,12 @@ int main_cpupoolcreate(int argc, char **
 
     if (!xlu_cfg_get_string (config, "name", &buf, 0))
         name = strdup(buf);
-    else
+    else if (filename)
         name = libxl_basename(filename);
+    else {
+        fprintf(stderr, "Missing cpupool name!\n");
+        return -ERROR_FAIL;
+    }
     if (!libxl_name_to_cpupoolid(ctx, name, &poolid)) {
         fprintf(stderr, "Pool name \"%s\" already
exists\n", name);
         return -ERROR_FAIL;
@@ -5595,7 +5602,7 @@ int main_cpupoolcreate(int argc, char **
 
     libxl_uuid_generate(&uuid);
 
-    printf("Using config file \"%s\"\n", filename);
+    printf("Using config file \"%s\"\n", config_src);
     printf("cpupool name:   %s\n", name);
     printf("scheduler:      %s\n", libxl_scheduler_to_string(sched));
     printf("number of cpus: %d\n", n_cpus);
diff -r 30cc13e25e01 -r 0fb728d56bae tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Tue Apr 03 19:02:19 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Tue Apr 03 19:28:04 2012 +0100
@@ -364,12 +364,14 @@ struct cmd_spec cmd_table[] = {
     },
     { "cpupool-create",
       &main_cpupoolcreate, 1,
-      "Create a CPU pool based an ConfigFile",
-      "[options] <ConfigFile> [vars]",
+      "Create a new CPU pool",
+      "[options] [<ConfigFile>] [Variable=value ...]",
       "-h, --help                   Print this help.\n"
       "-f FILE, --defconfig=FILE    Use the given configuration
file.\n"
       "-n, --dryrun                 Dry run - prints the resulting
configuration.\n"
-      "                              (deprecated in favour of global -N
option)."
+      "                              (deprecated in favour of global -N
option).\n"
+      "\nSee the xlcpupool.cfg manpage for more information.",
+
     },
     { "cpupool-list",
       &main_cpupoollist, 0,

On Tue, 2012-04-03 at 19:29 +0100, George Dunlap wrote:
> Since the key information can be fairly simply put on the command-line,
> there''s no need to require an actual config file.
> 
> Also improve the help to cross-reference the xlcpupool.cfg manpage.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Code looks good to me, couple of comments on the docs aspects below.
> diff -r 30cc13e25e01 -r 0fb728d56bae docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1	Tue Apr 03 19:02:19 2012 +0100
> +++ b/docs/man/xl.pod.1	Tue Apr 03 19:28:04 2012 +0100
> @@ -848,11 +848,13 @@ Cpu-pools can be specified either by nam
>  
>  =over 4
>  
> -=item B<cpupool-create> [I<OPTIONS>] I<ConfigFile>
[I<Variable=Value> ...]
> +=item B<cpupool-create> [I<OPTIONS>] [I<ConfigFile>]
[I<Variable=Value> ...]
>  
> -Create a cpu pool based an I<ConfigFile>.
> +Create a cpu pool based an config from a I<ConfigFile> or
command-line parameters.
>  Variable settings from the I<ConfigFile> may be altered by
specifying new
> -or additional assignments on the command line.
> +or additional assignments on the command line.
Could do with rewrapping? Or a blank line between the paras if that''s
what they are meant to be (that''s prexisting in that case)
> +
> +See the xlcpupool.cfg manpage for more information.
I think this should be "L<xlcpupool.cfg(N)>" in here. (N for
whichever
section the manpage is in).

 
> diff -r 30cc13e25e01 -r 0fb728d56bae tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c	Tue Apr 03 19:02:19 2012 +0100
> +++ b/tools/libxl/xl_cmdtable.c	Tue Apr 03 19:28:04 2012 +0100
> @@ -364,12 +364,14 @@ struct cmd_spec cmd_table[] = {
>      },
>      { "cpupool-create",
>        &main_cpupoolcreate, 1,
> -      "Create a CPU pool based an ConfigFile",
> -      "[options] <ConfigFile> [vars]",
> +      "Create a new CPU pool",
> +      "[options] [<ConfigFile>] [Variable=value ...]",
>        "-h, --help                   Print this help.\n"
>        "-f FILE, --defconfig=FILE    Use the given configuration
file.\n"
>        "-n, --dryrun                 Dry run - prints the resulting
configuration.\n"
> -      "                              (deprecated in favour of global
-N option)."
> +      "                              (deprecated in favour of global
-N option).\n"
> +      "\nSee the xlcpupool.cfg manpage for more information.",
xlcpupool.cfg(N) here too.
> +
>      },
>      { "cpupool-list",
>        &main_cpupoollist, 0,
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

George Dunlap writes ("[Xen-devel] [PATCH] xl: Don''t require a
config file for cpupools"):
> Since the key information can be fairly simply put on the command-line,
> there''s no need to require an actual config file.
Thanks.  Just a few tiny coding style nits.  I agree with Ian
Campbell''s comments, and also:
> -    const char *filename = NULL;
> +    const char *filename = NULL, *config_src=NULL;
This has inconsistent use of whitespace.
> +    if (filename)
> +    {
This should have { on the same line as the if.
> +        if (libxl_read_file_contents(ctx, filename, (void
**)&config_data,
> +                                     &config_len)) {
> +            fprintf(stderr, "Failed to read config file: %s:
%s\n",
> +                    filename, strerror(errno));
> +            return -ERROR_FAIL;
> +        }
> +        config_src=filename;
We put spaces around "=".  (Here and a few lines further on.)
> +    }
> +    else
And on the same line as the else.  And having { } for the if, I think
putting { } for the else block would be more conventional.
> -    printf("Using config file \"%s\"\n", filename);
> +    printf("Using config file \"%s\"\n", config_src);
This will print
   Using config file "command line"
which is rather an odd message.  Perhaps change the string to
`<command line>'' and remove the quotes ?

Ian.

On Mon, Apr 23, 2012 at 4:25 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
>> -    printf("Using config file \"%s\"\n",
filename);
>> +    printf("Using config file \"%s\"\n",
config_src);
>
> This will print
>   Using config file "command line"
> which is rather an odd message.  Perhaps change the string to
> `<command line>'' and remove the quotes ?
Changing the string to <command line> makes sense.  I think the point
of putting quotes there is to avoid getting strange results if your
filenames have spaces.  But I suppose if you''re strange enough to be
using spaces in your filenames, you should be able to figure out
what''s going on yourself. :-)

New patch on the way...
 -George