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@xxxxxxxxxx>
>> ---
>> 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)
>From 1ad95f18453e1c58341fdd83c8bf77ee24a78eed Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
Date: Thu, 16 Apr 2015 21:22:35 -0400
Subject: [PATCH] Change FindLUs
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
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 1b42b89..f1fc459 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -428,10 +428,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;
@@ -439,6 +438,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);
@@ -454,7 +454,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) {
@@ -464,25 +463,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)
{
@@ -569,7 +563,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 {
@@ -585,7 +579,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);
@@ -908,7 +902,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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list