[libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun

[Ján Tomko]

On Mon, Apr 20, 2015 at 12:50:07PM -0400, John Ferlan wrote:
> 
> ...
> 
> >> +    /* Check if the pool is using a stable target path. The call to
> >> +     * virStorageBackendStablePath will fail if the pool target path
> >> +     * isn't stable and just return the strdup'd 'devpath' anyway.
> >> +     * This would be indistinguishable to failing to find the stable
> >> +     * path to the device if the virDirRead loop to search the
> >> +     * target pool path for our devpath had failed.
> >> +     */
> >> +    if (!virStorageBackendPoolPathIsStable(pool->def->target.path)) {
> >> +        virReportError(VIR_ERR_INVALID_ARG,
> >> +                       _("unable to use target path '%s' for dev '%s'"),
> >> +                       NULLSTR(pool->def->target.path), dev);
> >> +        goto cleanup;
> >> +    }
> > 
> > /dev is a valid non-stable pool target path.
> > 
> >> +
> >>      if (VIR_ALLOC(vol) < 0)
> >>          goto cleanup;
> >>  
> >> @@ -187,13 +211,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
> >>                                                          true)) == NULL)
> >>          goto cleanup;
> >>  
> >> -    if (STREQ(devpath, vol->target.path) &&
> >> -        !(STREQ(pool->def->target.path, "/dev") ||
> >> -          STREQ(pool->def->target.path, "/dev/"))) {
> >> +    if (STREQ(devpath, vol->target.path)) {
> >>  
> > 
> > Before, when virStorageBackendStablePath returned the same devpath because
> > the pool path was "/dev", we continued with processing the volume.
> > 
> > After this patch, we won't even get here because of the first check.
> > 
> > Failure to stabilize the path should be expected here, if the pool
> > target path is not stable.
> > 
> 
> OK, but because virStorageBackendStablePath won't process the
> pool->target.path of "/dev", we'll return the duplicated 'devpath' and
> return -2 for every volume in the pool thus making it look empty.
> 
> What good is that?
> 

None. We should process the volume instead of returning -2.

Jan

> Wouldn't it be better to tell the user that "/dev" is not a valid stable
> path name... The path really needs to be more specific... I suppose one
> could change virStorageBackendStablePath to accept "/dev" and do the
> search, but that "could" take a while depending on the size of the
> "/dev" file system.
> 
> 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/20150421/1bd564b8/attachment-0001.sig>