6:16 p.m.
Adjust error path logic for processing LU's in order to better ascertain
the correct cause for failure. The first two patches are merely setting
for the third one which determines that the pool definition used a non
existent path in the configuration thus no 'real' targets were able to
be created.
John Ferlan (3):
iscsi: Check for presence of error before creating new one
scsi: Adjust return values from processLU
scsi: Check for invalid target.path after processLU failure
src/storage/storage_backend_iscsi.c | 5 +++--
src/storage/storage_backend_scsi.c | 13 +++++++++++--
2 files changed, 14 insertions(+), 4 deletions(-)
--
2.1.0
6:16 p.m.
New subject: [libvirt] [PATCH 1/3] iscsi: Check for presence of error before creating new one
If virStorageBackendSCSIFindLUs fails, but the failure has an error
message - the iscsi code didn't honor that creating it's own wonderful
message such as "error: Failed to find LUs on host 60: ..." - not overly
helpful. Since a few of the called paths generate a message, check for
that before using that generic one.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 079c767..1dac238 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -147,8 +147,9 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
}
if (virStorageBackendSCSIFindLUs(pool, host) < 0) {
- virReportSystemError(errno,
- _("Failed to find LUs on host %u"), host);
+ if (virGetLastError() == NULL)
+ virReportSystemError(errno,
+ _("Failed to find LUs on host %u"), host);
retval = -1;
}
--
2.1.0
6:03 a.m.
New subject: [libvirt] [PATCH 1/3] iscsi: Check for presence of error before creating new one
On Mon, Mar 30, 2015 at 07:16:32PM -0400, John Ferlan wrote:
If virStorageBackendSCSIFindLUs fails, but the failure has an error
message - the iscsi code didn't honor that creating it's own wonderful
message such as "error: Failed to find LUs on host 60: ..." - not overly
helpful. Since a few of the called paths generate a message, check for
that before using that generic one.
Both of the paths returning -1 generate an error:
* return -1 after opendir failed
* retval = virDirRead
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 079c767..1dac238 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -147,8 +147,9 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
}
if (virStorageBackendSCSIFindLUs(pool, host) < 0) {
- virReportSystemError(errno,
- _("Failed to find LUs on host %u"), host);
+ if (virGetLastError() == NULL)
This condition is never true. And it should only be used if
we can't fix the function to consistently set an error in all
cases.
Jan
+ virReportSystemError(errno,
+ _("Failed to find LUs on host %u"), host);
retval = -1;
}
6:52 a.m.
New subject: [libvirt] [PATCH 1/3] iscsi: Check for presence of error before creating new one
On 03/31/2015 07:03 AM, Ján Tomko wrote:
On Mon, Mar 30, 2015 at 07:16:32PM -0400, John Ferlan wrote:
> If virStorageBackendSCSIFindLUs fails, but the failure has an error
> message - the iscsi code didn't honor that creating it's own wonderful
> message such as "error: Failed to find LUs on host 60: ..." - not overly
> helpful. Since a few of the called paths generate a message, check for
> that before using that generic one.
Both of the paths returning -1 generate an error:
* return -1 after opendir failed
* retval = virDirRead
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_iscsi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
> index 079c767..1dac238 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -147,8 +147,9 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
> }
>
> if (virStorageBackendSCSIFindLUs(pool, host) < 0) {
> - virReportSystemError(errno,
> - _("Failed to find LUs on host %u"), host);
> + if (virGetLastError() == NULL)
This condition is never true. And it should only be used if
we can't fix the function to consistently set an error in all
cases.
Fair enough - just being overly cautious or un-optimistic I suppose
John
6:16 p.m.
New subject: [libvirt] [PATCH 2/3] scsi: Adjust return values from processLU
Currently processLU returns a 0 when either a 'non disk/lun' volume or
a processed and found disk/lun value. On return we set *found = true
in either case.
If we don't find any "real" LU's that could be indicative of some other
problem that we may need to message. Therefore, only set the *found when
we've successfully processed a LU.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 58e7e6d..2f1f5ed 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -412,7 +412,7 @@ processLU(virStoragePoolObjPtr pool,
host, bus, target, lun);
goto out;
}
- retval = 0;
+ retval = 1;
VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully",
host, bus, target, lun);
@@ -460,7 +460,7 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
VIR_DEBUG("Found possible LU '%s'", lun_dirent->d_name);
- if (processLU(pool, scanhost, bus, target, lun) == 0)
+ if (processLU(pool, scanhost, bus, target, lun) == 1)
*found = true;
}
--
2.1.0
6:16 p.m.
New subject: [libvirt] [PATCH 3/3] scsi: Check for invalid target.path after processLU failure
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
After processing all the LU's and find no "real" LU's - check if
perhaps
the cause of that was a poorly formed 'target.path'. The expection is
generally that the path is /dev/disk/by-path or at least something in /dev.
Although it's not impossible for the user to provide something. If they
do provide something it should be valid or we should just cause a failure
to start the pool with an appropriate message.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 2f1f5ed..c3a1892 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -467,6 +467,15 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
if (!*found)
VIR_DEBUG("No LU found for pool %s", pool->def->name);
+ if (!*found && !virFileExists(pool->def->target.path) &&
+ !STRPREFIX(pool->def->target.path, "/dev")) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("No LUs found for pool '%s' target "
+ "path '%s' not found"),
+ pool->def->name, pool->def->target.path);
+ retval = -1;
+ }
+
closedir(devicedir);
return retval;
--
2.1.0
6:57 a.m.
New subject: [libvirt] [PATCH 3/3] scsi: Check for invalid target.path after processLU failure
On Mon, Mar 30, 2015 at 07:16:34PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
After processing all the LU's and find no "real" LU's - check if
perhaps
the cause of that was a poorly formed 'target.path'. The expection is
generally that the path is /dev/disk/by-path or at least something in /dev.
Although it's not impossible for the user to provide something. If they
do provide something it should be valid or we should just cause a failure
to start the pool with an appropriate message.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 2f1f5ed..c3a1892 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -467,6 +467,15 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
if (!*found)
VIR_DEBUG("No LU found for pool %s", pool->def->name);
+ if (!*found && !virFileExists(pool->def->target.path) &&
+ !STRPREFIX(pool->def->target.path, "/dev")) {
Checking for *found here seems pointless. After the logic change in the
previous patch, it is implied by either of the following conditions:
a) If the target path does not start with "/dev", *found will be false:
virStorageBackendStablePath will short-circuit, just strduping
the volume path, and virStorageBackendSCSINewLun will return -1
in that case.
b) If the target path does not exist, it will either be rejected by the
above code path, or by the failed opendir in
virStorageBackendStablePath.
If all you want to do is forbid to start an {,i}SCSI pool that does not
start with /dev, we can do that much earlier in {,I}SCSIStartPool.
To catch wrong paths in /dev, I think the proper way is to stop ignoring
the return value of processLU and make it return -1 on fatal errors
(OOM, pool target path does not exist, etc.) and -2 on errors that won't
necessarily stop us from finding other volumes.
Jan
8:49 a.m.
New subject: [libvirt] [PATCH 3/3] scsi: Check for invalid target.path after processLU failure
On 03/31/2015 07:57 AM, Ján Tomko wrote:
On Mon, Mar 30, 2015 at 07:16:34PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1171933
>
> After processing all the LU's and find no "real" LU's - check if
perhaps
> the cause of that was a poorly formed 'target.path'. The expection is
> generally that the path is /dev/disk/by-path or at least something in /dev.
> Although it's not impossible for the user to provide something. If they
> do provide something it should be valid or we should just cause a failure
> to start the pool with an appropriate message.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_scsi.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 2f1f5ed..c3a1892 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -467,6 +467,15 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
> if (!*found)
> VIR_DEBUG("No LU found for pool %s", pool->def->name);
>
> + if (!*found && !virFileExists(pool->def->target.path) &&
> + !STRPREFIX(pool->def->target.path, "/dev")) {
Checking for *found here seems pointless. After the logic change in the
previous patch, it is implied by either of the following conditions:
a) If the target path does not start with "/dev", *found will be false:
virStorageBackendStablePath will short-circuit, just strduping
the volume path, and virStorageBackendSCSINewLun will return -1
in that case.
b) If the target path does not exist, it will either be rejected by the
above code path, or by the failed opendir in
virStorageBackendStablePath.
If all you want to do is forbid to start an {,i}SCSI pool that does not
start with /dev, we can do that much earlier in {,I}SCSIStartPool.
The goal was to not start one that has a non-existent pool target.path,
but where/how that's determined is a little bit more involved than other
pools which could use virFileExists() on the pool's target.path in a
Check or Refresh and decide that it's not possible to start the pool.
For iscsi, that path creation is not "managed" by libvirt, hence the
wait loop in virStorageBackendStablePath.
I suppose I could check in start/check that if the target.path doesn't
start with /dev[/], then do a virFileExists on the provided path. If
that path doesn't exist, then fail the startup. That would "solve" the
bug without messing with processLU's return values.
To catch wrong paths in /dev, I think the proper way is to stop
ignoring
the return value of processLU and make it return -1 on fatal errors
(OOM, pool target path does not exist, etc.) and -2 on errors that won't
necessarily stop us from finding other volumes.
Having processLU return 1 had more to do with distinguishing the
difference between a non disk/lun and a finding a disk/lun. What I found
was for iSCSI "found = true" was being set because of the
/sys/bus/scsi/devices/<id>/type file had 0xC (or 12 or a controller)
(<id> is the host:bus:target:lun).
The concern over wrong paths or something that doesn't start with
/dev[/] and having it be a failure is there has to be a reason a non
/dev[/] path was allowed and if we return -1 just because of that I'm
unclear what effect that has since the code is shared between scsi and
iscsi... I do agree that other failures in virStorageBackendSCSINewLun
should be differentiated.
I've made some adjustments and will repost shortly
John
3522
days inactive
3524
days old
7 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko