[PATCH v8 0/11] qdisk local attach

[PATCH v8 0/11] qdisk local attach

[Stefano Stabellini]

Hi all,
this patch implements local_attach support for QDISK: that means that
you can use qcow2 as disk format for your PV guests disks and use
pygrub to retrieve the kernel and initrd in dom0.

The idea is that we start a QEMU instance at boot time to listen to
local_attach requests. When libxl_device_disk_local_attach is called on
a QDISK images, libxl sets up a backend/frontend pair in dom0 for the disk
so that blkfront in dom0 will create a new xvd device for it.
Then pygrub can be pointed at this device to retrieve kernel and
initrd.


Changes in v8:
- keep libxl__device_disk_local_attach/detach in libxl.c;
- free pdev_path and script in local_detach;
- use libxl__strdup instead of strdup.


Changes in v7:
- rename tmpdisk to localdisk;
- add a comment in libxl__bootloader_state about localdisk;
- keep libxl__device_from_disk in libxl.c;
- implement libxl__device_disk_add in libxl.c;
- remove the upper bound in libxl__devid_to_localdev and document why.


Changes in v6:
- rebase on "nstore: rename public xenstore headers";
- new patch: "libxl_string_to_backend: add qdisk";
- new patch: "main_blockdetach: destroy the disk on successful removal";
- split "libxl: introduce libxl__device_disk_add" in two patches;
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs;
- document blkdev_start;
- use libxl__strdup to allocate blkdev_start; 
- more comments in libxl__devid_to_localdev;
- inline GCSPRINTF;
- introduce ret for xs_* error codes.


Changes in v5:
- remove _hidden from the libxl_device_disk_local_attach/detach
  implementation;
- libxl__device_disk_local_attach: rename disk to in_disk;
- libxl__device_disk_local_attach: rename tmpdisk to disk;
- libxl__device_disk_local_attach: copy disk to new_disk only on success;
- libxl__device_disk_local_attach: remove check on libxl__zalloc success.
- rename libxl__device_generic_add_t to libxl__device_generic_add;
- change the type of the blkdev_start parameter to
  libxl__device_disk_local_attach to const char *;
- remove domid paramter to libxl__alloc_vdev (assume
  LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev;
- libxl__device_disk_local_attach: replace LIBXL__LOG with LOG;
- libxl__device_disk_local_attach: unify error paths.


Changes in v4:
- split the first patch into 2 patches: the first is a simple move, the
  second one adds a new parameter;
- libxl__device_generic_add: do not end the transaction is the caller
  provided it;
- libxl__device_generic_add: fix success exit path;
- pass blkdev_start in libxl_domain_build_info;
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- improve error handling and exit paths in libxl__alloc_vdev and
  libxl__device_disk_local_attach.


Changes in v3:
- libxl__device_disk_local_attach: wait for the backend to be
"connected" before returning.


Changes in v2:
- make libxl_device_disk_local_(de)attach internal functions;
- allocate the new disk in libxl_device_disk_local_attatch on the GC;
- remove libxl__device_generic_add_t, add a transaction parameter to
libxl__device_generic_add instead;
- add a new patch to introduce a blkdev_start option to xl.conf;
- reimplement libxl__alloc_vdev checking for the frontend path and
starting from blkdev_start;
- introduce a Linux specific libxl__devid_to_vdev function based on last
Jan's patch to blkfront.


Stefano Stabellini (11):
      libxl: make libxl_device_disk_local_attach/detach internal functions
      libxl: libxl__device_disk_local_attach return a new libxl_device_disk
      libxl: add a transaction parameter to libxl__device_generic_add
      libxl: export libxl__device_from_disk
      libxl: introduce libxl__device_disk_add
      xl/libxl: add a blkdev_start parameter
      libxl: introduce libxl__alloc_vdev
      xl/libxl: implement QDISK libxl_device_disk_local_attach
      libxl__device_disk_local_attach: wait for state "connected"
      libxl_string_to_backend: add qdisk
      main_blockdetach: destroy the disk on successful removal
 
 docs/man/xl.conf.pod.5                          |    6 +
 tools/examples/xl.conf                          |    3 +
 tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
 tools/hotplug/Linux/init.d/xencommons           |    3 +
 tools/libxl/libxl.c                             |  163 ++++++++++++++++++-----
 tools/libxl/libxl.h                             |    7 -
 tools/libxl/libxl_bootloader.c                  |    5 +-
 tools/libxl/libxl_create.c                      |    3 +
 tools/libxl/libxl_device.c                      |   17 ++-
 tools/libxl/libxl_internal.h                    |   32 ++++-
 tools/libxl/libxl_linux.c                       |   52 +++++++
 tools/libxl/libxl_netbsd.c                      |    6 +
 tools/libxl/libxl_pci.c                         |    2 +-
 tools/libxl/libxl_types.idl                     |    1 +
 tools/libxl/libxl_utils.c                       |    2 +
 tools/libxl/xl.c                                |    3 +
 tools/libxl/xl.h                                |    1 +
 tools/libxl/xl_cmdimpl.c                        |    5 +-
 18 files changed, 264 insertions(+), 50 deletions(-)


Cheers,

Stefano

[PATCH v8 01/11] libxl: make libxl_device_disk_local_attach/detach internal functions

[Stefano Stabellini]

Changes in v8:
- keep libxl__device_disk_local_attach/detach in libxl.c.

Changes in v5:

- remove _hidden from the implementation.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c            |   11 +++--------
 tools/libxl/libxl.h            |    7 -------
 tools/libxl/libxl_bootloader.c |    4 ++--
 tools/libxl/libxl_internal.h   |    9 +++++++++
 4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e3d05c2..cd870c4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1735,9 +1735,9 @@ out:
     return ret;
 }
 
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
 {
-    GC_INIT(ctx);
+    libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     char *ret = NULL;
     int rc;
@@ -1792,11 +1792,10 @@ char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
  out:
     if (dev != NULL)
         ret = strdup(dev);
-    GC_FREE;
     return ret;
 }
 
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
+int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
 {
     /* Nothing to do for PHYSTYPE_PHY. */
 
@@ -1804,10 +1803,6 @@ int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
      * For other device types assume that the blktap2 process is
      * needed by the soon to be started domain and do nothing.
      */
-    /*
-     * FIXME
-     * This appears to leak the disk in failure paths
-     */
 
     return 0;
 }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 316d290..21e6510 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -686,13 +686,6 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
  */
 int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
 
