thr3ads.net - Xen devel - [PATCH v4 0/8] qdisk local attach [Apr 2012]

If this information is useful, please help other people find it:
Share via:

Stefano Stabellini

2012-Apr-24 10:45 UTC

[PATCH v4 0/8] qdisk local attach

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


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


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



Stefano Stabellini (8):
      libxl: make libxl_device_disk_local_attach/detach internal functions
      libxl: libxl__device_disk_local_attach return a new libxl_device_disk
      libxl: add a transaction parameter to libxl__device_generic_add
      libxl: 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                             |    7 -
 tools/libxl/libxl_bootloader.c                  |   11 +-
 tools/libxl/libxl_create.c                      |    3 +
 tools/libxl/libxl_device.c                      |   17 +-
 tools/libxl/libxl_internal.c                    |  316 +++++++++++++++++++++++
 tools/libxl/libxl_internal.h                    |   25 ++-
 tools/libxl/libxl_linux.c                       |   45 ++++
 tools/libxl/libxl_netbsd.c                      |    6 +
 tools/libxl/libxl_pci.c                         |    2 +-
 tools/libxl/libxl_types.idl                     |    1 +
 tools/libxl/xl.c                                |    3 +
 tools/libxl/xl.h                                |    1 +
 tools/libxl/xl_cmdimpl.c                        |    2 +
 17 files changed, 432 insertions(+), 248 deletions(-)

Cheers,

Stefano

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions

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 |    4 +-
 tools/libxl/libxl_internal.c   |   72 +++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h   |    9 +++++
 5 files changed, 83 insertions(+), 82 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..977c6d3 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -386,7 +386,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);
     if (!diskpath) {
         goto out_close;
     }
@@ -453,7 +453,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     rc = 0;
 out_close:
     if (diskpath) {
-        libxl_device_disk_local_detach(ctx, disk);
+        libxl__device_disk_local_detach(gc, disk);
         free(diskpath);
     }
     if (fifo_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b89aef7..1f428b2 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,78 @@ out:
     return rc;
 }
 
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk
*disk)
+{
+    libxl_ctx *ctx = gc->owner;
+    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);
+    return ret;
+}
+
+_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..6927aef 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1001,6 +1001,15 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
  */
 _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
 
+/*
+ * Make a disk available in this (the control) domain. Returns path to
+ * a device.
+ */
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+        libxl_device_disk *disk);
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
+        libxl_device_disk *disk);
+
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 
 struct libxl__xen_console_reader {
-- 
1.7.2.5

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Introduce a new libxl_device_disk** parameter to
libxl__device_disk_local_attach, the parameter is allocated 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_bootloader.c |   10 ++++----
 tools/libxl/libxl_internal.c   |   47 +++++++++++++++++++++++----------------
 tools/libxl/libxl_internal.h   |    3 +-
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 977c6d3..83cac78 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,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
-    diskpath = libxl__device_disk_local_attach(gc, disk);
+    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
     if (!diskpath) {
+        rc = ERROR_FAIL;
         goto out_close;
     }
 
@@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
     rc = 0;
 out_close:
-    if (diskpath) {
-        libxl__device_disk_local_detach(gc, 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 1f428b2..3af77e7 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,64 +323,73 @@ out:
     return rc;
 }
 
-_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk
*disk)
+_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;
-    char *ret = NULL;
     int rc;
-
-    rc = libxl__device_disk_setdefault(gc, disk);
+    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 (disk->backend) {
+    switch (tmpdisk->backend) {
         case LIBXL_DISK_BACKEND_PHY:
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
%s",
-                       disk->pdev_path);
-            dev = disk->pdev_path;
+                       tmpdisk->pdev_path);
+            dev = tmpdisk->pdev_path;
             break;
         case LIBXL_DISK_BACKEND_TAP:
-            switch (disk->format) {
+            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)",
-                           disk->pdev_path);
-                dev = disk->pdev_path;
+                           tmpdisk->pdev_path);
+                dev = tmpdisk->pdev_path;
                 break;
             case LIBXL_DISK_FORMAT_VHD:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path,
