[libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup

[John Ferlan]

On 11/11/2014 10:05 AM, Michal Privoznik wrote:
> On 11.11.2014 13:38, John Ferlan wrote:
>>
>>
>> On 11/11/2014 07:21 AM, Michal Privoznik wrote:
>>> On 10.11.2014 23:45, John Ferlan wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1160565
>>>>
>>>> If a 'parent' attribute is provided for the fchost, then at startup
>>>> time check to ensure it is a vport capable scsi_host. If the parent
>>>> is not vport capable, then disallow the startup. The following is the
>>>> expected results:
>>>>
>>>> error: Failed to start pool fc_pool
>>>> error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable
>>>>
>>>> where the XML for the fc_pool is:
>>>>
>>>>       <pool type='scsi'>
>>>>         <name>fc_pool</name>
>>>>         <source>
>>>>           <adapter type='fc_host' parent='scsi_host2' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/>
>>>>         </source>
>>>> ...
>>>>
>>>> and 'scsi_host2' is not vport capable.
>>>>
>>>> Providing an incorrect parent and a correct wwnn/wwpn could lead to
>>>> failures at shutdown (deleteVport) where the assumption is the parent
>>>> is for the fchost.
>>>>
>>>> NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
>>>>         then we will be creating one with code (virManageVport) which
>>>>         assumes the parent is vport capable.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>    src/storage/storage_backend_scsi.c | 22 ++++++++++++++++++----
>>>>    1 file changed, 18 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>>>> index 02160bc..549d8db 100644
>>>> --- a/src/storage/storage_backend_scsi.c
>>>> +++ b/src/storage/storage_backend_scsi.c
>>>> @@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
>>>>        if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>>>>            return 0;
>>>>
>>>> +    /* If a parent was provided, then let's make sure it's vhost capable */
>>>> +    if (adapter.data.fchost.parent) {
>>>> +        if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
>>>> +            return -1;
>>
>> ^^^ [1] ^^^
>>>> +
>>>> +        if (!virIsCapableFCHost(NULL, parent_host)) {
>>>> +            virReportError(VIR_ERR_XML_ERROR,
>>>> +                           _("parent '%s' specified for vHBA "
>>>> +                             "is not vport capable"),
>>>> +                           adapter.data.fchost.parent);
>>>> +            return -1;
>>>> +        }
>>>> +    }
>>>> +
>>>>        /* This filters either HBA or already created vHBA */
>>>>        if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
>>>>                                          adapter.data.fchost.wwpn))) {
>>>> @@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
>>>>
>>>>        if (!adapter.data.fchost.parent &&
>>>>            !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
>>>> -         virReportError(VIR_ERR_XML_ERROR, "%s",
>>>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>>>>                           _("'parent' for vHBA not specified, and "
>>>>                             "cannot find one on this host"));
>>>>             return -1;
>>>> -    }
>>>>
>>>> -    if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
>>>> -        return -1;
>>>> +        if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
>>>> +            return -1;
>>>> +    }
>>>
>>> This chunk seems odd. After the error is reported, the control jumps out
>>> from the function, so virGetSCSIHostNumer is not called at all. Did you
>>> forget to commit something?
>>>
>>
>> The earlier chunk of code where the parent exists, we figure the
>> parent_host value. [1]
>>
>> This chunk is - if a parent wasn't provided, find a capable vport, then
>> get the parent_host value.
>>
>> I moved it inside the if because it makes no sense to call the function
>> twice in the event we had a parent value..
> 
> My point is, when the conditions are met, and the error is reported the 
> control jumps out of the function right after virReportError(). That's 
> because there's 'return -1' after that. However, the next line in the 
> same block is yet another function call. This, however, will never be 
> called so it's a dead code. Hence my question.
> 
> Michal
> 

Doh!  Of course as you'll find in later patches the logic is adjusted
and thus where my brain was already at.  I'll fix this one though to be:

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc
index 549d8db..88928c9 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -577,12 +577,13 @@ createVport(virStoragePoolSourceAdapter adapter)
         return 0;
     }
 
-    if (!adapter.data.fchost.parent &&
-        !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("'parent' for vHBA not specified, and "
-                         "cannot find one on this host"));
-         return -1;
+    if (!adapter.data.fchost.parent) {
+        if (!(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("'parent' for vHBA not specified, and "
+                             "cannot find one on this host"));
+            return -1;
+        }
 
         if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
             return -1;