-/*
- * Make a disk available in this (the control) domain. Returns path to
- * a device.
- */
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
-
 /* Network Interfaces */
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
 int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index f3a590b..e8950b1 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
     if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
 
     if (bl->diskpath) {
-        libxl_device_disk_local_detach(CTX, bl->disk);
+        libxl__device_disk_local_detach(gc, bl->disk);
         free(bl->diskpath);
         bl->diskpath = 0;
     }
@@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
         goto out;
     }
 
-    bl->diskpath = libxl_device_disk_local_attach(CTX, bl->disk);
+    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
     if (!bl->diskpath) {
         rc = ERROR_FAIL;
         goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 52f5435..d34e561 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1260,6 +1260,15 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
  */
 _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
 
+/*
+ * Make a disk available in this (the control) domain. Returns path to
+ * a device.
+ */
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+        libxl_device_disk *disk);
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
+        libxl_device_disk *disk);
+
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 
 struct libxl__xen_console_reader {
-- 
1.7.2.5

[PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

Introduce a new libxl_device_disk* parameter to
libxl__device_disk_local_attach, the parameter is allocated by the
caller. libxl__device_disk_local_attach is going to fill the new disk
with informations about the new locally attached disk.  The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.

Changes in v8:
- free pdev_path and script in local_detach;
- use libxl__strdup instead of strdup.

Changes in v7:
- rename tmpdisk to localdisk;
- add a comment in libxl__bootloader_state about localdisk.

Changes in v6:
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs.

Changes in v5:
- rename disk to in_disk;
- rename tmpdisk to disk;
- copy disk to new_disk only on success;
- remove check on libxl__zalloc success.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c            |   18 +++++++++++++++---
 tools/libxl/libxl_bootloader.c |    4 ++--
 tools/libxl/libxl_internal.h   |   10 +++++++++-
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd870c4..762e72a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1735,13 +1735,24 @@ out:
     return ret;
 }
 
-char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc,
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *disk)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     char *ret = NULL;
     int rc;
 
+    if (in_disk->pdev_path == NULL)
+        return NULL;
+
+    memcpy(disk, in_disk, sizeof(libxl_device_disk));
+    disk->pdev_path = libxl__strdup(NULL, in_disk->pdev_path);
+    if (in_disk->script != NULL)
+        disk->script = libxl__strdup(NULL, in_disk->script);
+    disk->vdev = NULL;
+
     rc = libxl__device_disk_setdefault(gc, disk);
     if (rc) goto out;
 
@@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
                            " attach a qdisk image if the format is not raw");
                 break;
             }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
-                       disk->pdev_path);
+            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
             dev = disk->pdev_path;
             break;
         default:
@@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
      * needed by the soon to be started domain and do nothing.
      */
 
+    free(disk->pdev_path);
+    free(disk->script);
     return 0;
 }
 
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index e8950b1..82371f1 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
     if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
 
     if (bl->diskpath) {
-        libxl__device_disk_local_detach(gc, bl->disk);
+        libxl__device_disk_local_detach(gc, &bl->localdisk);
         free(bl->diskpath);
         bl->diskpath = 0;
     }
@@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
         goto out;
     }
 
-    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
+    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk);
     if (!bl->diskpath) {
         rc = ERROR_FAIL;
         goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d34e561..21b8b54 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1265,7 +1265,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
  * a device.
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
-        libxl_device_disk *disk);
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *new_disk);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
@@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
     libxl__bootloader_console_callback *console_available;
     libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
     libxl_device_disk *disk;
+    /* Should be zeroed by caller on entry.  Will be filled in by
+     * bootloader machinery; represents the local attachment of the
+     * disk for the benefit of the bootloader.  Must be detached by
+     * the caller using libxl__device_disk_local_detach, but only
+     * after the domain's kernel and initramfs have been loaded into
+     * memory and the file references disposed of. */
+    libxl_device_disk localdisk;
     uint32_t domid;
     /* private to libxl__run_bootloader */
     char *outputpath, *outputdir, *logfile;
-- 
1.7.2.5

[PATCH v8 03/11] libxl: add a transaction parameter to libxl__device_generic_add

[Stefano Stabellini]

Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
XBT_NULL, allocate a proper one.

Update all the callers.

This patch contains a large number of unchecked xenstore operations, we
might want to fix this in the future.

Changes in v4:
- libxl__device_generic_add: do not end the transaction is the caller
provided it;
- libxl__device_generic_add: fix success exit path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |   10 +++++-----
 tools/libxl/libxl_device.c   |   17 +++++++++++------
 tools/libxl/libxl_internal.h |    4 ++--
 tools/libxl/libxl_pci.c      |    2 +-
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 762e72a..987086d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1472,7 +1472,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
     flexarray_append(front, "device-type");
     flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
 
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
 
@@ -1957,7 +1957,7 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
     flexarray_append(front, "mac");
     flexarray_append(front, libxl__sprintf(gc,
                                     LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
 
@@ -2250,7 +2250,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
     }
 
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
@@ -2321,7 +2321,7 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb)
     flexarray_append(front, "state");
     flexarray_append(front, libxl__sprintf(gc, "%d", 1));
 
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
@@ -2454,7 +2454,7 @@ int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb)
                           libxl__sprintf(gc, "%d", vfb->backend_domid));
     flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
 
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
     rc = 0;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2006406..3da60e1 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
     return libxl__device_kind_from_string(strkind, &dev->backend_kind);
 }
 
-int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
-                             char **bents, char **fents)
+int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+        libxl__device *device, char **bents, char **fents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *frontend_path, *backend_path;
-    xs_transaction_t t;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions backend_perms[2];
+    int create_transaction = t == XBT_NULL;
 
     frontend_path = libxl__device_frontend_path(gc, device);
     backend_path = libxl__device_backend_path(gc, device);
@@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
     backend_perms[1].perms = XS_PERM_READ;
 
 retry_transaction:
-    t = xs_transaction_start(ctx->xsh);
+    if (create_transaction)
+        t = xs_transaction_start(ctx->xsh);
     /* FIXME: read frontend_path and check state before removing stuff */
 
     if (fents) {
@@ -100,13 +101,17 @@ retry_transaction:
         libxl__xs_writev(gc, t, backend_path, bents);
     }
 
+    if (!create_transaction)
+        return 0;
+
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
-        else
+        else {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
+            return ERROR_FAIL;
+        }
     }
-
     return 0;
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 21b8b54..03fba22 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -792,8 +792,8 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                                       libxl__device_console *console,
                                       libxl__domain_build_state *state);
 
-_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
-                             char **bents, char **fents);
+_hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+        libxl__device *device, char **bents, char **fents);
 _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
 _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
 _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e980995..92764fc 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -103,7 +103,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_append_pair(front, "backend-id", libxl__sprintf(gc, "%d", 0));
     flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
 
