3:20 p.m.
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
3:21 p.m.
New subject: [libvirt] [PATCH v3 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 | 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
7:33 a.m.
New subject: [libvirt] [PATCH v3 1/4] storage: Split out the valid path check
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(a)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
3:21 p.m.
New subject: [libvirt] [PATCH v3 2/4] refactor virStorageBackendSCSINewLun
Signed-off-by: John Ferlan <jferlan(a)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
7:35 a.m.
New subject: [libvirt] [PATCH v3 2/4] refactor virStorageBackendSCSINewLun
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(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 55 ++++++++++++++------------------------
1 file changed, 20 insertions(+), 35 deletions(-)
ACK
Jan
11:33 a.m.
New subject: [libvirt] [PATCH v3 2/4] refactor virStorageBackendSCSINewLun
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(a)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
3:21 p.m.
New subject: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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
8:14 a.m.
New subject: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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
6:42 p.m.
New subject: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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
8:01 a.m.
New subject: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:43 p.m.
New subject: [libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun
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
3:21 p.m.
New subject: [libvirt] [PATCH v3 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 | 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
9:06 a.m.
New subject: [libvirt] [PATCH v3 4/4] scsi: Adjust return values from processLU
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(a)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
8:24 p.m.
New subject: [libvirt] [PATCH v3 4/4] scsi: Adjust return values from processLU
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(a)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)
7:55 a.m.
New subject: [libvirt] [PATCH v3 4/4] scsi: Adjust return values from processLU
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(a)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
3540
days inactive
3551
days old
14 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko