storage: Ensure fc_host parent matches wwnn/wwpn

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>

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index d3e6f05c2baad441ff40940cab507b07405de5bf..9d77c452c93f7417f9ec48afdf62444875fd1022 100644 (file)
--- 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 88928c9264591a344fb6ac2d173c8948f223358a..bfef219674bd9656bcbb1bcfafdec012ff27c7f4 100644 (file)
--- 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) {
@@ -705,11 +813,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