[PATCH v3 0/7] qdisk local attach

[PATCH v3 0/7] 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 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 (7):
      libxl: libxl__device_disk_local_attach return a new libxl_device_disk
      libxl: add a transaction parameter to libxl__device_generic_add
      libxl: introduce libxl__device_disk_add_t
      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"

 tools/examples/xl.conf                          |    3 +
 tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
 tools/hotplug/Linux/init.d/xencommons           |    3 +
 tools/libxl/libxl.c                             |  232 +-----------------
 tools/libxl/libxl.h                             |   17 +-
 tools/libxl/libxl_bootloader.c                  |   13 +-
 tools/libxl/libxl_create.c                      |   18 +-
 tools/libxl/libxl_device.c                      |   14 +-
 tools/libxl/libxl_internal.c                    |  299 +++++++++++++++++++++++
 tools/libxl/libxl_internal.h                    |   24 ++-
 tools/libxl/libxl_linux.c                       |   40 +++
 tools/libxl/libxl_netbsd.c                      |    6 +
 tools/libxl/libxl_pci.c                         |    2 +-
 tools/libxl/xl.c                                |    3 +
 tools/libxl/xl.h                                |    1 +
 tools/libxl/xl_cmdimpl.c                        |    5 +-
 16 files changed, 422 insertions(+), 261 deletions(-)


Cheers,

Stefano

[PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

- make libxl_device_disk_local_attach/detach two internal functions;

- pass a gc rather than a ctx to them;

- introduce a new libxl_device_disk** parameter to
libxl__device_disk_local_attach, the parameter is allocated on the gc by
libxl__device_disk_local_attach. It is going to fill it with
informations about the new locally attached disk.  The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c            |   73 -----------------------------------
 tools/libxl/libxl.h            |    7 ---
 tools/libxl/libxl_bootloader.c |    9 ++--
 tools/libxl/libxl_internal.c   |   82 ++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h   |   11 +++++
 5 files changed, 97 insertions(+), 85 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 60dbfdc..e645d2a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1664,79 +1664,6 @@ out:
     return ret;
 }
 
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
-    GC_INIT(ctx);
-    char *dev = NULL;
-    char *ret = NULL;
-    int rc;
-
-    rc = libxl__device_disk_setdefault(gc, disk);
-    if (rc) goto out;
-
-    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);
-                break;
-            }
-            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;
-            }
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
-                       disk->pdev_path);
-            dev = disk->pdev_path;
-            break;
-        default:
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
-                "type: %d", disk->backend);
-            break;
-    }
-
- out:
-    if (dev != NULL)
-        ret = strdup(dev);
-    GC_FREE;
-    return ret;
-}
-
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
-    /* 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.
-     */
-
-    return 0;
-}
-
 /******************************************************************************/
 
 int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index edbca53..779c8dd 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -618,13 +618,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 2774062..429253d 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     char *fifo = NULL;
     char *diskpath = NULL;
     char **args = NULL;
+    libxl_device_disk *tmpdisk = NULL;
 
     char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
     char *tempdir;
@@ -386,7 +387,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
-    diskpath = libxl_device_disk_local_attach(ctx, disk);
+    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
     if (!diskpath) {
         goto out_close;
     }
@@ -452,10 +453,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
     rc = 0;
 out_close:
-    if (diskpath) {
-        libxl_device_disk_local_detach(ctx, disk);
-        free(diskpath);
-    }
+    if (tmpdisk)
+        libxl__device_disk_local_detach(gc, tmpdisk);
     if (fifo_fd > -1)
         close(fifo_fd);
     if (bootloader_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b89aef7..95fb918 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,88 @@ out:
     return rc;
 }
 
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+        const libxl_device_disk *disk,
+        libxl_device_disk **new_disk)
+{
+    libxl_ctx *ctx = gc->owner;
+    char *dev = NULL;
+    int rc;
+    libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
+    if (tmpdisk == NULL) goto out;
+
+    *new_disk = tmpdisk;
+    memcpy(tmpdisk, disk, sizeof(libxl_device_disk));
+    if (disk->pdev_path != NULL)
+        tmpdisk->pdev_path = libxl__strdup(gc, disk->pdev_path);
+    if (disk->script != NULL)
+        tmpdisk->script = libxl__strdup(gc, disk->script);
+    tmpdisk->vdev = NULL;
+
+    rc = libxl__device_disk_setdefault(gc, tmpdisk);
+    if (rc) goto out;
+
+    switch (tmpdisk->backend) {
+        case LIBXL_DISK_BACKEND_PHY:
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
+                       tmpdisk->pdev_path);
+            dev = tmpdisk->pdev_path;
+            break;
+        case LIBXL_DISK_BACKEND_TAP:
+            switch (tmpdisk->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)",
+                           tmpdisk->pdev_path);
+                dev = tmpdisk->pdev_path;
+                break;
+            case LIBXL_DISK_FORMAT_VHD:
+                dev = libxl__blktap_devpath(gc, tmpdisk->pdev_path,
+                                            tmpdisk->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", tmpdisk->format);
+                break;
+            }
+            break;
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
+                           " 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);
+            dev = tmpdisk->pdev_path;
+            break;
+        default:
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
+                "type: %d", tmpdisk->backend);
+            break;
+    }
+
+ out:
+    return dev;
+}
+
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
+        libxl_device_disk *disk)
+{
+    /* 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.
+     */
+
+    return 0;
+}
+
 libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc,
                                                                uint32_t domid)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a4b933b..4e0d203 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1001,6 +1001,17 @@ _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,
+        const libxl_device_disk *disk,
+        libxl_device_disk **new_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

Re: [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> - make libxl_device_disk_local_attach/detach two internal functions;
> 
> - pass a gc rather than a ctx to them;
> 
> - introduce a new libxl_device_disk** parameter to
> libxl__device_disk_local_attach, the parameter is allocated on the gc by
> libxl__device_disk_local_attach. It is going to fill it with
> informations about the new locally attached disk.  The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.

Mixing the changes up with teh code motion makes this quite hard to
review.  Would it be easy to separate those out or should I do some
kind of manual hack to compare the old and new code ?

Ian.

Re: [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell]

On Tue, 2012-04-17 at 18:25 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > - make libxl_device_disk_local_attach/detach two internal functions;
> > 
> > - pass a gc rather than a ctx to them;
> > 
> > - introduce a new libxl_device_disk** parameter to
> > libxl__device_disk_local_attach, the parameter is allocated on the gc by
> > libxl__device_disk_local_attach. It is going to fill it with
> > informations about the new locally attached disk.  The new
> > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > afterwards.
> 
> Mixing the changes up with teh code motion makes this quite hard to
> review.

This was also going to be my first comment.

Re: [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Stefano Stabellini]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 1/7] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > - make libxl_device_disk_local_attach/detach two internal functions;
> > 
> > - pass a gc rather than a ctx to them;
> > 
> > - introduce a new libxl_device_disk** parameter to
> > libxl__device_disk_local_attach, the parameter is allocated on the gc by
> > libxl__device_disk_local_attach. It is going to fill it with
> > informations about the new locally attached disk.  The new
> > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > afterwards.
> 
> Mixing the changes up with teh code motion makes this quite hard to
> review.  Would it be easy to separate those out or should I do some
> kind of manual hack to compare the old and new code ?
 