-    libxl__device_generic_add(gc, &device,
+    libxl__device_generic_add(gc, XBT_NULL, &device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count));
 
-- 
1.7.2.5

[PATCH v8 04/11] libxl: export libxl__device_from_disk

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_internal.h |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 987086d..eb1ad4f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1327,7 +1327,7 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
     return rc;
 }
 
-static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
                                    libxl__device *device)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 03fba22..32eb09c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1260,6 +1260,10 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
  */
 _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
 
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+                                   libxl_device_disk *disk,
+                                   libxl__device *device);
+
 /*
  * Make a disk available in this (the control) domain. Returns path to
  * a device.
-- 
1.7.2.5

[PATCH v8 05/11] libxl: introduce libxl__device_disk_add

[Stefano Stabellini]

Introduce libxl__device_disk_add that takes an additional
xs_transaction_t paramter.
Implement libxl_device_disk_add using libxl__device_disk_add.


Changes in v7:

- implement libxl__device_disk_add in libxl.c.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |   14 +++++++++++---
 tools/libxl/libxl_internal.h |    2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index eb1ad4f..8d128a6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1367,14 +1367,15 @@ int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
-int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
+int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
+        xs_transaction_t t, libxl_device_disk *disk)
 {
-    GC_INIT(ctx);
     flexarray_t *front;
     flexarray_t *back;
     char *dev;
     libxl__device device;
     int major, minor, rc;
+    libxl_ctx *ctx = gc->owner;
 
     rc = libxl__device_disk_setdefault(gc, disk);
     if (rc) goto out;
@@ -1472,7 +1473,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
     flexarray_append(front, "device-type");
     flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
 
-    libxl__device_generic_add(gc, XBT_NULL, &device,
+    libxl__device_generic_add(gc, t, &device,
                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(gc, front, front->count));
 
@@ -1482,6 +1483,13 @@ out_free:
     flexarray_free(back);
     flexarray_free(front);
 out:
+    return rc;
+}
+
+int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
+{
+    GC_INIT(ctx);
+    int rc = libxl__device_disk_add(gc, domid, XBT_NULL, disk);
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 32eb09c..b917553 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1263,6 +1263,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
 _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
                                    libxl__device *device);
+_hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
+        xs_transaction_t t, libxl_device_disk *disk);
 
 /*
  * Make a disk available in this (the control) domain. Returns path to
-- 
1.7.2.5

[PATCH v8 06/11] xl/libxl: add a blkdev_start parameter

[Stefano Stabellini]

Introduce a blkdev_start in xl.conf and a corresponding string in
libxl_domain_build_info.

Add a blkdev_start parameter to libxl__device_disk_local_attach: it is
going to be used in a following patch.

blkdev_start specifies the first block device to be used for temporary
block device allocations by the toolstack.

Changes in v6:
- document blkdev_start;
- use libxl__strdup to allocate blkdev_start.

Changes in v5:
- change the type of the blkdev_start parameter to
libxl__device_disk_local_attach to const char *.

Changes in v4:
- pass blkdev_start in libxl_domain_build_info.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 docs/man/xl.conf.pod.5         |    6 ++++++
 tools/examples/xl.conf         |    3 +++
 tools/libxl/libxl.c            |    3 ++-
 tools/libxl/libxl_bootloader.c |    3 ++-
 tools/libxl/libxl_create.c     |    3 +++
 tools/libxl/libxl_internal.h   |    3 ++-
 tools/libxl/libxl_types.idl    |    1 +
 tools/libxl/xl.c               |    3 +++
 tools/libxl/xl.h               |    1 +
 tools/libxl/xl_cmdimpl.c       |    2 ++
 10 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..149430c 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -84,6 +84,12 @@ previous C<xm> toolstack this can be configured to use the old C<SXP>
 
 Default: C<json>
 
+=item B<blkdev_start="NAME">
+
+Configures the name of the first block device to be used for temporary
+block device allocations by the toolstack.
+The default choice is "xvda".
+
 =back
 
 =head1 SEE ALSO
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 56d3b3b..ebf057c 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,6 @@
 
 # default output format used by "xl list -l"
 #output_format="json"
+
+# first block device to be used for temporary VM disk mounts
+#blkdev_start="xvda"
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 8d128a6..1a01b24 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1745,7 +1745,8 @@ out:
 
 char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
-        libxl_device_disk *disk)
+        libxl_device_disk *disk,
+        const char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 82371f1..4e1100d 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -330,7 +330,8 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
         goto out;
     }
 
-    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk);
+    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk,
+            info->blkdev_start);
     if (!bl->diskpath) {
         rc = ERROR_FAIL;
         goto out;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 14721eb..9fb4b81 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         }
     }
 
+    if (b_info->blkdev_start == NULL)
+        b_info->blkdev_start = libxl__strdup(0, "xvda");
+
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!b_info->u.hvm.bios)
             switch (b_info->device_model_version) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b917553..4be99ca 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1272,7 +1272,8 @@ _hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
-        libxl_device_disk *new_disk);
+        libxl_device_disk *new_disk,
+        const char *blkdev_start);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a21bd85..2dc2b68 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -253,6 +253,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("localtime",       libxl_defbool),
     ("disable_migrate", libxl_defbool),
     ("cpuid",           libxl_cpuid_policy_list),
+    ("blkdev_start",    string),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 750a61c..827f3f8 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -38,6 +38,7 @@ xentoollog_logger_stdiostream *logger;
 int dryrun_only;
 int force_execution;
 int autoballoon = 1;
+char *blkdev_start;
 char *lockfile;
 char *default_vifscript = NULL;
 char *default_bridge = NULL;
@@ -94,6 +95,8 @@ static void parse_global_config(const char *configfile,
             fprintf(stderr, "invalid default output format \"%s\"\n", buf);
         }
     }
+    if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
+        blkdev_start = strdup(buf);
     xlu_cfg_destroy(config);
 }
 
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b7eacaa..74a8a1a 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -117,6 +117,7 @@ extern int dryrun_only;
 extern char *lockfile;
 extern char *default_vifscript;
 extern char *default_bridge;
+extern char *blkdev_start;
 
 enum output_format {
     OUTPUT_FORMAT_JSON,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c55a69..ce9237b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -626,6 +626,8 @@ static void parse_config_data(const char *config_source,
     }
 
     libxl_domain_build_info_init_type(b_info, c_info->type);
+    if (blkdev_start)
+        b_info->blkdev_start = strdup(blkdev_start);
 
     /* the following is the actual config parsing with overriding values in the structures */
     if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
