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

[Ján Tomko]

On Tue, Apr 21, 2015 at 06:18:56AM -0400, John Ferlan wrote:
> 
> 
> On 04/21/2015 03:13 AM, Ján Tomko wrote:
> > 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.
> > 
> 
> Does the following squashed in work for you?  Essentially restoring the
> /dev || /dev/ check after virStorageBackendStablePath and adding it to
> the virStorageBackendPoolPathIsStable ?
> 
> iff --git a/src/storage/storage_backend_scsi.c
> b/src/storage/storage_backend_scsi.c
> index ae3cd9a..b97b2b0 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -175,7 +175,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>       * 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)) {
> +    if (!virStorageBackendPoolPathIsStable(pool->def->target.path) &&
> +        !(STREQ(pool->def->target.path, "/dev") ||
> +          STREQ(pool->def->target.path, "/dev/"))) {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("unable to use target path '%s' for dev '%s'"),
>                         NULLSTR(pool->def->target.path), dev);
> @@ -211,7 +213,9 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>                                                          true)) == NULL)
>          goto cleanup;
> 
> -    if (STREQ(devpath, vol->target.path)) {
> +    if (STREQ(devpath, vol->target.path) &&
> +        !(STREQ(pool->def->target.path, "/dev") ||
> +          STREQ(pool->def->target.path, "/dev/"))) {
> 
>          VIR_DEBUG("No stable path found for '%s' in '%s'",
>                    devpath, pool->def->target.path);
> 

ACK

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/20150421/220ddbf8/attachment-0001.sig>