Re: [PATCH] libxl: locally attach disks with hotplug scripts for bootloader execution

[George Dunlap <george.dunlap@eu.citrix.com>]

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

On 06/25/2015 03:17 PM, Roger Pau Monne wrote:
> Or else bootloader execution fails. Tested using an iSCSI disk.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported-by: Hildebrand, Nils <Nils.Hildebrand@bva.bund.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/libxl/libxl.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9117b01..6430836 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3063,9 +3063,16 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
>
>      switch (disk->backend) {
>          case LIBXL_DISK_BACKEND_PHY:
> -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
> -                       disk->pdev_path);
> -            dev = disk->pdev_path;
> +            if (disk->script == NULL && disk->backend_domname == NULL) {
> +                LOG(DEBUG, "locally attaching PHY disk %s", disk->pdev_path);
> +                dev = disk->pdev_path;
> +            } else {
> +                libxl__prepare_ao_device(ao, &dls->aodev);
> +                dls->aodev.callback = local_device_attach_cb;
> +                device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk, &dls->aodev,
> +                                libxl__alloc_vdev, (void *) blkdev_start);
> +                return;
> +            }

Although having said that -- isn't there also a bug here wrt qdisk
backends in a driver domain not being attached?

Could we do something like the attached patch?

This will do a local attach for blktap as well, which isn't strictly
necessary, but should work pretty much the same as for the block
scripts.  (And anyway I'm about to replace the blktap stuff with block
scripts anyway.)

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libxl-Make-local_initiate_attach-more-rational.patch --]
[-- Type: text/x-patch; name="0001-libxl-Make-local_initiate_attach-more-rational.patch", Size: 6147 bytes --]

From c4584a7a61e047bb87fb6133116c35dc0e79ab6d Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@eu.citrix.com>
Date: Thu, 25 Jun 2015 18:11:21 +0100
Subject: [PATCH] libxl: Make local_initiate_attach more rational

There are a lot of paths through
libxl__device_disk_local_initiate_attach(), but they all really boil
down to one thing: Can we just access the file directly, or do we need
to attach it?

The requirements for direct access are fairly simple:
* Is this local (as opposed to a driver domain)?
* Is this a raw format (as opposed to cooked)?
* Does this have no scripts associated with it?

If it meets all those requirements, we can access it directly;
otherwise we need to attach it.

This fixes a bug where bootloader execution fails for disks with
hotplug scripts.

This should fix a theoretical bug when using a qdisk backend in a
driver domain. (Not tested.)

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxl/libxl.c | 89 +++++++++++++++++------------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d86ea62..8e795e3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3073,45 +3073,8 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
 
     switch (disk->backend) {
         case LIBXL_DISK_BACKEND_PHY:
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
-                       disk->pdev_path);
-            dev = disk->pdev_path;
-            break;
         case LIBXL_DISK_BACKEND_TAP:
-            switch (disk->format) {
-            case LIBXL_DISK_FORMAT_RAW:
-                /* optimise away the early tapdisk attach in this case */
-                LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
-                           " tap disk %s directly (ie without using blktap)",
-                           disk->pdev_path);
-                dev = disk->pdev_path;
-                break;
-            case LIBXL_DISK_FORMAT_VHD:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path,
-                                            disk->format);
-                break;
-            case LIBXL_DISK_FORMAT_QCOW:
-            case LIBXL_DISK_FORMAT_QCOW2:
-                abort(); /* prevented by libxl__device_disk_set_backend */
-            default:
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                           "unrecognized disk format: %d", disk->format);
-                rc = ERROR_FAIL;
-                goto out;
-            }
-            break;
         case LIBXL_DISK_BACKEND_QDISK:
-            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                libxl__prepare_ao_device(ao, &dls->aodev);
-                dls->aodev.callback = local_device_attach_cb;
-                device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk,
-                                &dls->aodev, libxl__alloc_vdev,
-                                (void *) blkdev_start);
-                return;
-            } else {
-                dev = disk->pdev_path;
-            }
-            LOG(DEBUG, "locally attaching qdisk %s", dev);
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
@@ -3120,6 +3083,21 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
             goto out;
     }
 
+    /* If this is in a driver domain, or it's not a raw format, or it involves
+     * running a script, we have to do a local attach. */
+    if (disk->backend_domname != NULL
+        || disk->format != LIBXL_DISK_FORMAT_RAW
+        || disk->script != NULL) {
+        libxl__prepare_ao_device(ao, &dls->aodev);
+        dls->aodev.callback = local_device_attach_cb;
+        device_disk_add(egc, LIBXL_TOOLSTACK_DOMID, disk, &dls->aodev,
+                        libxl__alloc_vdev, (void *) blkdev_start);
+        return;
+    }
+
+    LOG(DEBUG, "locally attaching RAW disk %s", disk->pdev_path);
+    dev = disk->pdev_path;
+
     if (dev != NULL)
         dls->diskpath = libxl__strdup(gc, dev);
 
@@ -3152,7 +3130,7 @@ static void local_device_attach_cb(libxl__egc *egc, libxl__ao_device *aodev)
     }
 
     dev = GCSPRINTF("/dev/%s", disk->vdev);
-    LOG(DEBUG, "locally attaching qdisk %s", dev);
+    LOG(DEBUG, "locally attaching disk %s", dev);
 
     rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
     if (rc < 0)
@@ -3191,29 +3169,18 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
 
     if (!dls->diskpath) goto out;
 
-    switch (disk->backend) {
-        case LIBXL_DISK_BACKEND_QDISK:
-            if (disk->vdev != NULL) {
-                GCNEW(device);
-                rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
-                                             disk, device);
-                if (rc != 0) goto out;
-
-                aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
-                aodev->dev = device;
-                aodev->callback = local_device_detach_cb;
-                aodev->force = 0;
-                libxl__initiate_device_remove(egc, aodev);
-                return;
-            }
-            /* disk->vdev == NULL; fall through */
-        default:
-            /*
-             * Nothing to do for PHYSTYPE_PHY.
-             * For other device types assume that the blktap2 process is
-             * needed by the soon to be started domain and do nothing.
-             */
-            goto out;
+    if (disk->vdev != NULL) {
+        GCNEW(device);
+        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
+                                     disk, device);
+        if (rc != 0) goto out;
+        
+        aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
+        aodev->dev = device;
+        aodev->callback = local_device_detach_cb;
+        aodev->force = 0;
+        libxl__initiate_device_remove(egc, aodev);
+        return;
     }
 
 out:
-- 
1.9.1


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

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