[libvirt] [PATCH 5/5]: Rewrite findLuns function
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. Signed-off-by: Chris Lalancette <clalance@redhat.com>
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.
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.
Jim Meyering wrote:
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.
Oops! Cut and pasted that from elsewhere, and didn't fix up the error messages. I'll fix that up as you advise.
+ closedir(sysdir); + return -1; +
The above line has just three TABs. Best to delete it.
Yeah, probably leftover from my whitespace fixup. I'll fix that as well.
The rest, including patches 1-4, looks fine, so, pending whatever tests you deem adequate, ACK.
Thanks for the review! Chris Lalancette
Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only with his changes. 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. Signed-off-by: Chris Lalancette <clalance@redhat.com>
On Tue, Jun 17, 2008 at 01:02:30PM +0200, Chris Lalancette wrote:
Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only with his changes.
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.
I guess it got sufficient review and feedback, +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Chris Lalancette wrote:
Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only with his changes.
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.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
Committed Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Jim Meyering