I'll do that for you

[PATCH v3 2/7] 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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c          |   10 +++++-----
 tools/libxl/libxl_device.c   |   14 +++++++-------
 tools/libxl/libxl_internal.h |    4 ++--
 tools/libxl/libxl_pci.c      |    2 +-
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e645d2a..85082eb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1401,7 +1401,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));
 
@@ -1795,7 +1795,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));
 
@@ -2073,7 +2073,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;
@@ -2144,7 +2144,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;
@@ -2277,7 +2277,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 c7e057d..05909c5 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) {
@@ -101,13 +102,12 @@ retry_transaction:
     }
 
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
-        if (errno == EAGAIN)
+        if (errno == EAGAIN && create_transaction)
             goto retry_transaction;
         else
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
     }
-
-    return 0;
+    return ERROR_FAIL;
 }
 
 typedef struct {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4e0d203..c0991eb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -681,8 +681,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 e8b8839..202a101 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -102,7 +102,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

Re: [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add"):
> Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
> XBT_NULL, allocate a proper one.
> 
> Update all the callers.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

> +    int create_transaction = t == XBT_NULL;

Although I don't much like this style.  I would have written
  +    int create_transaction = !t;
but I guess you're not going to swallow that ...

Ian.

Re: [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add

[Ian Campbell]

On Mon, 2012-04-16 at 19:33 +0100, Stefano Stabellini wrote:
> Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
> XBT_NULL, allocate a proper one.
> 
> Update all the callers.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |   10 +++++-----
>  tools/libxl/libxl_device.c   |   14 +++++++-------
>  tools/libxl/libxl_internal.h |    4 ++--
>  tools/libxl/libxl_pci.c      |    2 +-
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e645d2a..85082eb 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1401,7 +1401,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));
>  
> @@ -1795,7 +1795,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));
>  
> @@ -2073,7 +2073,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;
> @@ -2144,7 +2144,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;
> @@ -2277,7 +2277,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 c7e057d..05909c5 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) {
> @@ -101,13 +102,12 @@ retry_transaction:
>      }
>  
>      if (!xs_transaction_end(ctx->xsh, t, 0)) {

Do we really want to end the transaction for caller provided t? (i.e.
when create_transaction == False)

It would seem more expected to me to return to the caller and expect
them to complete the transaction and perform error handling etc. If the
caller doesn't have things of its own to do in the transaction then why
does it have one in hand to pass in?

> -        if (errno == EAGAIN)
> +        if (errno == EAGAIN && create_transaction)
>              goto retry_transaction;
>          else
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
>      }
> -
> -    return 0;
> +    return ERROR_FAIL;

Where is the success exit path in this function now?

>  }
>  
>  typedef struct {
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4e0d203..c0991eb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -681,8 +681,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 e8b8839..202a101 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -102,7 +102,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));
>  

Re: [PATCH v3 2/7] libxl: add a transaction parameter to libxl__device_generic_add

[Stefano Stabellini]

On Wed, 18 Apr 2012, Ian Campbell wrote:
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index c7e057d..05909c5 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) {
> > @@ -101,13 +102,12 @@ retry_transaction:
> >      }
> >  
> >      if (!xs_transaction_end(ctx->xsh, t, 0)) {
> 
> Do we really want to end the transaction for caller provided t? (i.e.
> when create_transaction == False)
> 
> It would seem more expected to me to return to the caller and expect
> them to complete the transaction and perform error handling etc. If the
> caller doesn't have things of its own to do in the transaction then why
> does it have one in hand to pass in?

I think you are right, I'll change it.


> > -        if (errno == EAGAIN)
> > +        if (errno == EAGAIN && create_transaction)
> >              goto retry_transaction;
> >          else
> >              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
> >      }
> > -
> > -    return 0;
> > +    return ERROR_FAIL;
> 
> Where is the success exit path in this function now?

Good point, I'll fix that too.

[PATCH v3 3/7] libxl: introduce libxl__device_disk_add_t

[Stefano Stabellini]

Introduce libxl__device_disk_add_t that takes an additional
xs_transaction_t paramter.
Implement libxl_device_disk_add using libxl__device_disk_add_t.
Move libxl__device_from_disk to libxl_internal.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c          |  151 +----------------------------------------
 tools/libxl/libxl_internal.c |  157 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    6 ++
 3 files changed, 164 insertions(+), 150 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 85082eb..0d7c0f6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1258,159 +1258,10 @@ 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,
