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

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

Stefano Stabellini

2012-May-04 11:12 UTC

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


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


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


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


Stefano Stabellini (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
      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                    |  319 +++++++++++++++++++++++
 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, 435 insertions(+), 248 deletions(-)

Cheers,

Stefano

Stefano Stabellini

2012-May-04 11:13 UTC

head link

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

Changes in v5:

- remove _hidden from the implementation.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 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 9984d46..1357efc 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 d59f0ee..79d5679 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -619,13 +619,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..fc771ff 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,78 @@ out:
     return rc;
 }
 
+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;
+}
+
+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 34ea15c..ddd6a37 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1003,6 +1003,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-May-04 11:13 UTC

head link

[PATCH v5 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.

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_bootloader.c |   10 +++++-----
 tools/libxl/libxl_internal.c   |   20 ++++++++++++++------
 tools/libxl/libxl_internal.h   |    3 ++-
 3 files changed, 21 insertions(+), 12 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 fc771ff..55dc55c 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,12 +323,21 @@ out:
     return rc;
 }
 
-char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc,
+        const libxl_device_disk *in_disk,
+        libxl_device_disk **new_disk)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
-    char *ret = NULL;
     int rc;
+    libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
+
+    memcpy(disk, in_disk, sizeof(libxl_device_disk));
+    if (in_disk->pdev_path != NULL)
+        disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
+    if (in_disk->script != NULL)
+        disk->script = libxl__strdup(gc, in_disk->script);
+    disk->vdev = NULL;
 
     rc = libxl__device_disk_setdefault(gc, disk);
     if (rc) goto out;
@@ -368,7 +377,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
                 break;
             }
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s\n",
-                       disk->pdev_path);
+                       in_disk->pdev_path);
             dev = disk->pdev_path;
             break;
         default:
@@ -378,9 +387,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
     }
 
  out:
-    if (dev != NULL)
-        ret = strdup(dev);
-    return ret;
+    *new_disk = disk;
+    return dev;
 }
 
 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 ddd6a37..965d13b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1008,7 +1008,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc,
char *be_path);
  * a device.
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
-        libxl_device_disk *disk);
+        const libxl_device_disk *in_disk,
+        libxl_device_disk **new_disk);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
-- 
1.7.2.5

Stefano Stabellini

2012-May-04 11:13 UTC

head link

[PATCH v5 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.

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

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 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 1357efc..525f0d6 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));
 
@@ -1802,7 +1802,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));
 
@@ -2080,7 +2080,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;
@@ -2151,7 +2151,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;
@@ -2284,7 +2284,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 965d13b..2c8f06a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -683,8 +683,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 3856bd9..335c645 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-May-04 11:13 UTC

head link

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

Changes in v5:
- rename libxl__device_generic_add_t to libxl__device_generic_add.

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 525f0d6..941dbb9 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(gc, domid, XBT_NULL, disk);
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 55dc55c..1bf5d73 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,163 @@ out:
     return rc;
 }
 
+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__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;
+}
+
 char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
         libxl_device_disk **new_disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2c8f06a..096c96d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1003,6 +1003,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(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-May-04 11:13 UTC

head link

[PATCH v5 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.

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

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

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

Changes in v4:
- pass blkdev_start in libxl_domain_build_info.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 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 7ab2f72..f2fcdf0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         }
     }
 
+    if (b_info->blkdev_start == NULL)
+        b_info->blkdev_start = 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 1bf5d73..bd5ebb3 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -482,7 +482,8 @@ out:
 
 char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
-        libxl_device_disk **new_disk)
+        libxl_device_disk **new_disk,
+        const char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
     char *dev = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 096c96d..0074ec4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1015,7 +1015,8 @@ _hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t
domid,
  */
 _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
-        libxl_device_disk **new_disk);
+        libxl_device_disk **new_disk,
+        const char *blkdev_start);
 _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
         libxl_device_disk *disk);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 68599cb..7131696 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -253,6 +253,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
     ("localtime",       libxl_defbool),
     ("disable_migrate", libxl_defbool),
     ("cpuid",           libxl_cpuid_policy_list),
+    ("blkdev_start",    string),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 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 f0d0ec8..3fbe14d 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -112,6 +112,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 28f5cab..7338562 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -595,6 +595,8 @@ static void parse_config_data(const char
*configfile_filename_report,
     }
 
     libxl_domain_build_info_init_type(b_info, c_info->type);
+    if (blkdev_start)
+        b_info->blkdev_start = strdup(blkdev_start);
 
     /* the following is the actual config parsing with overriding values in the
structures */
     if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
-- 
1.7.2.5

Stefano Stabellini

2012-May-04 11:13 UTC

head link

