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(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? > 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.

>> - 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)