[libvirt] [PATCH v3 4/4] scsi: Adjust return values from processLU
John Ferlan
jferlan at redhat.com
Fri Apr 17 01:24:41 UTC 2015
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 at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Change-FindLUs.patch
Type: text/x-patch
Size: 3484 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150416/0364e38d/attachment-0001.bin>
More information about the libvir-list
mailing list