Ian Jackson
2011-Jul-06 15:07 UTC
[Xen-devel] [PATCH] libxl: sane disk backend selection and validation
Introduce a new function libxl__device_disk_set_backend which
does some sanity checks and determines which backend ought to be used.
If the caller specifies LIBXL_DISK_BACKEND_UNKNOWN (which has the
value 0), it tries PHY, TAP and QDISK in that order. Otherwise it
tries only the specified value.
libxl__device_disk_set_backend (and its helper function
disk_try_backend) inherit the role (and small amounts of the code)
from validate_virtual_disk. This is called during do_domain_create
and also from libxl_disk_device_add (for the benefit of hotplug
devices).
It also now takes over the role of the scattered fragments of backend
selection found in libxl_device_disk_add,
libxl_device_disk_local_attach and libxl__need_xenpv_qemu. These
latter functions now simply do the job for the backend they find has
already been specified and checked.
The restrictions on the capabilities of each backend, as expressed in
disk_try_backend (and to an extent in libxl_device_disk_local_attach)
are intended to be identical to the previous arrangements.
In 23618:3173b68c8a94 combined with 23622:160f7f39841b,
23623:c7180c353eb2, "xl" effectively became much more likely to select
TAP as the backend. With this change to libxl the default backend
selected by the libxl__device_disk_set_backend is intended to once
again to be PHY where possible.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 7e4404a8f5f9 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Mon Jul 04 07:57:32 2011 +0100
+++ b/tools/libxl/libxl.c Wed Jul 06 16:06:32 2011 +0100
@@ -902,49 +902,6 @@ int libxl_vncviewer_exec(libxl_ctx *ctx,
/******************************************************************************/
-static int validate_virtual_disk(libxl__gc *gc, char *file_name,
- libxl_device_disk *disk)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- struct stat stat_buf;
- char *delimiter;
-
- if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
- if (disk->is_cdrom)
- return 0;
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Empty disk %s is not a CDROM
device\n",
- disk->vdev);
- return ERROR_INVAL;
- }
-
- if (disk->format == LIBXL_DISK_FORMAT_RAW) {
- delimiter = strchr(file_name, '':'');
- if (delimiter) {
- if (!strncmp(file_name, "vhd:",
sizeof("vhd:")-1)) {
- disk->format = LIBXL_DISK_FORMAT_VHD;
- file_name = ++delimiter;
- }
- }
- }
-
- if ( stat(file_name, &stat_buf) != 0 ) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s",
file_name);
- return ERROR_INVAL;
- }
- if (disk->backend == LIBXL_DISK_BACKEND_PHY) {
- if ( !(S_ISBLK(stat_buf.st_mode)) ) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a
block device!\n",
- file_name);
- return ERROR_INVAL;
- }
- } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is
0!\n", file_name);
- return ERROR_INVAL;
- }
-
- return 0;
-}
-
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk
*disk)
{
libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -954,10 +911,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
int devid;
libxl__device device;
int major, minor, rc;
-
- rc = validate_virtual_disk(&gc, disk->pdev_path, disk);
- if (rc)
- return rc;
front = flexarray_make(16, 1);
if (!front) {
@@ -990,34 +943,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
device.devid = devid;
device.domid = domid;
device.kind = DEVICE_VBD;
-
-
- /*
- * Fixing the incoming backend type to try to decide on which
- * backend to use. Unfortunately at the moment this code is
- * utterly broken, but it more or less works.
- */
-
- /*
- * Backend type UNKNOWN should mean "caller does not want to
specify",
- * not "break pointlessely". (Callers should not be required to
- * specify the backend if they don''t want to.)
- */
- if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
- disk->backend = LIBXL_DISK_BACKEND_TAP;
-
- /* If blktap is not available then fallback to qdisk */
- if (disk->backend == LIBXL_DISK_BACKEND_TAP &&
!libxl__blktap_enabled(&gc))
- disk->backend = LIBXL_DISK_BACKEND_QDISK;
-
- /*
- * blktap cannot handle empty disks (aka cdroms). Fall back to
- * qdisk because qemu-xen creates the disk based on the xenstore
- * entries.
- */
- if (disk->backend == LIBXL_DISK_BACKEND_TAP &&
- disk->format == LIBXL_DISK_FORMAT_EMPTY)
- disk->backend == LIBXL_DISK_BACKEND_QDISK;
switch (disk->backend) {
case LIBXL_DISK_BACKEND_PHY:
@@ -1138,68 +1063,49 @@ char * libxl_device_disk_local_attach(li
libxl__gc gc = LIBXL_INIT_GC(ctx);
char *dev = NULL;
char *ret = NULL;
+ int rc;
- /*
- * Fixing the incoming backend type to try to decide on which
- * backend to use. Unfortunately at the moment this code is
- * utterly broken, but it more or less works.
- */
-
- /*
- * Backend type UNKNOWN should mean "caller does not want to
specify",
- * not "break pointlessely". (Callers should not be required to
- * specify the backend if they don''t want to.)
- */
- if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
- disk->backend = LIBXL_DISK_BACKEND_TAP;
+ rc = libxl__device_disk_set_backend(&gc, disk, 0);
+ if (rc) goto out;
switch (disk->backend) {
case LIBXL_DISK_BACKEND_PHY:
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "physical block device
must"
- " be raw");
- break;
- }
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching PHY disk %s to
domain 0",
- disk->pdev_path);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
%s",
+ disk->pdev_path);
dev = disk->pdev_path;
break;
case LIBXL_DISK_BACKEND_TAP:
- if (disk->format == LIBXL_DISK_FORMAT_VHD ||
- disk->format == LIBXL_DISK_FORMAT_RAW)
- {
- if (libxl__blktap_enabled(&gc))
- dev = libxl__blktap_devpath(&gc, disk->pdev_path,
disk->format);
- else {
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is
required"
- " to open a vhd disk");
- break;
- } else {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching tap
disk %s to domain 0",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- }
- }
+ 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;
- } else if (disk->format == LIBXL_DISK_FORMAT_QCOW ||
- disk->format == LIBXL_DISK_FORMAT_QCOW2) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a
qcow or qcow2 disk image");
+ case LIBXL_DISK_FORMAT_VHD:
+ dev = libxl__blktap_devpath(&gc, disk->pdev_path,
+ disk->format);
break;
- } else {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
- "type: %d", disk->backend);
+ case LIBXL_DISK_FORMAT_QCOW:
+ case LIBXL_DISK_FORMAT_QCOW2:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally
attach"
+ " a qcow or qcow2 disk image");
+ break;
+ 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");
+ 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, "attaching qdisk %s to
domain 0\n",
- disk->pdev_path);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s\n",
+ disk->pdev_path);
dev = disk->pdev_path;
break;
default:
@@ -1208,6 +1114,7 @@ char * libxl_device_disk_local_attach(li
break;
}
+ out:
if (dev != NULL)
ret = strdup(dev);
libxl__free_all(&gc);
diff -r 7e4404a8f5f9 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c Mon Jul 04 07:57:32 2011 +0100
+++ b/tools/libxl/libxl_create.c Wed Jul 06 16:06:32 2011 +0100
@@ -424,6 +424,13 @@ static int do_domain_create(libxl__gc *g
goto error_out;
}
+
+ for (i = 0; i < d_config->num_disks; i++) {
+ ret = libxl__device_disk_set_backend(gc, &d_config->disks[i],
+ dm_info);
+ if (ret) goto error_out;
+ }
+
if ( restore_fd < 0 ) {
ret = libxl_run_bootloader(ctx, &d_config->b_info,
d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
if (ret) {
diff -r 7e4404a8f5f9 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Mon Jul 04 07:57:32 2011 +0100
+++ b/tools/libxl/libxl_device.c Wed Jul 06 16:06:32 2011 +0100
@@ -116,6 +116,140 @@ retry_transaction:
rc = 0;
out:
return rc;
+}
+
+typedef struct {
+ libxl__gc *gc;
+ libxl_device_disk *disk;
+ const libxl_device_model_info *dm_info;
+ struct stat stab;
+} disk_try_backend_args;
+
+static int disk_try_backend(disk_try_backend_args *a,
+ libxl_disk_backend backend) {
+ /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or
+ * backend on success */
+ libxl_ctx *ctx = libxl__gc_owner(a->gc);
+ switch (backend) {
+
+ case LIBXL_DISK_BACKEND_PHY:
+ if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+ a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
phy"
+ " unsuitable due to format %s",
+ a->disk->vdev,
+
libxl__device_disk_string_of_format(a->disk->format));
+ return 0;
+ }
+ if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
+ !S_ISBLK(a->stab.st_mode)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
phy"
+ " unsuitable as phys path not a block device",
+ a->disk->vdev);
+ return ERROR_INVAL;
+ }
+
+ return backend;
+
+ case LIBXL_DISK_BACKEND_TAP:
+ if (!libxl__blktap_enabled(a->gc)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
tap"
+ " unsuitable because blktap not available",
+ a->disk->vdev);
+ return 0;
+ }
+ if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY ||
+ (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
tap"
+ " unsuitable because empty devices not
supported",
+ a->disk->vdev);
+ return 0;
+ }
+ return backend;
+
+ case LIBXL_DISK_BACKEND_QDISK:
+ if (!a->dm_info) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
qdisk"
+ " unsuitable for dynamic attach",
+ a->disk->vdev);
+ return 0;
+ }
+ switch (a->dm_info->device_model_version) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
qdisk"
+ " unsuitable because using old qemu",
+ a->disk->vdev);
+ return 0;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ return backend;
+ }
+ abort();
+
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend "
+ " %d unknown", a->disk->vdev, backend);
+ return 0;
+
+ }
+}
+
+int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk,
+ const libxl_device_model_info *dm_info) {
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl_disk_backend ok;
+ disk_try_backend_args a;
+
+ a.gc = gc;
+ a.disk = disk;
+ a.dm_info = dm_info;
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s spec.backend=%s",
+ disk->vdev,
+ libxl__device_disk_string_of_backend(disk->backend));
+
+ if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
+ if (!disk->is_cdrom) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
+ " but not cdrom",
+ disk->vdev);
+ return ERROR_INVAL;
+ }
+ memset(&a.stab, 0, sizeof(a.stab));
+ } else {
+ if (stat(disk->pdev_path, &a.stab)) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+ "failed to stat: %s",
+ disk->vdev, disk->pdev_path);
+ return ERROR_INVAL;
+ }
+ if (!S_ISBLK(a.stab.st_mode) &
+ !S_ISREG(a.stab.st_mode)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+ "phys path is not a block dev or file:
%s",
+ disk->vdev, disk->pdev_path);
+ return ERROR_INVAL;
+ }
+ }
+
+ if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) {
+ ok= disk_try_backend(&a, disk->backend);
+ } else {
+ ok+ disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?:
+ disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?:
+ disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK);
+ if (ok)
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend
%s",
+ disk->vdev,
+ libxl__device_disk_string_of_backend(ok));
+ }
+ if (!ok) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "no suitable backend for disk
%s",
+ disk->vdev);
+ return ERROR_INVAL;
+ }
+ disk->backend = ok;
+ return 0;
}
char *libxl__device_disk_string_of_format(libxl_disk_format format)
diff -r 7e4404a8f5f9 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c Mon Jul 04 07:57:32 2011 +0100
+++ b/tools/libxl/libxl_dm.c Wed Jul 06 16:06:32 2011 +0100
@@ -1001,25 +1001,18 @@ int libxl__need_xenpv_qemu(libxl__gc *gc
}
if (nr_disks > 0) {
- int blktap_enabled = -1;
for (i = 0; i < nr_disks; i++) {
switch (disks[i].backend) {
- case LIBXL_DISK_BACKEND_TAP:
- if (blktap_enabled == -1)
- blktap_enabled = libxl__blktap_enabled(gc);
- if (!blktap_enabled) {
- ret = 1;
- goto out;
- }
- break;
-
case LIBXL_DISK_BACKEND_QDISK:
ret = 1;
goto out;
+ case LIBXL_DISK_BACKEND_TAP:
case LIBXL_DISK_BACKEND_PHY:
- case LIBXL_DISK_BACKEND_UNKNOWN:
break;
+
+ case LIBXL_DISK_FORMAT_UNKNOWN:
+ abort(); /* impossible due to libxl__device_disk_set_backend */
}
}
}
diff -r 7e4404a8f5f9 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Mon Jul 04 07:57:32 2011 +0100
+++ b/tools/libxl/libxl_internal.h Wed Jul 06 16:06:32 2011 +0100
@@ -205,6 +205,8 @@ _hidden void libxl__userdata_destroyall(
/* from xl_device */
_hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
_hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
+_hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk *disk,
+ const libxl_device_model_info *dm_info);
_hidden int libxl__device_physdisk_major_minor(const char *physpath, int
*major, int *minor);
_hidden int libxl__device_disk_dev_number(const char *virtpath,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jul-06 15:49 UTC
Re: [Xen-devel] [PATCH] libxl: sane disk backend selection and validation
On Wed, 2011-07-06 at 16:07 +0100, Ian Jackson wrote:> Introduce a new function libxl__device_disk_set_backend which > does some sanity checks and determines which backend ought to be used. > > If the caller specifies LIBXL_DISK_BACKEND_UNKNOWN (which has the > value 0), it tries PHY, TAP and QDISK in that order. Otherwise it > tries only the specified value. > > libxl__device_disk_set_backend (and its helper function > disk_try_backend) inherit the role (and small amounts of the code) > from validate_virtual_disk. This is called during do_domain_create > and also from libxl_disk_device_add (for the benefit of hotplug > devices).do_domain_create adds disks using libxl_disk_device_add, so is it really needed in both?> > It also now takes over the role of the scattered fragments of backend > selection found in libxl_device_disk_add, > libxl_device_disk_local_attach and libxl__need_xenpv_qemu. These > latter functions now simply do the job for the backend they find has > already been specified and checked. > > The restrictions on the capabilities of each backend, as expressed in > disk_try_backend (and to an extent in libxl_device_disk_local_attach) > are intended to be identical to the previous arrangements. > > In 23618:3173b68c8a94 combined with 23622:160f7f39841b, > 23623:c7180c353eb2, "xl" effectively became much more likely to select > TAP as the backend. With this change to libxl the default backend > selected by the libxl__device_disk_set_backend is intended to once > again to be PHY where possible. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>> diff -r 7e4404a8f5f9 tools/libxl/libxl_create.c > --- a/tools/libxl/libxl_create.c Mon Jul 04 07:57:32 2011 +0100 > +++ b/tools/libxl/libxl_create.c Wed Jul 06 16:06:32 2011 +0100 > @@ -424,6 +424,13 @@ static int do_domain_create(libxl__gc *g > goto error_out; > } > > + > + for (i = 0; i < d_config->num_disks; i++) { > + ret = libxl__device_disk_set_backend(gc, &d_config->disks[i], > + dm_info); > + if (ret) goto error_out; > + }I presume this happens too late for the locally attached case since you call it there too?> + > if ( restore_fd < 0 ) { > ret = libxl_run_bootloader(ctx, &d_config->b_info, d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid); > if (ret) { > diff -r 7e4404a8f5f9 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Mon Jul 04 07:57:32 2011 +0100 > +++ b/tools/libxl/libxl_device.c Wed Jul 06 16:06:32 2011 +0100 > @@ -116,6 +116,140 @@ retry_transaction: > rc = 0; > out: > return rc; > +} > + > +typedef struct { > + libxl__gc *gc; > + libxl_device_disk *disk; > + const libxl_device_model_info *dm_info; > + struct stat stab; > +} disk_try_backend_args;The struct seems a little like overkill compared with just passing the arguments but nevermind.> + > +static int disk_try_backend(disk_try_backend_args *a, > + libxl_disk_backend backend) { > + /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or > + * backend on success */ > + libxl_ctx *ctx = libxl__gc_owner(a->gc); > + switch (backend) { > + > + case LIBXL_DISK_BACKEND_PHY: > + if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW || > + a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy" > + " unsuitable due to format %s", > + a->disk->vdev, > + libxl__device_disk_string_of_format(a->disk->format)); > + return 0; > + } > + if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY && > + !S_ISBLK(a->stab.st_mode)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy" > + " unsuitable as phys path not a block device", > + a->disk->vdev); > + return ERROR_INVAL;Can phy devices really be empty? Or must they refer to a block device (which may itself be an empty CD-ROM device but that''s not the same)?> + } > + > + return backend; > + > + case LIBXL_DISK_BACKEND_TAP: > + if (!libxl__blktap_enabled(a->gc)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap" > + " unsuitable because blktap not available", > + a->disk->vdev); > + return 0; > + } > + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY || > + (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap" > + " unsuitable because empty devices not supported", > + a->disk->vdev); > + return 0; > + } > + return backend; > + > + case LIBXL_DISK_BACKEND_QDISK: > + if (!a->dm_info) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend qdisk" > + " unsuitable for dynamic attach", > + a->disk->vdev); > + return 0; > + } > + switch (a->dm_info->device_model_version) { > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend qdisk" > + " unsuitable because using old qemu", > + a->disk->vdev);Wasn''t that support backported at one point?> + return 0; > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > + return backend; > + } > + abort(); > + > + default: > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend " > + " %d unknown", a->disk->vdev, backend); > + return 0; > + > + } > +} > + > +int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk, > + const libxl_device_model_info *dm_info) { > + libxl_ctx *ctx = libxl__gc_owner(gc); > + libxl_disk_backend ok; > + disk_try_backend_args a; > + > + a.gc = gc; > + a.disk = disk; > + a.dm_info = dm_info; > + > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s spec.backend=%s", > + disk->vdev, > + libxl__device_disk_string_of_backend(disk->backend));This (and other similar) could be libxl_disk_backend_to_string since 23568:d67133289286. In fact the original could possibly be nuked? (or is it used to lookups the xenstore name in some cases which may not match the enum name?).> + > + if (disk->format == LIBXL_DISK_FORMAT_EMPTY) { > + if (!disk->is_cdrom) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty" > + " but not cdrom", > + disk->vdev); > + return ERROR_INVAL; > + } > + memset(&a.stab, 0, sizeof(a.stab)); > + } else { > + if (stat(disk->pdev_path, &a.stab)) { > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s " > + "failed to stat: %s", > + disk->vdev, disk->pdev_path); > + return ERROR_INVAL; > + } > + if (!S_ISBLK(a.stab.st_mode) & > + !S_ISREG(a.stab.st_mode)) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s " > + "phys path is not a block dev or file: %s", > + disk->vdev, disk->pdev_path); > + return ERROR_INVAL; > + } > + } > + > + if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) { > + ok= disk_try_backend(&a, disk->backend); > + } else { > + ok> + disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?: > + disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?: > + disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK); > + if (ok) > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend %s", > + disk->vdev, > + libxl__device_disk_string_of_backend(ok)); > + } > + if (!ok) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "no suitable backend for disk %s", > + disk->vdev); > + return ERROR_INVAL; > + } > + disk->backend = ok; > + return 0; > } > > char *libxl__device_disk_string_of_format(libxl_disk_format format) > diff -r 7e4404a8f5f9 tools/libxl/libxl_dm.c > --- a/tools/libxl/libxl_dm.c Mon Jul 04 07:57:32 2011 +0100 > +++ b/tools/libxl/libxl_dm.c Wed Jul 06 16:06:32 2011 +0100 > @@ -1001,25 +1001,18 @@ int libxl__need_xenpv_qemu(libxl__gc *gc > } > > if (nr_disks > 0) { > - int blktap_enabled = -1; > for (i = 0; i < nr_disks; i++) { > switch (disks[i].backend) { > - case LIBXL_DISK_BACKEND_TAP: > - if (blktap_enabled == -1) > - blktap_enabled = libxl__blktap_enabled(gc); > - if (!blktap_enabled) { > - ret = 1; > - goto out; > - } > - break; > - > case LIBXL_DISK_BACKEND_QDISK: > ret = 1; > goto out; > > + case LIBXL_DISK_BACKEND_TAP: > case LIBXL_DISK_BACKEND_PHY: > - case LIBXL_DISK_BACKEND_UNKNOWN: > break; > + > + case LIBXL_DISK_FORMAT_UNKNOWN: > + abort(); /* impossible due to libxl__device_disk_set_backend */This switch could become a simple if (x == .... QDISK) now...> } > } > } > diff -r 7e4404a8f5f9 tools/libxl/libxl_internal.h > --- a/tools/libxl/libxl_internal.h Mon Jul 04 07:57:32 2011 +0100 > +++ b/tools/libxl/libxl_internal.h Wed Jul 06 16:06:32 2011 +0100 > @@ -205,6 +205,8 @@ _hidden void libxl__userdata_destroyall( > /* from xl_device */ > _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend); > _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format); > +_hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk *disk, > + const libxl_device_model_info *dm_info); > > _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor); > _hidden int libxl__device_disk_dev_number(const char *virtpath, > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jul-06 15:57 UTC
Re: [Xen-devel] [PATCH] libxl: sane disk backend selection and validation
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: sane disk backend
selection and validation"):> On Wed, 2011-07-06 at 16:07 +0100, Ian Jackson wrote:
> > libxl__device_disk_set_backend (and its helper function
> > disk_try_backend) inherit the role (and small amounts of the code)
> > from validate_virtual_disk. This is called during do_domain_create
> > and also from libxl_disk_device_add (for the benefit of hotplug
> > devices).
>
> do_domain_create adds disks using libxl_disk_device_add, so is it really
> needed in both?
Yes, because libxl_disk_device_add is called too late in
do_domain_create for (eg) use for pygrub.
libxl__device_disk_set_backend is idempotent.
> > + for (i = 0; i < d_config->num_disks; i++) {
> > + ret = libxl__device_disk_set_backend(gc,
&d_config->disks[i],
> > + dm_info);
> > + if (ret) goto error_out;
> > + }
>
> I presume this happens too late for the locally attached case since you
> call it there too?
Right.
> > +typedef struct {
> > + libxl__gc *gc;
> > + libxl_device_disk *disk;
> > + const libxl_device_model_info *dm_info;
> > + struct stat stab;
> > +} disk_try_backend_args;
>
> The struct seems a little like overkill compared with just passing the
> arguments but nevermind.
Otherwise I''d want a macro to make the list of backends to try not
involve linewrapping, or perhaps a const array of backends, or
something.
> > + if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY
&&
> > + !S_ISBLK(a->stab.st_mode)) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s,
backend phy"
> > + " unsuitable as phys path not a block
device",
> > + a->disk->vdev);
> > + return ERROR_INVAL;
>
> Can phy devices really be empty? Or must they refer to a block device
> (which may itself be an empty CD-ROM device but that''s not the
same)?
I don''t see any reason why a phy device shouldn''t be empty,
but I
haven''t tested it.
> > + switch (a->dm_info->device_model_version) {
> > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s,
backend qdisk"
> > + " unsuitable because using old
qemu",
> > + a->disk->vdev);
>
> Wasn''t that support backported at one point?
/me looks.
Oh yes, so it was. I guess this whole thing should go then, including
the plumbing through of dm_info which is then unnecessary.
> > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s
spec.backend=%s",
> > + disk->vdev,
> > +
libxl__device_disk_string_of_backend(disk->backend));
>
> This (and other similar) could be libxl_disk_backend_to_string since
> 23568:d67133289286. In fact the original could possibly be nuked? (or is
> it used to lookups the xenstore name in some cases which may not match
> the enum name?).
I think you are right and this should be changed here. The only
remaining caller of libxl__device_disk_string_of_backend is for the
xenstore write. And indeed the current thing is wrong since it
returns "phy".
> > case LIBXL_DISK_BACKEND_QDISK:
> > ret = 1;
> > goto out;
> >
> > + case LIBXL_DISK_BACKEND_TAP:
> > case LIBXL_DISK_BACKEND_PHY:
> > - case LIBXL_DISK_BACKEND_UNKNOWN:
> > break;
> > +
> > + case LIBXL_DISK_FORMAT_UNKNOWN:
> > + abort(); /* impossible due to
libxl__device_disk_set_backend */
>
> This switch could become a simple if (x == .... QDISK) now...
Yes.
Updated patch shortly.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jul-06 16:06 UTC
[Xen-devel] [PATCH, v2] libxl: sane disk backend selection and validation
Introduce a new function libxl__device_disk_set_backend which
does some sanity checks and determines which backend ought to be used.
If the caller specifies LIBXL_DISK_BACKEND_UNKNOWN (which has the
value 0), it tries PHY, TAP and QDISK in that order. Otherwise it
tries only the specified value.
libxl__device_disk_set_backend (and its helper function
disk_try_backend) inherit the role (and small amounts of the code)
from validate_virtual_disk. This is called during do_domain_create
and also from libxl_disk_device_add (for the benefit of hotplug
devices).
It also now takes over the role of the scattered fragments of backend
selection found in libxl_device_disk_add,
libxl_device_disk_local_attach and libxl__need_xenpv_qemu. These
latter functions now simply do the job for the backend they find has
already been specified and checked.
The restrictions on the capabilities of each backend, as expressed in
disk_try_backend (and to an extent in libxl_device_disk_local_attach)
are intended to be identical to the previous arrangements.
In 23618:3173b68c8a94 combined with 23622:160f7f39841b,
23623:c7180c353eb2, "xl" effectively became much more likely to select
TAP as the backend. With this change to libxl the default backend
selected by the libxl__device_disk_set_backend is intended to once
again to be PHY where possible.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
changes since v1:
- allow QDISK for traditional qemu-xen too
- do not pass dm_info through to libxl__device_disk_set_backend
- use libxl_disk_*_to_string in a few places rather than ..._string_of_*
- simplify qdisk test in libxl__need_xenpv_qemu
diff -r d5dfaa568441 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Wed Jul 06 16:32:16 2011 +0100
+++ b/tools/libxl/libxl.c Wed Jul 06 17:03:06 2011 +0100
@@ -902,49 +902,6 @@ int libxl_vncviewer_exec(libxl_ctx *ctx,
/******************************************************************************/
-static int validate_virtual_disk(libxl__gc *gc, char *file_name,
- libxl_device_disk *disk)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
- struct stat stat_buf;
- char *delimiter;
-
- if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
- if (disk->is_cdrom)
- return 0;
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Empty disk %s is not a CDROM
device\n",
- disk->vdev);
- return ERROR_INVAL;
- }
-
- if (disk->format == LIBXL_DISK_FORMAT_RAW) {
- delimiter = strchr(file_name, '':'');
- if (delimiter) {
- if (!strncmp(file_name, "vhd:",
sizeof("vhd:")-1)) {
- disk->format = LIBXL_DISK_FORMAT_VHD;
- file_name = ++delimiter;
- }
- }
- }
-
- if ( stat(file_name, &stat_buf) != 0 ) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s",
file_name);
- return ERROR_INVAL;
- }
- if (disk->backend == LIBXL_DISK_BACKEND_PHY) {
- if ( !(S_ISBLK(stat_buf.st_mode)) ) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a
block device!\n",
- file_name);
- return ERROR_INVAL;
- }
- } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is
0!\n", file_name);
- return ERROR_INVAL;
- }
-
- return 0;
-}
-
int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk
*disk)
{
libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -954,10 +911,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
int devid;
libxl__device device;
int major, minor, rc;
-
- rc = validate_virtual_disk(&gc, disk->pdev_path, disk);
- if (rc)
- return rc;
front = flexarray_make(16, 1);
if (!front) {
@@ -990,34 +943,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
device.devid = devid;
device.domid = domid;
device.kind = DEVICE_VBD;
-
-
- /*
- * Fixing the incoming backend type to try to decide on which
- * backend to use. Unfortunately at the moment this code is
- * utterly broken, but it more or less works.
- */
-
- /*
- * Backend type UNKNOWN should mean "caller does not want to
specify",
- * not "break pointlessely". (Callers should not be required to
- * specify the backend if they don''t want to.)
- */
- if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
- disk->backend = LIBXL_DISK_BACKEND_TAP;
-
- /* If blktap is not available then fallback to qdisk */
- if (disk->backend == LIBXL_DISK_BACKEND_TAP &&
!libxl__blktap_enabled(&gc))
- disk->backend = LIBXL_DISK_BACKEND_QDISK;
-
- /*
- * blktap cannot handle empty disks (aka cdroms). Fall back to
- * qdisk because qemu-xen creates the disk based on the xenstore
- * entries.
- */
- if (disk->backend == LIBXL_DISK_BACKEND_TAP &&
- disk->format == LIBXL_DISK_FORMAT_EMPTY)
- disk->backend == LIBXL_DISK_BACKEND_QDISK;
switch (disk->backend) {
case LIBXL_DISK_BACKEND_PHY:
@@ -1138,68 +1063,49 @@ char * libxl_device_disk_local_attach(li
libxl__gc gc = LIBXL_INIT_GC(ctx);
char *dev = NULL;
char *ret = NULL;
+ int rc;
- /*
- * Fixing the incoming backend type to try to decide on which
- * backend to use. Unfortunately at the moment this code is
- * utterly broken, but it more or less works.
- */
-
- /*
- * Backend type UNKNOWN should mean "caller does not want to
specify",
- * not "break pointlessely". (Callers should not be required to
- * specify the backend if they don''t want to.)
- */
- if (disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)
- disk->backend = LIBXL_DISK_BACKEND_TAP;
+ rc = libxl__device_disk_set_backend(&gc, disk);
+ if (rc) goto out;
switch (disk->backend) {
case LIBXL_DISK_BACKEND_PHY:
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "physical block device
must"
- " be raw");
- break;
- }
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching PHY disk %s to
domain 0",
- disk->pdev_path);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching PHY disk
%s",
+ disk->pdev_path);
dev = disk->pdev_path;
break;
case LIBXL_DISK_BACKEND_TAP:
- if (disk->format == LIBXL_DISK_FORMAT_VHD ||
- disk->format == LIBXL_DISK_FORMAT_RAW)
- {
- if (libxl__blktap_enabled(&gc))
- dev = libxl__blktap_devpath(&gc, disk->pdev_path,
disk->format);
- else {
- if (disk->format != LIBXL_DISK_FORMAT_RAW) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is
required"
- " to open a vhd disk");
- break;
- } else {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "attaching tap
disk %s to domain 0",
- disk->pdev_path);
- dev = disk->pdev_path;
- break;
- }
- }
+ 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;
- } else if (disk->format == LIBXL_DISK_FORMAT_QCOW ||
- disk->format == LIBXL_DISK_FORMAT_QCOW2) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a
qcow or qcow2 disk image");
+ case LIBXL_DISK_FORMAT_VHD:
+ dev = libxl__blktap_devpath(&gc, disk->pdev_path,
+ disk->format);
break;
- } else {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
- "type: %d", disk->backend);
+ case LIBXL_DISK_FORMAT_QCOW:
+ case LIBXL_DISK_FORMAT_QCOW2:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally
attach"
+ " a qcow or qcow2 disk image");
+ break;
+ 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");
+ 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, "attaching qdisk %s to
domain 0\n",
- disk->pdev_path);
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk
%s\n",
+ disk->pdev_path);
dev = disk->pdev_path;
break;
default:
@@ -1208,6 +1114,7 @@ char * libxl_device_disk_local_attach(li
break;
}
+ out:
if (dev != NULL)
ret = strdup(dev);
libxl__free_all(&gc);
diff -r d5dfaa568441 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c Wed Jul 06 16:32:16 2011 +0100
+++ b/tools/libxl/libxl_create.c Wed Jul 06 17:03:06 2011 +0100
@@ -424,6 +424,12 @@ static int do_domain_create(libxl__gc *g
goto error_out;
}
+
+ for (i = 0; i < d_config->num_disks; i++) {
+ ret = libxl__device_disk_set_backend(gc, &d_config->disks[i]);
+ if (ret) goto error_out;
+ }
+
if ( restore_fd < 0 ) {
ret = libxl_run_bootloader(ctx, &d_config->b_info,
d_config->num_disks > 0 ? &d_config->disks[0] : NULL, domid);
if (ret) {
diff -r d5dfaa568441 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Wed Jul 06 16:32:16 2011 +0100
+++ b/tools/libxl/libxl_device.c Wed Jul 06 17:03:06 2011 +0100
@@ -116,6 +116,122 @@ retry_transaction:
rc = 0;
out:
return rc;
+}
+
+typedef struct {
+ libxl__gc *gc;
+ libxl_device_disk *disk;
+ struct stat stab;
+} disk_try_backend_args;
+
+static int disk_try_backend(disk_try_backend_args *a,
+ libxl_disk_backend backend) {
+ /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or
+ * backend on success */
+ libxl_ctx *ctx = libxl__gc_owner(a->gc);
+ switch (backend) {
+
+ case LIBXL_DISK_BACKEND_PHY:
+ if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+ a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
phy"
+ " unsuitable due to format %s",
+ a->disk->vdev,
+ libxl_disk_format_to_string(a->disk->format));
+ return 0;
+ }
+ if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
+ !S_ISBLK(a->stab.st_mode)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
phy"
+ " unsuitable as phys path not a block device",
+ a->disk->vdev);
+ return ERROR_INVAL;
+ }
+
+ return backend;
+
+ case LIBXL_DISK_BACKEND_TAP:
+ if (!libxl__blktap_enabled(a->gc)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
tap"
+ " unsuitable because blktap not available",
+ a->disk->vdev);
+ return 0;
+ }
+ if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY ||
+ (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
tap"
+ " unsuitable because empty devices not
supported",
+ a->disk->vdev);
+ return 0;
+ }
+ return backend;
+
+ case LIBXL_DISK_BACKEND_QDISK:
+ return backend;
+
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend "
+ " %d unknown", a->disk->vdev, backend);
+ return 0;
+
+ }
+}
+
+int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl_disk_backend ok;
+ disk_try_backend_args a;
+
+ a.gc = gc;
+ a.disk = disk;
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s spec.backend=%s",
+ disk->vdev,
+ libxl_disk_backend_to_string(disk->backend));
+
+ if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
+ if (!disk->is_cdrom) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
+ " but not cdrom",
+ disk->vdev);
+ return ERROR_INVAL;
+ }
+ memset(&a.stab, 0, sizeof(a.stab));
+ } else {
+ if (stat(disk->pdev_path, &a.stab)) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+ "failed to stat: %s",
+ disk->vdev, disk->pdev_path);
+ return ERROR_INVAL;
+ }
+ if (!S_ISBLK(a.stab.st_mode) &
+ !S_ISREG(a.stab.st_mode)) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+ "phys path is not a block dev or file:
%s",
+ disk->vdev, disk->pdev_path);
+ return ERROR_INVAL;
+ }
+ }
+
+ if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) {
+ ok= disk_try_backend(&a, disk->backend);
+ } else {
+ ok+ disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?:
+ disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?:
+ disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK);
+ if (ok)
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend
%s",
+ disk->vdev,
+ libxl_disk_backend_to_string(ok));
+ }
+ if (!ok) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "no suitable backend for disk
%s",
+ disk->vdev);
+ return ERROR_INVAL;
+ }
+ disk->backend = ok;
+ return 0;
}
char *libxl__device_disk_string_of_format(libxl_disk_format format)
diff -r d5dfaa568441 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c Wed Jul 06 16:32:16 2011 +0100
+++ b/tools/libxl/libxl_dm.c Wed Jul 06 17:03:06 2011 +0100
@@ -1001,25 +1001,10 @@ int libxl__need_xenpv_qemu(libxl__gc *gc
}
if (nr_disks > 0) {
- int blktap_enabled = -1;
for (i = 0; i < nr_disks; i++) {
- switch (disks[i].backend) {
- case LIBXL_DISK_BACKEND_TAP:
- if (blktap_enabled == -1)
- blktap_enabled = libxl__blktap_enabled(gc);
- if (!blktap_enabled) {
- ret = 1;
- goto out;
- }
- break;
-
- case LIBXL_DISK_BACKEND_QDISK:
+ if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK) {
ret = 1;
goto out;
-
- case LIBXL_DISK_BACKEND_PHY:
- case LIBXL_DISK_BACKEND_UNKNOWN:
- break;
}
}
}
diff -r d5dfaa568441 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Wed Jul 06 16:32:16 2011 +0100
+++ b/tools/libxl/libxl_internal.h Wed Jul 06 17:03:06 2011 +0100
@@ -205,6 +205,7 @@ _hidden void libxl__userdata_destroyall(
/* from xl_device */
_hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
_hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
+_hidden int libxl__device_disk_set_backend(libxl__gc*, libxl_device_disk*);
_hidden int libxl__device_physdisk_major_minor(const char *physpath, int
*major, int *minor);
_hidden int libxl__device_disk_dev_number(const char *virtpath,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jul-07 11:29 UTC
Re: [Xen-devel] [PATCH, v2] libxl: sane disk backend selection and validation
On Wed, 6 Jul 2011, Ian Jackson wrote:> +static int disk_try_backend(disk_try_backend_args *a, > + libxl_disk_backend backend) { > + /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or > + * backend on success */ > + libxl_ctx *ctx = libxl__gc_owner(a->gc); > + switch (backend) { > + > + case LIBXL_DISK_BACKEND_PHY: > + if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW || > + a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy" > + " unsuitable due to format %s", > + a->disk->vdev, > + libxl_disk_format_to_string(a->disk->format)); > + return 0; > + } > + if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY && > + !S_ISBLK(a->stab.st_mode)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy" > + " unsuitable as phys path not a block device", > + a->disk->vdev); > + return ERROR_INVAL; > + } > + > + return backend; > + > + case LIBXL_DISK_BACKEND_TAP: > + if (!libxl__blktap_enabled(a->gc)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap" > + " unsuitable because blktap not available", > + a->disk->vdev); > + return 0; > + } > + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY || > + (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) { > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap" > + " unsuitable because empty devices not supported", > + a->disk->vdev); > + return 0; > + } > + return backend; > +TAP should only be used for raw or vhd formats, not for qcow or qcow2. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jul-08 17:35 UTC
Re: [Xen-devel] [PATCH, v2] libxl: sane disk backend selection and validation
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH, v2] libxl: sane disk
backend selection and validation"):> TAP should only be used for raw or vhd formats, not for qcow or qcow2.
Fixed in this changeset, I think.
Ian.
# HG changeset patch
# User Ian Jackson <Ian.Jackson@eu.citrix.com>
# Date 1310146524 -3600
# Node ID 00d2c5ca26fd40f55f4255ac883ad882a5207cc8
# Parent 443c6a7b6079f490d9adbb46352c20c2628fb144
libxl: do not use tap disk backend other than for raw and vhd
tap does not support qcow/qcow2; update disk_try_backend accordingly.
Break out the "backend not suitable for this format" message so it can
be reused. Remove now-redundant reporting from
libxl_device_disk_local_attach and replace with abort().
Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 443c6a7b6079 -r 00d2c5ca26fd tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Jul 08 18:12:26 2011 +0100
+++ b/tools/libxl/libxl.c Fri Jul 08 18:35:24 2011 +0100
@@ -1089,9 +1089,7 @@ char * libxl_device_disk_local_attach(li
break;
case LIBXL_DISK_FORMAT_QCOW:
case LIBXL_DISK_FORMAT_QCOW2:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally
attach"
- " a qcow or qcow2 disk image");
- break;
+ abort(); /* prevented by libxl__device_disk_set_backend */
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"unrecognized disk format: %d",
disk->format);
diff -r 443c6a7b6079 -r 00d2c5ca26fd tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Fri Jul 08 18:12:26 2011 +0100
+++ b/tools/libxl/libxl_device.c Fri Jul 08 18:35:24 2011 +0100
@@ -134,11 +134,7 @@ static int disk_try_backend(disk_try_bac
case LIBXL_DISK_BACKEND_PHY:
if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
phy"
- " unsuitable due to format %s",
- a->disk->vdev,
- libxl_disk_format_to_string(a->disk->format));
- return 0;
+ goto bad_format;
}
if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
!S_ISBLK(a->stab.st_mode)) {
@@ -157,12 +153,9 @@ static int disk_try_backend(disk_try_bac
a->disk->vdev);
return 0;
}
- if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY ||
- (S_ISREG(a->stab.st_mode) && !a->stab.st_size)) {
- LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
tap"
- " unsuitable because empty devices not
supported",
- a->disk->vdev);
- return 0;
+ if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+ a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
+ goto bad_format;
}
return backend;
@@ -175,6 +168,15 @@ static int disk_try_backend(disk_try_bac
return 0;
}
+ abort(); /* notreached */
+
+ bad_format:
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend %s"
+ " unsuitable due to format %s",
+ a->disk->vdev,
+ libxl_disk_backend_to_string(backend),
+ libxl_disk_format_to_string(a->disk->format));
+ return 0;
}
int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jul-20 12:51 UTC
[Xen-devel] Re: [PATCH, v2] libxl: sane disk backend selection and validation
On Wed, 2011-07-06 at 17:06 +0100, Ian Jackson wrote:> diff -r d5dfaa568441 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Wed Jul 06 16:32:16 2011 +0100 > +++ b/tools/libxl/libxl_device.c Wed Jul 06 17:03:06 2011 +0100 > @@ -116,6 +116,122 @@ retry_transaction: > rc = 0; > out: > return rc; > +} > + > +typedef struct { > + libxl__gc *gc; > + libxl_device_disk *disk; > + struct stat stab; > +} disk_try_backend_args; > + > +static int disk_try_backend(disk_try_backend_args *a, > + libxl_disk_backend backend) {I just noticed this, and out of curiosity rather than thinking it should change, is there a reason to avoid returning enum types for functions? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Aug-03 14:45 UTC
[Xen-devel] Re: [PATCH, v2] libxl: sane disk backend selection and validation
Ian Campbell writes ("Re: [PATCH, v2] libxl: sane disk backend selection
and validation"):> I just noticed this, and out of curiosity rather than thinking it should
> change, is there a reason to avoid returning enum types for functions?
No. I think in this case it''s just a mistake.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel