Chris Lalancette <clalance@xxxxxxxxxx> 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]);
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list