Chris Lalancette <clalance@redhat.com> wrote:

The newer version of iscsi-initiator-utils (6.2.0.865-0.2), available in the f8-updates repository, is required as part of the fix for a hang when doing iscsi logout (the other fix is in the 2.6.25 kernel). Unfortunately, this version also changes the output of the iscsiadm -m session -P 3 command; this output was being used to gather the scsi devices available in a pool and present them as volumes. Consequently, when starting an iSCSI pool on a machine with these newer versions of the iscsi tools, the pool will be successfully created but you won't be able to see any of the LUNs presented.

The attached patch *lessens* our dependency on the iscsiadm output by using sysfs to gather the /dev devices. Note that this patch is not to be applied; I'm just putting it out there so people can take a look at it. DanB has suggested getting rid of our dependency on the iscsiadm output altogether is the way to go, and I agree with him there; this is mostly just a short-term solution, to possibly base future work on.

Hi Chris, This looks like a fine short-term fix. I see that the new regexp is just relaxed, so old iscsiadm output will still be accepted. Good ;-) Regarding the code, a couple nits:

Chris Lalancette --- libvirt-0.4.0/src/storage_backend_iscsi.c.orig 2008-02-22 15:56:03.000000000 -0500 +++ libvirt-0.4.0/src/storage_backend_iscsi.c 2008-02-22 15:57:38.000000000 -0500 @@ -161,24 +161,59 @@ static int virStorageBackendISCSIConnect static int virStorageBackendISCSIMakeLUN(virConnectPtr conn, virStoragePoolObjPtr pool, char **const groups, - void *data ATTRIBUTE_UNUSED) + void *data) { virStorageVolDefPtr vol; int fd = -1; - char scsiid[100]; - char *dev = groups[4]; + unsigned int channel; + char lunid[100]; int opentries = 0; char *devpath = NULL; + char *session = (char *) data;

This cast seems unnecessary (at least for C -- but I don't think anyone is trying to compile libvirt with C++).

+ char sysfs_path[PATH_MAX]; + char *dev = NULL; + DIR *sysdir; + struct dirent *block_dirent; + struct stat sbuf; + int len; + + channel = strtoul(groups[1],NULL,10);

That's fine if groups[1] is already known to represent a syntactically valid integer that's in [0..UINT_MAX]. Otherwise, consider using one of the conversion functions in util.c. Maybe virStrToLong_ui, with a <= UINT_MAX test.

+ snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device/target%s:%.1d:%s/%s:%.1d:%s:%s/block",session,groups[0],channel,groups[2],groups[0],channel,groups[2],groups[3]);

With shorter lines, e.g., like this, I find this a lot easier to read: snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device/" "target%s:%.1d:%s/%s:%.1d:%s:%s/block", session, groups[0], channel, groups[2], groups[0], channel, groups[2], groups[3]);