-                                   libxl_device_disk *disk,
-                                   libxl__device *device)
-{
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    int devid;
-
-    devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
-    if (devid==-1) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
-               " virtual disk identifier %s", disk->vdev);
-        return ERROR_INVAL;
-    }
-
-    device->backend_domid = disk->backend_domid;
-    device->backend_devid = devid;
-
-    switch (disk->backend) {
-        case LIBXL_DISK_BACKEND_PHY:
-            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
-            break;
-        case LIBXL_DISK_BACKEND_TAP:
-            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
-            break;
-        case LIBXL_DISK_BACKEND_QDISK:
-            device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
-            break;
-        default:
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
-                       disk->backend);
-            return ERROR_INVAL;
-    }
-
-    device->domid = domid;
-    device->devid = devid;
-    device->kind  = LIBXL__DEVICE_KIND_VBD;
-
-    return 0;
-}
-
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     GC_INIT(ctx);
-    flexarray_t *front;
-    flexarray_t *back;
-    char *dev;
-    libxl__device device;
-    int major, minor, rc;
-
-    rc = libxl__device_disk_setdefault(gc, disk);
-    if (rc) goto out;
-
-    front = flexarray_make(16, 1);
-    if (!front) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-    back = flexarray_make(16, 1);
-    if (!back) {
-        rc = ERROR_NOMEM;
-        goto out_free;
-    }
-
-    if (disk->script) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
-                   " not yet supported, sorry");
-        rc = ERROR_INVAL;
-        goto out_free;
-    }
-
-    rc = libxl__device_from_disk(gc, domid, disk, &device);
-    if (rc != 0) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
-               " virtual disk identifier %s", disk->vdev);
-        goto out_free;
-    }
-
-    switch (disk->backend) {
-        case LIBXL_DISK_BACKEND_PHY:
-            dev = disk->pdev_path;
-    do_backend_phy:
-            libxl__device_physdisk_major_minor(dev, &major, &minor);
-            flexarray_append(back, "physical-device");
-            flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
-
-            flexarray_append(back, "params");
-            flexarray_append(back, dev);
-
-            assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
-            break;
-        case LIBXL_DISK_BACKEND_TAP:
-            dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
-            if (!dev) {
-                rc = ERROR_FAIL;
-                goto out_free;
-            }
-            flexarray_append(back, "tapdisk-params");
-            flexarray_append(back, libxl__sprintf(gc, "%s:%s",
-                libxl__device_disk_string_of_format(disk->format),
-                disk->pdev_path));
-
-            /* now create a phy device to export the device to the guest */
-            goto do_backend_phy;
-        case LIBXL_DISK_BACKEND_QDISK:
-            flexarray_append(back, "params");
-            flexarray_append(back, libxl__sprintf(gc, "%s:%s",
-                          libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
-            assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
-            break;
-        default:
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
-            rc = ERROR_INVAL;
-            goto out_free;
-    }
-
-    flexarray_append(back, "frontend-id");
-    flexarray_append(back, libxl__sprintf(gc, "%d", domid));
-    flexarray_append(back, "online");
-    flexarray_append(back, "1");
-    flexarray_append(back, "removable");
-    flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
-    flexarray_append(back, "bootable");
-    flexarray_append(back, libxl__sprintf(gc, "%d", 1));
-    flexarray_append(back, "state");
-    flexarray_append(back, libxl__sprintf(gc, "%d", 1));
-    flexarray_append(back, "dev");
-    flexarray_append(back, disk->vdev);
-    flexarray_append(back, "type");
-    flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
-    flexarray_append(back, "mode");
-    flexarray_append(back, disk->readwrite ? "w" : "r");
-    flexarray_append(back, "device-type");
-    flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
-
-    flexarray_append(front, "backend-id");
-    flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
-    flexarray_append(front, "state");
-    flexarray_append(front, libxl__sprintf(gc, "%d", 1));
-    flexarray_append(front, "virtual-device");
-    flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
-    flexarray_append(front, "device-type");
-    flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
-
-    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;
-
-out_free:
-    flexarray_free(back);
-    flexarray_free(front);
-out:
+    int rc = libxl__device_disk_add_t(gc, domid, XBT_NULL, disk);
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 95fb918..9ecfcc7 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,163 @@ out:
     return rc;
 }
 
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+                                   libxl_device_disk *disk,
+                                   libxl__device *device)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int devid;
+
+    devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+    if (devid==-1) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+               " virtual disk identifier %s", disk->vdev);
+        return ERROR_INVAL;
+    }
+
+    device->backend_domid = disk->backend_domid;
+    device->backend_devid = devid;
+
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_PHY:
+            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+            break;
+        case LIBXL_DISK_BACKEND_TAP:
+            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+            break;
+        case LIBXL_DISK_BACKEND_QDISK:
+            device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
+            break;
+        default:
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
+                       disk->backend);
+            return ERROR_INVAL;
+    }
+
+    device->domid = domid;
+    device->devid = devid;
+    device->kind  = LIBXL__DEVICE_KIND_VBD;
+
+    return 0;
+}
+
+_hidden int libxl__device_disk_add_t(libxl__gc *gc, uint32_t domid,
+        xs_transaction_t t, libxl_device_disk *disk)
+{
+    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;
+
+    front = flexarray_make(16, 1);
+    if (!front) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
+    back = flexarray_make(16, 1);
+    if (!back) {
+        rc = ERROR_NOMEM;
+        goto out_free;
+    }
+
+    if (disk->script) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
+                   " not yet supported, sorry");
+        rc = ERROR_INVAL;
+        goto out_free;
+    }
+
+    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    if (rc != 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+               " virtual disk identifier %s", disk->vdev);
+        goto out_free;
+    }
+
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_PHY:
+            dev = disk->pdev_path;
+    do_backend_phy:
+            libxl__device_physdisk_major_minor(dev, &major, &minor);
+            flexarray_append(back, "physical-device");
+            flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
+
+            flexarray_append(back, "params");
+            flexarray_append(back, dev);
+
+            assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
+            break;
+        case LIBXL_DISK_BACKEND_TAP:
+            dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
+            if (!dev) {
+                rc = ERROR_FAIL;
+                goto out_free;
+            }
+            flexarray_append(back, "tapdisk-params");
+            flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+                libxl__device_disk_string_of_format(disk->format),
+                disk->pdev_path));
+
+            /* now create a phy device to export the device to the guest */
+            goto do_backend_phy;
+        case LIBXL_DISK_BACKEND_QDISK:
+            flexarray_append(back, "params");
+            flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+                          libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
+            assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
+            break;
+        default:
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
+            rc = ERROR_INVAL;
+            goto out_free;
+    }
+
+    flexarray_append(back, "frontend-id");
+    flexarray_append(back, libxl__sprintf(gc, "%d", domid));
+    flexarray_append(back, "online");
+    flexarray_append(back, "1");
+    flexarray_append(back, "removable");
+    flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
+    flexarray_append(back, "bootable");
+    flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+    flexarray_append(back, "state");
+    flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+    flexarray_append(back, "dev");
+    flexarray_append(back, disk->vdev);
+    flexarray_append(back, "type");
+    flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
+    flexarray_append(back, "mode");
+    flexarray_append(back, disk->readwrite ? "w" : "r");
+    flexarray_append(back, "device-type");
+    flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+
+    flexarray_append(front, "backend-id");
+    flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
+    flexarray_append(front, "state");
+    flexarray_append(front, libxl__sprintf(gc, "%d", 1));
+    flexarray_append(front, "virtual-device");
+    flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
+    flexarray_append(front, "device-type");
+    flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
+
+    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));
+
+    rc = 0;
+
+out_free:
+    flexarray_free(back);
+    flexarray_free(front);
+out:
+    return rc;
+}
+
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *disk,
         libxl_device_disk **new_disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c0991eb..c171f24 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1001,6 +1001,12 @@ _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);
+_hidden int libxl__device_disk_add_t(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
  * a device.
-- 
1.7.2.5

Re: [PATCH v3 3/7] libxl: introduce libxl__device_disk_add_t

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 3/7] libxl: introduce libxl__device_disk_add_t"):
> Introduce libxl__device_disk_add_t that takes an additional
> xs_transaction_t paramter.
> Implement libxl_device_disk_add using libxl__device_disk_add_t.
> Move libxl__device_from_disk to libxl_internal.c.

Is this supposed to be "no functional change" ?  If so it would really
help to say so.

Ian.

Re: [PATCH v3 3/7] libxl: introduce libxl__device_disk_add_t

[Stefano Stabellini]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 3/7] libxl: introduce libxl__device_disk_add_t"):
> > Introduce libxl__device_disk_add_t that takes an additional
> > xs_transaction_t paramter.
> > Implement libxl_device_disk_add using libxl__device_disk_add_t.
> > Move libxl__device_from_disk to libxl_internal.c.
> 
> Is this supposed to be "no functional change" ?  If so it would really
> help to say so.
 
yes

[PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Stefano Stabellini]

