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

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

[Roger Pau Monne]

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;
+            }
             break;
         case LIBXL_DISK_BACKEND_TAP:
             switch (disk->format) {
@@ -3142,7 +3149,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)
@@ -3183,6 +3190,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
 
     switch (disk->backend) {
         case LIBXL_DISK_BACKEND_QDISK:
+        case LIBXL_DISK_BACKEND_PHY:
             if (disk->vdev != NULL) {
                 GCNEW(device);
                 rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
@@ -3199,7 +3207,6 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
             /* 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.
              */
-- 
1.9.5 (Apple Git-50.3)


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

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

[George Dunlap]

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>

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Tested-by: George Dunlap <george.dunlap@eu.citrix.com>

> 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;
> +            }
>              break;
>          case LIBXL_DISK_BACKEND_TAP:
>              switch (disk->format) {
> @@ -3142,7 +3149,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)
> @@ -3183,6 +3190,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>  
>      switch (disk->backend) {
>          case LIBXL_DISK_BACKEND_QDISK:
> +        case LIBXL_DISK_BACKEND_PHY:
>              if (disk->vdev != NULL) {
>                  GCNEW(device);
>                  rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
> @@ -3199,7 +3207,6 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>              /* 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.
>               */
> 


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

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

[George Dunlap]

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

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

[Roger Pau Monné]

El 25/06/15 a les 19.22, George Dunlap ha escrit:
> 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.)
> 

The logic in the patch looks fine to me, if you can assert that blktap
is also capable of local-attach.

> 
> 
> 0001-libxl-Make-local_initiate_attach-more-rational.patch
> 
> 
>>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 "

Do we need to keep the whole switch statement just for the default
(error) case? Wouldn't it be better to just replace it with an if condition?

Roger.

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

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

[Ian Campbell]

On Thu, 2015-06-25 at 16:17 +0200, 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;

Perhaps make this and the qdisk stuff common via a new helper?

> @@ -3142,7 +3149,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)
> @@ -3183,6 +3190,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>  
>      switch (disk->backend) {
>          case LIBXL_DISK_BACKEND_QDISK:
> +        case LIBXL_DISK_BACKEND_PHY:

What ensures that things which get here were lcoally attached rather
than just plain block devices? Is disk->vdev always NULL in the latter
case, or is there something further up?

>              if (disk->vdev != NULL) {
>                  GCNEW(device);
>                  rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
> @@ -3199,7 +3207,6 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>              /* 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.
>               */



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

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

[Ian Campbell]

On Thu, 2015-06-25 at 18:22 +0100, George Dunlap wrote:
> 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?

As an alternative (rather than an incremental) to Roger's, right?

I didn't have any comments (i.e. LGTM) but I was hoping for an ack or
R-by/T-by from Roger.

Ian.

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

[Roger Pau Monné]

El 30/06/15 a les 14.31, Ian Campbell ha escrit:
> On Thu, 2015-06-25 at 16:17 +0200, 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;
> 
> Perhaps make this and the qdisk stuff common via a new helper?

I think this is already solved in the new patch posted by George.

>> @@ -3142,7 +3149,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)
>> @@ -3183,6 +3190,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>>  
>>      switch (disk->backend) {
>>          case LIBXL_DISK_BACKEND_QDISK:
>> +        case LIBXL_DISK_BACKEND_PHY:
> 
> What ensures that things which get here were lcoally attached rather
> than just plain block devices? Is disk->vdev always NULL in the latter
> case, or is there something further up?

Yes, disk->vdev is always NULL if the disk hasn't been locally attached.

Roger.


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

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

[George Dunlap]

On 06/30/2015 01:35 PM, Ian Campbell wrote:
> On Thu, 2015-06-25 at 18:22 +0100, George Dunlap wrote:
>> 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?
> 
> As an alternative (rather than an incremental) to Roger's, right?
> 
> I didn't have any comments (i.e. LGTM) but I was hoping for an ack or
> R-by/T-by from Roger.

I think he was waiting for me to confirm that it would work with blktap,
which I haven't had time to test yet.

Or rather, I've tested it with my new
use-blktap-scripts-rather-than-the-library patches, but we want to avoid
regressions if we can.

 -George