[libvirt] [PATCH 0/2] Resolve fc_host startup config issues
Fix a couple of bugs related to attempting to start an fc_host with incorrect 'parent' attributes. One consideration is a parent that is not capable to support the vport operations and the other is providing the incorrect scsi_host# for the parent from the nodedev-create used to generate the wwnn,wwpn Updated the documentation in order to add some more cautions I've find during my investigation and make it clearer what must be provided. John Ferlan (2): storage: Check for valid fc_host parent at startup storage: Ensure fc_host parent matches wwnn/wwpn docs/formatstorage.html.in | 15 ++++- src/storage/storage_backend_scsi.c | 115 ++++++++++++++++++++++++++++++++++--- 2 files changed, 119 insertions(+), 11 deletions(-) -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1160565 If a 'parent' attribute is provided for the fchost, then at startup time check to ensure it is a vport capable scsi_host. If the parent is not vport capable, then disallow the startup. The following is the expected results: error: Failed to start pool fc_pool error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable where the XML for the fc_pool is: <pool type='scsi'> <name>fc_pool</name> <source> <adapter type='fc_host' parent='scsi_host2' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/> </source> ... and 'scsi_host2' is not vport capable. Providing an incorrect parent and a correct wwnn/wwpn could lead to failures at shutdown (deleteVport) where the assumption is the parent is for the fchost. NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host, then we will be creating one with code (virManageVport) which assumes the parent is vport capable. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 02160bc..549d8db 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter) if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) return 0; + /* If a parent was provided, then let's make sure it's vhost capable */ + if (adapter.data.fchost.parent) { + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + return -1; + + if (!virIsCapableFCHost(NULL, parent_host)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' specified for vHBA " + "is not vport capable"), + adapter.data.fchost.parent); + return -1; + } + } + /* This filters either HBA or already created vHBA */ if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) { @@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter) if (!adapter.data.fchost.parent && !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) { - virReportError(VIR_ERR_XML_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("'parent' for vHBA not specified, and " "cannot find one on this host")); return -1; - } - if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) - return -1; + if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0) + return -1; + } if (virManageVport(parent_host, adapter.data.fchost.wwpn, adapter.data.fchost.wwnn, VPORT_CREATE) < 0) -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1160565 The existing code assumed that the configuration of a 'parent' attribute was correct for the createVport path. As it turns out, that may not be the case which leads errors during the deleteVport path because the wwnn/wwpn isn't associated with the parent. With this change the following is reported: error: Failed to start pool fc_pool_host3 error: XML error: Parent attribute 'scsi_host4' does not match parent 'scsi_host3' determined for the 'scsi_host16' wwnn/wwpn lookup. for XML as follows: <pool type='scsi'> <name>fc_pool</name> <source> <adapter type='fc_host' parent='scsi_host4' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/> </source> Where 'nodedev-dumpxml scsi_host16' provides: <device> <name>scsi_host16</name> <path>/sys/devices/pci0000:00/0000:00:04.0/0000:10:00.0/host3/vport-3:0-11/host16</path> <parent>scsi_host3</parent> <capability type='scsi_host'> <host>16</host> <unique_id>13</unique_id> <capability type='fc_host'> <wwnn>5001a4aaf3ca174b</wwnn> <wwpn>5001a4a77192b864</wwpn> ... The patch also adjusts the description of the storage pool to describe the restrictions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 15 +++++- src/storage/storage_backend_scsi.c | 93 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d3e6f05..9d77c45 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -166,7 +166,13 @@ 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. + 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". <span class="since">Since 1.0.4</span> </dd> </dl> @@ -176,7 +182,12 @@ parent scsi_host device defined in the <a href="formatnode.html">Node Device</a> database as the <a href="http://wiki.libvirt.org/page/NPIV_in_libvirt">NPIV</a> - virtual Host Bus Adapter (vHBA). + virtual Host Bus Adapter (vHBA). The value provided must be + a vport capable scsi_host. The value is not the scsi_host of + the vHBA created by 'virsh nodedev-create', rather it is + the parent of that vHBA. If the value is not provided, libvirt + will determine the parent based either finding the wwnn,wwpn + defined for an existing scsi_host or by creating a vHBA. <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 549d8db..20bf82e 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -34,6 +34,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "viraccessapicheck.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -547,8 +548,78 @@ getAdapterName(virStoragePoolSourceAdapter adapter) return name; } +/* + * 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 +checkVhbaSCSIHostParent(virConnectPtr conn, + const char *name, + const char *parent_name) +{ + char *nodedev_name = NULL; + virNodeDevicePtr device = NULL; + char *xml = NULL; + virNodeDeviceDefPtr def = NULL; + virErrorPtr savedError = 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; + + /* We get passed "host#" from the return from virGetFCHostNameByWWN, + * so we need to adjust that to what the nodedev lookup expects + */ + if (virAsprintf(&nodedev_name, "scsi_%s", name) < 0) + goto cleanup; + + /* Compare the scsi_host for the name with the provided parent + * if not the same, then fail + */ + if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) { + virReportError(VIR_ERR_XML_ERROR, + _("Cannot find '%s' in node device database"), + nodedev_name); + goto cleanup; + } + + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup; + + if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) + goto cleanup; + + if (STRNEQ(parent_name, def->parent)) { + virReportError(VIR_ERR_XML_ERROR, + _("Parent attribute '%s' does not match parent '%s' " + "determined for the '%s' wwnn/wwpn lookup."), + parent_name, def->parent, nodedev_name); + goto cleanup; + } + + retval = true; + + cleanup: + if (!retval) + savedError = virSaveLastError(); + VIR_FREE(nodedev_name); + virNodeDeviceDefFree(def); + VIR_FREE(xml); + virNodeDeviceFree(device); + if (savedError) { + virSetError(savedError); + virFreeError(savedError); + } + return retval; +} + static int -createVport(virStoragePoolSourceAdapter adapter) +createVport(virConnectPtr conn, + virStoragePoolSourceAdapter adapter) { unsigned int parent_host; char *name = NULL; @@ -570,11 +641,23 @@ createVport(virStoragePoolSourceAdapter adapter) } } - /* This filters either HBA or already created vHBA */ + /* 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 = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) { + int retval = 0; + + /* If a parent was provided, let's make sure the 'name' we've + * retrieved has the same parent + */ + if (adapter.data.fchost.parent && + !checkVhbaSCSIHostParent(conn, name, adapter.data.fchost.parent)) + retval = -1; + VIR_FREE(name); - return 0; + return retval; } if (!adapter.data.fchost.parent && @@ -704,11 +787,11 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, } static int -virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { virStoragePoolSourceAdapter adapter = pool->def->source.adapter; - return createVport(adapter); + return createVport(conn, adapter); } static int -- 1.9.3
Don't bother reviewing this series - there's another related bug for which I need to make some adjustments to 2/2 - so I'll just with that fix as 3/3. John On 11/06/2014 03:57 PM, John Ferlan wrote:
Fix a couple of bugs related to attempting to start an fc_host with incorrect 'parent' attributes. One consideration is a parent that is not capable to support the vport operations and the other is providing the incorrect scsi_host# for the parent from the nodedev-create used to generate the wwnn,wwpn
Updated the documentation in order to add some more cautions I've find during my investigation and make it clearer what must be provided.
John Ferlan (2): storage: Check for valid fc_host parent at startup storage: Ensure fc_host parent matches wwnn/wwpn
docs/formatstorage.html.in | 15 ++++- src/storage/storage_backend_scsi.c | 115 ++++++++++++++++++++++++++++++++++--- 2 files changed, 119 insertions(+), 11 deletions(-)
participants (1)
-
John Ferlan