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/ And add device_type == 5 to allow CDROMs to work if (device_type != 0 && device_type != 5) { retval = 0; goto out; }

From c749c3cc3d90cf9ae31db0c68fd41b7ce24c2bf3 Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Fri, 27 Mar 2009 17:48:18 -0400 Subject: [PATCH 1/1] Changes based on feedback from DanPB

* Fixed LU scan logic so that it now hopefully works for all transport types on both RHEL5 and F10. The preceeding version didn't work on RHEL5. * Removed the unused struct from storage_backend_scsi.h --- src/storage_backend_iscsi.c | 20 ++++++++++---- src/storage_backend_scsi.c | 61 +++++++++++++++++++----------------------- src/storage_backend_scsi.h | 24 +++++------------ 3 files changed, 49 insertions(+), 56 deletions(-)

diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index 9da7cdc..b516add 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -172,23 +172,31 @@ virStorageBackendISCSIConnection(virConnectPtr conn,

static int -virStorageBackendISCSIFindLUNs(virConnectPtr conn, - virStoragePoolObjPtr pool, - const char *session) +virStorageBackendISCSIFindLUs(virConnectPtr conn, + virStoragePoolObjPtr pool, + const char *session) { char sysfs_path[PATH_MAX]; int retval = 0; + uint32_t host;

snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device", session);

- if (virStorageBackendSCSIFindTargets(conn, pool, sysfs_path, "target") < 0) { + if (virStorageBackendSCSIGetHostNumber(conn, sysfs_path, &host) < 0) { virReportSystemError(conn, errno, - _("Failed to get target list for path '%s'"), + _("Failed to get host number for iSCSI session " + "with path '%s'"), sysfs_path); retval = -1; }

+ if (virStorageBackendSCSIFindLUs(conn, pool, host) < 0) { + virReportSystemError(conn, errno, + _("Failed to find LUs on host %u"), host); + retval = -1; + } + return retval; }

@@ -302,7 +310,7 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn, goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) goto cleanup; - if (virStorageBackendISCSIFindLUNs(conn, pool, session) < 0) + if (virStorageBackendISCSIFindLUs(conn, pool, session) < 0) goto cleanup; VIR_FREE(session);

diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c index e0ced8f..2c5168a 100644 --- a/src/storage_backend_scsi.c +++ b/src/storage_backend_scsi.c @@ -382,68 +382,67 @@ out: }

