|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |