Re: [libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
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