[PATCH v5 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 v5:
- remove domid paramter to libxl__alloc_vdev (assume
  LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev.

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

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

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index bd5ebb3..7a1e017 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,37 @@ out:
     return rc;
 }
 
+/* libxl__alloc_vdev only works on the local domain, that is the domain
+ * where the toolstack is running */
+static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
+        xs_transaction_t t)
+{
+    int devid = 0, disk = 0, part = 0;
+    char *vdev = NULL;
+    char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
+
+    libxl__device_disk_dev_number(blkdev_start, &disk, &part);
+    if (part != 0) {
+        LOG(ERROR, "blkdev_start is invalid");
+        return NULL;
+    }
+
+    do {
+        vdev = GCSPRINTF("d%dp0", disk);
+        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;
+        }
+        disk++;
+    } while (disk <= (1 << 20));
+    return NULL;
+}
+
 char * libxl__device_disk_local_attach(libxl__gc *gc,
         const libxl_device_disk *in_disk,
         libxl_device_disk **new_disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0074ec4..a451c79 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
@@ -763,6 +765,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-May-04 11:13 UTC

head link

[PATCH v5 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.

Chanegs in v5:
- replace LIBXL__LOG with LOG.

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
 tools/hotplug/Linux/init.d/xencommons           |    3 +
 tools/libxl/libxl_internal.c                    |   58 ++++++++++++++++++----
 3 files changed, 53 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 7a1e017..e180498 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -519,6 +519,7 @@ 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 *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
 
     memcpy(disk, in_disk, sizeof(libxl_device_disk));
@@ -561,13 +562,35 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             if (disk->format != LIBXL_DISK_FORMAT_RAW) {
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
-                           " attach a qdisk image if the format is not
raw");
-                break;
+                do {
+                    t = xs_transaction_start(ctx->xsh);
+                    if (t == XBT_NULL) {
+                        LOG(ERROR, "failed to start a xenstore
transaction");
+                        goto out;
+                    }
+                    disk->vdev = libxl__alloc_vdev(gc, blkdev_start, t);
+                    if (disk->vdev == NULL) {
+                        LOG(ERROR, "libxl__alloc_vdev failed");
+                        goto out;
+                    }
+                    if (libxl__device_disk_add(gc, LIBXL_TOOLSTACK_DOMID,
+                                t, disk)) {
+                        LOG(ERROR, "libxl_device_disk_add failed");
+                        goto out;
+                    }
+                    rc = xs_transaction_end(ctx->xsh, t, 0);
+                } while (rc == 0 && errno == EAGAIN);
+                t = XBT_NULL;
+                if (rc == 0) {
+                    LOGE(ERROR, "xenstore transaction failed");
+                    goto out;
+                }
+                dev = GCSPRINTF("/dev/%s", disk->vdev);
+            } else {
+                dev = disk->pdev_path;
             }
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s\n",
-                       in_disk->pdev_path);
-            dev = disk->pdev_path;
+                       dev);
             break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
"
@@ -576,18 +599,31 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
     }
 
  out:
+    if (t != XBT_NULL)
+        xs_transaction_end(ctx->xsh, t, 1);
     *new_disk = disk;
     return dev;
 }
 
 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;
 }
-- 
1.7.2.5

Stefano Stabellini

2012-May-04 11:13 UTC

head link

[PATCH v5 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 v5:
- unify error paths.

Changes in v4:
- improve exit paths.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_internal.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index e180498..05faff5 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -517,8 +517,9 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
         const char *blkdev_start)
 {
     libxl_ctx *ctx = gc->owner;
-    char *dev = NULL;
+    char *dev = NULL, *be_path = NULL;
     int rc;
+    libxl__device device;
     xs_transaction_t t = XBT_NULL;
     libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
 
@@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
             break;
     }
 
+    if (disk->vdev != NULL) {
+        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
+        if (rc < 0)
+            goto out;
+        be_path = libxl__device_backend_path(gc, &device);
+        rc = libxl__wait_for_backend(gc, be_path, "4");
+        if (rc < 0)
+            goto out;
+    }
+
+    *new_disk = disk;
+    return dev;
  out:
     if (t != XBT_NULL)
         xs_transaction_end(ctx->xsh, t, 1);
-    *new_disk = disk;
-    return dev;
+    else
+        libxl__device_disk_local_detach(gc, disk);
+    return NULL;
 }
 
 int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
-- 
1.7.2.5

Ian Campbell

2012-May-04 14:40 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +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.
> 
> Changes in v5:
> - rename disk to in_disk;
> - rename tmpdisk to disk;
> - copy disk to new_disk only on success;
> - remove check on libxl__zalloc success.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl_bootloader.c |   10 +++++-----
>  tools/libxl/libxl_internal.c   |   20 ++++++++++++++------
>  tools/libxl/libxl_internal.h   |    3 ++-
>  3 files changed, 21 insertions(+), 12 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;
Do you need to keep tmpdisk valid outside the scope of this function?
You don''t seem to in this patch (I didn''t look over the next
patches
yet).

If not then you could just use "libxl_device_disk tmpdisk" and have
libxl__device_disk_local_attach update in place instead allocating and
passing the pointer back.
>      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 fc771ff..55dc55c 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,12 +323,21 @@ out:
>      return rc;
>  }
>  
> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk
*disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk **new_disk)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
> -    char *ret = NULL;
>      int rc;
> +    libxl_device_disk *disk = libxl__zalloc(gc,
sizeof(libxl_device_disk));
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    if (in_disk->pdev_path != NULL)
> +        disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
> +    if (in_disk->script != NULL)
> +        disk->script = libxl__strdup(gc, in_disk->script);
> +    disk->vdev = NULL;
>  
>      rc = libxl__device_disk_setdefault(gc, disk);
>      if (rc) goto out;
> @@ -368,7 +377,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
>                  break;
>              }
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
qdisk %s\n",
> -                       disk->pdev_path);
> +                       in_disk->pdev_path);
>              dev = disk->pdev_path;
>              break;
>          default:
> @@ -378,9 +387,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
>      }
>  
>   out:
> -    if (dev != NULL)
> -        ret = strdup(dev);
> -    return ret;
> +    *new_disk = disk;
> +    return dev;
>  }
>  
>  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 ddd6a37..965d13b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1008,7 +1008,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc
*gc, char *be_path);
>   * a device.
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> -        libxl_device_disk *disk);
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk **new_disk);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>

Ian Campbell

2012-May-04 14:41 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +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.
> 
> This patch contains a large number of unchecked xenstore operations, we
> might want to fix this in the future.
> 
> Changes in v4:
> - libxl__device_generic_add: do not end the transaction is the caller
> provided it;
> - libxl__device_generic_add: fix success exit path.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl.c          |   10 +++++-----
>  tools/libxl/libxl_device.c   |   17 +++++++++++------
>  tools/libxl/libxl_internal.h |    4 ++--
>  tools/libxl/libxl_pci.c      |    2 +-
>  4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 1357efc..525f0d6 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));
>  
> @@ -1802,7 +1802,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));
>  
> @@ -2080,7 +2080,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;
> @@ -2151,7 +2151,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;
> @@ -2284,7 +2284,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 965d13b..2c8f06a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -683,8 +683,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 3856bd9..335c645 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));
>