-- 
1.7.2.5

[PATCH v8 07/11] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini]

Introduce libxl__alloc_vdev: find a spare virtual block device in the
domain passed as argument.


Changes in v7:
- remove the upper bound and document why.

Changes in v6:
- more comments in libxl__devid_to_localdev;
- inline GCSPRINTF.

Changes in v5:
- remove domid paramter to libxl__alloc_vdev (assume
  LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev.

Changes in v4:
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- better error handling;

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |   32 +++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    4 +++
 tools/libxl/libxl_linux.c    |   52 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_netbsd.c   |    6 +++++
 4 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1a01b24..3cf0d53 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1743,6 +1743,38 @@ out:
     return ret;
 }
 
+/* libxl__alloc_vdev only works on the local domain, that is the domain
+ * where the toolstack is running */
+static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
+        xs_transaction_t t)
+{
+    int devid = 0, disk = 0, part = 0;
+    char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
+
+    libxl__device_disk_dev_number(blkdev_start, &disk, &part);
+    if (part != 0) {
+        LOG(ERROR, "blkdev_start is invalid");
+        return NULL;
+    }
+
+    do {
+        devid = libxl__device_disk_dev_number(GCSPRINTF("d%dp0", disk),
+                NULL, NULL);
+        if (devid < 0)
+            return NULL;
+        if (libxl__xs_read(gc, t,
+                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+                        dompath, devid)) == NULL) {
+            if (errno == ENOENT)
+                return libxl__devid_to_localdev(gc, devid);
+            else
+                return NULL;
+        }
+        disk++;
+    } while (1);
+    return NULL;
+}
+
 char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
         libxl_device_disk *disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4be99ca..337ce28 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -78,6 +78,8 @@
 #define LIBXL_PV_EXTRA_MEMORY 1024
 #define LIBXL_HVM_EXTRA_MEMORY 2048
 #define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
 #define QEMU_SIGNATURE "DeviceModelRecord0002"
 #define STUBDOM_CONSOLE_LOGGING 0
 #define STUBDOM_CONSOLE_SAVE 1
@@ -915,6 +917,8 @@ static inline void libxl__domaindeathcheck_stop(libxl__gc *gc,
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
 
+_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..0169b2f 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,55 @@ int libxl__try_phy_backend(mode_t st_mode)
 
     return 1;
 }
+
+#define EXT_SHIFT 28
+#define EXTENDED (1<<EXT_SHIFT)
+#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
+#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
+/* the size of the buffer to store the device name is 32 bytes to match the
+ * equivalent buffer in the Linux kernel code */
+#define BUFFER_SIZE 32
+
+/* Same as in Linux.
+ * encode_disk_name might end up using up to 29 bytes (BUFFER_SIZE - 3)
+ * including the trailing \0.
+ *
+ * The code is safe because 26 raised to the power of 28 (that is the
+ * maximum offset that can be stored in the allocated buffer as a
+ * string) is far greater than UINT_MAX on 64 bits so offset cannot be
+ * big enough to exhaust the available bytes in ret. */
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+    if (n >= 26)
+        ptr = encode_disk_name(ptr, n / 26 - 1);
+    *ptr = 'a' + n % 26;
+    return ptr + 1;
+}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+    unsigned int minor;
+    int offset;
+    int nr_parts;
+    char *ptr = NULL;
+    char *ret = libxl__zalloc(gc, BUFFER_SIZE);
+
+    if (!VDEV_IS_EXTENDED(devid)) {
+        minor = devid & 0xff;
+        nr_parts = 16;
+    } else {
+        minor = BLKIF_MINOR_EXT(devid);
+        nr_parts = 256;
+    }
+    offset = minor / nr_parts;
+
+    strcpy(ret, "xvd");
+    ptr = encode_disk_name(ret + 3, offset);
+    if (minor % nr_parts == 0)
+        *ptr = 0;
+    else
+        /* overflow cannot happen, thanks to the upper bound */
+        snprintf(ptr, ret + 32 - ptr,
+                "%d", minor & (nr_parts - 1));
+    return ret;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
 
     return 0;
 }
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+    /* TODO */
+    return NULL;
+}
-- 
1.7.2.5

[PATCH v8 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Stefano Stabellini]

- Spawn a QEMU instance at boot time to handle disk local attach
requests.

- Implement libxl_device_disk_local_attach for QDISKs in terms of a
regular disk attach with the frontend and backend running in the same
domain.

Changes in v6:
- introduce xs_ret for xs_* error codes.

Changes in v5:
- replace LIBXL__LOG with LOG.

Changes on v4:
- improve error handling and exit paths in libxl__device_disk_local_attach.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by:Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
 tools/hotplug/Linux/init.d/xencommons           |    3 +
 tools/libxl/libxl.c                             |   63 ++++++++++++++++++----
 3 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons
index 6543204..38ea85a 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons
@@ -12,3 +12,6 @@
 
 # Running xenbackendd in debug mode
 #XENBACKENDD_DEBUG=[yes|on|1]
+
+# qemu path
+#QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
index 2f81ea2..9dda6e2 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -104,6 +104,9 @@ do_start () {
 	xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
 	test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
 	test "`uname`" != "NetBSD" || xenbackendd $XENBACKENDD_ARGS
+	echo Starting QEMU as disk backend for dom0
+	test -z "$QEMU_XEN" && QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
+	$QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize -monitor /dev/null
 }
 do_stop () {
         echo Stopping xenconsoled
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3cf0d53..223ae40 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1783,7 +1783,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     char *ret = NULL;
-    int rc;
+    int rc, xs_ret;
+    xs_transaction_t t = XBT_NULL;
 
     if (in_disk->pdev_path == NULL)
         return NULL;
@@ -1827,12 +1828,34 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
-                           " attach a qdisk image if the format is not raw");
-                break;
+                do {
+                    t = xs_transaction_start(ctx->xsh);
+                    if (t == XBT_NULL) {
+                        LOG(ERROR, "failed to start a xenstore transaction");
+                        goto out;
+                    }
+                    disk->vdev = libxl__alloc_vdev(gc, blkdev_start, t);
+                    if (disk->vdev == NULL) {
+                        LOG(ERROR, "libxl__alloc_vdev failed");
+                        goto out;
+                    }
+                    if (libxl__device_disk_add(gc, LIBXL_TOOLSTACK_DOMID,
+                                t, disk)) {
+                        LOG(ERROR, "libxl_device_disk_add failed");
+                        goto out;
+                    }
+                    xs_ret = xs_transaction_end(ctx->xsh, t, 0);
+                } while (xs_ret == 0 && errno == EAGAIN);
+                t = XBT_NULL;
+                if (xs_ret == 0) {
+                    LOGE(ERROR, "xenstore transaction failed");
+                    goto out;
+                }
+                dev = GCSPRINTF("/dev/%s", disk->vdev);
+            } else {
+                dev = disk->pdev_path;
             }
