[libvirt] [PATCH v3 4/4] scsi: Adjust return values from processLU

[Ján Tomko]

On Thu, Apr 16, 2015 at 09:24:41PM -0400, John Ferlan wrote:
> 
> 
> 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.
> 

Okay, I was just thinking out loud.

> >> + *  -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.
> 

They suggest sarcasm to me, but maybe it's just me.

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

Using separate variables for the return value of the current function
and the return value of called functions makes the flow easier to
follow. IIRC we've had some bugs caused by a leftover returned value
from an earlier function call.

> >>          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 haven't looked at all the other callers, but it does seems 

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

I don't think it's worth the effort - especially since we ignore
the failure anyway.

> John
> 
> (attached a possible diff)

Looks good to me.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150417/e9b15fd1/attachment-0001.sig>