Re: [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&gt; > --- > 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.