WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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