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 v6:
- rebase on "nstore: rename public xenstore headers";
- new patch: "libxl_string_to_backend: add qdisk";
- new patch: "main_blockdetach: destroy the disk on successful
removal";
- split "libxl: introduce libxl__device_disk_add" in two patches;
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs;
- document blkdev_start;
- use libxl__strdup to allocate blkdev_start;
- more comments in libxl__devid_to_localdev;
- inline GCSPRINTF;
- introduce ret for xs_* error codes.
Changes in v5:
- remove _hidden from the libxl_device_disk_local_attach/detach
implementation;
- libxl__device_disk_local_attach: rename disk to in_disk;
- libxl__device_disk_local_attach: rename tmpdisk to disk;
- libxl__device_disk_local_attach: copy disk to new_disk only on success;
- libxl__device_disk_local_attach: remove check on libxl__zalloc success.
- rename libxl__device_generic_add_t to libxl__device_generic_add;
- change the type of the blkdev_start parameter to
libxl__device_disk_local_attach to const char *;
- remove domid paramter to libxl__alloc_vdev (assume
LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev;
- libxl__device_disk_local_attach: replace LIBXL__LOG with LOG;
- libxl__device_disk_local_attach: unify error paths.
Changes in v4:
- split the first patch into 2 patches: the first is a simple move, the
second one adds a new parameter;
- libxl__device_generic_add: do not end the transaction is the caller
provided it;
- libxl__device_generic_add: fix success exit path;
- pass blkdev_start in libxl_domain_build_info;
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- improve error handling and exit paths in libxl__alloc_vdev and
libxl__device_disk_local_attach.
Changes in v3:
- libxl__device_disk_local_attach: wait for the backend to be
"connected" before returning.
Changes in v2:
- make libxl_device_disk_local_(de)attach internal functions;
- allocate the new disk in libxl_device_disk_local_attatch on the GC;
- remove libxl__device_generic_add_t, add a transaction parameter to
libxl__device_generic_add instead;
- add a new patch to introduce a blkdev_start option to xl.conf;
- reimplement libxl__alloc_vdev checking for the frontend path and
starting from blkdev_start;
- introduce a Linux specific libxl__devid_to_vdev function based on last
Jan''s patch to blkfront.
Stefano Stabellini (11):
libxl: make libxl_device_disk_local_attach/detach internal functions
libxl: libxl__device_disk_local_attach return a new libxl_device_disk
libxl: add a transaction parameter to libxl__device_generic_add
libxl: move libxl__device_from_disk to libxl_internal.c
libxl: introduce libxl__device_disk_add
xl/libxl: add a blkdev_start parameter
libxl: introduce libxl__alloc_vdev
xl/libxl: implement QDISK libxl_device_disk_local_attach
libxl__device_disk_local_attach: wait for state "connected"
libxl_string_to_backend: add qdisk
main_blockdetach: destroy the disk on successful removal
docs/man/xl.conf.pod.5 | 6 +
tools/examples/xl.conf | 3 +
tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
tools/hotplug/Linux/init.d/xencommons | 3 +
tools/libxl/libxl.c | 238 +----------------
tools/libxl/libxl.h | 7 -
tools/libxl/libxl_bootloader.c | 7 +-
tools/libxl/libxl_create.c | 3 +
tools/libxl/libxl_device.c | 17 +-
tools/libxl/libxl_internal.c | 325 +++++++++++++++++++++++
tools/libxl/libxl_internal.h | 26 ++-
tools/libxl/libxl_linux.c | 48 ++++
tools/libxl/libxl_netbsd.c | 6 +
tools/libxl/libxl_pci.c | 2 +-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/libxl_utils.c | 2 +
tools/libxl/xl.c | 3 +
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 5 +-
19 files changed, 454 insertions(+), 252 deletions(-)
Cheers,
Stefano
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 01/11] 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 | 77 ----------------------------------------
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(+), 86 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4d01cf8..35f6ff9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1689,83 +1689,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.
- */
- /*
- * FIXME
- * This appears to leak the disk in failure paths
- */
-
- 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 979940a..f79a024 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -679,13 +679,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 fb1302b..6563dbd 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,7 @@ static void bootloader_cleanup(libxl__egc *egc,
libxl__bootloader_state *bl)
if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
if (bl->diskpath) {
- libxl_device_disk_local_detach(CTX, bl->disk);
+ libxl__device_disk_local_detach(gc, bl->disk);
free(bl->diskpath);
bl->diskpath = 0;
}
@@ -330,7 +330,7 @@ void libxl__bootloader_run(libxl__egc *egc,
libxl__bootloader_state *bl)
goto out;
}
- bl->diskpath = libxl_device_disk_local_attach(CTX, bl->disk);
+ bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
if (!bl->diskpath) {
rc = ERROR_FAIL;
goto out;
diff --git a/tools/libxl/libxl_internal.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 b2fe8bb..6449615 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1233,6 +1233,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-18 14:08 UTC
[PATCH v6 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
Introduce a new libxl_device_disk* parameter to
libxl__device_disk_local_attach, the parameter is allocated by the
caller. libxl__device_disk_local_attach is going to fill the new disk
with informations about the new locally attached disk. The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.
Changes in v6:
- return error in case pdev_path is NULL;
- libxl__device_disk_local_attach update the new disk, the caller
allocates it;
- remove \n from logs.
Changes in v5:
- rename disk to in_disk;
- rename tmpdisk to disk;
- copy disk to new_disk only on success;
- remove check on libxl__zalloc success.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_bootloader.c | 6 ++++--
tools/libxl/libxl_internal.c | 17 ++++++++++++++---
tools/libxl/libxl_internal.h | 4 +++-
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 6563dbd..625c55d 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -228,7 +228,9 @@ static void bootloader_cleanup(libxl__egc *egc,
libxl__bootloader_state *bl)
if (bl->outputdir) libxl__remove_directory(gc, bl->outputdir);
if (bl->diskpath) {
- libxl__device_disk_local_detach(gc, bl->disk);
+ libxl__device_disk_local_detach(gc, &bl->tmpdisk);
+ free(bl->tmpdisk.pdev_path);
+ free(bl->tmpdisk.script);
free(bl->diskpath);
bl->diskpath = 0;
}
@@ -330,7 +332,7 @@ void libxl__bootloader_run(libxl__egc *egc,
libxl__bootloader_state *bl)
goto out;
}
- bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk);
+ bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk,
&bl->tmpdisk);
if (!bl->diskpath) {
rc = ERROR_FAIL;
goto out;
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index fc771ff..5e5083b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,13 +323,24 @@ 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 *disk)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
char *ret = NULL;
int rc;
+ if (in_disk->pdev_path == NULL)
+ return NULL;
+
+ memcpy(disk, in_disk, sizeof(libxl_device_disk));
+ disk->pdev_path = strdup(in_disk->pdev_path);
+ if (in_disk->script != NULL)
+ disk->script = strdup(in_disk->script);
+ disk->vdev = NULL;
+
rc = libxl__device_disk_setdefault(gc, disk);
if (rc) goto out;
@@ -367,8 +378,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_device_disk *disk)
" attach a qdisk image if the format is not
raw");
break;
}
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s\n",
- disk->pdev_path);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s",
+ in_disk->pdev_path);
dev = disk->pdev_path;
break;
default:
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6449615..a9127db 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1238,7 +1238,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);
@@ -1767,6 +1768,7 @@ struct libxl__bootloader_state {
libxl__bootloader_console_callback *console_available;
libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
libxl_device_disk *disk;
+ libxl_device_disk tmpdisk;
uint32_t domid;
/* private to libxl__run_bootloader */
char *outputpath, *outputdir, *logfile;
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 03/11] 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>
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 35f6ff9..64d2dd8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1426,7 +1426,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));
@@ -1827,7 +1827,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));
@@ -2120,7 +2120,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;
@@ -2191,7 +2191,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;
@@ -2324,7 +2324,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 a9127db..cb5393d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -781,8 +781,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-18 14:08 UTC
[PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.c | 40 ----------------------------------------
tools/libxl/libxl_internal.c | 40 ++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 4 ++++
3 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 64d2dd8..3f53da5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1281,46 +1281,6 @@ 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);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 5e5083b..276429c 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -429,6 +429,46 @@ libxl_device_model_version
libxl__device_model_version_running(libxl__gc *gc,
return value;
}
+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;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cb5393d..3578414 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1233,6 +1233,10 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
*/
_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device);
+
/*
* Make a disk available in this (the control) domain. Returns path to
* a device.
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 05/11] 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.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.c | 113 +---------------------------------------
tools/libxl/libxl_internal.c | 119 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 2 +
3 files changed, 122 insertions(+), 112 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3f53da5..1bed2fe 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1284,118 +1284,7 @@ int libxl__device_disk_setdefault(libxl__gc *gc,
libxl_device_disk *disk)
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) {
- LOG(ERROR, "failed to get blktap devpath for %p\n",
- disk->pdev_path);
- 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 276429c..70a8c4b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,125 @@ out:
return rc;
}
+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) {
+ LOG(ERROR, "failed to get blktap devpath for %p\n",
+ disk->pdev_path);
+ 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 *disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3578414..98f9762 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1236,6 +1236,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc,
char *be_path);
_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
libxl_device_disk *disk,
libxl__device *device);
+_hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
+ xs_transaction_t t, libxl_device_disk *disk);
/*
* Make a disk available in this (the control) domain. Returns path to
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 06/11] 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 v6:
- document blkdev_start;
- use libxl__strdup to allocate blkdev_start.
Changes in v5:
- change the type of the blkdev_start parameter to
libxl__device_disk_local_attach to const char *.
Changes in v4:
- pass blkdev_start in libxl_domain_build_info.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
docs/man/xl.conf.pod.5 | 6 ++++++
tools/examples/xl.conf | 3 +++
tools/libxl/libxl_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 ++
10 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..149430c 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -84,6 +84,12 @@ previous C<xm> toolstack this can be configured to use
the old C<SXP>
Default: C<json>
+=item B<blkdev_start="NAME">
+
+Configures the name of the first block device to be used for temporary
+block device allocations by the toolstack.
+The default choice is "xvda".
+
=back
=head1 SEE ALSO
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 56d3b3b..ebf057c 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,6 @@
# default output format used by "xl list -l"
#output_format="json"
+
+# first block device to be used for temporary VM disk mounts
+#blkdev_start="xvda"
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 625c55d..d972f40 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -332,7 +332,8 @@ void libxl__bootloader_run(libxl__egc *egc,
libxl__bootloader_state *bl)
goto out;
}
- bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk,
&bl->tmpdisk);
+ bl->diskpath = libxl__device_disk_local_attach(gc, bl->disk,
&bl->tmpdisk,
+ info->blkdev_start);
if (!bl->diskpath) {
rc = ERROR_FAIL;
goto out;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 14721eb..9fb4b81 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
}
+ if (b_info->blkdev_start == NULL)
+ b_info->blkdev_start = libxl__strdup(0, "xvda");
+
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!b_info->u.hvm.bios)
switch (b_info->device_model_version) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 70a8c4b..f3e816e 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -444,7 +444,8 @@ out:
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
- libxl_device_disk *disk)
+ libxl_device_disk *disk,
+ const char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 98f9762..d623c1c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1245,7 +1245,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 551e367..1b5ab72 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -253,6 +253,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
("localtime", libxl_defbool),
("disable_migrate", libxl_defbool),
("cpuid", libxl_cpuid_policy_list),
+ ("blkdev_start", string),
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 750a61c..827f3f8 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -38,6 +38,7 @@ xentoollog_logger_stdiostream *logger;
int dryrun_only;
int force_execution;
int autoballoon = 1;
+char *blkdev_start;
char *lockfile;
char *default_vifscript = NULL;
char *default_bridge = NULL;
@@ -94,6 +95,8 @@ static void parse_global_config(const char *configfile,
fprintf(stderr, "invalid default output format
\"%s\"\n", buf);
}
}
+ if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
+ blkdev_start = strdup(buf);
xlu_cfg_destroy(config);
}
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5d0d504..9055aa4 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -114,6 +114,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 5fc5cde..806c003 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-18 14:08 UTC
[PATCH v6 07/11] libxl: introduce libxl__alloc_vdev
Introduce libxl__alloc_vdev: find a spare virtual block device in the
domain passed as argument.
Changes in v6:
- more comments in libxl__devid_to_localdev;
- inline GCSPRINTF.
Changes in v5:
- remove domid paramter to libxl__alloc_vdev (assume
LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev.
Changes in v4:
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- better error handling;
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_internal.c | 32 ++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 4 +++
tools/libxl/libxl_linux.c | 48 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_netbsd.c | 6 +++++
4 files changed, 90 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f3e816e..4aef643 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -442,6 +442,38 @@ 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 *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
+
+ libxl__device_disk_dev_number(blkdev_start, &disk, &part);
+ if (part != 0) {
+ LOG(ERROR, "blkdev_start is invalid");
+ return NULL;
+ }
+
+ do {
+ devid = libxl__device_disk_dev_number(GCSPRINTF("d%dp0",
disk),
+ NULL, NULL);
+ if (devid < 0)
+ return NULL;
+ if (libxl__xs_read(gc, t,
+ libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+ dompath, devid)) == NULL) {
+ if (errno == ENOENT)
+ return libxl__devid_to_localdev(gc, devid);
+ else
+ return NULL;
+ }
+ disk++;
+ } while (1);
+ return NULL;
+}
+
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
libxl_device_disk *disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d623c1c..c42d4a2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -78,6 +78,8 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
#define STUBDOM_CONSOLE_LOGGING 0
#define STUBDOM_CONSOLE_SAVE 1
@@ -904,6 +906,8 @@ static inline void libxl__domaindeathcheck_stop(libxl__gc
*gc,
_hidden int libxl__try_phy_backend(mode_t st_mode);
+_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
+
/* from libxl_pci */
_hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..02b80b6 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,51 @@ 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.
+ * The maximum buffer length is 32 bytes.
+ * The code is safe thanks to the upper bound enforced by the caller. */
+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
+ /* overflow cannot happen, thanks to the upper bound */
+ snprintf(ptr, ret + 32 - ptr,
+ "%d", minor & (nr_parts - 1));
+ return ret;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
return 0;
}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ /* TODO */
+ return NULL;
+}
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach
- Spawn a QEMU instance at boot time to handle disk local attach
requests.
- Implement libxl_device_disk_local_attach for QDISKs in terms of a
regular disk attach with the frontend and backend running in the same
domain.
Changes in v6:
- introduce xs_ret for xs_* error codes.
Changes in v5:
- replace LIBXL__LOG with LOG.
Changes on v4:
- improve error handling and exit paths in libxl__device_disk_local_attach.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by:Ian Campbell <ian.campbell@citrix.com>
---
tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
tools/hotplug/Linux/init.d/xencommons | 3 +
tools/libxl/libxl_internal.c | 60 ++++++++++++++++++-----
3 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons
b/tools/hotplug/Linux/init.d/sysconfig.xencommons
index 6543204..38ea85a 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons
@@ -12,3 +12,6 @@
# Running xenbackendd in debug mode
#XENBACKENDD_DEBUG=[yes|on|1]
+
+# qemu path
+#QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
diff --git a/tools/hotplug/Linux/init.d/xencommons
b/tools/hotplug/Linux/init.d/xencommons
index 2f81ea2..9dda6e2 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -104,6 +104,9 @@ do_start () {
xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
test "`uname`" != "NetBSD" || xenbackendd
$XENBACKENDD_ARGS
+ echo Starting QEMU as disk backend for dom0
+ test -z "$QEMU_XEN" &&
QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
+ $QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize
-monitor /dev/null
}
do_stop () {
echo Stopping xenconsoled
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 4aef643..d66f7b1 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -482,7 +482,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
char *ret = NULL;
- int rc;
+ int rc, xs_ret;
+ xs_transaction_t t = XBT_NULL;
if (in_disk->pdev_path == NULL)
return NULL;
@@ -526,13 +527,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;
+ }
+ xs_ret = xs_transaction_end(ctx->xsh, t, 0);
+ } while (xs_ret == 0 && errno == EAGAIN);
+ t = XBT_NULL;
+ if (xs_ret == 0) {
+ LOGE(ERROR, "xenstore transaction failed");
+ goto out;
+ }
+ dev = GCSPRINTF("/dev/%s", disk->vdev);
+ } else {
+ dev = disk->pdev_path;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s",
- in_disk->pdev_path);
- dev = disk->pdev_path;
+ dev);
break;
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
"
@@ -541,6 +564,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
}
out:
+ if (t != XBT_NULL)
+ xs_transaction_end(ctx->xsh, t, 1);
if (dev != NULL)
ret = strdup(dev);
return ret;
@@ -548,12 +573,23 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
{
- /* Nothing to do for PHYSTYPE_PHY. */
-
- /*
- * 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-18 14:08 UTC
[PATCH v6 09/11] 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 | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index d66f7b1..a11c75b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,9 +480,10 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
const char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
- char *dev = NULL;
+ char *dev = NULL, *be_path = NULL;
char *ret = NULL;
int rc, xs_ret;
+ libxl__device device;
xs_transaction_t t = XBT_NULL;
if (in_disk->pdev_path == NULL)
@@ -563,12 +564,25 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
break;
}
- out:
- if (t != XBT_NULL)
- xs_transaction_end(ctx->xsh, t, 1);
+ if (disk->vdev != NULL) {
+ rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk,
&device);
+ if (rc < 0)
+ goto out;
+ be_path = libxl__device_backend_path(gc, &device);
+ rc = libxl__wait_for_backend(gc, be_path, "4");
+ if (rc < 0)
+ goto out;
+ }
if (dev != NULL)
ret = strdup(dev);
return ret;
+
+ out:
+ if (t != XBT_NULL)
+ xs_transaction_end(ctx->xsh, t, 1);
+ else
+ libxl__device_disk_local_detach(gc, disk);
+ return NULL;
}
int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 10/11] libxl_string_to_backend: add qdisk
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_utils.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 858410e..c065f59 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -240,6 +240,8 @@ int libxl_string_to_backend(libxl_ctx *ctx, char *s,
libxl_disk_backend *backend
*backend = LIBXL_DISK_BACKEND_PHY;
} else if (!strcmp(s, "file")) {
*backend = LIBXL_DISK_BACKEND_TAP;
+ } else if (!strcmp(s, "qdisk")) {
+ *backend = LIBXL_DISK_BACKEND_QDISK;
} else if (!strcmp(s, "tap")) {
p = strchr(s, '':'');
if (!p) {
--
1.7.2.5
Stefano Stabellini
2012-May-18 14:08 UTC
[PATCH v6 11/11] main_blockdetach: destroy the disk on successful removal
main_blockdetach needs to call libxl_device_disk_destroy so that all the
disk related entries are properly removed from xenstore.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/xl_cmdimpl.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 806c003..5a4a7f5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5144,7 +5144,8 @@ int main_blockdetach(int argc, char **argv)
}
if (libxl_device_disk_remove(ctx, domid, &disk, 0)) {
fprintf(stderr, "libxl_device_disk_remove failed.\n");
- }
+ } else
+ libxl_device_disk_destroy(ctx, domid, &disk);
return 0;
}
--
1.7.2.5
Ian Jackson
2012-May-18 14:26 UTC
Re: [PATCH v6 01/11] libxl: make libxl_device_disk_local_attach/detach internal functions
Stefano Stabellini writes ("[PATCH v6 01/11] 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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2012-May-18 14:33 UTC
Re: [PATCH v6 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
Stefano Stabellini writes ("[PATCH v6 02/11] libxl:
libxl__device_disk_local_attach return a new
libxl_device_disk"):> Introduce a new libxl_device_disk* parameter to
> libxl__device_disk_local_attach, the parameter is allocated by the
> caller. libxl__device_disk_local_attach is going to fill the new disk
> with informations about the new locally attached disk. The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
In this declaration:
> @@ -1767,6 +1768,7 @@ struct libxl__bootloader_state {
> libxl__bootloader_console_callback *console_available;
> libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline}
updated */
> libxl_device_disk *disk;
> + libxl_device_disk tmpdisk;
> uint32_t domid;
We need information about what this "tmpdisk" is. All of the other
parameters here are input parameters, except as otherwise noted in the
comment.
Also I''m not convinced that "tmpdisk" is quite the right
name. You
also need to explain the distinction between "disk" and
"tmpdisk".
Perhaps:
const libxl_device_disk *disk; /* as specified by user */
libxl_device_disk localdisk;
/* Should be zeroed by caller on entry. Will be filled in by
* bootloader machinery; represents the local attachment of the
* disk for the benefit of the bootloader. Must be detached by
* the caller using libxl__device_disk_local_detach, but only
* after the domain''s kernel and initramfs have been loaded into
* memory and the file references disposed of. */
?
The implementation looks sane.
Ian.
Ian Jackson
2012-May-18 14:34 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
Stefano Stabellini writes ("[PATCH v6 04/11] libxl: move
libxl__device_from_disk to libxl_internal.c"):> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2012-May-18 14:36 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
Stefano Stabellini writes ("[PATCH v6 05/11] 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.
Can''t this be done in such a way that the diff isn''t
"delete this
function completely" followed by "here is a new function" ?
I don''t really understand why you want to move the function at all,
TBH. I think keeping the public wrapper and the internal
implementation together is fine. There is no need IMO to move the
internal function to libxl_internal.c.
Ian.
Stefano Stabellini writes ("[PATCH v6 07/11] libxl: introduce
libxl__alloc_vdev"):> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...> +/* Same as in Linux.
> + * The maximum buffer length is 32 bytes.
> + * The code is safe thanks to the upper bound enforced by the caller. */
> +static char *encode_disk_name(char *ptr, unsigned int n)
So this says that encode_disk_name may use up to 32 bytes in *ptr
(including or excluding the trailing nul?)
_Why_ may the maximum buffer length used be 32 bytes ? Also, "maximum
buffer length" is a confusing phrase because it seems to refer to the
/actual/ length of the buffer rather than the amount /used/.
And this...
> + char *ret = libxl__zalloc(gc, 32);
... allocates a 32 byte buffer which we then fill with ...
> + strcpy(ret, "xvd");
> + ptr = encode_disk_name(ret + 3, offset);
3 characters plus the encoded disk name. So possibly using up to 36
bytes.
In fact it seems to me that there could be a much tighter upper bound
on the length of output of encode_disk_name (based on arithmetic) but
it needs to be properly calculated and documented and perhaps put in a
#define.
Ian.
Ian Jackson
2012-May-18 14:42 UTC
Re: [PATCH v6 08/11] xl/libxl: implement QDISK libxl_device_disk_local_attach
Stefano Stabellini writes ("[PATCH v6 08/11] xl/libxl: implement QDISK
libxl_device_disk_local_attach"):> - Spawn a QEMU instance at boot time to handle disk local attach
> requests.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by:Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Campbell
2012-May-18 14:42 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
On Fri, 2012-05-18 at 15:36 +0100, Ian Jackson wrote:> There is no need IMO to move the > internal function to libxl_internal.c.I''ve always thought the existence libxl_internal.c was just begging for people to group functions along the wrong criteria anyway... The whole concept of what lives where inside tools/libxl/libxl*.c is all pretty ad-hoc anyway. Ian.
Ian Jackson
2012-May-18 14:44 UTC
Re: [PATCH v6 09/11] libxl__device_disk_local_attach: wait for state "connected"
Stefano Stabellini writes ("[PATCH v6 09/11]
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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Stefano Stabellini writes ("[PATCH v6 10/11] libxl_string_to_backend: add
qdisk"):> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2012-May-18 14:44 UTC
Re: [PATCH v6 11/11] main_blockdetach: destroy the disk on successful removal
Stefano Stabellini writes ("[PATCH v6 11/11] main_blockdetach: destroy the
disk on successful removal"):> main_blockdetach needs to call libxl_device_disk_destroy so that all the
> disk related entries are properly removed from xenstore.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Jackson
2012-May-18 14:47 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
Ian Campbell writes ("Re: [PATCH v6 05/11] libxl: introduce
libxl__device_disk_add"):> On Fri, 2012-05-18 at 15:36 +0100, Ian Jackson wrote:
> > There is no need IMO to move the
> > internal function to libxl_internal.c.
>
> I''ve always thought the existence libxl_internal.c was just
begging for
> people to group functions along the wrong criteria anyway...
Yes.
> The whole concept of what lives where inside tools/libxl/libxl*.c is all
> pretty ad-hoc anyway.
Quite.
Ian.
Roger Pau Monne
2012-May-18 14:55 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
Stefano Stabellini wrote:> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com> > --- > tools/libxl/libxl.c | 40 ---------------------------------------- > tools/libxl/libxl_internal.c | 40 ++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_internal.h | 4 ++++ > 3 files changed, 44 insertions(+), 40 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 64d2dd8..3f53da5 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1281,46 +1281,6 @@ 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); > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c > index 5e5083b..276429c 100644 > --- a/tools/libxl/libxl_internal.c > +++ b/tools/libxl/libxl_internal.c > @@ -429,6 +429,46 @@ libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc, > return value; > } > > +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > + libxl_device_disk *disk, > + libxl__device *device)I think this should be moved to libxl_device instead of libxl_internal, since it''s a device related function.> +{ > + 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; > +} > + > /* > * Local variables: > * mode: C > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index cb5393d..3578414 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1233,6 +1233,10 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc, > */ > _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path); > > +_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > + libxl_device_disk *disk, > + libxl__device *device); > + > /* > * Make a disk available in this (the control) domain. Returns path to > * a device.
Roger Pau Monne
2012-May-18 14:56 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
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. > > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com> > --- > tools/libxl/libxl.c | 113 +--------------------------------------- > tools/libxl/libxl_internal.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_internal.h | 2 + > 3 files changed, 122 insertions(+), 112 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 3f53da5..1bed2fe 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1284,118 +1284,7 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk) > 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) { > - LOG(ERROR, "failed to get blktap devpath for %p\n", > - disk->pdev_path); > - 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 276429c..70a8c4b 100644 > --- a/tools/libxl/libxl_internal.c > +++ b/tools/libxl/libxl_internal.c > @@ -323,6 +323,125 @@ out: > return rc; > } > > +int libxl__device_disk_add(libxl__gc *gc, uint32_t domid, > + xs_transaction_t t, libxl_device_disk *disk)As with the other patch, I think this should go to libxl_device.> +{ > + 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) { > + LOG(ERROR, "failed to get blktap devpath for %p\n", > + disk->pdev_path); > + 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 *disk) > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 3578414..98f9762 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1236,6 +1236,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path); > _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > libxl_device_disk *disk, > libxl__device *device); > +_hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid, > + xs_transaction_t t, libxl_device_disk *disk); > > /* > * Make a disk available in this (the control) domain. Returns path to > -- > 1.7.2.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2012-May-18 15:02 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
On Fri, 2012-05-18 at 15:55 +0100, Roger Pau Monne wrote:> > @@ -429,6 +429,46 @@ libxl_device_model_version > libxl__device_model_version_running(libxl__gc *gc, > > return value; > > } > > > > +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > > + libxl_device_disk *disk, > > + libxl__device *device) > > I think this should be moved to libxl_device instead of > libxl_internal, since it''s a device related function.I think it''d be better to leave it in the original file, libxl is so confused about what goes where that it doesn''t really matter... If we do want to move it we can do it later as a pure code motion change. Ian.
Roger Pau Monne
2012-May-18 15:16 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
Ian Campbell wrote:> On Fri, 2012-05-18 at 15:55 +0100, Roger Pau Monne wrote: >>> @@ -429,6 +429,46 @@ libxl_device_model_version >> libxl__device_model_version_running(libxl__gc *gc, >>> return value; >>> } >>> >>> +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, >>> + libxl_device_disk *disk, >>> + libxl__device *device) >> I think this should be moved to libxl_device instead of >> libxl_internal, since it''s a device related function. > > I think it''d be better to leave it in the original file, libxl is so > confused about what goes where that it doesn''t really matter... > > If we do want to move it we can do it later as a pure code motion > change.In my hotplug series I''m moving most libxl_device_{...}_add to provide an internal implementation that takes an ao, making something quite similar to what Stefano does, if I start moving all those to libxl_internal we will fill this file with functions that could be somewhere else (libxl_device). I understand that the libxl policy is put functions where they seem to belong (device related functions to libxl_device, domain related ones to libxl_dom...), and if they fit in none of this categories then put them in libxl_internal or create a new file. We can leave it in libxl_internal for now, and I will move it on my series.> Ian. > >
Ian Jackson
2012-May-18 15:23 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v6 04/11] libxl: move
libxl__device_from_disk to libxl_internal.c"):> In my hotplug series I''m moving most libxl_device_{...}_add to
provide
> an internal implementation that takes an ao, making something quite
> similar to what Stefano does, if I start moving all those to
> libxl_internal we will fill this file with functions that could be
> somewhere else (libxl_device). I understand that the libxl policy is put
> functions where they seem to belong (device related functions to
> libxl_device, domain related ones to libxl_dom...), and if they fit in
> none of this categories then put them in libxl_internal or create a new
> file.
>
> We can leave it in libxl_internal for now, and I will move it on my series.
I don''t think we have a policy. I think at this stage I would much
prefer not to move whole functions from one file to another for purely
cosmetic reasons.
(It''s a different question if you need to move the body of a function
into the middle of another one, or something.)
Ian.
Ian Campbell
2012-May-18 15:25 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
On Fri, 2012-05-18 at 16:16 +0100, Roger Pau Monne wrote:> Ian Campbell wrote: > > On Fri, 2012-05-18 at 15:55 +0100, Roger Pau Monne wrote: > >>> @@ -429,6 +429,46 @@ libxl_device_model_version > >> libxl__device_model_version_running(libxl__gc *gc, > >>> return value; > >>> } > >>> > >>> +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > >>> + libxl_device_disk *disk, > >>> + libxl__device *device) > >> I think this should be moved to libxl_device instead of > >> libxl_internal, since it''s a device related function. > > > > I think it''d be better to leave it in the original file, libxl is so > > confused about what goes where that it doesn''t really matter... > > > > If we do want to move it we can do it later as a pure code motion > > change. > > In my hotplug series I''m moving most libxl_device_{...}_add to provide > an internal implementation that takes an ao, making something quite > similar to what Stefano does, if I start moving all those to > libxl_internal we will fill this file with functions that could be > somewhere else (libxl_device).I didn''t propose moving this function tolibxl_internal, quite the opposite. I proposed leaving it in libxl.c where it is (or rather , where the body current effectively is). If and when we want to move it we can do that as a separate pure code motion change.> I understand that the libxl policy is put > functions where they seem to belong (device related functions to > libxl_device, domain related ones to libxl_dom...),The policy is basically "confused, historical, no one really knows for sure".> and if they fit in > none of this categories then put them in libxl_internal or create a new > file.IMHO libxl_internal should go away and never be used, it just encourages things to go into a dumping ground file, which is effectively what it has become. Ian.
Stefano Stabellini
2012-May-21 16:30 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
On Fri, 18 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v6 05/11] 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. > > Can''t this be done in such a way that the diff isn''t "delete this > function completely" followed by "here is a new function" ?I don''t know. Do you have any suggestions? TBH if I were you, I would just open two terminals and compare line by line, but let me know if you want to do something specific..> I don''t really understand why you want to move the function at all, > TBH. I think keeping the public wrapper and the internal > implementation together is fine. There is no need IMO to move the > internal function to libxl_internal.c.ATM there are no hidden functions in libxl.c. Do you want to change this "policy"?
Stefano Stabellini
2012-May-21 16:31 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
On Fri, 18 May 2012, Ian Jackson wrote:> Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c"): > > In my hotplug series I''m moving most libxl_device_{...}_add to provide > > an internal implementation that takes an ao, making something quite > > similar to what Stefano does, if I start moving all those to > > libxl_internal we will fill this file with functions that could be > > somewhere else (libxl_device). I understand that the libxl policy is put > > functions where they seem to belong (device related functions to > > libxl_device, domain related ones to libxl_dom...), and if they fit in > > none of this categories then put them in libxl_internal or create a new > > file. > > > > We can leave it in libxl_internal for now, and I will move it on my series. > > I don''t think we have a policy. I think at this stage I would much > prefer not to move whole functions from one file to another for purely > cosmetic reasons. > > (It''s a different question if you need to move the body of a function > into the middle of another one, or something.)The policy used to be "no internal functions in libxl.c". If you want to change it, just let me know.
Stefano Stabellini
2012-May-21 16:33 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
On Fri, 18 May 2012, Ian Campbell wrote:> On Fri, 2012-05-18 at 16:16 +0100, Roger Pau Monne wrote: > > Ian Campbell wrote: > > > On Fri, 2012-05-18 at 15:55 +0100, Roger Pau Monne wrote: > > >>> @@ -429,6 +429,46 @@ libxl_device_model_version > > >> libxl__device_model_version_running(libxl__gc *gc, > > >>> return value; > > >>> } > > >>> > > >>> +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > > >>> + libxl_device_disk *disk, > > >>> + libxl__device *device) > > >> I think this should be moved to libxl_device instead of > > >> libxl_internal, since it''s a device related function. > > > > > > I think it''d be better to leave it in the original file, libxl is so > > > confused about what goes where that it doesn''t really matter... > > > > > > If we do want to move it we can do it later as a pure code motion > > > change. > > > > In my hotplug series I''m moving most libxl_device_{...}_add to provide > > an internal implementation that takes an ao, making something quite > > similar to what Stefano does, if I start moving all those to > > libxl_internal we will fill this file with functions that could be > > somewhere else (libxl_device). > > I didn''t propose moving this function tolibxl_internal, quite the > opposite. I proposed leaving it in libxl.c where it is (or rather , > where the body current effectively is). If and when we want to move it > we can do that as a separate pure code motion change.Do we have an agreement that I should repost this patch keeping libxl__device_from_disk in libxl.c?
Stefano Stabellini
2012-May-21 16:34 UTC
Re: [PATCH v6 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
On Fri, 18 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v6 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"): > > Introduce a new libxl_device_disk* parameter to > > libxl__device_disk_local_attach, the parameter is allocated by the > > caller. libxl__device_disk_local_attach is going to fill the new disk > > with informations about the new locally attached disk. The new > > libxl_device_disk should be passed to libxl_device_disk_local_detach > > afterwards. > > In this declaration: > > > @@ -1767,6 +1768,7 @@ struct libxl__bootloader_state { > > libxl__bootloader_console_callback *console_available; > > libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */ > > libxl_device_disk *disk; > > + libxl_device_disk tmpdisk; > > uint32_t domid; > > We need information about what this "tmpdisk" is. All of the other > parameters here are input parameters, except as otherwise noted in the > comment. > > Also I''m not convinced that "tmpdisk" is quite the right name. You > also need to explain the distinction between "disk" and "tmpdisk". > > Perhaps: > > const libxl_device_disk *disk; /* as specified by user */ > libxl_device_disk localdisk; > /* Should be zeroed by caller on entry. Will be filled in by > * bootloader machinery; represents the local attachment of the > * disk for the benefit of the bootloader. Must be detached by > * the caller using libxl__device_disk_local_detach, but only > * after the domain''s kernel and initramfs have been loaded into > * memory and the file references disposed of. */ > > ?fine by me
Ian Campbell
2012-May-21 16:34 UTC
Re: [PATCH v6 04/11] libxl: move libxl__device_from_disk to libxl_internal.c
On Mon, 2012-05-21 at 17:33 +0100, Stefano Stabellini wrote:> On Fri, 18 May 2012, Ian Campbell wrote: > > On Fri, 2012-05-18 at 16:16 +0100, Roger Pau Monne wrote: > > > Ian Campbell wrote: > > > > On Fri, 2012-05-18 at 15:55 +0100, Roger Pau Monne wrote: > > > >>> @@ -429,6 +429,46 @@ libxl_device_model_version > > > >> libxl__device_model_version_running(libxl__gc *gc, > > > >>> return value; > > > >>> } > > > >>> > > > >>> +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid, > > > >>> + libxl_device_disk *disk, > > > >>> + libxl__device *device) > > > >> I think this should be moved to libxl_device instead of > > > >> libxl_internal, since it''s a device related function. > > > > > > > > I think it''d be better to leave it in the original file, libxl is so > > > > confused about what goes where that it doesn''t really matter... > > > > > > > > If we do want to move it we can do it later as a pure code motion > > > > change. > > > > > > In my hotplug series I''m moving most libxl_device_{...}_add to provide > > > an internal implementation that takes an ao, making something quite > > > similar to what Stefano does, if I start moving all those to > > > libxl_internal we will fill this file with functions that could be > > > somewhere else (libxl_device). > > > > I didn''t propose moving this function tolibxl_internal, quite the > > opposite. I proposed leaving it in libxl.c where it is (or rather , > > where the body current effectively is). If and when we want to move it > > we can do that as a separate pure code motion change. > > Do we have an agreement that I should repost this patch keeping > libxl__device_from_disk in libxl.c?I agree that this is fine. I''d never heard about the "no internal functions in libxl.c" policy and I don''t think it is a good one ;-) Ian.
Ian Jackson
2012-May-21 16:48 UTC
Re: [PATCH v6 05/11] libxl: introduce libxl__device_disk_add
Stefano Stabellini writes ("Re: [PATCH v6 05/11] libxl: introduce
libxl__device_disk_add"):> On Fri, 18 May 2012, Ian Jackson wrote:
> > Can''t this be done in such a way that the diff isn''t
"delete this
> > function completely" followed by "here is a new
function" ?
>
> I don''t know. Do you have any suggestions? TBH if I were you, I
would
> just open two terminals and compare line by line, but let me know if you
> want to do something specific..
If you leave all the existing code for the internal function in place
and simply change the function declaration and prologue, and
corresponding epilogue, you should find that diff doesn''t mention the
contents at all.
The new public wrapper will of course be new.
> ATM there are no hidden functions in libxl.c.
> Do you want to change this "policy"?
I think there is no reason for this and it is not something I want to
preserve. The distinction between internal and external is not a good
basis for which file something should be in.
Ian.
Stefano Stabellini
2012-May-21 17:50 UTC
Re: [PATCH v6 07/11] libxl: introduce libxl__alloc_vdev
On Fri, 18 May 2012, Ian Jackson wrote:> Stefano Stabellini writes ("[PATCH v6 07/11] libxl: introduce libxl__alloc_vdev"): > > Introduce libxl__alloc_vdev: find a spare virtual block device in the > > domain passed as argument. > ... > > +/* Same as in Linux. > > + * The maximum buffer length is 32 bytes. > > + * The code is safe thanks to the upper bound enforced by the caller. */ > > +static char *encode_disk_name(char *ptr, unsigned int n) > > So this says that encode_disk_name may use up to 32 bytes in *ptr > (including or excluding the trailing nul?) > > _Why_ may the maximum buffer length used be 32 bytes ? Also, "maximum > buffer length" is a confusing phrase because it seems to refer to the > /actual/ length of the buffer rather than the amount /used/. > > And this... > > > + char *ret = libxl__zalloc(gc, 32); > > ... allocates a 32 byte buffer which we then fill with ... > > > + strcpy(ret, "xvd"); > > + ptr = encode_disk_name(ret + 3, offset); > > 3 characters plus the encoded disk name. So possibly using up to 36 > bytes. > > In fact it seems to me that there could be a much tighter upper bound > on the length of output of encode_disk_name (based on arithmetic) but > it needs to be properly calculated and documented and perhaps put in a > #define.If my math is correct there is no need to enforce an upper limit because even if we suppose that offset is ULONG_MAX on 64 bits it would still be lower than 26 elevated at 28 that is the actual upper limit. I am going to write this in a comment.
Stefano Stabellini
2012-May-21 18:02 UTC
Re: [PATCH v6 07/11] libxl: introduce libxl__alloc_vdev
On Mon, 21 May 2012, Stefano Stabellini wrote:> On Fri, 18 May 2012, Ian Jackson wrote: > > Stefano Stabellini writes ("[PATCH v6 07/11] libxl: introduce libxl__alloc_vdev"): > > > Introduce libxl__alloc_vdev: find a spare virtual block device in the > > > domain passed as argument. > > ... > > > +/* Same as in Linux. > > > + * The maximum buffer length is 32 bytes. > > > + * The code is safe thanks to the upper bound enforced by the caller. */ > > > +static char *encode_disk_name(char *ptr, unsigned int n) > > > > So this says that encode_disk_name may use up to 32 bytes in *ptr > > (including or excluding the trailing nul?) > > > > _Why_ may the maximum buffer length used be 32 bytes ? Also, "maximum > > buffer length" is a confusing phrase because it seems to refer to the > > /actual/ length of the buffer rather than the amount /used/. > > > > And this... > > > > > + char *ret = libxl__zalloc(gc, 32); > > > > ... allocates a 32 byte buffer which we then fill with ... > > > > > + strcpy(ret, "xvd"); > > > + ptr = encode_disk_name(ret + 3, offset); > > > > 3 characters plus the encoded disk name. So possibly using up to 36 > > bytes. > > > > In fact it seems to me that there could be a much tighter upper bound > > on the length of output of encode_disk_name (based on arithmetic) but > > it needs to be properly calculated and documented and perhaps put in a > > #define. > > If my math is correct there is no need to enforce an upper limit because > even if we suppose that offset is ULONG_MAX on 64 bits it would still be > lower than 26 elevated at 28 that is the actual upper limit.I meant 26 raised to the power of 28
Stefano Stabellini writes ("Re: [PATCH v6 07/11] libxl: introduce
libxl__alloc_vdev"):> If my math is correct there is no need to enforce an upper limit because
> even if we suppose that offset is ULONG_MAX on 64 bits it would still be
> lower than 26 elevated at 28 that is the actual upper limit.
Right. (In English you mean "26 to the power of 28" or you might
choose to write "26^28".)
> I am going to write this in a comment.
Yes, that''s what I was trying to get at.
Ian.