-                                            disk->format);
+                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",
disk->format);
+                           "unrecognized disk format: %d",
tmpdisk->format);
                 break;
             }
             break;
         case LIBXL_DISK_BACKEND_QDISK:
-            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+            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 = disk->pdev_path;
+            dev = tmpdisk->pdev_path;
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
"
-                "type: %d", disk->backend);
+                "type: %d", tmpdisk->backend);
             break;
     }
 
  out:
-    if (dev != NULL)
-        ret = strdup(dev);
-    return ret;
+    return dev;
 }
 
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk
*disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6927aef..2f1cf0f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1006,7 +1006,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc,
char *be_path);
  * a device.
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
-        libxl_device_disk *disk);
+        const libxl_device_disk *disk,
+        libxl_device_disk **new_disk);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
-- 
1.7.2.5

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 3/8] 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.

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

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 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..88cdc7d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
     return libxl__device_kind_from_string(strkind, &dev->backend_kind);
 }
 
-int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
-                             char **bents, char **fents)
+int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+        libxl__device *device, char **bents, char **fents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *frontend_path, *backend_path;
-    xs_transaction_t t;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions backend_perms[2];
+    int create_transaction = t == XBT_NULL;
 
     frontend_path = libxl__device_frontend_path(gc, device);
     backend_path = libxl__device_backend_path(gc, device);
@@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device
*device,
     backend_perms[1].perms = XS_PERM_READ;
 
 retry_transaction:
-    t = xs_transaction_start(ctx->xsh);
+    if (create_transaction)
+        t = xs_transaction_start(ctx->xsh);
     /* FIXME: read frontend_path and check state before removing stuff */
 
     if (fents) {
@@ -100,13 +101,17 @@ retry_transaction:
         libxl__xs_writev(gc, t, backend_path, bents);
     }
 
+    if (!create_transaction)
+        return 0;
+
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
-        else
+        else {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction
failed");
+            return ERROR_FAIL;
+        }
     }
-
     return 0;
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2f1cf0f..00856e0 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

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 4/8] 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.
No functional change.

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 3af77e7..f348529 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 00856e0..2708cc5 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

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 5/8] xl/libxl: add a blkdev_start parameter

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

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

Changes in v4:
- pass blkdev_start in libxl_domain_build_info.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/examples/xl.conf         |    3 +++
 tools/libxl/libxl_bootloader.c |    3 ++-
 tools/libxl/libxl_create.c     |    3 +++
 tools/libxl/libxl_internal.c   |    3 ++-
 tools/libxl/libxl_internal.h   |    3 ++-
 tools/libxl/libxl_types.idl    |    1 +
 tools/libxl/xl.c               |    3 +++
 tools/libxl/xl.h               |    1 +
 tools/libxl/xl_cmdimpl.c       |    2 ++
 9 files changed, 19 insertions(+), 3 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_bootloader.c b/tools/libxl/libxl_bootloader.c
index 83cac78..123b06e 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -387,7 +387,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,
+            info->blkdev_start);
     if (!diskpath) {
         rc = ERROR_FAIL;
         goto out_close;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f9c2a76..5b97047 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -100,6 +100,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         }
     }
 
+    if (b_info->blkdev_start == NULL)
+        b_info->blkdev_start = strdup("xvda");
+
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!b_info->u.hvm.bios)
             switch (b_info->device_model_version) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f348529..f467572 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 2708cc5..97775fb 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/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5cf9708..24d2322 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -242,6 +242,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
     ("localtime",       libxl_defbool),
     ("disable_migrate", libxl_defbool),
     ("cpuid",           libxl_cpuid_policy_list),
