[PATCH RFC] libxl: set disk defaults in remove/destroy functions

[PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Jim Fehlig]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

The attached patch is a hack I cooked up to fix one of the libvirt-TCK
Xen failures.  The test (200-disk-hotplug.t) attempts to hot add and
remove a disk from a running domain.  The add works fine, but remove
fails with

libxl: debug: libxl.c:3858:libxl_device_disk_remove: ao 0x7f9b9c0015a0:
create: how=(nil) callback=(nil) poller=
0x7f9ba0004590
libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk
backend type: 0

The test does not define a backend type, in which case the libvirt libxl
driver allows libxl to choose an appropriate backend via
libxl__device_disk_set_backend().  The backend type is never set on a
remove operation, hence it fails with the above error.

I spent some time trying to figure out the best place to set backend
type on remove, but in the end could only come up with this hack.  It
wouldn't feel so gross if I could have simply added
'libxl__device_##type##_setdefault(gc, type);' to the existing
DEFINE_DEVICE_REMOVE macro, but alas libxl__device_nic_setdefault() has
a different prototype than the other devices.

Better suggestions welcomed!  One I considered was fixing this in
libvirt.  But the Xen community suggested allowing libxl to choose a
suitable backend when not specified, so I think this recommendation
should be symmetrical in the add and remove operations.

Regards,
Jim


[-- Attachment #2: 0001-libxl-set-disk-defaults-in-remove-destroy-functions.patch --]
[-- Type: text/x-diff, Size: 4214 bytes --]

>From 48f5dc7b80384538ff343af46379efefe51986be Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
Date: Mon, 26 Jan 2015 15:30:20 -0700
Subject: [PATCH] libxl: set disk defaults in remove/destroy functions

It is possible to hot add a disk without specifying a backend,
allowing libxl to determine a suitable one.  E.g. a disk can be
hot added with the following libvirt domXML snippet

<disk type='file' device='disk'>
  <source file='/some/path'/>
  <target dev='xvdb' bus='xen'/>
</disk>

But trying to remove the disk with the same XML snippet results in

libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk
       backend type: 0

Unlike disk_add, disk_remove does not call libxl__device_disk_setdefault().
This patch ensures libxl__device_disk_setdefault() is called on the
remove operation as well.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 tools/libxl/libxl.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 82227e8..e08638f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4108,6 +4108,36 @@ out:
  * libxl_device_vfb_remove
  * libxl_device_vfb_destroy
  */
+#define DEFINE_DISK_DEVICE_REMOVE(removedestroy, f)                     \
+    int libxl_device_disk_##removedestroy(libxl_ctx *ctx,               \
+        uint32_t domid, libxl_device_disk *disk,                        \
+        const libxl_asyncop_how *ao_how)                                \
+    {                                                                   \
+        AO_CREATE(ctx, domid, ao_how);                                  \
+        libxl__device *device;                                          \
+        libxl__ao_device *aodev;                                        \
+        int rc;                                                         \
+                                                                        \
+        rc = libxl__device_disk_setdefault(gc, disk);                   \
+        if (rc) goto out;                                               \
+                                                                        \
+        GCNEW(device);                                                  \
+        rc = libxl__device_from_disk(gc, domid, disk, device);          \
+        if (rc != 0) goto out;                                          \
+                                                                        \
+        GCNEW(aodev);                                                   \
+        libxl__prepare_ao_device(ao, aodev);                            \
+        aodev->action = LIBXL__DEVICE_ACTION_REMOVE;                    \
+        aodev->dev = device;                                            \
+        aodev->callback = device_addrm_aocomplete;                      \
+        aodev->force = f;                                               \
+        libxl__initiate_device_remove(egc, aodev);                      \
+                                                                        \
+    out:                                                                \
+        if (rc) return AO_ABORT(rc);                                    \
+        return AO_INPROGRESS;                                           \
+    }
+
 #define DEFINE_DEVICE_REMOVE(type, removedestroy, f)                    \
     int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,           \
         uint32_t domid, libxl_device_##type *type,                      \
