[v2,04/21] xl: introduce a domain type option

Commit Message

Roger Pau Monne Sept. 7, 2017, 10:16 a.m. UTC

Introduce a new type option to xl configuration files in order to
specify the domain type. This supersedes the current builder option.

The new option is documented in the xl.cfg man page, and the previous
builder option is marked as deprecated.

Signed-off-by: Roger Pau Monné <[e-mail]>
Acked-by: Wei Liu <[e-mail]
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
---
 docs/man/xl.cfg.pod.5.in | 22 ++++++++++++++++++++--
 tools/xl/xl_parse.c      | 20 +++++++++++++++++---
 2 files changed, 37 insertions(+), 5 deletions(-)

Comments

Ian Jackson Sept. 20, 2017, 2:50 p.m. UTC | #1

Roger Pau Monne writes ("[PATCH v2 04/21] xl: introduce a domain type option"):
> Introduce a new type option to xl configuration files in order to
> specify the domain type. This supersedes the current builder option.
...> 
> Th
>      libxl_defbool_set(&c_info->run_hotplug_scripts, run_hotplug_scripts);
>      c_info->type = LIBXL_DOMAIN_TYPE_PV;
> -    if (!xlu_cfg_get_string (config, "builder", &buf, 0) &&
> -        !strncmp(buf, "hvm", strlen(buf)))
> -        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
> +    if (!xlu_cfg_get_string (config, "builder", &buf, 0)) {
> +        fprintf(stderr,
> +                "The builder option is being deprecated, please use type instead.\n");

Line length.  Probably best to shuffle the message to the left, rather
than linewrapping, for ease of grepping.

The error message would benefit from some `quotes' around the
parameter names.

It would be nice if you did this in a way that meant that a config
file which specified both `type="hvm"' and `builder="hvm"' did not
generate a warning.

And it ought to be an error to specify `type="pv"' with
`builder="hvm"' surely.

Ian.

Roger Pau Monne Sept. 21, 2017, 5:16 p.m. UTC | #2

On Wed, Sep 20, 2017 at 03:50:48PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 04/21] xl: introduce a domain type option"):
> > Introduce a new type option to xl configuration files in order to
> > specify the domain type. This supersedes the current builder option.
> ...> 
> > Th
> >      libxl_defbool_set(&c_info->run_hotplug_scripts, run_hotplug_scripts);
> >      c_info->type = LIBXL_DOMAIN_TYPE_PV;
> > -    if (!xlu_cfg_get_string (config, "builder", &buf, 0) &&
> > -        !strncmp(buf, "hvm", strlen(buf)))
> > -        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
> > +    if (!xlu_cfg_get_string (config, "builder", &buf, 0)) {
> > +        fprintf(stderr,
> > +                "The builder option is being deprecated, please use type instead.\n");
> 
> Line length.  Probably best to shuffle the message to the left, rather
> than linewrapping, for ease of grepping.
> 
> The error message would benefit from some `quotes' around the
> parameter names.
> 
> It would be nice if you did this in a way that meant that a config
> file which specified both `type="hvm"' and `builder="hvm"' did not
> generate a warning.

Hm, really? I can do that but it seems weird because it doesn't
promote deprecation.

Thanks, Roger.

Ian Jackson Sept. 21, 2017, 5:22 p.m. UTC | #3

Roger Pau Monné writes ("Re: [PATCH v2 04/21] xl: introduce a domain type option"):
> On Wed, Sep 20, 2017 at 03:50:48PM +0100, Ian Jackson wrote:
> > It would be nice if you did this in a way that meant that a config
> > file which specified both `type="hvm"' and `builder="hvm"' did not
> > generate a warning.
> 
> Hm, really? I can do that but it seems weird because it doesn't
> promote deprecation.

It does promote deprecation, surely.  Such a config file is compatible
with all past and all planned future versions of Xen.  In particular,
if we stop honouring builder= at all, it will still work.

Ian.

Patch

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 79cb2eaea7..ab53436da2 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -54,9 +54,9 @@  Pairs may be separated either by a newline or a semicolon.  Both
 of the following are valid:
 
   name="h0"
-  builder="hvm"
+  type="hvm"
 
-  name="h0"; builder="hvm"
+  name="h0"; type="hvm"
 
 =head1 OPTIONS
 
@@ -77,6 +77,24 @@  single host must be unique.
 
 =over 4
 
+=item B<type="pv">
+
+Specifies that this is to be a PV domain, suitable for hosting Xen-aware guest
+operating systems. This is the default.
+
+=item B<type="hvm">
+
+Specifies that this is to be an HVM domain. That is, a fully virtualised
+computer with emulated BIOS, disk and network peripherals, etc.
+
+=back
+
+=head3 Deprecated guest type selection
+
+Note that the builder option is being deprecated in favor of the type option.
+
+=over 4
+
 =item B<builder="generic">
 
 Specifies that this is to be a PV domain, suitable for hosting Xen-aware guest
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 61f9a38573..65297352bd 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -854,9 +854,23 @@  void parse_config_data(const char *config_source,
 
     libxl_defbool_set(&c_info->run_hotplug_scripts, run_hotplug_scripts);
     c_info->type = LIBXL_DOMAIN_TYPE_PV;
-    if (!xlu_cfg_get_string (config, "builder", &buf, 0) &&
-        !strncmp(buf, "hvm", strlen(buf)))
-        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
+    if (!xlu_cfg_get_string (config, "builder", &buf, 0)) {
+        fprintf(stderr,
+                "The builder option is being deprecated, please use type instead.\n");
+        if (!strncmp(buf, "hvm", strlen(buf)))
+            c_info->type = LIBXL_DOMAIN_TYPE_HVM;
+    }
+
+    if (!xlu_cfg_get_string (config, "type", &buf, 0)) {
+        if (!strncmp(buf, "hvm", strlen(buf))) {
+            c_info->type = LIBXL_DOMAIN_TYPE_HVM;
+        } else if (!strncmp(buf, "pv", strlen(buf))) {
+            c_info->type = LIBXL_DOMAIN_TYPE_PV;
+        } else {
+            fprintf(stderr, "Invalid domain type %s.\n", buf);
+            exit(1);
+        }
+    }
 
     xlu_cfg_get_defbool(config, "hap", &c_info->hap, 0);