+    ("blkdev_start",    string),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..4596c4f 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;
 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..12a0934 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -585,6 +585,8 @@ static void parse_config_data(const char
*configfile_filename_report,
 
     libxl_domain_build_info_init(b_info);
     libxl_domain_build_info_init_type(b_info, c_info->type);
+    if (blkdev_start)
+        b_info->blkdev_start = strdup(blkdev_start);
 
     /* the following is the actual config parsing with overriding values in the
structures */
     if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
-- 
1.7.2.5

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

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

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

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

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f467572..ac73070 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,28 @@ 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) {
+            if (errno == ENOENT)
+                return libxl__devid_to_localdev(gc, devid);
+            else
+                return NULL;
+        }
+        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 97775fb..ed145ab 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_localdev(libxl__gc *gc, int devid);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..cb596a9 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,48 @@ 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))
+
+/* same as in Linux */
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+    if (n >= 26)
+        ptr = encode_disk_name(ptr, n / 26 - 1);
+    *ptr = ''a'' + n % 26;
+    return ptr + 1;
+}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+    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;
+
+	/* arbitrary upper bound */
+	if (offset > 676)
+		return NULL;
+
+    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..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
 
     return 0;
 }
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+    /* TODO */
+    return NULL;
+}
-- 
1.7.2.5

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach

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

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


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

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                    |   63 +++++++++++++++++++----
 tools/libxl/libxl_internal.h                    |    2 +
 4 files changed, 60 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 ac73070..bb75491 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -510,6 +510,7 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc
*gc,
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
     int rc;
+    xs_transaction_t t = XBT_NULL;
     libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
     if (tmpdisk == NULL) goto out;
 
@@ -554,13 +555,40 @@ _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;
+                do {
+                    t = xs_transaction_start(ctx->xsh);
+                    if (t == XBT_NULL) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                                "failed to start a xenstore
transaction");
+                        goto out;
+                    }
+                    tmpdisk->vdev = libxl__alloc_vdev(gc,
+                            LIBXL_TOOLSTACK_DOMID, blkdev_start, t);
+                    if (tmpdisk->vdev == NULL) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                                "libxl__alloc_vdev failed\n");
+                        goto out;
+                    }
+                    if (libxl__device_disk_add_t(gc, LIBXL_TOOLSTACK_DOMID,
+                                t, tmpdisk)) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                                "libxl_device_disk_add failed\n");
+                        goto out;
+                    }
+                    rc = xs_transaction_end(ctx->xsh, t, 0);
+                } while (rc == 0 && errno == EAGAIN);
+                t = XBT_NULL;
+                if (rc == 0) {
+                    LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                            "xenstore transaction failed");
+                    goto out;
+                }
+                dev = GCSPRINTF("/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
"
@@ -569,17 +597,30 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc
*gc,
     }
 
  out:
+    if (t != XBT_NULL)
+        xs_transaction_end(ctx->xsh, t, 1);
     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.
-     */
+    switch (disk->backend) {
+        case LIBXL_DISK_BACKEND_QDISK:
+            if (disk->vdev != NULL) {
+                libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
+                        disk, 0);
+                return libxl_device_disk_destroy(gc->owner,
+                        LIBXL_TOOLSTACK_DOMID, disk);
+            }
+            break;
+        default:
+            /*
+             * Nothing to do for PHYSTYPE_PHY.
+             * For other device types assume that the blktap2 process is
+             * needed by the soon to be started domain and do nothing.
+             */
+            break;
+    }
 
     return 0;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ed145ab..dfd7b49 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -77,6 +77,8 @@
 #define LIBXL_PV_EXTRA_MEMORY 1024
 #define LIBXL_HVM_EXTRA_MEMORY 2048
 #define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
 #define QEMU_SIGNATURE "DeviceModelRecord0002"
 #define STUBDOM_CONSOLE_LOGGING 0
 #define STUBDOM_CONSOLE_SAVE 1
-- 
1.7.2.5

Stefano Stabellini

2012-Apr-24 10:45 UTC

head link

[PATCH v4 8/8] 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.

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

Changes in v4:
- improve exit paths.

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 bb75491..98a67d6 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -508,8 +508,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;
     xs_transaction_t t = XBT_NULL;
     libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
     if (tmpdisk == NULL) goto out;
@@ -596,10 +597,23 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc
*gc,
             break;
     }
 
+    if (tmpdisk->vdev != NULL) {
+        rc = libxl__device_from_disk(gc, 0, tmpdisk, &device);
+        if (rc < 0)
+            goto out_wait_err;
+        be_path = libxl__device_backend_path(gc, &device);
+        rc = libxl__wait_for_backend(gc, be_path, "4");
+        if (rc < 0)
+            goto out_wait_err;
+    }
+
  out:
     if (t != XBT_NULL)
         xs_transaction_end(ctx->xsh, t, 1);
     return dev;
