On Mon, Mar 30, 2009 at 05:50:38PM -0400, Dave Allan wrote:
> Daniel P. Berrange wrote:
> >
> >This works now on RHEL-5, F9 and F10.
> >
> >I still had to disable this code
> >
> > if (STREQLEN(devpath, vol->target.path, PATH_MAX)) {
> > VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
> > devpath, pool->def->target.path);
> > retval = -1;
> > goto free_vol;
> > }
> >
> >because it breaks pools configured with a target dir of /dev/
>
> That's a good point. Let's allow people to use non-stable paths by
> specifying a target path of /dev. That preserves the behavior I want,
> which is to prevent people from accidentally using non-stable paths when
> they asked for by-id, etc, but lets them use unstable paths if they want
> to. That's a one-line change that I have implemented in the attached patch.
>
> >And add device_type == 5 to allow CDROMs to work
> >
> > if (device_type != 0 &&
> > device_type != 5) {
> > retval = 0;
> > goto out;
> > }
>
> The more I think about this question, the more I think we need to ensure
> that within a particular pool there is only one device type, also
> implemented in the attached patch. The problem with mixing device types
> within the pool is that it forces the consumer application to do device
> type checking every time it uses a volume from the pool, because the
> different device types cannot be used identically in all cases. I'm not
> entirely in agreement that we should allow device types other than disk
> in this API, but if we ensure that each pool contains only devices of a
> particular type, I don't see any harm in it.
I don't think it makes sense to restrict a pool to only contain devices
of a single type. It is perfectly reasonable for an IDE controller to
have a disk and a CDROM device on it, and restricting this would mean
users needing to create 2 separate pools for the same controller just
to be able to see all the devices.
Other pool types are quite happy to mix volume types, eg raw, qcow2,
vmdk files in filesystem based pool, and I see no reason not to allow
this for HBA based pools.
> diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c
> index 2c5168a..9f6a085 100644
> --- a/src/storage_backend_scsi.c
> +++ b/src/storage_backend_scsi.c
> @@ -149,8 +149,11 @@ virStorageBackendSCSINewLun(virConnectPtr conn,
> goto free_vol;
> }
>
> - if (STREQLEN(devpath, vol->target.path, PATH_MAX)) {
> - VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
> + if (STREQLEN(devpath, vol->target.path, PATH_MAX) &&
> + !(STREQ(pool->def->target.path, "/dev") ||
> + STREQ(pool->def->target.path, "/dev/"))) {
> +
> + VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
> devpath, pool->def->target.path);
> retval = -1;
> goto free_vol;
ACK to this chunk.
> @@ -343,16 +346,19 @@ processLU(virConnectPtr conn,
>
> if (getDeviceType(conn, host, bus, target, lun, &device_type) < 0) {
> virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> - _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"),
> + _("Failed to determine if %u:%u:%u:%u "
> + "is a Direct-Access LUN"),
> host, bus, target, lun);
> retval = -1;
> goto out;
> }
>
> - /* We don't use anything except Direct-Access devices, but finding
> - * one isn't an error, either. */
> - if (device_type != 0) {
> + /* Check to see if the discovered device is the correct type for
> + * the pool. */
> + if (device_type != pool->def->deviceType) {
> retval = 0;
> + VIR_DEBUG(_("Found a device of type %d but pool device type is %d"),
> + device_type, pool->def->deviceType);
> goto out;
> }
>
IMHo we should just be allowing types 0 & 5 directly here, with no
other restriction.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list