[libvirt] [PATCH 2/2] conf: Fix vHBA checkParent logic for pool creation

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1458708

When originally designed in commit id '42a021c1', providing the 'parent'
attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn' wwpn='vHBA_wwpn'/>
was checked to make sure that the "parent" of the wwnn/wwpn matched that
of the provided parent "just in case" someone created the node device first,
then defined the storage pool using that node device afterwards. The result
is to not perform the vport_create call when the scsi_host represented by
the wwnn/wwpn already exists since it would fail.

Eventually someone came up with the brilliant idea to provide wwnn/wwpn of
an HBA instead of a vHBA, e.g. <adapter type='fc_host' wwnn='HBA_wwnn'
wwpn='HBA_wwpn'/>. This is the same as creating a storage pool backed to
the HBA that just happens to also be vport (vHBA) capable. The logic to
bypass the vport_create call was the same as the vHBA code since the wwn's
already exist. Once that was determined to work, adding in the 'parent'
attribute caused a problem for the DeleteVport code, which was fixed by
commit id '2c8e30ee7e'.

The next test tried was providing a valid pair of wwns that would find
the scsi_host HBA, but providing the wrong name for the 'parent' attribute.
This caused a different failure because at DeleteVport time if a parent
is provided it was assumed valid especially since the CreateVport code
would check that by calling virVHBAPathExists.

So alter the checkParent code to first ensure that the provided parent_name
is a valid HBA/vHBA, then check if the found scsi_host is an HBA or a vHBA
and make the appropriate check similar to the check made during DestroyVport.
This restores some checks that were "lost" during various refactorings such
as commit id '79ab0935' which altered the return value logic and commit id
'9fdc8c426' which moved the parent host name validity check, but neglected
to add a similar check for this odd HBA configuration. As it turns out prior
to this patch, the checkParent code would fail for an HBA, but that was
masked by the changed return value checking logic.

Signed-off-by: John Ferlan <jferlan at redhat.com>
---
 src/conf/node_device_conf.c | 50 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index e5947e6..d4075b5 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2268,6 +2268,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;
@@ -2278,20 +2279,53 @@ checkParent(virConnectPtr conn,
     if (!conn)
         return true;
 
-    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+    if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("parent '%s' is not properly formatted"), name);
         goto cleanup;
+    }
 
-    if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+    if (!virVHBAPathExists(NULL, host_num)) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("parent '%s' is not a vHBA/HBA"), parent_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);
+    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 scsi_host_name is vport capable, then it's an HBA, thus compare
+     * only against the parent_name; otherwise, as long as the scsi_host_name
+     * path exists, then the scsi_host_name is a vHBA in which case we need
+     * to compare against it's parent. */
+    if (virVHBAIsVportCapable(NULL, host_num)) {
+        if (STRNEQ(parent_name, scsi_host_name)) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("parent HBA '%s' doesn't match the wwnn/wwpn "
+                             "scsi_host '%s'"),
+                           parent_name, scsi_host_name);
+            goto cleanup;
+        }
+    } else {
+        if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
+            goto cleanup;
+
+        if (STRNEQ(parent_name, vhba_parent)) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("parent vHBA '%s' doesn't match the wwnn/wwpn "
+                             "scsi_host '%s' parent '%s'"),
+                           parent_name, scsi_host_name, vhba_parent);
+            goto cleanup;
+        }
+    }
+
+
     retval = true;
 
  cleanup:
@@ -2333,7 +2367,7 @@ virNodeDeviceCreateVport(virConnectPtr conn,
     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 (fchost->parent && !checkParent(conn, name, fchost->parent))
             VIR_FREE(name);
 
         return name;
-- 
2.9.4