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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home