[libvirt] [PATCH v2 3/3] conf: Fix vHBA checkParent logic for pool creation

[John Ferlan]

On 07/19/2017 09:21 AM, Erik Skultety wrote:
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index 365ab77..d225e4f 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -220,6 +220,7 @@ checkParent(virConnectPtr conn,
>>              const char *name,
>>              const char *parent_name)
>>  {
>> +    unsigned int host_num;
>>      char *scsi_host_name = NULL;
>>      char *vhba_parent = NULL;
>>      bool retval = false;
>> @@ -230,20 +231,52 @@ checkParent(virConnectPtr conn,
>>      if (!conn)
>>          return true;
>>
>> -    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +    if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       _("parent '%s' is not properly formatted"), name);
>>          goto cleanup;
>> +    }
>>
>> -    if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
>> +    if (!virVHBAPathExists(NULL, host_num)) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       ("parent '%s' is not a vHBA/HBA"), parent_name);
> 
>     Under what circumstances can parent be a ^^^^vHBA??
> 

The function tests whether "/sys/class/fc_host/host%d" exists... On my
system:

ls /sys/class/fc_host
host3  host4  host8

where host3 and host4 are HBA and host8 is vHBA

The difference between them?  host3/host4 have files "vport_create",
"vport_delete", "max_npiv_vports", "npiv_vports_inuse", and "issue_lip".

The XML for an HBA would be:

    <adapter type='fc_host' [parent='scsi_host3'] wwnn='HBA_wwnn'
wwpn='HBA_wwpn'/>

and the vHBA:

    <adapter type='fc_host' [parent='scsi_host3'] wwnn='vHBA_wwnn'
wwpn='vHBA_wwpn'/>

Both are legal XML.

>>          goto cleanup;
>> +    }
>>
>> -    if (STRNEQ(parent_name, vhba_parent)) {
>> -        virReportError(VIR_ERR_XML_ERROR,
>> -                       _("Parent attribute '%s' does not match parent '%s' "
>> -                         "determined for the '%s' wwnn/wwpn lookup."),
>> -                       parent_name, vhba_parent, name);
>> +    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +        goto cleanup;
>> +
>> +    if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("host name '%s' is not properly formatted"), name);
>>          goto cleanup;
>>      }
>>
>> +    /* If scsi_host_name is vport capable, then it's an HBA, thus compare
>> +     * only against the parent_name; otherwise, as long as the scsi_host_name
>> +     * path exists, then the scsi_host_name is a vHBA in which case we need
>> +     * to compare against it's parent. */
>> +    if (virVHBAIsVportCapable(NULL, host_num)) {
> 
> I truly find it pointless to try to do any kind of parent validation on HBA
> device. Can a HBA device actually have a scsi_host parent? If not, then we
> should only validate whether the device given by its wwnn and wwpn exist (we
> already know that since we're here) is vport capable and then if the 'parent'
> attribute has been defined, then return an error about invalid configuration,
> rather than trying to compare parent names in that matter.
> 
> Erik
> 

vport capability is essentially the presence of "vport_create" in the
/sys/class/fc_host/host# or /sys/class/scsi_host/host# path.

An HBA has some sort of a PCI_* parent based on it's address.

For this particular bug, someone provided a parent and it was wrong, so
that's what's being checked.

Although I agree the insanity of the checks for someone providing wrong
data is annoying; however, if not checked then it was possible to create
a storage pool backed to the HBA defined by the wwnn/wwpn provided;
however, when it comes time to destroy it, the code ended up calling the
VPORT_DELETE and obviously failing, causing the command to fail to
remove the pool. And there was no way to remove the pool with the
invalid parent name.  So at startup, let's make sure if the parent name
is provided that it's correct.

These start checks essentially mimic the destroy tests. One could argue
though that the failure paths in shutdown for virSCSIHostGetNumber and
virNodeDeviceGetParentName should never happen, but assuming that is
paramount to someone finding a way to make it happen.

John