Daniel P. Berrange wrote:
On Sat, Mar 28, 2009 at 10:40:48AM -0400, Dave Allan wrote:
Daniel P. Berrange wrote:
This unfortauntely does not work on RHEL5, because there are no targetX.X.X
links here. Only the LUNs appear in this directory.
The location which appears to be present on both old and new kernels is
under:
/sys/class/scsi_host/host0/device/targetX.X.X
This appears to be present for both SCSI and iSCSI hosts.
Unfortunately, that directory isn't present for FC hosts on F10. :(
I've put together a scan strategy that I think will for for both RHEL5
and F10 for all transport types. Let me know if it works for you. The
attached patch should be applied on top of the udev fix I sent
yesterday. This patch is another one that doesn't read well as a patch,
but should be pretty straightforward once it's applied.
I have not addressed the question of what device types to allow, because
I want to think about that one a bit more.
I also took out the unused struct you noticed.
Thanks for all the feedback and testing.
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.
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 fb716757c669da7f82430ac20b43587636df14e6 Mon Sep 17 00:00:00 2001
From: David Allan <dallan@xxxxxxxxxx>
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list