[libvirt] [PATCH v3 0/4] storage: handle scsi/iscsi error paths better
Rework of unpushed patches from series: http://www.redhat.com/archives/libvir-list/2015-April/msg00050.html Details: Patch 1 - Split out the non /dev/* checks into their own to start satisfying Jan's comment in his review of the former patch 3: http://www.redhat.com/archives/libvir-list/2015-April/msg00100.html Patch 2 - New - Flip the logic from assume success, change status to failure, and return to assume failure and only have success when everything completes successfully Patch 3 - Adjustments for previous patch 3 to make use of patch 1's split of the checking for using the /dev path. If we find that the /dev/* is not in use for the pool, then force an error earlier. This makes the check later on not need to compare the returned path to the generated 'devpath' to compensate for the logic in virStorageBackendStablePath which will strdup the incoming target path if something goes wrong in looking up the directory. Since both paths will use the new virStorageBackendPoolUseDevPath slightly differently, if devpath is the same as vol->target.path, then we know it's not because the pool target path was to blame. Patch 4 - Is the former patch 5, but now since patch 3 returns a failure in virStorageBackendSCSINewLun for that bad path, we can use it to be the error arbiter and bug fixer. Former patch 6 - no longer necessary, although it would circumvent any of the above patches being necessary to solve the bug... John Ferlan (4): storage: Split out the valid path check refactor virStorageBackendSCSINewLun scsi: Adjust return value for virStorageBackendSCSINewLun scsi: Adjust return values from processLU src/storage/storage_backend.c | 42 +++++++++---- src/storage/storage_backend.h | 1 + src/storage/storage_backend_scsi.c | 124 +++++++++++++++++++++---------------- 3 files changed, 100 insertions(+), 67 deletions(-) -- 2.1.0
For virStorageBackendStablePath, in order to make decisions in other code split out the checks regarding whether the pool's target is empty, using /dev, using /dev/, or doesn't start with /dev Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 42 +++++++++++++++++++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9322571..77c3be3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1675,6 +1675,25 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; } +/* + * Check how the storage pool target path is set. + * + * Returns false if: + * 1. No target path set + * 2. Target path is /dev or /dev/ + * 3. Target path does not start with /dev + */ +bool +virStorageBackendPoolUseDevPath(virStoragePoolObjPtr pool) +{ + if (pool->def->target.path == NULL || + STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/") || + !STRPREFIX(pool->def->target.path, "/dev")) + return false; + + return true; +} /* * Given a volume path directly in /dev/XXX, iterate over the @@ -1704,20 +1723,17 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, int retry = 0; int direrr; - /* Short circuit if pool has no target, or if its /dev */ - if (pool->def->target.path == NULL || - STREQ(pool->def->target.path, "/dev") || - STREQ(pool->def->target.path, "/dev/")) - goto ret_strdup; - - /* Skip whole thing for a pool which isn't in /dev - * so we don't mess filesystem/dir based pools + /* + * If this is a logical pool, then just strdup the passed devpath + * and return. Logical pools are under /dev but already have stable + * paths, so they don't need to search under the pool target path. + * + * For the SCSI/iSCSI pools, if the pool target path isn't under + * /dev, then just strdup the passed devpath and return so we don't + * mess filesystem/dir based pools */ - if (!STRPREFIX(pool->def->target.path, "/dev")) - goto ret_strdup; - - /* Logical pools are under /dev but already have stable paths */ - if (pool->def->type == VIR_STORAGE_POOL_LOGICAL) + if (pool->def->type == VIR_STORAGE_POOL_LOGICAL || + !virStorageBackendPoolUseDevPath(pool)) goto ret_strdup; /* We loop here because /dev/disk/by-{id,path} may not have existed diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index fd2451c..1d5cfc2 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -187,6 +187,7 @@ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, struct stat *sb); +bool virStorageBackendPoolUseDevPath(virStoragePoolObjPtr pool); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, bool loop); -- 2.1.0
On Tue, Apr 07, 2015 at 04:21:00PM -0400, John Ferlan wrote:
For virStorageBackendStablePath, in order to make decisions in other code split out the checks regarding whether the pool's target is empty, using /dev, using /dev/, or doesn't start with /dev
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 42 +++++++++++++++++++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9322571..77c3be3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1675,6 +1675,25 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; }
+/* + * Check how the storage pool target path is set. + *
Answering 'how?' with a bool seems strange to me :)
+ * Returns false if: + * 1. No target path set + * 2. Target path is /dev or /dev/ + * 3. Target path does not start with /dev
This comment just restates the function logic, not its intention. And it's just a matter of time until someone changes the logic without updating the comment. I think it is not necessary to comment the function.
+ */ +bool +virStorageBackendPoolUseDevPath(virStoragePoolObjPtr pool)
'UseDevPath' confuses me. How about 'PoolPathIsStable' (or PoolPathCanBeStable, since we only check if it starts with dev). Moreover, this takes the pool as an argument, but only looks at the path. It should either look at the pool type and deal with logical pools as well, or just take a string as an argument.
+{ + if (pool->def->target.path == NULL || + STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/") || + !STRPREFIX(pool->def->target.path, "/dev")) + return false;
This would look better split into separate conditions, like it was in the original function.
+ + return true; +}
/* * Given a volume path directly in /dev/XXX, iterate over the @@ -1704,20 +1723,17 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, int retry = 0; int direrr;
- /* Short circuit if pool has no target, or if its /dev */ - if (pool->def->target.path == NULL || - STREQ(pool->def->target.path, "/dev") || - STREQ(pool->def->target.path, "/dev/")) - goto ret_strdup; - - /* Skip whole thing for a pool which isn't in /dev - * so we don't mess filesystem/dir based pools + /* + * If this is a logical pool, then just strdup the passed devpath + * and return. Logical pools are under /dev but already have stable + * paths, so they don't need to search under the pool target path. + *
This comment is too verbose for my taste, I think the original version: /* Logical pools are under /dev but already have stable paths */ is enough.
+ * For the SCSI/iSCSI pools, if the pool target path isn't under + * /dev, then just strdup the passed devpath and return so we don't + * mess filesystem/dir based pools */
SCSI/iSCSI pools are not filesystem/dir based. Jan
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 55 ++++++++++++++------------------------ 1 file changed, 20 insertions(+), 35 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 2a3e1e4..b96caec 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -154,14 +154,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, uint32_t lun, const char *dev) { - virStorageVolDefPtr vol; + virStorageVolDefPtr vol = NULL; char *devpath = NULL; - int retval = 0; + int retval = -1; - if (VIR_ALLOC(vol) < 0) { - retval = -1; - goto out; - } + if (VIR_ALLOC(vol) < 0) + goto cleanup; vol->type = VIR_STORAGE_VOL_BLOCK; @@ -170,15 +168,11 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, * in the volume name. We only need uniqueness per-pool, so * just leave 'host' out */ - if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0) { - retval = -1; - goto free_vol; - } + if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0) + goto cleanup; - if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { - retval = -1; - goto free_vol; - } + if (virAsprintf(&devpath, "/dev/%s", dev) < 0) + goto cleanup; VIR_DEBUG("Trying to create volume for '%s'", devpath); @@ -190,10 +184,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, */ if ((vol->target.path = virStorageBackendStablePath(pool, devpath, - true)) == NULL) { - retval = -1; - goto free_vol; - } + true)) == NULL) + goto cleanup; if (STREQ(devpath, vol->target.path) && !(STREQ(pool->def->target.path, "/dev") || @@ -202,34 +194,27 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, VIR_DEBUG("No stable path found for '%s' in '%s'", devpath, pool->def->target.path); - retval = -1; - goto free_vol; + goto cleanup; } if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { - retval = -1; - goto free_vol; - } + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + goto cleanup; - if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) { - retval = -1; - goto free_vol; - } + if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) + goto cleanup; pool->def->capacity += vol->target.capacity; pool->def->allocation += vol->target.allocation; - if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) { - retval = -1; - goto free_vol; - } + if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) + goto cleanup; - goto out; + vol = NULL; + retval = 0; - free_vol: + cleanup: virStorageVolDefFree(vol); - out: VIR_FREE(devpath); return retval; } -- 2.1.0
On Tue, Apr 07, 2015 at 04:21:01PM -0400, John Ferlan wrote: This could use some comment about inverting the logic of the retval variable.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 55 ++++++++++++++------------------------ 1 file changed, 20 insertions(+), 35 deletions(-)
ACK Jan
On 04/16/2015 08:35 AM, Ján Tomko wrote:
On Tue, Apr 07, 2015 at 04:21:01PM -0400, John Ferlan wrote:
This could use some comment about inverting the logic of the retval variable.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 55 ++++++++++++++------------------------ 1 file changed, 20 insertions(+), 35 deletions(-)
ACK
Jan
Weird - usually my verbosity gets flagged as being too much, but not like me to have no commit message... In any case, adjusted commit message and pushed. Working through other comments and will post a v4 later today. Tks John
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume. If not, then return failure. This will differentiate a failed virStorageBackendStablePath which won't need to be fatal. Thus, we'll add a -2 return value to differentiate that the failure was a result of either the inability to find the symlink for the device or failure to open the target path directory Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..d3c6470 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev) } +/* + * Attempt to create a new LUN + * + * Returns: + * + * 0 => Success + * -1 => Failure due to some sort of OOM or other fatal issue found when + * attempting to get/update information about a found volume + * -2 => Failure to find a stable path, not fatal, caller can try another + */ static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, uint32_t host ATTRIBUTE_UNUSED, @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1; + /* Before we get too far - let's see if the pool is using target path + * starting with /dev. Attempts to find a stable path not based on a + * pool target starting with /dev will fail and do lots of unnecessary + * work - so we'll short circuit here. + */ + if (!virStorageBackendPoolUseDevPath(pool)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + } + if (VIR_ALLOC(vol) < 0) goto cleanup; @@ -187,13 +209,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, true)) == NULL) goto cleanup; - if (STREQ(devpath, vol->target.path) && - !(STREQ(pool->def->target.path, "/dev") || - STREQ(pool->def->target.path, "/dev/"))) { + if (STREQ(devpath, vol->target.path)) { VIR_DEBUG("No stable path found for '%s' in '%s'", devpath, pool->def->target.path); + retval = -2; goto cleanup; } -- 2.1.0
On Tue, Apr 07, 2015 at 04:21:02PM -0400, John Ferlan wrote:
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume. If not, then return failure.
This will differentiate a failed virStorageBackendStablePath which won't need to be fatal. Thus, we'll add a -2 return value to differentiate that the failure was a result of either the inability to find the symlink for the device or failure to open the target path directory
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..d3c6470 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev) }
+/* + * Attempt to create a new LUN + * + * Returns: + * + * 0 => Success + * -1 => Failure due to some sort of OOM or other fatal issue found when + * attempting to get/update information about a found volume + * -2 => Failure to find a stable path, not fatal, caller can try another + */ static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, uint32_t host ATTRIBUTE_UNUSED, @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1;
+ /* Before we get too far - let's see if the pool is using target path + * starting with /dev. Attempts to find a stable path not based on a + * pool target starting with /dev will fail and do lots of unnecessary + * work - so we'll short circuit here.
This also rejects the non-stable path '/dev' that was accepted before. Jan
On 04/16/2015 09:14 AM, Ján Tomko wrote:
On Tue, Apr 07, 2015 at 04:21:02PM -0400, John Ferlan wrote:
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume. If not, then return failure.
This will differentiate a failed virStorageBackendStablePath which won't need to be fatal. Thus, we'll add a -2 return value to differentiate that the failure was a result of either the inability to find the symlink for the device or failure to open the target path directory
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..d3c6470 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev) }
+/* + * Attempt to create a new LUN + * + * Returns: + * + * 0 => Success + * -1 => Failure due to some sort of OOM or other fatal issue found when + * attempting to get/update information about a found volume + * -2 => Failure to find a stable path, not fatal, caller can try another + */ static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, uint32_t host ATTRIBUTE_UNUSED, @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1;
+ /* Before we get too far - let's see if the pool is using target path + * starting with /dev. Attempts to find a stable path not based on a + * pool target starting with /dev will fail and do lots of unnecessary + * work - so we'll short circuit here.
This also rejects the non-stable path '/dev' that was accepted before.
Not quite sure I see the issue - can you be more specific? Am I missing something obvious? Previously if "pool->def->target.path" is "/dev" (or NULL or "/dev/" or didn't start with "/dev"), then we'd return a strdup'd 'devpath' into 'vol->target.path'. I see no way 'devpath' could be '/dev' or '/dev/' since it is formatted as : if (virAsprintf(&devpath, "/dev/%s", dev) < 0) goto cleanup; where 'dev' is an argument to this function as found by processLU and cannot be NULL from a call: if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); goto out; which fills 'block_device' from either getNewStyleBlockDevice or getOldStyleBlockDevice. John
On Thu, Apr 16, 2015 at 07:42:55PM -0400, John Ferlan wrote:
On 04/16/2015 09:14 AM, Ján Tomko wrote:
On Tue, Apr 07, 2015 at 04:21:02PM -0400, John Ferlan wrote:
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume. If not, then return failure.
This will differentiate a failed virStorageBackendStablePath which won't need to be fatal. Thus, we'll add a -2 return value to differentiate that the failure was a result of either the inability to find the symlink for the device or failure to open the target path directory
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..d3c6470 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev) }
+/* + * Attempt to create a new LUN + * + * Returns: + * + * 0 => Success + * -1 => Failure due to some sort of OOM or other fatal issue found when + * attempting to get/update information about a found volume + * -2 => Failure to find a stable path, not fatal, caller can try another + */ static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, uint32_t host ATTRIBUTE_UNUSED, @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1;
+ /* Before we get too far - let's see if the pool is using target path + * starting with /dev. Attempts to find a stable path not based on a + * pool target starting with /dev will fail and do lots of unnecessary + * work - so we'll short circuit here.
This also rejects the non-stable path '/dev' that was accepted before.
Not quite sure I see the issue - can you be more specific? Am I missing something obvious?
Previously if "pool->def->target.path" is "/dev" (or NULL or "/dev/" or didn't start with "/dev"), then we'd return a strdup'd 'devpath' into 'vol->target.path'.
I see no way 'devpath' could be '/dev' or '/dev/' since it is formatted as :
if (virAsprintf(&devpath, "/dev/%s", dev) < 0) goto cleanup;
That's the path to the device. The comment above is right about checking the pool target path in the pool definition. After this patch, if pool->def->target.path is "/dev", we wouldn't even get to the virStorageBackendStablePath call, because of the check in virStorageBackendPoolUseDevPath - it will return false, we'll jump to cleanup and skip over this volume. Jan
where 'dev' is an argument to this function as found by processLU and cannot be NULL from a call:
if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); goto out;
which fills 'block_device' from either getNewStyleBlockDevice or getOldStyleBlockDevice.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
work - so we'll short circuit here.
This also rejects the non-stable path '/dev' that was accepted before.
Not quite sure I see the issue - can you be more specific? Am I missing something obvious?
Previously if "pool->def->target.path" is "/dev" (or NULL or "/dev/" or didn't start with "/dev"), then we'd return a strdup'd 'devpath' into 'vol->target.path'.
I see no way 'devpath' could be '/dev' or '/dev/' since it is formatted as :
if (virAsprintf(&devpath, "/dev/%s", dev) < 0) goto cleanup;
That's the path to the device. The comment above is right about checking the pool target path in the pool definition.
After this patch, if pool->def->target.path is "/dev", we wouldn't even get to the virStorageBackendStablePath call, because of the check in virStorageBackendPoolUseDevPath - it will return false, we'll jump to cleanup and skip over this volume.
Hmmm.. maybe I need to think outloud... Assume I change the check to: if (!virStorageBackendPoolPathIsStable(pool->def->target.path) && !(STREQ(pool->def->target.path, "/dev") || STREQ(pool->def->target.path, "/dev/"))) { So, if pool->def->target.path is "/dev", then we'll alloc vol, formulate 'devpath', then call virStorageBackendStablePath. Then virStorageBackendPoolPathIsStable is called which returns false because the pool->def->target.path is "/dev". That causes us to strdup the incoming "/devpath" and return. Upon return we'd end up returning -2 to the caller. So, is this the path you were considering? If not, then please describe in detail. As it turns out setting pool->def->target.path to "/dev" or "/dev/" doesn't cause us to follow down the path of the virReadDir (could take a while if it did). Help turn the lightbulb on! John
https://bugzilla.redhat.com/show_bug.cgi?id=1171933 Adjust the processLU error returns to be a bit more logical. Currently, the calling code cannot determine the difference between a non disk/lun volume and a processed/found disk/lun. It can also not differentiate between perhaps real/fatal error and one that won't necessarily stop the code from finding other volumes. After this patch virStorageBackendSCSIFindLUsInternal will stop processing as soon as a "fatal" message occurs rather than continuting processing for no apparent reason. It will also only set the *found value when at least one of the processLU's was successful. With the failed return, if the reason for the stop was that the pool target path did not exist, was /dev, was /dev/, or did not start with /dev, then iSCSI pool startup and refresh will fail. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 44 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index d3c6470..4367b9e 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -371,6 +371,16 @@ getBlockDevice(uint32_t host, } +/* + * Process a Logical Unit entry from the scsi host device directory + * + * Returns: + * + * 0 => Found a valid entry + * -1 => Some sort of fatal error + * -2 => A "non-fatal" error such as a non disk/lun entry or an entry + * without a block device found + */ static int processLU(virStoragePoolObjPtr pool, uint32_t host, @@ -389,7 +399,7 @@ processLU(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), host, bus, target, lun); - goto out; + return -1; } /* We don't create volumes for devices other than disk and cdrom @@ -397,32 +407,25 @@ processLU(virStoragePoolObjPtr pool, * isn't an error, either. */ if (!(device_type == VIR_STORAGE_DEVICE_TYPE_DISK || device_type == VIR_STORAGE_DEVICE_TYPE_ROM)) - { - retval = 0; - goto out; - } + return -2; VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN", host, bus, target, lun); if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); - goto out; + return -2; } - if (virStorageBackendSCSINewLun(pool, - host, bus, target, lun, - block_device) < 0) { + retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun, + block_device); + if (retval < 0) VIR_DEBUG("Failed to create new storage volume for %u:%u:%u:%u", host, bus, target, lun); - goto out; - } - retval = 0; - - VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", - host, bus, target, lun); + else + VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", + host, bus, target, lun); - out: VIR_FREE(block_device); return retval; } @@ -456,6 +459,8 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, *found = false; while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { + int rc; + if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; @@ -463,7 +468,12 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name); - if (processLU(pool, scanhost, bus, target, lun) == 0) + rc = processLU(pool, scanhost, bus, target, lun); + if (rc == -1) { + retval = -1; + break; + } + if (rc == 0) *found = true; } -- 2.1.0
On Tue, Apr 07, 2015 at 04:21:03PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
Adjust the processLU error returns to be a bit more logical. Currently, the calling code cannot determine the difference between a non disk/lun volume and a processed/found disk/lun. It can also not differentiate between perhaps real/fatal error and one that won't necessarily stop the code from finding other volumes.
After this patch virStorageBackendSCSIFindLUsInternal will stop processing as soon as a "fatal" message occurs rather than continuting processing for no apparent reason. It will also only set the *found value when at least one of the processLU's was successful.
With the failed return, if the reason for the stop was that the pool target path did not exist, was /dev, was /dev/, or did not start with /dev, then iSCSI pool startup and refresh will fail.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 44 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index d3c6470..4367b9e 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -371,6 +371,16 @@ getBlockDevice(uint32_t host, }
+/* + * Process a Logical Unit entry from the scsi host device directory + * + * Returns: + * + * 0 => Found a valid entry
Maybe 1 = found, 0 = not found, but no error?
+ * -1 => Some sort of fatal error + * -2 => A "non-fatal" error such as a non disk/lun entry or an entry
Why the quotes? Also, non-disk/cdrom isn't an error. If we return -2 for that case as well, I'd phrase this as: non-fatal error or a non-disk entry.
+ * without a block device found + */ static int processLU(virStoragePoolObjPtr pool, uint32_t host, @@ -389,7 +399,7 @@ processLU(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), host, bus, target, lun); - goto out; + return -1; }
/* We don't create volumes for devices other than disk and cdrom @@ -397,32 +407,25 @@ processLU(virStoragePoolObjPtr pool, * isn't an error, either. */ if (!(device_type == VIR_STORAGE_DEVICE_TYPE_DISK || device_type == VIR_STORAGE_DEVICE_TYPE_ROM)) - { - retval = 0; - goto out; - } + return -2;
VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN", host, bus, target, lun);
if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); - goto out; + return -2;
Shouldn't this be fatal?
}
- if (virStorageBackendSCSINewLun(pool, - host, bus, target, lun, - block_device) < 0) { + retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun, + block_device); + if (retval < 0) VIR_DEBUG("Failed to create new storage volume for %u:%u:%u:%u", host, bus, target, lun); - goto out; - } - retval = 0; - - VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", - host, bus, target, lun); + else + VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", + host, bus, target, lun);
I prefer the original logic where the success path is unindented: if (ret < 0) { VIR_DEBUG("failure..."); goto cleanup; } ret = 0; VIR_DEBUG("success")
- out: VIR_FREE(block_device); return retval; } @@ -456,6 +459,8 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
*found = false; while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { + int rc; +
A 'rc' variable separated from the 'ret' value of the function could be used for the virDirRead as well
if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; @@ -463,7 +468,12 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);
- if (processLU(pool, scanhost, bus, target, lun) == 0) + rc = processLU(pool, scanhost, bus, target, lun); + if (rc == -1) { + retval = -1; + break;
and this would just jump to cleanup.
+ }
+ if (rc == 0) *found = true;
The int func(bool *found) signature is a bit strange, why not just return 1 on found? I see 'found' is used by virStoragePoolFCRefreshThread, but there is no error checking there. But we'd need yet another return code with the meaning of EAGAIN for that, which is not probably worth it as the whole function is void. Jan
On 04/16/2015 10:06 AM, Ján Tomko wrote:
On Tue, Apr 07, 2015 at 04:21:03PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
Adjust the processLU error returns to be a bit more logical. Currently, the calling code cannot determine the difference between a non disk/lun volume and a processed/found disk/lun. It can also not differentiate between perhaps real/fatal error and one that won't necessarily stop the code from finding other volumes.
After this patch virStorageBackendSCSIFindLUsInternal will stop processing as soon as a "fatal" message occurs rather than continuting processing for no apparent reason. It will also only set the *found value when at least one of the processLU's was successful.
With the failed return, if the reason for the stop was that the pool target path did not exist, was /dev, was /dev/, or did not start with /dev, then iSCSI pool startup and refresh will fail.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 44 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index d3c6470..4367b9e 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -371,6 +371,16 @@ getBlockDevice(uint32_t host, }
+/* + * Process a Logical Unit entry from the scsi host device directory + * + * Returns: + * + * 0 => Found a valid entry
Maybe 1 = found, 0 = not found, but no error?
We can choose whatever numbers we want as long as virStorageBackendSCSIFindLUsInternal actually uses them. I think we've been using -2 in some places lately as kind a non-fatal "marker" of sorts and -1 as a fatal error. I see no reason to change it here.
+ * -1 => Some sort of fatal error + * -2 => A "non-fatal" error such as a non disk/lun entry or an entry
Why the quotes?
Why not? I'm fine with your phraseology below and will use it.
Also, non-disk/cdrom isn't an error. If we return -2 for that case as well, I'd phrase this as: non-fatal error or a non-disk entry.
+ * without a block device found + */ static int processLU(virStoragePoolObjPtr pool, uint32_t host, @@ -389,7 +399,7 @@ processLU(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), host, bus, target, lun); - goto out; + return -1; }
/* We don't create volumes for devices other than disk and cdrom @@ -397,32 +407,25 @@ processLU(virStoragePoolObjPtr pool, * isn't an error, either. */ if (!(device_type == VIR_STORAGE_DEVICE_TYPE_DISK || device_type == VIR_STORAGE_DEVICE_TYPE_ROM)) - { - retval = 0; - goto out; - } + return -2;
VIR_DEBUG("%u:%u:%u:%u is a Direct-Access LUN", host, bus, target, lun);
if (getBlockDevice(host, bus, target, lun, &block_device) < 0) { VIR_DEBUG("Failed to find block device for this LUN"); - goto out; + return -2;
Shouldn't this be fatal?
I suppose the VIR_DEBUG threw me off, but if that fails, then "block_device" isn't generated for some real reason (memory, opendir fail, ReadDir fail, strrchr fail) - probably not something we want to continue from.
}
- if (virStorageBackendSCSINewLun(pool, - host, bus, target, lun, - block_device) < 0) { + retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun, + block_device); + if (retval < 0) VIR_DEBUG("Failed to create new storage volume for %u:%u:%u:%u", host, bus, target, lun); - goto out; - } - retval = 0; - - VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", - host, bus, target, lun); + else + VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", + host, bus, target, lun);
I prefer the original logic where the success path is unindented: if (ret < 0) { VIR_DEBUG("failure..."); goto cleanup; }
ret = 0; VIR_DEBUG("success")
Fine 6 of one, half dozen of another
- out: VIR_FREE(block_device); return retval; } @@ -456,6 +459,8 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
*found = false; while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { + int rc; +
A 'rc' variable separated from the 'ret' value of the function could be used for the virDirRead as well
virDirRead will return -1 on error which is what we want to return to the caller. I see no reason to change as we'd only then have to assign retval to whatever value we invent - yet another thing to check.
if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; @@ -463,7 +468,12 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);
- if (processLU(pool, scanhost, bus, target, lun) == 0) + rc = processLU(pool, scanhost, bus, target, lun); + if (rc == -1) { + retval = -1; + break;
and this would just jump to cleanup.
Cleanup would be the same spot as the break - so what's the difference?
+ }
+ if (rc == 0) *found = true;
The int func(bool *found) signature is a bit strange, why not just return 1 on found?
Certainly not the first function to use bool *found. When added it was done to avoid adjusting all the callers - it was a specific purpose.
I see 'found' is used by virStoragePoolFCRefreshThread, but there is no error checking there.
That code needs a way to attempt to populate a pool for a fc_host because it takes "time" for the device tree to be populated after a createPort. Rather than have start/restart "pause" - the thread was added (there was a bz)
But we'd need yet another return code with the meaning of EAGAIN for that, which is not probably worth it as the whole function is void.
If this processing needs to change, then perhaps it's best to add a patch before this just adjusts the return values. If that's what you want let me know. John (attached a possible diff)
On Thu, Apr 16, 2015 at 09:24:41PM -0400, John Ferlan wrote:
On 04/16/2015 10:06 AM, Ján Tomko wrote:
On Tue, Apr 07, 2015 at 04:21:03PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
Adjust the processLU error returns to be a bit more logical. Currently, the calling code cannot determine the difference between a non disk/lun volume and a processed/found disk/lun. It can also not differentiate between perhaps real/fatal error and one that won't necessarily stop the code from finding other volumes.
After this patch virStorageBackendSCSIFindLUsInternal will stop processing as soon as a "fatal" message occurs rather than continuting processing for no apparent reason. It will also only set the *found value when at least one of the processLU's was successful.
With the failed return, if the reason for the stop was that the pool target path did not exist, was /dev, was /dev/, or did not start with /dev, then iSCSI pool startup and refresh will fail.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 44 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index d3c6470..4367b9e 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -371,6 +371,16 @@ getBlockDevice(uint32_t host, }
+/* + * Process a Logical Unit entry from the scsi host device directory + * + * Returns: + * + * 0 => Found a valid entry
Maybe 1 = found, 0 = not found, but no error?
We can choose whatever numbers we want as long as virStorageBackendSCSIFindLUsInternal actually uses them.
I think we've been using -2 in some places lately as kind a non-fatal "marker" of sorts and -1 as a fatal error.
I see no reason to change it here.
Okay, I was just thinking out loud.
+ * -1 => Some sort of fatal error + * -2 => A "non-fatal" error such as a non disk/lun entry or an entry
Why the quotes?
Why not? I'm fine with your phraseology below and will use it.
They suggest sarcasm to me, but maybe it's just me.
- out: VIR_FREE(block_device); return retval; } @@ -456,6 +459,8 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
*found = false; while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { + int rc; +
A 'rc' variable separated from the 'ret' value of the function could be used for the virDirRead as well
virDirRead will return -1 on error which is what we want to return to the caller.
I see no reason to change as we'd only then have to assign retval to whatever value we invent - yet another thing to check.
Using separate variables for the return value of the current function and the return value of called functions makes the flow easier to follow. IIRC we've had some bugs caused by a leftover returned value from an earlier function call.
if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; @@ -463,7 +468,12 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);
- if (processLU(pool, scanhost, bus, target, lun) == 0) + rc = processLU(pool, scanhost, bus, target, lun); + if (rc == -1) { + retval = -1; + break;
and this would just jump to cleanup.
Cleanup would be the same spot as the break - so what's the difference?
+ }
+ if (rc == 0) *found = true;
The int func(bool *found) signature is a bit strange, why not just return 1 on found?
Certainly not the first function to use bool *found. When added it was done to avoid adjusting all the callers - it was a specific purpose.
I haven't looked at all the other callers, but it does seems
I see 'found' is used by virStoragePoolFCRefreshThread, but there is no error checking there.
That code needs a way to attempt to populate a pool for a fc_host because it takes "time" for the device tree to be populated after a createPort. Rather than have start/restart "pause" - the thread was added (there was a bz)
But we'd need yet another return code with the meaning of EAGAIN for that, which is not probably worth it as the whole function is void.
If this processing needs to change, then perhaps it's best to add a patch before this just adjusts the return values. If that's what you want let me know.
I don't think it's worth the effort - especially since we ignore the failure anyway.
John
(attached a possible diff)
Looks good to me. Jan
participants (2)
-
John Ferlan -
Ján Tomko