[libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host

[John Ferlan]

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

Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
which added parentaddr and unique_id to allow unique identification of
a scsi_host, but assumed that all the pool entries and the incoming
definition would be similarly defined. If the existing pool uses the
'name' attribute and an incoming pool is using the parentaddr/unique_id,
then the code will attempt to compare the existing name string against
the incoming name string which doesn't exist (is NULL) and results in
a core (STREQ).

Conversely, if the existing pool used the parentaddr/unique_id and the
to be defined pool used the name, then the comparison would be against
the parentaddr, but since the incoming pool doesn't have one - that would
leave the comparison against a parentaddr of all 0's and a unique_id of 0,
which will always comparison to fail. This means someone could define the
same source adapter for two pools

This patch resolves that.

Signed-off-by: John Ferlan <jferlan at redhat.com>
---
 src/conf/storage_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 6 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 74267bd..c17565e 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2062,7 +2062,7 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
     return ret;
 }
 
-static bool
+static bool ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 matchSCSIAdapterName(const char *pool_name,
                      const char *def_name)
 {
@@ -2105,16 +2105,63 @@ matchSCSIAdapterParent(virStoragePoolSourceAdapter poolAdapter,
     return false;
 }
 
-static bool
+static int
 matchSCSIAdapter(virStoragePoolSourceAdapter poolAdapter,
                  virStoragePoolSourceAdapter defAdapter)
 {
-    if (poolAdapter.data.scsi_host.name) {
+    char *name = NULL;
+    char *parentaddr = NULL;
+    const char *cmpname;
+    unsigned int unique_id;
+    virDevicePCIAddressPtr poolPCIptr;
+    bool retval = -1;
+
+    /* simple cases - both use name or both use parentaddr */
+    if (poolAdapter.data.scsi_host.name &&
+        defAdapter.data.scsi_host.name) {
         return matchSCSIAdapterName(poolAdapter.data.scsi_host.name,
                                     defAdapter.data.scsi_host.name);
-    } else {
+    } else if (poolAdapter.data.scsi_host.has_parent &&
+               defAdapter.data.scsi_host.has_parent) {
         return matchSCSIAdapterParent(poolAdapter, defAdapter);
     }
+
+    if (poolAdapter.data.scsi_host.has_parent) {
+        /* If the poolAdapter is using parentaddr, but incoming is using the
+         * "scsi_host#" or "host#", then we need to formulate the "host#"
+         * string of the poolAdapter for comparison.  We cannot save that
+         * during startup in the pooldef since when/if we write out the
+         * XML for the pool, if we find the name field filled in we use that
+         * defeating the purpose of having parentaddr/unique_id
+         */
+        cmpname = defAdapter.data.scsi_host.name;
+        unique_id = poolAdapter.data.scsi_host.unique_id;
+        poolPCIptr = &poolAdapter.data.scsi_host.parentaddr;
+    } else {
+        /* Otherwise the pool is using "host#" or "scsi_host#" and the incoming
+         * def is using the "parentaddr" and "uniqueid" - so formulate the
+         * host# name of the incoming def and make sure it doesn't match the
+         * current pool definition
+         */
+        cmpname = poolAdapter.data.scsi_host.name;
+        unique_id = defAdapter.data.scsi_host.unique_id;
+        poolPCIptr = &defAdapter.data.scsi_host.parentaddr;
+    }
+
+    if (virAsprintf(&parentaddr, "%04x:%02x:%02x.%01x",
+                    poolPCIptr->domain, poolPCIptr->bus,
+                    poolPCIptr->slot, poolPCIptr->function) < 0)
+        goto cleanup;
+
+    if (!(name = virFindSCSIHostByPCI(NULL, parentaddr, unique_id)))
+        goto cleanup;
+
+    retval = matchSCSIAdapterName(name, cmpname);
+
+ cleanup:
+    VIR_FREE(name);
+    VIR_FREE(parentaddr);
+    return retval;
 }
 
 int
@@ -2163,8 +2210,12 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
                        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST &&
                        def->source.adapter.type ==
                        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
-                if (matchSCSIAdapter(pool->def->source.adapter,
-                                     def->source.adapter))
+                int rc;
+                rc = matchSCSIAdapter(pool->def->source.adapter,
+                                      def->source.adapter);
+                if (rc < 0)
+                    goto error; /* Error reported during matchSCSIAdapter */
+                if (rc)
                     matchpool = pool;
             }
             break;
@@ -2205,6 +2256,10 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools,
         ret = -1;
     }
     return ret;
+
+ error:
+    virStoragePoolObjUnlock(pool);
+    return -1;
 }
 
 void
-- 
1.9.3