Introduce a blkdev_start in xl.conf and pass it to
libxl_domain_create_* and all the way through libxl_run_bootloader and
libxl__device_disk_local_attach.

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/examples/xl.conf         |    3 +++
 tools/libxl/libxl.h            |   10 +++++++---
 tools/libxl/libxl_bootloader.c |    6 ++++--
 tools/libxl/libxl_create.c     |   18 ++++++++++++------
 tools/libxl/libxl_internal.c   |    3 ++-
 tools/libxl/libxl_internal.h   |    3 ++-
 tools/libxl/xl.c               |    3 +++
 tools/libxl/xl.h               |    1 +
 tools/libxl/xl_cmdimpl.c       |    5 +++--
 9 files changed, 37 insertions(+), 15 deletions(-)

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.h b/tools/libxl/libxl.h
index 779c8dd..151a6e0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -464,8 +464,11 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
 
 /* domain related functions */
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
-int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
-int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
+int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
+		libxl_console_ready cb, void *priv, uint32_t *domid, char* blkdev_start);
+int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
+		libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd,
+		char *blkdev_start);
 void libxl_domain_config_dispose(libxl_domain_config *d_config);
 int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
                           uint32_t domid, int fd);
@@ -493,7 +496,8 @@ int libxl_get_max_cpus(libxl_ctx *ctx);
 int libxl_run_bootloader(libxl_ctx *ctx,
                          libxl_domain_build_info *info,
                          libxl_device_disk *disk,
-                         uint32_t domid);
+                         uint32_t domid,
+                         char *blkdev_start);
 
   /* 0 means ERROR_ENOMEM, which we have logged */
 
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 429253d..fe56615 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -323,7 +323,8 @@ static void parse_bootloader_result(libxl__gc *gc,
 int libxl_run_bootloader(libxl_ctx *ctx,
                          libxl_domain_build_info *info,
                          libxl_device_disk *disk,
-                         uint32_t domid)
+                         uint32_t domid,
+                         char *blkdev_start)
 {
     GC_INIT(ctx);
     int ret, rc = 0;
@@ -387,7 +388,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
-    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
+    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk,
+            blkdev_start);
     if (!diskpath) {
         goto out_close;
     }
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 6029445..5064bc3 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -544,7 +544,8 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
 
 static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                             libxl_console_ready cb, void *priv,
-                            uint32_t *domid_out, int restore_fd)
+                            uint32_t *domid_out, int restore_fd,
+							char* blkdev_start)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     libxl__spawner_starting *dm_starting = 0;
@@ -581,7 +582,9 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
     }
 
     if ( restore_fd < 0 ) {
-        ret = libxl_run_bootloader(ctx, &d_config->b_info, d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
+        ret = libxl_run_bootloader(ctx, &d_config->b_info,
+                d_config->num_disks > 0 ? &d_config->disks[0] : NULL,
+                domid, blkdev_start);
         if (ret) {
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                        "failed to run bootloader: %d", ret);
@@ -735,21 +738,24 @@ error_out:
 }
 
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
-                            libxl_console_ready cb, void *priv, uint32_t *domid)
+                            libxl_console_ready cb, void *priv, uint32_t *domid,
+							char* blkdev_start)
 {
     GC_INIT(ctx);
     int rc;
-    rc = do_domain_create(gc, d_config, cb, priv, domid, -1);
+    rc = do_domain_create(gc, d_config, cb, priv, domid, -1, blkdev_start);
     GC_FREE;
     return rc;
 }
 
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
-                                libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd)
+                                libxl_console_ready cb, void *priv, uint32_t *domid,
+								int restore_fd, char *blkdev_start)
 {
     GC_INIT(ctx);
     int rc;
-    rc = do_domain_create(gc, d_config, cb, priv, domid, restore_fd);
+    rc = do_domain_create(gc, d_config, cb, priv, domid, restore_fd,
+			blkdev_start);
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 9ecfcc7..6131ec5 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -482,7 +482,8 @@ out:
 
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *disk,
-        libxl_device_disk **new_disk)
+        libxl_device_disk **new_disk,
+        char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c171f24..aab8300 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1013,7 +1013,8 @@ _hidden int libxl__device_disk_add_t(libxl__gc *gc, uint32_t domid,
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *disk,
-        libxl_device_disk **new_disk);
+        libxl_device_disk **new_disk,
+        char *blkdev_start);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..6a735e9 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -35,6 +35,7 @@
 xentoollog_logger_stdiostream *logger;
 int dryrun_only;
 int autoballoon = 1;
+char *blkdev_start = "xvda";
 char *lockfile;
 char *default_vifscript = NULL;
 char *default_bridge = NULL;