+ out_wait_err:
+    libxl__device_disk_local_detach(gc, tmpdisk);
+    return NULL;
 }
 
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk
*disk)
-- 
1.7.2.5

Ian Jackson

2012-Apr-24 14:52 UTC

head link

Re: [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 5/8] xl/libxl: add a
blkdev_start parameter"):> Introduce a blkdev_start in xl.conf and a corresponding string in
> libxl_domain_build_info.
...> +        libxl_device_disk **new_disk,
> +        char *blkdev_start)
Shouldn''t this be  const char *  ?

You should mention in the commit message that in this patch the value
is still ignored - ie, it will be used later.

Ian.

Ian Jackson

2012-Apr-24 14:58 UTC

head link

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl: introduce
libxl__alloc_vdev"):> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...> +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> +        char *blkdev_start, xs_transaction_t t)
> +{
If this function is ever called with domid != our own, this will
malfunction, because ...
> +            if (errno == ENOENT)
> +                return libxl__devid_to_localdev(gc, devid);
... libxl__devid_to_localdev only answers the question about the
current domain.

This needs to be mentioned in a documentation comment by the function,
at the very least.  Does your series invoke it with a domid other than
our own ?
> +            else
> +                return NULL;
> +        }
> +        vdev[strlen(vdev) - 1]++;
> +    } while (vdev[strlen(vdev) - 1] <= ''z'');
There is a scaling limit here of not starting more than 26 domains
simultaneously.  Is that acceptable ?  I''m tempted to suggest not.
Note that "simultaneously" includes the case where all 27 of them are
simply sat waiting for someone to press "return" on a pygrub prompt.

Ian.

Ian Jackson

2012-Apr-24 15:02 UTC

head link

Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 2/8] libxl:
libxl__device_disk_local_attach return a new libxl_device_disk"):
...> +_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;
> -    char *ret = NULL;
>      int rc;
> -
> -    rc = libxl__device_disk_setdefault(gc, disk);
> +    libxl_device_disk *tmpdisk = libxl__zalloc(gc,
sizeof(libxl_device_disk));
> +    if (tmpdisk == NULL) goto out;
libxl__zalloc is guaranteed not to fail, nowadays.
> -    switch (disk->backend) {
> +    switch (tmpdisk->backend) {
Did you see my comment about arranging for "disk" to be the new
structure and renaming the formal parameter ?  That would (a) remove a
bunch of useless stuff from the diff (b) probably make the code
clearer anyway, since nothing inside this function should be using the
actually-passed libxl_device_disk* other than the code whose purpose
is to make a copy of it.

If you resend with that change I will find it much easier to review
this.

Ian.

Ian Campbell

2012-Apr-24 15:04 UTC

head link

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

On Tue, 2012-04-24 at 15:58 +0100, Ian Jackson wrote:> Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl:
introduce libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> > +        char *blkdev_start, xs_transaction_t t)
> > +{
> 
> If this function is ever called with domid != our own, this will
> malfunction, because ...
> 
> > +            if (errno == ENOENT)
> > +                return libxl__devid_to_localdev(gc, devid);
> 
> ... libxl__devid_to_localdev only answers the question about the
> current domain.
> 
> This needs to be mentioned in a documentation comment by the function,
> at the very least.  Does your series invoke it with a domid other than
> our own ?
> 
> > +            else
> > +                return NULL;
> > +        }
> > +        vdev[strlen(vdev) - 1]++;
> > +    } while (vdev[strlen(vdev) - 1] <= ''z'');
> 
> There is a scaling limit here of not starting more than 26 domains
> simultaneously.  Is that acceptable ?  I''m tempted to suggest not.
While I agree we also need to start being pragmatic about ever getting
4.2 out the door...

If it''s a trivial job to support aa, ab etc then lets do it, if not
then
lets leave it for 4.3?
> Note that "simultaneously" includes the case where all 27 of them
are
> simply sat waiting for someone to press "return" on a pygrub
prompt.
> 
> Ian.

