Re: [libvirt] [PATCH 1/1] Add SCSI pool support.

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.

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;

@@ -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; }