[libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing

[John Ferlan]

On 07/25/2017 03:45 AM, Erik Skultety wrote:
>> +/**
>> + * @name: Name from a wwnn/wwpn lookup
>> + *
>> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
>> + * an HBA as that should be a configuration error. It's only possible to
>> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
>> + * have created using the node device create functionality. Using the
>> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
>> + * vHBA is just not valid
>> + *
>> + * Returns the @scsi_host_name to be used or NULL on errror
>> + */
>> +static char *
>> +checkName(const char *name)
>> +{
>> +    char *scsi_host_name = NULL;
>> +    unsigned int host_num;
>> +
>> +    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +        return NULL;
>> +
>> +    if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("host name '%s' is not properly formatted"),
>> +                       name);
>> +        goto error;
>> +    }
>> +
>> +    /* If scsi_host_name is vport capable, then it's an HBA. This is
>> +     * a configuration error as the wwnn/wwpn should only be for a vHBA */
>> +    if (virVHBAIsVportCapable(NULL, host_num)) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("the wwnn/wwpn for '%s' are assigned to an HBA"),
>> +                       scsi_host_name);
>> +        goto error;
>> +    }
>> +
>> +    return scsi_host_name;
>> +
>> + error:
>> +    VIR_FREE(scsi_host_name);
>> +    return NULL;
>> +}
>> +
> 
> ...
> 
>> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
>>              virStorageAdapterFCHostPtr fchost)
>>  {
>>      char *name = NULL;
>> +    char *scsi_host_name = NULL;
>>      virStoragePoolFCRefreshInfoPtr cbdata = NULL;
>>      virThread thread;
>>      int ret = -1;
>> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
>>       * this pool and we don't have to create the vHBA
>>       */
>>      if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> +        if (!(scsi_host_name = checkName(name)))
>> +            goto cleanup;
> 
> Ok, so I learn from my mistakes, do you plan on building upon this in the
> foreseeable future with a follow-up series you haven't sent yet, but have on a
> local branch? Because if not, I don't really see a point in returning a string
> which only gets free'd on the function exit - checkName should probably become
> something like "isVHBA" and return boolean. And even if you do have a follow-up
> on a local branch, I still think that converting the return value should be
> part of that series, solely because until then, @scsi_host_name wouldn't have
> any meaning.
> 
> Erik
> 

No this is it - I want to stop thinking about NPIV for a while... I
returned the 'scsi_host_name' string because in my mind I had passed it
to checkParent, but apparently I didn't do that, sigh.

Does that make more sense now?

John