[libvirt] [PATCH v2] storage: Disallow usage of the HBA for a fc_host backing
Disallow providing the wwnn/wwpn of the HBA in the adapter XML: <adapter type='fc_host' [parent='scsi_hostN'] wwnn='HBA_wwnn' wwpn='HBA_wwpn'/> This should be considered a configuration error since a vHBA would not be created. In order to use the HBA as the backing the following XML should be used: <adapter type='scsi_host' name='scsi_hostN'/> So add a check prior to the checkParent call to validate that the provided wwnn/wwpn resolves to a vHBA and not an HBA. Signed-off-by: John Ferlan <jferlan@redhat.com> --- This used to be patch 4 of the series: https://www.redhat.com/archives/libvir-list/2017-July/msg00837.html which while the series was in it's 3rd review cycle, this particular patch was new to the series and thus becomes a v2 since the three other patches from the series were pushed. docs/formatstorage.html.in | 27 +++++++++-------- src/storage/storage_backend_scsi.c | 62 +++++++++++++++++++++++++++++++++----- 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 4946ddf..27578e8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -207,18 +207,21 @@ </dl> <dl> <dt><code>wwnn</code> and <code>wwpn</code></dt> - <dd>The "World Wide Node Name" (<code>wwnn</code>) and "World Wide - Port Name" (<code>wwpn</code>) are used by the "fc_host" adapter - to uniquely identify the device in the Fibre Channel storage fabric - (the device can be either a HBA or vHBA). Both wwnn and wwpn should - be specified. Use the command 'virsh nodedev-dumpxml' to determine - how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and - wwpn have very specific numerical format requirements based on the - hypervisor being used, thus care should be taken if you decide to - generate your own to follow the standards; otherwise, the pool - will fail to start with an opaque error message indicating failure - to write to the vport_create file during vport create/delete due - to "No such file or directory". + <dd>The required "World Wide Node Name" (<code>wwnn</code>) and + "World Wide Port Name" (<code>wwpn</code>) are used by the + "fc_host" adapter to uniquely identify the vHBA device in the + Fibre Channel storage fabric. If the vHBA device already exists + as a Node Device, then libvirt will use it; otherwise, the vHBA + will be created using the provided values. It is considered a + configuration error use the values from the HBA as those would + be for a "scsi_host" <code>type</code> pool instead. The + <code>wwnn</code> and <code>wwpn</code> have very specific + format requirements based on the hypervisor being used, thus + care should be taken if you decide to generate your own to + follow the standards; otherwise, the pool will fail to start + with an opaque error message indicating failure to write to + the vport_create file during vport create/delete due to + "No such file or directory". <span class="since">Since 1.0.4</span> </dd> </dl> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index af12889..8892822 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -211,21 +211,66 @@ getAdapterName(virStorageAdapterPtr adapter) } +/** + * @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; +} + + /* * Using the host# name found via wwnn/wwpn lookup in the fc_host * sysfs tree to get the parent 'scsi_host#' to ensure it matches. */ static bool checkParent(virConnectPtr conn, - const char *name, + const char *scsi_host_name, const char *parent_name) { unsigned int host_num; - char *scsi_host_name = NULL; char *vhba_parent = NULL; bool retval = false; - VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); + VIR_DEBUG("conn=%p, scsi_host_name=%s, parent_name=%s", + conn, scsi_host_name, parent_name); /* autostarted pool - assume we're OK */ if (!conn) @@ -245,9 +290,6 @@ checkParent(virConnectPtr conn, goto cleanup; } - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) - goto cleanup; - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) goto cleanup; @@ -255,7 +297,7 @@ checkParent(virConnectPtr conn, 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); + parent_name, vhba_parent, scsi_host_name); goto cleanup; } @@ -263,7 +305,6 @@ checkParent(virConnectPtr conn, cleanup: VIR_FREE(vhba_parent); - VIR_FREE(scsi_host_name); return retval; } @@ -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; + /* If a parent was provided, let's make sure the 'name' we've * retrieved has the same parent. If not this will cause failure. */ if (!fchost->parent || checkParent(conn, name, fchost->parent)) @@ -333,6 +378,7 @@ createVport(virConnectPtr conn, cleanup: VIR_FREE(name); + VIR_FREE(scsi_host_name); return ret; } -- 2.9.4
+/** + * @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
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
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
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
One would hope it doesn't fail...
Even if it does, virSCSIHostGetNumber does report an error already.
Your suggestions make checkName much cleaner:
if (virSCSIHostGetNumber(name, &host_num) && virVHBAIsVportCapable(NULL, host_num)) return true;
if virSCSIHostGetNumber fails, it will report an error on its own but you'll also format the one below which wouldn't be true, because at that point you still don't know whether wwnn/wwpn refer to either an HBA or a vHBA
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
I'm fine with ^this change. Erik
participants (2)
-
Erik Skultety -
John Ferlan