Ian Campbell

2012-May-04 14:49 UTC

head link

Re: [PATCH v5 4/8] libxl: introduce libxl__device_disk_add

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini
wrote:> 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.
> 
> Changes in v5:
> - rename libxl__device_generic_add_t to libxl__device_generic_add.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@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 525f0d6..941dbb9 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(gc, domid, XBT_NULL, disk);
>      GC_FREE;
>      return rc;
>  }
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 55dc55c..1bf5d73 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,6 +323,163 @@ out:
>      return rc;
>  }
>  
> +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__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;
> +}
> +
>  char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const libxl_device_disk *in_disk,
>          libxl_device_disk **new_disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 2c8f06a..096c96d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1003,6 +1003,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(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.

Ian Campbell

2012-May-04 14:54 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini
wrote:> Introduce a blkdev_start in xl.conf and a corresponding string in
> libxl_domain_build_info.
> 
> Add a blkdev_start parameter to libxl__device_disk_local_attach: it is
> going to be used in a following patch.
> 
> blkdev_start specifies the first block device to be used for temporary
> block device allocations by the toolstack.
> 
> Changes in v5:
> - change the type of the blkdev_start parameter to
> libxl__device_disk_local_attach to const char *.
> 
> Changes in v4:
> - pass blkdev_start in libxl_domain_build_info.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  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"
Need a patch to docs/man/xl.conf.pod.5 as well. If you add that then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>


I suppose this is necessarily Linux specific and we''ll need a patch
from
the BSD folks?
> 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 7ab2f72..f2fcdf0 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          }
>      }
>  
> +    if (b_info->blkdev_start == NULL)
> +        b_info->blkdev_start = 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 1bf5d73..bd5ebb3 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -482,7 +482,8 @@ out:
>  
>  char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const libxl_device_disk *in_disk,
> -        libxl_device_disk **new_disk)
> +        libxl_device_disk **new_disk,
> +        const char *blkdev_start)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 096c96d..0074ec4 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1015,7 +1015,8 @@ _hidden int libxl__device_disk_add(libxl__gc *gc,
uint32_t domid,
>   */
>  _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const libxl_device_disk *in_disk,
> -        libxl_device_disk **new_disk);
> +        libxl_device_disk **new_disk,
> +        const char *blkdev_start);
>  _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
>          libxl_device_disk *disk);
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 68599cb..7131696 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -253,6 +253,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>      ("localtime",       libxl_defbool),
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
> +    ("blkdev_start",    string),
>      
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 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 f0d0ec8..3fbe14d 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -112,6 +112,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 28f5cab..7338562 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -595,6 +595,8 @@ static void parse_config_data(const char
*configfile_filename_report,
>      }
>  
>      libxl_domain_build_info_init_type(b_info, c_info->type);
> +    if (blkdev_start)
> +        b_info->blkdev_start = strdup(blkdev_start);
>  
>      /* the following is the actual config parsing with overriding values
in the structures */
>      if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))

Ian Campbell

2012-May-04 14:59 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini
wrote:> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
> 
> Changes in v5:
> - remove domid paramter to libxl__alloc_vdev (assume
>   LIBXL_TOOLSTACK_DOMID);
> - remove scaling limit from libxl__alloc_vdev.
> 
> Changes in v4:
> - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> - introduce upper bound for encode_disk_name;
> - better error handling;
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl_internal.c |   31 ++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |    4 +++
>  tools/libxl/libxl_linux.c    |   45
++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_netbsd.c   |    6 +++++
>  4 files changed, 86 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index bd5ebb3..7a1e017 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -480,6 +480,37 @@ out:
>      return rc;
>  }
>  
> +/* libxl__alloc_vdev only works on the local domain, that is the domain
> + * where the toolstack is running */
> +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> +        xs_transaction_t t)
> +{
> +    int devid = 0, disk = 0, part = 0;
> +    char *vdev = NULL;
> +    char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> +
> +    libxl__device_disk_dev_number(blkdev_start, &disk, &part);
If you specify the default blkdev_start in xl as d0 instead of xvda
doesn''t at least this bit magically become portable to BSD etc too?

Or couldn''t it actually be an int and save you parsing at all?
> +    if (part != 0) {
> +        LOG(ERROR, "blkdev_start is invalid");
> +        return NULL;
> +    }
> +
> +    do {
> +        vdev = GCSPRINTF("d%dp0", disk);
> +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
libxl__device_disk_dev_number does the right thing with "d<N>"
(i.e.
without the hardcoded "p0"), I think.

I''d be tempted to inline the GCSPRINTF in the arg and do away with vdev
since you don''t use it again.
> +        if (libxl__xs_read(gc, t,
> +                    libxl__sprintf(gc,
"%s/device/vbd/%d/backend",
> +                        dompath, devid)) == NULL) {
> +            if (errno == ENOENT)
> +                return libxl__devid_to_localdev(gc, devid);
> +            else
> +                return NULL;
> +        }
> +        disk++;
> +    } while (disk <= (1 << 20));
That''s a whole lotta disks ;-)
> +    return NULL;
> +}
> +
>  char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const libxl_device_disk *in_disk,
>          libxl_device_disk **new_disk,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 0074ec4..a451c79 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
> @@ -763,6 +765,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
Aha, here''s where we need a BSD contribution. CC someone?
> @@ -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;
> +}

Ian Campbell

