storage: Add mixed fc_host/scsi_host duplicate adapter source checks

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

The virStoragePoolSourceFindDuplicate only checks the incoming definition
against the same type of pool as the def; however, for "scsi_host" and
"fc_host" adapter pools, it's possible that either some pool "scsi_host"
adapter definition is already using the scsi_hostN that the "fc_host"
adapter definition wants to use or some "fc_host" pool adapter definition
is using a vHBA scsi_hostN or parent scsi_hostN that an incoming "scsi_host"
definition is trying to use.

This patch adds the mismatched type checks and adds extraneous comments
to describe what each check is determining.

This patch also modifies the documentation to be describe what scsi_hostN
devices a "scsi_host" source adapter should use and which to avoid. It also
updates the parent definition to specifically call out that for mixed
environments it's better to define which parent to use so that the duplicate
pool checks can be done properly.

docs/formatstorage.html.in

@@ -157,6 +157,25 @@
             compatibility, this attribute is optional <b>only</b> for the
             "scsi_host" adapter, but is mandatory for the "fc_host" adapter.
             <span class="since">Since 1.0.5</span>
+            A "fc_host" capable scsi_hostN can be determined by using
+            <code>virsh nodedev-list --cap fc_host</code>.
+            <span class="since">Since 1.2.8</span>
+            <p>
+            Note: Regardless of whether a "scsi_host" adapter type is defined
+            using a <code>name</code> or a <code>parentaddr</code>, it
+            should refer to a real scsi_host adapter as found through a
+            <code>virsh nodedev-list scsi_host</code> and <code>virsh
+            nodedev-dumpxml scsi_hostN</code> on one of the scsi_host's
+            displayed. It should not refer to a "fc_host" capable scsi_hostN
+            nor should it refer to the vHBA created for some "fc_host"
+            adapter. For a vHBA the <code>nodedev-dumpxml</code>
+            output parent setting will be the "fc_host" capable scsi_hostN
+            value. Additionally, do not refer to an iSCSI scsi_hostN for the
+            "scsi_host" source. An iSCSI scsi_hostN's
+            <code>nodedev-dumpxml</code> output parent field is generally
+            "computer". This is a libvirt created parent value indicating
+            no parent was defined for the node device.
+            </p>
             </dd>
         </dl>
         <dl>
@@ -187,7 +206,12 @@
             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.
+            defined for an existing scsi_host or by creating a vHBA. Providing
+            the parent attribute is also useful for the duplicate pool
+            definition checks. This is more important in environments where
+            both the "fc_host" and "scsi_host" source adapter pools are being
+            used in order to ensure a new definition doesn't duplicate using
+            the scsi_hostN of some existing storage pool.
             <span class="since">Since 1.0.4</span>
           </dd>
           <dt><code>managed</code></dt>

src/conf/storage_conf.c

@@ -2169,6 +2169,83 @@ getSCSIHostNumber(virStoragePoolSourceAdapter adapter,
     VIR_FREE(name);
     return ret;
 }
+
+/*
+ * matchFCHostToSCSIHost:
+ *
+ * @conn: Connection pointer
+ * @fc_adapter: fc_host adapter (either def or pool->def)
+ * @scsi_hostnum: Already determined "scsi_pool" hostnum
+ *
+ * Returns true/false whether there is a match between the incoming
+ *         fc_adapter host# and the scsi_host host#
+ */
+static bool
+matchFCHostToSCSIHost(virConnectPtr conn,
+                      virStoragePoolSourceAdapter fc_adapter,
+                      unsigned int scsi_hostnum)
+{
+    char *name = NULL;
+    char *parent_name = NULL;
+    unsigned int fc_hostnum;
+
+    /* If we have a parent defined, get it's hostnum, and compare to the
+     * scsi_hostnum. If they are the same, then we have a match
+     */
+    if (fc_adapter.data.fchost.parent &&
+        virGetSCSIHostNumber(fc_adapter.data.fchost.parent, &fc_hostnum) == 0 &&
+        scsi_hostnum == fc_hostnum)
+        return true;
+
+    /* If we find an fc_adapter name, then either libvirt created a vHBA
+     * for this fc_host or a 'virsh nodedev-create' generated a vHBA.
+     */
+    if ((name = virGetFCHostNameByWWN(NULL, fc_adapter.data.fchost.wwnn,
+                                      fc_adapter.data.fchost.wwpn))) {
+
+        /* Get the scsi_hostN for the vHBA in order to see if it
+         * matches our scsi_hostnum
+         */
+        if (virGetSCSIHostNumber(name, &fc_hostnum) == 0 &&
+            scsi_hostnum == fc_hostnum) {
+            VIR_FREE(name);
+            return true;
+        }
+
+        /* We weren't provided a parent, so we have to query the node
+         * device driver in order to ascertain the parent of the vHBA.
+         * If the parent fc_hostnum is the same as the scsi_hostnum, we
+         * have a match.
+         */
+        if (conn && !fc_adapter.data.fchost.parent) {
+            parent_name = virStoragePoolGetVhbaSCSIHostParent(conn, name);
+            if (parent_name) {
+                if (virGetSCSIHostNumber(parent_name, &fc_hostnum) == 0 &&
+                    scsi_hostnum == fc_hostnum) {
+                    VIR_FREE(parent_name);
+                    VIR_FREE(name);
+                    return true;
+                }
+                VIR_FREE(parent_name);
+            } else {
+                /* Throw away the error and fall through */
+                virResetLastError();
+                VIR_DEBUG("Could not determine parent vHBA");
+            }
+        }
+        VIR_FREE(name);
+    }
+
+    /* NB: Lack of a name means that this vHBA hasn't yet been created,
+     *     which means our scsi_host cannot be using the vHBA. Furthermore,
+     *     lack of a provided parent means libvirt is going to choose the
+     *     "best" fc_host capable adapter based on availabilty. That could
+     *     conflict with an existing scsi_host definition, but there's no
+     *     way to know that now.
+     */
+    return false;
+}
+
 static bool
 matchSCSIAdapterParent(virStoragePoolObjPtr pool,
                        virStoragePoolDefPtr def)