@@ -4138,8 +4168,8 @@ out:
 /* Define all remove/destroy functions and undef the macro */
 
 /* disk */
-DEFINE_DEVICE_REMOVE(disk, remove, 0)
-DEFINE_DEVICE_REMOVE(disk, destroy, 1)
+DEFINE_DISK_DEVICE_REMOVE(remove, 0)
+DEFINE_DISK_DEVICE_REMOVE(destroy, 1)
 
 /* nic */
 DEFINE_DEVICE_REMOVE(nic, remove, 0)
@@ -4162,6 +4192,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
  * 1. add support for secondary consoles to xenconsoled
  * 2. dynamically add/remove qemu chardevs via qmp messages. */
 
+#undef DEFINE_DISK_DEVICE_REMOVE
 #undef DEFINE_DEVICE_REMOVE
 
 /******************************************************************************/
-- 
1.8.0.1


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Campbell]

On Mon, 2015-01-26 at 16:14 -0700, Jim Fehlig wrote:

Cc-ing the other toolstack maintainers, both of whom have more
familiarity with this part of libxl than I.

> The attached patch is a hack I cooked up to fix one of the libvirt-TCK
> Xen failures.  The test (200-disk-hotplug.t) attempts to hot add and
> remove a disk from a running domain.  The add works fine, but remove
> fails with
> 
> libxl: debug: libxl.c:3858:libxl_device_disk_remove: ao 0x7f9b9c0015a0:
> create: how=(nil) callback=(nil) poller=
> 0x7f9ba0004590
> libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk
> backend type: 0
> 
> The test does not define a backend type, in which case the libvirt libxl
> driver allows libxl to choose an appropriate backend via
> libxl__device_disk_set_backend().  The backend type is never set on a
> remove operation, hence it fails with the above error.
> 
> I spent some time trying to figure out the best place to set backend
> type on remove, but in the end could only come up with this hack.  It
> wouldn't feel so gross if I could have simply added
> 'libxl__device_##type##_setdefault(gc, type);' to the existing
> DEFINE_DEVICE_REMOVE macro, but alas libxl__device_nic_setdefault() has
> a different prototype than the other devices.
> 
> Better suggestions welcomed!  One I considered was fixing this in
> libvirt.  But the Xen community suggested allowing libxl to choose a
> suitable backend when not specified, so I think this recommendation
> should be symmetrical in the add and remove operations.
> 
> Regards,
> Jim
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Campbell]

On Mon, 2015-02-02 at 13:27 +0000, Ian Campbell wrote:
> On Mon, 2015-01-26 at 16:14 -0700, Jim Fehlig wrote:
> 
> Cc-ing the other toolstack maintainers, both of whom have more
> familiarity with this part of libxl than I.
> 
> > The attached patch is a hack I cooked up to fix one of the libvirt-TCK
> > Xen failures.  The test (200-disk-hotplug.t) attempts to hot add and
> > remove a disk from a running domain.  The add works fine, but remove
> > fails with
> > 
> > libxl: debug: libxl.c:3858:libxl_device_disk_remove: ao 0x7f9b9c0015a0:
> > create: how=(nil) callback=(nil) poller=
> > 0x7f9ba0004590
> > libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk
> > backend type: 0
> > 
> > The test does not define a backend type, in which case the libvirt libxl
> > driver allows libxl to choose an appropriate backend via
> > libxl__device_disk_set_backend().  The backend type is never set on a
> > remove operation, hence it fails with the above error.
> > 
> > I spent some time trying to figure out the best place to set backend
> > type on remove, but in the end could only come up with this hack.  It
> > wouldn't feel so gross if I could have simply added
> > 'libxl__device_##type##_setdefault(gc, type);' to the existing
> > DEFINE_DEVICE_REMOVE macro, but alas libxl__device_nic_setdefault() has
> > a different prototype than the other devices.

