* [PATCH V2] libxl: make libxl_cdrom_insert async
@ 2012-07-25 9:12 Ian Campbell
2012-07-25 9:20 ` Ian Campbell
0 siblings, 1 reply; 2+ messages in thread
From: Ian Campbell @ 2012-07-25 9:12 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1343207529 -3600
# Node ID 4cb46317b245e0d4c729e3500bf03fdecf4ae4fa
# Parent fe5c863907a4988cd3f98087c393e23125d3ee15
libxl: make libxl_cdrom_insert async.
This functionality is a bit of a mess and several configurations are
not properly supported.
The protocol for changing is basically to change the params node in
the disk xenstore backend. There is no interlock or error reporting in
this protocol. Completely removing the device and recreating it is not
necessary nor expected. For reference the equivalent xend code is
tools/python/xen/xend/server/blkif.py::BlkifController::reconfigureDevice().
Device model stub domains are not supported. There appears to be no
way correctly to do a media change on the emulated device while also
changing the stub domains PV backend to point to the new
backend. Reworking this is a significant task deferred until 4.3. xend
(via the equivalent "xm block-configure" functionality) also does not
support media change for stub domains (confirmed by code inspection
and experiment). Unlike xend this version errors out instead of
silently not achieving anything in this case.
There is no support for qemu-xen (upstream) media change. I expect
this is supported on the qemu side and required QMP plumbing on the
libxl side. Again this is deferred until 4.3.
On the plus side the current implementation is trivially "asynchronous".
Adds a libxl__xs_writev_atonce helper to write a key-value list to
xenstore in one go.
Tested with Windows 7.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2:
- use GCSPRINTF and libxl__strup(NOGC,...)
- remove incorrect comment copied from switch_logdirty_xswatch
- don't clear then set the prarms key -- just set it
diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Wed Jul 25 09:48:09 2012 +0100
+++ b/tools/libxl/libxl.c Wed Jul 25 10:12:09 2012 +0100
@@ -2131,17 +2131,49 @@ int libxl_device_disk_getinfo(libxl_ctx
return 0;
}
-int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
+int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how)
{
- int num, i;
- uint32_t stubdomid;
- libxl_device_disk *disks;
- int ret = ERROR_FAIL;
-
- if (!disk->pdev_path) {
- disk->pdev_path = strdup("");
- disk->format = LIBXL_DISK_FORMAT_EMPTY;
+ AO_CREATE(ctx, domid, ao_how);
+ int num = 0, i;
+ libxl_device_disk *disks = NULL;
+ int rc, dm_ver;
+
+ libxl__device device;
+ const char * path;
+
+ flexarray_t *insert = NULL;
+
+ libxl_domain_type type = libxl__domain_type(gc, domid);
+ if (type == LIBXL_DOMAIN_TYPE_INVALID) {
+ rc = ERROR_FAIL;
+ goto out;
}
+ if (type != LIBXL_DOMAIN_TYPE_HVM) {
+ LOG(ERROR, "cdrom-insert requires an HVM domain");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ if (libxl_get_stubdom_id(ctx, domid) != 0) {
+ LOG(ERROR, "cdrom-insert doesn't work for stub domains");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ dm_ver = libxl__device_model_version_running(gc, domid);
+ if (dm_ver == -1) {
+ LOG(ERROR, "cannot determine device model version");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ if (dm_ver != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
+ LOG(ERROR, "cdrom-insert does not work with %s",
+ libxl_device_model_version_to_string(dm_ver));
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
disks = libxl_device_disk_list(ctx, domid, &num);
for (i = 0; i < num; i++) {
if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
@@ -2150,25 +2182,52 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
}
if (i == num) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
+ rc = ERROR_FAIL;
goto out;
}
- ret = 0;
-
- libxl_device_disk_remove(ctx, domid, disks + i, 0);
- /* fixme-ao */
- libxl_device_disk_add(ctx, domid, disk, 0);
- stubdomid = libxl_get_stubdom_id(ctx, domid);
- if (stubdomid) {
- libxl_device_disk_remove(ctx, stubdomid, disks + i, 0);
- /* fixme-ao */
- libxl_device_disk_add(ctx, stubdomid, disk, 0);
+ rc = libxl__device_disk_setdefault(gc, disk);
+ if (rc) goto out;
+
+ if (!disk->pdev_path) {
+ disk->pdev_path = libxl__strdup(NOGC, "");
+ disk->format = LIBXL_DISK_FORMAT_EMPTY;
}
+
+ rc = libxl__device_from_disk(gc, domid, disk, &device);
+ if (rc) goto out;
+ path = libxl__device_backend_path(gc, &device);
+
+ insert = flexarray_make(4, 1);
+
+ flexarray_append_pair(insert, "type",
+ libxl__device_disk_string_of_backend(disk->backend));
+ if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
+ flexarray_append_pair(insert, "params",
+ GCSPRINTF("%s:%s",
+ libxl__device_disk_string_of_format(disk->format),
+ disk->pdev_path));
+ else
+ flexarray_append_pair(insert, "params", "");
+
+ rc = libxl__xs_writev_atonce(gc, path,
+ libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
+ if (rc) goto out;
+
+ /* success, no actual async */
+ libxl__ao_complete(egc, ao, 0);
+
+ rc = 0;
+
out:
for (i = 0; i < num; i++)
libxl_device_disk_dispose(&disks[i]);
free(disks);
- return ret;
+
+ if (insert) flexarray_free(insert);
+
+ if (rc) return AO_ABORT(rc);
+ return AO_INPROGRESS;
}
/* libxl__alloc_vdev only works on the local domain, that is the domain
diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Wed Jul 25 09:48:09 2012 +0100
+++ b/tools/libxl/libxl.h Wed Jul 25 10:12:09 2012 +0100
@@ -693,7 +693,8 @@ int libxl_device_disk_getinfo(libxl_ctx
* Insert a CD-ROM device. A device corresponding to disk must already
* be attached to the guest.
*/
-int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
+int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
+ const libxl_asyncop_how *ao_how);
/* Network Interfaces */
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Wed Jul 25 09:48:09 2012 +0100
+++ b/tools/libxl/libxl_internal.h Wed Jul 25 10:12:09 2012 +0100
@@ -501,8 +501,13 @@ _hidden int libxl__remove_file_or_direct
_hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
+/* treats kvs as pairs of keys and values and writes each to dir. */
_hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
const char *dir, char **kvs);
+/* _atonce creates a transaction and writes all keys at once */
+_hidden int libxl__xs_writev_atonce(libxl__gc *gc,
+ const char *dir, char **kvs);
+
_hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
/* Each fn returns 0 on success.
diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_xshelp.c
--- a/tools/libxl/libxl_xshelp.c Wed Jul 25 09:48:09 2012 +0100
+++ b/tools/libxl/libxl_xshelp.c Wed Jul 25 10:12:09 2012 +0100
@@ -61,6 +61,31 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
return 0;
}
+int libxl__xs_writev_atonce(libxl__gc *gc,
+ const char *dir, char *kvs[])
+{
+ int rc;
+ xs_transaction_t t = XBT_NULL;
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ rc = libxl__xs_writev(gc, t, dir, kvs);
+ if (rc) goto out;
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc<0) goto out;
+ }
+
+out:
+ libxl__xs_transaction_abort(gc, &t);
+
+ return rc;
+
+}
+
int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
const char *path, const char *fmt, ...)
{
diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed Jul 25 09:48:09 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed Jul 25 10:12:09 2012 +0100
@@ -2000,7 +2000,7 @@ start:
case LIBXL_EVENT_TYPE_DISK_EJECT:
/* XXX what is this for? */
- libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk);
+ libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk, NULL);
break;
default:;
@@ -2220,7 +2220,7 @@ static void cd_insert(const char *dom, c
disk.backend_domid = 0;
- libxl_cdrom_insert(ctx, domid, &disk);
+ libxl_cdrom_insert(ctx, domid, &disk, NULL);
libxl_device_disk_dispose(&disk);
free(buf);
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH V2] libxl: make libxl_cdrom_insert async
2012-07-25 9:12 [PATCH V2] libxl: make libxl_cdrom_insert async Ian Campbell
@ 2012-07-25 9:20 ` Ian Campbell
0 siblings, 0 replies; 2+ messages in thread
From: Ian Campbell @ 2012-07-25 9:20 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
I forgot to say... this is actually based on Roger's series because I
was expecting to need async disk add/remove, but as it turns out I don't
so I could rebase before that series.
On Wed, 2012-07-25 at 10:12 +0100, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1343207529 -3600
> # Node ID 4cb46317b245e0d4c729e3500bf03fdecf4ae4fa
> # Parent fe5c863907a4988cd3f98087c393e23125d3ee15
> libxl: make libxl_cdrom_insert async.
>
> This functionality is a bit of a mess and several configurations are
> not properly supported.
>
> The protocol for changing is basically to change the params node in
> the disk xenstore backend. There is no interlock or error reporting in
> this protocol. Completely removing the device and recreating it is not
> necessary nor expected. For reference the equivalent xend code is
> tools/python/xen/xend/server/blkif.py::BlkifController::reconfigureDevice().
>
> Device model stub domains are not supported. There appears to be no
> way correctly to do a media change on the emulated device while also
> changing the stub domains PV backend to point to the new
> backend. Reworking this is a significant task deferred until 4.3. xend
> (via the equivalent "xm block-configure" functionality) also does not
> support media change for stub domains (confirmed by code inspection
> and experiment). Unlike xend this version errors out instead of
> silently not achieving anything in this case.
>
> There is no support for qemu-xen (upstream) media change. I expect
> this is supported on the qemu side and required QMP plumbing on the
> libxl side. Again this is deferred until 4.3.
>
> On the plus side the current implementation is trivially "asynchronous".
>
> Adds a libxl__xs_writev_atonce helper to write a key-value list to
> xenstore in one go.
>
> Tested with Windows 7.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>
> v2:
> - use GCSPRINTF and libxl__strup(NOGC,...)
> - remove incorrect comment copied from switch_logdirty_xswatch
> - don't clear then set the prarms key -- just set it
>
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl.c Wed Jul 25 10:12:09 2012 +0100
> @@ -2131,17 +2131,49 @@ int libxl_device_disk_getinfo(libxl_ctx
> return 0;
> }
>
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
> +int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
> + const libxl_asyncop_how *ao_how)
> {
> - int num, i;
> - uint32_t stubdomid;
> - libxl_device_disk *disks;
> - int ret = ERROR_FAIL;
> -
> - if (!disk->pdev_path) {
> - disk->pdev_path = strdup("");
> - disk->format = LIBXL_DISK_FORMAT_EMPTY;
> + AO_CREATE(ctx, domid, ao_how);
> + int num = 0, i;
> + libxl_device_disk *disks = NULL;
> + int rc, dm_ver;
> +
> + libxl__device device;
> + const char * path;
> +
> + flexarray_t *insert = NULL;
> +
> + libxl_domain_type type = libxl__domain_type(gc, domid);
> + if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> + rc = ERROR_FAIL;
> + goto out;
> }
> + if (type != LIBXL_DOMAIN_TYPE_HVM) {
> + LOG(ERROR, "cdrom-insert requires an HVM domain");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + if (libxl_get_stubdom_id(ctx, domid) != 0) {
> + LOG(ERROR, "cdrom-insert doesn't work for stub domains");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + dm_ver = libxl__device_model_version_running(gc, domid);
> + if (dm_ver == -1) {
> + LOG(ERROR, "cannot determine device model version");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + if (dm_ver != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
> + LOG(ERROR, "cdrom-insert does not work with %s",
> + libxl_device_model_version_to_string(dm_ver));
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> disks = libxl_device_disk_list(ctx, domid, &num);
> for (i = 0; i < num; i++) {
> if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
> @@ -2150,25 +2182,52 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
> }
> if (i == num) {
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
> + rc = ERROR_FAIL;
> goto out;
> }
>
> - ret = 0;
> -
> - libxl_device_disk_remove(ctx, domid, disks + i, 0);
> - /* fixme-ao */
> - libxl_device_disk_add(ctx, domid, disk, 0);
> - stubdomid = libxl_get_stubdom_id(ctx, domid);
> - if (stubdomid) {
> - libxl_device_disk_remove(ctx, stubdomid, disks + i, 0);
> - /* fixme-ao */
> - libxl_device_disk_add(ctx, stubdomid, disk, 0);
> + rc = libxl__device_disk_setdefault(gc, disk);
> + if (rc) goto out;
> +
> + if (!disk->pdev_path) {
> + disk->pdev_path = libxl__strdup(NOGC, "");
> + disk->format = LIBXL_DISK_FORMAT_EMPTY;
> }
> +
> + rc = libxl__device_from_disk(gc, domid, disk, &device);
> + if (rc) goto out;
> + path = libxl__device_backend_path(gc, &device);
> +
> + insert = flexarray_make(4, 1);
> +
> + flexarray_append_pair(insert, "type",
> + libxl__device_disk_string_of_backend(disk->backend));
> + if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
> + flexarray_append_pair(insert, "params",
> + GCSPRINTF("%s:%s",
> + libxl__device_disk_string_of_format(disk->format),
> + disk->pdev_path));
> + else
> + flexarray_append_pair(insert, "params", "");
> +
> + rc = libxl__xs_writev_atonce(gc, path,
> + libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
> + if (rc) goto out;
> +
> + /* success, no actual async */
> + libxl__ao_complete(egc, ao, 0);
> +
> + rc = 0;
> +
> out:
> for (i = 0; i < num; i++)
> libxl_device_disk_dispose(&disks[i]);
> free(disks);
> - return ret;
> +
> + if (insert) flexarray_free(insert);
> +
> + if (rc) return AO_ABORT(rc);
> + return AO_INPROGRESS;
> }
>
> /* libxl__alloc_vdev only works on the local domain, that is the domain
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl.h Wed Jul 25 10:12:09 2012 +0100
> @@ -693,7 +693,8 @@ int libxl_device_disk_getinfo(libxl_ctx
> * Insert a CD-ROM device. A device corresponding to disk must already
> * be attached to the guest.
> */
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
> +int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
> + const libxl_asyncop_how *ao_how);
>
> /* Network Interfaces */
> int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl_internal.h Wed Jul 25 10:12:09 2012 +0100
> @@ -501,8 +501,13 @@ _hidden int libxl__remove_file_or_direct
>
> _hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
>
> +/* treats kvs as pairs of keys and values and writes each to dir. */
> _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
> const char *dir, char **kvs);
> +/* _atonce creates a transaction and writes all keys at once */
> +_hidden int libxl__xs_writev_atonce(libxl__gc *gc,
> + const char *dir, char **kvs);
> +
> _hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
> const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
> /* Each fn returns 0 on success.
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_xshelp.c
> --- a/tools/libxl/libxl_xshelp.c Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl_xshelp.c Wed Jul 25 10:12:09 2012 +0100
> @@ -61,6 +61,31 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
> return 0;
> }
>
> +int libxl__xs_writev_atonce(libxl__gc *gc,
> + const char *dir, char *kvs[])
> +{
> + int rc;
> + xs_transaction_t t = XBT_NULL;
> +
> + for (;;) {
> + rc = libxl__xs_transaction_start(gc, &t);
> + if (rc) goto out;
> +
> + rc = libxl__xs_writev(gc, t, dir, kvs);
> + if (rc) goto out;
> +
> + rc = libxl__xs_transaction_commit(gc, &t);
> + if (!rc) break;
> + if (rc<0) goto out;
> + }
> +
> +out:
> + libxl__xs_transaction_abort(gc, &t);
> +
> + return rc;
> +
> +}
> +
> int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
> const char *path, const char *fmt, ...)
> {
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c Wed Jul 25 10:12:09 2012 +0100
> @@ -2000,7 +2000,7 @@ start:
>
> case LIBXL_EVENT_TYPE_DISK_EJECT:
> /* XXX what is this for? */
> - libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk);
> + libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk, NULL);
> break;
>
> default:;
> @@ -2220,7 +2220,7 @@ static void cd_insert(const char *dom, c
>
> disk.backend_domid = 0;
>
> - libxl_cdrom_insert(ctx, domid, &disk);
> + libxl_cdrom_insert(ctx, domid, &disk, NULL);
>
> libxl_device_disk_dispose(&disk);
> free(buf);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-07-25 9:20 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-25 9:12 [PATCH V2] libxl: make libxl_cdrom_insert async Ian Campbell
2012-07-25 9:20 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.