[v3,05/22] xl: introduce a domain type option

Commit Message

Roger Pau Monné Sept. 25, 2017, 10:51 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]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
---
Changes since v2:
 - Left shift error message.
 - Detect wrong combination of 'builder' and 'type' options.
 - Do not complain if both type and builder and set to the same value.
 - Drop Wei's Ack (patch changed substantially).
---
 docs/man/xl.cfg.pod.5.in | 23 +++++++++++++++++++++--
 tools/xl/xl_parse.c      | 45 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 62 insertions(+), 6 deletions(-)

Comments

Ian Jackson Sept. 27, 2017, 2:21 p.m. UTC | #1

Roger Pau Monne writes ("[PATCH v3 05/22] 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.
> 
> The new option is documented in the xl.cfg man page, and the previous
> builder option is marked as deprecated.

Acked-by: Ian Jackson <[e-mail]>

Although,

> +#define CHECK_AND_SET_TYPE(type, new)                                  \
> +({                                                                     \

personally I would prefer the ({ on the previous line, and anyway, I
think you could probably avoid this macro by using a temporary here:

> +    /* Deprecated since Xen 4.10. */
> +    if (!xlu_cfg_get_string(config, "builder", &buf, 0)) {
> +        if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
> +            fprintf(stderr,
> +"The \"builder\" option is being deprecated, please use \"type\" instead.\n");
> +        if (!strncmp(buf, "hvm", strlen(buf)))
> +            CHECK_AND_SET_TYPE(c_info->type, LIBXL_DOMAIN_TYPE_HVM);

  +            type_from_builder = LIBXL_DOMAIN_TYPE_HVM;

> +        else if (!strncmp(buf, "generic", strlen(buf)))
> +            CHECK_AND_SET_TYPE(c_info->type, LIBXL_DOMAIN_TYPE_PV);
> +        else {
> +            fprintf(stderr, "Invalid domain type %s.\n", buf);
> +            exit(1);
> +        }

  +       if (c_info->type != LIBXL_DOMAIN_TYPE_INVALID &&
  +           c_info->type != type_from_builder) {

Up to you, though.  I won't insist on this change.  Hence my ack.

Ian.

Patch

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 247ae99ca7..df889ea7e4 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,25 @@  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 f672161541..f9afa9ad35 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -900,10 +900,47 @@  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, "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);
+        }
+    }
+
+#define CHECK_AND_SET_TYPE(type, new)                                  \
+({                                                                     \
+    if (((type) != LIBXL_DOMAIN_TYPE_INVALID) &&                       \
+        ((type) != (new))) {                                           \
+        fprintf(stderr,                                                \
+        "Contradicting \"builder\" and \"type\" options specified.\n");\
+        exit(1);                                                       \
+    }                                                                  \
+    (type) = (new);                                                    \
+})
+
+    /* Deprecated since Xen 4.10. */
+    if (!xlu_cfg_get_string(config, "builder", &buf, 0)) {
+        if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
+            fprintf(stderr,
+"The \"builder\" option is being deprecated, please use \"type\" instead.\n");
+        if (!strncmp(buf, "hvm", strlen(buf)))
+            CHECK_AND_SET_TYPE(c_info->type, LIBXL_DOMAIN_TYPE_HVM);
+        else if (!strncmp(buf, "generic", strlen(buf)))
+            CHECK_AND_SET_TYPE(c_info->type, LIBXL_DOMAIN_TYPE_PV);
+        else {
+            fprintf(stderr, "Invalid domain type %s.\n", buf);
+            exit(1);
+        }
+    }
+#undef CHECK_AND_SET_TYPE
+
+    if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
+        c_info->type = LIBXL_DOMAIN_TYPE_PV;
 
     xlu_cfg_get_defbool(config, "hap", &c_info->hap, 0);