(I'm not sure this is the right apporach, but if it were...)

FWIW libxl__ functions aren't stable, so in theory this could be changed
to rationalise them all (e.g. by adding domid to the rest), but that
would be a bit churnful.

Perhaps better would be to add a new parameter to0 DEFINE_DEVICE_REMOVE
like extra_setdefault_args which is pasted in the appropriate place?

> > Better suggestions welcomed!  One I considered was fixing this in
> > libvirt.  But the Xen community suggested allowing libxl to choose a
> > suitable backend when not specified, so I think this recommendation
> > should be symmetrical in the add and remove operations.

I suppose on remove its not so much a case of choosing a suitable
backend as reflecting the actual current reality for that device. Which
suggests that libxl__device_from_disk ought to be figuring out the
actual backend somehow rather than being told it.

Ian.

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> Perhaps better would be to add a new parameter to0 DEFINE_DEVICE_REMOVE
> like extra_setdefault_args which is pasted in the appropriate place?

I don't think this is right because...

> I suppose on remove its not so much a case of choosing a suitable
> backend as reflecting the actual current reality for that device. Which
> suggests that libxl__device_from_disk ought to be figuring out the
> actual backend somehow rather than being told it.

... yes.  To remove a device you need to tear down the actual backend,
not whatever the default might be.  libxl_domain_destroy must be able
to do this, surely ?  (I haven't checked the code...)

Ian.

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Wei Liu]

On Mon, Feb 02, 2015 at 01:27:13PM +0000, Ian Campbell wrote:
> On Mon, 2015-01-26 at 16:14 -0700, Jim Fehlig wrote:
> 
> Cc-ing the other toolstack maintainers, both of whom have more
> familiarity with this part of libxl than I.
> 
> > The attached patch is a hack I cooked up to fix one of the libvirt-TCK
> > Xen failures.  The test (200-disk-hotplug.t) attempts to hot add and
> > remove a disk from a running domain.  The add works fine, but remove
> > fails with
> > 
> > libxl: debug: libxl.c:3858:libxl_device_disk_remove: ao 0x7f9b9c0015a0:
> > create: how=(nil) callback=(nil) poller=
> > 0x7f9ba0004590
> > libxl: error: libxl.c:2399:libxl__device_from_disk: unrecognized disk
> > backend type: 0
> > 
> > The test does not define a backend type, in which case the libvirt libxl
> > driver allows libxl to choose an appropriate backend via
> > libxl__device_disk_set_backend().  The backend type is never set on a
> > remove operation, hence it fails with the above error.
> > 
> > I spent some time trying to figure out the best place to set backend
> > type on remove, but in the end could only come up with this hack.  It
> > wouldn't feel so gross if I could have simply added
> > 'libxl__device_##type##_setdefault(gc, type);' to the existing
> > DEFINE_DEVICE_REMOVE macro, but alas libxl__device_nic_setdefault() has
> > a different prototype than the other devices.
> > 
> > Better suggestions welcomed!  One I considered was fixing this in
> > libvirt.  But the Xen community suggested allowing libxl to choose a
> > suitable backend when not specified, so I think this recommendation
> > should be symmetrical in the add and remove operations.
> > 

FWIW xl block-detach calls libxl_vdev_to_device_disk to convert a vdev
to disk. That function reads xenstore to get the actual backend of that
specific vdev. Don't know how useful it is to libvirt though.

Maybe we should look up disk's backend in libxl_device_disk_remove? Not
sure what's the best approach.

Wei.

> > Regards,
> > Jim
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> FWIW xl block-detach calls libxl_vdev_to_device_disk to convert a vdev
> to disk. That function reads xenstore to get the actual backend of that
> specific vdev. Don't know how useful it is to libvirt though.
> 
> Maybe we should look up disk's backend in libxl_device_disk_remove? Not
> sure what's the best approach.

libxl_device_disk_remove should definitely not require the caller to
do what xl currently does.

Ian.

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Wei Liu]