-static int +int virStorageBackendSCSIFindLUs(virConnectPtr conn, virStoragePoolObjPtr pool, - uint32_t scanhost, - uint32_t scanbus, - uint32_t scantarget) + uint32_t scanhost) { int retval = 0; - uint32_t host, bus, target, lun; - char *target_path = NULL; - DIR *targetdir = NULL; + uint32_t bus, target, lun; + char *device_path = NULL; + DIR *devicedir = NULL; struct dirent *lun_dirent = NULL; + char devicepattern[64]; + + VIR_DEBUG(_("Discovering LUs on host %u"), scanhost);

- VIR_DEBUG(_("Discovering LUs on host %u bus %u target %u"), - scanhost, scanbus, scantarget); + virStorageBackendWaitForDevices(conn);

- if (virAsprintf(&target_path, "/sys/bus/scsi/devices/target%u:%u:%u", - scanhost, scanbus, scantarget) < 0) { + if (virAsprintf(&device_path, "/sys/bus/scsi/devices") < 0) { virReportOOMError(conn); goto out; }

- targetdir = opendir(target_path); + devicedir = opendir(device_path);

- if (targetdir == NULL) { + if (devicedir == NULL) { virReportSystemError(conn, errno, - _("Failed to opendir path '%s'"), target_path); + _("Failed to opendir path '%s'"), device_path); retval = -1; goto out; }

- while ((lun_dirent = readdir(targetdir))) { - if (sscanf(lun_dirent->d_name, "%u:%u:%u:%u\n", - &host, &bus, &target, &lun) != 4) { + snprintf(devicepattern, sizeof(devicepattern), "%u:%%u:%%u:%%u\n", scanhost); + + while ((lun_dirent = readdir(devicedir))) { + if (sscanf(lun_dirent->d_name, devicepattern, + &bus, &target, &lun) != 3) { continue; }

VIR_DEBUG(_("Found LU '%s'"), lun_dirent->d_name);

- processLU(conn, pool, host, bus, target, lun); + processLU(conn, pool, scanhost, bus, target, lun); }

- closedir(targetdir); + closedir(devicedir);

out: - VIR_FREE(target_path); + VIR_FREE(device_path); return retval; }

int -virStorageBackendSCSIFindTargets(virConnectPtr conn, - virStoragePoolObjPtr pool, - const char *sysfs_path, - const char *pattern) +virStorageBackendSCSIGetHostNumber(virConnectPtr conn, + const char *sysfs_path, + uint32_t *host) { int retval = 0; - uint32_t host, bus, target; DIR *sysdir = NULL; struct dirent *dirent = NULL;

- VIR_DEBUG(_("Discovering targets in '%s'"), sysfs_path); + VIR_DEBUG(_("Finding host number from '%s'"), sysfs_path);

virStorageBackendWaitForDevices(conn);

@@ -457,14 +456,13 @@ virStorageBackendSCSIFindTargets(virConnectPtr conn, }

while ((dirent = readdir(sysdir))) { - if (STREQLEN(dirent->d_name, pattern, strlen(pattern))) { + if (STREQLEN(dirent->d_name, "target", strlen("target"))) { if (sscanf(dirent->d_name, - "target%u:%u:%u", &host, &bus, &target) != 3) { + "target%u:", host) != 1) { VIR_DEBUG(_("Failed to parse target '%s'"), dirent->d_name); retval = -1; break; } - virStorageBackendSCSIFindLUs(conn, pool, host, bus, target); } }

@@ -478,7 +476,6 @@ static int virStorageBackendSCSIRefreshPool(virConnectPtr conn, virStoragePoolObjPtr pool) { - char targetN[64]; int retval = 0; uint32_t host;

@@ -492,9 +489,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn,

VIR_DEBUG(_("Scanning host%u"), host);

- snprintf(targetN, sizeof(targetN), "target%u", host); - - virStorageBackendSCSIFindTargets(conn, pool, "/sys/bus/scsi/devices", targetN); + virStorageBackendSCSIFindLUs(conn, pool, host);

out: return retval; diff --git a/src/storage_backend_scsi.h b/src/storage_backend_scsi.h index 678ccd6..d63951d 100644 --- a/src/storage_backend_scsi.h +++ b/src/storage_backend_scsi.h @@ -30,25 +30,15 @@ #define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host" #define LINUX_SYSFS_SCSI_HOST_POSTFIX "device"

-struct _virDirectoryWalkData { - virConnectPtr conn; - virStoragePoolObjPtr pool; - const char *dir_path; - const char *pattern_to_match; - size_t expected_matches; - virHashTablePtr matches; // will be created by walk function - virHashIterator iterator; - virHashDeallocator deallocator; -}; -typedef struct _virDirectoryWalkData virDirectoryWalkData; -typedef virDirectoryWalkData *virDirectoryWalkDataPtr; - extern virStorageBackend virStorageBackendSCSI;

int -virStorageBackendSCSIFindTargets(virConnectPtr conn, - virStoragePoolObjPtr pool, - const char *sysfs_path, - const char *pattern); +virStorageBackendSCSIGetHostNumber(virConnectPtr conn, + const char *sysfs_path, + uint32_t *host); +int +virStorageBackendSCSIFindLUs(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t scanhost);

#endif /* __VIR_STORAGE_BACKEND_SCSI_H__ */ -- 1.6.0.6

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 :|

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 :|

Dave Allan wrote:

Daniel P. Berrange wrote:

...

On Mon, Mar 30, 2009 at 05:50:38PM -0400, Dave Allan 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

Daniel P. Berrange wrote: 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.

Having the consumer create a pool for each device type is the way I was envisioning its being used.

...

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.

That is true. My assumption was that they were all being used by the VMs as disks. Is that not the case, i.e, are people using storage pools to back virtual MMC devices as well? If people are already mixing device types (from the guest's perspective) within pools, then making a distinction here is less important. It still does weird things to pool capacity, and volume creation fails unless there's media in the drive.

I guess what it comes down to is I disagree with allowing anything not a disk in the pool, but if people are already doing it, then I don't disagree enough to continue to argue about 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;

ACK to this chunk.

Cool. Thanks.

...

...

@@ -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.

Attached is what I think is a final version of the scsi host pool code. It's the set of patches we've been discussing rolled up into a single patch, so it should look ok, but let me know if you have additional comments. I allowed both disk and rom and took out the XML enhancement for device type. Dave

From 50801da32e9f7db0be1d56aedff8343f95fed1b4 Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Tue, 3 Mar 2009 16:53:39 -0500 Subject: [PATCH 1/1] Add SCSI pool support.

This code scans a particular host for targets and LUs and adds any LUs it finds as storage volumes. It does not add a volume if it cannot find a stable path for a particular LU and the user specifies a stable target path. A user may request unstable paths by specifying a target path of /dev. Fixed LU scan logic so that it now hopefully works for all transport types on both RHEL5 and F10. --- configure.in | 11 + po/POTFILES.in | 1 + src/Makefile.am | 8 + src/storage_backend.c | 6 + src/storage_backend_iscsi.c | 339 ++--------------------------- src/storage_backend_scsi.c | 510 +++++++++++++++++++++++++++++++++++++++++++ src/storage_backend_scsi.h | 43 ++++ src/storage_conf.c | 18 ++ src/storage_conf.h | 7 + 9 files changed, 618 insertions(+), 325 deletions(-) create mode 100644 src/storage_backend_scsi.c create mode 100644 src/storage_backend_scsi.h diff --git a/configure.in b/configure.in index 6b2bb5e..c941b7e 100644 --- a/configure.in +++ b/configure.in @@ -784,6 +784,8 @@ AC_ARG_WITH([storage-lvm], [ --with-storage-lvm with LVM backend for the storage driver (on)],[],[with_storage_lvm=check]) AC_ARG_WITH([storage-iscsi], [ --with-storage-iscsi with iSCSI backend for the storage driver (on)],[],[with_storage_iscsi=check]) +AC_ARG_WITH([storage-scsi], +[ --with-storage-scsi with SCSI backend for the storage driver (on)],[],[with_storage_scsi=check]) AC_ARG_WITH([storage-disk], [ --with-storage-disk with GPartd Disk backend for the storage driver (on)],[],[with_storage_disk=check]) @@ -793,6 +795,7 @@ if test "$with_libvirtd" = "no"; then with_storage_fs=no with_storage_lvm=no with_storage_iscsi=no + with_storage_scsi=no with_storage_disk=no fi if test "$with_storage_dir" = "yes" ; then @@ -921,6 +924,13 @@ if test "$with_storage_iscsi" = "yes" -o "$with_storage_iscsi" = "check"; then fi AM_CONDITIONAL([WITH_STORAGE_ISCSI], [test "$with_storage_iscsi" = "yes"]) +if test "$with_storage_scsi" = "check"; then + with_storage_scsi=yes + + AC_DEFINE_UNQUOTED([WITH_STORAGE_SCSI], 1, + [whether SCSI backend for storage driver is enabled]) + AM_CONDITIONAL([WITH_STORAGE_SCSI], [test "$with_storage_scsi" = "yes"]) +fi LIBPARTED_CFLAGS= @@ -1357,6 +1367,7 @@ AC_MSG_NOTICE([ FS: $with_storage_fs]) AC_MSG_NOTICE([ NetFS: $with_storage_fs]) AC_MSG_NOTICE([ LVM: $with_storage_lvm]) AC_MSG_NOTICE([ iSCSI: $with_storage_iscsi]) +AC_MSG_NOTICE([ SCSI: $with_storage_scsi]) AC_MSG_NOTICE([ Disk: $with_storage_disk]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Security Drivers]) diff --git a/po/POTFILES.in b/po/POTFILES.in index 8b19b7d..dc86835 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -30,6 +30,7 @@ src/storage_backend_disk.c src/storage_backend_fs.c src/storage_backend_iscsi.c src/storage_backend_logical.c +src/storage_backend_scsi.c src/storage_conf.c src/storage_driver.c src/test.c diff --git a/src/Makefile.am b/src/Makefile.am index d5aac11..60b2d46 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -159,6 +159,9 @@ STORAGE_DRIVER_LVM_SOURCES = \ STORAGE_DRIVER_ISCSI_SOURCES = \ storage_backend_iscsi.h storage_backend_iscsi.c +STORAGE_DRIVER_SCSI_SOURCES = \ + storage_backend_scsi.h storage_backend_scsi.c + STORAGE_DRIVER_DISK_SOURCES = \ storage_backend_disk.h storage_backend_disk.c @@ -353,6 +356,10 @@ if WITH_STORAGE_ISCSI libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_ISCSI_SOURCES) endif +if WITH_STORAGE_SCSI +libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES) +endif + if WITH_STORAGE_DISK libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES) endif @@ -408,6 +415,7 @@ EXTRA_DIST += \ $(STORAGE_DRIVER_FS_SOURCES) \ $(STORAGE_DRIVER_LVM_SOURCES) \ $(STORAGE_DRIVER_ISCSI_SOURCES) \ + $(STORAGE_DRIVER_SCSI_SOURCES) \ $(STORAGE_DRIVER_DISK_SOURCES) \ $(NODE_DEVICE_DRIVER_SOURCES) \ $(NODE_DEVICE_DRIVER_HAL_SOURCES) \ diff --git a/src/storage_backend.c b/src/storage_backend.c index 787630c..71b8020 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -54,6 +54,9 @@ #if WITH_STORAGE_ISCSI #include "storage_backend_iscsi.h" #endif +#if WITH_STORAGE_SCSI +#include "storage_backend_scsi.h" +#endif #if WITH_STORAGE_DISK #include "storage_backend_disk.h" #endif @@ -78,6 +81,9 @@ static virStorageBackendPtr backends[] = { #if WITH_STORAGE_ISCSI &virStorageBackendISCSI, #endif +#if WITH_STORAGE_SCSI + &virStorageBackendSCSI, +#endif #if WITH_STORAGE_DISK &virStorageBackendDisk, #endif diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index d5b10e5..b516add 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -35,6 +35,7 @@ #include <dirent.h> #include "virterror_internal.h" +#include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" @@ -169,343 +170,33 @@ virStorageBackendISCSIConnection(virConnectPtr conn, return 0; } -static int -virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool, - unsigned int lun, const char *dev) -{ - virStorageVolDefPtr vol; - int fd = -1; - char *devpath = NULL; - int opentries = 0; - - if (VIR_ALLOC(vol) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - vol->type = VIR_STORAGE_VOL_BLOCK; - - if (virAsprintf(&(vol->name), "lun-%d", lun) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - /* It can take a little while between logging into the ISCSI - * server and udev creating the /dev nodes, so if we get ENOENT - * we must retry a few times - they should eventually appear. - * We currently wait for upto 5 seconds. Is this good enough ? - * Perhaps not on a very heavily loaded system Any other - * options... ? - */ - reopen: - if ((fd = open(devpath, O_RDONLY)) < 0) { - opentries++; - if (errno == ENOENT && opentries < 50) { - usleep(100 * 1000); - goto reopen; - } - virReportSystemError(conn, errno, - _("cannot open '%s'"), - devpath); - goto cleanup; - } - - /* Now figure out the stable path - * - * XXX this method is O(N) because it scans the pool target - * dir every time its run. Should figure out a more efficient - * way of doing this... - */ - if ((vol->target.path = virStorageBackendStablePath(conn, - pool, - devpath)) == NULL) - goto cleanup; - - VIR_FREE(devpath); - - if (virStorageBackendUpdateVolTargetInfoFD(conn, - &vol->target, - fd, - &vol->allocation, - &vol->capacity) < 0) - goto cleanup; - - /* XXX use unique iSCSI id instead */ - vol->key = strdup(vol->target.path); - if (vol->key == NULL) { - virReportOOMError(conn); - goto cleanup; - } - - - pool->def->capacity += vol->capacity; - pool->def->allocation += vol->allocation; - - if (VIR_REALLOC_N(pool->volumes.objs, - pool->volumes.count+1) < 0) { - virReportOOMError(conn); - goto cleanup; - } - pool->volumes.objs[pool->volumes.count++] = vol; - - close(fd); - - return 0; - - cleanup: - if (fd != -1) close(fd); - VIR_FREE(devpath); - virStorageVolDefFree(vol); - return -1; -} - -static int notdotdir(const struct dirent *dir) -{ - return !(STREQLEN(dir->d_name, ".", 1) || STREQLEN(dir->d_name, "..", 2)); -} - -/* Function to check if the type file in the given sysfs_path is a - * Direct-Access device (i.e. type 0). Return -1 on failure, 0 if not - * a Direct-Access device, and 1 if a Direct-Access device - */ -static int directAccessDevice(const char *sysfs_path) -{ - char typestr[3]; - char *gottype, *p; - FILE *typefile; - int type; - - typefile = fopen(sysfs_path, "r"); - if (typefile == NULL) { - /* there was no type file; that doesn't seem right */ - return -1; - } - gottype = fgets(typestr, 3, typefile); - fclose(typefile); - - if (gottype == NULL) { - /* we couldn't read the type file; have to give up */ - return -1; - } - - /* we don't actually care about p, but if you pass NULL and the last - * character is not \0, virStrToLong_i complains - */ - if (virStrToLong_i(typestr, &p, 10, &type) < 0) { - /* Hm, type wasn't an integer; seems strange */ - return -1; - } - - if (type != 0) { - /* saw a device other than Direct-Access */ - return 0; - } - - return 1; -} static int -virStorageBackendISCSIFindLUNs(virConnectPtr conn, - virStoragePoolObjPtr pool, - const char *session) +virStorageBackendISCSIFindLUs(virConnectPtr conn, + virStoragePoolObjPtr pool, + const char *session) { char sysfs_path[PATH_MAX]; - uint32_t host, bus, target, lun; - DIR *sysdir; - struct dirent *sys_dirent; - struct dirent **namelist; - int i, n, tries, retval, directaccess; - char *block, *scsidev, *block2; - - retval = 0; - block = NULL; - scsidev = NULL; + int retval = 0; + uint32_t host; snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device", session); - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { + if (virStorageBackendSCSIGetHostNumber(conn, sysfs_path, &host) < 0) { virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), + _("Failed to get host number for iSCSI session " + "with path '%s'"), sysfs_path); - return -1; + retval = -1; } - while ((sys_dirent = readdir(sysdir))) { - /* double-negative, so we can use the same function for scandir below */ - if (!notdotdir(sys_dirent)) - continue; - - if (STREQLEN(sys_dirent->d_name, "target", 6)) { - if (sscanf(sys_dirent->d_name, "target%u:%u:%u", - &host, &bus, &target) != 3) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to parse target from sysfs path %s/%s"), - sysfs_path, sys_dirent->d_name); - closedir(sysdir); - return -1; - } - break; - } - } - closedir(sysdir); - - /* we now have the host, bus, and target; let's scan for LUNs */ - snprintf(sysfs_path, PATH_MAX, - "/sys/class/iscsi_session/session%s/device/target%u:%u:%u", - session, host, bus, target); - n = scandir(sysfs_path, &namelist, notdotdir, versionsort); - if (n <= 0) { - /* we didn't find any reasonable entries; return failure */ + if (virStorageBackendSCSIFindLUs(conn, pool, host) < 0) { virReportSystemError(conn, errno, - _("Failed to find any LUNs for session '%s'"), - session); - return -1; - } - - for (i=0; i<n; i++) { - block = NULL; - scsidev = NULL; - - if (sscanf(namelist[i]->d_name, "%u:%u:%u:%u\n", - &host, &bus, &target, &lun) != 4) - continue; - - /* we found a LUN */ - /* Note, however, that just finding a LUN doesn't mean it is - * actually useful to us. There are a few different types of - * LUNs, enumerated in the linux kernel in - * drivers/scsi/scsi.c:scsi_device_types[]. Luckily, these device - * types form part of the ABI between the kernel and userland, so - * are unlikely to change. For now, we ignore everything that isn't - * type 0; that is, a Direct-Access device - */ - snprintf(sysfs_path, PATH_MAX, - "/sys/bus/scsi/devices/%u:%u:%u:%u/type", - host, bus, target, lun); - - directaccess = directAccessDevice(sysfs_path); - if (directaccess < 0) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), - host, bus, target, lun); - retval = -1; - goto namelist_cleanup; - } - else if (directaccess == 0) { - /* not a direct-access device; skip */ - continue; - } - /* implicit else if (access == 1); Direct-Access device */ - - /* It might take some time for the - * /sys/bus/scsi/devices/host:bus:target:lun/block{:sda,/sda} - * link to show up; wait up to 5 seconds for it, then give up - */ - tries = 0; - while (block == NULL && tries < 50) { - snprintf(sysfs_path, PATH_MAX, "/sys/bus/scsi/devices/%u:%u:%u:%u", - host, bus, target, lun); - - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - retval = -1; - goto namelist_cleanup; - } - while ((sys_dirent = readdir(sysdir))) { - if (!notdotdir(sys_dirent)) - continue; - if (STREQLEN(sys_dirent->d_name, "block", 5)) { - block = strdup(sys_dirent->d_name); - break; - } - } - closedir(sysdir); - tries++; - if (block == NULL) - usleep(100 * 1000); - } - - if (block == NULL) { - /* we couldn't find the device link for this device; fail */ - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to find device link for lun %d"), - lun); - retval = -1; - goto namelist_cleanup; - } - - if (strlen(block) == 5) { - /* OK, this is exactly "block"; must be new-style */ - snprintf(sysfs_path, PATH_MAX, - "/sys/bus/scsi/devices/%u:%u:%u:%u/block", - host, bus, target, lun); - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - retval = -1; - goto namelist_cleanup; - } - while ((sys_dirent = readdir(sysdir))) { - if (!notdotdir(sys_dirent)) - continue; - - scsidev = strdup(sys_dirent->d_name); - break; - } - closedir(sysdir); - } - else { - /* old-style; just parse out the sd */ - block2 = strrchr(block, ':'); - if (block2 == NULL) { - /* Hm, wasn't what we were expecting; have to give up */ - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to parse block path %s"), - block); - retval = -1; - goto namelist_cleanup; - } - block2++; - scsidev = strdup(block2); - } - if (scsidev == NULL) { - virReportOOMError(conn); - retval = -1; - goto namelist_cleanup; - } - - retval = virStorageBackendISCSINewLun(conn, pool, lun, scsidev); - if (retval < 0) - break; - VIR_FREE(scsidev); - VIR_FREE(block); + _("Failed to find LUs on host %u"), host); + retval = -1; } -namelist_cleanup: - /* we call these VIR_FREE here to make sure we don't leak memory on - * error cases; in the success case, these are already freed but NULL, - * which should be fine - */ - VIR_FREE(scsidev); - VIR_FREE(block); - - for (i=0; i<n; i++) - VIR_FREE(namelist[i]); - - VIR_FREE(namelist); - return retval; } @@ -615,13 +306,11 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn, pool->def->allocation = pool->def->capacity = pool->def->available = 0; - virStorageBackendWaitForDevices(conn); - if ((session = virStorageBackendISCSISession(conn, pool, 0)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) goto cleanup; - if (virStorageBackendISCSIFindLUNs(conn, pool, session) < 0) + if (virStorageBackendISCSIFindLUs(conn, pool, session) < 0) goto cleanup; VIR_FREE(session); diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c new file mode 100644 index 0000000..a962d1c --- /dev/null +++ b/src/storage_backend_scsi.c @@ -0,0 +1,510 @@ +/* + * storage_backend_scsi.c: storage backend for SCSI handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#include <config.h> + +#include <unistd.h> +#include <stdio.h> +#include <dirent.h> +#include <fcntl.h> + +#include "virterror_internal.h" +#include "storage_backend_scsi.h" +#include "memory.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +/* Function to check if the type file in the given sysfs_path is a + * Direct-Access device (i.e. type 0). Return -1 on failure, type of + * the device otherwise. + */ +static int +getDeviceType(virConnectPtr conn, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + int *type) +{ + char *type_path = NULL; + char typestr[3]; + char *gottype, *p; + FILE *typefile; + int retval = 0; + + if (virAsprintf(&type_path, "/sys/bus/scsi/devices/%u:%u:%u:%u/type", + host, bus, target, lun) < 0) { + virReportOOMError(conn); + goto out; + } + + typefile = fopen(type_path, "r"); + if (typefile == NULL) { + virReportSystemError(conn, errno, + _("Could not find typefile '%s'"), + type_path); + /* there was no type file; that doesn't seem right */ + retval = -1; + goto out; + } + + gottype = fgets(typestr, 3, typefile); + fclose(typefile); + + if (gottype == NULL) { + virReportSystemError(conn, errno, + _("Could not read typefile '%s'"), + type_path); + /* we couldn't read the type file; have to give up */ + retval = -1; + goto out; + } + + /* we don't actually care about p, but if you pass NULL and the last + * character is not \0, virStrToLong_i complains + */ + if (virStrToLong_i(typestr, &p, 10, type) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Device type '%s' is not an integer"), + typestr); + /* Hm, type wasn't an integer; seems strange */ + retval = -1; + goto out; + } + + VIR_DEBUG(_("Device type is %d"), *type); + +out: + VIR_FREE(type_path); + return retval; +} + + +static int +virStorageBackendSCSINewLun(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + const char *dev) +{ + virStorageVolDefPtr vol; + char *devpath = NULL; + int retval = 0; + + if (VIR_ALLOC(vol) < 0) { + virReportOOMError(conn); + retval = -1; + goto out; + } + + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + VIR_DEBUG(_("Trying to create volume for '%s'"), devpath); + + /* Now figure out the stable path + * + * XXX this method is O(N) because it scans the pool target + * dir every time its run. Should figure out a more efficient + * way of doing this... + */ + if ((vol->target.path = virStorageBackendStablePath(conn, + pool, + devpath)) == NULL) { + retval = -1; + goto free_vol; + } + + 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; + } + + if (virStorageBackendUpdateVolTargetInfo(conn, + &vol->target, + &vol->allocation, + &vol->capacity) < 0) { + + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to update volume for '%s'"), + devpath); + retval = -1; + goto free_vol; + } + + /* XXX should use logical unit's UUID instead */ + vol->key = strdup(vol->target.path); + if (vol->key == NULL) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + pool->def->capacity += vol->capacity; + pool->def->allocation += vol->allocation; + + if (VIR_REALLOC_N(pool->volumes.objs, + pool->volumes.count + 1) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + pool->volumes.objs[pool->volumes.count++] = vol; + + goto out; + +free_vol: + virStorageVolDefFree(vol); +out: + VIR_FREE(devpath); + return retval; +} + + +static int +getNewStyleBlockDevice(virConnectPtr conn, + const char *lun_path, + const char *block_name ATTRIBUTE_UNUSED, + char **block_device) +{ + char *block_path = NULL; + DIR *block_dir = NULL; + struct dirent *block_dirent = NULL; + int retval = 0; + + if (virAsprintf(&block_path, "%s/block", lun_path) < 0) { + virReportOOMError(conn); + goto out; + } + + VIR_DEBUG(_("Looking for block device in '%s'"), block_path); + + block_dir = opendir(block_path); + if (block_dir == NULL) { + virReportSystemError(conn, errno, + _("Failed to opendir sysfs path '%s'"), + block_path); + retval = -1; + goto out; + } + + while ((block_dirent = readdir(block_dir))) { + + if (STREQLEN(block_dirent->d_name, ".", 1)) { + continue; + } + + *block_device = strdup(block_dirent->d_name); + VIR_DEBUG(_("Block device is '%s'"), *block_device); + + break; + } + + closedir(block_dir); + +out: + VIR_FREE(block_path); + return retval; +} + + +static int +getOldStyleBlockDevice(virConnectPtr conn, + const char *lun_path ATTRIBUTE_UNUSED, + const char *block_name, + char **block_device) +{ + char *blockp = NULL; + int retval = 0; + + /* old-style; just parse out the sd */ + blockp = strrchr(block_name, ':'); + if (blockp == NULL) { + /* Hm, wasn't what we were expecting; have to give up */ + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to parse block name %s"), + block_name); + retval = -1; + } else { + blockp++; + *block_device = strdup(blockp); + + VIR_DEBUG(_("Block device is '%s'"), *block_device); + } + + return retval; +} + + +static int +getBlockDevice(virConnectPtr conn, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + char **block_device) +{ + char *lun_path = NULL; + DIR *lun_dir = NULL; + struct dirent *lun_dirent = NULL; + int retval = 0; + + if (virAsprintf(&lun_path, "/sys/bus/scsi/devices/%u:%u:%u:%u", + host, bus, target, lun) < 0) { + virReportOOMError(conn); + goto out; + } + + lun_dir = opendir(lun_path); + if (lun_dir == NULL) { + virReportSystemError(conn, errno, + _("Failed to opendir sysfs path '%s'"), + lun_path); + retval = -1; + goto out; + } + + while ((lun_dirent = readdir(lun_dir))) { + if (STREQLEN(lun_dirent->d_name, "block", 5)) { + if (strlen(lun_dirent->d_name) == 5) { + retval = getNewStyleBlockDevice(conn, + lun_path, + lun_dirent->d_name, + block_device); + } else { + retval = getOldStyleBlockDevice(conn, + lun_path, + lun_dirent->d_name, + block_device); + } + break; + } + } + + closedir(lun_dir); + +out: + VIR_FREE(lun_path); + return retval; +} + + +static int +processLU(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun) +{ + char *type_path = NULL; + int retval = 0; + int device_type; + char *block_device = NULL; + + VIR_DEBUG(_("Processing LU %u:%u:%u:%u"), + host, bus, target, lun); + + 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"), + host, bus, target, lun); + retval = -1; + goto out; + } + + /* We don't create volumes for devices other than disk and cdrom + * devices, but finding a device that isn't one of those types + * isn't an error, either. */ + if (!(device_type == VIR_STORAGE_DEVICE_TYPE_DISK || + device_type == VIR_STORAGE_DEVICE_TYPE_ROM)) + { + retval = 0; + goto out; + } + + VIR_DEBUG(_("%u:%u:%u:%u is a Direct-Access LUN"), + host, bus, target, lun); + + if (getBlockDevice(conn, host, bus, target, lun, &block_device) < 0) { + goto out; + } + + if (virStorageBackendSCSINewLun(conn, pool, + host, bus, target, lun, + block_device) < 0) { + VIR_DEBUG(_("Failed to create new storage volume for %u:%u:%u:%u"), + host, bus, target, lun); + retval = -1; + goto out; + } + + VIR_DEBUG(_("Created new storage volume for %u:%u:%u:%u successfully"), + host, bus, target, lun); + + VIR_FREE(type_path); + +out: + return retval; +} + + +int +virStorageBackendSCSIFindLUs(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t scanhost) +{ + int retval = 0; + uint32_t bus, target, lun; + char *device_path = NULL; + DIR *devicedir = NULL; + struct dirent *lun_dirent = NULL; + char devicepattern[64]; + + VIR_DEBUG(_("Discovering LUs on host %u"), scanhost); + + virStorageBackendWaitForDevices(conn); + + if (virAsprintf(&device_path, "/sys/bus/scsi/devices") < 0) { + virReportOOMError(conn); + goto out; + } + + devicedir = opendir(device_path); + + if (devicedir == NULL) { + virReportSystemError(conn, errno, + _("Failed to opendir path '%s'"), device_path); + retval = -1; + goto out; + } + + snprintf(devicepattern, sizeof(devicepattern), "%u:%%u:%%u:%%u\n", scanhost); + + while ((lun_dirent = readdir(devicedir))) { + if (sscanf(lun_dirent->d_name, devicepattern, + &bus, &target, &lun) != 3) { + continue; + } + + VIR_DEBUG(_("Found LU '%s'"), lun_dirent->d_name); + + processLU(conn, pool, scanhost, bus, target, lun); + } + + closedir(devicedir); + +out: + VIR_FREE(device_path); + return retval; +} + + +int +virStorageBackendSCSIGetHostNumber(virConnectPtr conn, + const char *sysfs_path, + uint32_t *host) +{ + int retval = 0; + DIR *sysdir = NULL; + struct dirent *dirent = NULL; + + VIR_DEBUG(_("Finding host number from '%s'"), sysfs_path); + + virStorageBackendWaitForDevices(conn); + + sysdir = opendir(sysfs_path); + + if (sysdir == NULL) { + virReportSystemError(conn, errno, + _("Failed to opendir path '%s'"), sysfs_path); + retval = -1; + goto out; + } + + while ((dirent = readdir(sysdir))) { + if (STREQLEN(dirent->d_name, "target", strlen("target"))) { + if (sscanf(dirent->d_name, + "target%u:", host) != 1) { + VIR_DEBUG(_("Failed to parse target '%s'"), dirent->d_name); + retval = -1; + break; + } + } + } + + closedir(sysdir); +out: + return retval; +} + + +static int +virStorageBackendSCSIRefreshPool(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + int retval = 0; + uint32_t host; + + pool->def->allocation = pool->def->capacity = pool->def->available = 0; + + if (sscanf(pool->def->source.adapter, "host%u", &host) != 1) { + VIR_DEBUG(_("Failed to get host number from '%s'"), pool->def->source.adapter); + retval = -1; + goto out; + } + + VIR_DEBUG(_("Scanning host%u"), host); + + virStorageBackendSCSIFindLUs(conn, pool, host); + +out: + return retval; +} + + +virStorageBackend virStorageBackendSCSI = { + .type = VIR_STORAGE_POOL_SCSI, + + .refreshPool = virStorageBackendSCSIRefreshPool, +}; diff --git a/src/storage_backend_scsi.h b/src/storage_backend_scsi.h new file mode 100644 index 0000000..808d47b --- /dev/null +++ b/src/storage_backend_scsi.h @@ -0,0 +1,43 @@ +/* + * storage_backend_scsi.h: storage backend for SCSI handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#ifndef __VIR_STORAGE_BACKEND_SCSI_H__ +#define __VIR_STORAGE_BACKEND_SCSI_H__ + +#include "storage_backend.h" + +#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host" +#define LINUX_SYSFS_SCSI_HOST_POSTFIX "device" + +extern virStorageBackend virStorageBackendSCSI; + +int +virStorageBackendSCSIGetHostNumber(virConnectPtr conn, + const char *sysfs_path, + uint32_t *host); +int +virStorageBackendSCSIFindLUs(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t scanhost); + +#endif /* __VIR_STORAGE_BACKEND_SCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 1c9a4e5..e2870fd 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -187,6 +187,14 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .formatToString = virStoragePoolFormatDiskTypeToString, } }, + { .poolType = VIR_STORAGE_POOL_SCSI, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER), + }, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } + }, { .poolType = VIR_STORAGE_POOL_DISK, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), @@ -269,6 +277,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { VIR_FREE(source->devices); VIR_FREE(source->dir); VIR_FREE(source->name); + VIR_FREE(source->adapter); if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { VIR_FREE(source->auth.chap.login); @@ -572,6 +581,15 @@ virStoragePoolDefParseDoc(virConnectPtr conn, } } + if (options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) { + if ((ret->source.adapter = virXPathString(conn, + "string(/pool/source/adapter/@name)", + ctxt)) == NULL) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing storage pool source adapter name")); + goto cleanup; + } + } authType = virXPathString(conn, "string(/pool/source/auth/@type)", ctxt); if (authType == NULL) { diff --git a/src/storage_conf.h b/src/storage_conf.h index fc0fe0e..4e35ccb 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -117,6 +117,13 @@ enum virStoragePoolType { VIR_ENUM_DECL(virStoragePool) +enum virStoragePoolDeviceType { + VIR_STORAGE_DEVICE_TYPE_DISK = 0x00, + VIR_STORAGE_DEVICE_TYPE_ROM = 0x05, + + VIR_STORAGE_DEVICE_TYPE_LAST, +}; + enum virStoragePoolAuthType { VIR_STORAGE_POOL_AUTH_NONE, -- 1.6.0.6

Daniel Veillard wrote:

On Tue, Mar 31, 2009 at 05:49:47PM -0400, Dave Allan wrote:

...

Dave Allan wrote: Attached is what I think is a final version of the scsi host pool code. It's the set of patches we've been discussing rolled up into a single patch, so it should look ok, but let me know if you have additional comments. I allowed both disk and rom and took out the XML enhancement for device type.

Dave

Patch looks fine to me, but I admit I didn't tested it

Can anybody volunteer to try it out? (Dan has been shouldering the entire burden so far.) I've done a fair amount of testing over the course of writing it, but at least one other person should try it out before we commit it. In particular, I haven't tried it with media in a CD or DVD drive (my devel system is remote), so I don't know if a volume actually gets created properly. All the debug output is correct, so I am reasonably confident, but it should be run once before committing. I'm happy to advise on that effort. For testing, you'll need a few DMXs and fully redundant fabrics. ;) I'm kidding, of course...but seriously, if anybody else that has FC infrastructure wants to try it out, that would be great feedback.

[...]

...

--- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -187,6 +187,14 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .formatToString = virStoragePoolFormatDiskTypeToString, } }, + { .poolType = VIR_STORAGE_POOL_SCSI, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER), + }, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } + }, { .poolType = VIR_STORAGE_POOL_DISK, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE),

I think at some point the C99 initialization should be turned back into plain old initialization, but that's something we should probably do more globally independantly.

Agreed, since it sounds from the other thread like old-style is what we're standardizing on. I don't feel strongly which is better, but we should standardize. I'll try to put together a patch for that sooner rather than later. Dave

On Thu, Mar 26, 2009 at 05:55:48PM -0400, David Allan wrote:

This version of the code scans a particular host for targets and LUs and adds any LUs it finds as storage volumes.

-static int -virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool, - unsigned int lun, const char *dev) -{ - virStorageVolDefPtr vol; - int fd = -1; - char *devpath = NULL; - int opentries = 0; - - if (VIR_ALLOC(vol) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - vol->type = VIR_STORAGE_VOL_BLOCK; - - if (virAsprintf(&(vol->name), "lun-%d", lun) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - /* It can take a little while between logging into the ISCSI - * server and udev creating the /dev nodes, so if we get ENOENT - * we must retry a few times - they should eventually appear. - * We currently wait for upto 5 seconds. Is this good enough ? - * Perhaps not on a very heavily loaded system Any other - * options... ? - */ - reopen: - if ((fd = open(devpath, O_RDONLY)) < 0) { - opentries++; - if (errno == ENOENT && opentries < 50) { - usleep(100 * 1000); - goto reopen; - } - virReportSystemError(conn, errno, - _("cannot open '%s'"), - devpath); - goto cleanup; - }

The new generic 'NewLUN' method has lost this bit of retry / sleep logic. This unfortauntely causes us to randomly loose LUNs due to fact that udev may not yet have created the /dev/ node or the stable path. In this example, I have a iSCSI pool with a single LUN that is active. On the iSCSI target I then export a second LUNs, and refresh the pool on the libvirt client. The first time I refresh, the OS sees the new LUN (as per lsscsi), but libvirt misses it. I have to refresh libvirt again to see it # virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4 [root@dhcp-1-98 ~]# [root@dhcp-1-98 ~]# lsscsi [0:0:0:0] disk ATA HDT722516DLA380 V43O /dev/sda [1:0:0:0] cd/dvd PLEXTOR DVDR PX-755A 1.04 /dev/sr0 [5:0:0:0] storage IET Controller 0001 - [5:0:0:4] disk IET VIRTUAL-DISK 0001 /dev/sdb [root@dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi Pool lettuceiscsi refreshed [root@dhcp-1-98 ~]# lsscsi [0:0:0:0] disk ATA HDT722516DLA380 V43O /dev/sda [1:0:0:0] cd/dvd PLEXTOR DVDR PX-755A 1.04 /dev/sr0 [5:0:0:0] storage IET Controller 0001 - [5:0:0:2] disk IET VIRTUAL-DISK 0001 /dev/sdc [5:0:0:4] disk IET VIRTUAL-DISK 0001 /dev/sdb [root@dhcp-1-98 ~]# [root@dhcp-1-98 ~]# virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4 [root@dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi Pool lettuceiscsi refreshed [root@dhcp-1-98 ~]# virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.2 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-2 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4

- - /* Now figure out the stable path - * - * XXX this method is O(N) because it scans the pool target - * dir every time its run. Should figure out a more efficient - * way of doing this... - */ - if ((vol->target.path = virStorageBackendStablePath(conn, - pool, - devpath)) == NULL) - goto cleanup; - - VIR_FREE(devpath); - - if (virStorageBackendUpdateVolTargetInfoFD(conn, - &vol->target, - fd, - &vol->allocation, - &vol->capacity) < 0) - goto cleanup; - - /* XXX use unique iSCSI id instead */ - vol->key = strdup(vol->target.path); - if (vol->key == NULL) { - virReportOOMError(conn); - goto cleanup; - } - - - pool->def->capacity += vol->capacity; - pool->def->allocation += vol->allocation; - - if (VIR_REALLOC_N(pool->volumes.objs, - pool->volumes.count+1) < 0) { - virReportOOMError(conn); - goto cleanup; - } - pool->volumes.objs[pool->volumes.count++] = vol; - - close(fd); - - return 0; - - cleanup: - if (fd != -1) close(fd); - VIR_FREE(devpath); - virStorageVolDefFree(vol); - return -1; -} - -static int notdotdir(const struct dirent *dir) -{ - return !(STREQLEN(dir->d_name, ".", 1) || STREQLEN(dir->d_name, "..", 2)); -} - -/* Function to check if the type file in the given sysfs_path is a - * Direct-Access device (i.e. type 0). Return -1 on failure, 0 if not - * a Direct-Access device, and 1 if a Direct-Access device - */ -static int directAccessDevice(const char *sysfs_path) -{ - char typestr[3]; - char *gottype, *p; - FILE *typefile; - int type; - - typefile = fopen(sysfs_path, "r"); - if (typefile == NULL) { - /* there was no type file; that doesn't seem right */ - return -1; - } - gottype = fgets(typestr, 3, typefile); - fclose(typefile); - - if (gottype == NULL) { - /* we couldn't read the type file; have to give up */ - return -1; - } - - /* we don't actually care about p, but if you pass NULL and the last - * character is not \0, virStrToLong_i complains - */ - if (virStrToLong_i(typestr, &p, 10, &type) < 0) { - /* Hm, type wasn't an integer; seems strange */ - return -1; - } - - if (type != 0) { - /* saw a device other than Direct-Access */ - return 0; - } - - return 1; -}

static int virStorageBackendISCSIFindLUNs(virConnectPtr conn, @@ -315,196 +177,17 @@ virStorageBackendISCSIFindLUNs(virConnectPtr conn, const char *session) { char sysfs_path[PATH_MAX]; - uint32_t host, bus, target, lun; - DIR *sysdir; - struct dirent *sys_dirent; - struct dirent **namelist; - int i, n, tries, retval, directaccess; - char *block, *scsidev, *block2; - - retval = 0; - block = NULL; - scsidev = NULL; + int retval = 0;

snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device", session);

- sysdir = opendir(sysfs_path); - if (sysdir == NULL) { + if (virStorageBackendSCSIFindTargets(conn, pool, sysfs_path, "target") < 0) { virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), + _("Failed to get target list for path '%s'"), sysfs_path); - return -1; + retval = -1; } - while ((sys_dirent = readdir(sysdir))) { - /* double-negative, so we can use the same function for scandir below */ - if (!notdotdir(sys_dirent)) - continue; - - if (STREQLEN(sys_dirent->d_name, "target", 6)) { - if (sscanf(sys_dirent->d_name, "target%u:%u:%u", - &host, &bus, &target) != 3) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to parse target from sysfs path %s/%s"), - sysfs_path, sys_dirent->d_name); - closedir(sysdir); - return -1; - } - break; - } - } - closedir(sysdir); - - /* we now have the host, bus, and target; let's scan for LUNs */ - snprintf(sysfs_path, PATH_MAX, - "/sys/class/iscsi_session/session%s/device/target%u:%u:%u", - session, host, bus, target); - - n = scandir(sysfs_path, &namelist, notdotdir, versionsort); - if (n <= 0) { - /* we didn't find any reasonable entries; return failure */ - virReportSystemError(conn, errno, - _("Failed to find any LUNs for session '%s'"), - session); - return -1; - } - - for (i=0; i<n; i++) { - block = NULL; - scsidev = NULL; - - if (sscanf(namelist[i]->d_name, "%u:%u:%u:%u\n", - &host, &bus, &target, &lun) != 4) - continue; - - /* we found a LUN */ - /* Note, however, that just finding a LUN doesn't mean it is - * actually useful to us. There are a few different types of - * LUNs, enumerated in the linux kernel in - * drivers/scsi/scsi.c:scsi_device_types[]. Luckily, these device - * types form part of the ABI between the kernel and userland, so - * are unlikely to change. For now, we ignore everything that isn't - * type 0; that is, a Direct-Access device - */ - snprintf(sysfs_path, PATH_MAX, - "/sys/bus/scsi/devices/%u:%u:%u:%u/type", - host, bus, target, lun); - - directaccess = directAccessDevice(sysfs_path); - if (directaccess < 0) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), - host, bus, target, lun); - retval = -1; - goto namelist_cleanup; - } - else if (directaccess == 0) { - /* not a direct-access device; skip */ - continue; - } - /* implicit else if (access == 1); Direct-Access device */ - - /* It might take some time for the - * /sys/bus/scsi/devices/host:bus:target:lun/block{:sda,/sda} - * link to show up; wait up to 5 seconds for it, then give up - */ - tries = 0; - while (block == NULL && tries < 50) { - snprintf(sysfs_path, PATH_MAX, "/sys/bus/scsi/devices/%u:%u:%u:%u", - host, bus, target, lun); - - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - retval = -1; - goto namelist_cleanup; - } - while ((sys_dirent = readdir(sysdir))) { - if (!notdotdir(sys_dirent)) - continue; - if (STREQLEN(sys_dirent->d_name, "block", 5)) { - block = strdup(sys_dirent->d_name); - break; - } - } - closedir(sysdir); - tries++; - if (block == NULL) - usleep(100 * 1000); - } - - if (block == NULL) { - /* we couldn't find the device link for this device; fail */ - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to find device link for lun %d"), - lun); - retval = -1; - goto namelist_cleanup; - } - - if (strlen(block) == 5) { - /* OK, this is exactly "block"; must be new-style */ - snprintf(sysfs_path, PATH_MAX, - "/sys/bus/scsi/devices/%u:%u:%u:%u/block", - host, bus, target, lun); - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - retval = -1; - goto namelist_cleanup; - } - while ((sys_dirent = readdir(sysdir))) { - if (!notdotdir(sys_dirent)) - continue; - - scsidev = strdup(sys_dirent->d_name); - break; - } - closedir(sysdir); - } - else { - /* old-style; just parse out the sd */ - block2 = strrchr(block, ':'); - if (block2 == NULL) { - /* Hm, wasn't what we were expecting; have to give up */ - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to parse block path %s"), - block); - retval = -1; - goto namelist_cleanup; - } - block2++; - scsidev = strdup(block2); - } - if (scsidev == NULL) { - virReportOOMError(conn); - retval = -1; - goto namelist_cleanup; - } - - retval = virStorageBackendISCSINewLun(conn, pool, lun, scsidev); - if (retval < 0) - break; - VIR_FREE(scsidev); - VIR_FREE(block); - } - -namelist_cleanup: - /* we call these VIR_FREE here to make sure we don't leak memory on - * error cases; in the success case, these are already freed but NULL, - * which should be fine - */ - VIR_FREE(scsidev); - VIR_FREE(block); - - for (i=0; i<n; i++) - VIR_FREE(namelist[i]); - - VIR_FREE(namelist);

return retval; } diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c new file mode 100644 index 0000000..62c05ae --- /dev/null +++ b/src/storage_backend_scsi.c @@ -0,0 +1,508 @@ +/* + * storage_backend_scsi.c: storage backend for SCSI handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#include <config.h> + +#include <unistd.h> +#include <stdio.h> +#include <dirent.h> +#include <fcntl.h> + +#include "virterror_internal.h" +#include "storage_backend_scsi.h" +#include "memory.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +/* Function to check if the type file in the given sysfs_path is a + * Direct-Access device (i.e. type 0). Return -1 on failure, type of + * the device otherwise. + */ +static int +getDeviceType(virConnectPtr conn, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + int *type) +{ + char *type_path = NULL; + char typestr[3]; + char *gottype, *p; + FILE *typefile; + int retval = 0; + + if (virAsprintf(&type_path, "/sys/bus/scsi/devices/%u:%u:%u:%u/type", + host, bus, target, lun) < 0) { + virReportOOMError(conn); + goto out; + } + + typefile = fopen(type_path, "r"); + if (typefile == NULL) { + virReportSystemError(conn, errno, + _("Could not find typefile '%s'"), + type_path); + /* there was no type file; that doesn't seem right */ + retval = -1; + goto out; + } + + gottype = fgets(typestr, 3, typefile); + fclose(typefile); + + if (gottype == NULL) { + virReportSystemError(conn, errno, + _("Could not read typefile '%s'"), + type_path); + /* we couldn't read the type file; have to give up */ + retval = -1; + goto out; + } + + /* we don't actually care about p, but if you pass NULL and the last + * character is not \0, virStrToLong_i complains + */ + if (virStrToLong_i(typestr, &p, 10, type) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Device type '%s' is not an integer"), + typestr); + /* Hm, type wasn't an integer; seems strange */ + retval = -1; + goto out; + } + + VIR_DEBUG(_("Device type is %d"), *type); + +out: + VIR_FREE(type_path); + return retval; +} + + +static int +virStorageBackendSCSINewLun(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + const char *dev) +{ + virStorageVolDefPtr vol; + char *devpath = NULL; + int retval = 0; + + if (VIR_ALLOC(vol) < 0) { + virReportOOMError(conn); + retval = -1; + goto out; + } + + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + VIR_DEBUG(_("Trying to create volume for '%s'"), devpath); +

This is where I think we need to reintroduce the open/try loop to wait for the nodes to appear in /dev

+ /* Now figure out the stable path + * + * XXX this method is O(N) because it scans the pool target + * dir every time its run. Should figure out a more efficient + * way of doing this... + */ + if ((vol->target.path = virStorageBackendStablePath(conn, + pool, + devpath)) == NULL) { + retval = -1; + goto free_vol; + } + + 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; + }

Also this bit needs to be removed, because we need to fallback to the generic /dev path if no stable link is present, or if the user has explicitly configured /dev/ as the pool target

+ + if (virStorageBackendUpdateVolTargetInfo(conn, + &vol->target, + &vol->allocation, + &vol->capacity) < 0) { + + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to update volume for '%s'"), + devpath); + retval = -1; + goto free_vol; + } + + /* XXX should use logical unit's UUID instead */ + vol->key = strdup(vol->target.path); + if (vol->key == NULL) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + pool->def->capacity += vol->capacity; + pool->def->allocation += vol->allocation; + + if (VIR_REALLOC_N(pool->volumes.objs, + pool->volumes.count + 1) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + pool->volumes.objs[pool->volumes.count++] = vol; + + goto out; + +free_vol: + virStorageVolDefFree(vol); +out: + VIR_FREE(devpath); + return retval; +} + + diff --git a/src/storage_backend_scsi.h b/src/storage_backend_scsi.h new file mode 100644 index 0000000..678ccd6 --- /dev/null +++ b/src/storage_backend_scsi.h @@ -0,0 +1,54 @@ + +#ifndef __VIR_STORAGE_BACKEND_SCSI_H__ +#define __VIR_STORAGE_BACKEND_SCSI_H__ + +#include "storage_backend.h" +#include "hash.h" + +#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host" +#define LINUX_SYSFS_SCSI_HOST_POSTFIX "device" + +struct _virDirectoryWalkData { + virConnectPtr conn; + virStoragePoolObjPtr pool; + const char *dir_path; + const char *pattern_to_match; + size_t expected_matches; + virHashTablePtr matches; // will be created by walk function + virHashIterator iterator; + virHashDeallocator deallocator; +}; +typedef struct _virDirectoryWalkData virDirectoryWalkData; +typedef virDirectoryWalkData *virDirectoryWalkDataPtr;

This struct/typedef doesn't appear to be used anywhere in the code. Guessing this is just a left-over from earlier version. FYI, my testing was on 2.6.18 RHEL-5 kernel, and 2.6.27.9 F10/9 kernels Regards, 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 :|

On Fri, Mar 27, 2009 at 11:51:57AM -0400, Dave Allan wrote:

Daniel P. Berrange wrote:

...

Spot the obvious mistake...

virStorageBackendWaitForDevices(conn);

if ((session = virStorageBackendISCSISession(conn, pool, 0)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) goto cleanup; if (virStorageBackendISCSIFindLUNs(conn, pool, session) < 0) goto cleanup;

Its calling udev settle before it has actually told the iSCSI client to rescan the target for new LUNs :-) Probably best to move the call to virStorageBackendWaitForDevices() into the method virStorageBackendISCSIFindLUNs() itself.

Hah...that would break things, wouldn't it? I've moved the call. Give the attached a go and see if it fixes it.

This seems to have solved it.

From 106b4f9b91340b724d9768182bc8788ab48022c0 Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Fri, 27 Mar 2009 11:45:12 -0400 Subject: [PATCH 1/1] Fixed broken placement of udevadm noticed by DanPB.

--- src/storage_backend_iscsi.c | 2 -- src/storage_backend_scsi.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index 2ececdd..9da7cdc 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -298,8 +298,6 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn,

pool->def->allocation = pool->def->capacity = pool->def->available = 0;

- virStorageBackendWaitForDevices(conn); - if ((session = virStorageBackendISCSISession(conn, pool, 0)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0) diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c index 62c05ae..e0ced8f 100644 --- a/src/storage_backend_scsi.c +++ b/src/storage_backend_scsi.c @@ -445,6 +445,8 @@ virStorageBackendSCSIFindTargets(virConnectPtr conn,

VIR_DEBUG(_("Discovering targets in '%s'"), sysfs_path);

+ virStorageBackendWaitForDevices(conn); + sysdir = opendir(sysfs_path);

if (sysdir == NULL) { @@ -482,8 +484,6 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn,

pool->def->allocation = pool->def->capacity = pool->def->available = 0;

- virStorageBackendWaitForDevices(conn); - if (sscanf(pool->def->source.adapter, "host%u", &host) != 1) { VIR_DEBUG(_("Failed to get host number from '%s'"), pool->def->source.adapter); retval = -1; --

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 :|