7:38 p.m.
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
7:38 p.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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(a)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
11:06 a.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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(a)redhat.com>
---
src/storage/storage_backend.c | 26 +++++++++++++-------------
src/storage/storage_backend.h | 1 +
2 files changed, 14 insertions(+), 13 deletions(-)
ACK
Jan
11:23 a.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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(a)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
11:54 a.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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(a)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.
2:12 a.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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(a)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
5:15 a.m.
New subject: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
}
>>>
>>> +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(a)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
7:38 p.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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
11:18 a.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:50 a.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
...
> + /* 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
2:13 a.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:18 a.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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);
6:07 a.m.
New subject: [libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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
7:38 p.m.
New subject: [libvirt] [PATCH v4 3/4] scsi: Change return values for virStorageBackendSCSIFindLUs
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(a)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
11:20 a.m.
New subject: [libvirt] [PATCH v4 3/4] scsi: Change return values for virStorageBackendSCSIFindLUs
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(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
ACK
Jan
7:38 p.m.
New subject: [libvirt] [PATCH v4 4/4] scsi: Adjust return values from processLU
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(a)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
2:14 a.m.
New subject: [libvirt] [PATCH v4 4/4] scsi: Adjust return values from processLU
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(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
ACK
Jan
3503
days inactive
3504
days old
16 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko