[Libvir] [RFC][PATCH]: Make the iscsi storage backend mostly use sysfs

[Jim Meyering]

Chris Lalancette <clalance at 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]);