-            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
-            dev = disk->pdev_path;
+            LOG(DEBUG, "locally attaching qdisk %s", dev);
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
@@ -1841,6 +1864,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
     }
 
  out:
+    if (t != XBT_NULL)
+        xs_transaction_end(ctx->xsh, t, 1);
     if (dev != NULL)
         ret = strdup(dev);
     return ret;
@@ -1848,16 +1873,30 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
 
 int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
 {
-    /* Nothing to do for PHYSTYPE_PHY. */
+    int rc = 0;
 
-    /*
-     * For other device types assume that the blktap2 process is
-     * needed by the soon to be started domain and do nothing.
-     */
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (disk->vdev != NULL) {
+                libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
+                        disk, 0);
+                rc = libxl_device_disk_destroy(gc->owner,
+                        LIBXL_TOOLSTACK_DOMID, disk);
+            }
+            break;
+        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.
+             */
+            break;
+    }
 
     free(disk->pdev_path);
     free(disk->script);
-    return 0;
+
+    return rc;
 }
 
 /******************************************************************************/
-- 
1.7.2.5

[PATCH v8 09/11] libxl__device_disk_local_attach: wait for state "connected"

[Stefano Stabellini]

In order to make sure that the block device is available and ready to be
used, wait for state "connected" in the backend before returning.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Changes in v5:
- unify error paths.

Changes in v4:
- improve exit paths.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 223ae40..626abde 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1781,9 +1781,10 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
         const char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
-    char *dev = NULL;
+    char *dev = NULL, *be_path = NULL;
     char *ret = NULL;
     int rc, xs_ret;
+    libxl__device device;
     xs_transaction_t t = XBT_NULL;
 
     if (in_disk->pdev_path == NULL)
@@ -1863,12 +1864,25 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
             break;
     }
 
- out:
-    if (t != XBT_NULL)
-        xs_transaction_end(ctx->xsh, t, 1);
+    if (disk->vdev != NULL) {
+        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
+        if (rc < 0)
+            goto out;
+        be_path = libxl__device_backend_path(gc, &device);
+        rc = libxl__wait_for_backend(gc, be_path, "4");
+        if (rc < 0)
+            goto out;
+    }
     if (dev != NULL)
         ret = strdup(dev);
     return ret;
+
+ out:
+    if (t != XBT_NULL)
+        xs_transaction_end(ctx->xsh, t, 1);
+    else
+        libxl__device_disk_local_detach(gc, disk);
+    return NULL;
 }
 
 int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
-- 
1.7.2.5

[PATCH v8 10/11] libxl_string_to_backend: add qdisk

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_utils.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 73b00b3..4cf403d 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -240,6 +240,8 @@ int libxl_string_to_backend(libxl_ctx *ctx, char *s, libxl_disk_backend *backend
         *backend = LIBXL_DISK_BACKEND_PHY;
     } else if (!strcmp(s, "file")) {
         *backend = LIBXL_DISK_BACKEND_TAP;
+    } else if (!strcmp(s, "qdisk")) {
+        *backend = LIBXL_DISK_BACKEND_QDISK;
     } else if (!strcmp(s, "tap")) {
         p = strchr(s, ':');
         if (!p) {
-- 
1.7.2.5

[PATCH v8 11/11] main_blockdetach: destroy the disk on successful removal

[Stefano Stabellini]

main_blockdetach needs to call libxl_device_disk_destroy so that all the
disk related entries are properly removed from xenstore.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ce9237b..1167ed0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5396,7 +5396,8 @@ int main_blockdetach(int argc, char **argv)
     }
     if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
         fprintf(stderr, "libxl_device_disk_remove failed.\n");
-    }
+    } else
+        libxl_device_disk_destroy(ctx, domid, &disk);
     return 0;
 }
 
-- 
1.7.2.5

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell]

On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> Introduce a new libxl_device_disk* parameter to
> libxl__device_disk_local_attach, the parameter is allocated by the
> caller. libxl__device_disk_local_attach is going to fill the new disk
> with informations about the new locally attached disk.  The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
> 
> Changes in v8:
> - free pdev_path and script in local_detach;
> - use libxl__strdup instead of strdup.
> 
> Changes in v7:
> - rename tmpdisk to localdisk;
> - add a comment in libxl__bootloader_state about localdisk.
> 
> Changes in v6:
> - return error in case pdev_path is NULL;
> - libxl__device_disk_local_attach update the new disk, the caller
> allocates it;
> - remove \n from logs.
> 
> Changes in v5:
> - rename disk to in_disk;
> - rename tmpdisk to disk;
> - copy disk to new_disk only on success;
> - remove check on libxl__zalloc success.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl.c            |   18 +++++++++++++++---
>  tools/libxl/libxl_bootloader.c |    4 ++--
>  tools/libxl/libxl_internal.h   |   10 +++++++++-
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index cd870c4..762e72a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1735,13 +1735,24 @@ out:
>      return ret;
>  }
>  
> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk *disk)

Why the weird indent?

>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
>      char *ret = NULL;
>      int rc;
>  
> +    if (in_disk->pdev_path == NULL)
> +        return NULL;
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    disk->pdev_path = libxl__strdup(NULL, in_disk->pdev_path);
> +    if (in_disk->script != NULL)
> +        disk->script = libxl__strdup(NULL, in_disk->script);
> +    disk->vdev = NULL;
> +
>      rc = libxl__device_disk_setdefault(gc, disk);
>      if (rc) goto out;
>  
> @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
>                             " attach a qdisk image if the format is not raw");
>                  break;
>              }
> -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);
> +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
>              dev = disk->pdev_path;
>              break;
>          default:
> @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
>       * needed by the soon to be started domain and do nothing.
>       */
>  
> +    free(disk->pdev_path);
> +    free(disk->script);

This is open coding libxl_device_disk_dispose(disk) but missed the vdev
member, is that deliberate?

