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

[John Ferlan]

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