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

[Michal Privoznik]

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