Using "xl create" without domain config file

Using "xl create" without domain config file

[W. Michael Petullo]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

I found it useful to use xm without a domain configuration file:

	xm create /dev/null kernel=...

The xl command does not support this. See a previous thread I started:

	http://lists.xen.org/archives/html/xen-devel/2011-08/msg00330.html

I just found the time to modify xl so that it works without a domain
configuration file. For example:

	xl create kernel=\"...\" ...

Would it be possible to add this feature? The xl command already supports
augmenting a domain configuration file with additional information
from the command line. My changes just modify xl so that the entire
configuration may be specified on the command line.

To test this patch, run:

	1. xl create -c /path/to/config

	2. xl create -c /path/to/config someAdditionalDomainParam=\"value\"

	3. xl create -c kernel=\"/path/to/kernel\"

The third test covers the new case.

-- 
Mike

:wq

[-- Attachment #2: xen-4.1.2-xl-command-line-domain-def.patch --]
[-- Type: text/plain, Size: 2381 bytes --]

diff -u --recursive --new-file xen-4.1.2-vanilla/tools/libxl/xl_cmdimpl.c xen-4.1.2/tools/libxl/xl_cmdimpl.c
--- xen-4.1.2-vanilla/tools/libxl/xl_cmdimpl.c	2011-10-20 12:05:43.000000000 -0500
+++ xen-4.1.2/tools/libxl/xl_cmdimpl.c	2012-05-30 23:37:51.261948881 -0500
@@ -1442,30 +1442,32 @@
                                        &config_data, &config_len);
         if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                            config_file, strerror(errno)); return ERROR_FAIL; }
-        if (!restore_file && extra_config && strlen(extra_config)) {
-            if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
-                fprintf(stderr, "Failed to attach extra configration\n");
-                return ERROR_FAIL;
-            }
-            /* allocate space for the extra config plus two EOLs plus \0 */
-            config_data = realloc(config_data, config_len
-                + strlen(extra_config) + 2 + 1);
-            if (!config_data) {
-                fprintf(stderr, "Failed to realloc config_data\n");
-                return ERROR_FAIL;
-            }
-            config_len += sprintf(config_data + config_len, "\n%s\n",
-                extra_config);
-        }
     } else {
-        if (!config_data) {
-            fprintf(stderr, "Config file not specified and"
+        if (!config_data && !extra_config) {
+            fprintf(stderr, "Domain config file not specified,"
+	            " none on command line, and"
                     " none in save file\n");
             return ERROR_INVAL;
         }
         config_file = "<saved>";
     }
 
+    if (!restore_file && extra_config && strlen(extra_config)) {
+        if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
+            fprintf(stderr, "Failed to attach extra configration\n");
+            return ERROR_FAIL;
+        }
+        /* allocate space for the extra config plus two EOLs plus \0 */
+        config_data = realloc(config_data, config_len
+            + strlen(extra_config) + 2 + 1);
+        if (!config_data) {
+            fprintf(stderr, "Failed to realloc config_data\n");
+            return ERROR_FAIL;
+        }
+        config_len += sprintf(config_data + config_len, "\n%s\n",
+            extra_config);
+    }
+
     if (!dom_info->quiet)
         printf("Parsing config file %s\n", config_file);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Using "xl create" without domain config file

[Ian Campbell]

On Thu, 2012-05-31 at 05:50 +0100, W. Michael Petullo wrote:
> I found it useful to use xm without a domain configuration file:
> 
> 	xm create /dev/null kernel=...
> 
> The xl command does not support this. See a previous thread I started:
> 
> 	http://lists.xen.org/archives/html/xen-devel/2011-08/msg00330.html
> 
> I just found the time to modify xl so that it works without a domain
> configuration file. For example:
> 
> 	xl create kernel=\"...\" ...
> 
> Would it be possible to add this feature? The xl command already supports
> augmenting a domain configuration file with additional information
> from the command line. My changes just modify xl so that the entire
> configuration may be specified on the command line.
> 
> To test this patch, run:
> 
> 	1. xl create -c /path/to/config
> 
> 	2. xl create -c /path/to/config someAdditionalDomainParam=\"value\"
> 
> 	3. xl create -c kernel=\"/path/to/kernel\"
> 
> The third test covers the new case.

Does #3 actually work? I ask because name=".." is mandatory...

Please can you supply a proposed commit message including a
signed-off-by as per http://wiki.xen.org/wiki/Submitting_Xen_Patches

Please can you also check that docs/man/xl*.pod do not need updating
after this change.

Ian.

Re: Using "xl create" without domain config file

[Ian Jackson]

W. Michael Petullo writes ("[Xen-devel] Using "xl create" without domain config file"):
> I found it useful to use xm without a domain configuration file:
> 
> 	xm create /dev/null kernel=...
> 
> The xl command does not support this. See a previous thread I started:
> 
> 	http://lists.xen.org/archives/html/xen-devel/2011-08/msg00330.html

I would be happy to support the /dev/null form.

> I just found the time to modify xl so that it works without a domain
> configuration file. For example:
> 
> 	xl create kernel=\"...\" ...

This, however, is ambiguous.  It's not clear from your patch below how
this ambiguity is resolved but I don't think what you've done can be
sufficient in itself.

What if the domain config filename contains a `=' ?

Ian.

Re: Using "xl create" without domain config file

[W. Michael Petullo]

>> I found it useful to use xm without a domain configuration file:
>> 
>> 	xm create /dev/null kernel=...
>> 
>> The xl command does not support this. See a previous thread I started:
>> 
>> 	http://lists.xen.org/archives/html/xen-devel/2011-08/msg00330.html
 
> I would be happy to support the /dev/null form.

This patch supports the /dev/null form.

# HG changeset patch
# Parent 435493696053a079ec17d6e1a63e5f2be3a2c9d0
xl: Allow use of /dev/null with xl create to enable command-line definition

xm allows specifying /dev/null as the domain configuration argument
to its create option; add same functionality to xl. Whereas xl used to
check to ensure the domain configuration argument was a regular file,
it now checks to ensure it is not a directory. This allows specifying
an entire domain configuration on the command line.

Signed-off-by: W. Michael Petullo <mike@flyn.org>

diff -r 435493696053 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Fri May 25 08:18:47 2012 +0100
+++ b/tools/libxl/libxl_utils.c	Sat Jun 02 09:11:27 2012 -0500
@@ -324,8 +324,8 @@ int libxl_read_file_contents(libxl_ctx *
         goto xe;
     }
 
-    if (!S_ISREG(stab.st_mode)) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is not a plain file", filename);
+    if (S_ISDIR(stab.st_mode)) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is a directory", filename);
         errno = ENOTTY;
         goto xe;
     }

Re: Using "xl create" without domain config file

[Ian Jackson]

W. Michael Petullo writes ("Re: [Xen-devel] Using "xl create" without domain config file"):
...
> -    if (!S_ISREG(stab.st_mode)) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is not a plain file", filename);
> +    if (S_ISDIR(stab.st_mode)) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is a directory", filename);

This is not correct.  If, for example, /dev/tty is specified, it will
go wrong.

The reason for the restriction to plain files is that those are the
only thing on which stat works to provide the size.  If we are to
support other objects, we need to change the reading algorithm.

Ian.

Re: Using "xl create" without domain config file

[Ian Jackson]

Ian Jackson writes ("Re: [Xen-devel] Using "xl create" without domain config file"):
> W. Michael Petullo writes ("Re: [Xen-devel] Using "xl create" without domain config file"):
> ...
> > -    if (!S_ISREG(stab.st_mode)) {
> > -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is not a plain file", filename);
> > +    if (S_ISDIR(stab.st_mode)) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is a directory", filename);
> 
> This is not correct.  If, for example, /dev/tty is specified, it will
> go wrong.
> 
> The reason for the restriction to plain files is that those are the
> only thing on which stat works to provide the size.  If we are to
> support other objects, we need to change the reading algorithm.

Another alternative would be to special-case the string "/dev/null"
(in xl, not libxl) and not read a config file at all in that case.

Ian.

Re: Using "xl create" without domain config file

[W. Michael Petullo]

[...]

>>> -    if (!S_ISREG(stab.st_mode)) {
>>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is not a plain file", filename);
>>> +    if (S_ISDIR(stab.st_mode)) {
>>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is a directory", filename);
>>
>> This is not correct.  If, for example, /dev/tty is specified, it will
>> go wrong.
>>
>> The reason for the restriction to plain files is that those are the
>> only thing on which stat works to provide the size.  If we are to
>> support other objects, we need to change the reading algorithm.
>
> Another alternative would be to special-case the string "/dev/null"
> (in xl, not libxl) and not read a config file at all in that case.

This patch treats /dev/null as a special case.

# HG changeset patch
# Parent 435493696053a079ec17d6e1a63e5f2be3a2c9d0
xl: Allow use of /dev/null with xl create to enable command-line definition

xm allows specifying /dev/null as the domain configuration argument to its
create option; add same functionality to xl. xl treats the configuration
argument /dev/null as a special case.  This allows specifying an entire
domain configuration on the command line.

Signed-off-by: W. Michael Petullo <mike@flyn.org>

diff -r 435493696053 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c  Fri May 25 08:18:47 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c  Thu Jun 07 22:40:58 2012 -0500
@@ -1454,10 +1454,13 @@ static int create_domain(struct domain_c

     if (config_file) {
         free(config_data);  config_data = 0;
-        ret = libxl_read_file_contents(&ctx, config_file,
-                                       &config_data, &config_len);
-        if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
-                           config_file, strerror(errno)); return ERROR_FAIL; }
+        // /dev/null represents special case (read config. from command line)
+        if (strcmp(config_file, "/dev/null")) {
+            ret = libxl_read_file_contents(&ctx, config_file,
+                                           &config_data, &config_len);
+            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
+                               config_file, strerror(errno)); return ERROR_FAIL; }
+        }
         if (!restore_file && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configration\n");

Re: Using "xl create" without domain config file

[Ian Jackson]

W. Michael Petullo writes ("Re: [Xen-devel] Using "xl create" without domain config file"):
> [...]
> > Another alternative would be to special-case the string "/dev/null"
> > (in xl, not libxl) and not read a config file at all in that case.
> 
> This patch treats /dev/null as a special case.

Thanks, this is going in the right direction I think.  Just two
niggles:

> diff -r 435493696053 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c  Fri May 25 08:18:47 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c  Thu Jun 07 22:40:58 2012 -0500
> @@ -1454,10 +1454,13 @@ static int create_domain(struct domain_c
> 
>      if (config_file) {
>          free(config_data);  config_data = 0;
...
> +        // /dev/null represents special case (read config. from command line)

libxl coding style uses /*...*/ comments.

> +        if (strcmp(config_file, "/dev/null")) {
> +            ret = libxl_read_file_contents(&ctx, config_file,
> +                                           &config_data, &config_len);

I think this fails to set config_len to 0 in the /dev/null case, which
it should do.  Although config_len is initialised to 0 it may have
been set to a non-0 value from the length of the old config file if
we're doing a restore, and in this case it needs to be rezeroed.

> +            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
> +                               config_file, strerror(errno)); return ERROR_FAIL; }
> +        }
>          if (!restore_file && extra_config && strlen(extra_config)) {
>              if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
>                  fprintf(stderr, "Failed to attach extra configration\n");

thanks,
Ian.

Re: Using "xl create" without domain config file

[W. Michael Petullo]

[...]
> Thanks, this is going in the right direction I think.  Just two
> niggles:
 
>> diff -r 435493696053 tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c  Fri May 25 08:18:47 2012 +0100
>> +++ b/tools/libxl/xl_cmdimpl.c  Thu Jun 07 22:40:58 2012 -0500
>> @@ -1454,10 +1454,13 @@ static int create_domain(struct domain_c
>> 
>>      if (config_file) {
>>          free(config_data);  config_data = 0;
[...]
>> +        // /dev/null represents special case (read config. from command line)
 
> libxl coding style uses /*...*/ comments.
 
>> +        if (strcmp(config_file, "/dev/null")) {
>> +            ret = libxl_read_file_contents(&ctx, config_file,
>> +                                           &config_data, &config_len);
 
> I think this fails to set config_len to 0 in the /dev/null case, which
> it should do.  Although config_len is initialised to 0 it may have
> been set to a non-0 value from the length of the old config file if
> we're doing a restore, and in this case it needs to be rezeroed.
 
>> +            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
>> +                               config_file, strerror(errno)); return ERROR_FAIL; }
>> +        }
>>          if (!restore_file && extra_config && strlen(extra_config)) {
>>              if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
>>                  fprintf(stderr, "Failed to attach extra configration\n");

This version addresses the two issues above:

# HG changeset patch
# Parent 435493696053a079ec17d6e1a63e5f2be3a2c9d0
xl: Allow use of /dev/null with xl create to enable command-line definition

xm allows specifying /dev/null as the domain configuration argument to its
create option; add same functionality to xl. xl treats the configuration
argument /dev/null as a special case.  This allows specifying an entire
domain configuration on the command line.

Signed-off-by: W. Michael Petullo <mike@flyn.org>

diff -r 435493696053 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri May 25 08:18:47 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jun 11 15:18:23 2012 -0500
@@ -1454,10 +1454,15 @@ static int create_domain(struct domain_c
 
     if (config_file) {
         free(config_data);  config_data = 0;
-        ret = libxl_read_file_contents(&ctx, config_file,
-                                       &config_data, &config_len);
-        if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
-                           config_file, strerror(errno)); return ERROR_FAIL; }
+        /* /dev/null represents special case (read config. from command line) */
+        if (!strcmp(config_file, "/dev/null")) {
+            config_len = 0;
+        } else {
+            ret = libxl_read_file_contents(&ctx, config_file,
+                                           &config_data, &config_len);
+            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
+                               config_file, strerror(errno)); return ERROR_FAIL; }
+        }
         if (!restore_file && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configration\n");

Re: Using "xl create" without domain config file

[Ian Campbell]

> # HG changeset patch
> # Parent 435493696053a079ec17d6e1a63e5f2be3a2c9d0
> xl: Allow use of /dev/null with xl create to enable command-line definition
> 
> xm allows specifying /dev/null as the domain configuration argument to its
> create option; add same functionality to xl. xl treats the configuration
> argument /dev/null as a special case.  This allows specifying an entire
> domain configuration on the command line.
> 
> Signed-off-by: W. Michael Petullo <mike@flyn.org>

Hi Michael, 

I have applied this patch but it seems to be against the xen 4.1 tree
and not against xen-unstable. In this case I think I was able to
trivially resolve the conflicts, but please do check I got it right.

For future reference we generally require patches to be against
unstable. Once applied there they can then be nominated for backporting
to 4.1 but except in very unusual circumstances we would not apply a
patch directly to a stable branch.

You might also want to consider patching the xl config-update command?

Ian.

> 
> diff -r 435493696053 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Fri May 25 08:18:47 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Mon Jun 11 15:18:23 2012 -0500
> @@ -1454,10 +1454,15 @@ static int create_domain(struct domain_c
>  
>      if (config_file) {
>          free(config_data);  config_data = 0;
> -        ret = libxl_read_file_contents(&ctx, config_file,
> -                                       &config_data, &config_len);
> -        if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
> -                           config_file, strerror(errno)); return ERROR_FAIL; }
> +        /* /dev/null represents special case (read config. from command line) */
> +        if (!strcmp(config_file, "/dev/null")) {
> +            config_len = 0;
> +        } else {
> +            ret = libxl_read_file_contents(&ctx, config_file,
> +                                           &config_data, &config_len);
> +            if (ret) { fprintf(stderr, "Failed to read config file: %s: %s\n",
> +                               config_file, strerror(errno)); return ERROR_FAIL; }
> +        }
>          if (!restore_file && extra_config && strlen(extra_config)) {
>              if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
>                  fprintf(stderr, "Failed to attach extra configration\n");
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Using "xl create" without domain config file

[W. Michael Petullo]

>> # HG changeset patch
>> # Parent 435493696053a079ec17d6e1a63e5f2be3a2c9d0
>> xl: Allow use of /dev/null with xl create to enable command-line definition
>> 
>> xm allows specifying /dev/null as the domain configuration argument to its
>> create option; add same functionality to xl. xl treats the configuration
>> argument /dev/null as a special case.  This allows specifying an entire
>> domain configuration on the command line.
>> 
>> Signed-off-by: W. Michael Petullo <mike@flyn.org>
 
> I have applied this patch but it seems to be against the xen 4.1 tree
> and not against xen-unstable. In this case I think I was able to
> trivially resolve the conflicts, but please do check I got it right.

Things work nicely in Xen 4.2.0 RC2. Thank you.

-- 
Mike

:wq