>      return 0;
>  }
>  
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index e8950b1..82371f1 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
>      if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
>  
>      if (bl->diskpath) {
> -        libxl__device_disk_local_detach(gc, bl->disk);
> +        libxl__device_disk_local_detach(gc, &bl->localdisk);
>          free(bl->diskpath);
>          bl->diskpath = 0;
>      }
> @@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
>          goto out;
>      }
>  
> -    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
> +    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk);
>      if (!bl->diskpath) {
>          rc = ERROR_FAIL;
>          goto out;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d34e561..21b8b54 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1265,7 +1265,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
>   * a device.
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> -        libxl_device_disk *disk);
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk *new_disk);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>  
> @@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
>      libxl__bootloader_console_callback *console_available;
>      libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
>      libxl_device_disk *disk;
> +    /* Should be zeroed by caller on entry.  Will be filled in by
> +     * bootloader machinery; represents the local attachment of the
> +     * disk for the benefit of the bootloader.  Must be detached by
> +     * the caller using libxl__device_disk_local_detach, but only
> +     * after the domain's kernel and initramfs have been loaded into
> +     * memory and the file references disposed of. */
> +    libxl_device_disk localdisk;
>      uint32_t domid;
>      /* private to libxl__run_bootloader */
>      char *outputpath, *outputdir, *logfile;

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

On Tue, 29 May 2012, Ian Campbell wrote:
> On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > Introduce a new libxl_device_disk* parameter to
> > libxl__device_disk_local_attach, the parameter is allocated by the
> > caller. libxl__device_disk_local_attach is going to fill the new disk
> > with informations about the new locally attached disk.  The new
> > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > afterwards.
> > 
> > Changes in v8:
> > - free pdev_path and script in local_detach;
> > - use libxl__strdup instead of strdup.
> > 
> > Changes in v7:
> > - rename tmpdisk to localdisk;
> > - add a comment in libxl__bootloader_state about localdisk.
> > 
> > Changes in v6:
> > - return error in case pdev_path is NULL;
> > - libxl__device_disk_local_attach update the new disk, the caller
> > allocates it;
> > - remove \n from logs.
> > 
> > Changes in v5:
> > - rename disk to in_disk;
> > - rename tmpdisk to disk;
> > - copy disk to new_disk only on success;
> > - remove check on libxl__zalloc success.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxl/libxl.c            |   18 +++++++++++++++---
> >  tools/libxl/libxl_bootloader.c |    4 ++--
> >  tools/libxl/libxl_internal.h   |   10 +++++++++-
> >  3 files changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index cd870c4..762e72a 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1735,13 +1735,24 @@ out:
> >      return ret;
> >  }
> >  
> > -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> > +char * libxl__device_disk_local_attach(libxl__gc *gc,
> > +        const libxl_device_disk *in_disk,
> > +        libxl_device_disk *disk)
> 
> Why the weird indent?

Uhm.. I don't know, I think it was just the default on vim.


> >  {
> >      libxl_ctx *ctx = gc->owner;
> >      char *dev = NULL;
> >      char *ret = NULL;
> >      int rc;
> >  
> > +    if (in_disk->pdev_path == NULL)
> > +        return NULL;
> > +
> > +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> > +    disk->pdev_path = libxl__strdup(NULL, in_disk->pdev_path);
> > +    if (in_disk->script != NULL)
> > +        disk->script = libxl__strdup(NULL, in_disk->script);
> > +    disk->vdev = NULL;
> > +
> >      rc = libxl__device_disk_setdefault(gc, disk);
> >      if (rc) goto out;
> >  
> > @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> >                             " attach a qdisk image if the format is not raw");
> >                  break;
> >              }
> > -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > -                       disk->pdev_path);
> > +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
> >              dev = disk->pdev_path;
> >              break;
> >          default:
> > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> >       * needed by the soon to be started domain and do nothing.
> >       */
> >  
> > +    free(disk->pdev_path);
> > +    free(disk->script);
> 
> This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> member, is that deliberate?

I think it is a mistake: all these strings used to be allocated on the
gc, on previous versions of the series. However meanwhile the event
series went in, changing completely libxl__bootloader_run (that is the
caller of libxl__device_disk_local_attach).
Allocating stuff on the gc is not correct anymore, so now they need to
be explicitly freed. I think I should call libxl_device_disk_dispose
here and strdup in libxl__device_disk_local_attach to make sure vdev is
not on the gc.

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

On Tue, 29 May 2012, Stefano Stabellini wrote:
> On Tue, 29 May 2012, Ian Campbell wrote:
> > On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > > Introduce a new libxl_device_disk* parameter to
> > > libxl__device_disk_local_attach, the parameter is allocated by the
> > > caller. libxl__device_disk_local_attach is going to fill the new disk
> > > with informations about the new locally attached disk.  The new
> > > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > > afterwards.
> > > 
> > > Changes in v8:
> > > - free pdev_path and script in local_detach;
> > > - use libxl__strdup instead of strdup.
> > > 
> > > Changes in v7:
> > > - rename tmpdisk to localdisk;
> > > - add a comment in libxl__bootloader_state about localdisk.
> > > 
> > > Changes in v6:
> > > - return error in case pdev_path is NULL;
> > > - libxl__device_disk_local_attach update the new disk, the caller
> > > allocates it;
> > > - remove \n from logs.
> > > 
> > > Changes in v5:
> > > - rename disk to in_disk;
> > > - rename tmpdisk to disk;
> > > - copy disk to new_disk only on success;
> > > - remove check on libxl__zalloc success.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  tools/libxl/libxl.c            |   18 +++++++++++++++---
> > >  tools/libxl/libxl_bootloader.c |    4 ++--
> > >  tools/libxl/libxl_internal.h   |   10 +++++++++-
> > >  3 files changed, 26 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index cd870c4..762e72a 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -1735,13 +1735,24 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> > > +char * libxl__device_disk_local_attach(libxl__gc *gc,
> > > +        const libxl_device_disk *in_disk,
> > > +        libxl_device_disk *disk)
> > 
> > Why the weird indent?
> 
> Uhm.. I don't know, I think it was just the default on vim.
> 
> 
> > >  {
> > >      libxl_ctx *ctx = gc->owner;
> > >      char *dev = NULL;
> > >      char *ret = NULL;
> > >      int rc;
> > >  
> > > +    if (in_disk->pdev_path == NULL)
> > > +        return NULL;
> > > +
> > > +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> > > +    disk->pdev_path = libxl__strdup(NULL, in_disk->pdev_path);
> > > +    if (in_disk->script != NULL)
> > > +        disk->script = libxl__strdup(NULL, in_disk->script);
> > > +    disk->vdev = NULL;
> > > +
> > >      rc = libxl__device_disk_setdefault(gc, disk);
> > >      if (rc) goto out;
> > >  
> > > @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> > >                             " attach a qdisk image if the format is not raw");
> > >                  break;
> > >              }
> > > -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > > -                       disk->pdev_path);
> > > +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
> > >              dev = disk->pdev_path;
> > >              break;
> > >          default:
> > > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> > >       * needed by the soon to be started domain and do nothing.
> > >       */
> > >  
> > > +    free(disk->pdev_path);
> > > +    free(disk->script);
> > 
> > This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> > member, is that deliberate?
> 
> I think it is a mistake: all these strings used to be allocated on the
> gc, on previous versions of the series. However meanwhile the event
> series went in, changing completely libxl__bootloader_run (that is the
> caller of libxl__device_disk_local_attach).
> Allocating stuff on the gc is not correct anymore, so now they need to
> be explicitly freed. I think I should call libxl_device_disk_dispose
> here and strdup in libxl__device_disk_local_attach to make sure vdev is
> not on the gc.

