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

[John Ferlan]

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..

John


>>
>>       if (virManageVport(parent_host, adapter.data.fchost.wwpn,
>>                          adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
>>
> 
> Michal
>