@@ -2193,7 +2270,8 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
 
 
 int
-virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
+virStoragePoolSourceFindDuplicate(virConnectPtr conn,
+                                  virStoragePoolObjListPtr pools,
                                   virStoragePoolDefPtr def)
 {
     size_t i;
@@ -2253,6 +2331,38 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
                     break;
                 if (pool_hostnum == def_hostnum)
                     matchpool = pool;
+            } else if (pool->def->source.adapter.type ==
+                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST &&
+                       def->source.adapter.type ==
+                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
+                unsigned int scsi_hostnum;
+
+                /* Get the scsi_hostN for the scsi_host source adapter def */
+                if (getSCSIHostNumber(def->source.adapter,
+                                      &scsi_hostnum) < 0)
+                    break;
+
+                if (matchFCHostToSCSIHost(conn, pool->def->source.adapter,
+                                          scsi_hostnum)) {
+                    matchpool = pool;
+                    break;
+                }
+
+            } else if (pool->def->source.adapter.type ==
+                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
+                       def->source.adapter.type ==
+                       VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+                unsigned int scsi_hostnum;
+
+                if (getSCSIHostNumber(pool->def->source.adapter,
+                                      &scsi_hostnum) < 0)
+                    break;
+
+                if (matchFCHostToSCSIHost(conn, def->source.adapter,
+                                          scsi_hostnum)) {
+                    matchpool = pool;
+                    break;
+                }
             }
             break;
         case VIR_STORAGE_POOL_ISCSI:

src/conf/storage_conf.h

@@ -394,7 +394,8 @@ char *virStoragePoolGetVhbaSCSIHostParent(virConnectPtr conn,
                                           const char *name)
     ATTRIBUTE_NONNULL(1);
 
-int virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
+int virStoragePoolSourceFindDuplicate(virConnectPtr conn,
+                                      virStoragePoolObjListPtr pools,
                                       virStoragePoolDefPtr def);
 
 void virStoragePoolObjLock(virStoragePoolObjPtr obj);

src/parallels/parallels_storage.c

@@ -719,7 +719,7 @@ parallelsStoragePoolDefineXML(virConnectPtr conn,
     if (virStoragePoolObjIsDuplicate(&privconn->pools, def, 0) < 0)
         goto cleanup;
 
-    if (virStoragePoolSourceFindDuplicate(&privconn->pools, def) < 0)
+    if (virStoragePoolSourceFindDuplicate(conn, &privconn->pools, def) < 0)
         goto cleanup;
 
     if (parallelsStoragePoolGetAlloc(def))

src/storage/storage_driver.c

@@ -610,7 +610,7 @@ storagePoolCreateXML(virConnectPtr conn,
     if (virStoragePoolObjIsDuplicate(&driver->pools, def, 1) < 0)
         goto cleanup;
 
-    if (virStoragePoolSourceFindDuplicate(&driver->pools, def) < 0)
+    if (virStoragePoolSourceFindDuplicate(conn, &driver->pools, def) < 0)
         goto cleanup;
 
     if ((backend = virStorageBackendForType(def->type)) == NULL)
@@ -669,7 +669,7 @@ storagePoolDefineXML(virConnectPtr conn,
     if (virStoragePoolObjIsDuplicate(&driver->pools, def, 0) < 0)
         goto cleanup;
 
-    if (virStoragePoolSourceFindDuplicate(&driver->pools, def) < 0)
+    if (virStoragePoolSourceFindDuplicate(conn, &driver->pools, def) < 0)
         goto cleanup;
 
     if (virStorageBackendForType(def->type) == NULL)