[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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 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)


>From 1ad95f18453e1c58341fdd83c8bf77ee24a78eed Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan redhat com>
Date: Thu, 16 Apr 2015 21:22:35 -0400
Subject: [PATCH] Change FindLUs

Signed-off-by: John Ferlan <jferlan 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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]