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 ...
@@ -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 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 -> 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