From 36873e3b85a70d8bbd6d9e259165050e93f43612 Mon Sep 17 00:00:00 2001

Message-Id: <36873e3b85a70d8bbd6d9e259165050e93f43612@dist-git>

From: John Ferlan <jferlan@redhat.com>

Date: Wed, 12 Nov 2014 13:34:47 -0500

Subject: [PATCH] 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>

(cherry picked from commit 42a021c1204d7dcb531f5d476e65b53a8bd4f704)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 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 e25bba7..7872112 100644

--- a/docs/formatstorage.html.in

+++ b/docs/formatstorage.html.in

@@ -165,7 +165,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".

             Since 1.0.4

@@ -175,7 +181,12 @@

             parent scsi_host device defined in the

             Node Device database as the

             NPIV

-            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.

             Since 1.0.4

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c

index 88928c9..bfef219 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) {

@@ -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

-- 

2.1.3