The appended patch switches back the behavior to what we had in v5 and
before: pdev_path, script, and vdev are all allocated on the gc,
therefore freed automatically.


---

libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Introduce a new libxl_device_disk* parameter to
libxl__device_disk_local_attach, the parameter is allocated by the
caller. libxl__device_disk_local_attach is going to fill the new disk
with informations about the new locally attached disk.  The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.

Changes in v9:
- allocate pdev_path, script, and vdev on the gc.

Changes in v8:
- free pdev_path and script in local_detach;
- use libxl__strdup instead of strdup.

Changes in v7:
- rename tmpdisk to localdisk;
- add a comment in libxl__bootloader_state about localdisk.

Changes in v6:
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs.

Changes in v5:
- rename disk to in_disk;
- rename tmpdisk to disk;
- copy disk to new_disk only on success;
- remove check on libxl__zalloc success.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cd870c4..be1c900 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1735,13 +1735,24 @@ out:
     return ret;
 }
 
-char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc,
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *disk)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     char *ret = NULL;
     int rc;
 
+    if (in_disk->pdev_path == NULL)
+        return NULL;
+
+    memcpy(disk, in_disk, sizeof(libxl_device_disk));
+    disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
+    if (in_disk->script != NULL)
+        disk->script = libxl__strdup(gc, in_disk->script);
+    disk->vdev = NULL;
+
     rc = libxl__device_disk_setdefault(gc, disk);
     if (rc) goto out;
 
@@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
                            " attach a qdisk image if the format is not raw");
                 break;
             }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
-                       disk->pdev_path);
+            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
             dev = disk->pdev_path;
             break;
         default:
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index e8950b1..82371f1 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
     if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
 
     if (bl->diskpath) {
-        libxl__device_disk_local_detach(gc, bl->disk);
+        libxl__device_disk_local_detach(gc, &bl->localdisk);
         free(bl->diskpath);
         bl->diskpath = 0;
     }
@@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
         goto out;
     }
 
-    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
+    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk);
     if (!bl->diskpath) {
         rc = ERROR_FAIL;
         goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d34e561..21b8b54 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1265,7 +1265,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
  * a device.
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
-        libxl_device_disk *disk);
+        const libxl_device_disk *in_disk,
+        libxl_device_disk *new_disk);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
@@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
     libxl__bootloader_console_callback *console_available;
     libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
     libxl_device_disk *disk;
+    /* Should be zeroed by caller on entry.  Will be filled in by
+     * bootloader machinery; represents the local attachment of the
+     * disk for the benefit of the bootloader.  Must be detached by
+     * the caller using libxl__device_disk_local_detach, but only
+     * after the domain's kernel and initramfs have been loaded into
+     * memory and the file references disposed of. */
+    libxl_device_disk localdisk;
     uint32_t domid;
     /* private to libxl__run_bootloader */
     char *outputpath, *outputdir, *logfile;

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell]

On Tue, 2012-05-29 at 15:48 +0100, Stefano Stabellini wrote:
> On Tue, 29 May 2012, Stefano Stabellini wrote:
> > On Tue, 29 May 2012, Ian Campbell wrote:
> > > On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > > > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> > > >       * needed by the soon to be started domain and do nothing.
> > > >       */
> > > >  
> > > > +    free(disk->pdev_path);
> > > > +    free(disk->script);
> > > 
> > > This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> > > member, is that deliberate?
> > 
> > I think it is a mistake: all these strings used to be allocated on the
> > gc, on previous versions of the series. However meanwhile the event
> > series went in, changing completely libxl__bootloader_run (that is the
> > caller of libxl__device_disk_local_attach).
> > Allocating stuff on the gc is not correct anymore, so now they need to
> > be explicitly freed. I think I should call libxl_device_disk_dispose
> > here and strdup in libxl__device_disk_local_attach to make sure vdev is
> > not on the gc.
> 
> The appended patch switches back the behavior to what we had in v5 and
> before: pdev_path, script, and vdev are all allocated on the gc,
> therefore freed automatically.

Thanks, this looks good to me.

If I slot this in in the place of the original will the rest of the
series apply or shall I wait for a resend/retest?

Ian.

