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@xxxxxxxxxx> --- docs/formatstorage.html.in | 15 ++++- src/storage/storage_backend_scsi.c | 118 +++++++++++++++++++++++++++++++++++-- 2 files changed, 126 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..5220292 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,103 @@ 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#'. On entry we need 'conn' + * set. We won't get here from the autostart path since the caller + * will return true before calling this function. For the shutdown + * path we won't be able to delete the vport. + */ +static char * ATTRIBUTE_NONNULL(1) +getVhbaSCSIHostParent(virConnectPtr conn, + const char *name) +{ + char *nodedev_name = NULL; + virNodeDevicePtr device = NULL; + char *xml = NULL; + virNodeDeviceDefPtr def = NULL; + char *vhba_parent = NULL; + virErrorPtr savedError = NULL; + + VIR_DEBUG("conn=%p, name=%s", conn, name); + + /* 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; + + ignore_value(VIR_STRDUP(vhba_parent, def->parent)); + + cleanup: + if (!vhba_parent) + savedError = virSaveLastError(); + VIR_FREE(nodedev_name); + virNodeDeviceDefFree(def); + VIR_FREE(xml); + virNodeDeviceFree(device); + if (savedError) { + virSetError(savedError); + virFreeError(savedError); + } + return vhba_parent; +} + +/* + * 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 *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 (!(vhba_parent = getVhbaSCSIHostParent(conn, 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); + return retval; +} + static int -createVport(virStoragePoolSourceAdapter adapter) +createVport(virConnectPtr conn, + virStoragePoolSourceAdapter adapter) { unsigned int parent_host; char *name = NULL; @@ -570,11 +666,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 +812,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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list