Chris Lalancette <clalance@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; ...