On Mon, Feb 02, 2015 at 04:38:00PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> > FWIW xl block-detach calls libxl_vdev_to_device_disk to convert a vdev
> > to disk. That function reads xenstore to get the actual backend of that
> > specific vdev. Don't know how useful it is to libvirt though.
> > 
> > Maybe we should look up disk's backend in libxl_device_disk_remove? Not
> > sure what's the best approach.
> 
> libxl_device_disk_remove should definitely not require the caller to
> do what xl currently does.
> 

I come up with something like this (only compile tested).

---8<---
>From d2913eb11788db23a1e1541c86e9ffe1a2cb9a30 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 2 Feb 2015 17:56:47 +0000
Subject: [PATCH] libxl: libxl__device_from_disk should retrieve backend from
 xenstore

Also change the function to use goto idiom while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 82227e8..eff68f2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2264,12 +2264,49 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int devid;
+    int rc = 0;
 
     devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
     if (devid==-1) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
                " virtual disk identifier %s", disk->vdev);
-        return ERROR_INVAL;
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN) {
+        char *dompath, *be_path, *be_string;
+
+        dompath = libxl__xs_get_dompath(gc, domid);
+        if (!dompath) {
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        be_path = libxl__xs_read(gc, XBT_NULL,
+                                 GCSPRINTF("%s/device/vbd/%d/backend",
+                                           dompath, devid));
+        if  (!be_path) {
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = sscanf(be_path, "/local/domain/%d/", &disk->backend_domid);
+        if (rc != 1) {
+            LOG(ERROR, "Unable to fetch device backend domid from %s",
+                be_path);
+            rc =  ERROR_FAIL;
+            goto out;
+        }
+
+        be_string = libxl__xs_read(gc, XBT_NULL,
+                                   GCSPRINTF("%s/type", be_path));
+        if (!be_string) {
+            LOG(ERROR, "Missing xenstore node %s/type", be_path);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        libxl_string_to_backend(ctx, be_string, &disk->backend);
     }
 
     device->backend_domid = disk->backend_domid;
@@ -2288,14 +2325,16 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
                        disk->backend);
-            return ERROR_INVAL;
+            rc = ERROR_INVAL;
+            goto out;
     }
 
     device->domid = domid;
     device->devid = devid;
     device->kind  = LIBXL__DEVICE_KIND_VBD;
 
-    return 0;
+out:
+    return rc;
 }
 
 /* Specific function called directly only by local disk attach,
-- 
1.9.1

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> On Mon, Feb 02, 2015 at 04:38:00PM +0000, Ian Jackson wrote:
> > libxl_device_disk_remove should definitely not require the caller to
> > do what xl currently does.
> 
> I come up with something like this (only compile tested).

Something like this perhaps, yes.  Although it seems to have rather
too much similarity with parts of libxl__device_disk_from_xs_be...

Ian.

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Wei Liu]

On Mon, Feb 02, 2015 at 06:06:15PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> > On Mon, Feb 02, 2015 at 04:38:00PM +0000, Ian Jackson wrote:
> > > libxl_device_disk_remove should definitely not require the caller to
> > > do what xl currently does.
> > 
> > I come up with something like this (only compile tested).
> 
> Something like this perhaps, yes.  Although it seems to have rather
> too much similarity with parts of libxl__device_disk_from_xs_be...
> 

I wanted to refactor those bits but I couldn't come up with a sensible
function name. I'm particularly bad at naming things. :-(

If you come up with a name I can factor those bits out.

Wei.

> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: set disk defaults in remove/destroy functions

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
> On Mon, Feb 02, 2015 at 06:06:15PM +0000, Ian Jackson wrote:
> > Something like this perhaps, yes.  Although it seems to have rather
> > too much similarity with parts of libxl__device_disk_from_xs_be...
> 
> I wanted to refactor those bits but I couldn't come up with a sensible
> function name. I'm particularly bad at naming things. :-(

libxl__device_disk_backend_type_from_xs ?

Ian.