diff mbox series

[2/2] tools/xl: Allow specifying JSON for domain configuration file format

Message ID 09213ac26738ee51401b454534c6b437766481b7.1650422518.git.ehem+xen@m5p.com (mailing list archive)
State Superseded
Headers show
Series Allow use of JSON in domain configuration files | expand

Commit Message

Elliott Mitchell April 20, 2022, 1:23 a.m. UTC 
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.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 tools/xl/xl.h           |  5 +++++
 tools/xl/xl_cmdtable.c  |  2 ++
 tools/xl/xl_vmcontrol.c | 13 +++++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

Comments

Anthony PERARD April 29, 2022, 12:34 p.m. UTC | #1 
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.

"potential", right, but it isn't hasn't been really tested. When
implemented, I think the intend of the json format was for libxl to
communicate with itself while migrating a guest (or save/restore). It
would be nice to know if it actually can work.

> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..a0c03f96df 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -49,6 +49,11 @@ struct domain_create {
>      int migrate_fd; /* -1 means none */
>      int send_back_fd; /* -1 means none */
>      char **migration_domname_r; /* from malloc */
> +    enum {
> +        FORMAT_DEFAULT,
> +        FORMAT_JSON,
> +        FORMAT_LEGACY,
> +    } format;

I think the name "format" here is too generic, we are in the struct
"domain_create" and this new format field isn't intended to specify the
format of the domain. I think "config_file_format" would be better, as
it is only used for the format of the config_file.

Also I don't think having "_DEFAULT" would be useful, we need to know
what the format is intended to be before starting to parse the file, and
I don't think changing the default is going to work. So for the enum, we
could have "_LEGACY = 0".

The prefix "FORMAT_" feels a bit generic, maybe "CONFIG_FORMAT_" would
be better, or something else.

>  };
>  
>  int create_domain(struct domain_create *dom_info);
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index f546beaceb..04d579a596 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -31,6 +31,8 @@ const struct cmd_spec cmd_table[] = {
>        "-h                      Print this help.\n"
>        "-p                      Leave the domain paused after it is created.\n"
>        "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
> +      "-j, --json              Interpret configuration file as JSON format\n"
> +      "-J                      Use traditional configuration file format (current default)\n"

I don't think this new "-J" option would be useful, just the "-j" should be
enough.

>        "-n, --dryrun            Dry run - prints the resulting configuration\n"
>        "                         (deprecated in favour of global -N option).\n"
>        "-q, --quiet             Quiet.\n"
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 2ec4140258..41bd919d1d 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.format == FORMAT_JSON ? true : false;

This doesn't build, "dom_info" is a pointer.

Also, "? true : false;" is weird in C.


>      } else {
>          if (!config_data) {
>              fprintf(stderr, "Config file not specified and"
> @@ -1173,6 +1173,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'},
> @@ -1181,18 +1182,23 @@ int main_create(int argc, char **argv)
>  
>      dom_info.extra_config = NULL;
>  
> +    dom_info.format = FORMAT_DEFAULT;
> +
>      if (argv[1] && argv[1][0] != '-' && !strchr(argv[1], '=')) {
>          filename = argv[1];
>          argc--; argv++;
>      }
>  
> -    SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) {
> +    SWITCH_FOREACH_OPT(opt, "FJfjnq:AVcdeip", opts, "create", 0) {
>      case 'A':
>          vnc = vncautopass = 1;
>          break;
>      case 'F':
>          daemonize = 0;
>          break;
> +    case 'J':
> +        dom_info.format = FORMAT_LEGACY;
> +        break;
>      case 'V':
>          vnc = 1;
>          break;
> @@ -1212,6 +1218,9 @@ int main_create(int argc, char **argv)
>      case 'i':
>          ignore_masks = 1;
>          break;
> +    case 'j':
> +        dom_info.format = FORMAT_JSON;

This setting is ignored, as "dom_info" is reset later.

> +        break;
>      case 'n':
>          dryrun_only = 1;
>          break;

Thanks,
Elliott Mitchell April 30, 2022, 1:06 a.m. UTC | #2 
On Fri, Apr 29, 2022 at 01:34:40PM +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.
> 
> "potential", right, but it isn't hasn't been really tested. When
> implemented, I think the intend of the json format was for libxl to
> communicate with itself while migrating a guest (or save/restore). It
> would be nice to know if it actually can work.

What I wonder is why the behind the scenes format is flexible enough to
fully handle domain configuration, yet wasn't reused for domain
configuration.  Then I look at the parser for domain configuration files
and it isn't really that great.

Add those two together and moving towards domain configuration files
being JSON seems natural.  Look at the "vif" and "disk" sections.  Those
could be very naturally handled as JSON, while the current parser isn't
rather limited.

There may be need to modify libxl_domain_config_from_json() to ensure it
gives good error messages.

> > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> > ---
> > diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> > index c5c4bedbdd..a0c03f96df 100644
> > --- a/tools/xl/xl.h
> > +++ b/tools/xl/xl.h
> > @@ -49,6 +49,11 @@ struct domain_create {
> >      int migrate_fd; /* -1 means none */
> >      int send_back_fd; /* -1 means none */
> >      char **migration_domname_r; /* from malloc */
> > +    enum {
> > +        FORMAT_DEFAULT,
> > +        FORMAT_JSON,
> > +        FORMAT_LEGACY,
> > +    } format;
> 
> I think the name "format" here is too generic, we are in the struct
> "domain_create" and this new format field isn't intended to specify the
> format of the domain. I think "config_file_format" would be better, as
> it is only used for the format of the config_file.

What about "config_format" to match below?

> Also I don't think having "_DEFAULT" would be useful, we need to know
> what the format is intended to be before starting to parse the file, and
> I don't think changing the default is going to work. So for the enum, we
> could have "_LEGACY = 0".
> 
> The prefix "FORMAT_" feels a bit generic, maybe "CONFIG_FORMAT_" would
> be better, or something else.

Okay.

Over time the default can be changed.  Document plans to move to JSON
exclusively.  Then after a few versions switch the default.  Then after
more versions remove the old format.

> >  };
> >  
> >  int create_domain(struct domain_create *dom_info);
> > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> > index f546beaceb..04d579a596 100644
> > --- a/tools/xl/xl_cmdtable.c
> > +++ b/tools/xl/xl_cmdtable.c
> > @@ -31,6 +31,8 @@ const struct cmd_spec cmd_table[] = {
> >        "-h                      Print this help.\n"
> >        "-p                      Leave the domain paused after it is created.\n"
> >        "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
> > +      "-j, --json              Interpret configuration file as JSON format\n"
> > +      "-J                      Use traditional configuration file format (current default)\n"
> 
> I don't think this new "-J" option would be useful, just the "-j" should be
> enough.

I was thinking of this as a transition mechanism.  Have "-J" for when the
default has been changed, but the code to handle the original format
hasn't been removed.

> >        "-n, --dryrun            Dry run - prints the resulting configuration\n"
> >        "                         (deprecated in favour of global -N option).\n"
> >        "-q, --quiet             Quiet.\n"
> > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> > index 2ec4140258..41bd919d1d 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.format == FORMAT_JSON ? true : false;
> 
> This doesn't build, "dom_info" is a pointer.
> 
> Also, "? true : false;" is weird in C.

Uh, yeah.  Too many different coding standards.  Plus things being
passed around.  Erk.  %-/

> >      } else {
> >          if (!config_data) {
> >              fprintf(stderr, "Config file not specified and"
> > @@ -1173,6 +1173,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'},
> > @@ -1181,18 +1182,23 @@ int main_create(int argc, char **argv)
> >  
> >      dom_info.extra_config = NULL;
> >  
> > +    dom_info.format = FORMAT_DEFAULT;
> > +
> >      if (argv[1] && argv[1][0] != '-' && !strchr(argv[1], '=')) {
> >          filename = argv[1];
> >          argc--; argv++;
> >      }
> >  
> > -    SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) {
> > +    SWITCH_FOREACH_OPT(opt, "FJfjnq:AVcdeip", opts, "create", 0) {
> >      case 'A':
> >          vnc = vncautopass = 1;
> >          break;
> >      case 'F':
> >          daemonize = 0;
> >          break;
> > +    case 'J':
> > +        dom_info.format = FORMAT_LEGACY;
> > +        break;
> >      case 'V':
> >          vnc = 1;
> >          break;
> > @@ -1212,6 +1218,9 @@ int main_create(int argc, char **argv)
> >      case 'i':
> >          ignore_masks = 1;
> >          break;
> > +    case 'j':
> > +        dom_info.format = FORMAT_JSON;
> 
> This setting is ignored, as "dom_info" is reset later.

Uh huh.  Indeed.  I saw the "dom_info.extra_config = NULL;" and figured
dom_info was valid at that point, but the memset() is later.  Certainly
need to remedy that.

Having looked, that has gotten pretty awful.  That really needs some
rework to avoid confusion.  Next version...
diff mbox series

Patch

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..a0c03f96df 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -49,6 +49,11 @@  struct domain_create {
     int migrate_fd; /* -1 means none */
     int send_back_fd; /* -1 means none */
     char **migration_domname_r; /* from malloc */
+    enum {
+        FORMAT_DEFAULT,
+        FORMAT_JSON,
+        FORMAT_LEGACY,
+    } format;
 };
 
 int create_domain(struct domain_create *dom_info);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index f546beaceb..04d579a596 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -31,6 +31,8 @@  const struct cmd_spec cmd_table[] = {
       "-h                      Print this help.\n"
       "-p                      Leave the domain paused after it is created.\n"
       "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\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"
       "-q, --quiet             Quiet.\n"
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 2ec4140258..41bd919d1d 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.format == FORMAT_JSON ? true : false;
     } else {
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
@@ -1173,6 +1173,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'},
@@ -1181,18 +1182,23 @@  int main_create(int argc, char **argv)
 
     dom_info.extra_config = NULL;
 
+    dom_info.format = FORMAT_DEFAULT;
+
     if (argv[1] && argv[1][0] != '-' && !strchr(argv[1], '=')) {
         filename = argv[1];
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) {
+    SWITCH_FOREACH_OPT(opt, "FJfjnq:AVcdeip", opts, "create", 0) {
     case 'A':
         vnc = vncautopass = 1;
         break;
     case 'F':
         daemonize = 0;
         break;
+    case 'J':
+        dom_info.format = FORMAT_LEGACY;
+        break;
     case 'V':
         vnc = 1;
         break;
@@ -1212,6 +1218,9 @@  int main_create(int argc, char **argv)
     case 'i':
         ignore_masks = 1;
         break;
+    case 'j':
+        dom_info.format = FORMAT_JSON;
+        break;
     case 'n':
         dryrun_only = 1;
         break;