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

["Roger Pau Monné" <roger.pau@citrix.com>]

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