[libvirt] [PATCH v4 0/4] storage: handle scsi/iscsi error paths better
v3 here: http://www.redhat.com/archives/libvir-list/2015-April/msg00240.html changes: Patch 1: (prior patch 1) Adjustments from code review - mostly just creating a new function using the "const char *" path instead of pool pointer. Called only from virStorageBackendStablePath Patch 2: (prior patch 3) Rather than continue to try to talk through the v3, here's the latest changes. These will just check in the SCSINewLun if the source pool target path doesn't start with /dev, then to just fail. This includes if the path is "/dev" or "/dev/". Theory for failure is that even if we allowed "/dev" or "/dev/" to continue down into the call to virStorageBackendStablePath all we'd get back was the duplicated 'devpath' which we'd claim was non-fatal. Patch 3: (NEW) Adjust virStorageBackendSCSIFindLUs to return a count of LU's found rather than the current "0" or -1" with a setting of the boolean found (which gets ignored in most cases). By returning a count, 0, or -1 the caller can decide what to do with the data. Patch 4: (prior patch 4) Couple of minor changes regarding comments and the use of a goto instead of if then else in processLU retval checks. Kept the flow as previous including using 'retval' in virStorageBackendSCSIFindLUs rather than creating yet another local status that would need to be checked. John Ferlan (4): storage: Split out the valid path check scsi: Adjust return value for virStorageBackendSCSINewLun scsi: Change return values for virStorageBackendSCSIFindLUs scsi: Adjust return values from processLU src/storage/storage_backend.c | 26 +++++----- src/storage/storage_backend.h | 1 + src/storage/storage_backend_scsi.c | 101 ++++++++++++++++++++++++------------- 3 files changed, 79 insertions(+), 49 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 | 26 +++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0435983..b07e0d9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1674,6 +1674,17 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; } +bool +virStorageBackendPoolPathIsStable(const char *path) +{ + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) + return false; + + if (!STRPREFIX(path, "/dev")) + return false; + + return true; +} /* * Given a volume path directly in /dev/XXX, iterate over the @@ -1703,20 +1714,9 @@ 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 (!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 || + !virStorageBackendPoolPathIsStable(pool->def->target.path)) 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 bb1e8d8..85a8a4b 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 virStorageBackendPoolPathIsStable(const char *path); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, bool loop); -- 2.1.0
s/valid/stable/ in the subject On Sun, Apr 19, 2015 at 08:38:33PM -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 | 26 +++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-)
ACK Jan
On 04/19/2015 06:38 PM, 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 | 26 +++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0435983..b07e0d9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1674,6 +1674,17 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; }
+bool +virStorageBackendPoolPathIsStable(const char *path) +{ + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) + return false; + + if (!STRPREFIX(path, "/dev")) + return false;
I think you want "/dev/" here as the prefix to be required; otherwise, "/device" would match the prefix. (This also means that someone using "//dev/..." would fail the check, but that's probably something we don't need to worry about).
- /* 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 (!STRPREFIX(pool->def->target.path, "/dev")) - goto ret_strdup;
Of course, the bug was pre-existing in the code you are just refactoring from, but now that we've spotted it, we might as well fix it. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/20/2015 12:23 PM, Eric Blake wrote:
On 04/19/2015 06:38 PM, 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 | 26 +++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0435983..b07e0d9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1674,6 +1674,17 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; }
+bool +virStorageBackendPoolPathIsStable(const char *path) +{ + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) + return false; + + if (!STRPREFIX(path, "/dev")) + return false;
I think you want "/dev/" here as the prefix to be required; otherwise, "/device" would match the prefix. (This also means that someone using "//dev/..." would fail the check, but that's probably something we don't need to worry about).
Hmm... Sure I see that... I can make that adjustment. I'll wait a bit before pushing just so see if there's other feedback... John
- /* 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 (!STRPREFIX(pool->def->target.path, "/dev")) - goto ret_strdup;
Of course, the bug was pre-existing in the code you are just refactoring from, but now that we've spotted it, we might as well fix it.
On Mon, Apr 20, 2015 at 12:54:46PM -0400, John Ferlan wrote:
On 04/20/2015 12:23 PM, Eric Blake wrote:
On 04/19/2015 06:38 PM, 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 | 26 +++++++++++++------------- src/storage/storage_backend.h | 1 + 2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0435983..b07e0d9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1674,6 +1674,17 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return 0; }
+bool +virStorageBackendPoolPathIsStable(const char *path) +{ + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) + return false; + + if (!STRPREFIX(path, "/dev")) + return false;
I think you want "/dev/" here as the prefix to be required; otherwise, "/device" would match the prefix. (This also means that someone using "//dev/..." would fail the check, but that's probably something we don't need to worry about).
Hmm... Sure I see that... I can make that adjustment. I'll wait a bit before pushing just so see if there's other feedback...
I think that change should be separate from this code motion. Jan
}
+bool +virStorageBackendPoolPathIsStable(const char *path) +{ + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) + return false; + + if (!STRPREFIX(path, "/dev")) + return false;
I think you want "/dev/" here as the prefix to be required; otherwise, "/device" would match the prefix. (This also means that someone using "//dev/..." would fail the check, but that's probably something we don't need to worry about).
Hmm... Sure I see that... I can make that adjustment. I'll wait a bit before pushing just so see if there's other feedback...
I think that change should be separate from this code motion.
OK, so consider patch 1.5/4: Fix the if (!STRPREFIX(path, "/dev")) to be if (!STRPREFIX(path, "/dev/")) to ensure a path such as "/device" isn't declared stable. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b07e0d9..e0311e1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1680,7 +1680,7 @@ virStorageBackendPoolPathIsStable(const char *path) if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) return false; - if (!STRPREFIX(path, "/dev")) + if (!STRPREFIX(path, "/dev/")) return false; return true; -- 2.1.0
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume. 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 | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..ae3cd9a 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,20 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1; + /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + 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 +211,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 Sun, Apr 19, 2015 at 08:38:34PM -0400, John Ferlan wrote:
Use virStorageBackendPoolUseDevPath API to determine whether creation of stable target path is possible for the volume.
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 | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b96caec..ae3cd9a 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,20 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, char *devpath = NULL; int retval = -1;
+ /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + }
/dev is a valid non-stable pool target path.
+ if (VIR_ALLOC(vol) < 0) goto cleanup;
@@ -187,13 +211,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)) {
Before, when virStorageBackendStablePath returned the same devpath because the pool path was "/dev", we continued with processing the volume. After this patch, we won't even get here because of the first check. Failure to stabilize the path should be expected here, if the pool target path is not stable. Jan
VIR_DEBUG("No stable path found for '%s' in '%s'", devpath, pool->def->target.path);
+ retval = -2; goto cleanup; }
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
...
+ /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + }
/dev is a valid non-stable pool target path.
+ if (VIR_ALLOC(vol) < 0) goto cleanup;
@@ -187,13 +211,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)) {
Before, when virStorageBackendStablePath returned the same devpath because the pool path was "/dev", we continued with processing the volume.
After this patch, we won't even get here because of the first check.
Failure to stabilize the path should be expected here, if the pool target path is not stable.
OK, but because virStorageBackendStablePath won't process the pool->target.path of "/dev", we'll return the duplicated 'devpath' and return -2 for every volume in the pool thus making it look empty. What good is that? Wouldn't it be better to tell the user that "/dev" is not a valid stable path name... The path really needs to be more specific... I suppose one could change virStorageBackendStablePath to accept "/dev" and do the search, but that "could" take a while depending on the size of the "/dev" file system. John
On Mon, Apr 20, 2015 at 12:50:07PM -0400, John Ferlan wrote:
...
+ /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + }
/dev is a valid non-stable pool target path.
+ if (VIR_ALLOC(vol) < 0) goto cleanup;
@@ -187,13 +211,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)) {
Before, when virStorageBackendStablePath returned the same devpath because the pool path was "/dev", we continued with processing the volume.
After this patch, we won't even get here because of the first check.
Failure to stabilize the path should be expected here, if the pool target path is not stable.
OK, but because virStorageBackendStablePath won't process the pool->target.path of "/dev", we'll return the duplicated 'devpath' and return -2 for every volume in the pool thus making it look empty.
What good is that?
None. We should process the volume instead of returning -2. Jan
Wouldn't it be better to tell the user that "/dev" is not a valid stable path name... The path really needs to be more specific... I suppose one could change virStorageBackendStablePath to accept "/dev" and do the search, but that "could" take a while depending on the size of the "/dev" file system.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/21/2015 03:13 AM, Ján Tomko wrote:
On Mon, Apr 20, 2015 at 12:50:07PM -0400, John Ferlan wrote:
...
+ /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + }
/dev is a valid non-stable pool target path.
+ if (VIR_ALLOC(vol) < 0) goto cleanup;
@@ -187,13 +211,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)) {
Before, when virStorageBackendStablePath returned the same devpath because the pool path was "/dev", we continued with processing the volume.
After this patch, we won't even get here because of the first check.
Failure to stabilize the path should be expected here, if the pool target path is not stable.
OK, but because virStorageBackendStablePath won't process the pool->target.path of "/dev", we'll return the duplicated 'devpath' and return -2 for every volume in the pool thus making it look empty.
What good is that?
None. We should process the volume instead of returning -2.
Does the following squashed in work for you? Essentially restoring the /dev || /dev/ check after virStorageBackendStablePath and adding it to the virStorageBackendPoolPathIsStable ? iff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ae3cd9a..b97b2b0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -175,7 +175,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, * path to the device if the virDirRead loop to search the * target pool path for our devpath had failed. */ - if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + if (!virStorageBackendPoolPathIsStable(pool->def->target.path) && + !(STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/"))) { virReportError(VIR_ERR_INVALID_ARG, _("unable to use target path '%s' for dev '%s'"), NULLSTR(pool->def->target.path), dev); @@ -211,7 +213,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, true)) == NULL) goto cleanup; - if (STREQ(devpath, vol->target.path)) { + if (STREQ(devpath, vol->target.path) && + !(STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/"))) { VIR_DEBUG("No stable path found for '%s' in '%s'", devpath, pool->def->target.path);
On Tue, Apr 21, 2015 at 06:18:56AM -0400, John Ferlan wrote:
On 04/21/2015 03:13 AM, Ján Tomko wrote:
On Mon, Apr 20, 2015 at 12:50:07PM -0400, John Ferlan wrote:
...
+ /* Check if the pool is using a stable target path. The call to + * virStorageBackendStablePath will fail if the pool target path + * isn't stable and just return the strdup'd 'devpath' anyway. + * This would be indistinguishable to failing to find the stable + * path to the device if the virDirRead loop to search the + * target pool path for our devpath had failed. + */ + if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to use target path '%s' for dev '%s'"), + NULLSTR(pool->def->target.path), dev); + goto cleanup; + }
/dev is a valid non-stable pool target path.
+ if (VIR_ALLOC(vol) < 0) goto cleanup;
@@ -187,13 +211,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)) {
Before, when virStorageBackendStablePath returned the same devpath because the pool path was "/dev", we continued with processing the volume.
After this patch, we won't even get here because of the first check.
Failure to stabilize the path should be expected here, if the pool target path is not stable.
OK, but because virStorageBackendStablePath won't process the pool->target.path of "/dev", we'll return the duplicated 'devpath' and return -2 for every volume in the pool thus making it look empty.
What good is that?
None. We should process the volume instead of returning -2.
Does the following squashed in work for you? Essentially restoring the /dev || /dev/ check after virStorageBackendStablePath and adding it to the virStorageBackendPoolPathIsStable ?
iff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ae3cd9a..b97b2b0 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -175,7 +175,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, * path to the device if the virDirRead loop to search the * target pool path for our devpath had failed. */ - if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) { + if (!virStorageBackendPoolPathIsStable(pool->def->target.path) && + !(STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/"))) { virReportError(VIR_ERR_INVALID_ARG, _("unable to use target path '%s' for dev '%s'"), NULLSTR(pool->def->target.path), dev); @@ -211,7 +213,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, true)) == NULL) goto cleanup;
- if (STREQ(devpath, vol->target.path)) { + if (STREQ(devpath, vol->target.path) && + !(STREQ(pool->def->target.path, "/dev") || + STREQ(pool->def->target.path, "/dev/"))) {
VIR_DEBUG("No stable path found for '%s' in '%s'", devpath, pool->def->target.path);
ACK Jan
Rather than passing/returning a pointer to a boolean to indicate that perhaps we should try again - adjust the return of the call to return the count of LU's found during processing, then let the caller decide what to do with that value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index ae3cd9a..b98311c 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -430,10 +430,9 @@ processLU(virStoragePoolObjPtr pool, } -static int -virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, - uint32_t scanhost, - bool *found) +int +virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, + uint32_t scanhost) { int retval = 0; uint32_t bus, target, lun; @@ -441,6 +440,7 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, DIR *devicedir = NULL; struct dirent *lun_dirent = NULL; char devicepattern[64]; + int found = 0; VIR_DEBUG("Discovering LUs on host %u", scanhost); @@ -456,7 +456,6 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, snprintf(devicepattern, sizeof(devicepattern), "%u:%%u:%%u:%%u\n", scanhost); - *found = false; while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { @@ -466,25 +465,20 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool, VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name); if (processLU(pool, scanhost, bus, target, lun) == 0) - *found = true; + found++; } - if (!*found) - VIR_DEBUG("No LU found for pool %s", pool->def->name); - closedir(devicedir); - return retval; -} + if (retval < 0) + return -1; -int -virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, - uint32_t scanhost) -{ - bool found; /* This path doesn't care whether found or not */ - return virStorageBackendSCSIFindLUsInternal(pool, scanhost, &found); + VIR_DEBUG("Found %d LUs for pool %s", found, pool->def->name); + + return found; } + static int virStorageBackendSCSITriggerRescan(uint32_t host) { @@ -571,7 +565,7 @@ virStoragePoolFCRefreshThread(void *opaque) const char *name = cbdata->name; virStoragePoolObjPtr pool = cbdata->pool; unsigned int host; - bool found = false; + int found; int tries = 2; do { @@ -587,7 +581,7 @@ virStoragePoolFCRefreshThread(void *opaque) virGetSCSIHostNumber(name, &host) == 0 && virStorageBackendSCSITriggerRescan(host) == 0) { virStoragePoolObjClearVols(pool); - virStorageBackendSCSIFindLUsInternal(pool, host, &found); + found = virStorageBackendSCSIFindLUs(pool, host); } virStoragePoolObjUnlock(pool); } while (!found && --tries); @@ -910,7 +904,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendSCSITriggerRescan(host) < 0) goto out; - virStorageBackendSCSIFindLUs(pool, host); + ignore_value(virStorageBackendSCSIFindLUs(pool, host)); ret = 0; out: -- 2.1.0
On Sun, Apr 19, 2015 at 08:38:35PM -0400, John Ferlan wrote:
Rather than passing/returning a pointer to a boolean to indicate that perhaps we should try again - adjust the return of the call to return the count of LU's found during processing, then let the caller decide what to do with that value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
ACK Jan
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 | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index b98311c..9b1f6a9 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -373,6 +373,15 @@ 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 => non-fatal error or a non-disk entry + */ static int processLU(virStoragePoolObjPtr pool, uint32_t host, @@ -391,7 +400,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 @@ -399,32 +408,28 @@ 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 -1; } - 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; + goto cleanup; } - retval = 0; VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully", host, bus, target, lun); - out: + cleanup: VIR_FREE(block_device); return retval; } @@ -457,6 +462,8 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, snprintf(devicepattern, sizeof(devicepattern), "%u:%%u:%%u:%%u\n", scanhost); while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { + int rc; + if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; @@ -464,7 +471,12 @@ virStorageBackendSCSIFindLUs(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++; } -- 2.1.0
On Sun, Apr 19, 2015 at 08:38:36PM -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 | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
ACK Jan
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko