[libvirt] [PATCH 0/3] Couple of storage_backend_scsi.c fixes
As promised here [1] I'm proposing my own patch to fix the problem referenced here [2]. 1: https://www.redhat.com/archives/libvir-list/2015-June/msg00621.html 2: https://www.redhat.com/archives/libvir-list/2015-June/msg00567.html John Ferlan (1): scsi: Adjust return status from getBlockDevice Michal Privoznik (2): getNewStyleBlockDevice: Adjust formatting getOldStyleBlockDevice: Adjust formatting src/storage/storage_backend_scsi.c | 91 ++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 38 deletions(-) -- 2.3.6
Instead of initializing return value to zero (success) and overwriting it on every failure just before the control jumps onto 'out' label, let's initialize to an error value and set to zero only when we are sure about the success. Just follow the pattern we have in the rest of the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_scsi.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index e6c8bb5..3c1bae6 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -255,44 +255,41 @@ getNewStyleBlockDevice(const char *lun_path, char *block_path = NULL; DIR *block_dir = NULL; struct dirent *block_dirent = NULL; - int retval = 0; + int retval = -1; int direrr; if (virAsprintf(&block_path, "%s/block", lun_path) < 0) - goto out; + goto cleanup; VIR_DEBUG("Looking for block device in '%s'", block_path); - block_dir = opendir(block_path); - if (block_dir == NULL) { + if (!(block_dir = opendir(block_path))) { virReportSystemError(errno, _("Failed to opendir sysfs path '%s'"), block_path); - retval = -1; - goto out; + goto cleanup; } while ((direrr = virDirRead(block_dir, &block_dirent, block_path)) > 0) { - if (STREQLEN(block_dirent->d_name, ".", 1)) continue; - if (VIR_STRDUP(*block_device, block_dirent->d_name) < 0) { - closedir(block_dir); - retval = -1; - goto out; - } + if (VIR_STRDUP(*block_device, block_dirent->d_name) < 0) + goto cleanup; VIR_DEBUG("Block device is '%s'", *block_device); break; } + if (direrr < 0) - retval = -1; + goto cleanup; - closedir(block_dir); + retval = 0; - out: + cleanup: + if (block_dir) + closedir(block_dir); VIR_FREE(block_path); return retval; } -- 2.3.6
Instead of initializing return value to zero (success) and overwriting it on every failure just before the control jumps onto 'out' label, let's initialize to an error value and set to zero only when we are sure about the success. Just follow the pattern we have in the rest of the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_scsi.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 3c1bae6..ddfbade 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -301,27 +301,25 @@ getOldStyleBlockDevice(const char *lun_path ATTRIBUTE_UNUSED, char **block_device) { char *blockp = NULL; - int retval = 0; + int retval = -1; /* old-style; just parse out the sd */ - blockp = strrchr(block_name, ':'); - if (blockp == NULL) { + if (!(blockp = strrchr(block_name, ':'))) { /* Hm, wasn't what we were expecting; have to give up */ virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse block name %s"), block_name); - retval = -1; + goto cleanup; } else { blockp++; - if (VIR_STRDUP(*block_device, blockp) < 0) { - retval = -1; - goto out; - } + if (VIR_STRDUP(*block_device, blockp) < 0) + goto cleanup; VIR_DEBUG("Block device is '%s'", *block_device); } - out: + retval = 0; + cleanup: return retval; } -- 2.3.6
From: John Ferlan <jferlan@redhat.com> https://bugzilla.redhat.com/show_bug.cgi?id=1224233 Currently it's not possible to determine the difference between a fatal memory allocation or failure to open/read the directory error with a perhaps less fatal, I didn't find the "block" device in the directory (which may be a disk entry without a block device). In the case of the latter, we shouldn't cause failure to continue searching in the caller (virStorageBackendSCSIFindLUs), rather we should allow trying reading the next directory entry. Signed-off-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_scsi.c | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ddfbade..b426145 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -324,6 +324,15 @@ getOldStyleBlockDevice(const char *lun_path ATTRIBUTE_UNUSED, } +/* + * Search a device entry for the "block" file + * + * Returns + * + * 0 => Found it + * -1 => Fatal error + * -2 => Didn't find in lun_path directory + */ static int getBlockDevice(uint32_t host, uint32_t bus, @@ -337,36 +346,47 @@ getBlockDevice(uint32_t host, int retval = -1; int direrr; + *block_device = NULL; + if (virAsprintf(&lun_path, "/sys/bus/scsi/devices/%u:%u:%u:%u", host, bus, target, lun) < 0) - goto out; + goto cleanup; - lun_dir = opendir(lun_path); - if (lun_dir == NULL) { + if (!(lun_dir = opendir(lun_path))) { virReportSystemError(errno, _("Failed to opendir sysfs path '%s'"), lun_path); - goto out; + goto cleanup; } while ((direrr = virDirRead(lun_dir, &lun_dirent, lun_path)) > 0) { if (STREQLEN(lun_dirent->d_name, "block", 5)) { if (strlen(lun_dirent->d_name) == 5) { - retval = getNewStyleBlockDevice(lun_path, - lun_dirent->d_name, - block_device); + if (getNewStyleBlockDevice(lun_path, + lun_dirent->d_name, + block_device) < 0) + goto cleanup; } else { - retval = getOldStyleBlockDevice(lun_path, - lun_dirent->d_name, - block_device); + if (getOldStyleBlockDevice(lun_path, + lun_dirent->d_name, + block_device) < 0) + goto cleanup; } break; } } + if (direrr < 0) + goto cleanup; + if (!*block_device) { + retval = -2; + goto cleanup; + } - closedir(lun_dir); + retval = 0; - out: + cleanup: + if (lun_dir) + closedir(lun_dir); VIR_FREE(lun_path); return retval; } @@ -412,9 +432,9 @@ processLU(virStoragePoolObjPtr pool, VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN", host, bus, target, lun); - if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { + if ((retval = getBlockDevice(host, bus, target, lun, &block_device)) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); - return -1; + return retval; } retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun, -- 2.3.6
On 06/15/2015 07:24 AM, Michal Privoznik wrote:
From: John Ferlan <jferlan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1224233
Currently it's not possible to determine the difference between a fatal memory allocation or failure to open/read the directory error with a perhaps less fatal, I didn't find the "block" device in the directory (which may be a disk entry without a block device).
In the case of the latter, we shouldn't cause failure to continue searching in the caller (virStorageBackendSCSIFindLUs), rather we should allow trying reading the next directory entry.
Signed-off-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_scsi.c | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-)
ACK - I had assumed only 1 & 2 were adjustments and felt odd acking my own change, but now I see that there were more changes here as well... John
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ddfbade..b426145 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -324,6 +324,15 @@ getOldStyleBlockDevice(const char *lun_path ATTRIBUTE_UNUSED, }
+/* + * Search a device entry for the "block" file + * + * Returns + * + * 0 => Found it + * -1 => Fatal error + * -2 => Didn't find in lun_path directory + */ static int getBlockDevice(uint32_t host, uint32_t bus, @@ -337,36 +346,47 @@ getBlockDevice(uint32_t host, int retval = -1; int direrr;
+ *block_device = NULL; + if (virAsprintf(&lun_path, "/sys/bus/scsi/devices/%u:%u:%u:%u", host, bus, target, lun) < 0) - goto out; + goto cleanup;
- lun_dir = opendir(lun_path); - if (lun_dir == NULL) { + if (!(lun_dir = opendir(lun_path))) { virReportSystemError(errno, _("Failed to opendir sysfs path '%s'"), lun_path); - goto out; + goto cleanup; }
while ((direrr = virDirRead(lun_dir, &lun_dirent, lun_path)) > 0) { if (STREQLEN(lun_dirent->d_name, "block", 5)) { if (strlen(lun_dirent->d_name) == 5) { - retval = getNewStyleBlockDevice(lun_path, - lun_dirent->d_name, - block_device); + if (getNewStyleBlockDevice(lun_path, + lun_dirent->d_name, + block_device) < 0) + goto cleanup; } else { - retval = getOldStyleBlockDevice(lun_path, - lun_dirent->d_name, - block_device); + if (getOldStyleBlockDevice(lun_path, + lun_dirent->d_name, + block_device) < 0) + goto cleanup; } break; } } + if (direrr < 0) + goto cleanup; + if (!*block_device) { + retval = -2; + goto cleanup; + }
- closedir(lun_dir); + retval = 0;
- out: + cleanup: + if (lun_dir) + closedir(lun_dir); VIR_FREE(lun_path); return retval; } @@ -412,9 +432,9 @@ processLU(virStoragePoolObjPtr pool, VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN", host, bus, target, lun);
- if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { + if ((retval = getBlockDevice(host, bus, target, lun, &block_device)) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); - return -1; + return retval; }
retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun,
On 06/15/2015 07:24 AM, Michal Privoznik wrote:
As promised here [1] I'm proposing my own patch to fix the problem referenced here [2].
1: https://www.redhat.com/archives/libvir-list/2015-June/msg00621.html
2: https://www.redhat.com/archives/libvir-list/2015-June/msg00567.html
John Ferlan (1): scsi: Adjust return status from getBlockDevice
Michal Privoznik (2): getNewStyleBlockDevice: Adjust formatting getOldStyleBlockDevice: Adjust formatting
src/storage/storage_backend_scsi.c | 91 ++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 38 deletions(-)
ACK to 1 & 2 John
participants (2)
-
John Ferlan -
Michal Privoznik