[libvirt] [PATCH v3 0/4] Fix a couple issues found w/ vHBA logic
v2: https://www.redhat.com/archives/libvir-list/2017-July/msg00661.html Changes since v2: * Patch 1 - make the requested logic adjustment * Patch 2 - no change * Patch 3 - make this *just* the check that the provided @parent_name is a valid fc_host (whether it's vHBA or HBA). * Patch 4 - Alter the checkParent logic to cause a failure if the passed @name (from the wwnn/wwpn's provided) are for an HBA instead of a vHBA. Make this an invalid configuration. Modify the docs to describe that the provided wwnn/wwpn must be for an existing vHBA, are to be used to create a vHBA, but cannot be from an existing HBA. NB: No change to the Destroy code since it's possible one of these exists and there needs to be a way to shut it down. The failure could turn into a "conversion" of sorts as well by changing the type to 'scsi_host', saving the generated scsi_host name, removing the parent and wwnn & wwpn fields. Tested using my vHBA system with various valid and invalid configurations. John Ferlan (4): storage: Fix existing parent check for vHBA creation storage: Remove @conn from virNodeDeviceCreateVport storage: Check if provided parent is vHBA capable storage: Disallow usage of the HBA for a fc_host backing docs/formatstorage.html.in | 27 ++++++----- src/conf/node_device_conf.c | 63 ++----------------------- src/conf/node_device_conf.h | 3 +- src/storage/storage_backend_scsi.c | 94 +++++++++++++++++++++++++++++++++++++- 4 files changed, 112 insertions(+), 75 deletions(-) -- 2.9.4
https://bugzilla.redhat.com/show_bug.cgi?id=1472277 Commit id '106930aaa' altered the order of checking for an existing vHBA (e.g something created via nodedev-create functionality outside of the storage pool logic) which inadvertantly broke the code to decide whether to alter/force the fchost->managed field to be 'yes' because the storage pool will be managing the created vHBA in order to ensure when the storage pool is destroyed that the vHBA is also destroyed. This patch moves the check (and checkParent helper) for an existing vHBA back into the createVport in storage_backend_scsi. It also adjusts the checkParent logic to more closely follow the intentions prior to commit id '79ab0935'. The changes made by commit id '08c0ea16f' are only necessary to run the virStoragePoolFCRefreshThread when a vHBA was really created because there's a timing lag such that the refreshPool call made after a startPool from storagePoolCreate* wouldn't necessarily find LUNs, but the thread would. For an already existing vHBA, using the thread is unnecessary since the vHBA already exists and the lag to configure the LUNs wouldn't exist. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 55 -------------------------------------- src/storage/storage_backend_scsi.c | 54 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 55 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 503b129..9c0ffa5 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2259,48 +2259,6 @@ virNodeDeviceGetParentName(virConnectPtr conn, } -/* - * 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 *parent_name) -{ - 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); - - /* autostarted pool - assume we're OK */ - if (!conn) - return true; - - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) - goto cleanup; - - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) - goto cleanup; - - if (STRNEQ(parent_name, vhba_parent)) { - 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); - goto cleanup; - } - - retval = true; - - cleanup: - VIR_FREE(vhba_parent); - VIR_FREE(scsi_host_name); - return retval; -} - - /** * @conn: Connection pointer * @fchost: Pointer to vHBA adapter @@ -2326,19 +2284,6 @@ virNodeDeviceCreateVport(virConnectPtr conn, VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'", conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); - /* If we find an existing HBA/vHBA within the fc_host sysfs - * using the wwnn/wwpn, then a nodedev is already created for - * this pool and we don't have to create the vHBA - */ - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - /* 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)) - VIR_FREE(name); - - return name; - } - if (fchost->parent) { if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) goto cleanup; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index f7378d3..e6aa643 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -211,6 +211,48 @@ getAdapterName(virStorageAdapterPtr adapter) } +/* + * 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 *parent_name) +{ + 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); + + /* autostarted pool - assume we're OK */ + if (!conn) + return true; + + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) + goto cleanup; + + if (STRNEQ(parent_name, vhba_parent)) { + 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); + goto cleanup; + } + + retval = true; + + cleanup: + VIR_FREE(vhba_parent); + VIR_FREE(scsi_host_name); + return retval; +} + + static int createVport(virConnectPtr conn, virStoragePoolDefPtr def, @@ -226,6 +268,18 @@ createVport(virConnectPtr conn, conn, NULLSTR(configFile), NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); + /* If we find an existing HBA/vHBA within the fc_host sysfs + * using the wwnn/wwpn, then a nodedev is already created for + * this pool and we don't have to create the vHBA + */ + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { + /* 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)) + ret = 0; + + goto cleanup; + } /* Since we're creating the vHBA, then we need to manage removing it * as well. Since we need this setting to "live" through a libvirtd -- 2.9.4
It's no longer needed since the checkParent code moved back to storage_backend_scsi.c Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 8 +++----- src/conf/node_device_conf.h | 3 +-- src/storage/storage_backend_scsi.c | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 9c0ffa5..82f02fa 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2260,7 +2260,6 @@ virNodeDeviceGetParentName(virConnectPtr conn, /** - * @conn: Connection pointer * @fchost: Pointer to vHBA adapter * * Create a vHBA for Storage. This code accomplishes this via searching @@ -2273,16 +2272,15 @@ virNodeDeviceGetParentName(virConnectPtr conn, * Returns vHBA name on success, NULL on failure with an error message set */ char * -virNodeDeviceCreateVport(virConnectPtr conn, - virStorageAdapterFCHostPtr fchost) +virNodeDeviceCreateVport(virStorageAdapterFCHostPtr fchost) { unsigned int parent_host; char *name = NULL; char *parent_hoststr = NULL; bool skip_capable_check = false; - VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'", - conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); + VIR_DEBUG("parent='%s', wwnn='%s' wwpn='%s'", + NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); if (fchost->parent) { if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index d10683d..da56eaf 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -384,8 +384,7 @@ virNodeDeviceGetParentName(virConnectPtr conn, const char *nodedev_name); char * -virNodeDeviceCreateVport(virConnectPtr conn, - virStorageAdapterFCHostPtr fchost); +virNodeDeviceCreateVport(virStorageAdapterFCHostPtr fchost); int virNodeDeviceDeleteVport(virConnectPtr conn, diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index e6aa643..359d2d2 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -294,7 +294,7 @@ createVport(virConnectPtr conn, } } - if (!(name = virNodeDeviceCreateVport(conn, fchost))) + if (!(name = virNodeDeviceCreateVport(fchost))) goto cleanup; /* Creating our own VPORT didn't leave enough time to find any LUN's, -- 2.9.4
https://bugzilla.redhat.com/show_bug.cgi?id=1458708 If the parent provided for the storage pool adapter is not vHBA capable, then issue a configuration error even though the provided wwnn/wwpn were found. It is a configuration error to provide a mismatched parent to the wwnn/wwpn. The @parent is optional and is used as a means to perform duplicate pool source checks. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 359d2d2..af12889 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -220,6 +220,7 @@ checkParent(virConnectPtr conn, const char *name, const char *parent_name) { + unsigned int host_num; char *scsi_host_name = NULL; char *vhba_parent = NULL; bool retval = false; @@ -230,6 +231,20 @@ checkParent(virConnectPtr conn, if (!conn) return true; + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not properly formatted"), + parent_name); + goto cleanup; + } + + if (!virVHBAPathExists(NULL, host_num)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("parent '%s' is not an fc_host for the wwnn/wwpn"), + parent_name); + goto cleanup; + } + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) goto cleanup; -- 2.9.4
On Thu, Jul 20, 2017 at 03:48:48PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1458708
If the parent provided for the storage pool adapter is not vHBA capable, then issue a configuration error even though the provided wwnn/wwpn were found.
It is a configuration error to provide a mismatched parent to the wwnn/wwpn. The @parent is optional and is used as a means to perform duplicate pool source checks.
So far, ACK to 1-3. Erik
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'/> This also alters the caller such that the @parent_name param into checkParent can be NULL so as to confirm that at least the provided wwnn/wwpn found a vHBA instead of an HBA. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 27 +++++++++++++---------- src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 23 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..c802738 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn, if (!conn) return true; - if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("parent '%s' is not properly formatted"), - parent_name); + /* If there's a parent_name, then make sure it's valid */ + if (parent_name) { + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not properly formatted"), + parent_name); + goto cleanup; + } + + if (!virVHBAPathExists(NULL, host_num)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("parent '%s' is not an fc_host for the wwnn/wwpn"), + parent_name); + goto cleanup; + } + } + + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("host name '%s' is not properly formatted"), + name); goto cleanup; } - if (!virVHBAPathExists(NULL, host_num)) { + /* 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, - _("parent '%s' is not an fc_host for the wwnn/wwpn"), - parent_name); + _("the wwnn/wwpn for '%s' are assigned to an HBA"), + scsi_host_name); goto cleanup; } - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + /* No parent name, then no need to get/compare against vhba_parent */ + if (!parent_name) { + retval = true; goto cleanup; + } if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) goto cleanup; @@ -288,9 +313,7 @@ createVport(virConnectPtr conn, * this pool and we don't have to create the vHBA */ if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - /* 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)) + if (checkParent(conn, name, fchost->parent)) ret = 0; goto cleanup; -- 2.9.4
On Thu, Jul 20, 2017 at 03:48:49PM -0400, John Ferlan wrote:
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'/>
This also alters the caller such that the @parent_name param into checkParent can be NULL so as to confirm that at least the provided wwnn/wwpn found a vHBA instead of an HBA.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 27 +++++++++++++---------- src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 23 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..c802738 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn, if (!conn) return true;
- if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("parent '%s' is not properly formatted"), - parent_name); + /* If there's a parent_name, then make sure it's valid */ + if (parent_name) { + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not properly formatted"), + parent_name); + goto cleanup; + } + + if (!virVHBAPathExists(NULL, host_num)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("parent '%s' is not an fc_host for the wwnn/wwpn"), + parent_name); + goto cleanup; + } + }
^Here you're handling the device's parent - it's existence
+ + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("host name '%s' is not properly formatted"), + name); goto cleanup; }
- if (!virVHBAPathExists(NULL, host_num)) { + /* 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, - _("parent '%s' is not an fc_host for the wwnn/wwpn"), - parent_name); + _("the wwnn/wwpn for '%s' are assigned to an HBA"), + scsi_host_name); goto cleanup; }
^Here you're handling the device itself.
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + /* No parent name, then no need to get/compare against vhba_parent */ + if (!parent_name) { + retval = true; goto cleanup; + }
And ^here you're again handling the device's parent - it's !existence and perform some further parent device validation in the already existing hunk below. Would it make more sense to first verify the device whether it's a vHBA, if not then throw an error and then, if @parent_name wasn't supplied, return successfully otherwise do both the parent existence check and validation, both in a large "else" block. The checks make sense and work as described, I just find it more understandable - the logic would be IMHO more continuous, as opposed "scattered" (is that even the right term for this? But I think you get the picture...) if (scsi_host_name is vport capable) # it's an HBA error return failure if (!parent_name) # no parent to validate return success else if (parent doesn't exist) error - not an fc_host return failure if (provided_parent != actual_parent) error - invalid parent return failure return success Erik.
if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) goto cleanup; @@ -288,9 +313,7 @@ createVport(virConnectPtr conn, * this pool and we don't have to create the vHBA */ if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - /* 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)) + if (checkParent(conn, name, fchost->parent)) ret = 0;
goto cleanup; -- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/24/2017 11:00 AM, Erik Skultety wrote:
On Thu, Jul 20, 2017 at 03:48:49PM -0400, John Ferlan wrote:
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'/>
This also alters the caller such that the @parent_name param into checkParent can be NULL so as to confirm that at least the provided wwnn/wwpn found a vHBA instead of an HBA.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 27 +++++++++++++---------- src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 23 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..c802738 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn, if (!conn) return true;
- if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("parent '%s' is not properly formatted"), - parent_name); + /* If there's a parent_name, then make sure it's valid */ + if (parent_name) { + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not properly formatted"), + parent_name); + goto cleanup; + } + + if (!virVHBAPathExists(NULL, host_num)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("parent '%s' is not an fc_host for the wwnn/wwpn"), + parent_name); + goto cleanup; + } + }
^Here you're handling the device's parent - it's existence
+ + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("host name '%s' is not properly formatted"), + name); goto cleanup; }
- if (!virVHBAPathExists(NULL, host_num)) { + /* 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, - _("parent '%s' is not an fc_host for the wwnn/wwpn"), - parent_name); + _("the wwnn/wwpn for '%s' are assigned to an HBA"), + scsi_host_name); goto cleanup; }
^Here you're handling the device itself.
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + /* No parent name, then no need to get/compare against vhba_parent */ + if (!parent_name) { + retval = true; goto cleanup; + }
And ^here you're again handling the device's parent - it's !existence and perform some further parent device validation in the already existing hunk below. Would it make more sense to first verify the device whether it's a vHBA, if not then throw an error and then, if @parent_name wasn't supplied, return successfully otherwise do both the parent existence check and validation, both in a large "else" block. The checks make sense and work as described, I just find it more understandable - the logic would be IMHO more continuous, as opposed "scattered" (is that even the right term for this? But I think you get the picture...)
True - I was also trying to keep diffs to a minimum while also trying to preserve both sets of error checking while considering patch 3 ordering. Patch 3 adds the parent_name check above "scsi_host/vhba_parent" because I didn't want to stick the logic between the virAsprintf and GetParentName call and it felt awkward to needlessly get vhba_parent without first checking if @parent_name was going to fail. But you're right I can do better on this and stop making checkParent be the slave to too many masters. I'll push the first 3 and repost something a bit different that I think will be more palatable. Thanks - John FWIW: I did originally have the logic swapped like shown below, but the patch 4 diffs got really ugly...
if (scsi_host_name is vport capable) # it's an HBA error return failure
if (!parent_name) # no parent to validate return success else if (parent doesn't exist) error - not an fc_host return failure
if (provided_parent != actual_parent) error - invalid parent return failure
return success
Erik.
if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) goto cleanup; @@ -288,9 +313,7 @@ createVport(virConnectPtr conn, * this pool and we don't have to create the vHBA */ if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - /* 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)) + if (checkParent(conn, name, fchost->parent)) ret = 0;
goto cleanup; -- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
John Ferlan