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

Daniel P. Berrange wrote: 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. 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. The XML then looks like: <pool type="scsi"> <name>cdroms</name> <devicetype>cdrom</devicetype> <source> <adapter name="host1"/> </source> <target> <path>/dev/</path> </target> </pool> <snipped the rest of the previous patch> Dave From: David Allan <dallan(a)redhat.com&gt; Date: Mon, 30 Mar 2009 16:56:44 -0400 Subject: [PATCH 1/1] Permit pools of devices other than disks. This patch allows the user to specify a device type for a pool in the XML defining the pool. The code will enforce that all volumes in the pool match that type. The type defaults to disk if none is specified. A second, minor, change allows the use of unstable paths if the user specifies a target path of /dev. --- src/storage_backend_scsi.c | 18 ++++++++++++------ src/storage_conf.c | 39 +++++++++++++++++++++++++++++++++++++++ src/storage_conf.h | 8 ++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) 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; } diff --git a/src/storage_conf.c b/src/storage_conf.c index e2870fd..036cc1f 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -42,6 +42,7 @@ #include "buf.h" #include "util.h" #include "memory.h" +#include "logging.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -450,6 +451,40 @@ error: } +static int +virStorageDefParseDeviceType(virConnectPtr conn, + xmlXPathContextPtr ctxt, + int *deviceType) +{ + char *type; + int retval = 0; + + type = virXPathString(conn, "string(/pool/devicetype)", ctxt); + + VIR_DEBUG(_("type: '%s'"), type); + + /* deviceType defaults to disk if there is no <devicetype> element */ + if (type == NULL || STRCASEEQ(type, "disk")) { + *deviceType = VIR_STORAGE_DEVICE_TYPE_DISK; + goto out; + } + + if (STRCASEEQ(type, "rom") || STRCASEEQ(type, "cdrom")) { + *deviceType = VIR_STORAGE_DEVICE_TYPE_ROM; + goto out; + } + + VIR_DEBUG(_("Unknown device type '%s' specified"), type); + retval = -1; + +out: + VIR_FREE(type); + VIR_DEBUG(_("deviceType is %d"), *deviceType); + + return retval; +} + + static virStoragePoolDefPtr virStoragePoolDefParseDoc(virConnectPtr conn, xmlXPathContextPtr ctxt, @@ -622,6 +657,10 @@ virStoragePoolDefParseDoc(virConnectPtr conn, "/pool/target/permissions", 0700) < 0) goto cleanup; + if (virStorageDefParseDeviceType(conn, ctxt, &ret->deviceType) < 0) { + goto cleanup; + } + return ret; cleanup: diff --git a/src/storage_conf.h b/src/storage_conf.h index fc0fe0e..be6089f 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -115,6 +115,13 @@ enum virStoragePoolType { VIR_STORAGE_POOL_LAST, }; +enum virStoragePoolDeviceType { + VIR_STORAGE_DEVICE_TYPE_DISK = 0x00, + VIR_STORAGE_DEVICE_TYPE_ROM = 0x05, + + VIR_STORAGE_DEVICE_TYPE_LAST, +}; + VIR_ENUM_DECL(virStoragePool) @@ -212,6 +219,7 @@ struct _virStoragePoolDef { char *name; unsigned char uuid[VIR_UUID_BUFLEN]; int type; /* virStoragePoolType */ + int deviceType; /* virStoragePoolDeviceType */ unsigned long long allocation; unsigned long long capacity; -- 1.6.0.6