Ian Jackson

2012-Apr-24 15:16 UTC

head link

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

Ian Campbell writes ("Re: [Xen-devel] [PATCH v4 6/8] libxl: introduce
libxl__alloc_vdev"):> On Tue, 2012-04-24 at 15:58 +0100, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl:
introduce libxl__alloc_vdev"):
> > > +    } while (vdev[strlen(vdev) - 1] <=
''z'');
> > 
> > There is a scaling limit here of not starting more than 26 domains
> > simultaneously.  Is that acceptable ?  I''m tempted to suggest
not.
> 
> While I agree we also need to start being pragmatic about ever getting
> 4.2 out the door...
> 
> If it''s a trivial job to support aa, ab etc then lets do it, if
not then
> lets leave it for 4.3?
The problem is because the code is trying to avoid too much knowledge
of the devid scheme; in particular, it''s trying to avoid introducing
the inverse function to libxl__device_disk_dev_number.  Although it
/does/ depend intimately on the details of
libxl__device_disk_dev_number.

It is arguably a bug that libxl__device_disk_dev_number combines the
parsing of vdev strings with the composition of disk/partition numbers
into devids.  But we can avoid needing to decompose it by passing
libxl__device_disk_dev_number a format which is easier to create than
one with a base-26-encoded disk number.

What I would do is:
  * call libxl__device_disk_dev_number once on blkdev_start with
    non-null arguments for pdisk and ppartition; check that
    the partition is 0.
  * loop incrementing the disk value
  * on each iteration,
    - generate a vdev = GCSPRINTF("d%d", disk);
    - use libxl__device_disk_dev_number on that vdev to get the devid
    - check whether this devid is available

Ian.

Ian Campbell

2012-Apr-24 15:16 UTC

head link

Re: [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions

On Tue, 2012-04-24 at 11:45 +0100, Stefano Stabellini wrote:

No functional change other than ctx->gc?
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index b89aef7..1f428b2 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,6 +323,78 @@ out:
>      return rc;
>  }
>  
> +_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
Strictly the hidden is only needed on the prototype. I guess there''s no
harm having it here too (but you know what linkers are like...)
> +{
[...]> +}
Looking at the callers this could possibly be a gc''d string, and
perhaps
const in the return here. That''s for another day though I think.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian Jackson

2012-Apr-24 15:18 UTC

head link

