* [PATCH v5 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 11:13 ` [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
` (7 subsequent siblings)
8 siblings, 0 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Changes in v5:
- remove _hidden from the implementation.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 73 ----------------------------------------
tools/libxl/libxl.h | 7 ----
tools/libxl/libxl_bootloader.c | 4 +-
tools/libxl/libxl_internal.c | 72 +++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 9 +++++
5 files changed, 83 insertions(+), 82 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9984d46..1357efc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1664,79 +1664,6 @@ out:
return ret;
}
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
- GC_INIT(ctx);
- char *dev = NULL;
- char *ret = NULL;
- int rc;
-
- rc = libxl__device_disk_setdefault(gc, disk);
- if (rc) goto out;
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- case LIBXL_DISK_BACKEND_TAP:
- switch (disk->format) {
- case LIBXL_DISK_FORMAT_RAW:
- /* optimise away the early tapdisk attach in this case */
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
- " tap disk %s directly (ie without using blktap)",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- case LIBXL_DISK_FORMAT_VHD:
- dev = libxl__blktap_devpath(gc, disk->pdev_path,
- disk->format);
- break;
- case LIBXL_DISK_FORMAT_QCOW:
- case LIBXL_DISK_FORMAT_QCOW2:
- abort(); /* prevented by libxl__device_disk_set_backend */
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "unrecognized disk format: %d", disk->format);
- break;
- }
- break;
- case LIBXL_DISK_BACKEND_QDISK:
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
- " attach a qdisk image if the format is not raw");
- break;
- }
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
- "type: %d", disk->backend);
- break;
- }
-
- out:
- if (dev != NULL)
- ret = strdup(dev);
- GC_FREE;
- return ret;
-}
-
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
- /* Nothing to do for PHYSTYPE_PHY. */
-
- /*
- * For other device types assume that the blktap2 process is
- * needed by the soon to be started domain and do nothing.
- */
-
- return 0;
-}
-
/******************************************************************************/
int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index d59f0ee..79d5679 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -619,13 +619,6 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
*/
int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
-/*
- * Make a disk available in this (the control) domain. Returns path to
- * a device.
- */
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
-
/* Network Interfaces */
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 2774062..977c6d3 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -386,7 +386,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl_device_disk_local_attach(ctx, disk);
+ diskpath = libxl__device_disk_local_attach(gc, disk);
if (!diskpath) {
goto out_close;
}
@@ -453,7 +453,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
rc = 0;
out_close:
if (diskpath) {
- libxl_device_disk_local_detach(ctx, disk);
+ libxl__device_disk_local_detach(gc, disk);
free(diskpath);
}
if (fifo_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b89aef7..fc771ff 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,78 @@ out:
return rc;
}
+char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+{
+ libxl_ctx *ctx = gc->owner;
+ char *dev = NULL;
+ char *ret = NULL;
+ int rc;
+
+ rc = libxl__device_disk_setdefault(gc, disk);
+ if (rc) goto out;
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ switch (disk->format) {
+ case LIBXL_DISK_FORMAT_RAW:
+ /* optimise away the early tapdisk attach in this case */
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
+ " tap disk %s directly (ie without using blktap)",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ case LIBXL_DISK_FORMAT_VHD:
+ dev = libxl__blktap_devpath(gc, disk->pdev_path,
+ disk->format);
+ break;
+ case LIBXL_DISK_FORMAT_QCOW:
+ case LIBXL_DISK_FORMAT_QCOW2:
+ abort(); /* prevented by libxl__device_disk_set_backend */
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "unrecognized disk format: %d", disk->format);
+ break;
+ }
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
+ " attach a qdisk image if the format is not raw");
+ break;
+ }
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
+ "type: %d", disk->backend);
+ break;
+ }
+
+ out:
+ if (dev != NULL)
+ ret = strdup(dev);
+ return ret;
+}
+
+int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
+{
+ /* Nothing to do for PHYSTYPE_PHY. */
+
+ /*
+ * For other device types assume that the blktap2 process is
+ * needed by the soon to be started domain and do nothing.
+ */
+
+ return 0;
+}
+
libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc,
uint32_t domid)
{
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 34ea15c..ddd6a37 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1003,6 +1003,15 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
*/
_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+/*
+ * Make a disk available in this (the control) domain. Returns path to
+ * a device.
+ */
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+ libxl_device_disk *disk);
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
+ libxl_device_disk *disk);
+
_hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
struct libxl__xen_console_reader {
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
2012-05-04 11:13 ` [PATCH v5 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 14:40 ` Ian Campbell
2012-05-04 16:27 ` Ian Jackson
2012-05-04 11:13 ` [PATCH v5 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
` (6 subsequent siblings)
8 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce a new libxl_device_disk** parameter to
libxl__device_disk_local_attach, the parameter is allocated on the gc by
libxl__device_disk_local_attach. It is going to fill it with
informations about the new locally attached disk. The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.
Changes in v5:
- rename disk to in_disk;
- rename tmpdisk to disk;
- copy disk to new_disk only on success;
- remove check on libxl__zalloc success.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_bootloader.c | 10 +++++-----
tools/libxl/libxl_internal.c | 20 ++++++++++++++------
tools/libxl/libxl_internal.h | 3 ++-
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 977c6d3..83cac78 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
char *fifo = NULL;
char *diskpath = NULL;
char **args = NULL;
+ libxl_device_disk *tmpdisk = NULL;
char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
char *tempdir;
@@ -386,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl__device_disk_local_attach(gc, disk);
+ diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
if (!diskpath) {
+ rc = ERROR_FAIL;
goto out_close;
}
@@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
rc = 0;
out_close:
- if (diskpath) {
- libxl__device_disk_local_detach(gc, disk);
- free(diskpath);
- }
+ if (tmpdisk)
+ libxl__device_disk_local_detach(gc, tmpdisk);
if (fifo_fd > -1)
close(fifo_fd);
if (bootloader_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index fc771ff..55dc55c 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,12 +323,21 @@ out:
return rc;
}
-char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+char * libxl__device_disk_local_attach(libxl__gc *gc,
+ const libxl_device_disk *in_disk,
+ libxl_device_disk **new_disk)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
- char *ret = NULL;
int rc;
+ libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
+
+ memcpy(disk, in_disk, sizeof(libxl_device_disk));
+ if (in_disk->pdev_path != NULL)
+ disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
+ if (in_disk->script != NULL)
+ disk->script = libxl__strdup(gc, in_disk->script);
+ disk->vdev = NULL;
rc = libxl__device_disk_setdefault(gc, disk);
if (rc) goto out;
@@ -368,7 +377,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
break;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
- disk->pdev_path);
+ in_disk->pdev_path);
dev = disk->pdev_path;
break;
default:
@@ -378,9 +387,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
}
out:
- if (dev != NULL)
- ret = strdup(dev);
- return ret;
+ *new_disk = disk;
+ return dev;
}
int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ddd6a37..965d13b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1008,7 +1008,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
* a device.
*/
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
- libxl_device_disk *disk);
+ const libxl_device_disk *in_disk,
+ libxl_device_disk **new_disk);
_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
libxl_device_disk *disk);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-04 11:13 ` [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
@ 2012-05-04 14:40 ` Ian Campbell
2012-05-04 16:27 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 14:40 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce a new libxl_device_disk** parameter to
> libxl__device_disk_local_attach, the parameter is allocated on the gc by
> libxl__device_disk_local_attach. It is going to fill it with
> informations about the new locally attached disk. The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
>
> Changes in v5:
> - rename disk to in_disk;
> - rename tmpdisk to disk;
> - copy disk to new_disk only on success;
> - remove check on libxl__zalloc success.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/libxl/libxl_bootloader.c | 10 +++++-----
> tools/libxl/libxl_internal.c | 20 ++++++++++++++------
> tools/libxl/libxl_internal.h | 3 ++-
> 3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 977c6d3..83cac78 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> char *fifo = NULL;
> char *diskpath = NULL;
> char **args = NULL;
> + libxl_device_disk *tmpdisk = NULL;
Do you need to keep tmpdisk valid outside the scope of this function?
You don't seem to in this patch (I didn't look over the next patches
yet).
If not then you could just use "libxl_device_disk tmpdisk" and have
libxl__device_disk_local_attach update in place instead allocating and
passing the pointer back.
> char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
> char *tempdir;
> @@ -386,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> goto out_close;
> }
>
> - diskpath = libxl__device_disk_local_attach(gc, disk);
> + diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
> if (!diskpath) {
> + rc = ERROR_FAIL;
> goto out_close;
> }
>
> @@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>
> rc = 0;
> out_close:
> - if (diskpath) {
> - libxl__device_disk_local_detach(gc, disk);
> - free(diskpath);
> - }
> + if (tmpdisk)
> + libxl__device_disk_local_detach(gc, tmpdisk);
> if (fifo_fd > -1)
> close(fifo_fd);
> if (bootloader_fd > -1)
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index fc771ff..55dc55c 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,12 +323,21 @@ out:
> return rc;
> }
>
> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> + const libxl_device_disk *in_disk,
> + libxl_device_disk **new_disk)
> {
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> - char *ret = NULL;
> int rc;
> + libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
> +
> + memcpy(disk, in_disk, sizeof(libxl_device_disk));
> + if (in_disk->pdev_path != NULL)
> + disk->pdev_path = libxl__strdup(gc, in_disk->pdev_path);
> + if (in_disk->script != NULL)
> + disk->script = libxl__strdup(gc, in_disk->script);
> + disk->vdev = NULL;
>
> rc = libxl__device_disk_setdefault(gc, disk);
> if (rc) goto out;
> @@ -368,7 +377,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> break;
> }
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> - disk->pdev_path);
> + in_disk->pdev_path);
> dev = disk->pdev_path;
> break;
> default:
> @@ -378,9 +387,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> }
>
> out:
> - if (dev != NULL)
> - ret = strdup(dev);
> - return ret;
> + *new_disk = disk;
> + return dev;
> }
>
> int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index ddd6a37..965d13b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1008,7 +1008,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
> * a device.
> */
> _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> - libxl_device_disk *disk);
> + const libxl_device_disk *in_disk,
> + libxl_device_disk **new_disk);
> _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
> libxl_device_disk *disk);
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-04 11:13 ` [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
2012-05-04 14:40 ` Ian Campbell
@ 2012-05-04 16:27 ` Ian Jackson
2012-05-14 13:04 ` Stefano Stabellini
1 sibling, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:27 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("[PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> Introduce a new libxl_device_disk** parameter to
> libxl__device_disk_local_attach, the parameter is allocated on the gc by
> libxl__device_disk_local_attach. It is going to fill it with
> informations about the new locally attached disk. The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
...
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index fc771ff..55dc55c 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
...
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> - disk->pdev_path);
> + in_disk->pdev_path);
I think in_disk->pdev_path can be NULL here ?
Also log messages should not contain "\n"; the logging functions add
it.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-04 16:27 ` Ian Jackson
@ 2012-05-14 13:04 ` Stefano Stabellini
2012-05-14 13:06 ` Ian Campbell
2012-05-14 14:15 ` Ian Jackson
0 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 13:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Fri, 4 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > Introduce a new libxl_device_disk** parameter to
> > libxl__device_disk_local_attach, the parameter is allocated on the gc by
> > libxl__device_disk_local_attach. It is going to fill it with
> > informations about the new locally attached disk. The new
> > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > afterwards.
> ...
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index fc771ff..55dc55c 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> ...
> > LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > - disk->pdev_path);
> > + in_disk->pdev_path);
>
> I think in_disk->pdev_path can be NULL here ?
It cannot, unless a wrong configuration was provided. In that case we'll
fail to open the empty disk later on.
> Also log messages should not contain "\n"; the logging functions add
> it.
OK
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-14 13:04 ` Stefano Stabellini
@ 2012-05-14 13:06 ` Ian Campbell
2012-05-14 14:15 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-14 13:06 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Mon, 2012-05-14 at 14:04 +0100, Stefano Stabellini wrote:
> On Fri, 4 May 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > > Introduce a new libxl_device_disk** parameter to
> > > libxl__device_disk_local_attach, the parameter is allocated on the gc by
> > > libxl__device_disk_local_attach. It is going to fill it with
> > > informations about the new locally attached disk. The new
> > > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > > afterwards.
> > ...
> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index fc771ff..55dc55c 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > ...
> > > LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > > - disk->pdev_path);
> > > + in_disk->pdev_path);
> >
> > I think in_disk->pdev_path can be NULL here ?
>
> It cannot, unless a wrong configuration was provided.
so it can?
> In that case we'll fail to open the empty disk later on.
But logging the NULL pointer doesn't seem right. If this case is invalid
we should detect it as such and error out, not carry on until some other
secondary error causes us to abort.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-05-14 13:04 ` Stefano Stabellini
2012-05-14 13:06 ` Ian Campbell
@ 2012-05-14 14:15 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-05-14 14:15 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> On Fri, 4 May 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > > LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > > - disk->pdev_path);
> > > + in_disk->pdev_path);
> >
> > I think in_disk->pdev_path can be NULL here ?
>
> It cannot, unless a wrong configuration was provided. In that case we'll
> fail to open the empty disk later on.
But not until we've called LIBXL__LOG(..., "%s", NULL) which is
undefined behaviour (according to the spec) and which on all systems
we are likely to run on will print "(null)".
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 3/8] libxl: add a transaction parameter to libxl__device_generic_add
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
2012-05-04 11:13 ` [PATCH v5 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
2012-05-04 11:13 ` [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 14:41 ` Ian Campbell
2012-05-04 11:13 ` [PATCH v5 4/8] libxl: introduce libxl__device_disk_add Stefano Stabellini
` (5 subsequent siblings)
8 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
XBT_NULL, allocate a proper one.
Update all the callers.
This patch contains a large number of unchecked xenstore operations, we
might want to fix this in the future.
Changes in v4:
- libxl__device_generic_add: do not end the transaction is the caller
provided it;
- libxl__device_generic_add: fix success exit path.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 10 +++++-----
tools/libxl/libxl_device.c | 17 +++++++++++------
tools/libxl/libxl_internal.h | 4 ++--
tools/libxl/libxl_pci.c | 2 +-
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1357efc..525f0d6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1401,7 +1401,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(front, "device-type");
flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
@@ -1802,7 +1802,7 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(front, "mac");
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
@@ -2080,7 +2080,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
}
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
@@ -2151,7 +2151,7 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb)
flexarray_append(front, "state");
flexarray_append(front, libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
@@ -2284,7 +2284,7 @@ int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb)
libxl__sprintf(gc, "%d", vfb->backend_domid));
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..88cdc7d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
return libxl__device_kind_from_string(strkind, &dev->backend_kind);
}
-int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents)
+int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device, char **bents, char **fents)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
char *frontend_path, *backend_path;
- xs_transaction_t t;
struct xs_permissions frontend_perms[2];
struct xs_permissions backend_perms[2];
+ int create_transaction = t == XBT_NULL;
frontend_path = libxl__device_frontend_path(gc, device);
backend_path = libxl__device_backend_path(gc, device);
@@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
backend_perms[1].perms = XS_PERM_READ;
retry_transaction:
- t = xs_transaction_start(ctx->xsh);
+ if (create_transaction)
+ t = xs_transaction_start(ctx->xsh);
/* FIXME: read frontend_path and check state before removing stuff */
if (fents) {
@@ -100,13 +101,17 @@ retry_transaction:
libxl__xs_writev(gc, t, backend_path, bents);
}
+ if (!create_transaction)
+ return 0;
+
if (!xs_transaction_end(ctx->xsh, t, 0)) {
if (errno == EAGAIN)
goto retry_transaction;
- else
+ else {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
+ return ERROR_FAIL;
+ }
}
-
return 0;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 965d13b..2c8f06a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -683,8 +683,8 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
libxl__device_console *console,
libxl__domain_build_state *state);
-_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents);
+_hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device, char **bents, char **fents);
_hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
_hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
_hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 3856bd9..335c645 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -102,7 +102,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
flexarray_append_pair(front, "backend-id", libxl__sprintf(gc, "%d", 0));
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 3/8] libxl: add a transaction parameter to libxl__device_generic_add
2012-05-04 11:13 ` [PATCH v5 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
@ 2012-05-04 14:41 ` Ian Campbell
0 siblings, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 14:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
> XBT_NULL, allocate a proper one.
>
> Update all the callers.
>
> This patch contains a large number of unchecked xenstore operations, we
> might want to fix this in the future.
>
> Changes in v4:
> - libxl__device_generic_add: do not end the transaction is the caller
> provided it;
> - libxl__device_generic_add: fix success exit path.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl.c | 10 +++++-----
> tools/libxl/libxl_device.c | 17 +++++++++++------
> tools/libxl/libxl_internal.h | 4 ++--
> tools/libxl/libxl_pci.c | 2 +-
> 4 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 1357efc..525f0d6 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1401,7 +1401,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
> flexarray_append(front, "device-type");
> flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
>
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
>
> @@ -1802,7 +1802,7 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
> flexarray_append(front, "mac");
> flexarray_append(front, libxl__sprintf(gc,
> LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
>
> @@ -2080,7 +2080,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
> }
>
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
> rc = 0;
> @@ -2151,7 +2151,7 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb)
> flexarray_append(front, "state");
> flexarray_append(front, libxl__sprintf(gc, "%d", 1));
>
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
> rc = 0;
> @@ -2284,7 +2284,7 @@ int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb)
> libxl__sprintf(gc, "%d", vfb->backend_domid));
> flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
>
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
> rc = 0;
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index c7e057d..88cdc7d 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
> return libxl__device_kind_from_string(strkind, &dev->backend_kind);
> }
>
> -int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> - char **bents, char **fents)
> +int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
> + libxl__device *device, char **bents, char **fents)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> char *frontend_path, *backend_path;
> - xs_transaction_t t;
> struct xs_permissions frontend_perms[2];
> struct xs_permissions backend_perms[2];
> + int create_transaction = t == XBT_NULL;
>
> frontend_path = libxl__device_frontend_path(gc, device);
> backend_path = libxl__device_backend_path(gc, device);
> @@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> backend_perms[1].perms = XS_PERM_READ;
>
> retry_transaction:
> - t = xs_transaction_start(ctx->xsh);
> + if (create_transaction)
> + t = xs_transaction_start(ctx->xsh);
> /* FIXME: read frontend_path and check state before removing stuff */
>
> if (fents) {
> @@ -100,13 +101,17 @@ retry_transaction:
> libxl__xs_writev(gc, t, backend_path, bents);
> }
>
> + if (!create_transaction)
> + return 0;
> +
> if (!xs_transaction_end(ctx->xsh, t, 0)) {
> if (errno == EAGAIN)
> goto retry_transaction;
> - else
> + else {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
> + return ERROR_FAIL;
> + }
> }
> -
> return 0;
> }
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 965d13b..2c8f06a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -683,8 +683,8 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
> libxl__device_console *console,
> libxl__domain_build_state *state);
>
> -_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> - char **bents, char **fents);
> +_hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
> + libxl__device *device, char **bents, char **fents);
> _hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
> _hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
> _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 3856bd9..335c645 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -102,7 +102,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
> flexarray_append_pair(front, "backend-id", libxl__sprintf(gc, "%d", 0));
> flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
>
> - libxl__device_generic_add(gc, &device,
> + libxl__device_generic_add(gc, XBT_NULL, &device,
> libxl__xs_kvs_of_flexarray(gc, back, back->count),
> libxl__xs_kvs_of_flexarray(gc, front, front->count));
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 4/8] libxl: introduce libxl__device_disk_add
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (2 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
2012-05-04 14:49 ` [PATCH v5 4/8] libxl: introduce libxl__device_disk_add Ian Campbell
2012-05-04 11:13 ` [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
` (4 subsequent siblings)
8 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce libxl__device_disk_add that takes an additional
xs_transaction_t paramter.
Implement libxl_device_disk_add using libxl__device_disk_add.
Move libxl__device_from_disk to libxl_internal.c.
No functional change.
Changes in v5:
- rename libxl__device_generic_add_t to libxl__device_generic_add.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.c | 151 +----------------------------------------
tools/libxl/libxl_internal.c | 157 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 6 ++
3 files changed, 164 insertions(+), 150 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 525f0d6..941dbb9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1258,159 +1258,10 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
return rc;
}
-static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
- libxl_device_disk *disk,
- libxl__device *device)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- int devid;
-
- devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
- if (devid==-1) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->vdev);
- return ERROR_INVAL;
- }
-
- device->backend_domid = disk->backend_domid;
- device->backend_devid = devid;
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_TAP:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_QDISK:
- device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
- disk->backend);
- return ERROR_INVAL;
- }
-
- device->domid = domid;
- device->devid = devid;
- device->kind = LIBXL__DEVICE_KIND_VBD;
-
- return 0;
-}
-
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
{
GC_INIT(ctx);
- flexarray_t *front;
- flexarray_t *back;
- char *dev;
- libxl__device device;
- int major, minor, rc;
-
- rc = libxl__device_disk_setdefault(gc, disk);
- if (rc) goto out;
-
- front = flexarray_make(16, 1);
- if (!front) {
- rc = ERROR_NOMEM;
- goto out;
- }
- back = flexarray_make(16, 1);
- if (!back) {
- rc = ERROR_NOMEM;
- goto out_free;
- }
-
- if (disk->script) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
- " not yet supported, sorry");
- rc = ERROR_INVAL;
- goto out_free;
- }
-
- rc = libxl__device_from_disk(gc, domid, disk, &device);
- if (rc != 0) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->vdev);
- goto out_free;
- }
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- dev = disk->pdev_path;
- do_backend_phy:
- libxl__device_physdisk_major_minor(dev, &major, &minor);
- flexarray_append(back, "physical-device");
- flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
-
- flexarray_append(back, "params");
- flexarray_append(back, dev);
-
- assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
- break;
- case LIBXL_DISK_BACKEND_TAP:
- dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
- if (!dev) {
- rc = ERROR_FAIL;
- goto out_free;
- }
- flexarray_append(back, "tapdisk-params");
- flexarray_append(back, libxl__sprintf(gc, "%s:%s",
- libxl__device_disk_string_of_format(disk->format),
- disk->pdev_path));
-
- /* now create a phy device to export the device to the guest */
- goto do_backend_phy;
- case LIBXL_DISK_BACKEND_QDISK:
- flexarray_append(back, "params");
- flexarray_append(back, libxl__sprintf(gc, "%s:%s",
- libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
- assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
- rc = ERROR_INVAL;
- goto out_free;
- }
-
- flexarray_append(back, "frontend-id");
- flexarray_append(back, libxl__sprintf(gc, "%d", domid));
- flexarray_append(back, "online");
- flexarray_append(back, "1");
- flexarray_append(back, "removable");
- flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
- flexarray_append(back, "bootable");
- flexarray_append(back, libxl__sprintf(gc, "%d", 1));
- flexarray_append(back, "state");
- flexarray_append(back, libxl__sprintf(gc, "%d", 1));
- flexarray_append(back, "dev");
- flexarray_append(back, disk->vdev);
- flexarray_append(back, "type");
- flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
- flexarray_append(back, "mode");
- flexarray_append(back, disk->readwrite ? "w" : "r");
- flexarray_append(back, "device-type");
- flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
-
- flexarray_append(front, "backend-id");
- flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
- flexarray_append(front, "state");
- flexarray_append(front, libxl__sprintf(gc, "%d", 1));
- flexarray_append(front, "virtual-device");
- flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
- flexarray_append(front, "device-type");
- flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
-
- libxl__device_generic_add(gc, XBT_NULL, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
-
- rc = 0;
-
-out_free:
- flexarray_free(back);
- flexarray_free(front);
-out:
+ int rc = libxl__device_disk_add(gc, domid, XBT_NULL, disk);
GC_FREE;
return rc;
}
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 55dc55c..1bf5d73 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,163 @@ out:
return rc;
}
+int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ int devid;
+
+ devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+ if (devid==-1) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ return ERROR_INVAL;
+ }
+
+ device->backend_domid = disk->backend_domid;
+ device->backend_devid = devid;
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
+ disk->backend);
+ return ERROR_INVAL;
+ }
+
+ device->domid = domid;
+ device->devid = devid;
+ device->kind = LIBXL__DEVICE_KIND_VBD;
+
+ return 0;
+}
+
+int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
+ xs_transaction_t t, libxl_device_disk *disk)
+{
+ flexarray_t *front;
+ flexarray_t *back;
+ char *dev;
+ libxl__device device;
+ int major, minor, rc;
+ libxl_ctx *ctx = gc->owner;
+
+ rc = libxl__device_disk_setdefault(gc, disk);
+ if (rc) goto out;
+
+ front = flexarray_make(16, 1);
+ if (!front) {
+ rc = ERROR_NOMEM;
+ goto out;
+ }
+ back = flexarray_make(16, 1);
+ if (!back) {
+ rc = ERROR_NOMEM;
+ goto out_free;
+ }
+
+ if (disk->script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
+ " not yet supported, sorry");
+ rc = ERROR_INVAL;
+ goto out_free;
+ }
+
+ rc = libxl__device_from_disk(gc, domid, disk, &device);
+ if (rc != 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ goto out_free;
+ }
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ dev = disk->pdev_path;
+ do_backend_phy:
+ libxl__device_physdisk_major_minor(dev, &major, &minor);
+ flexarray_append(back, "physical-device");
+ flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
+
+ flexarray_append(back, "params");
+ flexarray_append(back, dev);
+
+ assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
+ if (!dev) {
+ rc = ERROR_FAIL;
+ goto out_free;
+ }
+ flexarray_append(back, "tapdisk-params");
+ flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+ libxl__device_disk_string_of_format(disk->format),
+ disk->pdev_path));
+
+ /* now create a phy device to export the device to the guest */
+ goto do_backend_phy;
+ case LIBXL_DISK_BACKEND_QDISK:
+ flexarray_append(back, "params");
+ flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+ libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
+ assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
+ rc = ERROR_INVAL;
+ goto out_free;
+ }
+
+ flexarray_append(back, "frontend-id");
+ flexarray_append(back, libxl__sprintf(gc, "%d", domid));
+ flexarray_append(back, "online");
+ flexarray_append(back, "1");
+ flexarray_append(back, "removable");
+ flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
+ flexarray_append(back, "bootable");
+ flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(back, "state");
+ flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(back, "dev");
+ flexarray_append(back, disk->vdev);
+ flexarray_append(back, "type");
+ flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
+ flexarray_append(back, "mode");
+ flexarray_append(back, disk->readwrite ? "w" : "r");
+ flexarray_append(back, "device-type");
+ flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+
+ flexarray_append(front, "backend-id");
+ flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
+ flexarray_append(front, "state");
+ flexarray_append(front, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(front, "virtual-device");
+ flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
+ flexarray_append(front, "device-type");
+ flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
+
+ libxl__device_generic_add(gc, t, &device,
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count));
+
+ rc = 0;
+
+out_free:
+ flexarray_free(back);
+ flexarray_free(front);
+out:
+ return rc;
+}
+
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
libxl_device_disk **new_disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2c8f06a..096c96d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1003,6 +1003,12 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
*/
_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device);
+_hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
+ xs_transaction_t t, libxl_device_disk *disk);
/*
* Make a disk available in this (the control) domain. Returns path to
* a device.
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v4 0/8] qdisk local attach
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
` (7 more replies)
0 siblings, 8 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Ian Campbell, Stefano Stabellini
Hi all,
this patch implements local_attach support for QDISK: that means that
you can use qcow2 as disk format for your PV guests disks and use
pygrub to retrieve the kernel and initrd in dom0.
The idea is that we start a QEMU instance at boot time to listen to
local_attach requests. When libxl_device_disk_local_attach is called on
a QDISK images, libxl sets up a backend/frontend pair in dom0 for the disk
so that blkfront in dom0 will create a new xvd device for it.
Then pygrub can be pointed at this device to retrieve kernel and
initrd.
Changes in v4:
- split the first patch into 2 patches: the first is a simple move, the
second one adds a new parameter;
- libxl__device_generic_add: do not end the transaction is the caller
provided it;
- libxl__device_generic_add: fix success exit path;
- pass blkdev_start in libxl_domain_build_info;
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- improve error handling and exit paths in libxl__alloc_vdev and
libxl__device_disk_local_attach.
Changes in v3:
- libxl__device_disk_local_attach: wait for the backend to be
"connected" before returning.
Changes in v2:
- make libxl_device_disk_local_(de)attach internal functions;
- allocate the new disk in libxl_device_disk_local_attatch on the GC;
- remove libxl__device_generic_add_t, add a transaction parameter to
libxl__device_generic_add instead;
- add a new patch to introduce a blkdev_start option to xl.conf;
- reimplement libxl__alloc_vdev checking for the frontend path and
starting from blkdev_start;
- introduce a Linux specific libxl__devid_to_vdev function based on last
Jan's patch to blkfront.
Stefano Stabellini (8):
libxl: make libxl_device_disk_local_attach/detach internal functions
libxl: libxl__device_disk_local_attach return a new libxl_device_disk
libxl: add a transaction parameter to libxl__device_generic_add
libxl: introduce libxl__device_disk_add_t
xl/libxl: add a blkdev_start parameter
libxl: introduce libxl__alloc_vdev
xl/libxl: implement QDISK libxl_device_disk_local_attach
libxl__device_disk_local_attach: wait for state "connected"
tools/examples/xl.conf | 3 +
tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
tools/hotplug/Linux/init.d/xencommons | 3 +
tools/libxl/libxl.c | 232 +----------------
tools/libxl/libxl.h | 7 -
tools/libxl/libxl_bootloader.c | 11 +-
tools/libxl/libxl_create.c | 3 +
tools/libxl/libxl_device.c | 17 +-
tools/libxl/libxl_internal.c | 316 +++++++++++++++++++++++
tools/libxl/libxl_internal.h | 25 ++-
tools/libxl/libxl_linux.c | 45 ++++
tools/libxl/libxl_netbsd.c | 6 +
tools/libxl/libxl_pci.c | 2 +-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 3 +
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 2 +
17 files changed, 432 insertions(+), 248 deletions(-)
Cheers,
Stefano
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:16 ` Ian Campbell
2012-04-24 10:45 ` [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
` (6 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.c | 73 ----------------------------------------
tools/libxl/libxl.h | 7 ----
tools/libxl/libxl_bootloader.c | 4 +-
tools/libxl/libxl_internal.c | 72 +++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 9 +++++
5 files changed, 83 insertions(+), 82 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 60dbfdc..e645d2a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1664,79 +1664,6 @@ out:
return ret;
}
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
- GC_INIT(ctx);
- char *dev = NULL;
- char *ret = NULL;
- int rc;
-
- rc = libxl__device_disk_setdefault(gc, disk);
- if (rc) goto out;
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- case LIBXL_DISK_BACKEND_TAP:
- switch (disk->format) {
- case LIBXL_DISK_FORMAT_RAW:
- /* optimise away the early tapdisk attach in this case */
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
- " tap disk %s directly (ie without using blktap)",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- case LIBXL_DISK_FORMAT_VHD:
- dev = libxl__blktap_devpath(gc, disk->pdev_path,
- disk->format);
- break;
- case LIBXL_DISK_FORMAT_QCOW:
- case LIBXL_DISK_FORMAT_QCOW2:
- abort(); /* prevented by libxl__device_disk_set_backend */
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "unrecognized disk format: %d", disk->format);
- break;
- }
- break;
- case LIBXL_DISK_BACKEND_QDISK:
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
- " attach a qdisk image if the format is not raw");
- break;
- }
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
- "type: %d", disk->backend);
- break;
- }
-
- out:
- if (dev != NULL)
- ret = strdup(dev);
- GC_FREE;
- return ret;
-}
-
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
-{
- /* Nothing to do for PHYSTYPE_PHY. */
-
- /*
- * For other device types assume that the blktap2 process is
- * needed by the soon to be started domain and do nothing.
- */
-
- return 0;
-}
-
/******************************************************************************/
int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index edbca53..779c8dd 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -618,13 +618,6 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
*/
int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk);
-/*
- * Make a disk available in this (the control) domain. Returns path to
- * a device.
- */
-char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk);
-int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk);
-
/* Network Interfaces */
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t domid,
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 2774062..977c6d3 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -386,7 +386,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl_device_disk_local_attach(ctx, disk);
+ diskpath = libxl__device_disk_local_attach(gc, disk);
if (!diskpath) {
goto out_close;
}
@@ -453,7 +453,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
rc = 0;
out_close:
if (diskpath) {
- libxl_device_disk_local_detach(ctx, disk);
+ libxl__device_disk_local_detach(gc, disk);
free(diskpath);
}
if (fifo_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b89aef7..1f428b2 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,78 @@ out:
return rc;
}
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+{
+ libxl_ctx *ctx = gc->owner;
+ char *dev = NULL;
+ char *ret = NULL;
+ int rc;
+
+ rc = libxl__device_disk_setdefault(gc, disk);
+ if (rc) goto out;
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ switch (disk->format) {
+ case LIBXL_DISK_FORMAT_RAW:
+ /* optimise away the early tapdisk attach in this case */
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
+ " tap disk %s directly (ie without using blktap)",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ case LIBXL_DISK_FORMAT_VHD:
+ dev = libxl__blktap_devpath(gc, disk->pdev_path,
+ disk->format);
+ break;
+ case LIBXL_DISK_FORMAT_QCOW:
+ case LIBXL_DISK_FORMAT_QCOW2:
+ abort(); /* prevented by libxl__device_disk_set_backend */
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "unrecognized disk format: %d", disk->format);
+ break;
+ }
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
+ " attach a qdisk image if the format is not raw");
+ break;
+ }
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
+ disk->pdev_path);
+ dev = disk->pdev_path;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
+ "type: %d", disk->backend);
+ break;
+ }
+
+ out:
+ if (dev != NULL)
+ ret = strdup(dev);
+ return ret;
+}
+
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
+{
+ /* Nothing to do for PHYSTYPE_PHY. */
+
+ /*
+ * For other device types assume that the blktap2 process is
+ * needed by the soon to be started domain and do nothing.
+ */
+
+ return 0;
+}
+
libxl_device_model_version libxl__device_model_version_running(libxl__gc *gc,
uint32_t domid)
{
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a4b933b..6927aef 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1001,6 +1001,15 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
*/
_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+/*
+ * Make a disk available in this (the control) domain. Returns path to
+ * a device.
+ */
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+ libxl_device_disk *disk);
+_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
+ libxl_device_disk *disk);
+
_hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
struct libxl__xen_console_reader {
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions
2012-04-24 10:45 ` [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
@ 2012-04-24 15:16 ` Ian Campbell
0 siblings, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-04-24 15:16 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Tue, 2012-04-24 at 11:45 +0100, Stefano Stabellini wrote:
No functional change other than ctx->gc?
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index b89aef7..1f428b2 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,6 +323,78 @@ out:
> return rc;
> }
>
> +_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
Strictly the hidden is only needed on the prototype. I guess there's no
harm having it here too (but you know what linkers are like...)
> +{
[...]
> +}
Looking at the callers this could possibly be a gc'd string, and perhaps
const in the return here. That's for another day though I think.
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:02 ` Ian Jackson
2012-04-24 15:25 ` Ian Campbell
2012-04-24 10:45 ` [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
` (5 subsequent siblings)
7 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce a new libxl_device_disk** parameter to
libxl__device_disk_local_attach, the parameter is allocated on the gc by
libxl__device_disk_local_attach. It is going to fill it with
informations about the new locally attached disk. The new
libxl_device_disk should be passed to libxl_device_disk_local_detach
afterwards.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_bootloader.c | 10 ++++----
tools/libxl/libxl_internal.c | 47 +++++++++++++++++++++++----------------
tools/libxl/libxl_internal.h | 3 +-
3 files changed, 35 insertions(+), 25 deletions(-)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 977c6d3..83cac78 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
char *fifo = NULL;
char *diskpath = NULL;
char **args = NULL;
+ libxl_device_disk *tmpdisk = NULL;
char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
char *tempdir;
@@ -386,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl__device_disk_local_attach(gc, disk);
+ diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
if (!diskpath) {
+ rc = ERROR_FAIL;
goto out_close;
}
@@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
rc = 0;
out_close:
- if (diskpath) {
- libxl__device_disk_local_detach(gc, disk);
- free(diskpath);
- }
+ if (tmpdisk)
+ libxl__device_disk_local_detach(gc, tmpdisk);
if (fifo_fd > -1)
close(fifo_fd);
if (bootloader_fd > -1)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 1f428b2..3af77e7 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,64 +323,73 @@ out:
return rc;
}
-_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
+_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
+ const libxl_device_disk *disk,
+ libxl_device_disk **new_disk)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
- char *ret = NULL;
int rc;
-
- rc = libxl__device_disk_setdefault(gc, disk);
+ libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
+ if (tmpdisk == NULL) goto out;
+
+ *new_disk = tmpdisk;
+ memcpy(tmpdisk, disk, sizeof(libxl_device_disk));
+ if (disk->pdev_path != NULL)
+ tmpdisk->pdev_path = libxl__strdup(gc, disk->pdev_path);
+ if (disk->script != NULL)
+ tmpdisk->script = libxl__strdup(gc, disk->script);
+ tmpdisk->vdev = NULL;
+
+ rc = libxl__device_disk_setdefault(gc, tmpdisk);
if (rc) goto out;
- switch (disk->backend) {
+ switch (tmpdisk->backend) {
case LIBXL_DISK_BACKEND_PHY:
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
- disk->pdev_path);
- dev = disk->pdev_path;
+ tmpdisk->pdev_path);
+ dev = tmpdisk->pdev_path;
break;
case LIBXL_DISK_BACKEND_TAP:
- switch (disk->format) {
+ switch (tmpdisk->format) {
case LIBXL_DISK_FORMAT_RAW:
/* optimise away the early tapdisk attach in this case */
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
" tap disk %s directly (ie without using blktap)",
- disk->pdev_path);
- dev = disk->pdev_path;
+ tmpdisk->pdev_path);
+ dev = tmpdisk->pdev_path;
break;
case LIBXL_DISK_FORMAT_VHD:
- dev = libxl__blktap_devpath(gc, disk->pdev_path,
- disk->format);
+ dev = libxl__blktap_devpath(gc, tmpdisk->pdev_path,
+ tmpdisk->format);
break;
case LIBXL_DISK_FORMAT_QCOW:
case LIBXL_DISK_FORMAT_QCOW2:
abort(); /* prevented by libxl__device_disk_set_backend */
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
- "unrecognized disk format: %d", disk->format);
+ "unrecognized disk format: %d", tmpdisk->format);
break;
}
break;
case LIBXL_DISK_BACKEND_QDISK:
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
+ if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
" attach a qdisk image if the format is not raw");
break;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
disk->pdev_path);
- dev = disk->pdev_path;
+ dev = tmpdisk->pdev_path;
break;
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
- "type: %d", disk->backend);
+ "type: %d", tmpdisk->backend);
break;
}
out:
- if (dev != NULL)
- ret = strdup(dev);
- return ret;
+ return dev;
}
_hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6927aef..2f1cf0f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1006,7 +1006,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
* a device.
*/
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
- libxl_device_disk *disk);
+ const libxl_device_disk *disk,
+ libxl_device_disk **new_disk);
_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
libxl_device_disk *disk);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-04-24 10:45 ` [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
@ 2012-04-24 15:02 ` Ian Jackson
2012-04-24 15:25 ` Ian Campbell
1 sibling, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:02 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
...
> +_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> + const libxl_device_disk *disk,
> + libxl_device_disk **new_disk)
> {
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> - char *ret = NULL;
> int rc;
> -
> - rc = libxl__device_disk_setdefault(gc, disk);
> + libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
> + if (tmpdisk == NULL) goto out;
libxl__zalloc is guaranteed not to fail, nowadays.
> - switch (disk->backend) {
> + switch (tmpdisk->backend) {
Did you see my comment about arranging for "disk" to be the new
structure and renaming the formal parameter ? That would (a) remove a
bunch of useless stuff from the diff (b) probably make the code
clearer anyway, since nothing inside this function should be using the
actually-passed libxl_device_disk* other than the code whose purpose
is to make a copy of it.
If you resend with that change I will find it much easier to review
this.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk
2012-04-24 10:45 ` [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
2012-04-24 15:02 ` Ian Jackson
@ 2012-04-24 15:25 ` Ian Campbell
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-04-24 15:25 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Tue, 2012-04-24 at 11:45 +0100, Stefano Stabellini wrote:
> Introduce a new libxl_device_disk** parameter to
> libxl__device_disk_local_attach, the parameter is allocated on the gc by
> libxl__device_disk_local_attach. It is going to fill it with
> informations about the new locally attached disk. The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/libxl/libxl_bootloader.c | 10 ++++----
> tools/libxl/libxl_internal.c | 47 +++++++++++++++++++++++----------------
> tools/libxl/libxl_internal.h | 3 +-
> 3 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 977c6d3..83cac78 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -330,6 +330,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> char *fifo = NULL;
> char *diskpath = NULL;
> char **args = NULL;
> + libxl_device_disk *tmpdisk = NULL;
>
> char tempdir_template[] = "/var/run/libxl/bl.XXXXXX";
> char *tempdir;
> @@ -386,8 +387,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> goto out_close;
> }
>
> - diskpath = libxl__device_disk_local_attach(gc, disk);
> + diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
> if (!diskpath) {
> + rc = ERROR_FAIL;
> goto out_close;
> }
>
> @@ -452,10 +454,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>
> rc = 0;
> out_close:
> - if (diskpath) {
> - libxl__device_disk_local_detach(gc, disk);
> - free(diskpath);
> - }
> + if (tmpdisk)
> + libxl__device_disk_local_detach(gc, tmpdisk);
> if (fifo_fd > -1)
> close(fifo_fd);
> if (bootloader_fd > -1)
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 1f428b2..3af77e7 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,64 +323,73 @@ out:
> return rc;
> }
>
> -_hidden char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk *disk)
> +_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> + const libxl_device_disk *disk,
> + libxl_device_disk **new_disk)
> {
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> - char *ret = NULL;
> int rc;
> -
> - rc = libxl__device_disk_setdefault(gc, disk);
> + libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
> + if (tmpdisk == NULL) goto out;
> +
> + *new_disk = tmpdisk;
It would be good practice to not do this until we have succeeded, or do
you expect the caller to clean this up on error, even if the disk is
half constructed? I'd suggest that it ought be up to this function to
unwind on failure not the called.
tmpdisk isn't actually temporary, it's actually the result of this
function and lives for a fair while afterwards, perhaps loopdisk? or
dom0disk? or was this the patch where IanJ suggested renaming the param
disk (e.g. to guest_disk) and then using disk for this one? (That's
actually a good idea)
> + memcpy(tmpdisk, disk, sizeof(libxl_device_disk));
You don't actually need zalloc if you immediately memcpy over the whole
thing. Minor thing though.
> + if (disk->pdev_path != NULL)
> + tmpdisk->pdev_path = libxl__strdup(gc, disk->pdev_path);
> + if (disk->script != NULL)
> + tmpdisk->script = libxl__strdup(gc, disk->script);
> + tmpdisk->vdev = NULL;
> +
> + rc = libxl__device_disk_setdefault(gc, tmpdisk);
> if (rc) goto out;
>
> - switch (disk->backend) {
> + switch (tmpdisk->backend) {
> case LIBXL_DISK_BACKEND_PHY:
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk %s",
> - disk->pdev_path);
> - dev = disk->pdev_path;
> + tmpdisk->pdev_path);
> + dev = tmpdisk->pdev_path;
> break;
> case LIBXL_DISK_BACKEND_TAP:
> - switch (disk->format) {
> + switch (tmpdisk->format) {
> case LIBXL_DISK_FORMAT_RAW:
> /* optimise away the early tapdisk attach in this case */
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching"
> " tap disk %s directly (ie without using blktap)",
> - disk->pdev_path);
> - dev = disk->pdev_path;
> + tmpdisk->pdev_path);
> + dev = tmpdisk->pdev_path;
> break;
> case LIBXL_DISK_FORMAT_VHD:
> - dev = libxl__blktap_devpath(gc, disk->pdev_path,
> - disk->format);
> + dev = libxl__blktap_devpath(gc, tmpdisk->pdev_path,
> + tmpdisk->format);
> break;
> case LIBXL_DISK_FORMAT_QCOW:
> case LIBXL_DISK_FORMAT_QCOW2:
> abort(); /* prevented by libxl__device_disk_set_backend */
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> - "unrecognized disk format: %d", disk->format);
> + "unrecognized disk format: %d", tmpdisk->format);
> break;
> }
> break;
> case LIBXL_DISK_BACKEND_QDISK:
> - if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> + if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> " attach a qdisk image if the format is not raw");
> break;
> }
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> disk->pdev_path);
> - dev = disk->pdev_path;
> + dev = tmpdisk->pdev_path;
> break;
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
> - "type: %d", disk->backend);
> + "type: %d", tmpdisk->backend);
> break;
> }
>
> out:
> - if (dev != NULL)
> - ret = strdup(dev);
> - return ret;
> + return dev;
> }
>
> _hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6927aef..2f1cf0f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1006,7 +1006,8 @@ _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
> * a device.
> */
> _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> - libxl_device_disk *disk);
> + const libxl_device_disk *disk,
> + libxl_device_disk **new_disk);
> _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
> libxl_device_disk *disk);
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 1/8] libxl: make libxl_device_disk_local_attach/detach internal functions Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:34 ` Ian Jackson
2012-04-24 10:45 ` [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t Stefano Stabellini
` (4 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
XBT_NULL, allocate a proper one.
Update all the callers.
Changes in v4:
- libxl__device_generic_add: do not end the transaction is the caller
provided it;
- libxl__device_generic_add: fix success exit path.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 10 +++++-----
tools/libxl/libxl_device.c | 17 +++++++++++------
tools/libxl/libxl_internal.h | 4 ++--
tools/libxl/libxl_pci.c | 2 +-
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e645d2a..85082eb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1401,7 +1401,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *dis
flexarray_append(front, "device-type");
flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
@@ -1795,7 +1795,7 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic)
flexarray_append(front, "mac");
flexarray_append(front, libxl__sprintf(gc,
LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac)));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
@@ -2073,7 +2073,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
}
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
@@ -2144,7 +2144,7 @@ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb)
flexarray_append(front, "state");
flexarray_append(front, libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
@@ -2277,7 +2277,7 @@ int libxl_device_vfb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vfb *vfb)
libxl__sprintf(gc, "%d", vfb->backend_domid));
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
rc = 0;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..88cdc7d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,14 +58,14 @@ int libxl__parse_backend_path(libxl__gc *gc,
return libxl__device_kind_from_string(strkind, &dev->backend_kind);
}
-int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents)
+int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device, char **bents, char **fents)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
char *frontend_path, *backend_path;
- xs_transaction_t t;
struct xs_permissions frontend_perms[2];
struct xs_permissions backend_perms[2];
+ int create_transaction = t == XBT_NULL;
frontend_path = libxl__device_frontend_path(gc, device);
backend_path = libxl__device_backend_path(gc, device);
@@ -81,7 +81,8 @@ int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
backend_perms[1].perms = XS_PERM_READ;
retry_transaction:
- t = xs_transaction_start(ctx->xsh);
+ if (create_transaction)
+ t = xs_transaction_start(ctx->xsh);
/* FIXME: read frontend_path and check state before removing stuff */
if (fents) {
@@ -100,13 +101,17 @@ retry_transaction:
libxl__xs_writev(gc, t, backend_path, bents);
}
+ if (!create_transaction)
+ return 0;
+
if (!xs_transaction_end(ctx->xsh, t, 0)) {
if (errno == EAGAIN)
goto retry_transaction;
- else
+ else {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
+ return ERROR_FAIL;
+ }
}
-
return 0;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2f1cf0f..00856e0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -681,8 +681,8 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
libxl__device_console *console,
libxl__domain_build_state *state);
-_hidden int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
- char **bents, char **fents);
+_hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
+ libxl__device *device, char **bents, char **fents);
_hidden char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device);
_hidden char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
_hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e8b8839..202a101 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -102,7 +102,7 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
flexarray_append_pair(front, "backend-id", libxl__sprintf(gc, "%d", 0));
flexarray_append_pair(front, "state", libxl__sprintf(gc, "%d", 1));
- libxl__device_generic_add(gc, &device,
+ libxl__device_generic_add(gc, XBT_NULL, &device,
libxl__xs_kvs_of_flexarray(gc, back, back->count),
libxl__xs_kvs_of_flexarray(gc, front, front->count));
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add
2012-04-24 10:45 ` [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
@ 2012-04-24 15:34 ` Ian Jackson
0 siblings, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:34 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add"):
> Add a xs_transaction_t parameter to libxl__device_generic_add, if it is
> XBT_NULL, allocate a proper one.
...
> -int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> - char **bents, char **fents)
> +int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
> + libxl__device *device, char **bents, char **fents)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> char *frontend_path, *backend_path;
> - xs_transaction_t t;
> struct xs_permissions frontend_perms[2];
> struct xs_permissions backend_perms[2];
> + int create_transaction = t == XBT_NULL;
This works.
Another way to do it is to have a separate variable,
something like this:
xs_transaction_t our_transaction = t;
and then later
if (!t) {
our_transaction = t = xs_transaction_start....
if (!t) error handling;
}
and when you commit it set our_transaction to 0 and on error exit
if (our_transaction)
xs_transaction_end(..., our_transaction, 1);
I suggest this just because you may prefer to avoid separate boolean
sentinel variables - I know I do.
There is a difficulty in general with this function, which is that it
contains an enormous number of unchecked xenstore operations. I'm not
saying you need to fix this now, but if at a later point this were to
be fixed, the function would need to get a proper error exit path
which would have to destroy the transaction iff it was created.
I mention this for completeness but you may want to transpose it into
a comment somehow.
Anyway,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
` (2 preceding siblings ...)
2012-04-24 10:45 ` [PATCH v4 3/8] libxl: add a transaction parameter to libxl__device_generic_add Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:42 ` Ian Jackson
2012-04-24 10:45 ` [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
` (3 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce libxl__device_disk_add_t that takes an additional
xs_transaction_t paramter.
Implement libxl_device_disk_add using libxl__device_disk_add_t.
Move libxl__device_from_disk to libxl_internal.c.
No functional change.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl.c | 151 +----------------------------------------
tools/libxl/libxl_internal.c | 157 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 6 ++
3 files changed, 164 insertions(+), 150 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 85082eb..0d7c0f6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1258,159 +1258,10 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
return rc;
}
-static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
- libxl_device_disk *disk,
- libxl__device *device)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- int devid;
-
- devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
- if (devid==-1) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->vdev);
- return ERROR_INVAL;
- }
-
- device->backend_domid = disk->backend_domid;
- device->backend_devid = devid;
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_TAP:
- device->backend_kind = LIBXL__DEVICE_KIND_VBD;
- break;
- case LIBXL_DISK_BACKEND_QDISK:
- device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
- disk->backend);
- return ERROR_INVAL;
- }
-
- device->domid = domid;
- device->devid = devid;
- device->kind = LIBXL__DEVICE_KIND_VBD;
-
- return 0;
-}
-
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
{
GC_INIT(ctx);
- flexarray_t *front;
- flexarray_t *back;
- char *dev;
- libxl__device device;
- int major, minor, rc;
-
- rc = libxl__device_disk_setdefault(gc, disk);
- if (rc) goto out;
-
- front = flexarray_make(16, 1);
- if (!front) {
- rc = ERROR_NOMEM;
- goto out;
- }
- back = flexarray_make(16, 1);
- if (!back) {
- rc = ERROR_NOMEM;
- goto out_free;
- }
-
- if (disk->script) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
- " not yet supported, sorry");
- rc = ERROR_INVAL;
- goto out_free;
- }
-
- rc = libxl__device_from_disk(gc, domid, disk, &device);
- if (rc != 0) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->vdev);
- goto out_free;
- }
-
- switch (disk->backend) {
- case LIBXL_DISK_BACKEND_PHY:
- dev = disk->pdev_path;
- do_backend_phy:
- libxl__device_physdisk_major_minor(dev, &major, &minor);
- flexarray_append(back, "physical-device");
- flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
-
- flexarray_append(back, "params");
- flexarray_append(back, dev);
-
- assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
- break;
- case LIBXL_DISK_BACKEND_TAP:
- dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
- if (!dev) {
- rc = ERROR_FAIL;
- goto out_free;
- }
- flexarray_append(back, "tapdisk-params");
- flexarray_append(back, libxl__sprintf(gc, "%s:%s",
- libxl__device_disk_string_of_format(disk->format),
- disk->pdev_path));
-
- /* now create a phy device to export the device to the guest */
- goto do_backend_phy;
- case LIBXL_DISK_BACKEND_QDISK:
- flexarray_append(back, "params");
- flexarray_append(back, libxl__sprintf(gc, "%s:%s",
- libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
- assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
- break;
- default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
- rc = ERROR_INVAL;
- goto out_free;
- }
-
- flexarray_append(back, "frontend-id");
- flexarray_append(back, libxl__sprintf(gc, "%d", domid));
- flexarray_append(back, "online");
- flexarray_append(back, "1");
- flexarray_append(back, "removable");
- flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
- flexarray_append(back, "bootable");
- flexarray_append(back, libxl__sprintf(gc, "%d", 1));
- flexarray_append(back, "state");
- flexarray_append(back, libxl__sprintf(gc, "%d", 1));
- flexarray_append(back, "dev");
- flexarray_append(back, disk->vdev);
- flexarray_append(back, "type");
- flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
- flexarray_append(back, "mode");
- flexarray_append(back, disk->readwrite ? "w" : "r");
- flexarray_append(back, "device-type");
- flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
-
- flexarray_append(front, "backend-id");
- flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
- flexarray_append(front, "state");
- flexarray_append(front, libxl__sprintf(gc, "%d", 1));
- flexarray_append(front, "virtual-device");
- flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
- flexarray_append(front, "device-type");
- flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
-
- libxl__device_generic_add(gc, XBT_NULL, &device,
- libxl__xs_kvs_of_flexarray(gc, back, back->count),
- libxl__xs_kvs_of_flexarray(gc, front, front->count));
-
- rc = 0;
-
-out_free:
- flexarray_free(back);
- flexarray_free(front);
-out:
+ int rc = libxl__device_disk_add_t(gc, domid, XBT_NULL, disk);
GC_FREE;
return rc;
}
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 3af77e7..f348529 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -323,6 +323,163 @@ out:
return rc;
}
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ int devid;
+
+ devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+ if (devid==-1) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ return ERROR_INVAL;
+ }
+
+ device->backend_domid = disk->backend_domid;
+ device->backend_devid = devid;
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+ break;
+ case LIBXL_DISK_BACKEND_QDISK:
+ device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
+ disk->backend);
+ return ERROR_INVAL;
+ }
+
+ device->domid = domid;
+ device->devid = devid;
+ device->kind = LIBXL__DEVICE_KIND_VBD;
+
+ return 0;
+}
+
+_hidden int libxl__device_disk_add_t(libxl__gc *gc, uint32_t domid,
+ xs_transaction_t t, libxl_device_disk *disk)
+{
+ flexarray_t *front;
+ flexarray_t *back;
+ char *dev;
+ libxl__device device;
+ int major, minor, rc;
+ libxl_ctx *ctx = gc->owner;
+
+ rc = libxl__device_disk_setdefault(gc, disk);
+ if (rc) goto out;
+
+ front = flexarray_make(16, 1);
+ if (!front) {
+ rc = ERROR_NOMEM;
+ goto out;
+ }
+ back = flexarray_make(16, 1);
+ if (!back) {
+ rc = ERROR_NOMEM;
+ goto out_free;
+ }
+
+ if (disk->script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
+ " not yet supported, sorry");
+ rc = ERROR_INVAL;
+ goto out_free;
+ }
+
+ rc = libxl__device_from_disk(gc, domid, disk, &device);
+ if (rc != 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
+ " virtual disk identifier %s", disk->vdev);
+ goto out_free;
+ }
+
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_PHY:
+ dev = disk->pdev_path;
+ do_backend_phy:
+ libxl__device_physdisk_major_minor(dev, &major, &minor);
+ flexarray_append(back, "physical-device");
+ flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
+
+ flexarray_append(back, "params");
+ flexarray_append(back, dev);
+
+ assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
+ break;
+ case LIBXL_DISK_BACKEND_TAP:
+ dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
+ if (!dev) {
+ rc = ERROR_FAIL;
+ goto out_free;
+ }
+ flexarray_append(back, "tapdisk-params");
+ flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+ libxl__device_disk_string_of_format(disk->format),
+ disk->pdev_path));
+
+ /* now create a phy device to export the device to the guest */
+ goto do_backend_phy;
+ case LIBXL_DISK_BACKEND_QDISK:
+ flexarray_append(back, "params");
+ flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+ libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
+ assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
+ rc = ERROR_INVAL;
+ goto out_free;
+ }
+
+ flexarray_append(back, "frontend-id");
+ flexarray_append(back, libxl__sprintf(gc, "%d", domid));
+ flexarray_append(back, "online");
+ flexarray_append(back, "1");
+ flexarray_append(back, "removable");
+ flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
+ flexarray_append(back, "bootable");
+ flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(back, "state");
+ flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(back, "dev");
+ flexarray_append(back, disk->vdev);
+ flexarray_append(back, "type");
+ flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
+ flexarray_append(back, "mode");
+ flexarray_append(back, disk->readwrite ? "w" : "r");
+ flexarray_append(back, "device-type");
+ flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+
+ flexarray_append(front, "backend-id");
+ flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
+ flexarray_append(front, "state");
+ flexarray_append(front, libxl__sprintf(gc, "%d", 1));
+ flexarray_append(front, "virtual-device");
+ flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
+ flexarray_append(front, "device-type");
+ flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
+
+ libxl__device_generic_add(gc, t, &device,
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count));
+
+ rc = 0;
+
+out_free:
+ flexarray_free(back);
+ flexarray_free(front);
+out:
+ return rc;
+}
+
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *disk,
libxl_device_disk **new_disk)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 00856e0..2708cc5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1001,6 +1001,12 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
*/
_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+
+_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
+ libxl_device_disk *disk,
+ libxl__device *device);
+_hidden int libxl__device_disk_add_t(libxl__gc *gc, uint32_t domid,
+ xs_transaction_t t, libxl_device_disk *disk);
/*
* Make a disk available in this (the control) domain. Returns path to
* a device.
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t
2012-04-24 10:45 ` [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t Stefano Stabellini
@ 2012-04-24 15:42 ` Ian Jackson
2012-04-24 16:02 ` Ian Jackson
2012-05-04 16:30 ` [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t [and 1 more messages] Ian Jackson
0 siblings, 2 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:42 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t"):
> Introduce libxl__device_disk_add_t that takes an additional
> xs_transaction_t paramter.
Names ending "_t" are reserved for the implementation. But you can
just call the internal function libxl__device_disk_add. I think it's
fine to internal and external functions which differ only in _ and
parameters, provided that there is no nontrivial functionality in the
external function.
Apart from that it's rather hard to review because this patch seems to
have an awful lot of code motion mixed up with other changes.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t
2012-04-24 15:42 ` Ian Jackson
@ 2012-04-24 16:02 ` Ian Jackson
2012-05-04 16:30 ` [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t [and 1 more messages] Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 16:02 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel, Ian.Campbell
Ian Jackson writes ("Re: [Xen-devel] [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t"):
> Stefano Stabellini writes ("[Xen-devel] [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t"):
> > Introduce libxl__device_disk_add_t that takes an additional
> > xs_transaction_t paramter.
>
> Names ending "_t" are reserved for the implementation.
Sorry, I was unclear. I meant the C language implementation. So
libxl may not introduce anything called "..._t".
> But you can just call the internal function libxl__device_disk_add.
> I think it's fine to internal and external functions which differ
> only in _ and parameters, provided that there is no nontrivial
> functionality in the external function.
>
> Apart from that it's rather hard to review because this patch seems to
> have an awful lot of code motion mixed up with other changes.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t [and 1 more messages]
2012-04-24 15:42 ` Ian Jackson
2012-04-24 16:02 ` Ian Jackson
@ 2012-05-04 16:30 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:30 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[PATCH v5 4/8] libxl: introduce libxl__device_disk_add"):
> Introduce libxl__device_disk_add that takes an additional
> xs_transaction_t paramter.
> Implement libxl_device_disk_add using libxl__device_disk_add.
> Move libxl__device_from_disk to libxl_internal.c.
> No functional change.
The last time you posted this I wrote:
Ian Jackson writes ("Re: [Xen-devel] [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t"):
> Apart from that it's rather hard to review because this patch seems to
> have an awful lot of code motion mixed up with other changes.
Can you split the code motion out into a separate patch, so that we
can see what has changed in the code that is being moved ?
Also I think "no functional change" is stretching it a bit unless what
you mean is "no callers of libxl__device_disk_add yet".
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
` (3 preceding siblings ...)
2012-04-24 10:45 ` [PATCH v4 4/8] libxl: introduce libxl__device_disk_add_t Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 14:52 ` Ian Jackson
2012-04-24 10:45 ` [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
` (2 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce a blkdev_start in xl.conf and a corresponding string in
libxl_domain_build_info.
blkdev_start specifies the first block device to be used for temporary
block device allocations by the toolstack.
Changes in v4:
- pass blkdev_start in libxl_domain_build_info.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/examples/xl.conf | 3 +++
tools/libxl/libxl_bootloader.c | 3 ++-
tools/libxl/libxl_create.c | 3 +++
tools/libxl/libxl_internal.c | 3 ++-
tools/libxl/libxl_internal.h | 3 ++-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 3 +++
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 2 ++
9 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 56d3b3b..ebf057c 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,6 @@
# default output format used by "xl list -l"
#output_format="json"
+
+# first block device to be used for temporary VM disk mounts
+#blkdev_start="xvda"
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 83cac78..123b06e 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -387,7 +387,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
+ diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk,
+ info->blkdev_start);
if (!diskpath) {
rc = ERROR_FAIL;
goto out_close;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f9c2a76..5b97047 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -100,6 +100,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
}
+ if (b_info->blkdev_start == NULL)
+ b_info->blkdev_start = strdup("xvda");
+
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!b_info->u.hvm.bios)
switch (b_info->device_model_version) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f348529..f467572 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -482,7 +482,8 @@ out:
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *disk,
- libxl_device_disk **new_disk)
+ libxl_device_disk **new_disk,
+ char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2708cc5..97775fb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1013,7 +1013,8 @@ _hidden int libxl__device_disk_add_t(libxl__gc *gc, uint32_t domid,
*/
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *disk,
- libxl_device_disk **new_disk);
+ libxl_device_disk **new_disk,
+ char *blkdev_start);
_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
libxl_device_disk *disk);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5cf9708..24d2322 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -242,6 +242,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("localtime", libxl_defbool),
("disable_migrate", libxl_defbool),
("cpuid", libxl_cpuid_policy_list),
+ ("blkdev_start", string),
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..4596c4f 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -35,6 +35,7 @@
xentoollog_logger_stdiostream *logger;
int dryrun_only;
int autoballoon = 1;
+char *blkdev_start;
char *lockfile;
char *default_vifscript = NULL;
char *default_bridge = NULL;
@@ -91,6 +92,8 @@ static void parse_global_config(const char *configfile,
fprintf(stderr, "invalid default output format \"%s\"\n", buf);
}
}
+ if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
+ blkdev_start = strdup(buf);
xlu_cfg_destroy(config);
}
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 7e258d5..dba2c94 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -113,6 +113,7 @@ extern int dryrun_only;
extern char *lockfile;
extern char *default_vifscript;
extern char *default_bridge;
+extern char *blkdev_start;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c9e9943..12a0934 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -585,6 +585,8 @@ static void parse_config_data(const char *configfile_filename_report,
libxl_domain_build_info_init(b_info);
libxl_domain_build_info_init_type(b_info, c_info->type);
+ if (blkdev_start)
+ b_info->blkdev_start = strdup(blkdev_start);
/* the following is the actual config parsing with overriding values in the structures */
if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter
2012-04-24 10:45 ` [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
@ 2012-04-24 14:52 ` Ian Jackson
0 siblings, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 14:52 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter"):
> Introduce a blkdev_start in xl.conf and a corresponding string in
> libxl_domain_build_info.
...
> + libxl_device_disk **new_disk,
> + char *blkdev_start)
Shouldn't this be const char * ?
You should mention in the commit message that in this patch the value
is still ignored - ie, it will be used later.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
` (4 preceding siblings ...)
2012-04-24 10:45 ` [PATCH v4 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 14:58 ` Ian Jackson
2012-04-24 10:45 ` [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce libxl__alloc_vdev: find a spare virtual block device in the
domain passed as argument.
Changes in v4:
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- better error handling;
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_internal.c | 22 ++++++++++++++++++++
tools/libxl/libxl_internal.h | 2 +
tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_netbsd.c | 6 +++++
4 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f467572..ac73070 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,28 @@ out:
return rc;
}
+static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
+ char *blkdev_start, xs_transaction_t t)
+{
+ int devid = 0;
+ char *vdev = libxl__strdup(gc, blkdev_start);
+ char *dompath = libxl__xs_get_dompath(gc, domid);
+
+ do {
+ devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+ if (libxl__xs_read(gc, t,
+ libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+ dompath, devid)) == NULL) {
+ if (errno == ENOENT)
+ return libxl__devid_to_localdev(gc, devid);
+ else
+ return NULL;
+ }
+ vdev[strlen(vdev) - 1]++;
+ } while (vdev[strlen(vdev) - 1] <= 'z');
+ return NULL;
+}
+
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *disk,
libxl_device_disk **new_disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 97775fb..ed145ab 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -761,6 +761,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
*/
_hidden int libxl__try_phy_backend(mode_t st_mode);
+_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
+
/* from libxl_pci */
_hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..cb596a9 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)
return 1;
}
+
+#define EXT_SHIFT 28
+#define EXTENDED (1<<EXT_SHIFT)
+#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
+#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
+
+/* same as in Linux */
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+ if (n >= 26)
+ ptr = encode_disk_name(ptr, n / 26 - 1);
+ *ptr = 'a' + n % 26;
+ return ptr + 1;
+}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ int minor;
+ int offset;
+ int nr_parts;
+ char *ptr = NULL;
+ char *ret = libxl__zalloc(gc, 32);
+
+ if (!VDEV_IS_EXTENDED(devid)) {
+ minor = devid & 0xff;
+ nr_parts = 16;
+ } else {
+ minor = BLKIF_MINOR_EXT(devid);
+ nr_parts = 256;
+ }
+ offset = minor / nr_parts;
+
+ /* arbitrary upper bound */
+ if (offset > 676)
+ return NULL;
+
+ strcpy(ret, "xvd");
+ ptr = encode_disk_name(ret + 3, offset);
+ if (minor % nr_parts == 0)
+ *ptr = 0;
+ else
+ snprintf(ptr, ret + 32 - ptr,
+ "%d", minor & (nr_parts - 1));
+ return ret;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
return 0;
}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ /* TODO */
+ return NULL;
+}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev
2012-04-24 10:45 ` [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
@ 2012-04-24 14:58 ` Ian Jackson
2012-04-24 15:04 ` Ian Campbell
0 siblings, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 14:58 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Jackson, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev"):
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...
> +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> + char *blkdev_start, xs_transaction_t t)
> +{
If this function is ever called with domid != our own, this will
malfunction, because ...
> + if (errno == ENOENT)
> + return libxl__devid_to_localdev(gc, devid);
... libxl__devid_to_localdev only answers the question about the
current domain.
This needs to be mentioned in a documentation comment by the function,
at the very least. Does your series invoke it with a domid other than
our own ?
> + else
> + return NULL;
> + }
> + vdev[strlen(vdev) - 1]++;
> + } while (vdev[strlen(vdev) - 1] <= 'z');
There is a scaling limit here of not starting more than 26 domains
simultaneously. Is that acceptable ? I'm tempted to suggest not.
Note that "simultaneously" includes the case where all 27 of them are
simply sat waiting for someone to press "return" on a pygrub prompt.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev
2012-04-24 14:58 ` Ian Jackson
@ 2012-04-24 15:04 ` Ian Campbell
2012-04-24 15:16 ` Ian Jackson
0 siblings, 1 reply; 62+ messages in thread
From: Ian Campbell @ 2012-04-24 15:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Stefano Stabellini
On Tue, 2012-04-24 at 15:58 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> > + char *blkdev_start, xs_transaction_t t)
> > +{
>
> If this function is ever called with domid != our own, this will
> malfunction, because ...
>
> > + if (errno == ENOENT)
> > + return libxl__devid_to_localdev(gc, devid);
>
> ... libxl__devid_to_localdev only answers the question about the
> current domain.
>
> This needs to be mentioned in a documentation comment by the function,
> at the very least. Does your series invoke it with a domid other than
> our own ?
>
> > + else
> > + return NULL;
> > + }
> > + vdev[strlen(vdev) - 1]++;
> > + } while (vdev[strlen(vdev) - 1] <= 'z');
>
> There is a scaling limit here of not starting more than 26 domains
> simultaneously. Is that acceptable ? I'm tempted to suggest not.
While I agree we also need to start being pragmatic about ever getting
4.2 out the door...
If it's a trivial job to support aa, ab etc then lets do it, if not then
lets leave it for 4.3?
> Note that "simultaneously" includes the case where all 27 of them are
> simply sat waiting for someone to press "return" on a pygrub prompt.
>
> Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev
2012-04-24 15:04 ` Ian Campbell
@ 2012-04-24 15:16 ` Ian Jackson
0 siblings, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Stefano Stabellini
Ian Campbell writes ("Re: [Xen-devel] [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev"):
> On Tue, 2012-04-24 at 15:58 +0100, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev"):
> > > + } while (vdev[strlen(vdev) - 1] <= 'z');
> >
> > There is a scaling limit here of not starting more than 26 domains
> > simultaneously. Is that acceptable ? I'm tempted to suggest not.
>
> While I agree we also need to start being pragmatic about ever getting
> 4.2 out the door...
>
> If it's a trivial job to support aa, ab etc then lets do it, if not then
> lets leave it for 4.3?
The problem is because the code is trying to avoid too much knowledge
of the devid scheme; in particular, it's trying to avoid introducing
the inverse function to libxl__device_disk_dev_number. Although it
/does/ depend intimately on the details of
libxl__device_disk_dev_number.
It is arguably a bug that libxl__device_disk_dev_number combines the
parsing of vdev strings with the composition of disk/partition numbers
into devids. But we can avoid needing to decompose it by passing
libxl__device_disk_dev_number a format which is easier to create than
one with a base-26-encoded disk number.
What I would do is:
* call libxl__device_disk_dev_number once on blkdev_start with
non-null arguments for pdisk and ppartition; check that
the partition is 0.
* loop incrementing the disk value
* on each iteration,
- generate a vdev = GCSPRINTF("d%d", disk);
- use libxl__device_disk_dev_number on that vdev to get the devid
- check whether this devid is available
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
` (5 preceding siblings ...)
2012-04-24 10:45 ` [PATCH v4 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:39 ` Ian Jackson
2012-04-24 10:45 ` [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
- Spawn a QEMU instance at boot time to handle disk local attach
requests.
- Implement libxl_device_disk_local_attach for QDISKs in terms of a
regular disk attach with the frontend and backend running in the same
domain.
Changes on v4:
- improve error handling and exit paths in libxl__device_disk_local_attach.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
tools/hotplug/Linux/init.d/xencommons | 3 +
tools/libxl/libxl_internal.c | 63 +++++++++++++++++++----
tools/libxl/libxl_internal.h | 2 +
4 files changed, 60 insertions(+), 11 deletions(-)
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons
index 6543204..0f3b9ad 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons
@@ -12,3 +12,6 @@
# Running xenbackendd in debug mode
#XENBACKENDD_DEBUG=[yes|on|1]
+
+# qemu path and log file
+#QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
index 2f81ea2..9dda6e2 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -104,6 +104,9 @@ do_start () {
xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
test "`uname`" != "NetBSD" || xenbackendd $XENBACKENDD_ARGS
+ echo Starting QEMU as disk backend for dom0
+ test -z "$QEMU_XEN" && QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
+ $QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize -monitor /dev/null
}
do_stop () {
echo Stopping xenconsoled
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ac73070..bb75491 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -510,6 +510,7 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
int rc;
+ xs_transaction_t t = XBT_NULL;
libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
if (tmpdisk == NULL) goto out;
@@ -554,13 +555,40 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
break;
case LIBXL_DISK_BACKEND_QDISK:
if (tmpdisk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
- " attach a qdisk image if the format is not raw");
- break;
+ do {
+ t = xs_transaction_start(ctx->xsh);
+ if (t == XBT_NULL) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "failed to start a xenstore transaction");
+ goto out;
+ }
+ tmpdisk->vdev = libxl__alloc_vdev(gc,
+ LIBXL_TOOLSTACK_DOMID, blkdev_start, t);
+ if (tmpdisk->vdev == NULL) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "libxl__alloc_vdev failed\n");
+ goto out;
+ }
+ if (libxl__device_disk_add_t(gc, LIBXL_TOOLSTACK_DOMID,
+ t, tmpdisk)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "libxl_device_disk_add failed\n");
+ goto out;
+ }
+ rc = xs_transaction_end(ctx->xsh, t, 0);
+ } while (rc == 0 && errno == EAGAIN);
+ t = XBT_NULL;
+ if (rc == 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "xenstore transaction failed");
+ goto out;
+ }
+ dev = GCSPRINTF("/dev/%s", tmpdisk->vdev);
+ } else {
+ dev = tmpdisk->pdev_path;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
- disk->pdev_path);
- dev = tmpdisk->pdev_path;
+ dev);
break;
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
@@ -569,17 +597,30 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
}
out:
+ if (t != XBT_NULL)
+ xs_transaction_end(ctx->xsh, t, 1);
return dev;
}
_hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
{
- /* Nothing to do for PHYSTYPE_PHY. */
-
- /*
- * For other device types assume that the blktap2 process is
- * needed by the soon to be started domain and do nothing.
- */
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_QDISK:
+ if (disk->vdev != NULL) {
+ libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
+ disk, 0);
+ return libxl_device_disk_destroy(gc->owner,
+ LIBXL_TOOLSTACK_DOMID, disk);
+ }
+ break;
+ default:
+ /*
+ * Nothing to do for PHYSTYPE_PHY.
+ * For other device types assume that the blktap2 process is
+ * needed by the soon to be started domain and do nothing.
+ */
+ break;
+ }
return 0;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ed145ab..dfd7b49 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -77,6 +77,8 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
#define STUBDOM_CONSOLE_LOGGING 0
#define STUBDOM_CONSOLE_SAVE 1
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-04-24 10:45 ` [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
@ 2012-04-24 15:39 ` Ian Jackson
0 siblings, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:39 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index ac73070..bb75491 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -510,6 +510,7 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
...
> + if (tmpdisk->vdev == NULL) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "libxl__alloc_vdev failed\n");
LIBXL__LOG doesn't need an additional "\n". You have several of
these. Also you may prefer my new LOG convenience macro which is a
lot less verbose - look, no wrapping needed:
LOG(ERROR, "libxl__alloc_vdev failed);
> + switch (disk->backend) {
> + case LIBXL_DISK_BACKEND_QDISK:
> + if (disk->vdev != NULL) {
> + libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
> + disk, 0);
> + return libxl_device_disk_destroy(gc->owner,
> + LIBXL_TOOLSTACK_DOMID, disk);
> + }
> + break;
> + default:
> + /*
> + * Nothing to do for PHYSTYPE_PHY.
> + * For other device types assume that the blktap2 process is
> + * needed by the soon to be started domain and do nothing.
> + */
> + break;
I guess what I meant with my previous comment is that it might be
better to have _local_attach return some kind of context/state struct,
bigger than the libxl__device_disk*, that would be passed to _detach.
But this will do.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected"
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
` (6 preceding siblings ...)
2012-04-24 10:45 ` [PATCH v4 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
@ 2012-04-24 10:45 ` Stefano Stabellini
2012-04-24 15:18 ` Ian Jackson
7 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-04-24 10:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
In order to make sure that the block device is available and ready to be
used, wait for state "connected" in the backend before returning.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Changes in v4:
- improve exit paths.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_internal.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index bb75491..98a67d6 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -508,8 +508,9 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
- char *dev = NULL;
+ char *dev = NULL, *be_path = NULL;
int rc;
+ libxl__device device;
xs_transaction_t t = XBT_NULL;
libxl_device_disk *tmpdisk = libxl__zalloc(gc, sizeof(libxl_device_disk));
if (tmpdisk == NULL) goto out;
@@ -596,10 +597,23 @@ _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
break;
}
+ if (tmpdisk->vdev != NULL) {
+ rc = libxl__device_from_disk(gc, 0, tmpdisk, &device);
+ if (rc < 0)
+ goto out_wait_err;
+ be_path = libxl__device_backend_path(gc, &device);
+ rc = libxl__wait_for_backend(gc, be_path, "4");
+ if (rc < 0)
+ goto out_wait_err;
+ }
+
out:
if (t != XBT_NULL)
xs_transaction_end(ctx->xsh, t, 1);
return dev;
+ out_wait_err:
+ libxl__device_disk_local_detach(gc, tmpdisk);
+ return NULL;
}
_hidden int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected"
2012-04-24 10:45 ` [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
@ 2012-04-24 15:18 ` Ian Jackson
0 siblings, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-04-24 15:18 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian.Campbell
Stefano Stabellini writes ("[Xen-devel] [PATCH v4 8/8] libxl__device_disk_local_attach: wait for state "connected""):
> In order to make sure that the block device is available and ready to be
> used, wait for state "connected" in the backend before returning.
...
> Changes in v4:
> - improve exit paths.
...
> + if (tmpdisk->vdev != NULL) {
> + rc = libxl__device_from_disk(gc, 0, tmpdisk, &device);
> + if (rc < 0)
> + goto out_wait_err;
This still seems to have two error exit paths:
> out:
> if (t != XBT_NULL)
> xs_transaction_end(ctx->xsh, t, 1);
> return dev;
> + out_wait_err:
> + libxl__device_disk_local_detach(gc, tmpdisk);
> + return NULL;
Previously "out" was both a success and error exit path AFAICT.
I really don't care very much whether you want to combine the success
and error exit paths. But there should be only one error exit path.
So please no "goto out_<some particular thing>_err".
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 4/8] libxl: introduce libxl__device_disk_add
2012-05-04 11:13 ` [PATCH v5 4/8] libxl: introduce libxl__device_disk_add Stefano Stabellini
2012-04-24 10:45 ` [PATCH v4 0/8] qdisk local attach Stefano Stabellini
@ 2012-05-04 14:49 ` Ian Campbell
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 14:49 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce libxl__device_disk_add that takes an additional
> xs_transaction_t paramter.
> Implement libxl_device_disk_add using libxl__device_disk_add.
> Move libxl__device_from_disk to libxl_internal.c.
> No functional change.
>
> Changes in v5:
> - rename libxl__device_generic_add_t to libxl__device_generic_add.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl.c | 151 +----------------------------------------
> tools/libxl/libxl_internal.c | 157 ++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_internal.h | 6 ++
> 3 files changed, 164 insertions(+), 150 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 525f0d6..941dbb9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1258,159 +1258,10 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
> return rc;
> }
>
> -static int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
> - libxl_device_disk *disk,
> - libxl__device *device)
> -{
> - libxl_ctx *ctx = libxl__gc_owner(gc);
> - int devid;
> -
> - devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
> - if (devid==-1) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
> - " virtual disk identifier %s", disk->vdev);
> - return ERROR_INVAL;
> - }
> -
> - device->backend_domid = disk->backend_domid;
> - device->backend_devid = devid;
> -
> - switch (disk->backend) {
> - case LIBXL_DISK_BACKEND_PHY:
> - device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> - break;
> - case LIBXL_DISK_BACKEND_TAP:
> - device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> - break;
> - case LIBXL_DISK_BACKEND_QDISK:
> - device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
> - break;
> - default:
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
> - disk->backend);
> - return ERROR_INVAL;
> - }
> -
> - device->domid = domid;
> - device->devid = devid;
> - device->kind = LIBXL__DEVICE_KIND_VBD;
> -
> - return 0;
> -}
> -
> int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
> {
> GC_INIT(ctx);
> - flexarray_t *front;
> - flexarray_t *back;
> - char *dev;
> - libxl__device device;
> - int major, minor, rc;
> -
> - rc = libxl__device_disk_setdefault(gc, disk);
> - if (rc) goto out;
> -
> - front = flexarray_make(16, 1);
> - if (!front) {
> - rc = ERROR_NOMEM;
> - goto out;
> - }
> - back = flexarray_make(16, 1);
> - if (!back) {
> - rc = ERROR_NOMEM;
> - goto out_free;
> - }
> -
> - if (disk->script) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
> - " not yet supported, sorry");
> - rc = ERROR_INVAL;
> - goto out_free;
> - }
> -
> - rc = libxl__device_from_disk(gc, domid, disk, &device);
> - if (rc != 0) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
> - " virtual disk identifier %s", disk->vdev);
> - goto out_free;
> - }
> -
> - switch (disk->backend) {
> - case LIBXL_DISK_BACKEND_PHY:
> - dev = disk->pdev_path;
> - do_backend_phy:
> - libxl__device_physdisk_major_minor(dev, &major, &minor);
> - flexarray_append(back, "physical-device");
> - flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
> -
> - flexarray_append(back, "params");
> - flexarray_append(back, dev);
> -
> - assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
> - break;
> - case LIBXL_DISK_BACKEND_TAP:
> - dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> - if (!dev) {
> - rc = ERROR_FAIL;
> - goto out_free;
> - }
> - flexarray_append(back, "tapdisk-params");
> - flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> - libxl__device_disk_string_of_format(disk->format),
> - disk->pdev_path));
> -
> - /* now create a phy device to export the device to the guest */
> - goto do_backend_phy;
> - case LIBXL_DISK_BACKEND_QDISK:
> - flexarray_append(back, "params");
> - flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> - libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
> - assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
> - break;
> - default:
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
> - rc = ERROR_INVAL;
> - goto out_free;
> - }
> -
> - flexarray_append(back, "frontend-id");
> - flexarray_append(back, libxl__sprintf(gc, "%d", domid));
> - flexarray_append(back, "online");
> - flexarray_append(back, "1");
> - flexarray_append(back, "removable");
> - flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
> - flexarray_append(back, "bootable");
> - flexarray_append(back, libxl__sprintf(gc, "%d", 1));
> - flexarray_append(back, "state");
> - flexarray_append(back, libxl__sprintf(gc, "%d", 1));
> - flexarray_append(back, "dev");
> - flexarray_append(back, disk->vdev);
> - flexarray_append(back, "type");
> - flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
> - flexarray_append(back, "mode");
> - flexarray_append(back, disk->readwrite ? "w" : "r");
> - flexarray_append(back, "device-type");
> - flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> -
> - flexarray_append(front, "backend-id");
> - flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
> - flexarray_append(front, "state");
> - flexarray_append(front, libxl__sprintf(gc, "%d", 1));
> - flexarray_append(front, "virtual-device");
> - flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
> - flexarray_append(front, "device-type");
> - flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
> -
> - libxl__device_generic_add(gc, XBT_NULL, &device,
> - libxl__xs_kvs_of_flexarray(gc, back, back->count),
> - libxl__xs_kvs_of_flexarray(gc, front, front->count));
> -
> - rc = 0;
> -
> -out_free:
> - flexarray_free(back);
> - flexarray_free(front);
> -out:
> + int rc = libxl__device_disk_add(gc, domid, XBT_NULL, disk);
> GC_FREE;
> return rc;
> }
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 55dc55c..1bf5d73 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -323,6 +323,163 @@ out:
> return rc;
> }
>
> +int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
> + libxl_device_disk *disk,
> + libxl__device *device)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + int devid;
> +
> + devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
> + if (devid==-1) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
> + " virtual disk identifier %s", disk->vdev);
> + return ERROR_INVAL;
> + }
> +
> + device->backend_domid = disk->backend_domid;
> + device->backend_devid = devid;
> +
> + switch (disk->backend) {
> + case LIBXL_DISK_BACKEND_PHY:
> + device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> + break;
> + case LIBXL_DISK_BACKEND_TAP:
> + device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> + break;
> + case LIBXL_DISK_BACKEND_QDISK:
> + device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
> + break;
> + default:
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
> + disk->backend);
> + return ERROR_INVAL;
> + }
> +
> + device->domid = domid;
> + device->devid = devid;
> + device->kind = LIBXL__DEVICE_KIND_VBD;
> +
> + return 0;
> +}
> +
> +int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
> + xs_transaction_t t, libxl_device_disk *disk)
> +{
> + flexarray_t *front;
> + flexarray_t *back;
> + char *dev;
> + libxl__device device;
> + int major, minor, rc;
> + libxl_ctx *ctx = gc->owner;
> +
> + rc = libxl__device_disk_setdefault(gc, disk);
> + if (rc) goto out;
> +
> + front = flexarray_make(16, 1);
> + if (!front) {
> + rc = ERROR_NOMEM;
> + goto out;
> + }
> + back = flexarray_make(16, 1);
> + if (!back) {
> + rc = ERROR_NOMEM;
> + goto out_free;
> + }
> +
> + if (disk->script) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "External block scripts"
> + " not yet supported, sorry");
> + rc = ERROR_INVAL;
> + goto out_free;
> + }
> +
> + rc = libxl__device_from_disk(gc, domid, disk, &device);
> + if (rc != 0) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
> + " virtual disk identifier %s", disk->vdev);
> + goto out_free;
> + }
> +
> + switch (disk->backend) {
> + case LIBXL_DISK_BACKEND_PHY:
> + dev = disk->pdev_path;
> + do_backend_phy:
> + libxl__device_physdisk_major_minor(dev, &major, &minor);
> + flexarray_append(back, "physical-device");
> + flexarray_append(back, libxl__sprintf(gc, "%x:%x", major, minor));
> +
> + flexarray_append(back, "params");
> + flexarray_append(back, dev);
> +
> + assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
> + break;
> + case LIBXL_DISK_BACKEND_TAP:
> + dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> + if (!dev) {
> + rc = ERROR_FAIL;
> + goto out_free;
> + }
> + flexarray_append(back, "tapdisk-params");
> + flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> + libxl__device_disk_string_of_format(disk->format),
> + disk->pdev_path));
> +
> + /* now create a phy device to export the device to the guest */
> + goto do_backend_phy;
> + case LIBXL_DISK_BACKEND_QDISK:
> + flexarray_append(back, "params");
> + flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> + libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
> + assert(device.backend_kind == LIBXL__DEVICE_KIND_QDISK);
> + break;
> + default:
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
> + rc = ERROR_INVAL;
> + goto out_free;
> + }
> +
> + flexarray_append(back, "frontend-id");
> + flexarray_append(back, libxl__sprintf(gc, "%d", domid));
> + flexarray_append(back, "online");
> + flexarray_append(back, "1");
> + flexarray_append(back, "removable");
> + flexarray_append(back, libxl__sprintf(gc, "%d", (disk->removable) ? 1 : 0));
> + flexarray_append(back, "bootable");
> + flexarray_append(back, libxl__sprintf(gc, "%d", 1));
> + flexarray_append(back, "state");
> + flexarray_append(back, libxl__sprintf(gc, "%d", 1));
> + flexarray_append(back, "dev");
> + flexarray_append(back, disk->vdev);
> + flexarray_append(back, "type");
> + flexarray_append(back, libxl__device_disk_string_of_backend(disk->backend));
> + flexarray_append(back, "mode");
> + flexarray_append(back, disk->readwrite ? "w" : "r");
> + flexarray_append(back, "device-type");
> + flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> +
> + flexarray_append(front, "backend-id");
> + flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
> + flexarray_append(front, "state");
> + flexarray_append(front, libxl__sprintf(gc, "%d", 1));
> + flexarray_append(front, "virtual-device");
> + flexarray_append(front, libxl__sprintf(gc, "%d", device.devid));
> + flexarray_append(front, "device-type");
> + flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk");
> +
> + libxl__device_generic_add(gc, t, &device,
> + libxl__xs_kvs_of_flexarray(gc, back, back->count),
> + libxl__xs_kvs_of_flexarray(gc, front, front->count));
> +
> + rc = 0;
> +
> +out_free:
> + flexarray_free(back);
> + flexarray_free(front);
> +out:
> + return rc;
> +}
> +
> char * libxl__device_disk_local_attach(libxl__gc *gc,
> const libxl_device_disk *in_disk,
> libxl_device_disk **new_disk)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 2c8f06a..096c96d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1003,6 +1003,12 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
> */
> _hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
>
> +
> +_hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
> + libxl_device_disk *disk,
> + libxl__device *device);
> +_hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
> + xs_transaction_t t, libxl_device_disk *disk);
> /*
> * Make a disk available in this (the control) domain. Returns path to
> * a device.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (3 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 4/8] libxl: introduce libxl__device_disk_add Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 14:54 ` Ian Campbell
2012-05-04 16:32 ` Ian Jackson
2012-05-04 11:13 ` [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
` (3 subsequent siblings)
8 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce a blkdev_start in xl.conf and a corresponding string in
libxl_domain_build_info.
Add a blkdev_start parameter to libxl__device_disk_local_attach: it is
going to be used in a following patch.
blkdev_start specifies the first block device to be used for temporary
block device allocations by the toolstack.
Changes in v5:
- change the type of the blkdev_start parameter to
libxl__device_disk_local_attach to const char *.
Changes in v4:
- pass blkdev_start in libxl_domain_build_info.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/examples/xl.conf | 3 +++
tools/libxl/libxl_bootloader.c | 3 ++-
tools/libxl/libxl_create.c | 3 +++
tools/libxl/libxl_internal.c | 3 ++-
tools/libxl/libxl_internal.h | 3 ++-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl.c | 3 +++
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 2 ++
9 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 56d3b3b..ebf057c 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -12,3 +12,6 @@
# default output format used by "xl list -l"
#output_format="json"
+
+# first block device to be used for temporary VM disk mounts
+#blkdev_start="xvda"
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 83cac78..123b06e 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -387,7 +387,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
goto out_close;
}
- diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
+ diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk,
+ info->blkdev_start);
if (!diskpath) {
rc = ERROR_FAIL;
goto out_close;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 7ab2f72..f2fcdf0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
}
+ if (b_info->blkdev_start == NULL)
+ b_info->blkdev_start = strdup("xvda");
+
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!b_info->u.hvm.bios)
switch (b_info->device_model_version) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 1bf5d73..bd5ebb3 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -482,7 +482,8 @@ out:
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
- libxl_device_disk **new_disk)
+ libxl_device_disk **new_disk,
+ const char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 096c96d..0074ec4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1015,7 +1015,8 @@ _hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
*/
_hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
- libxl_device_disk **new_disk);
+ libxl_device_disk **new_disk,
+ const char *blkdev_start);
_hidden int libxl__device_disk_local_detach(libxl__gc *gc,
libxl_device_disk *disk);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 68599cb..7131696 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -253,6 +253,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("localtime", libxl_defbool),
("disable_migrate", libxl_defbool),
("cpuid", libxl_cpuid_policy_list),
+ ("blkdev_start", string),
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25..4596c4f 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -35,6 +35,7 @@
xentoollog_logger_stdiostream *logger;
int dryrun_only;
int autoballoon = 1;
+char *blkdev_start;
char *lockfile;
char *default_vifscript = NULL;
char *default_bridge = NULL;
@@ -91,6 +92,8 @@ static void parse_global_config(const char *configfile,
fprintf(stderr, "invalid default output format \"%s\"\n", buf);
}
}
+ if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
+ blkdev_start = strdup(buf);
xlu_cfg_destroy(config);
}
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index f0d0ec8..3fbe14d 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -112,6 +112,7 @@ extern int dryrun_only;
extern char *lockfile;
extern char *default_vifscript;
extern char *default_bridge;
+extern char *blkdev_start;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 28f5cab..7338562 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -595,6 +595,8 @@ static void parse_config_data(const char *configfile_filename_report,
}
libxl_domain_build_info_init_type(b_info, c_info->type);
+ if (blkdev_start)
+ b_info->blkdev_start = strdup(blkdev_start);
/* the following is the actual config parsing with overriding values in the structures */
if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter
2012-05-04 11:13 ` [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
@ 2012-05-04 14:54 ` Ian Campbell
2012-05-04 16:32 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 14:54 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce a blkdev_start in xl.conf and a corresponding string in
> libxl_domain_build_info.
>
> Add a blkdev_start parameter to libxl__device_disk_local_attach: it is
> going to be used in a following patch.
>
> blkdev_start specifies the first block device to be used for temporary
> block device allocations by the toolstack.
>
> Changes in v5:
> - change the type of the blkdev_start parameter to
> libxl__device_disk_local_attach to const char *.
>
> Changes in v4:
> - pass blkdev_start in libxl_domain_build_info.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/examples/xl.conf | 3 +++
> tools/libxl/libxl_bootloader.c | 3 ++-
> tools/libxl/libxl_create.c | 3 +++
> tools/libxl/libxl_internal.c | 3 ++-
> tools/libxl/libxl_internal.h | 3 ++-
> tools/libxl/libxl_types.idl | 1 +
> tools/libxl/xl.c | 3 +++
> tools/libxl/xl.h | 1 +
> tools/libxl/xl_cmdimpl.c | 2 ++
> 9 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> index 56d3b3b..ebf057c 100644
> --- a/tools/examples/xl.conf
> +++ b/tools/examples/xl.conf
> @@ -12,3 +12,6 @@
>
> # default output format used by "xl list -l"
> #output_format="json"
> +
> +# first block device to be used for temporary VM disk mounts
> +#blkdev_start="xvda"
Need a patch to docs/man/xl.conf.pod.5 as well. If you add that then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
I suppose this is necessarily Linux specific and we'll need a patch from
the BSD folks?
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 83cac78..123b06e 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -387,7 +387,8 @@ int libxl_run_bootloader(libxl_ctx *ctx,
> goto out_close;
> }
>
> - diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk);
> + diskpath = libxl__device_disk_local_attach(gc, disk, &tmpdisk,
> + info->blkdev_start);
> if (!diskpath) {
> rc = ERROR_FAIL;
> goto out_close;
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 7ab2f72..f2fcdf0 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -107,6 +107,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> }
> }
>
> + if (b_info->blkdev_start == NULL)
> + b_info->blkdev_start = strdup("xvda");
> +
> if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> if (!b_info->u.hvm.bios)
> switch (b_info->device_model_version) {
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 1bf5d73..bd5ebb3 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -482,7 +482,8 @@ out:
>
> char * libxl__device_disk_local_attach(libxl__gc *gc,
> const libxl_device_disk *in_disk,
> - libxl_device_disk **new_disk)
> + libxl_device_disk **new_disk,
> + const char *blkdev_start)
> {
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 096c96d..0074ec4 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1015,7 +1015,8 @@ _hidden int libxl__device_disk_add(libxl__gc *gc, uint32_t domid,
> */
> _hidden char * libxl__device_disk_local_attach(libxl__gc *gc,
> const libxl_device_disk *in_disk,
> - libxl_device_disk **new_disk);
> + libxl_device_disk **new_disk,
> + const char *blkdev_start);
> _hidden int libxl__device_disk_local_detach(libxl__gc *gc,
> libxl_device_disk *disk);
>
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 68599cb..7131696 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -253,6 +253,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("localtime", libxl_defbool),
> ("disable_migrate", libxl_defbool),
> ("cpuid", libxl_cpuid_policy_list),
> + ("blkdev_start", string),
>
> ("device_model_version", libxl_device_model_version),
> ("device_model_stubdomain", libxl_defbool),
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index a6ffd25..4596c4f 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -35,6 +35,7 @@
> xentoollog_logger_stdiostream *logger;
> int dryrun_only;
> int autoballoon = 1;
> +char *blkdev_start;
> char *lockfile;
> char *default_vifscript = NULL;
> char *default_bridge = NULL;
> @@ -91,6 +92,8 @@ static void parse_global_config(const char *configfile,
> fprintf(stderr, "invalid default output format \"%s\"\n", buf);
> }
> }
> + if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
> + blkdev_start = strdup(buf);
> xlu_cfg_destroy(config);
> }
>
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index f0d0ec8..3fbe14d 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -112,6 +112,7 @@ extern int dryrun_only;
> extern char *lockfile;
> extern char *default_vifscript;
> extern char *default_bridge;
> +extern char *blkdev_start;
>
> enum output_format {
> OUTPUT_FORMAT_JSON,
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 28f5cab..7338562 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -595,6 +595,8 @@ static void parse_config_data(const char *configfile_filename_report,
> }
>
> libxl_domain_build_info_init_type(b_info, c_info->type);
> + if (blkdev_start)
> + b_info->blkdev_start = strdup(blkdev_start);
>
> /* the following is the actual config parsing with overriding values in the structures */
> if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter
2012-05-04 11:13 ` [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
2012-05-04 14:54 ` Ian Campbell
@ 2012-05-04 16:32 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:32 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("[PATCH v5 5/8] xl/libxl: add a blkdev_start parameter"):
> + if (b_info->blkdev_start == NULL)
> + b_info->blkdev_start = strdup("xvda");
This should be libxl__strdup(0, "xvda")
to get the correct error behaviour.
Aside from that,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (4 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 5/8] xl/libxl: add a blkdev_start parameter Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 14:59 ` Ian Campbell
2012-05-04 16:39 ` Ian Jackson
2012-05-04 11:13 ` [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
` (2 subsequent siblings)
8 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
Introduce libxl__alloc_vdev: find a spare virtual block device in the
domain passed as argument.
Changes in v5:
- remove domid paramter to libxl__alloc_vdev (assume
LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev.
Changes in v4:
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- better error handling;
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 4 +++
tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_netbsd.c | 6 +++++
4 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index bd5ebb3..7a1e017 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,37 @@ out:
return rc;
}
+/* libxl__alloc_vdev only works on the local domain, that is the domain
+ * where the toolstack is running */
+static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
+ xs_transaction_t t)
+{
+ int devid = 0, disk = 0, part = 0;
+ char *vdev = NULL;
+ char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
+
+ libxl__device_disk_dev_number(blkdev_start, &disk, &part);
+ if (part != 0) {
+ LOG(ERROR, "blkdev_start is invalid");
+ return NULL;
+ }
+
+ do {
+ vdev = GCSPRINTF("d%dp0", disk);
+ devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+ if (libxl__xs_read(gc, t,
+ libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+ dompath, devid)) == NULL) {
+ if (errno == ENOENT)
+ return libxl__devid_to_localdev(gc, devid);
+ else
+ return NULL;
+ }
+ disk++;
+ } while (disk <= (1 << 20));
+ return NULL;
+}
+
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
libxl_device_disk **new_disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0074ec4..a451c79 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -77,6 +77,8 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
#define STUBDOM_CONSOLE_LOGGING 0
#define STUBDOM_CONSOLE_SAVE 1
@@ -763,6 +765,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
*/
_hidden int libxl__try_phy_backend(mode_t st_mode);
+_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
+
/* from libxl_pci */
_hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..cb596a9 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)
return 1;
}
+
+#define EXT_SHIFT 28
+#define EXTENDED (1<<EXT_SHIFT)
+#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
+#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
+
+/* same as in Linux */
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+ if (n >= 26)
+ ptr = encode_disk_name(ptr, n / 26 - 1);
+ *ptr = 'a' + n % 26;
+ return ptr + 1;
+}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ int minor;
+ int offset;
+ int nr_parts;
+ char *ptr = NULL;
+ char *ret = libxl__zalloc(gc, 32);
+
+ if (!VDEV_IS_EXTENDED(devid)) {
+ minor = devid & 0xff;
+ nr_parts = 16;
+ } else {
+ minor = BLKIF_MINOR_EXT(devid);
+ nr_parts = 256;
+ }
+ offset = minor / nr_parts;
+
+ /* arbitrary upper bound */
+ if (offset > 676)
+ return NULL;
+
+ strcpy(ret, "xvd");
+ ptr = encode_disk_name(ret + 3, offset);
+ if (minor % nr_parts == 0)
+ *ptr = 0;
+ else
+ snprintf(ptr, ret + 32 - ptr,
+ "%d", minor & (nr_parts - 1));
+ return ret;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
return 0;
}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ /* TODO */
+ return NULL;
+}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 11:13 ` [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
@ 2012-05-04 14:59 ` Ian Campbell
2012-05-04 16:46 ` Ian Jackson
2012-05-14 13:48 ` Stefano Stabellini
2012-05-04 16:39 ` Ian Jackson
1 sibling, 2 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 14:59 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
>
> Changes in v5:
> - remove domid paramter to libxl__alloc_vdev (assume
> LIBXL_TOOLSTACK_DOMID);
> - remove scaling limit from libxl__alloc_vdev.
>
> Changes in v4:
> - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> - introduce upper bound for encode_disk_name;
> - better error handling;
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> tools/libxl/libxl_internal.h | 4 +++
> tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_netbsd.c | 6 +++++
> 4 files changed, 86 insertions(+), 0 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index bd5ebb3..7a1e017 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -480,6 +480,37 @@ out:
> return rc;
> }
>
> +/* libxl__alloc_vdev only works on the local domain, that is the domain
> + * where the toolstack is running */
> +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> + xs_transaction_t t)
> +{
> + int devid = 0, disk = 0, part = 0;
> + char *vdev = NULL;
> + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> +
> + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
If you specify the default blkdev_start in xl as d0 instead of xvda
doesn't at least this bit magically become portable to BSD etc too?
Or couldn't it actually be an int and save you parsing at all?
> + if (part != 0) {
> + LOG(ERROR, "blkdev_start is invalid");
> + return NULL;
> + }
> +
> + do {
> + vdev = GCSPRINTF("d%dp0", disk);
> + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
without the hardcoded "p0"), I think.
I'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
since you don't use it again.
> + if (libxl__xs_read(gc, t,
> + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> + dompath, devid)) == NULL) {
> + if (errno == ENOENT)
> + return libxl__devid_to_localdev(gc, devid);
> + else
> + return NULL;
> + }
> + disk++;
> + } while (disk <= (1 << 20));
That's a whole lotta disks ;-)
> + return NULL;
> +}
> +
> char * libxl__device_disk_local_attach(libxl__gc *gc,
> const libxl_device_disk *in_disk,
> libxl_device_disk **new_disk,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 0074ec4..a451c79 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -77,6 +77,8 @@
> #define LIBXL_PV_EXTRA_MEMORY 1024
> #define LIBXL_HVM_EXTRA_MEMORY 2048
> #define LIBXL_MIN_DOM0_MEM (128*1024)
> +/* use 0 as the domid of the toolstack domain for now */
> +#define LIBXL_TOOLSTACK_DOMID 0
> #define QEMU_SIGNATURE "DeviceModelRecord0002"
> #define STUBDOM_CONSOLE_LOGGING 0
> #define STUBDOM_CONSOLE_SAVE 1
> @@ -763,6 +765,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
> */
> _hidden int libxl__try_phy_backend(mode_t st_mode);
>
> +_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
> +
> /* from libxl_pci */
>
> _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..cb596a9 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 1;
> }
> +
> +#define EXT_SHIFT 28
> +#define EXTENDED (1<<EXT_SHIFT)
> +#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
> +#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
> +
> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> + if (n >= 26)
> + ptr = encode_disk_name(ptr, n / 26 - 1);
> + *ptr = 'a' + n % 26;
> + return ptr + 1;
> +}
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> + int minor;
> + int offset;
> + int nr_parts;
> + char *ptr = NULL;
> + char *ret = libxl__zalloc(gc, 32);
> +
> + if (!VDEV_IS_EXTENDED(devid)) {
> + minor = devid & 0xff;
> + nr_parts = 16;
> + } else {
> + minor = BLKIF_MINOR_EXT(devid);
> + nr_parts = 256;
> + }
> + offset = minor / nr_parts;
> +
> + /* arbitrary upper bound */
> + if (offset > 676)
> + return NULL;
> +
> + strcpy(ret, "xvd");
> + ptr = encode_disk_name(ret + 3, offset);
> + if (minor % nr_parts == 0)
> + *ptr = 0;
> + else
> + snprintf(ptr, ret + 32 - ptr,
> + "%d", minor & (nr_parts - 1));
> + return ret;
> +}
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..dbf5f71 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
Aha, here's where we need a BSD contribution. CC someone?
> @@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 0;
> }
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> + /* TODO */
> + return NULL;
> +}
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 14:59 ` Ian Campbell
@ 2012-05-04 16:46 ` Ian Jackson
2012-05-04 17:08 ` Ian Campbell
2012-05-14 13:48 ` Stefano Stabellini
1 sibling, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:46 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
...
> > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
>
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn't at least this bit magically become portable to BSD etc too?
This bit is portable already. It's specified in terms of the guest
device name rather than the host device name. This may be confusing
but it saves on having a converter for host device names to disk
numbers.
> Or couldn't it actually be an int and save you parsing at all?
In general it's surely more friendly to allow the user to specify this
with a name, like we do with vdevs elsewhere.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 16:46 ` Ian Jackson
@ 2012-05-04 17:08 ` Ian Campbell
2012-05-04 17:16 ` Ian Jackson
0 siblings, 1 reply; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 17:08 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Stefano Stabellini
On Fri, 2012-05-04 at 17:46 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > > domain passed as argument.
> ...
> > > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
> >
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn't at least this bit magically become portable to BSD etc too?
>
> This bit is portable already. It's specified in terms of the guest
> device name rather than the host device name. This may be confusing
> but it saves on having a converter for host device names to disk
> numbers.
Right. I think I really should have made this comment on the previous
patch, What I was trying to suggest is that using the Linux specific
"xvda" in the example in the config file and as the default (as patch
5/8 does) is not portable, even though the infrastructure itself is
quite capable of dealing with the portable alternatives -- IOW using
"d0" both in the example and in the actual default would make xl be
automatically portable without other patches (libxl still needs a patch
for the BSD libxl__devid_to_localdev but that's it).
> > Or couldn't it actually be an int and save you parsing at all?
>
> In general it's surely more friendly to allow the user to specify this
> with a name, like we do with vdevs elsewhere.
That seems reasonable.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 17:08 ` Ian Campbell
@ 2012-05-04 17:16 ` Ian Jackson
2012-05-04 17:20 ` Ian Campbell
0 siblings, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 17:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Stefano Stabellini
Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> Right. I think I really should have made this comment on the previous
> patch, What I was trying to suggest is that using the Linux specific
> "xvda" in the example in the config file
"xvda" is not currently thought to be necessarily Linux-specific. See
docs/misc/vbd-interface.text. Perhaps that needs to be revisited, but
in general it's perfectly possible to pass "xvda" in a domain config
file for a BSD guest, or whatever. And Linux writing "xvdfoo" in the
config file doesn't always make Linux call the device /dev/xvdfoo.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 17:16 ` Ian Jackson
@ 2012-05-04 17:20 ` Ian Campbell
0 siblings, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 17:20 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Stefano Stabellini
On Fri, 2012-05-04 at 18:16 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > Right. I think I really should have made this comment on the previous
> > patch, What I was trying to suggest is that using the Linux specific
> > "xvda" in the example in the config file
>
> "xvda" is not currently thought to be necessarily Linux-specific. See
> docs/misc/vbd-interface.text. Perhaps that needs to be revisited, but
> in general it's perfectly possible to pass "xvda" in a domain config
> file for a BSD guest, or whatever. And Linux writing "xvdfoo" in the
> config file doesn't always make Linux call the device /dev/xvdfoo.
Hrm, I guess. It certainly gives the impression of being Linux specific,
even if technically it isn't, since people associate xvd* with Linux
even if it happens to be valid and work elsewhere. I guess in reality
very few people, including BSD folks, are going to change this (because
it'll just work) so there's no chance of them getting confused.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 14:59 ` Ian Campbell
2012-05-04 16:46 ` Ian Jackson
@ 2012-05-14 13:48 ` Stefano Stabellini
2012-05-14 13:56 ` Ian Campbell
1 sibling, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 13:48 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Ian Jackson, Stefano Stabellini
On Fri, 4 May 2012, Ian Campbell wrote:
> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> >
> > Changes in v5:
> > - remove domid paramter to libxl__alloc_vdev (assume
> > LIBXL_TOOLSTACK_DOMID);
> > - remove scaling limit from libxl__alloc_vdev.
> >
> > Changes in v4:
> > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > - introduce upper bound for encode_disk_name;
> > - better error handling;
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> > tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> > tools/libxl/libxl_internal.h | 4 +++
> > tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> > tools/libxl/libxl_netbsd.c | 6 +++++
> > 4 files changed, 86 insertions(+), 0 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index bd5ebb3..7a1e017 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -480,6 +480,37 @@ out:
> > return rc;
> > }
> >
> > +/* libxl__alloc_vdev only works on the local domain, that is the domain
> > + * where the toolstack is running */
> > +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> > + xs_transaction_t t)
> > +{
> > + int devid = 0, disk = 0, part = 0;
> > + char *vdev = NULL;
> > + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > +
> > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
>
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn't at least this bit magically become portable to BSD etc too?
It cannot work on BSD for other reason, see below.
In any case blkdev_start can be anything that
libxl__device_disk_dev_number can parse, so d0p0 works too.
> Or couldn't it actually be an int and save you parsing at all?
Yes, it could. But isn't "xvda" much more intuitive?
> > + if (part != 0) {
> > + LOG(ERROR, "blkdev_start is invalid");
> > + return NULL;
> > + }
> > +
> > + do {
> > + vdev = GCSPRINTF("d%dp0", disk);
> > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
>
> libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
> without the hardcoded "p0"), I think.
In my tests it doesn't work properly using just d<N>.
> I'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
> since you don't use it again.
OK
> > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > index 9e0ed6d..dbf5f71 100644
> > --- a/tools/libxl/libxl_netbsd.c
> > +++ b/tools/libxl/libxl_netbsd.c
>
> Aha, here's where we need a BSD contribution. CC someone?
There is no working gntdev or QDISK on BSD at the moment.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-14 13:48 ` Stefano Stabellini
@ 2012-05-14 13:56 ` Ian Campbell
0 siblings, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-14 13:56 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Mon, 2012-05-14 at 14:48 +0100, Stefano Stabellini wrote:
> On Fri, 4 May 2012, Ian Campbell wrote:
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > > domain passed as argument.
> > >
> > > Changes in v5:
> > > - remove domid paramter to libxl__alloc_vdev (assume
> > > LIBXL_TOOLSTACK_DOMID);
> > > - remove scaling limit from libxl__alloc_vdev.
> > >
> > > Changes in v4:
> > > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > > - introduce upper bound for encode_disk_name;
> > > - better error handling;
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > > tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> > > tools/libxl/libxl_internal.h | 4 +++
> > > tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> > > tools/libxl/libxl_netbsd.c | 6 +++++
> > > 4 files changed, 86 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index bd5ebb3..7a1e017 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > > @@ -480,6 +480,37 @@ out:
> > > return rc;
> > > }
> > >
> > > +/* libxl__alloc_vdev only works on the local domain, that is the domain
> > > + * where the toolstack is running */
> > > +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> > > + xs_transaction_t t)
> > > +{
> > > + int devid = 0, disk = 0, part = 0;
> > > + char *vdev = NULL;
> > > + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > > +
> > > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
> >
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn't at least this bit magically become portable to BSD etc too?
>
> It cannot work on BSD for other reason, see below.
> In any case blkdev_start can be anything that
> libxl__device_disk_dev_number can parse, so d0p0 works too.
>
>
> > Or couldn't it actually be an int and save you parsing at all?
>
> Yes, it could. But isn't "xvda" much more intuitive?
To Linux users, yes. But on BSD the virt devices have a different naming
scheme, so xvda wouldn't necessarily make much sense to them.
However I agree with IanJ's comment that letting the user use a name is
friendlier than just a bare number.
Anyway, lets just leave it as "xvda", it's not a huge deal.
> > > + if (part != 0) {
> > > + LOG(ERROR, "blkdev_start is invalid");
> > > + return NULL;
> > > + }
> > > +
> > > + do {
> > > + vdev = GCSPRINTF("d%dp0", disk);
> > > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> >
> > libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
> > without the hardcoded "p0"), I think.
>
> In my tests it doesn't work properly using just d<N>.
I misread it, the sscanf in libxl__device_disk_dev_number needs to match
at least twice (so "dN" and "pM"). In theory that could be changed but
lets not do that now.
> > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > > index 9e0ed6d..dbf5f71 100644
> > > --- a/tools/libxl/libxl_netbsd.c
> > > +++ b/tools/libxl/libxl_netbsd.c
> >
> > Aha, here's where we need a BSD contribution. CC someone?
>
> There is no working gntdev or QDISK on BSD at the moment.
Oh, right, that does make it somewhat pointless...
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 11:13 ` [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
2012-05-04 14:59 ` Ian Campbell
@ 2012-05-04 16:39 ` Ian Jackson
2012-05-14 14:10 ` Stefano Stabellini
1 sibling, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:39 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...
> + do {
> + vdev = GCSPRINTF("d%dp0", disk);
> + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> + if (libxl__xs_read(gc, t,
> + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> + dompath, devid)) == NULL) {
> + if (errno == ENOENT)
> + return libxl__devid_to_localdev(gc, devid);
> + else
> + return NULL;
> + }
> + disk++;
> + } while (disk <= (1 << 20));
This should be "<", as disk==(1<<20) is too big.
And the magic number 20 should not be repeated.
But it turns out that you don't need to do this check here at all
because libxl__device_disk_dev_number will check this, correctly.
All you need to do is actually pay attention to its error return.
> + return NULL;
All the NULL error exits should log an error surely.
> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> + if (n >= 26)
> + ptr = encode_disk_name(ptr, n / 26 - 1);
> + *ptr = 'a' + n % 26;
> + return ptr + 1;
> +}
This still makes it difficult to see that the buffer cannot be
overrun. I mentioned this in response to a previous posting of this
code and IIRC you were going to do something about it, if only a
comment explaining what the maximum buffer length is and why it's
safe.
> + strcpy(ret, "xvd");
> + ptr = encode_disk_name(ret + 3, offset);
> + if (minor % nr_parts == 0)
> + *ptr = 0;
> + else
> + snprintf(ptr, ret + 32 - ptr,
> + "%d", minor & (nr_parts - 1));
You do not handle snprintf buffer truncation sensibly here. As it
happens I think this overflow cannot happen but this deserves a
comment at the very least, to explain the lack of error handling.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-04 16:39 ` Ian Jackson
@ 2012-05-14 14:10 ` Stefano Stabellini
2012-05-14 14:22 ` Ian Jackson
0 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:10 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Fri, 4 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > + do {
> > + vdev = GCSPRINTF("d%dp0", disk);
> > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > + if (libxl__xs_read(gc, t,
> > + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> > + dompath, devid)) == NULL) {
> > + if (errno == ENOENT)
> > + return libxl__devid_to_localdev(gc, devid);
> > + else
> > + return NULL;
> > + }
> > + disk++;
> > + } while (disk <= (1 << 20));
>
> This should be "<", as disk==(1<<20) is too big.
> And the magic number 20 should not be repeated.
>
> But it turns out that you don't need to do this check here at all
> because libxl__device_disk_dev_number will check this, correctly.
>
> All you need to do is actually pay attention to its error return.
OK
> > + return NULL;
>
> All the NULL error exits should log an error surely.
The error log is in the next patch. Should I add a second log here?
Or maybe I should move the error log from the caller to the callee?
> > +/* same as in Linux */
> > +static char *encode_disk_name(char *ptr, unsigned int n)
> > +{
> > + if (n >= 26)
> > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > + *ptr = 'a' + n % 26;
> > + return ptr + 1;
> > +}
>
> This still makes it difficult to see that the buffer cannot be
> overrun. I mentioned this in response to a previous posting of this
> code and IIRC you were going to do something about it, if only a
> comment explaining what the maximum buffer length is and why it's
> safe.
I am keen on keeping the code as is, because it is the same that is in
Linux (see comment above).
I'll add a comment.
> > + strcpy(ret, "xvd");
> > + ptr = encode_disk_name(ret + 3, offset);
> > + if (minor % nr_parts == 0)
> > + *ptr = 0;
> > + else
> > + snprintf(ptr, ret + 32 - ptr,
> > + "%d", minor & (nr_parts - 1));
>
> You do not handle snprintf buffer truncation sensibly here. As it
> happens I think this overflow cannot happen but this deserves a
> comment at the very least, to explain the lack of error handling.
Same here, I am keen on keeping the code as is, because it is the same
that is in Linux.
I'll add a comment.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-14 14:10 ` Stefano Stabellini
@ 2012-05-14 14:22 ` Ian Jackson
2012-05-14 14:43 ` Stefano Stabellini
0 siblings, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-14 14:22 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> On Fri, 4 May 2012, Ian Jackson wrote:
> > All you need to do is actually pay attention to its error return.
>
> OK
>
> > > + return NULL;
> >
> > All the NULL error exits should log an error surely.
>
> The error log is in the next patch. Should I add a second log here?
> Or maybe I should move the error log from the caller to the callee?
Well, my view is that a function should either always log if it
returns ERROR_FAIL (or NULL if that's its calling pattern), or never
do so. And if it logs this should be in a doc comment.
I inferred that the function was supposed to log by the fact that the
other error paths logged (that is, I didn't read the doc comment or
the rest of the code).
I don't mind where the logging is done (although normally it's best to
do it closer to the source of the error) but I do want to make sure
that we don't have cases where there is an exit path which simply
fails without explaining why.
> > > +/* same as in Linux */
> > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > +{
> > > + if (n >= 26)
> > > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > > + *ptr = 'a' + n % 26;
> > > + return ptr + 1;
> > > +}
> >
> > This still makes it difficult to see that the buffer cannot be
> > overrun. I mentioned this in response to a previous posting of this
> > code and IIRC you were going to do something about it, if only a
> > comment explaining what the maximum buffer length is and why it's
> > safe.
>
> I am keen on keeping the code as is, because it is the same that is in
> Linux (see comment above).
I read the comment as "this implements the same transformation as the
Linux kernel" not "this is the same code as actually used in the Linux
kernel". If you mean the latter, do say so.
Also, in that case why is the recursive function name, formatting,
etc. not the same as in Linux (as far as they can be) ? Surely if
Linux changes this we will want to change our code too and that will
be easiest if we can c&p without needing to reformat, rename, etc.
> I'll add a comment.
OK. The comments, taken together, should constitute proofs that the
code does not overrun any buffers.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev
2012-05-14 14:22 ` Ian Jackson
@ 2012-05-14 14:43 ` Stefano Stabellini
0 siblings, 0 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:43 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 14 May 2012, Ian Jackson wrote:
> > > > +/* same as in Linux */
> > > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > > +{
> > > > + if (n >= 26)
> > > > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > > > + *ptr = 'a' + n % 26;
> > > > + return ptr + 1;
> > > > +}
> > >
> > > This still makes it difficult to see that the buffer cannot be
> > > overrun. I mentioned this in response to a previous posting of this
> > > code and IIRC you were going to do something about it, if only a
> > > comment explaining what the maximum buffer length is and why it's
> > > safe.
> >
> > I am keen on keeping the code as is, because it is the same that is in
> > Linux (see comment above).
>
> I read the comment as "this implements the same transformation as the
> Linux kernel" not "this is the same code as actually used in the Linux
> kernel". If you mean the latter, do say so.
Yes, it is actually the same used in the Linux kernel.
> Also, in that case why is the recursive function name, formatting,
> etc. not the same as in Linux (as far as they can be) ? Surely if
> Linux changes this we will want to change our code too and that will
> be easiest if we can c&p without needing to reformat, rename, etc.
It is exactly the same code, except for the tab indentation that I
replaced with space indentation to follow the coding style.
> > I'll add a comment.
>
> OK. The comments, taken together, should constitute proofs that the
> code does not overrun any buffers.
The two comments refer to the upper bound that we introduced, thanks to
the limit there can be no buffer overruns.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (5 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 15:11 ` Ian Campbell
2012-05-04 16:42 ` Ian Jackson
2012-05-04 11:13 ` [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
2012-05-10 13:07 ` [PATCH v5 0/8] qdisk local attach Jan Beulich
8 siblings, 2 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
- Spawn a QEMU instance at boot time to handle disk local attach
requests.
- Implement libxl_device_disk_local_attach for QDISKs in terms of a
regular disk attach with the frontend and backend running in the same
domain.
Chanegs in v5:
- replace LIBXL__LOG with LOG.
Changes on v4:
- improve error handling and exit paths in libxl__device_disk_local_attach.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
tools/hotplug/Linux/init.d/xencommons | 3 +
tools/libxl/libxl_internal.c | 58 ++++++++++++++++++----
3 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons
index 6543204..0f3b9ad 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons
@@ -12,3 +12,6 @@
# Running xenbackendd in debug mode
#XENBACKENDD_DEBUG=[yes|on|1]
+
+# qemu path and log file
+#QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
index 2f81ea2..9dda6e2 100644
--- a/tools/hotplug/Linux/init.d/xencommons
+++ b/tools/hotplug/Linux/init.d/xencommons
@@ -104,6 +104,9 @@ do_start () {
xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
test "`uname`" != "NetBSD" || xenbackendd $XENBACKENDD_ARGS
+ echo Starting QEMU as disk backend for dom0
+ test -z "$QEMU_XEN" && QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
+ $QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize -monitor /dev/null
}
do_stop () {
echo Stopping xenconsoled
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 7a1e017..e180498 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -519,6 +519,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
libxl_ctx *ctx = gc->owner;
char *dev = NULL;
int rc;
+ xs_transaction_t t = XBT_NULL;
libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
memcpy(disk, in_disk, sizeof(libxl_device_disk));
@@ -561,13 +562,35 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
break;
case LIBXL_DISK_BACKEND_QDISK:
if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
- " attach a qdisk image if the format is not raw");
- break;
+ do {
+ t = xs_transaction_start(ctx->xsh);
+ if (t == XBT_NULL) {
+ LOG(ERROR, "failed to start a xenstore transaction");
+ goto out;
+ }
+ disk->vdev = libxl__alloc_vdev(gc, blkdev_start, t);
+ if (disk->vdev == NULL) {
+ LOG(ERROR, "libxl__alloc_vdev failed");
+ goto out;
+ }
+ if (libxl__device_disk_add(gc, LIBXL_TOOLSTACK_DOMID,
+ t, disk)) {
+ LOG(ERROR, "libxl_device_disk_add failed");
+ goto out;
+ }
+ rc = xs_transaction_end(ctx->xsh, t, 0);
+ } while (rc == 0 && errno == EAGAIN);
+ t = XBT_NULL;
+ if (rc == 0) {
+ LOGE(ERROR, "xenstore transaction failed");
+ goto out;
+ }
+ dev = GCSPRINTF("/dev/%s", disk->vdev);
+ } else {
+ dev = disk->pdev_path;
}
LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
- in_disk->pdev_path);
- dev = disk->pdev_path;
+ dev);
break;
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
@@ -576,18 +599,31 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
}
out:
+ if (t != XBT_NULL)
+ xs_transaction_end(ctx->xsh, t, 1);
*new_disk = disk;
return dev;
}
int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
{
- /* Nothing to do for PHYSTYPE_PHY. */
-
- /*
- * For other device types assume that the blktap2 process is
- * needed by the soon to be started domain and do nothing.
- */
+ switch (disk->backend) {
+ case LIBXL_DISK_BACKEND_QDISK:
+ if (disk->vdev != NULL) {
+ libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
+ disk, 0);
+ return libxl_device_disk_destroy(gc->owner,
+ LIBXL_TOOLSTACK_DOMID, disk);
+ }
+ break;
+ default:
+ /*
+ * Nothing to do for PHYSTYPE_PHY.
+ * For other device types assume that the blktap2 process is
+ * needed by the soon to be started domain and do nothing.
+ */
+ break;
+ }
return 0;
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-04 11:13 ` [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
@ 2012-05-04 15:11 ` Ian Campbell
2012-05-04 16:42 ` Ian Jackson
1 sibling, 0 replies; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 15:11 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> - Spawn a QEMU instance at boot time to handle disk local attach
> requests.
>
> - Implement libxl_device_disk_local_attach for QDISKs in terms of a
> regular disk attach with the frontend and backend running in the same
> domain.
>
> Chanegs in v5:
> - replace LIBXL__LOG with LOG.
>
> Changes on v4:
> - improve error handling and exit paths in libxl__device_disk_local_attach.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/hotplug/Linux/init.d/sysconfig.xencommons | 3 +
> tools/hotplug/Linux/init.d/xencommons | 3 +
> tools/libxl/libxl_internal.c | 58 ++++++++++++++++++----
> 3 files changed, 53 insertions(+), 11 deletions(-)
>
> diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons b/tools/hotplug/Linux/init.d/sysconfig.xencommons
> index 6543204..0f3b9ad 100644
> --- a/tools/hotplug/Linux/init.d/sysconfig.xencommons
> +++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons
> @@ -12,3 +12,6 @@
>
> # Running xenbackendd in debug mode
> #XENBACKENDD_DEBUG=[yes|on|1]
> +
> +# qemu path and log file
What does "and log file" mean?
Apart from that:
Acked-by:Ian Campbell <ian.campbell@citrix.com>
> +#QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
> diff --git a/tools/hotplug/Linux/init.d/xencommons b/tools/hotplug/Linux/init.d/xencommons
> index 2f81ea2..9dda6e2 100644
> --- a/tools/hotplug/Linux/init.d/xencommons
> +++ b/tools/hotplug/Linux/init.d/xencommons
> @@ -104,6 +104,9 @@ do_start () {
> xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
> test -z "$XENBACKENDD_DEBUG" || XENBACKENDD_ARGS="-d"
> test "`uname`" != "NetBSD" || xenbackendd $XENBACKENDD_ARGS
> + echo Starting QEMU as disk backend for dom0
> + test -z "$QEMU_XEN" && QEMU_XEN=/usr/lib/xen/bin/qemu-system-i386
> + $QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize -monitor /dev/null
> }
> do_stop () {
> echo Stopping xenconsoled
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 7a1e017..e180498 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -519,6 +519,7 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> libxl_ctx *ctx = gc->owner;
> char *dev = NULL;
> int rc;
> + xs_transaction_t t = XBT_NULL;
> libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
>
> memcpy(disk, in_disk, sizeof(libxl_device_disk));
> @@ -561,13 +562,35 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> break;
> case LIBXL_DISK_BACKEND_QDISK:
> if (disk->format != LIBXL_DISK_FORMAT_RAW) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally"
> - " attach a qdisk image if the format is not raw");
> - break;
> + do {
> + t = xs_transaction_start(ctx->xsh);
> + if (t == XBT_NULL) {
> + LOG(ERROR, "failed to start a xenstore transaction");
> + goto out;
> + }
> + disk->vdev = libxl__alloc_vdev(gc, blkdev_start, t);
> + if (disk->vdev == NULL) {
> + LOG(ERROR, "libxl__alloc_vdev failed");
> + goto out;
> + }
> + if (libxl__device_disk_add(gc, LIBXL_TOOLSTACK_DOMID,
> + t, disk)) {
> + LOG(ERROR, "libxl_device_disk_add failed");
> + goto out;
> + }
> + rc = xs_transaction_end(ctx->xsh, t, 0);
> + } while (rc == 0 && errno == EAGAIN);
> + t = XBT_NULL;
> + if (rc == 0) {
> + LOGE(ERROR, "xenstore transaction failed");
> + goto out;
> + }
> + dev = GCSPRINTF("/dev/%s", disk->vdev);
> + } else {
> + dev = disk->pdev_path;
> }
> LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> - in_disk->pdev_path);
> - dev = disk->pdev_path;
> + dev);
> break;
> default:
> LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend "
> @@ -576,18 +599,31 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> }
>
> out:
> + if (t != XBT_NULL)
> + xs_transaction_end(ctx->xsh, t, 1);
> *new_disk = disk;
> return dev;
> }
>
> int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
> {
> - /* Nothing to do for PHYSTYPE_PHY. */
> -
> - /*
> - * For other device types assume that the blktap2 process is
> - * needed by the soon to be started domain and do nothing.
> - */
> + switch (disk->backend) {
> + case LIBXL_DISK_BACKEND_QDISK:
> + if (disk->vdev != NULL) {
> + libxl_device_disk_remove(gc->owner, LIBXL_TOOLSTACK_DOMID,
> + disk, 0);
> + return libxl_device_disk_destroy(gc->owner,
> + LIBXL_TOOLSTACK_DOMID, disk);
> + }
> + break;
> + default:
> + /*
> + * Nothing to do for PHYSTYPE_PHY.
> + * For other device types assume that the blktap2 process is
> + * needed by the soon to be started domain and do nothing.
> + */
> + break;
> + }
>
> return 0;
> }
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-04 11:13 ` [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
2012-05-04 15:11 ` Ian Campbell
@ 2012-05-04 16:42 ` Ian Jackson
2012-05-14 14:13 ` Stefano Stabellini
1 sibling, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-04 16:42 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("[PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> - Spawn a QEMU instance at boot time to handle disk local attach
> requests.
...
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 7a1e017..e180498 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
...
> + rc = xs_transaction_end(ctx->xsh, t, 0);
> + } while (rc == 0 && errno == EAGAIN);
> + t = XBT_NULL;
> + if (rc == 0) {
> + LOGE(ERROR, "xenstore transaction failed");
> + goto out;
Maybe I'm being excessively picky, but I think "rc" should be used
only for a variable which contains libxl error codes. I had to do a
double-take when I saw
if (rc == 0) {
error handling;
since rc==0 is of course the success case.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-04 16:42 ` Ian Jackson
@ 2012-05-14 14:13 ` Stefano Stabellini
2012-05-14 14:23 ` Ian Jackson
0 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:13 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Fri, 4 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > - Spawn a QEMU instance at boot time to handle disk local attach
> > requests.
> ...
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index 7a1e017..e180498 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> ...
> > + rc = xs_transaction_end(ctx->xsh, t, 0);
> > + } while (rc == 0 && errno == EAGAIN);
> > + t = XBT_NULL;
> > + if (rc == 0) {
> > + LOGE(ERROR, "xenstore transaction failed");
> > + goto out;
>
> Maybe I'm being excessively picky, but I think "rc" should be used
> only for a variable which contains libxl error codes. I had to do a
> double-take when I saw
> if (rc == 0) {
> error handling;
> since rc==0 is of course the success case.
Should I s/rc/ret/ in this function?
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-14 14:13 ` Stefano Stabellini
@ 2012-05-14 14:23 ` Ian Jackson
2012-05-14 14:45 ` Stefano Stabellini
0 siblings, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-14 14:23 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> On Fri, 4 May 2012, Ian Jackson wrote:
> > Maybe I'm being excessively picky, but I think "rc" should be used
> > only for a variable which contains libxl error codes. I had to do a
> > double-take when I saw
> > if (rc == 0) {
> > error handling;
> > since rc==0 is of course the success case.
>
> Should I s/rc/ret/ in this function?
I think you should s/rc/<something else>/ when it's the return value
from xs_*. ret would do. If rc is used for a libxl error code it
should remain as rc.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-14 14:23 ` Ian Jackson
@ 2012-05-14 14:45 ` Stefano Stabellini
2012-05-14 14:46 ` Ian Jackson
0 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:45 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 14 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > On Fri, 4 May 2012, Ian Jackson wrote:
> > > Maybe I'm being excessively picky, but I think "rc" should be used
> > > only for a variable which contains libxl error codes. I had to do a
> > > double-take when I saw
> > > if (rc == 0) {
> > > error handling;
> > > since rc==0 is of course the success case.
> >
> > Should I s/rc/ret/ in this function?
>
> I think you should s/rc/<something else>/ when it's the return value
> from xs_*. ret would do. If rc is used for a libxl error code it
> should remain as rc.
rc is already used for a libxl error code in the same function.
Maybe I should just add ret in addition to rc?
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-14 14:45 ` Stefano Stabellini
@ 2012-05-14 14:46 ` Ian Jackson
2012-05-14 14:52 ` Stefano Stabellini
0 siblings, 1 reply; 62+ messages in thread
From: Ian Jackson @ 2012-05-14 14:46 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Campbell
Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> On Mon, 14 May 2012, Ian Jackson wrote:
> > I think you should s/rc/<something else>/ when it's the return value
> > from xs_*. ret would do. If rc is used for a libxl error code it
> > should remain as rc.
>
> rc is already used for a libxl error code in the same function.
> Maybe I should just add ret in addition to rc?
I think that would be better. Do you agree ?
As I say this seems like quite a small nit I'm picking.
Ian.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach
2012-05-14 14:46 ` Ian Jackson
@ 2012-05-14 14:52 ` Stefano Stabellini
0 siblings, 0 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:52 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 14 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach"):
> > On Mon, 14 May 2012, Ian Jackson wrote:
> > > I think you should s/rc/<something else>/ when it's the return value
> > > from xs_*. ret would do. If rc is used for a libxl error code it
> > > should remain as rc.
> >
> > rc is already used for a libxl error code in the same function.
> > Maybe I should just add ret in addition to rc?
>
> I think that would be better. Do you agree ?
In general I try to make my code as easy to read as possible. A new
integer is a small price to pay for it.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected"
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (6 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 7/8] xl/libxl: implement QDISK libxl_device_disk_local_attach Stefano Stabellini
@ 2012-05-04 11:13 ` Stefano Stabellini
2012-05-04 15:13 ` Ian Campbell
2012-05-10 13:07 ` [PATCH v5 0/8] qdisk local attach Jan Beulich
8 siblings, 1 reply; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-04 11:13 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Jackson, Ian.Campbell, Stefano Stabellini
In order to make sure that the block device is available and ready to be
used, wait for state "connected" in the backend before returning.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Changes in v5:
- unify error paths.
Changes in v4:
- improve exit paths.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxl/libxl_internal.c | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index e180498..05faff5 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -517,8 +517,9 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
const char *blkdev_start)
{
libxl_ctx *ctx = gc->owner;
- char *dev = NULL;
+ char *dev = NULL, *be_path = NULL;
int rc;
+ libxl__device device;
xs_transaction_t t = XBT_NULL;
libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
@@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
break;
}
+ if (disk->vdev != NULL) {
+ rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
+ if (rc < 0)
+ goto out;
+ be_path = libxl__device_backend_path(gc, &device);
+ rc = libxl__wait_for_backend(gc, be_path, "4");
+ if (rc < 0)
+ goto out;
+ }
+
+ *new_disk = disk;
+ return dev;
out:
if (t != XBT_NULL)
xs_transaction_end(ctx->xsh, t, 1);
- *new_disk = disk;
- return dev;
+ else
+ libxl__device_disk_local_detach(gc, disk);
+ return NULL;
}
int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected"
2012-05-04 11:13 ` [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
@ 2012-05-04 15:13 ` Ian Campbell
2012-05-14 14:16 ` Stefano Stabellini
0 siblings, 1 reply; 62+ messages in thread
From: Ian Campbell @ 2012-05-04 15:13 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Ian Jackson
On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> In order to make sure that the block device is available and ready to be
> used, wait for state "connected" in the backend before returning.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Changes in v5:
> - unify error paths.
>
> Changes in v4:
> - improve exit paths.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> tools/libxl/libxl_internal.c | 20 +++++++++++++++++---
> 1 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index e180498..05faff5 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -517,8 +517,9 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> const char *blkdev_start)
> {
> libxl_ctx *ctx = gc->owner;
> - char *dev = NULL;
> + char *dev = NULL, *be_path = NULL;
> int rc;
> + libxl__device device;
> xs_transaction_t t = XBT_NULL;
> libxl_device_disk *disk = libxl__zalloc(gc, sizeof(libxl_device_disk));
>
> @@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> break;
> }
>
> + if (disk->vdev != NULL) {
> + rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
> + if (rc < 0)
> + goto out;
> + be_path = libxl__device_backend_path(gc, &device);
> + rc = libxl__wait_for_backend(gc, be_path, "4");
> + if (rc < 0)
> + goto out;
> + }
> +
> + *new_disk = disk;
> + return dev;
> out:
> if (t != XBT_NULL)
> xs_transaction_end(ctx->xsh, t, 1);
There's no way to reach the preceding "return dev" with the transaction
still open? Previously we would have fallen through and done it.
> - *new_disk = disk;
> - return dev;
> + else
> + libxl__device_disk_local_detach(gc, disk);
> + return NULL;
> }
>
> int libxl__device_disk_local_detach(libxl__gc *gc, libxl_device_disk *disk)
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected"
2012-05-04 15:13 ` Ian Campbell
@ 2012-05-14 14:16 ` Stefano Stabellini
0 siblings, 0 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-14 14:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Ian Jackson, Stefano Stabellini
On Fri, 4 May 2012, Ian Campbell wrote:
> > @@ -598,11 +599,24 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
> > break;
> > }
> >
> > + if (disk->vdev != NULL) {
> > + rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID, disk, &device);
> > + if (rc < 0)
> > + goto out;
> > + be_path = libxl__device_backend_path(gc, &device);
> > + rc = libxl__wait_for_backend(gc, be_path, "4");
> > + if (rc < 0)
> > + goto out;
> > + }
> > +
> > + *new_disk = disk;
> > + return dev;
> > out:
> > if (t != XBT_NULL)
> > xs_transaction_end(ctx->xsh, t, 1);
>
> There's no way to reach the preceding "return dev" with the transaction
> still open? Previously we would have fallen through and done it.
Yes, there is no way.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 0/8] qdisk local attach
2012-05-04 11:12 [PATCH v5 0/8] qdisk local attach Stefano Stabellini
` (7 preceding siblings ...)
2012-05-04 11:13 ` [PATCH v5 8/8] libxl__device_disk_local_attach: wait for state "connected" Stefano Stabellini
@ 2012-05-10 13:07 ` Jan Beulich
2012-05-11 15:57 ` Stefano Stabellini
8 siblings, 1 reply; 62+ messages in thread
From: Jan Beulich @ 2012-05-10 13:07 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Ian Jackson, Ian Campbell, xen-devel
>>> On 04.05.12 at 13:12, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> Hi all,
> this patch implements local_attach support for QDISK: that means that
> you can use qcow2 as disk format for your PV guests disks and use
> pygrub to retrieve the kernel and initrd in dom0.
>
> The idea is that we start a QEMU instance at boot time to listen to
> local_attach requests. When libxl_device_disk_local_attach is called on
> a QDISK images, libxl sets up a backend/frontend pair in dom0 for the disk
> so that blkfront in dom0 will create a new xvd device for it.
> Then pygrub can be pointed at this device to retrieve kernel and
> initrd.
So I pulled this into my local tree to be able to debug problems in
our gntdev driver in a reasonably isolated way (i.e. not needing
to fire up guests). While block-attach works fine, block-detach
doesn't at all. This may be related to me using the deprecated
file:/ form of specifying the source during attach, as all of the
supposedly modern forms I tried weren't accepted by the parser.
Jan
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 0/8] qdisk local attach
2012-05-10 13:07 ` [PATCH v5 0/8] qdisk local attach Jan Beulich
@ 2012-05-11 15:57 ` Stefano Stabellini
0 siblings, 0 replies; 62+ messages in thread
From: Stefano Stabellini @ 2012-05-11 15:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini
On Thu, 10 May 2012, Jan Beulich wrote:
> >>> On 04.05.12 at 13:12, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > Hi all,
> > this patch implements local_attach support for QDISK: that means that
> > you can use qcow2 as disk format for your PV guests disks and use
> > pygrub to retrieve the kernel and initrd in dom0.
> >
> > The idea is that we start a QEMU instance at boot time to listen to
> > local_attach requests. When libxl_device_disk_local_attach is called on
> > a QDISK images, libxl sets up a backend/frontend pair in dom0 for the disk
> > so that blkfront in dom0 will create a new xvd device for it.
> > Then pygrub can be pointed at this device to retrieve kernel and
> > initrd.
>
> So I pulled this into my local tree to be able to debug problems in
> our gntdev driver in a reasonably isolated way (i.e. not needing
> to fire up guests). While block-attach works fine, block-detach
> doesn't at all. This may be related to me using the deprecated
> file:/ form of specifying the source during attach, as all of the
> supposedly modern forms I tried weren't accepted by the parser.
Block-detach needs another small patch to libxl in order to work.
I'll add it as part of the next local_attach patch series.
^ permalink raw reply [flat|nested] 62+ messages in thread