@@ -91,6 +92,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 7e258d5..dba2c94 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -113,6 +113,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 c9e9943..1a3945c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1669,7 +1669,7 @@ start:
     if ( restore_file ) {
         ret = libxl_domain_create_restore(ctx, &d_config,
                                             cb, &child_console_pid,
-                                            &domid, restore_fd);
+                                            &domid, restore_fd, blkdev_start);
         /*
          * On subsequent reboot etc we should create the domain, not
          * restore/migrate-receive it again.
@@ -1677,7 +1677,8 @@ start:
         restore_file = NULL;
     }else{
         ret = libxl_domain_create_new(ctx, &d_config,
-                                        cb, &child_console_pid, &domid);
+                                        cb, &child_console_pid, &domid,
+										blkdev_start);
     }
     if ( ret )
         goto error_out;
-- 
1.7.2.5

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> Introduce a blkdev_start in xl.conf and pass it to
> libxl_domain_create_* and all the way through libxl_run_bootloader and
> libxl__device_disk_local_attach.

Surely this should be passed in the domain config structure rather
than being an adhoc parameter.

If the problem with that is that it really ought to be host-global and
come from xl.conf, then perhaps we need another config struct.  But
really I think that's overkill.  There is nothing wrong with taking
parameters from xl.conf and putting them in the libxl domain config.

Ian.

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Stefano Stabellini]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> > Introduce a blkdev_start in xl.conf and pass it to
> > libxl_domain_create_* and all the way through libxl_run_bootloader and
> > libxl__device_disk_local_attach.
> 
> Surely this should be passed in the domain config structure rather
> than being an adhoc parameter.

I don't think so, see below.

> If the problem with that is that it really ought to be host-global and
> come from xl.conf, then perhaps we need another config struct.  But
> really I think that's overkill.  There is nothing wrong with taking
> parameters from xl.conf and putting them in the libxl domain config.
 
Another host-global config struct is definitely overkill, but we cannot
really pass this parameter in the libxl domain config, because this
option has nothing to do with the domain config.
It would be confusing and incorrect.

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Ian Campbell]

On Fri, 2012-04-20 at 15:04 +0100, Stefano Stabellini wrote:
> On Tue, 17 Apr 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> > > Introduce a blkdev_start in xl.conf and pass it to
> > > libxl_domain_create_* and all the way through libxl_run_bootloader and
> > > libxl__device_disk_local_attach.
> > 
> > Surely this should be passed in the domain config structure rather
> > than being an adhoc parameter.
> 
> I don't think so, see below.
> 
> > If the problem with that is that it really ought to be host-global and
> > come from xl.conf, then perhaps we need another config struct.  But
> > really I think that's overkill.  There is nothing wrong with taking
> > parameters from xl.conf and putting them in the libxl domain config.
>  
> Another host-global config struct is definitely overkill, but we cannot
> really pass this parameter in the libxl domain config, because this
> option has nothing to do with the domain config.
> It would be confusing and incorrect.

You could argue that "domain config" was actually as much about details
on how to build the guest as it was about the "domain config" as such.
Members of that struct like the device model version, xsm ssid, disable
migrate, cpupool id, etc could all be argued to fit into the same "grey
area".

In any case adding a new parameter for each global variable which a
function happens to need to look at is not a good API.

If you don't like domain config as a home for this then the "overkill"
option of a new config struct seems like a reasonable answer. It's
possible that it only seems like overkill now because we don't have many
of such things, but we might be thankful for it when we (inevitably)
have half a dozen.

Alternatively a variable in the ctx and functions to set/get it might
work, but that seems like a worse version of the global config struct to
me...

Ian.

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Stefano Stabellini]

On Fri, 20 Apr 2012, Ian Campbell wrote:
> On Fri, 2012-04-20 at 15:04 +0100, Stefano Stabellini wrote:
> > On Tue, 17 Apr 2012, Ian Jackson wrote:
> > > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> > > > Introduce a blkdev_start in xl.conf and pass it to
> > > > libxl_domain_create_* and all the way through libxl_run_bootloader and
> > > > libxl__device_disk_local_attach.
> > > 
> > > Surely this should be passed in the domain config structure rather
> > > than being an adhoc parameter.
> > 
> > I don't think so, see below.
> > 
> > > If the problem with that is that it really ought to be host-global and
> > > come from xl.conf, then perhaps we need another config struct.  But
> > > really I think that's overkill.  There is nothing wrong with taking
> > > parameters from xl.conf and putting them in the libxl domain config.
> >  
> > Another host-global config struct is definitely overkill, but we cannot
> > really pass this parameter in the libxl domain config, because this
> > option has nothing to do with the domain config.
> > It would be confusing and incorrect.
> 
> You could argue that "domain config" was actually as much about details
> on how to build the guest as it was about the "domain config" as such.
> Members of that struct like the device model version, xsm ssid, disable
> migrate, cpupool id, etc could all be argued to fit into the same "grey
> area".

What about a new libxl_domain_build_info parameter?

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Ian Campbell]

On Mon, 2012-04-23 at 18:40 +0100, Stefano Stabellini wrote:
> On Fri, 20 Apr 2012, Ian Campbell wrote:
> > On Fri, 2012-04-20 at 15:04 +0100, Stefano Stabellini wrote:
> > > On Tue, 17 Apr 2012, Ian Jackson wrote:
> > > > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> > > > > Introduce a blkdev_start in xl.conf and pass it to
> > > > > libxl_domain_create_* and all the way through libxl_run_bootloader and
> > > > > libxl__device_disk_local_attach.
> > > > 
> > > > Surely this should be passed in the domain config structure rather
> > > > than being an adhoc parameter.
> > > 
> > > I don't think so, see below.
> > > 
> > > > If the problem with that is that it really ought to be host-global and
> > > > come from xl.conf, then perhaps we need another config struct.  But
> > > > really I think that's overkill.  There is nothing wrong with taking
> > > > parameters from xl.conf and putting them in the libxl domain config.
> > >  
> > > Another host-global config struct is definitely overkill, but we cannot
> > > really pass this parameter in the libxl domain config, because this
> > > option has nothing to do with the domain config.
> > > It would be confusing and incorrect.
> > 
> > You could argue that "domain config" was actually as much about details
> > on how to build the guest as it was about the "domain config" as such.
> > Members of that struct like the device model version, xsm ssid, disable
> > migrate, cpupool id, etc could all be argued to fit into the same "grey
> > area".
> 
> What about a new libxl_domain_build_info parameter?

Sure, when I say "domain config" I think I probably mean
"libxl_domain_config or one of the member therein"...

Ian.

Re: [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 4/7] xl/libxl: add a blkdev_start parameter"):
> On Tue, 17 Apr 2012, Ian Jackson wrote:
...
> > If the problem with that is that it really ought to be host-global and
> > come from xl.conf, then perhaps we need another config struct.  But
> > really I think that's overkill.  There is nothing wrong with taking
> > parameters from xl.conf and putting them in the libxl domain config.
>  
> Another host-global config struct is definitely overkill, but we cannot
> really pass this parameter in the libxl domain config, because this
> option has nothing to do with the domain config.
> It would be confusing and incorrect.

It's a parameter used during the domain setup.

Ian.

[PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini]

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_internal.c |   18 ++++++++++++++++++
 tools/libxl/libxl_internal.h |    2 ++
 tools/libxl/libxl_linux.c    |   40 ++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_netbsd.c   |    6 ++++++
 4 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 6131ec5..ac59aa6 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,24 @@ out:
     return rc;
 }
 
+static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
+        char *blkdev_start, xs_transaction_t t)
+{
+    int devid = 0;
+    char *vdev = libxl__strdup(gc, blkdev_start);
+    char *dompath = libxl__xs_get_dompath(gc, domid);
+
+    do {
+        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+        if (libxl__xs_read(gc, t,
+                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+                        dompath, devid)) == NULL)
+            return libxl__devid_to_vdev(gc, devid);
+        vdev[strlen(vdev) - 1]++;
+    } while(vdev[strlen(vdev) - 1] <= 'z');
+    return NULL;
+}
+
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *disk,
         libxl_device_disk **new_disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index aab8300..871df05 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -761,6 +761,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+_hidden char *libxl__devid_to_vdev(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..955a23b 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,43 @@ 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))
+
+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_vdev(libxl__gc *gc, int devid)
+{
+    int minor;
+    int offset;
+    int nr_parts;
+    char *ptr = NULL;
+    char *ret = libxl__zalloc(gc, 32);
+
+    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
+        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..c8977ac 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_vdev(libxl__gc *gc, int devid)
+{
+    /* TODO */
+    return NULL;
+}
-- 
1.7.2.5

Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> +        char *blkdev_start, xs_transaction_t t)
> +{
> +    int devid = 0;
> +    char *vdev = libxl__strdup(gc, blkdev_start);
> +    char *dompath = libxl__xs_get_dompath(gc, domid);
> +
> +    do {
> +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> +        if (libxl__xs_read(gc, t,
> +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> +                        dompath, devid)) == NULL)
> +            return libxl__devid_to_vdev(gc, devid);

What if the error is not ENOENT ?

> +        vdev[strlen(vdev) - 1]++;
> +    } while(vdev[strlen(vdev) - 1] <= 'z');
              ^
Missing whitespace.

> +_hidden char *libxl__devid_to_vdev(libxl__gc *gc, int devid);

I think the terminology here needs to be corrected.  "vdev" is the
virtual device name supplied to libxl - a symbolic way of specifying a
devid in a guest-independent way.

