libxl: set disk defaults in remove/destroy functions

RFC PATCH In review Jim Fehlig 4 participants 10 messages Jan 26, 2015 – Feb 3, 2015

Share & tools

Jim Fehlig

author

2015-01-26 23:14 UTC #0 lore raw

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

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

Quoted 1 line

From 48f5dc7b80384538ff343af46379efefe51986be Mon Sep 17 00:00:00 2001

From: Jim Fehlig <[email protected]>
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

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 <[email protected]>

---
 tools/libxl/libxl.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

tools/libxl/libxl.c +33 -2

index 82227e8..e08638f 100644
@@ -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
 
 /******************************************************************************/

Signature · 1 line

1.8.0.1

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-02-02 13:27 UTC #1 lore raw

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.

Quoted 35 lines

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
[email protected]
http://lists.xen.org/xen-devel

Ian Campbell

2015-02-02 13:36 UTC #2 lore raw

Quoted 27 lines · 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.
Quoted 22 lines
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?

Quoted 4 lines

Quoted 4 lines
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.

Ian Jackson

2015-02-02 13:41 UTC #3 lore raw

Ian Campbell writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):

Quoted 2 lines

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...

Quoted 4 lines

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.

Wei Liu

2015-02-02 16:32 UTC #4 lore raw

In reply to Ian Campbell

Quoted 33 lines · 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.
Quoted 28 lines
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.

Quoted 8 lines

Quoted 7 lines
Regards,
Jim
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Jackson

2015-02-02 16:38 UTC #5 lore raw

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):

Quoted 6 lines

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.

Wei Liu

2015-02-02 18:00 UTC #6 lore raw

Quoted 11 lines · Ian Jackson

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
Quoted 6 lines
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<---

Quoted 1 line

From d2913eb11788db23a1e1541c86e9ffe1a2cb9a30 Mon Sep 17 00:00:00 2001

From: Wei Liu <[email protected]>
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 <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Ian Jackson <[email protected]>

---
 tools/libxl/libxl.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

tools/libxl/libxl.c +42 -3

index 82227e8..eff68f2 100644
@@ -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,

Signature · 1 line

1.9.1

Ian Jackson

2015-02-02 18:06 UTC #7 lore raw

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):

Quoted 5 lines · Ian Jackson

Quoted 2 lines
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.

Wei Liu

2015-02-02 18:13 UTC #8 lore raw

Quoted 10 lines · Ian Jackson

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):
Quoted 5 lines · Ian Jackson
Quoted 2 lines
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.

Quoted 6 lines

Ian.
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ian Jackson

2015-02-03 11:27 UTC #9 lore raw

Wei Liu writes ("Re: [Xen-devel] [PATCH RFC] libxl: set disk defaults in remove/destroy functions"):

Quoted 6 lines · Ian Jackson

Quoted 2 lines
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.