12:29 p.m.
v1 here:
http://www.redhat.com/archives/libvir-list/2015-March/msg01562.html
changes:
1/6: Removed the virGetLastError() == NULL check
2/6: New - reading more closely showed an error path without a goto
3/6: New - Adjust return for virStorageBackendSCSINewLun to be able
to differentiate real error vs. deciding to return because
a stable path could not be determined
4/6: New - Found an unused variable in processLU
5/6: Adjust the logic to return -1 for "real" errors and -2 for errors
that are "recoverable". Removed the need for goto cleanup.
6/6: Add check for non-existent stable path at startup (and fail). Add
check during refresh for a found non standard stable path that returns
no volumes in the pool
John Ferlan (6):
iscsi: Use error message from virStorageBackendSCSIFindLUs
iscsi: Fix exit path for virStorageBackendISCSIFindLUs failure
scsi: Adjust return value for virStorageBackendSCSINewLun
scsi: Remove unused 'type_path' in processLU
scsi: Adjust return values from processLU
iscsi: Add checks for non standard stable target.path
src/storage/storage_backend_iscsi.c | 33 ++++++++++++++++-----
src/storage/storage_backend_scsi.c | 59 ++++++++++++++++++++++++-------------
2 files changed, 64 insertions(+), 28 deletions(-)
--
2.1.0
12:29 p.m.
New subject: [libvirt] [PATCH v2 1/6] iscsi: Use error message from virStorageBackendSCSIFindLUs
Don't supercede the error message virStorageBackendSCSIFindLUs as the
message such as "error: Failed to find LUs on host 60: ..." is not overly
clear as to what the real problem might be.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 079c767..48f0957 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -146,11 +146,8 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
retval = -1;
}
- if (virStorageBackendSCSIFindLUs(pool, host) < 0) {
- virReportSystemError(errno,
- _("Failed to find LUs on host %u"), host);
+ if (virStorageBackendSCSIFindLUs(pool, host) < 0)
retval = -1;
- }
VIR_FREE(sysfs_path);
--
2.1.0
5:32 a.m.
New subject: [libvirt] [PATCH v2 1/6] iscsi: Use error message from virStorageBackendSCSIFindLUs
On Wed, Apr 01, 2015 at 01:29:06PM -0400, John Ferlan wrote:
Don't supercede the error message virStorageBackendSCSIFindLUs as
the
message such as "error: Failed to find LUs on host 60: ..." is not overly
clear as to what the real problem might be.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
ACK
Jan
12:29 p.m.
New subject: [libvirt] [PATCH v2 2/6] iscsi: Fix exit path for virStorageBackendISCSIFindLUs failure
If the call to virStorageBackendISCSIGetHostNumber failed, we set
retval = -1, but yet still called virStorageBackendSCSIFindLUs.
Need to add a goto cleanup - while at it, adjust the logic to
initialize retval to -1 and only changed to 0 (zero) on success.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 48f0957..fba037f 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -131,23 +131,27 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
const char *session)
{
char *sysfs_path;
- int retval = 0;
+ int retval = -1;
uint32_t host;
if (virAsprintf(&sysfs_path,
"/sys/class/iscsi_session/session%s/device", session) <
0)
- return -1;
+ goto cleanup;
if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0) {
virReportSystemError(errno,
_("Failed to get host number for iSCSI session "
"with path '%s'"),
sysfs_path);
- retval = -1;
+ goto cleanup;
}
if (virStorageBackendSCSIFindLUs(pool, host) < 0)
- retval = -1;
+ goto cleanup;
+
+ retval = 0;
+
+ cleanup:
VIR_FREE(sysfs_path);
--
2.1.0
5:32 a.m.
New subject: [libvirt] [PATCH v2 2/6] iscsi: Fix exit path for virStorageBackendISCSIFindLUs failure
On Wed, Apr 01, 2015 at 01:29:07PM -0400, John Ferlan wrote:
If the call to virStorageBackendISCSIGetHostNumber failed, we set
retval = -1, but yet still called virStorageBackendSCSIFindLUs.
Need to add a goto cleanup - while at it, adjust the logic to
initialize retval to -1 and only changed to 0 (zero) on success.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
ACK
Jan
12:29 p.m.
New subject: [libvirt] [PATCH v2 3/6] scsi: Adjust return value for virStorageBackendSCSINewLun
Add a return -2 to differentiate that the failure was a result of a non
stable device path found or some other real error which would be messaged
in some manner.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 58e7e6d..6def373 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
+ */
static int
virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
uint32_t host ATTRIBUTE_UNUSED,
@@ -202,7 +212,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
VIR_DEBUG("No stable path found for '%s' in '%s'",
devpath, pool->def->target.path);
- retval = -1;
+ retval = -2;
goto free_vol;
}
--
2.1.0
6:31 a.m.
New subject: [libvirt] [PATCH v2 3/6] scsi: Adjust return value for virStorageBackendSCSINewLun
On Wed, Apr 01, 2015 at 01:29:08PM -0400, John Ferlan wrote:
Add a return -2 to differentiate that the failure was a result of a
non
stable device path found or some other real error which would be messaged
in some manner.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 58e7e6d..6def373 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
+ */
static int
virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
uint32_t host ATTRIBUTE_UNUSED,
@@ -202,7 +212,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
VIR_DEBUG("No stable path found for '%s' in '%s'",
devpath, pool->def->target.path);
- retval = -1;
+ retval = -2;
This mixes two different errors:
* virStorageBackendStablePath short-circuited based on pool target path
that's just as fatal as OOM and the attempt to find other volumes
will also fail
* virStorageBackendStablePath fell through to ret_strdup -
the directory exists, but a symlink to the device did not show up
This means the device could appear later, so it makes sense to retry
later.
And it doesn't handle the opendir failure in
virStorageBackendStablePath, which could also mean that the device will
appear later.
Given that this check here has to negate the !STREQ "/dev" checks in
virStorageBackendStablePath, maybe virStorageBackendStablePath should be
split up to two functions - one that returns an error when the device
path can't be stabilized and the other that would strdup the original path?
Jan
8:05 a.m.
New subject: [libvirt] [PATCH v2 3/6] scsi: Adjust return value for virStorageBackendSCSINewLun
On 04/02/2015 07:31 AM, Ján Tomko wrote:
On Wed, Apr 01, 2015 at 01:29:08PM -0400, John Ferlan wrote:
> Add a return -2 to differentiate that the failure was a result of a non
> stable device path found or some other real error which would be messaged
> in some manner.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_scsi.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 58e7e6d..6def373 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
> + */
> static int
> virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> uint32_t host ATTRIBUTE_UNUSED,
> @@ -202,7 +212,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> VIR_DEBUG("No stable path found for '%s' in
'%s'",
> devpath, pool->def->target.path);
>
> - retval = -1;
> + retval = -2;
This mixes two different errors:
* virStorageBackendStablePath short-circuited based on pool target path
that's just as fatal as OOM and the attempt to find other volumes
will also fail
* virStorageBackendStablePath fell through to ret_strdup -
the directory exists, but a symlink to the device did not show up
This means the device could appear later, so it makes sense to retry
later.
And it doesn't handle the opendir failure in
virStorageBackendStablePath, which could also mean that the device will
appear later.
Given that this check here has to negate the !STREQ "/dev" checks in
virStorageBackendStablePath, maybe virStorageBackendStablePath should be
split up to two functions - one that returns an error when the device
path can't be stabilized and the other that would strdup the original path?
True - the real bugger is virStorageBackendStablePath - since it's
shared code, my goal here was to minimize impact to the [i]SCSI paths.
In any case, I've pushed the 3 ACK'd patches and will see what I can
come up with for StablePath checking.
Tks -
John
12:29 p.m.
New subject: [libvirt] [PATCH v2 4/6] scsi: Remove unused 'type_path' in processLU
Seems to be a remnant that was never cleaned up from original submit...
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 6def373..e085da2 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -382,7 +382,6 @@ processLU(virStoragePoolObjPtr pool,
uint32_t target,
uint32_t lun)
{
- char *type_path = NULL;
int retval = -1;
int device_type;
char *block_device = NULL;
@@ -427,8 +426,6 @@ processLU(virStoragePoolObjPtr pool,
VIR_DEBUG("Created new storage volume for %u:%u:%u:%u successfully",
host, bus, target, lun);
- VIR_FREE(type_path);
-
out:
VIR_FREE(block_device);
return retval;
--
2.1.0
5:33 a.m.
New subject: [libvirt] [PATCH v2 4/6] scsi: Remove unused 'type_path' in processLU
On Wed, Apr 01, 2015 at 01:29:09PM -0400, John Ferlan wrote:
Seems to be a remnant that was never cleaned up from original
submit...
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 3 ---
1 file changed, 3 deletions(-)
ACK
Jan
12:29 p.m.
New subject: [libvirt] [PATCH v2 5/6] scsi: Adjust return values from processLU
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 (well) no apparent reason. It will also only set the *found value when
at least one of the processLU's was successful.
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 e085da2..8087960 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -375,6 +375,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,
@@ -393,7 +403,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
@@ -401,32 +411,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;
}
@@ -460,6 +463,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;
@@ -467,7 +472,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
12:29 p.m.
New subject: [libvirt] [PATCH v2 6/6] iscsi: Add checks for non standard stable target.path
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
If a non stable path is provided for the pool's target path, check to
see if the directory exists before allowing pool startup; otherwise,
later in the processLU calls to find LUN's all that happens is the
volume target.path will get the strdup'd value of the pool target.path
(which doesn't exist), so attempts to find the LU are unsuccessful
resulting in a started pool with no devices listed even though the
block devices for the iSCSI LU's do exist.
Additionally if the non stable path does exist and it's determined no
targets are found, then force failure in the refresh path.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index fba037f..b5a15b1 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -149,6 +149,15 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
if (virStorageBackendSCSIFindLUs(pool, host) < 0)
goto cleanup;
+ if (pool->volumes.count == 0 &&
+ !STRPREFIX(pool->def->target.path, "/dev")) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Non stable target path '%s' for pool '%s'
"
+ "found no target volumes"),
+ pool->def->target.path, pool->def->name);
+ return -1;
+ }
+
retval = 0;
cleanup:
@@ -393,6 +402,15 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
return -1;
}
+ if (!STRPREFIX(pool->def->target.path, "/dev") &&
+ !virFileExists(pool->def->target.path)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Non stable target path '%s' not found for pool
'%s'"),
+ pool->def->target.path, pool->def->name);
+ return -1;
+ }
+
+
if ((session = virStorageBackendISCSISession(pool, true)) == NULL) {
if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) ==
NULL)
goto cleanup;
--
2.1.0
6:46 a.m.
New subject: [libvirt] [PATCH v2 6/6] iscsi: Add checks for non standard stable target.path
On Wed, Apr 01, 2015 at 01:29:11PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
If a non stable path is provided for the pool's target path, check to
see if the directory exists before allowing pool startup; otherwise,
The only non-stable path that can possibly result in a non-empty pool is '/dev/'.
later in the processLU calls to find LUN's all that happens is
the
volume target.path will get the strdup'd value of the pool target.path
(which doesn't exist), so attempts to find the LU are unsuccessful
resulting in a started pool with no devices listed even though the
block devices for the iSCSI LU's do exist.
Additionally if the non stable path does exist and it's determined no
targets are found, then force failure in the refresh path.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index fba037f..b5a15b1 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -149,6 +149,15 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
if (virStorageBackendSCSIFindLUs(pool, host) < 0)
goto cleanup;
+ if (pool->volumes.count == 0 &&
This is always true if the path does not start with /dev.
https://www.redhat.com/archives/libvir-list/2015-March/msg01578.html
+ !STRPREFIX(pool->def->target.path, "/dev"))
{
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Non stable target path '%s' for pool '%s'
"
+ "found no target volumes"),
+ pool->def->target.path, pool->def->name);
+ return -1;
+ }
+
retval = 0;
cleanup:
@@ -393,6 +402,15 @@ virStorageBackendISCSIStartPool(virConnectPtr
conn,
return -1;
}
+ if (!STRPREFIX(pool->def->target.path, "/dev") &&
+ !virFileExists(pool->def->target.path)) {
If you remove the virFileExists condition, you don't need the error
check in the first hunk.
Jan
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Non stable target path '%s' not found for pool
'%s'"),
+ pool->def->target.path, pool->def->name);
+ return -1;
+ }
+
+
if ((session = virStorageBackendISCSISession(pool, true)) == NULL) {
if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) ==
NULL)
goto cleanup;
--
2.1.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3522
days inactive
3523
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko