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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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.
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.
>> 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; }
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.
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.
[...]>>> - 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");
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.
[...]> 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");
> # 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
>> # 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