Re: [Xen-devel] [PATCH] libxl: sane disk backend selection and validation

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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