> 
> 
> ---
> 
> libxl: libxl__device_disk_local_attach return a new libxl_device_disk
> 
> Introduce a new libxl_device_disk* parameter to
> libxl__device_disk_local_attach, the parameter is allocated by the
> caller. libxl__device_disk_local_attach is going to fill the new disk
> with informations about the new locally attached disk.  The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
> 
> Changes in v9:
> - allocate pdev_path, script, and vdev on the gc.
> 
> Changes in v8:
> - free pdev_path and script in local_detach;
> - use libxl__strdup instead of strdup.
> 
> Changes in v7:
> - rename tmpdisk to localdisk;
> - add a comment in libxl__bootloader_state about localdisk.
> 
> Changes in v6:
> - return error in case pdev_path is NULL;
> - libxl__device_disk_local_attach update the new disk, the caller
> allocates it;
> - remove \n from logs.
> 
> Changes in v5:
> - rename disk to in_disk;
> - rename tmpdisk to disk;
> - copy disk to new_disk only on success;
> - remove check on libxl__zalloc success.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index cd870c4..be1c900 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1735,13 +1735,24 @@ out:
>      return ret;
>  }
>  
> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk *disk)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
>      char *ret = NULL;
>      int rc;
>  
> +    if (in_disk->pdev_path == NULL)
> +        return NULL;
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
> +    if (in_disk->script != NULL)
> +        disk->script = libxl__strdup(gc, in_disk->script);
> +    disk->vdev = NULL;
> +
>      rc = libxl__device_disk_setdefault(gc, disk);
>      if (rc) goto out;
>  
> @@ -1779,8 +1790,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
>                             " attach a qdisk image if the format is not raw");
>                  break;
>              }
> -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);
> +            LOG(DEBUG, "locally attaching qdisk %s", in_disk->pdev_path);
>              dev = disk->pdev_path;
>              break;
>          default:
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index e8950b1..82371f1 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
>      if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
>  
>      if (bl->diskpath) {
> -        libxl__device_disk_local_detach(gc, bl->disk);
> +        libxl__device_disk_local_detach(gc, &bl->localdisk);
>          free(bl->diskpath);
>          bl->diskpath = 0;
>      }
> @@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc, libxl__bootloader_state *bl)
>          goto out;
>      }
>  
> -    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
> +    bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk, &bl->localdisk);
>      if (!bl->diskpath) {
>          rc = ERROR_FAIL;
>          goto out;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d34e561..21b8b54 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1265,7 +1265,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
>   * a device.
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> -        libxl_device_disk *disk);
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk *new_disk);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>  
> @@ -1803,6 +1804,13 @@ struct libxl__bootloader_state {
>      libxl__bootloader_console_callback *console_available;
>      libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
>      libxl_device_disk *disk;
> +    /* Should be zeroed by caller on entry.  Will be filled in by
> +     * bootloader machinery; represents the local attachment of the
> +     * disk for the benefit of the bootloader.  Must be detached by
> +     * the caller using libxl__device_disk_local_detach, but only
> +     * after the domain's kernel and initramfs have been loaded into
> +     * memory and the file references disposed of. */
> +    libxl_device_disk localdisk;
>      uint32_t domid;
>      /* private to libxl__run_bootloader */
>      char *outputpath, *outputdir, *logfile;

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

On Tue, 29 May 2012, Ian Campbell wrote:
> On Tue, 2012-05-29 at 15:48 +0100, Stefano Stabellini wrote:
> > On Tue, 29 May 2012, Stefano Stabellini wrote:
> > > On Tue, 29 May 2012, Ian Campbell wrote:
> > > > On Tue, 2012-05-29 at 11:39 +0100, Stefano Stabellini wrote:
> > > > > @@ -1804,6 +1814,8 @@ int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> > > > >       * needed by the soon to be started domain and do nothing.
> > > > >       */
> > > > >  
> > > > > +    free(disk->pdev_path);
> > > > > +    free(disk->script);
> > > > 
> > > > This is open coding libxl_device_disk_dispose(disk) but missed the vdev
> > > > member, is that deliberate?
> > > 
> > > I think it is a mistake: all these strings used to be allocated on the
> > > gc, on previous versions of the series. However meanwhile the event
> > > series went in, changing completely libxl__bootloader_run (that is the
> > > caller of libxl__device_disk_local_attach).
> > > Allocating stuff on the gc is not correct anymore, so now they need to
> > > be explicitly freed. I think I should call libxl_device_disk_dispose
> > > here and strdup in libxl__device_disk_local_attach to make sure vdev is
> > > not on the gc.
> > 
> > The appended patch switches back the behavior to what we had in v5 and
> > before: pdev_path, script, and vdev are all allocated on the gc,
> > therefore freed automatically.
> 
> Thanks, this looks good to me.
> 
> If I slot this in in the place of the original will the rest of the
> series apply or shall I wait for a resend/retest?

I have already tested the patch before sending it.
I think you might find that 1 hunk of patch #8 won't apply automatically
but it should be trivial to fix the conflict.
Let me know if I need to resend.

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
...
> The appended patch switches back the behavior to what we had in v5 and
> before: pdev_path, script, and vdev are all allocated on the gc,
> therefore freed automatically.

Thanks.

> libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
...
> The appended patch switches back the behavior to what we had in v5 and
> before: pdev_path, script, and vdev are all allocated on the gc,
> therefore freed automatically.

With the revised version of 02/11, I have now acked all 11 of these.

Ian.

Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell]

On Tue, 2012-05-29 at 16:25 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v8 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> ...
> > The appended patch switches back the behavior to what we had in v5 and
> > before: pdev_path, script, and vdev are all allocated on the gc,
> > therefore freed automatically.
> 
> With the revised version of 02/11, I have now acked all 11 of these.

Thanks, I'm about to apply...

Re: [PATCH v8 0/11] qdisk local attach

[Ian Campbell]

On Tue, 2012-05-29 at 11:38 +0100, Stefano Stabellini wrote:
> this patch implements local_attach support for QDISK: 

Applied, thanks!

I reworked some of the later commit summaries to be "libxl:..." or
"xl:...." in line with our normal convention, hope that's ok.

Ian.

Re: [PATCH v8 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Roger Pau Monne]

Stefano Stabellini wrote:
>   int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
>   {
> -    /* Nothing to do for PHYSTYPE_PHY. */
> +    int rc = 0;
>
> -    /*
> -     * For other device types assume that the blktap2 process is
> -     * needed by the soon to be started domain and do nothing.
> -     */
> +    switch (disk->backend) {
> +        case LIBXL_DISK_BACKEND_QDISK:
> +            if (disk->vdev != NULL) {
> +                libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
> +                        disk, 0);

I was just looking at this code, and I have the feeling this could be 
dangerous, since you call libxl__device_disk_local_detach from an 
already running ao (the bootloader ao), and libxl_device_disk_remove 
will initiate another ao, and set it to completed inside of an already 
running ao.

> +                rc = libxl_device_disk_destroy(gc->owner,
> +                        LIBXL_TOOLSTACK_DOMID, disk);
> +            }
> +            break;
> +        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.
> +             */
> +            break;
> +    }
>
>       free(disk->pdev_path);
>       free(disk->script);
> -    return 0;
> +
> +    return rc;
>   }
>
>   /******************************************************************************/

Re: [PATCH v8 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Ian Jackson]

Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v8 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> Stefano Stabellini wrote:
> >   int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> >   {
> > -    /* Nothing to do for PHYSTYPE_PHY. */
> > +    int rc = 0;
> >
> > -    /*
> > -     * For other device types assume that the blktap2 process is
> > -     * needed by the soon to be started domain and do nothing.
> > -     */
> > +    switch (disk->backend) {
> > +        case LIBXL_DISK_BACKEND_QDISK:
> > +            if (disk->vdev != NULL) {
> > +                libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
> > +                        disk, 0);
> 
> I was just looking at this code, and I have the feeling this could be 
> dangerous, since you call libxl__device_disk_local_detach from an 
> already running ao (the bootloader ao), and libxl_device_disk_remove 
> will initiate another ao, and set it to completed inside of an already 
> running ao.

This is indeed forbidden.  The inner ao function exit will reenter the
libxl event loop, which might do almost anything, including
unexpectedly recursively reentering the other asynchronous operation
(the lock doesn't prevent this since it's the same thread).

Ian.