Re: [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected"

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 8/8]
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.
...> Changes in v4:
> - improve exit paths.
...> +    if (tmpdisk->vdev != NULL) {
> +        rc = libxl__device_from_disk(gc, 0, tmpdisk, &device);
> +        if (rc < 0)
> +            goto out_wait_err;
This still seems to have two error exit paths:
>   out:
>      if (t != XBT_NULL)
>          xs_transaction_end(ctx->xsh, t, 1);
>      return dev;
> + out_wait_err:
> +    libxl__device_disk_local_detach(gc, tmpdisk);
> +    return NULL;
Previously "out" was both a success and error exit path AFAICT.

I really don''t care very much whether you want to combine the success
and error exit paths.  But there should be only one error exit path.
So please no "goto out_<some particular thing>_err".

Ian.

Ian Campbell

2012-Apr-24 15:25 UTC

head link

Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

On Tue, 2012-04-24 at 11:45 +0100, Stefano Stabellini
wrote:> 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_bootloader.c |   10 ++++----
>  tools/libxl/libxl_internal.c   |   47
+++++++++++++++++++++++----------------
>  tools/libxl/libxl_internal.h   |    3 +-
>  3 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/libxl/libxl_bootloader.c
b/tools/libxl/libxl_bootloader.c
> index 977c6d3..83cac78 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,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>          goto out_close;
>      }
>  
> -    diskpath = libxl__device_disk_local_attach(gc, disk);
> +    diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
>      if (!diskpath) {
> +        rc = ERROR_FAIL;
>          goto out_close;
>      }
>  
> @@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>  
>      rc = 0;
>  out_close:
> -    if (diskpath) {
> -        libxl__device_disk_local_detach(gc, 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 1f428b2..3af77e7 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,64 +323,73 @@ out:
>      return rc;
>  }
>  
> -_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
> +_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;
> -    char *ret = NULL;
>      int rc;
> -
> -    rc = libxl__device_disk_setdefault(gc, disk);
> +    libxl_device_disk *tmpdisk = libxl__zalloc(gc,
sizeof(libxl_device_disk));
> +    if (tmpdisk == NULL) goto out;
> +
> +    *new_disk = tmpdisk;
It would be good practice to not do this until we have succeeded, or do
you expect the caller to clean this up on error, even if the disk is
half constructed? I''d suggest that it ought be up to this function to
unwind on failure not the called.

tmpdisk isn''t actually temporary, it''s actually the result of
this
function and lives for a fair while afterwards, perhaps loopdisk? or
dom0disk? or was this the patch where IanJ suggested renaming the param
disk (e.g. to guest_disk) and then using disk for this one? (That''s
actually a good idea)
> +    memcpy(tmpdisk, disk, sizeof(libxl_device_disk));
You don''t actually need zalloc if you immediately memcpy over the whole
thing. Minor thing though.
> +    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 (disk->backend) {
> +    switch (tmpdisk->backend) {
>          case LIBXL_DISK_BACKEND_PHY:
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY
disk %s",
> -                       disk->pdev_path);
> -            dev = disk->pdev_path;
> +                       tmpdisk->pdev_path);
> +            dev = tmpdisk->pdev_path;
>              break;
>          case LIBXL_DISK_BACKEND_TAP:
> -            switch (disk->format) {
> +            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)",
> -                           disk->pdev_path);
> -                dev = disk->pdev_path;
> +                           tmpdisk->pdev_path);
> +                dev = tmpdisk->pdev_path;
>                  break;
>              case LIBXL_DISK_FORMAT_VHD:
> -                dev = libxl__blktap_devpath(gc, disk->pdev_path,
> -                                            disk->format);
> +                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",
disk->format);
> +                           "unrecognized disk format: %d",
tmpdisk->format);
>                  break;
>              }
>              break;
>          case LIBXL_DISK_BACKEND_QDISK:
> -            if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> +            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 = disk->pdev_path;
> +            dev = tmpdisk->pdev_path;
>              break;
>          default:
>              LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
> -                "type: %d", disk->backend);
> +                "type: %d", tmpdisk->backend);
>              break;
>      }
>  
>   out:
> -    if (dev != NULL)
> -        ret = strdup(dev);
> -    return ret;
> +    return dev;
>  }
>  
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
libxl_device_disk *disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6927aef..2f1cf0f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1006,7 +1006,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc
*gc, char *be_path);
>   * a device.
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> -        libxl_device_disk *disk);
> +        const libxl_device_disk *disk,
> +        libxl_device_disk **new_disk);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>

Ian Jackson

2012-Apr-24 15:34 UTC

head link

Re: [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 3/8] 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.
...> -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;
This works.

Another way to do it is to have a separate variable,
something like this:
   xs_transaction_t our_transaction = t;
and then later
   if (!t) {
       our_transaction = t = xs_transaction_start....
       if (!t) error handling;
   }
and when you commit it set our_transaction to 0 and on error exit
   if (our_transaction)
       xs_transaction_end(..., our_transaction, 1);

I suggest this just because you may prefer to avoid separate boolean
sentinel variables - I know I do.


There is a difficulty in general with this function, which is that it
contains an enormous number of unchecked xenstore operations.  I''m not
saying you need to fix this now, but if at a later point this were to
be fixed, the function would need to get a proper error exit path
which would have to destroy the transaction iff it was created.

I mention this for completeness but you may want to transpose it into
a comment somehow.


Anyway,

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

Ian Jackson

2012-Apr-24 15:39 UTC

head link

