[libvirt] [PATCH 5/5]: Rewrite findLuns function

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
> This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function
> to only rely on sysfs for finding LUNs, given a session number.  Along the way,
> it also fixes the bug where we wouldn't find LUNs for older kernels (with the
> block:sda format), and also (possibly) fixes a race condition where we could try
> to find the LUN before udev has finished connecting it.  I say it "possibly"
> fixes it because I haven't been able to hit it so far, but I definitely need
> more testing to try and confirm.
>
> Changes since last time:
> 1)  Don't blindly ignore the 0'th LUN (thanks Stefan).  Instead, look if the
> LUNs are Direct-Access devices (as defined by the kernel); if so, they are LUNs
> we can use.
> 2)  Fix up whitespace damage.
> 3)  Check the return value of the scsidev strdup as pointed out by Jim.
> 4)  Change all ISCSIADM commands to be const char *const as pointed out by Jim.
>
> diff -urp libvirt.sendtarget/src/storage_backend_iscsi.c libvirt.findLun/src/storage_backend_iscsi.c
> --- libvirt.sendtarget/src/storage_backend_iscsi.c	2008-06-16 14:35:34.000000000 +0200
> +++ libvirt.findLun/src/storage_backend_iscsi.c	2008-06-16 14:52:31.000000000 +0200
> @@ -119,7 +119,7 @@ virStorageBackendISCSISession(virConnect
...
> +    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 in sysfs path %s: %s"),
> +                                      sysfs_path, strerror(errno));

You can remove the ": %s" suffix and the strerror(errno) argument,
since errno isn't relevant here.  Or maybe better: leave the suffix
and use sys_dirent->d_name as the argument, so the diagnostic shows
what couldn't be parsed.

> +                closedir(sysdir);
> +                return -1;
> +				

The above line has just three TABs.
Best to delete it.

The rest, including patches 1-4, looks fine,
so, pending whatever tests you deem adequate,
ACK.