Whereas what we actually mean is the non-portable name in this guest
OS for a device.  How about
  libxl__devid_to_localdev
?

> +static char *encode_disk_name(char *ptr, unsigned int n)

There is no clearly defined upper bound on the buffer space needed by
this function.

> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..c8977ac 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
...
> +char *libxl__devid_to_vdev(libxl__gc *gc, int devid)
> +{
> +    /* TODO */
> +    return NULL;
> +}

I guess this is going to be fixed in a future version of the patch ?

Ian.

Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> > +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> > +        char *blkdev_start, xs_transaction_t t)
> > +{
> > +    int devid = 0;
> > +    char *vdev = libxl__strdup(gc, blkdev_start);
> > +    char *dompath = libxl__xs_get_dompath(gc, domid);
> > +
> > +    do {
> > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > +        if (libxl__xs_read(gc, t,
> > +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> > +                        dompath, devid)) == NULL)
> > +            return libxl__devid_to_vdev(gc, devid);
> 
> What if the error is not ENOENT ?

we should return NULL


> > +        vdev[strlen(vdev) - 1]++;
> > +    } while(vdev[strlen(vdev) - 1] <= 'z');
>               ^
> Missing whitespace.

OK

> > +_hidden char *libxl__devid_to_vdev(libxl__gc *gc, int devid);
> 
> I think the terminology here needs to be corrected.  "vdev" is the
> virtual device name supplied to libxl - a symbolic way of specifying a
> devid in a guest-independent way.
> 
> Whereas what we actually mean is the non-portable name in this guest
> OS for a device.  How about
>   libxl__devid_to_localdev
> ?

right, I think it would be a bit clearer

> 
> > +static char *encode_disk_name(char *ptr, unsigned int n)
> 
> There is no clearly defined upper bound on the buffer space needed by
> this function.

I know but this function is used as is in Linux where the stack is
even smaller. I'll add an upper bound anyway.


> > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > index 9e0ed6d..c8977ac 100644
> > --- a/tools/libxl/libxl_netbsd.c
> > +++ b/tools/libxl/libxl_netbsd.c
> ...
> > +char *libxl__devid_to_vdev(libxl__gc *gc, int devid)
> > +{
> > +    /* TODO */
> > +    return NULL;
> > +}
> 
> I guess this is going to be fixed in a future version of the patch ?

I don't think so: I don't know anything about netbsd and local_attach
doesn't work there anyway.

Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> On Tue, 17 Apr 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> > > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > > +        if (libxl__xs_read(gc, t,
> > > +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> > > +                        dompath, devid)) == NULL)
> > > +            return libxl__devid_to_vdev(gc, devid);
> > 
> > What if the error is not ENOENT ?
> 
> we should return NULL

I don't think that's correct.  If, say, the error is EACCES, then the
domain creation should be aborted with a message about that, since the
system has been installed incorrectly.

Compare this situation with the recent pygrub failure, where
libfsimage+pygrub turned all errors of the form "something went wrong
loading this plugin" into "the kernel was not found"; so when a
completely empty .so was loaded as a plugin the result was not "OMG
WTF this is totally broken" but "sorry can't find your kernel in this
filesystem" (when really the problem is that pygrub+libfsimage knew
that the filesystem was one they were supposed to support but the
plugin for it was utterly broken).

This reminds me of our other recent discussion about error handling,
of receiving unexpected toolstack migration info.  In general any
unanticipated situation should be treated as a fatal error.  Only
anticipated situations should result in the software continuing in a
degraded manner.

> > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > 
> > There is no clearly defined upper bound on the buffer space needed by
> > this function.
> 
> I know but this function is used as is in Linux where the stack is
> even smaller. I'll add an upper bound anyway.

At the very least a comment is needed to demonstrate that it's
correct, but a bound in the code would be better.  (Also I'm surprised
that you chose a recursive rather than iterative implementation of a
what is a base conversion routine...)

> > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > > index 9e0ed6d..c8977ac 100644
> > > --- a/tools/libxl/libxl_netbsd.c
> > > +++ b/tools/libxl/libxl_netbsd.c
> > ...
> > > +char *libxl__devid_to_vdev(libxl__gc *gc, int devid)
> > > +{
> > > +    /* TODO */
> > > +    return NULL;
> > > +}
> > 
> > I guess this is going to be fixed in a future version of the patch ?
> 
> I don't think so: I don't know anything about netbsd and local_attach
> doesn't work there anyway.

What is the error behaviour if NULL is returned here ?  I forget the
rest of the patch, but once again we should make sure that we abort if
this situation occurs.

Ian.

Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini]

On Fri, 20 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> > On Tue, 17 Apr 2012, Ian Jackson wrote:
> > > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"):
> > > > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > > > +        if (libxl__xs_read(gc, t,
> > > > +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> > > > +                        dompath, devid)) == NULL)
> > > > +            return libxl__devid_to_vdev(gc, devid);
> > > 
> > > What if the error is not ENOENT ?
> > 
> > we should return NULL
> 
> I don't think that's correct.  If, say, the error is EACCES, then the
> domain creation should be aborted with a message about that, since the
> system has been installed incorrectly.
> 
> Compare this situation with the recent pygrub failure, where
> libfsimage+pygrub turned all errors of the form "something went wrong
> loading this plugin" into "the kernel was not found"; so when a
> completely empty .so was loaded as a plugin the result was not "OMG
> WTF this is totally broken" but "sorry can't find your kernel in this
> filesystem" (when really the problem is that pygrub+libfsimage knew
> that the filesystem was one they were supposed to support but the
> plugin for it was utterly broken).
> 
> This reminds me of our other recent discussion about error handling,
> of receiving unexpected toolstack migration info.  In general any
> unanticipated situation should be treated as a fatal error.  Only
> anticipated situations should result in the software continuing in a
> degraded manner.

NULL is an abort condition and libxl__device_disk_local_attach prints a
useful message.


> > > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > 
> > > There is no clearly defined upper bound on the buffer space needed by
> > > this function.
> > 
> > I know but this function is used as is in Linux where the stack is
> > even smaller. I'll add an upper bound anyway.
> 
> At the very least a comment is needed to demonstrate that it's
> correct, but a bound in the code would be better.  (Also I'm surprised
> that you chose a recursive rather than iterative implementation of a
> what is a base conversion routine...)

OK

> > > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > > > index 9e0ed6d..c8977ac 100644
> > > > --- a/tools/libxl/libxl_netbsd.c
> > > > +++ b/tools/libxl/libxl_netbsd.c
> > > ...
> > > > +char *libxl__devid_to_vdev(libxl__gc *gc, int devid)
> > > > +{
> > > > +    /* TODO */
> > > > +    return NULL;
> > > > +}
> > > 
> > > I guess this is going to be fixed in a future version of the patch ?
> > 
> > I don't think so: I don't know anything about netbsd and local_attach
> > doesn't work there anyway.
> 
> What is the error behaviour if NULL is returned here ?  I forget the
> rest of the patch, but once again we should make sure that we abort if
> this situation occurs.