Re: [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 7/8] xl/libxl: implement
QDISK libxl_device_disk_local_attach"):> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index ac73070..bb75491 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -510,6 +510,7 @@ _hidden char *
libxl__device_disk_local_attach(libxl__gc *gc,
...> +                    if (tmpdisk->vdev == NULL) {
> +                        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                                "libxl__alloc_vdev failed\n");
LIBXL__LOG doesn''t need an additional "\n".  You have several
of
these.  Also you may prefer my new LOG convenience macro which is a
lot less verbose - look, no wrapping needed:
                           LOG(ERROR, "libxl__alloc_vdev failed);
> +    switch (disk->backend) {
> +        case LIBXL_DISK_BACKEND_QDISK:
> +            if (disk->vdev != NULL) {
> +                libxl_device_disk_remove(gc->owner,
LIBXL_TOOLSTACK_DOMID,
> +                        disk, 0);
> +                return libxl_device_disk_destroy(gc->owner,
> +                        LIBXL_TOOLSTACK_DOMID, disk);
> +            }
> +            break;
> +        default:
> +            /*
> +             * Nothing to do for PHYSTYPE_PHY.
> +             * For other device types assume that the blktap2 process is
> +             * needed by the soon to be started domain and do nothing.
> +             */
> +            break;
I guess what I meant with my previous comment is that it might be
better to have _local_attach return some kind of context/state struct,
bigger than the libxl__device_disk*, that would be passed to _detach.

But this will do.

Ian.

Ian Jackson

2012-Apr-24 15:42 UTC

head link

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t

Stefano Stabellini writes ("[Xen-devel] [PATCH v4 4/8] libxl: introduce
libxl__device_disk_add_t"):> Introduce libxl__device_disk_add_t that takes an additional
> xs_transaction_t paramter.
Names ending "_t" are reserved for the implementation.  But you can
just call the internal function libxl__device_disk_add.  I think it''s
fine to internal and external functions which differ only in _ and
parameters, provided that there is no nontrivial functionality in the
external function.

Apart from that it''s rather hard to review because this patch seems to
have an awful lot of code motion mixed up with other changes.

Ian.

Ian Jackson

2012-Apr-24 16:02 UTC

head link

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t

Ian Jackson writes ("Re: [Xen-devel] [PATCH v4 4/8] libxl: introduce
libxl__device_disk_add_t"):> Stefano Stabellini writes ("[Xen-devel] [PATCH v4 4/8] libxl:
introduce libxl__device_disk_add_t"):
> > Introduce libxl__device_disk_add_t that takes an additional
> > xs_transaction_t paramter.
> 
> Names ending "_t" are reserved for the implementation.
Sorry, I was unclear.  I meant the C language implementation.  So
libxl may not introduce anything called "..._t".
>  But you can just call the internal function libxl__device_disk_add.
> I think it''s fine to internal and external functions which differ
> only in _ and parameters, provided that there is no nontrivial
> functionality in the external function.
> 
> Apart from that it''s rather hard to review because this patch
seems to
> have an awful lot of code motion mixed up with other changes.
Ian.

Ian Jackson

2012-May-04 16:30 UTC

head link

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t [and 1 more messages]

Stefano Stabellini writes ("[PATCH v5 4/8] libxl: introduce
libxl__device_disk_add"):> Introduce libxl__device_disk_add that takes an additional
> xs_transaction_t paramter.
> Implement libxl_device_disk_add using libxl__device_disk_add.
> Move libxl__device_from_disk to libxl_internal.c.
> No functional change.
The last time you posted this I wrote:

Ian Jackson writes ("Re: [Xen-devel] [PATCH v4 4/8] libxl: introduce
libxl__device_disk_add_t"):> Apart from that it''s rather hard to review because this patch
seems to
> have an awful lot of code motion mixed up with other changes.
Can you split the code motion out into a separate patch, so that we
can see what has changed in the code that is being moved ?

Also I think "no functional change" is stretching it a bit unless what
you mean is "no callers of libxl__device_disk_add yet".

Ian.

Xen devel - Apr 2012 - [PATCH v4 0/8] qdisk local attach

[PATCH v4 0/8] qdisk local attach

[PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions

[PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add

[PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t

[PATCH v4 5/8] xl/libxl: add a blkdev_start parameter

[PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

[PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach

[PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected"

Re: [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev

Re: [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions

Re: [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected"

Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

Re: [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add

Re: [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t

Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t [and 1 more messages]