[PATCH v2] libxl: use qemu-xen (upstream QEMU) as device model by default

[PATCH v2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Stefano Stabellini]

Changes in v2:
- update the xl man page;
- write a small helper function in libxl_{linux,netbsd}.c to set the
default device_model, so that NetBSD can keep using the old version.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index fe4fac9..69a38b9 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1132,15 +1132,15 @@ guest. Valid values are:
 
 =over 4
 
-=item B<qemu-xen-traditional>
+=item B<qemu-xen>
 
-Use the device-model based upon the historical Xen fork of Qemu.  This
-device-model is currently the default.
+use the device-model merged into the upstream QEMU project.
+This device-model is the default for Linux dom0.
 
-=item B<qemu-xen>
+=item B<qemu-xen-traditional>
 
-use the device-model merged into the upstream QEMU project.  This
-device-model will become the default in a future version of Xen.
+Use the device-model based upon the historical Xen fork of Qemu.
+This device-model is still the default for NetBSD dom0.
 
 =back
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9d20086..6ec543a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -143,8 +143,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
 
     if (!b_info->device_model_version) {
         if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
-            b_info->device_model_version =
-                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+            b_info->device_model_version = libxl__default_device_model(gc);
         else {
             const char *dm;
             int rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cba3616..0ea11d1 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1557,6 +1557,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
+  /* Return the system-wide default device model:
+   * qemu-xen for Linux, qemu-xen-traditional for NetBSD.
+   */
+_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1fed3cd..409e9f2 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -266,3 +266,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
 out:
     return rc;
 }
+
+libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
+{
+    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9587833..aa04969 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -94,3 +94,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
 out:
     return rc;
 }
+
+libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
+{
+    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+}

Re: [PATCH v2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Ian Campbell]

On Tue, 2012-12-04 at 12:58 +0000, Stefano Stabellini wrote:
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index fe4fac9..69a38b9 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1132,15 +1132,15 @@ guest. Valid values are:
>  
>  =over 4
>  
> -=item B<qemu-xen-traditional>
> +=item B<qemu-xen>
>  
> -Use the device-model based upon the historical Xen fork of Qemu.  This
> -device-model is currently the default.
> +use the device-model merged into the upstream QEMU project.

Can you correct to "Use" as you move it please.

> +This device-model is the default for Linux dom0.
>  
> -=item B<qemu-xen>
> +=item B<qemu-xen-traditional>
>  
> -use the device-model merged into the upstream QEMU project.  This
> -device-model will become the default in a future version of Xen.
> +Use the device-model based upon the historical Xen fork of Qemu.
> +This device-model is still the default for NetBSD dom0.

Does that look like changing for 4.3?

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 9d20086..6ec543a 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -143,8 +143,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>  
>      if (!b_info->device_model_version) {
>          if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
> -            b_info->device_model_version =
> -                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +            b_info->device_model_version = libxl__default_device_model(gc);

For PV we stat() the executable before using it -- I don't remember why
but should we be doing the same here?

Also, will this not break things for people with
"device_model_stubdomain_override = 1" (but nothing else) in their
configuration?

I think the logic needs to be (perhaps in Linux's
libxl__default_device_model(gc) only)
	if (libxl_defbool_val(b_info->device_model_stubdomain)
		LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
	else
		LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;

? At least until we have upstream stubdoms in some form.

>          else {
>              const char *dm;
>              int rc;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index cba3616..0ea11d1 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1557,6 +1557,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
>    /* Based on /local/domain/$domid/dm-version xenstore key
>     * default is qemu xen traditional */
>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
> +  /* Return the system-wide default device model:
> +   * qemu-xen for Linux, qemu-xen-traditional for NetBSD.

I bet you someone forgets to update this when NetBSD changes ;-)

> +   */
> +_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
>  
>  /* Check how executes hotplug script currently */
>  int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);

[PATCH v3 0/2] libxl: change to qemu-xen (upstream) by default

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v2] libxl: use qemu-xen (upstream QEMU) as device model by default"):
> On Tue, 2012-12-04 at 12:58 +0000, Stefano Stabellini wrote:
> > -Use the device-model based upon the historical Xen fork of Qemu.  This
> > -device-model is currently the default.
> > +use the device-model merged into the upstream QEMU project.
> 
> Can you correct to "Use" as you move it please.

Fixed.

> > -use the device-model merged into the upstream QEMU project.  This
> > -device-model will become the default in a future version of Xen.
> > +Use the device-model based upon the historical Xen fork of Qemu.
> > +This device-model is still the default for NetBSD dom0.
> 
> Does that look like changing for 4.3?

I guess not but I don't think that should block this change.

> > -            b_info->device_model_version =
> > -                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> > +            b_info->device_model_version = libxl__default_device_model(gc);
> 
> For PV we stat() the executable before using it -- I don't remember why
> but should we be doing the same here?

Presumably in case the upstream build was disabled.  I think the same
logic applies so I have arranged to do the same.

> I think the logic needs to be (perhaps in Linux's
> libxl__default_device_model(gc) only)
> 	if (libxl_defbool_val(b_info->device_model_stubdomain)
> 		LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> 	else
> 		LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;

Yes, done.

> > +  /* Return the system-wide default device model:
> > +   * qemu-xen for Linux, qemu-xen-traditional for NetBSD.
> 
> I bet you someone forgets to update this when NetBSD changes ;-)

That remark just needs to go IMO.

So we now have:
 [PATCH 1/2] libxl: move check for existence of qemuu device model
 [PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default

[PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Jackson]

The stat in libxl__domain_build_info_setdefault's default device model
logic works to fall back to qemu-xen-traditional whenever the
executable for qemu-xen is not found.

We are going to use qemu-xen-traditional in more cases, so break this
check out into its own if statement.

Also add a pair of braces to make the if() statement symmetrical.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

--
New in v3 of series.
---
 tools/libxl/libxl_create.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index efeebf2..04bf4a5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -105,22 +105,25 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
 
     if (!b_info->device_model_version) {
-        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
+        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
             b_info->device_model_version =
                 LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
-        else {
+        } else {
+            b_info->device_model_version =
+                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+        }
+        if (b_info->device_model_version
+                == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
             const char *dm;
             int rc;
 
-            b_info->device_model_version =
-                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
             dm = libxl__domain_device_model(gc, b_info);
             rc = access(dm, X_OK);
             if (rc < 0) {
                 /* qemu-xen unavailable, use qemu-xen-traditional */
                 if (errno == ENOENT) {
                     LIBXL__LOG_ERRNO(CTX, XTL_VERBOSE, "qemu-xen is unavailable"
-                            ", use qemu-xen-traditional instead");
+                                     ", use qemu-xen-traditional instead");
                     b_info->device_model_version =
                         LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
                 } else {
-- 
1.7.2.5

[PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Ian Jackson]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

--
Changes in v3:
- Fix "use" to "Use" in documentation part
- Do not try to use qemu-xen (upstream) for stubdoms
- Remove a hostage-to-fortune remark in a comment about OS defaults

Changes in v2:
- update the xl man page;
- write a small helper function in libxl_{linux,netbsd}.c to set the
default device_model, so that NetBSD can keep using the old version.
---
 docs/man/xl.cfg.pod.5        |   12 ++++++------
 tools/libxl/libxl_create.c   |   10 +++++++---
 tools/libxl/libxl_internal.h |    2 ++
 tools/libxl/libxl_linux.c    |    5 +++++
 tools/libxl/libxl_netbsd.c   |    5 +++++
 5 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 25523c9..8db24d7 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1176,15 +1176,15 @@ guest. Valid values are:
 
 =over 4
 
-=item B<qemu-xen-traditional>
+=item B<qemu-xen>
 
-Use the device-model based upon the historical Xen fork of Qemu.  This
-device-model is currently the default.
+Use the device-model merged into the upstream QEMU project.
+This device-model is the default for Linux dom0.
 
-=item B<qemu-xen>
+=item B<qemu-xen-traditional>
 
-use the device-model merged into the upstream QEMU project.  This
-device-model will become the default in a future version of Xen.
+Use the device-model based upon the historical Xen fork of Qemu.
+This device-model is still the default for NetBSD dom0.
 
 =back
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 04bf4a5..7ec8c2b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -105,9 +105,13 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
 
     if (!b_info->device_model_version) {
-        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
-            b_info->device_model_version =
-                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
+            if (libxl_defbool_val(info->device_model_stubdomain)) {
+                b_info->device_model_version =
+                    LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+            } else {
+                b_info->device_model_version = libxl__default_device_model(gc);
+            }
         } else {
             b_info->device_model_version =
                 LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1567b4b..25172f0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1561,6 +1561,8 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
+  /* Return the system-wide default device model */
+_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1fed3cd..409e9f2 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -266,3 +266,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
 out:
     return rc;
 }
+
+libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
+{
+    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9587833..aa04969 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -94,3 +94,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
 out:
     return rc;
 }
+
+libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
+{
+    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+}
-- 
1.7.2.5

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Campbell]

On Fri, 2013-03-01 at 17:41 +0000, Ian Jackson wrote:
> +        if (b_info->device_model_version
> +                == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>              const char *dm;
>              int rc;
>  
> -            b_info->device_model_version =
> -                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
>              dm = libxl__domain_device_model(gc, b_info);
>              rc = access(dm, X_OK);
>              if (rc < 0) {
>                  /* qemu-xen unavailable, use qemu-xen-traditional */
>                  if (errno == ENOENT) {
>                      LIBXL__LOG_ERRNO(CTX, XTL_VERBOSE, "qemu-xen is unavailable"
> -                            ", use qemu-xen-traditional instead");
> +                                     ", use qemu-xen-traditional instead");
>                      b_info->device_model_version =
>                          LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;

After the following change (in 2/2) won't this stat potentially be
checking for the qemu-xen-trad binaries, which makes the error message
somewhat confusing and also makes the fallback unhelpful.

On a related note we could do with stating the device model even in the
case where the user has specified device_model_version explicitly, to
give better error reporting in that case too.

Ian.

Re: [PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Stefano Stabellini]

On Fri, 1 Mar 2013, Ian Jackson wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> --
> Changes in v3:
> - Fix "use" to "Use" in documentation part
> - Do not try to use qemu-xen (upstream) for stubdoms
> - Remove a hostage-to-fortune remark in a comment about OS defaults
> 
> Changes in v2:
> - update the xl man page;
> - write a small helper function in libxl_{linux,netbsd}.c to set the
> default device_model, so that NetBSD can keep using the old version.

it looks OK to me, thanks for picking it up


>  docs/man/xl.cfg.pod.5        |   12 ++++++------
>  tools/libxl/libxl_create.c   |   10 +++++++---
>  tools/libxl/libxl_internal.h |    2 ++
>  tools/libxl/libxl_linux.c    |    5 +++++
>  tools/libxl/libxl_netbsd.c   |    5 +++++
>  5 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 25523c9..8db24d7 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1176,15 +1176,15 @@ guest. Valid values are:
>  
>  =over 4
>  
> -=item B<qemu-xen-traditional>
> +=item B<qemu-xen>
>  
> -Use the device-model based upon the historical Xen fork of Qemu.  This
> -device-model is currently the default.
> +Use the device-model merged into the upstream QEMU project.
> +This device-model is the default for Linux dom0.
>  
> -=item B<qemu-xen>
> +=item B<qemu-xen-traditional>
>  
> -use the device-model merged into the upstream QEMU project.  This
> -device-model will become the default in a future version of Xen.
> +Use the device-model based upon the historical Xen fork of Qemu.
> +This device-model is still the default for NetBSD dom0.
>  
>  =back
>  
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 04bf4a5..7ec8c2b 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -105,9 +105,13 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
>  
>      if (!b_info->device_model_version) {
> -        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> -            b_info->device_model_version =
> -                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
> +            if (libxl_defbool_val(info->device_model_stubdomain)) {
> +                b_info->device_model_version =
> +                    LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +            } else {
> +                b_info->device_model_version = libxl__default_device_model(gc);
> +            }
>          } else {
>              b_info->device_model_version =
>                  LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 1567b4b..25172f0 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1561,6 +1561,8 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
>    /* Based on /local/domain/$domid/dm-version xenstore key
>     * default is qemu xen traditional */
>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
> +  /* Return the system-wide default device model */
> +_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
>  
>  /* Check how executes hotplug script currently */
>  int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 1fed3cd..409e9f2 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -266,3 +266,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>  out:
>      return rc;
>  }
> +
> +libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
> +{
> +    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
> +}
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9587833..aa04969 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -94,3 +94,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>  out:
>      return rc;
>  }
> +
> +libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
> +{
> +    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
> +}
> -- 
> 1.7.2.5
> 

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> On Fri, 2013-03-01 at 17:41 +0000, Ian Jackson wrote:
> > +        if (b_info->device_model_version
> > +                == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> >              const char *dm;
> >              int rc;
...
> >              rc = access(dm, X_OK);
> >              if (rc < 0) {
> >                  /* qemu-xen unavailable, use qemu-xen-traditional */
...
> After the following change (in 2/2) won't this stat potentially be
> checking for the qemu-xen-trad binaries, which makes the error message
> somewhat confusing and also makes the fallback unhelpful.

No, because the whole thing is in an if which applies only if we're
trying to use ..._QEMU_XEN.

> On a related note we could do with stating the device model even in the
> case where the user has specified device_model_version explicitly, to
> give better error reporting in that case too.

You mean logging it ?

Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Campbell]

On Mon, 2013-03-04 at 11:42 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> > On Fri, 2013-03-01 at 17:41 +0000, Ian Jackson wrote:
> > > +        if (b_info->device_model_version
> > > +                == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> > >              const char *dm;
> > >              int rc;
> ...
> > >              rc = access(dm, X_OK);
> > >              if (rc < 0) {
> > >                  /* qemu-xen unavailable, use qemu-xen-traditional */
> ...
> > After the following change (in 2/2) won't this stat potentially be
> > checking for the qemu-xen-trad binaries, which makes the error message
> > somewhat confusing and also makes the fallback unhelpful.
> 
> No, because the whole thing is in an if which applies only if we're
> trying to use ..._QEMU_XEN.

Oh, right. I combined the tow patches wrongly in my head.

Don't we also want to check for the presence of the trad binary though?

> > On a related note we could do with stating the device model even in the
> > case where the user has specified device_model_version explicitly, to
> > give better error reporting in that case too.
> 
> You mean logging it ?

Actually I meant "not trying to start the guest regardless and returning
success", which AIUI is what happens... Logging would be nice too
though.

> 
> Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> On Mon, 2013-03-04 at 11:42 +0000, Ian Jackson wrote:
> > No, because the whole thing is in an if which applies only if we're
> > trying to use ..._QEMU_XEN.
> 
> Oh, right. I combined the tow patches wrongly in my head.
> 
> Don't we also want to check for the presence of the trad binary though?

AIUI the trad binary is always built ?  AIUI there is a possibility
that the new one isn't ?  And that was the rationale for this check ?

CCing Stefano.

> > > On a related note we could do with stating the device model even in the
> > > case where the user has specified device_model_version explicitly, to
> > > give better error reporting in that case too.
> > 
> > You mean logging it ?
> 
> Actually I meant "not trying to start the guest regardless and returning
> success", which AIUI is what happens... Logging would be nice too
> though.

Oh I see.  TBH once we start to think about these kinds of
installation problems it's difficult to see where we'll stop.

Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Campbell]

On Tue, 2013-03-05 at 11:22 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> > On Mon, 2013-03-04 at 11:42 +0000, Ian Jackson wrote:
> > > No, because the whole thing is in an if which applies only if we're
> > > trying to use ..._QEMU_XEN.
> > 
> > Oh, right. I combined the tow patches wrongly in my head.
> > 
> > Don't we also want to check for the presence of the trad binary though?
> 
> AIUI the trad binary is always built ?

By us it is, but e.g. Debian's packaging has been known to omit it, or
the user may have overridden the path but made a typo.

>   AIUI there is a possibility
> that the new one isn't ?  And that was the rationale for this check ?
> 
> CCing Stefano.
> 
> > > > On a related note we could do with stating the device model even in the
> > > > case where the user has specified device_model_version explicitly, to
> > > > give better error reporting in that case too.
> > > 
> > > You mean logging it ?
> > 
> > Actually I meant "not trying to start the guest regardless and returning
> > success", which AIUI is what happens... Logging would be nice too
> > though.
> 
> Oh I see.  TBH once we start to think about these kinds of
> installation problems it's difficult to see where we'll stop.

This is one which has been tripping people up in practice.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> On Tue, 2013-03-05 at 11:22 +0000, Ian Jackson wrote:
> > Oh I see.  TBH once we start to think about these kinds of
> > installation problems it's difficult to see where we'll stop.
> 
> This is one which has been tripping people up in practice.

OK then.

Should we stat whichever one we intend to use and fall back to the
other with a warning, then ?

Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Campbell]

On Tue, 2013-03-05 at 16:10 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> > On Tue, 2013-03-05 at 11:22 +0000, Ian Jackson wrote:
> > > Oh I see.  TBH once we start to think about these kinds of
> > > installation problems it's difficult to see where we'll stop.
> > 
> > This is one which has been tripping people up in practice.
> 
> OK then.
> 
> Should we stat whichever one we intend to use and fall back to the
> other with a warning, then ?
> 

Could do that if libxl is the one doing the choosing, I suppose. I'd
also be happy with failing explicitly with an appropriate message.

In the case where the user gave an explicit path or version we should
obviously use it or fail. 

Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> Could do that if libxl is the one doing the choosing, I suppose. I'd
> also be happy with failing explicitly with an appropriate message.

Right.

> In the case where the user gave an explicit path or version we should
> obviously use it or fail. 

Indeed.

Ian.

Re: [PATCH 1/2] libxl: move check for existence of qemuu device model

[Ian Campbell]

On Tue, 2013-03-05 at 04:03 +0000, Ian Campbell wrote:
> On Mon, 2013-03-04 at 11:42 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/2] libxl: move check for existence of qemuu device model"):
> > > On Fri, 2013-03-01 at 17:41 +0000, Ian Jackson wrote:
> > > > +        if (b_info->device_model_version
> > > > +                == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> > > >              const char *dm;
> > > >              int rc;
> > ...
> > > >              rc = access(dm, X_OK);
> > > >              if (rc < 0) {
> > > >                  /* qemu-xen unavailable, use qemu-xen-traditional */
> > ...
> > > After the following change (in 2/2) won't this stat potentially be
> > > checking for the qemu-xen-trad binaries, which makes the error message
> > > somewhat confusing and also makes the fallback unhelpful.
> > 
> > No, because the whole thing is in an if which applies only if we're
> > trying to use ..._QEMU_XEN.
> 
> Oh, right. I combined the tow patches wrongly in my head.

So I think this patch is good as far as it goes and the rest of this
thread is really followup material.
So:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> Don't we also want to check for the presence of the trad binary though?
> 
> > > On a related note we could do with stating the device model even in the
> > > case where the user has specified device_model_version explicitly, to
> > > give better error reporting in that case too.
> > 
> > You mean logging it ?
> 
> Actually I meant "not trying to start the guest regardless and returning
> success", which AIUI is what happens... Logging would be nice too
> though.
> 
> > 
> > Ian.
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Ian Campbell]

On Sun, 2013-03-03 at 05:07 +0000, Stefano Stabellini wrote:
> On Fri, 1 Mar 2013, Ian Jackson wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > --
> > Changes in v3:
> > - Fix "use" to "Use" in documentation part
> > - Do not try to use qemu-xen (upstream) for stubdoms
> > - Remove a hostage-to-fortune remark in a comment about OS defaults
> > 
> > Changes in v2:
> > - update the xl man page;
> > - write a small helper function in libxl_{linux,netbsd}.c to set the
> > default device_model, so that NetBSD can keep using the old version.
> 
> it looks OK to me, thanks for picking it up

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] libxl: use qemu-xen (upstream QEMU) as device model by default"):
> On Sun, 2013-03-03 at 05:07 +0000, Stefano Stabellini wrote:
> > it looks OK to me, thanks for picking it up
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks, pushed both to staging.

Ian.