2012-May-04 15:11 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini
wrote:> - 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.
> 
> Chanegs in v5:
> - replace LIBXL__LOG with LOG.
> 
> Changes on v4:
> - improve error handling and exit paths in libxl__device_disk_local_attach.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/hotplug/Linux/init.d/sysconfig.xencommons |    3 +
>  tools/hotplug/Linux/init.d/xencommons           |    3 +
>  tools/libxl/libxl_internal.c                    |   58
++++++++++++++++++----
>  3 files changed, 53 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
What does "and log file" mean?

Apart from that:
Acked-by:Ian Campbell <ian.campbell@citrix.com>
> +#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 7a1e017..e180498 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -519,6 +519,7 @@ 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 *disk = libxl__zalloc(gc,
sizeof(libxl_device_disk));
>  
>      memcpy(disk, in_disk, sizeof(libxl_device_disk));
> @@ -561,13 +562,35 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>              break;
>          case LIBXL_DISK_BACKEND_QDISK:
>              if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot
locally"
> -                           " attach a qdisk image if the format is
not raw");
> -                break;
> +                do {
> +                    t = xs_transaction_start(ctx->xsh);
> +                    if (t == XBT_NULL) {
> +                        LOG(ERROR, "failed to start a xenstore
transaction");
> +                        goto out;
> +                    }
> +                    disk->vdev = libxl__alloc_vdev(gc, blkdev_start,
t);
> +                    if (disk->vdev == NULL) {
> +                        LOG(ERROR, "libxl__alloc_vdev failed");
> +                        goto out;
> +                    }
> +                    if (libxl__device_disk_add(gc, LIBXL_TOOLSTACK_DOMID,
> +                                t, disk)) {
> +                        LOG(ERROR, "libxl_device_disk_add
failed");
> +                        goto out;
> +                    }
> +                    rc = xs_transaction_end(ctx->xsh, t, 0);
> +                } while (rc == 0 && errno == EAGAIN);
> +                t = XBT_NULL;
> +                if (rc == 0) {
> +                    LOGE(ERROR, "xenstore transaction failed");
> +                    goto out;
> +                }
> +                dev = GCSPRINTF("/dev/%s", disk->vdev);
> +            } else {
> +                dev = disk->pdev_path;
>              }
>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
qdisk %s\n",
> -                       in_disk->pdev_path);
> -            dev = disk->pdev_path;
> +                       dev);
>              break;
>          default:
>              LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
> @@ -576,18 +599,31 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>      }
>  
>   out:
> +    if (t != XBT_NULL)
> +        xs_transaction_end(ctx->xsh, t, 1);
>      *new_disk = disk;
>      return dev;
>  }
>  
>  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;
>  }

Ian Campbell

2012-May-04 15:13 UTC

head link

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

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini
wrote:> In order to make sure that the block device is available and ready to be
> used, wait for state "connected" in the backend before returning.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Changes in v5:
> - unify error paths.
> 
> Changes in v4:
> - improve exit paths.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl_internal.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index e180498..05faff5 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -517,8 +517,9 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const char *blkdev_start)
>  {
>      libxl_ctx *ctx = gc->owner;
> -    char *dev = NULL;
> +    char *dev = NULL, *be_path = NULL;
>      int rc;
> +    libxl__device device;
>      xs_transaction_t t = XBT_NULL;
>      libxl_device_disk *disk = libxl__zalloc(gc,
sizeof(libxl_device_disk));
>  
> @@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>              break;
>      }
>  
> +    if (disk->vdev != NULL) {
> +        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
> +        if (rc < 0)
> +            goto out;
> +        be_path = libxl__device_backend_path(gc, &device);
> +        rc = libxl__wait_for_backend(gc, be_path, "4");
> +        if (rc < 0)
> +            goto out;
> +    }
> +
> +    *new_disk = disk;
> +    return dev;
>   out:
>      if (t != XBT_NULL)
>          xs_transaction_end(ctx->xsh, t, 1);
There''s no way to reach the preceding "return dev" with the
transaction
still open? Previously we would have fallen through and done it.
> -    *new_disk = disk;
> -    return dev;
> +    else
> +        libxl__device_disk_local_detach(gc, disk);
> +    return NULL;
>  }
>  
>  int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk
*disk)

Ian Jackson

2012-May-04 16:27 UTC

head link

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

Stefano Stabellini writes ("[PATCH v5 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.
...> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index fc771ff..55dc55c 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
...>              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
qdisk %s\n",
> -                       disk->pdev_path);
> +                       in_disk->pdev_path);
I think in_disk->pdev_path can be NULL here ?

Also log messages should not contain "\n"; the logging functions add
it.

Ian.

Ian Jackson

2012-May-04 16:32 UTC

head link

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

Stefano Stabellini writes ("[PATCH v5 5/8] xl/libxl: add a blkdev_start
parameter"):> +    if (b_info->blkdev_start == NULL)
> +        b_info->blkdev_start = strdup("xvda");
This should be   libxl__strdup(0, "xvda")
to get the correct error behaviour.

Aside from that,

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

Ian Jackson

2012-May-04 16:39 UTC

head link

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

Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...> +    do {
> +        vdev = GCSPRINTF("d%dp0", disk);
> +        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;
> +        }
> +        disk++;
> +    } while (disk <= (1 << 20));
This should be "<", as disk==(1<<20) is too big.
And the magic number 20 should not be repeated.

But it turns out that you don''t need to do this check here at all
because libxl__device_disk_dev_number will check this, correctly.

