Re: [Libvir] [RFC][PATCH]: Make the iscsi storage backend mostly use sysfs
Chris Lalancette <clalance(a)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]);