NULL is returned by libxl__alloc_vdev, then it is the same as before:
eventually the domain creation terminates with an fatal error.

[PATCH v3 6/7] 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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
 tools/hotplug/Linux/init.d/xencommons           |    3 +
 tools/libxl/libxl_internal.c                    |   49 +++++++++++++++++-----
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons
index 6543204..0f3b9ad 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 and log file
+#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_internal.c b/tools/libxl/libxl_internal.c
index ac59aa6..ba7b395 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -550,13 +550,31 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
-                           " attach a qdisk image if the format is not raw");
-                break;
+                xs_transaction_t t;
+                do {
+                    t = xs_transaction_start(ctx->xsh);
+                    /* use 0 as the domid of the toolstack domain for now */
+                    tmpdisk->vdev = libxl__alloc_vdev(gc, 0, blkdev_start, t);
+                    if (tmpdisk->vdev == NULL) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                                "libxl__alloc_vdev failed\n");
+                        xs_transaction_end(ctx->xsh, t, 1);
+                        goto out;
+                    }
+                    if (libxl__device_disk_add_t(gc, 0, t, tmpdisk)) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                                "libxl_device_disk_add failed\n");
+                        xs_transaction_end(ctx->xsh, t, 1);
+                        goto out;
+                    }
+                    rc = xs_transaction_end(ctx->xsh, t, 0);
+                } while (rc == 0 && errno == EAGAIN);
+                dev = libxl__sprintf(gc, "/dev/%s", tmpdisk->vdev);
+            } else {
+                dev = tmpdisk->pdev_path;
             }
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
-                       disk->pdev_path);
-            dev = tmpdisk->pdev_path;
+                       dev);
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
@@ -571,12 +589,21 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk)
 {
-    /* 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.
-     */
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+                libxl_device_disk_remove(gc->owner, 0, disk, 0);
+                return libxl_device_disk_destroy(gc->owner, 0, 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;
+    }
 
     return 0;
 }
-- 
1.7.2.5

Re: [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
...
> +                    t = xs_transaction_start(ctx->xsh);

This transaction should be in the local variables block for the whole
function, and initialised to 0.

> +                    /* use 0 as the domid of the toolstack domain for now */
> +                    tmpdisk->vdev = libxl__alloc_vdev(gc, 0, blkdev_start, t);

Should this 0 be abstracted into a #define or a variable ?

> +                    if (tmpdisk->vdev == NULL) {
> +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                                "libxl__alloc_vdev failed\n");
> +                        xs_transaction_end(ctx->xsh, t, 1);

And then all these xs_transaction_end calls turn into one call in the
exit path.

> +                dev = libxl__sprintf(gc, "/dev/%s", tmpdisk->vdev);

Perhaps you'd like to use GCSPRINTF now that we have it.

>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);

Perhaps you'd like to use LOG(DEBUG, ....") now that we have it ?

