[PATCH v2 3/3] tools/xl: Allow specifying JSON for domain configuration file format

[PATCH v2 3/3] tools/xl: Allow specifying JSON for domain configuration file format

[Elliott Mitchell]

JSON is currently used when saving domains to mass storage.  Being able
to use JSON as the normal input to `xl create` has potential to be
valuable.  Add the functionality.

Move the memset() earlier so to allow use of the structure sooner.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
v2:
Removing the UUOC situation.  Correct the comparison to match the correct
variable type.  Rename to "config_format" from "format".

Rename everything from "format" to "config_format".
---
 tools/xl/xl.h           |  5 +++++
 tools/xl/xl_cmdtable.c  |  2 ++
 tools/xl/xl_vmcontrol.c | 12 ++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 72538d6a81..4b0828431f 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -48,6 +48,11 @@ struct domain_create {
     int migrate_fd; /* -1 means none */
     int send_back_fd; /* -1 means none */
     char **migration_domname_r; /* from malloc */
+    enum {
+        CONFIG_FORMAT_DEFAULT,
+        CONFIG_FORMAT_JSON,
+        CONFIG_FORMAT_LEGACY,
+    } config_format; /* format specified for configuration */
 };
 
 int create_domain(struct domain_create *dom_info);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 35182ca196..8a791d8c49 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -30,6 +30,8 @@ const struct cmd_spec cmd_table[] = {
       "-F                      Run in foreground until death of the domain.\n"
       "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
       "-h                      Print this help.\n"
+      "-j, --json              Interpret configuration file as JSON format\n"
+      "-J                      Use traditional configuration file format (current default)\n"
       "-n, --dryrun            Dry run - prints the resulting configuration\n"
       "                         (deprecated in favour of global -N option).\n"
       "-p                      Leave the domain paused after it is created.\n"
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 4bf041fb01..dd8b3f81a6 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -789,7 +789,7 @@ int create_domain(struct domain_create *dom_info)
                 extra_config);
         }
         config_source=config_file;
