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

[John Ferlan]

On 07/25/2017 08:49 AM, Erik Skultety wrote:
> On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
>>
>>
>> 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
> 
> s/errror/error
> 
> ...
> 

OK

>>>> @@ -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?
> 
> Indeed, to be honest I missed the connection between name and scsi_host_name
> and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
> myself)....now that I see it a bit more clearly, it still left me wondering if
> it wouldn't make more sense to move the scsi_host_name formatting part to
> createVport (you free it here after all) right after you pass @name to

That's fine...

> checkName/isVHBA (whatever we settle on):
>     - you don't need to format it prior to your checkName, since backwards
>       compatibility takes care of eating "hostX" nicely
>     - you also don't need to report any error when virSCSIHostGetNumber fails,
>       since one gets formatted already and it didn't bring much value

One would hope it doesn't fail...

Your suggestions make checkName much cleaner:

    if (virSCSIHostGetNumber(name, &host_num) &&
        virVHBAIsVportCapable(NULL, host_num))
        return true;

    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                   _("the wwnn/wwpn for '%s' are assigned to an HBA"),
                    name);
    return false;

and only add :

    if (!(checkName(name)))
        goto cleanup

to the createVport

The @name to @scsi_host_name can return to checkParent.  I retest and
repost shortly.

Tks -

John

> 
> -> then, right after that call you actually format the "scsi_" name since you
> really need it to traverse the list of devices and find the parent in
> checkParent.
> 
> Erik
> 
>>
>> John