> -            dev = tmpdisk->pdev_path;
> +    switch (disk->backend) {
> +        case LIBXL_DISK_BACKEND_QDISK:
> +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {

This replicates the logic earlier, which decided whether to use a
qdisk.  I think it would be better to remember whether we did use a
qdisk and detach it iff so.

Ian.

Re: [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Stefano Stabellini]

Could you please avoid to remove so much context from your quotes? It
makes it very hard for me to read your emails.


On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> ...
> > +                    t = xs_transaction_start(ctx->xsh);
> 
> This transaction should be in the local variables block for the whole
> function, and initialised to 0.

I don't think I understand what you mean.
Do you mean moving xs_transaction_start outside the switch?
If so, why? It doesn't affect many of the other cases.


> > +                    /* use 0 as the domid of the toolstack domain for now */
> > +                    tmpdisk->vdev = libxl__alloc_vdev(gc, 0, blkdev_start, t);
> 
> Should this 0 be abstracted into a #define or a variable ?

Yes, good idea.


> > +                    if (tmpdisk->vdev == NULL) {
> > +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > +                                "libxl__alloc_vdev failed\n");
> > +                        xs_transaction_end(ctx->xsh, t, 1);
> 
> And then all these xs_transaction_end calls turn into one call in the
> exit path.

So maybe you do mean moving the transaction outside the switch.
In that case I disagree: the transaction is only useful in a very
limited set of cases (just one at the moment), while most of the others
don't need it.


> > +                dev = libxl__sprintf(gc, "/dev/%s", tmpdisk->vdev);
> 
> Perhaps you'd like to use GCSPRINTF now that we have it.
> 
> >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > -                       disk->pdev_path);
> 
> Perhaps you'd like to use LOG(DEBUG, ....") now that we have it ?
> 
> > -            dev = tmpdisk->pdev_path;
> > +    switch (disk->backend) {
> > +        case LIBXL_DISK_BACKEND_QDISK:
> > +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> 
> This replicates the logic earlier, which decided whether to use a
> qdisk.  I think it would be better to remember whether we did use a
> qdisk and detach it iff so.
 
There are cases in which we do use qdisk but we don't have to detach.

Re: [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> On Tue, 17 Apr 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > ...
> > > +                    t = xs_transaction_start(ctx->xsh);
> > 
> > This transaction should be in the local variables block for the whole
> > function, and initialised to 0.
> 
> I don't think I understand what you mean.
> Do you mean moving xs_transaction_start outside the switch?
> If so, why? It doesn't affect many of the other cases.

No, I mean that the variable should be defined at the top of the
function and initialised to 0.

In general, any resource allocated in a function other than from the
gc should be held in a variable set and used like "resource" in this
example:

  int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
  {

      foo *resource = 0;
      int rc;

      blah blah blah;

      resource = xmumble_acquire_resource(wibble wombat);
      if (!resource) { rc = ERROR_FAIL; goto out; }

      blah blah blah;

      rc = 0;

    out:
      xmumble_free_resource(resource);
      return rc;
  }

> > > +                    if (tmpdisk->vdev == NULL) {
> > > +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > > +                                "libxl__alloc_vdev failed\n");
> > > +                        xs_transaction_end(ctx->xsh, t, 1);
> > 
> > And then all these xs_transaction_end calls turn into one call in the
> > exit path.
> 
> So maybe you do mean moving the transaction outside the switch.
> In that case I disagree: the transaction is only useful in a very
> limited set of cases (just one at the moment), while most of the others
> don't need it.

No, I mean only the calls which abort the transaction should be in the
"goto out" section.

So:

  int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
  {

      xs_transaction_t our_trans;
      int rc;

      blah blah blah;

      for (;;) {
          our_trans = xs_transaction_start(...);
          if (!our_trans) { rc = ERROR_FAIL; goto out; }

          blah blah blah;

          r = xs_transaction_end(CTX->xsh, our_trans, 0);
          our_trans = 0; /* it's ended either way */
          if (r shows it was successful) break;
          if (r not as expected) { rc = ERROR_FAIL; goto out; }
      }

      blah blah blah;

      rc = 0;

    out:
      if (our_trans) xs_transaction_end(CTX->xsh, our_trans, 1);
      return rc;
  }

The point is that the following invariant is maintained: we have a
transaction open, which needs to be ended, iff !!our_trans.

> > > -            dev = tmpdisk->pdev_path;
> > > +    switch (disk->backend) {
> > > +        case LIBXL_DISK_BACKEND_QDISK:
> > > +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> > 
> > This replicates the logic earlier, which decided whether to use a
> > qdisk.  I think it would be better to remember whether we did use a
> > qdisk and detach it iff so.
>  
> There are cases in which we do use qdisk but we don't have to detach.

I'm not sure I follow.  This is the attachment of the VM's disk for
the benefit of the bootloader.  When we are done with running the
bootloader we need to clean it up.

Ian.

Re: [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach

[Stefano Stabellini]

On Fri, 20 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > On Tue, 17 Apr 2012, Ian Jackson wrote:
> > > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 6/7] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > > ...
> > > > +                    t = xs_transaction_start(ctx->xsh);
> > > 
> > > This transaction should be in the local variables block for the whole
> > > function, and initialised to 0.
> > 
> > I don't think I understand what you mean.
> > Do you mean moving xs_transaction_start outside the switch?
> > If so, why? It doesn't affect many of the other cases.
> 
> No, I mean that the variable should be defined at the top of the
> function and initialised to 0.
> 
> In general, any resource allocated in a function other than from the
> gc should be held in a variable set and used like "resource" in this
> example:
> 
>   int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
>   {
> 
>       foo *resource = 0;
>       int rc;
> 
>       blah blah blah;
> 
>       resource = xmumble_acquire_resource(wibble wombat);
>       if (!resource) { rc = ERROR_FAIL; goto out; }
> 
>       blah blah blah;
> 
>       rc = 0;
> 
>     out:
>       xmumble_free_resource(resource);
>       return rc;
>   }

After reading the rest of this email, I can see why this would be a
useful change to make.


> > > > +                    if (tmpdisk->vdev == NULL) {
> > > > +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > > > +                                "libxl__alloc_vdev failed\n");
> > > > +                        xs_transaction_end(ctx->xsh, t, 1);
> > > 
> > > And then all these xs_transaction_end calls turn into one call in the
> > > exit path.
> > 
> > So maybe you do mean moving the transaction outside the switch.
> > In that case I disagree: the transaction is only useful in a very
> > limited set of cases (just one at the moment), while most of the others
> > don't need it.
> 
> No, I mean only the calls which abort the transaction should be in the
> "goto out" section.
> 
> So:
> 
>   int libxl__do_something_requiring_xmumble_resource(libxl__gc *gc, flibble)
>   {
> 
>       xs_transaction_t our_trans;
>       int rc;
> 
>       blah blah blah;
> 
>       for (;;) {
>           our_trans = xs_transaction_start(...);
>           if (!our_trans) { rc = ERROR_FAIL; goto out; }
> 
>           blah blah blah;
> 
>           r = xs_transaction_end(CTX->xsh, our_trans, 0);
>           our_trans = 0; /* it's ended either way */
>           if (r shows it was successful) break;
>           if (r not as expected) { rc = ERROR_FAIL; goto out; }
>       }
> 
>       blah blah blah;
> 
>       rc = 0;
> 
>     out:
>       if (our_trans) xs_transaction_end(CTX->xsh, our_trans, 1);
>       return rc;
>   }
> 
> The point is that the following invariant is maintained: we have a
> transaction open, which needs to be ended, iff !!our_trans.

OK


> > > > -            dev = tmpdisk->pdev_path;
> > > > +    switch (disk->backend) {
> > > > +        case LIBXL_DISK_BACKEND_QDISK:
> > > > +            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> > > 
> > > This replicates the logic earlier, which decided whether to use a
> > > qdisk.  I think it would be better to remember whether we did use a
> > > qdisk and detach it iff so.
> >  
> > There are cases in which we do use qdisk but we don't have to detach.
> 
> I'm not sure I follow.  This is the attachment of the VM's disk for
> the benefit of the bootloader.  When we are done with running the
> bootloader we need to clean it up.
 
Sorry I didn't understand what you meant earlier.
I'll use another why to figure out if we need to remove the disk
that is not dependent on the format or the policy that we used earlier.

[PATCH v3 7/7] 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>
---
 tools/libxl/libxl_internal.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ba7b395..c955bf4 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -504,8 +504,9 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
-    char *dev = NULL;
+    char *dev = NULL, *be_path = NULL;
     int rc;
+    libxl__device device;
     libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
     if (tmpdisk == NULL) goto out;
 
@@ -581,6 +582,19 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
                 "type: %d", tmpdisk->backend);
             break;
     }
+    if (tmpdisk->vdev != NULL) {
+        rc = libxl__device_from_disk(gc, 0, tmpdisk, &device);
+        if (rc < 0) {
+            libxl__device_disk_local_detach(gc, tmpdisk);
+            return NULL;
+        }
+        be_path = libxl__device_backend_path(gc, &device);
+        rc = libxl__wait_for_backend(gc, be_path, "4");
+        if (rc < 0) {
+            libxl__device_disk_local_detach(gc, tmpdisk);
+            return NULL;
+        }
+    }
 
  out:
     return dev;
-- 
1.7.2.5

Re: [PATCH v3 7/7] libxl__device_disk_local_attach: wait for state "connected"

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v3 7/7] libxl__device_disk_local_attach: wait for state "connected""):
> 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.
...
> +        be_path = libxl__device_backend_path(gc, &device);
> +        rc = libxl__wait_for_backend(gc, be_path, "4");

Ideally we shouldn't be introducing new calls to functions called
"libxl__wait_for".  Although I appreciate you may not want to convert
libxl__device_disk_local_attach and all its callers to use callbacks.

> +        if (rc < 0) {
> +            libxl__device_disk_local_detach(gc, tmpdisk);
> +            return NULL;

Again, this is the "free something on every exit path" error handling
style.  Surely we should have a "goto out" or "goto out_err" or
something ?

Ian.

Re: [PATCH v3 7/7] libxl__device_disk_local_attach: wait for state "connected"

[Stefano Stabellini]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 7/7] libxl__device_disk_local_attach: wait for state "connected""):
> > 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.
> ...
> > +        be_path = libxl__device_backend_path(gc, &device);
> > +        rc = libxl__wait_for_backend(gc, be_path, "4");
> 
> Ideally we shouldn't be introducing new calls to functions called
> "libxl__wait_for".  Although I appreciate you may not want to convert
> libxl__device_disk_local_attach and all its callers to use callbacks.

Indeed.

> > +        if (rc < 0) {
> > +            libxl__device_disk_local_detach(gc, tmpdisk);
> > +            return NULL;
> 
> Again, this is the "free something on every exit path" error handling
> style.  Surely we should have a "goto out" or "goto out_err" or
> something ?
 
Yep, I'll do.