All you need to do is actually pay attention to its error return.
> +    return NULL;
All the NULL error exits should log an error surely.
> +/* 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;
> +}
This still makes it difficult to see that the buffer cannot be
overrun.  I mentioned this in response to a previous posting of this
code and IIRC you were going to do something about it, if only a
comment explaining what the maximum buffer length is and why it''s
safe.
> +    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));
You do not handle snprintf buffer truncation sensibly here.  As it
happens I think this overflow cannot happen but this deserves a
comment at the very least, to explain the lack of error handling.

Ian.

Ian Jackson

2012-May-04 16:42 UTC

head link

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

Stefano Stabellini writes ("[PATCH v5 7/8] xl/libxl: implement QDISK
libxl_device_disk_local_attach"):> - Spawn a QEMU instance at boot time to handle disk local attach
> requests.
...> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 7a1e017..e180498 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
...> +                    rc = xs_transaction_end(ctx->xsh, t, 0);
> +                } while (rc == 0 && errno == EAGAIN);
> +                t = XBT_NULL;
> +                if (rc == 0) {
> +                    LOGE(ERROR, "xenstore transaction failed");
> +                    goto out;
Maybe I''m being excessively picky, but I think "rc" should be
used
only for a variable which contains libxl error codes.  I had to do a
double-take when I saw
  if (rc == 0) {
     error handling;
since rc==0 is of course the success case.

Ian.

Ian Jackson

2012-May-04 16:46 UTC

head link

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

Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
...> > +    libxl__device_disk_dev_number(blkdev_start, &disk,
&part);
> 
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn''t at least this bit magically become portable to BSD etc
too?
This bit is portable already.  It''s specified in terms of the guest
device name rather than the host device name.  This may be confusing
but it saves on having a converter for host device names to disk
numbers.
> Or couldn''t it actually be an int and save you parsing at all?
In general it''s surely more friendly to allow the user to specify this
with a name, like we do with vdevs elsewhere.

Ian.

Ian Campbell

2012-May-04 17:08 UTC

head link

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

On Fri, 2012-05-04 at 17:46 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in
the
> > > domain passed as argument.
> ...
> > > +    libxl__device_disk_dev_number(blkdev_start, &disk,
&part);
> > 
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn''t at least this bit magically become portable to BSD
etc too?
> 
> This bit is portable already.  It''s specified in terms of the
guest
> device name rather than the host device name.  This may be confusing
> but it saves on having a converter for host device names to disk
> numbers.
Right. I think I really should have made this comment on the previous
patch, What I was trying to suggest is that using the Linux specific
"xvda" in the example in the config file and as the default (as patch
5/8 does) is not portable, even though the infrastructure itself is
quite capable of dealing with the portable alternatives -- IOW using
"d0" both in the example and in the actual default would make xl be
automatically portable without other patches (libxl still needs a patch
for the BSD libxl__devid_to_localdev but that''s it).
> > Or couldn''t it actually be an int and save you parsing at
all?
> 
> In general it''s surely more friendly to allow the user to specify
this
> with a name, like we do with vdevs elsewhere.
That seems reasonable.

Ian.

Ian Jackson

2012-May-04 17:16 UTC

head link

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

Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):> Right. I think I really should have made this comment on the previous
> patch, What I was trying to suggest is that using the Linux specific
> "xvda" in the example in the config file
"xvda" is not currently thought to be necessarily Linux-specific.  See
docs/misc/vbd-interface.text.  Perhaps that needs to be revisited, but
in general it''s perfectly possible to pass "xvda" in a domain
config
file for a BSD guest, or whatever.  And Linux writing "xvdfoo" in the
config file doesn''t always make Linux call the device /dev/xvdfoo.

Ian.

Ian Campbell

2012-May-04 17:20 UTC

head link

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

On Fri, 2012-05-04 at 18:16 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):
> > Right. I think I really should have made this comment on the previous
> > patch, What I was trying to suggest is that using the Linux specific
> > "xvda" in the example in the config file
> 
> "xvda" is not currently thought to be necessarily Linux-specific.
See
> docs/misc/vbd-interface.text.  Perhaps that needs to be revisited, but
> in general it''s perfectly possible to pass "xvda" in a
domain config
> file for a BSD guest, or whatever.  And Linux writing "xvdfoo" in
the
> config file doesn''t always make Linux call the device /dev/xvdfoo.
Hrm, I guess. It certainly gives the impression of being Linux specific,
even if technically it isn''t, since people associate xvd* with Linux
even if it happens to be valid and work elsewhere. I guess in reality
very few people, including BSD folks, are going to change this (because
it''ll just work) so there''s no chance of them getting
confused.

Ian.

Jan Beulich

2012-May-10 13:07 UTC

head link

Re: [PATCH v5 0/8] qdisk local attach

>>> On 04.05.12 at 13:12, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> 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.
So I pulled this into my local tree to be able to debug problems in
our gntdev driver in a reasonably isolated way (i.e. not needing
to fire up guests). While block-attach works fine, block-detach
doesn''t at all. This may be related to me using the deprecated
file:/ form of specifying the source during attach, as all of the
supposedly modern forms I tried weren''t accepted by the parser.

Jan

Stefano Stabellini

2012-May-11 15:57 UTC

head link

Re: [PATCH v5 0/8] qdisk local attach

On Thu, 10 May 2012, Jan Beulich wrote:> >>> On 04.05.12 at 13:12, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> > 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.
> 
> So I pulled this into my local tree to be able to debug problems in
> our gntdev driver in a reasonably isolated way (i.e. not needing
> to fire up guests). While block-attach works fine, block-detach
> doesn''t at all. This may be related to me using the deprecated
> file:/ form of specifying the source during attach, as all of the
> supposedly modern forms I tried weren''t accepted by the parser.
Block-detach needs another small patch to libxl in order to work.
I''ll add it as part of the next local_attach patch series.

Stefano Stabellini

2012-May-14 13:04 UTC

head link

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

On Fri, 4 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v5 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.
> ...
> > diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
> > index fc771ff..55dc55c 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> ...
> >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching
qdisk %s\n",
> > -                       disk->pdev_path);
> > +                       in_disk->pdev_path);
> 
> I think in_disk->pdev_path can be NULL here ?
It cannot, unless a wrong configuration was provided. In that case
we''ll
fail to open the empty disk later on.

> Also log messages should not contain "\n"; the logging functions
add
> it.
OK

Ian Campbell

2012-May-14 13:06 UTC

head link

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

On Mon, 2012-05-14 at 14:04 +0100, Stefano Stabellini
wrote:> On Fri, 4 May 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5 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.
> > ...
> > > diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
> > > index fc771ff..55dc55c 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > ...
> > >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally
attaching qdisk %s\n",
> > > -                       disk->pdev_path);
> > > +                       in_disk->pdev_path);
> > 
> > I think in_disk->pdev_path can be NULL here ?
> 
> It cannot, unless a wrong configuration was provided.
so it can?
> In that case we''ll fail to open the empty disk later on.
But logging the NULL pointer doesn''t seem right. If this case is
invalid
we should detect it as such and error out, not carry on until some other
secondary error causes us to abort.

Ian.

Stefano Stabellini

2012-May-14 13:48 UTC

head link

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

On Fri, 4 May 2012, Ian Campbell wrote:> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> > 
> > Changes in v5:
> > - remove domid paramter to libxl__alloc_vdev (assume
> >   LIBXL_TOOLSTACK_DOMID);
> > - remove scaling limit from libxl__alloc_vdev.
> > 
> > Changes in v4:
> > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > - introduce upper bound for encode_disk_name;
> > - better error handling;
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxl/libxl_internal.c |   31 ++++++++++++++++++++++++++++
> >  tools/libxl/libxl_internal.h |    4 +++
> >  tools/libxl/libxl_linux.c    |   45
++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxl_netbsd.c   |    6 +++++
> >  4 files changed, 86 insertions(+), 0 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
> > index bd5ebb3..7a1e017 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -480,6 +480,37 @@ out:
> >      return rc;
> >  }
> >  
> > +/* libxl__alloc_vdev only works on the local domain, that is the
domain
> > + * where the toolstack is running */
> > +static char * libxl__alloc_vdev(libxl__gc *gc, const char
*blkdev_start,
> > +        xs_transaction_t t)
> > +{
> > +    int devid = 0, disk = 0, part = 0;
> > +    char *vdev = NULL;
> > +    char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > +
> > +    libxl__device_disk_dev_number(blkdev_start, &disk,
&part);
> 
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn''t at least this bit magically become portable to BSD etc
too?
It cannot work on BSD for other reason, see below.
In any case blkdev_start can be anything that
libxl__device_disk_dev_number can parse, so d0p0 works too.

> Or couldn''t it actually be an int and save you parsing at all?
Yes, it could. But isn''t "xvda" much more intuitive?

> > +    if (part != 0) {
> > +        LOG(ERROR, "blkdev_start is invalid");
> > +        return NULL;
> > +    }
> > +
> > +    do {
> > +        vdev = GCSPRINTF("d%dp0", disk);
> > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> 
> libxl__device_disk_dev_number does the right thing with
"d<N>" (i.e.
> without the hardcoded "p0"), I think.
In my tests it doesn''t work properly using just d<N>.

> I''d be tempted to inline the GCSPRINTF in the arg and do away with
vdev
> since you don''t use it again.
OK 
> > 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
> 
> Aha, here''s where we need a BSD contribution. CC someone?
There is no working gntdev or QDISK on BSD at the moment.

Ian Campbell

2012-May-14 13:56 UTC

head link

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

On Mon, 2012-05-14 at 14:48 +0100, Stefano Stabellini
wrote:> On Fri, 4 May 2012, Ian Campbell wrote:
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in
the
> > > domain passed as argument.
> > > 
> > > Changes in v5:
> > > - remove domid paramter to libxl__alloc_vdev (assume
> > >   LIBXL_TOOLSTACK_DOMID);
> > > - remove scaling limit from libxl__alloc_vdev.
> > > 
> > > Changes in v4:
> > > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > > - introduce upper bound for encode_disk_name;
> > > - better error handling;
> > > 
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > ---
> > >  tools/libxl/libxl_internal.c |   31 ++++++++++++++++++++++++++++
> > >  tools/libxl/libxl_internal.h |    4 +++
> > >  tools/libxl/libxl_linux.c    |   45
++++++++++++++++++++++++++++++++++++++++++
> > >  tools/libxl/libxl_netbsd.c   |    6 +++++
> > >  4 files changed, 86 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
> > > index bd5ebb3..7a1e017 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > > @@ -480,6 +480,37 @@ out:
> > >      return rc;
> > >  }
> > >  
> > > +/* libxl__alloc_vdev only works on the local domain, that is the
domain
> > > + * where the toolstack is running */
> > > +static char * libxl__alloc_vdev(libxl__gc *gc, const char
*blkdev_start,
> > > +        xs_transaction_t t)
> > > +{
> > > +    int devid = 0, disk = 0, part = 0;
> > > +    char *vdev = NULL;
> > > +    char *dompath = libxl__xs_get_dompath(gc,
LIBXL_TOOLSTACK_DOMID);
> > > +
> > > +    libxl__device_disk_dev_number(blkdev_start, &disk,
&part);
> > 
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn''t at least this bit magically become portable to BSD
etc too?
> 
> It cannot work on BSD for other reason, see below.
> In any case blkdev_start can be anything that
> libxl__device_disk_dev_number can parse, so d0p0 works too.
> 
> 
> > Or couldn''t it actually be an int and save you parsing at
all?
> 
> Yes, it could. But isn''t "xvda" much more intuitive?
To Linux users, yes. But on BSD the virt devices have a different naming
scheme, so xvda wouldn''t necessarily make much sense to them.

However I agree with IanJ''s comment that letting the user use a name is
friendlier than just a bare number.

Anyway, lets just leave it as "xvda", it''s not a huge deal.
> > > +    if (part != 0) {
> > > +        LOG(ERROR, "blkdev_start is invalid");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    do {
> > > +        vdev = GCSPRINTF("d%dp0", disk);
> > > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > 
> > libxl__device_disk_dev_number does the right thing with
"d<N>" (i.e.
> > without the hardcoded "p0"), I think.
> 
> In my tests it doesn''t work properly using just d<N>.
I misread it, the sscanf in libxl__device_disk_dev_number needs to match
at least twice (so "dN" and "pM"). In theory that could be
changed but
lets not do that now.
> > > 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
> > 
> > Aha, here''s where we need a BSD contribution. CC someone?
> 
> There is no working gntdev or QDISK on BSD at the moment.
Oh, right, that does make it somewhat pointless...

Ian.

Stefano Stabellini

2012-May-14 14:10 UTC

head link

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

On Fri, 4 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > +    do {
> > +        vdev = GCSPRINTF("d%dp0", disk);
> > +        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;
> > +        }
> > +        disk++;
> > +    } while (disk <= (1 << 20));
> 
> This should be "<", as disk==(1<<20) is too big.
> And the magic number 20 should not be repeated.
> 
> But it turns out that you don''t need to do this check here at all
> because libxl__device_disk_dev_number will check this, correctly.
> 
> All you need to do is actually pay attention to its error return.
OK
> > +    return NULL;
> 
> All the NULL error exits should log an error surely.
The error log is in the next patch. Should I add a second log here?
Or maybe I should move the error log from the caller to the callee?

> > +/* 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;
> > +}
> 
> This still makes it difficult to see that the buffer cannot be
> overrun.  I mentioned this in response to a previous posting of this
> code and IIRC you were going to do something about it, if only a
> comment explaining what the maximum buffer length is and why it''s
> safe.
I am keen on keeping the code as is, because it is the same that is in
Linux (see comment above).
I''ll add a comment.
> > +    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));
> 
> You do not handle snprintf buffer truncation sensibly here.  As it
> happens I think this overflow cannot happen but this deserves a
> comment at the very least, to explain the lack of error handling.
Same here, I am keen on keeping the code as is, because it is the same
that is in Linux.
I''ll add a comment.

Stefano Stabellini

2012-May-14 14:13 UTC

head link

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

On Fri, 4 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v5 7/8] xl/libxl: implement QDISK
libxl_device_disk_local_attach"):
> > - Spawn a QEMU instance at boot time to handle disk local attach
> > requests.
> ...
> > diff --git a/tools/libxl/libxl_internal.c
b/tools/libxl/libxl_internal.c
> > index 7a1e017..e180498 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> ...
> > +                    rc = xs_transaction_end(ctx->xsh, t, 0);
> > +                } while (rc == 0 && errno == EAGAIN);
> > +                t = XBT_NULL;
> > +                if (rc == 0) {
> > +                    LOGE(ERROR, "xenstore transaction
failed");
> > +                    goto out;
> 
> Maybe I''m being excessively picky, but I think "rc"
should be used
> only for a variable which contains libxl error codes.  I had to do a
> double-take when I saw
>   if (rc == 0) {
>      error handling;
> since rc==0 is of course the success case.
Should I s/rc/ret/ in this function?

Ian Jackson

2012-May-14 14:15 UTC

head link

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

Stefano Stabellini writes ("Re: [PATCH v5 2/8] libxl:
libxl__device_disk_local_attach return a new
libxl_device_disk"):> On Fri, 4 May 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5 2/8] libxl:
libxl__device_disk_local_attach return a new libxl_device_disk"):
> > >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally
attaching qdisk %s\n",
> > > -                       disk->pdev_path);
> > > +                       in_disk->pdev_path);
> > 
> > I think in_disk->pdev_path can be NULL here ?
> 
> It cannot, unless a wrong configuration was provided. In that case
we''ll
> fail to open the empty disk later on.
But not until we''ve called LIBXL__LOG(..., "%s", NULL) which
is
undefined behaviour (according to the spec) and which on all systems
we are likely to run on will print "(null)".

Ian.

Stefano Stabellini

2012-May-14 14:16 UTC

head link

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

On Fri, 4 May 2012, Ian Campbell wrote:> > @@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc
*gc,
> >              break;
> >      }
> >  
> > +    if (disk->vdev != NULL) {
> > +        rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
> > +        if (rc < 0)
> > +            goto out;
> > +        be_path = libxl__device_backend_path(gc, &device);
> > +        rc = libxl__wait_for_backend(gc, be_path, "4");
> > +        if (rc < 0)
> > +            goto out;
> > +    }
> > +
> > +    *new_disk = disk;
> > +    return dev;
> >   out:
> >      if (t != XBT_NULL)
> >          xs_transaction_end(ctx->xsh, t, 1);
> 
> There''s no way to reach the preceding "return dev" with
the transaction
> still open? Previously we would have fallen through and done it.
Yes, there is no way.

Ian Jackson

2012-May-14 14:22 UTC

head link

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

Stefano Stabellini writes ("Re: [PATCH v5 6/8] libxl: introduce
libxl__alloc_vdev"):> On Fri, 4 May 2012, Ian Jackson wrote:
> > All you need to do is actually pay attention to its error return.
> 
> OK
> 
> > > +    return NULL;
> > 
> > All the NULL error exits should log an error surely.
> 
> The error log is in the next patch. Should I add a second log here?
> Or maybe I should move the error log from the caller to the callee?
Well, my view is that a function should either always log if it
returns ERROR_FAIL (or NULL if that''s its calling pattern), or never
do so.  And if it logs this should be in a doc comment.

I inferred that the function was supposed to log by the fact that the
other error paths logged (that is, I didn''t read the doc comment or
the rest of the code).

I don''t mind where the logging is done (although normally it''s
best to
do it closer to the source of the error) but I do want to make sure
that we don''t have cases where there is an exit path which simply
fails without explaining why.
> > > +/* 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;
> > > +}
> > 
> > This still makes it difficult to see that the buffer cannot be
> > overrun.  I mentioned this in response to a previous posting of this
> > code and IIRC you were going to do something about it, if only a
> > comment explaining what the maximum buffer length is and why
it''s
> > safe.
> 
> I am keen on keeping the code as is, because it is the same that is in
> Linux (see comment above).
I read the comment as "this implements the same transformation as the
Linux kernel" not "this is the same code as actually used in the Linux
kernel".  If you mean the latter, do say so.

Also, in that case why is the recursive function name, formatting,
etc. not the same as in Linux (as far as they can be) ?  Surely if
Linux changes this we will want to change our code too and that will
be easiest if we can c&p without needing to reformat, rename, etc.
> I''ll add a comment.
OK.  The comments, taken together, should constitute proofs that the
code does not overrun any buffers.

Thanks,
Ian.

Ian Jackson

2012-May-14 14:23 UTC

head link

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

Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK
libxl_device_disk_local_attach"):> On Fri, 4 May 2012, Ian Jackson wrote:
> > Maybe I''m being excessively picky, but I think "rc"
should be used
> > only for a variable which contains libxl error codes.  I had to do a
> > double-take when I saw
> >   if (rc == 0) {
> >      error handling;
> > since rc==0 is of course the success case.
> 
> Should I s/rc/ret/ in this function?
I think you should s/rc/<something else>/ when it''s the return
value
from xs_*.  ret would do.  If rc is used for a libxl error code it
should remain as rc.

Ian.

Stefano Stabellini

2012-May-14 14:43 UTC

head link

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

On Mon, 14 May 2012, Ian Jackson wrote:> > > > +/* 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;
> > > > +}
> > > 
> > > This still makes it difficult to see that the buffer cannot be
> > > overrun.  I mentioned this in response to a previous posting of
this
> > > code and IIRC you were going to do something about it, if only a
> > > comment explaining what the maximum buffer length is and why
it''s
> > > safe.
> > 
> > I am keen on keeping the code as is, because it is the same that is in
> > Linux (see comment above).
> 
> I read the comment as "this implements the same transformation as the
> Linux kernel" not "this is the same code as actually used in the
Linux
> kernel".  If you mean the latter, do say so.
Yes, it is actually the same used in the Linux kernel.

> Also, in that case why is the recursive function name, formatting,
> etc. not the same as in Linux (as far as they can be) ?  Surely if
> Linux changes this we will want to change our code too and that will
> be easiest if we can c&p without needing to reformat, rename, etc.
It is exactly the same code, except for the tab indentation that I
replaced with space indentation to follow the coding style.

> > I''ll add a comment.
> 
> OK.  The comments, taken together, should constitute proofs that the
> code does not overrun any buffers.
The two comments refer to the upper bound that we introduced, thanks to
the limit there can be no buffer overruns.

Stefano Stabellini

2012-May-14 14:45 UTC

head link

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

On Mon, 14 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement
QDISK libxl_device_disk_local_attach"):
> > On Fri, 4 May 2012, Ian Jackson wrote:
> > > Maybe I''m being excessively picky, but I think
"rc" should be used
> > > only for a variable which contains libxl error codes.  I had to
do a
> > > double-take when I saw
> > >   if (rc == 0) {
> > >      error handling;
> > > since rc==0 is of course the success case.
> > 
> > Should I s/rc/ret/ in this function?
> 
> I think you should s/rc/<something else>/ when it''s the
return value
> from xs_*.  ret would do.  If rc is used for a libxl error code it
> should remain as rc.
rc is already used for a libxl error code in the same function.
Maybe I should just add ret in addition to rc?

Ian Jackson

2012-May-14 14:46 UTC

head link

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

Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK
libxl_device_disk_local_attach"):> On Mon, 14 May 2012, Ian Jackson wrote:
> > I think you should s/rc/<something else>/ when it''s the
return value
> > from xs_*.  ret would do.  If rc is used for a libxl error code it
> > should remain as rc.
> 
> rc is already used for a libxl error code in the same function.
> Maybe I should just add ret in addition to rc?
I think that would be better.  Do you agree ?

As I say this seems like quite a small nit I''m picking.

Ian.

Stefano Stabellini

2012-May-14 14:52 UTC

head link

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

On Mon, 14 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement
QDISK libxl_device_disk_local_attach"):
> > On Mon, 14 May 2012, Ian Jackson wrote:
> > > I think you should s/rc/<something else>/ when
it''s the return value
> > > from xs_*.  ret would do.  If rc is used for a libxl error code
it
> > > should remain as rc.
> > 
> > rc is already used for a libxl error code in the same function.
> > Maybe I should just add ret in addition to rc?
> 
> I think that would be better.  Do you agree ?
In general I try to make my code as easy to read as possible. A new
integer is a small price to pay for it.

Xen devel - May 2012 - [PATCH v5 0/8] qdisk local attach

[PATCH v5 0/8] qdisk local attach

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

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

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

[PATCH v5 4/8] libxl: introduce libxl__device_disk_add

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

[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

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

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

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

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

Re: [PATCH v5 4/8] libxl: introduce libxl__device_disk_add

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

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

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

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

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

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

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

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

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

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

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

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

Re: [PATCH v5 0/8] qdisk local attach

Re: [PATCH v5 0/8] qdisk local attach

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

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

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

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

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

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

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

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

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

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

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

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

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

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