[libvirt] [RFC 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.
...

Hi Chris,

ACK to the first 4 parts.

Here there's one nit and one problem:

> +virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool,
> +							 unsigned int lun, char *dev)

"dev" const, and doesn't need to go past column 80.
Dan already mentioned TABs.

> +		if (strlen(block) == 5) {
> +			/* OK, this is exactly "block"; must be new-style */
> +			snprintf(sysfs_path, PATH_MAX,
> +					 "/sys/bus/scsi/devices/%u:%u:%u:%u/block",
> +					 host, bus, target, lun);
> +			sysdir = opendir(sysfs_path);
> +			if (sysdir == NULL) {
> +				virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +									  _("Failed to opendir sysfs path %s: %s"),
> +									  sysfs_path, strerror(errno));
> +				retval = -1;
> +				goto namelist_cleanup;
> +			}
> +			while ((sys_dirent = readdir(sysdir))) {
> +				if (!notdotdir(sys_dirent))
> +					continue;
> +
> +				scsidev = strdup(sys_dirent->d_name);
> +				break;
> +			}
> +			closedir(sysdir);
> +		}
> +		else {
> +			/* old-style; just parse out the sd */
> +			block2 = strrchr(block, ':');
> +			if (block2 == NULL) {
> +				/* Hm, wasn't what we were expecting; have to give up */
> +				virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +									  _("Failed to parse block path %s"),
> +									  block);
> +				retval = -1;
> +				goto namelist_cleanup;
> +			}
> +			block2++;
> +			scsidev = strdup(block2);
> +		}

This needs a check for scsidev == NULL, since
virStorageBackendISCSINewLun would segfault on a NULL;
it dereferences the pointer (via its "dev" parameter) with this:

   > +	if (asprintf(&devpath, "/dev/%s", dev) < 0) {

> +		retval = virStorageBackendISCSINewLun(conn, pool, lun, scsidev);
> +		if (retval < 0)
> +			break;
...