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

[Michal Privoznik]

On 11.11.2014 16:21, John Ferlan wrote:
>
>
> 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;
>

ACK with this squashed in.

Michal