-        config_in_json = false;
+        config_in_json = dom_info->config_format == CONFIG_FORMAT_JSON;
     } else {
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
@@ -1167,6 +1167,7 @@ int main_create(int argc, char **argv)
     struct domain_create dom_info = {
         /* Command-line options */
         .config_file = NULL,
+        .config_format = CONFIG_FORMAT_DEFAULT,
         .console_autoconnect = 0,
         .debug = 0,
         .daemonize = 1,
@@ -1189,6 +1190,7 @@ int main_create(int argc, char **argv)
         {"defconfig", 1, 0, 'f'},
         {"dryrun", 0, 0, 'n'},
         {"ignore-global-affinity-masks", 0, 0, 'i'},
+        {"json", 0, 0, 'j'},
         {"quiet", 0, 0, 'q'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
@@ -1200,13 +1202,16 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "AFVcdef:inpq", opts, "create", 0) {
+    SWITCH_FOREACH_OPT(opt, "AFJVcdef:ijnpq", opts, "create", 0) {
     case 'A':
         dom_info.vnc = dom_info.vncautopass = 1;
         break;
     case 'F':
         dom_info.daemonize = 0;
         break;
+    case 'J':
+        dom_info.config_format = CONFIG_FORMAT_LEGACY;
+        break;
     case 'V':
         dom_info.vnc = 1;
         break;
@@ -1226,6 +1231,9 @@ int main_create(int argc, char **argv)
     case 'i':
         dom_info.ignore_global_affinity_masks = 1;
         break;
+    case 'j':
+        dom_info.config_format = CONFIG_FORMAT_JSON;
+        break;
     case 'n':
         dryrun_only = 1;
         break;
-- 
2.30.2

[PATCH v2 1/3] tools/xl: Sort create command options

[Elliott Mitchell]

Hopefully simplify future changes by sorting options lists for
`xl create`.  While at it, declare the options list constant.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
v2:
Adding mention of making the list constant.

Fix the getopt list sorting
---
 tools/xl/xl_cmdtable.c  | 12 ++++++------
 tools/xl/xl_vmcontrol.c | 40 ++++++++++++++++++++--------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 661323d488..35182ca196 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -24,16 +24,16 @@ const struct cmd_spec cmd_table[] = {
       &main_create, 1, 1,
       "Create a domain from config file <filename>",
       "<ConfigFile> [options] [vars]",
-      "-h                      Print this help.\n"
-      "-p                      Leave the domain paused after it is created.\n"
       "-c                      Connect to the console after the domain is created.\n"
+      "-d                      Enable debug messages.\n"
+      "-e                      Do not wait in the background for the death of the domain.\n"
+      "-F                      Run in foreground until death of the domain.\n"
       "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
-      "-q, --quiet             Quiet.\n"
+      "-h                      Print this help.\n"
       "-n, --dryrun            Dry run - prints the resulting configuration\n"
       "                         (deprecated in favour of global -N option).\n"
-      "-d                      Enable debug messages.\n"
-      "-F                      Run in foreground until death of the domain.\n"
-      "-e                      Do not wait in the background for the death of the domain.\n"
+      "-p                      Leave the domain paused after it is created.\n"
+      "-q, --quiet             Quiet.\n"
       "-V, --vncviewer         Connect to the VNC display after the domain is created.\n"
       "-A, --vncviewer-autopass\n"
       "                        Pass VNC password to viewer via stdin.\n"
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 435155a033..d081c6c290 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1169,13 +1169,13 @@ int main_create(int argc, char **argv)
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
         quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0;
     int opt, rc;
-    static struct option opts[] = {
+    static const struct option opts[] = {
+        {"defconfig", 1, 0, 'f'},
         {"dryrun", 0, 0, 'n'},
+        {"ignore-global-affinity-masks", 0, 0, 'i'},
         {"quiet", 0, 0, 'q'},
-        {"defconfig", 1, 0, 'f'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
-        {"ignore-global-affinity-masks", 0, 0, 'i'},
         COMMON_LONG_OPTS
     };
 
@@ -1186,12 +1186,15 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) {
-    case 'f':
-        filename = optarg;
+    SWITCH_FOREACH_OPT(opt, "AFVcdef:inpq", opts, "create", 0) {
+    case 'A':
+        vnc = vncautopass = 1;
         break;
-    case 'p':
-        paused = 1;
+    case 'F':
+        daemonize = 0;
+        break;
+    case 'V':
+        vnc = 1;
         break;
     case 'c':
         console_autoconnect = 1;
@@ -1199,28 +1202,25 @@ int main_create(int argc, char **argv)
     case 'd':
         debug = 1;
         break;
-    case 'F':
-        daemonize = 0;
-        break;
     case 'e':
         daemonize = 0;
         monitor = 0;
         break;
+    case 'f':
+        filename = optarg;
+        break;
+    case 'i':
+        ignore_masks = 1;
+        break;
     case 'n':
         dryrun_only = 1;
         break;
+    case 'p':
+        paused = 1;
+        break;
     case 'q':
         quiet = 1;
         break;
-    case 'V':
-        vnc = 1;
-        break;
-    case 'A':
-        vnc = vncautopass = 1;
-        break;
-    case 'i':
-        ignore_masks = 1;
-        break;
     }
 
     memset(&dom_info, 0, sizeof(dom_info));
-- 
2.30.2

[PATCH v2 2/3] tools/xl: Use sparse init for dom_info, remove duplicate vars

[Elliott Mitchell]

Rather than having shadow variables for every element of dom_info, it is
better to properly initialize dom_info at the start.  This also removes
the misleading memset() in the middle of main_create().

Remove the dryrun element of domain_create as that has been displaced
by the global "dryrun_only" variable.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
v2:
This was added due to the confusing situation with dom_info.
---
 tools/xl/xl.h           |  1 -
 tools/xl/xl_vmcontrol.c | 76 ++++++++++++++++++++---------------------
 2 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..72538d6a81 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -34,7 +34,6 @@ struct domain_create {
     int daemonize;
     int monitor; /* handle guest reboots etc */
     int paused;
-    int dryrun;
     int quiet;
     int vnc;
     int vncautopass;
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index d081c6c290..4bf041fb01 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -854,8 +854,8 @@ int create_domain(struct domain_create *dom_info)
         }
     }
 
-    if (debug || dom_info->dryrun) {
-        FILE *cfg_print_fh = (debug && !dom_info->dryrun) ? stderr : stdout;
+    if (debug || dryrun_only) {
+        FILE *cfg_print_fh = (debug && !dryrun_only) ? stderr : stdout;
         if (default_output_format == OUTPUT_FORMAT_SXP) {
             printf_info_sexp(-1, &d_config, cfg_print_fh);
         } else {
@@ -873,7 +873,7 @@ int create_domain(struct domain_create *dom_info)
 
 
     ret = 0;
-    if (dom_info->dryrun)
+    if (dryrun_only)
         goto out;
 
 start:
@@ -1164,10 +1164,26 @@ out:
 
 int main_create(int argc, char **argv)
 {
-    const char *filename = NULL;
-    struct domain_create dom_info;
-    int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
-        quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0;
+    struct domain_create dom_info = {
+        /* Command-line options */
+        .config_file = NULL,
+        .console_autoconnect = 0,
+        .debug = 0,
+        .daemonize = 1,
+        .ignore_global_affinity_masks = 0,
+        .monitor = 1,
+        .paused = 0,
+        .quiet = 0,
+        .vnc = 0,
+        .vncautopass = 0,
+
+        /* Extra configuration file settings */
+        .extra_config = NULL,
+
+        /* FDs, initialize to invalid */
+        .migrate_fd = -1,
+        .send_back_fd = -1,
+    };
     int opt, rc;
     static const struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
@@ -1179,58 +1195,54 @@ int main_create(int argc, char **argv)
         COMMON_LONG_OPTS
     };
 
-    dom_info.extra_config = NULL;
-
     if (argv[1] && argv[1][0] != '-' && !strchr(argv[1], '=')) {
-        filename = argv[1];
+        dom_info.config_file = argv[1];
         argc--; argv++;
     }
 
     SWITCH_FOREACH_OPT(opt, "AFVcdef:inpq", opts, "create", 0) {
     case 'A':
-        vnc = vncautopass = 1;
+        dom_info.vnc = dom_info.vncautopass = 1;
         break;
     case 'F':
-        daemonize = 0;
+        dom_info.daemonize = 0;
         break;
     case 'V':
-        vnc = 1;
+        dom_info.vnc = 1;
         break;
     case 'c':
-        console_autoconnect = 1;
+        dom_info.console_autoconnect = 1;
         break;
     case 'd':
-        debug = 1;
+        dom_info.debug = 1;
         break;
     case 'e':
-        daemonize = 0;
-        monitor = 0;
+        dom_info.daemonize = 0;
+        dom_info.monitor = 0;
         break;
     case 'f':
-        filename = optarg;
+        dom_info.config_file = optarg;
         break;
     case 'i':
-        ignore_masks = 1;
+        dom_info.ignore_global_affinity_masks = 1;
         break;
     case 'n':
         dryrun_only = 1;
         break;
     case 'p':
-        paused = 1;
+        dom_info.paused = 1;
         break;
     case 'q':
-        quiet = 1;
+        dom_info.quiet = 1;
         break;
     }
 
-    memset(&dom_info, 0, sizeof(dom_info));
-
     for (; optind < argc; optind++) {
         if (strchr(argv[optind], '=') != NULL) {
             string_realloc_append(&dom_info.extra_config, argv[optind]);
             string_realloc_append(&dom_info.extra_config, "\n");
-        } else if (!filename) {
-            filename = argv[optind];
+        } else if (!dom_info.config_file) {
+            dom_info.config_file = argv[optind];
         } else {
             help("create");
             free(dom_info.extra_config);
@@ -1238,20 +1250,6 @@ int main_create(int argc, char **argv)
         }
     }
 
-    dom_info.debug = debug;
-    dom_info.daemonize = daemonize;
-    dom_info.monitor = monitor;
-    dom_info.paused = paused;
-    dom_info.dryrun = dryrun_only;
-    dom_info.quiet = quiet;
-    dom_info.config_file = filename;
-    dom_info.migrate_fd = -1;
-    dom_info.send_back_fd = -1;
-    dom_info.vnc = vnc;
-    dom_info.vncautopass = vncautopass;
-    dom_info.console_autoconnect = console_autoconnect;
-    dom_info.ignore_global_affinity_masks = ignore_masks;
-
     rc = create_domain(&dom_info);
     if (rc < 0) {
         free(dom_info.extra_config);
-- 
2.30.2

[PATCH v2 0/3] Allow use of JSON in domain configuration files

[Elliott Mitchell]

While the traditional domain configuration file format works acceptably,
I can see uses for having full JSON support.  As such add "-j" and "-J"
to `xl create` to specify format.  The traditional format is the current
default.

While attempting this, it came up that options for `xl create` aren't in
a consistent order.  I'm concerned about moving the VNC options apart,
but the others have been sorted.

Due to one issue in the previous round I ended up examing the dom_info
variable in main_create().  The situation there is a bit tangled.  There
were shadow variables for everything in dom_info.  Unfortunately the
short-hand serves to confuse, so I believe the appropriate action is to
remove the shadows.  Appears .dry_run had effectively been deprecated,
but not fully removed; as such now fully remove it.

Rename everything "format" to "config_format".

Elliott Mitchell (3):
  tools/xl: Sort create command options
  tools/xl: Use sparse init for dom_info, remove duplicate vars
  tools/xl: Allow specifying JSON for domain configuration file format

 tools/xl/xl.h           |   6 ++-
 tools/xl/xl_cmdtable.c  |  14 ++---
 tools/xl/xl_vmcontrol.c | 114 +++++++++++++++++++++-------------------
 3 files changed, 73 insertions(+), 61 deletions(-)

-- 
2.30.2

Re: [PATCH v2 1/3] tools/xl: Sort create command options

[Anthony PERARD]

On Tue, Apr 19, 2022 at 06:56:03PM -0700, Elliott Mitchell wrote:
> Hopefully simplify future changes by sorting options lists for
> `xl create`.  While at it, declare the options list constant.
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 2/3] tools/xl: Use sparse init for dom_info, remove duplicate vars

[Anthony PERARD]

On Fri, Apr 29, 2022 at 03:45:25PM -0700, Elliott Mitchell wrote:
> Rather than having shadow variables for every element of dom_info, it is
> better to properly initialize dom_info at the start.  This also removes
> the misleading memset() in the middle of main_create().
> 
> Remove the dryrun element of domain_create as that has been displaced
> by the global "dryrun_only" variable.
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks for the clean up.

-- 
Anthony PERARD

Re: [PATCH v2 3/3] tools/xl: Allow specifying JSON for domain configuration file format

[Anthony PERARD]

On Tue, Apr 19, 2022 at 06:23:41PM -0700, Elliott Mitchell wrote:
> JSON is currently used when saving domains to mass storage.  Being able
> to use JSON as the normal input to `xl create` has potential to be
> valuable.  Add the functionality.
> 
> Move the memset() earlier so to allow use of the structure sooner.
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>

So, I gave this a try and creating a guest from a json config, and that
fails very early with "Unknown guest type".

Have you actually tried to create a guest from config file written in
json?

Also, this would need documentation about the new option, about the
format. The man page need to be edited.

An example of a config file written in json would be nice as well.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 3/3] tools/xl: Allow specifying JSON for domain configuration file format

[Elliott Mitchell]

On Fri, May 20, 2022 at 03:12:46PM +0100, Anthony PERARD wrote:
> On Tue, Apr 19, 2022 at 06:23:41PM -0700, Elliott Mitchell wrote:
> > JSON is currently used when saving domains to mass storage.  Being able
> > to use JSON as the normal input to `xl create` has potential to be
> > valuable.  Add the functionality.
> > 
> > Move the memset() earlier so to allow use of the structure sooner.
> > 
> > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> 
> So, I gave this a try and creating a guest from a json config, and that
> fails very early with "Unknown guest type".
> 
> Have you actually tried to create a guest from config file written in
> json?
> 
> Also, this would need documentation about the new option, about the
> format. The man page need to be edited.
> 
> An example of a config file written in json would be nice as well.

I'll be trying for these at some point, but no timeframe yet.  This was
an idea which occurred to me when looking at things.  I'm wavering on
whether this is the way to go...

Real goal is I would like to generate a replacement for the `xendomains`
init script.  While functional the script is woefully inadaquate for
anything other than the tiniest installation.

Notably there can be ordering constraints for start/shutdown (worse,
those could be distinct).  One might also wish different strategies for
different domains (some get saved to disk on reboot, some might get
shutdown/restarted).


For some of the configuration for this, adding to domain.cfg files makes
sense.  This though ends up with the issue of what should the extra data
look like?

I'm oscillating between adding a section in something libxl's parser
takes as a comment, versus adding a configuration option to domain.cfg
(libxl's parser ignores unknown sections which is not entirely good!).
JSON's structure would be good for an addition, but JSON comes with its
own downsides.

Most likely such a thing would be implemented in Python.  Needs a bit
more math than shell is good for.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2 3/3] tools/xl: Allow specifying JSON for domain configuration file format

[Anthony PERARD]

On Tue, May 31, 2022 at 06:25:13PM -0700, Elliott Mitchell wrote:
> On Fri, May 20, 2022 at 03:12:46PM +0100, Anthony PERARD wrote:
> > On Tue, Apr 19, 2022 at 06:23:41PM -0700, Elliott Mitchell wrote:
> > > JSON is currently used when saving domains to mass storage.  Being able
> > > to use JSON as the normal input to `xl create` has potential to be
> > > valuable.  Add the functionality.
> > > 
> > > Move the memset() earlier so to allow use of the structure sooner.
> > > 
> > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> > 
> > So, I gave this a try and creating a guest from a json config, and that
> > fails very early with "Unknown guest type".
> > 
> > Have you actually tried to create a guest from config file written in
> > json?
> > 
> > Also, this would need documentation about the new option, about the
> > format. The man page need to be edited.
> > 
> > An example of a config file written in json would be nice as well.
> 
> I'll be trying for these at some point, but no timeframe yet.  This was
> an idea which occurred to me when looking at things.  I'm wavering on
> whether this is the way to go...
> 
> Real goal is I would like to generate a replacement for the `xendomains`
> init script.  While functional the script is woefully inadaquate for
> anything other than the tiniest installation.
> 
> Notably there can be ordering constraints for start/shutdown (worse,
> those could be distinct).  One might also wish different strategies for
> different domains (some get saved to disk on reboot, some might get
> shutdown/restarted).

Is this that something like `libvirt` or some other toolstacks could help
with? Or maybe you are looking for something that as a small footprint
on the system and just run once at boot and at shutdown.

> For some of the configuration for this, adding to domain.cfg files makes
> sense.  This though ends up with the issue of what should the extra data
> look like?

Maybe `xl` could be taught to ignore some options that have a prefix
like "xendomain_", even if at the moment it ignore everything it doesn't
know I think.

> I'm oscillating between adding a section in something libxl's parser
> takes as a comment, versus adding a configuration option to domain.cfg

A comment section could work too I guess, like there's one for `sysv`
init system which describe dependency as comments.

> (libxl's parser ignores unknown sections which is not entirely good!).
> JSON's structure would be good for an addition, but JSON comes with its
> own downsides.
> 
> Most likely such a thing would be implemented in Python.  Needs a bit
> more math than shell is good for.

If you plan to convert the `xendomains` init script to python, I don't
think that would be a good idea, as it is probably better to not add a
dependency for a init script that has been a shell script for a while.
But introducing a new utility in python or other language might be fine.

Cheers,

-- 
Anthony PERARD