[libvirt] [PATCH v3 3/4] scsi: Adjust return value for virStorageBackendSCSINewLun

[Ján Tomko]

On Thu, Apr 16, 2015 at 07:42:55PM -0400, John Ferlan wrote:
> 
> 
> On 04/16/2015 09:14 AM, Ján Tomko wrote:
> > On Tue, Apr 07, 2015 at 04:21:02PM -0400, John Ferlan wrote:
> >> Use virStorageBackendPoolUseDevPath API to determine whether creation of
> >> stable target path is possible for the volume. If not, then return failure.
> >>
> >> This will differentiate a failed virStorageBackendStablePath which won't
> >> need to be fatal. Thus, we'll add a -2 return value to differentiate that
> >> the failure was a result of either the inability to find the symlink for
> >> the device or failure to open the target path directory
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/storage/storage_backend_scsi.c | 27 ++++++++++++++++++++++++---
> >>  1 file changed, 24 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> >> index b96caec..d3c6470 100644
> >> --- a/src/storage/storage_backend_scsi.c
> >> +++ b/src/storage/storage_backend_scsi.c
> >> @@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev)
> >>  }
> >>  
> >>  
> >> +/*
> >> + * Attempt to create a new LUN
> >> + *
> >> + * Returns:
> >> + *
> >> + *  0  => Success
> >> + *  -1 => Failure due to some sort of OOM or other fatal issue found when
> >> + *        attempting to get/update information about a found volume
> >> + *  -2 => Failure to find a stable path, not fatal, caller can try another
> >> + */
> >>  static int
> >>  virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> >>                              uint32_t host ATTRIBUTE_UNUSED,
> >> @@ -158,6 +168,18 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> >>      char *devpath = NULL;
> >>      int retval = -1;
> >>  
> >> +    /* Before we get too far - let's see if the pool is using target path
> >> +     * starting with /dev. Attempts to find a stable path not based on a
> >> +     * pool target starting with /dev will fail and do lots of unnecessary
> >> +     * work - so we'll short circuit here.
> > 
> > This also rejects the non-stable path '/dev' that was accepted before.
> > 
> 
> Not quite sure I see the issue - can you be more specific?  Am I missing
> something obvious?
> 
> Previously if "pool->def->target.path" is "/dev" (or NULL or "/dev/" or
> didn't start with "/dev"), then we'd return a strdup'd 'devpath' into
> 'vol->target.path'.
> 
> I see no way 'devpath' could be '/dev' or '/dev/' since it is formatted as :
> 
>      if (virAsprintf(&devpath, "/dev/%s", dev) < 0)
>          goto cleanup;

That's the path to the device. The comment above is right about checking
the pool target path in the pool definition.

After this patch, if pool->def->target.path is "/dev", we wouldn't even
get to the virStorageBackendStablePath call, because of the check in
virStorageBackendPoolUseDevPath - it will return false, we'll jump to
cleanup and skip over this volume.

Jan

> 
> where 'dev' is an argument to this function as found by processLU and
> cannot be NULL from a call:
> 
>     if (getBlockDevice(host, bus, target, lun, &block_device) < 0) {
>         VIR_DEBUG("Failed to find block device for this LUN");
>         goto out;
> 
> which fills 'block_device' from either getNewStyleBlockDevice or
> getOldStyleBlockDevice.
> 
> John
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/a89ace20/attachment-0001.sig>