From 5ba2ce658b3b213013dab461ba787150fd4d6080 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@redhat.com>
Date: Thu, 20 Jul 2017 09:07:52 -0400
Subject: [PATCH] nodedev: Alter node device deletion logic

Alter the node device deletion logic to make use of the parent field
from the obj->def rather than call virNodeDeviceObjListGetParentHost.
As it turns out the saved @def won't have parent_wwnn/wwpn or
parent_fabric_wwn, so the only logical path would be to call
virNodeDeviceObjListGetParentHostByParent which we can accomplish
directly via virNodeDeviceObjListFindByName.
---
 src/node_device/node_device_driver.c | 26 +++++++++++++++++++-------
 src/test/test_driver.c               | 26 +++++++++++++-------------
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 0a19908709..f56ff34831 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -594,8 +594,9 @@ nodeDeviceDestroy(virNodeDevicePtr device)
     int ret = -1;
     virNodeDeviceObjPtr obj = NULL;
     virNodeDeviceDefPtr def;
+    char *parent = NULL;
     char *wwnn = NULL, *wwpn = NULL;
-    int parent_host = -1;
+    unsigned int parent_host;
 
     if (!(obj = nodeDeviceObjFindByName(device->name)))
         return -1;
@@ -609,13 +610,23 @@ nodeDeviceDestroy(virNodeDevicePtr device)
     if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0)
         goto cleanup;
 
-    /* virNodeDeviceGetParentHost will cause the device object's lock
-     * to be taken, so grab the object def which will have the various
-     * fields used to search (name, parent, parent_wwnn, parent_wwpn,
-     * or parent_fabric_wwn) and drop the object lock. */
+    /* Because we're about to release the lock and thus run into a race
+     * possibility (however improbable) with a udevAddOneDevice change
+     * event which would essentially free the existing @def (obj->def) and
+     * replace it with something new, we need to grab the parent field
+     * and then find the parent obj in order to manage the vport */
+    if (VIR_STRDUP(parent, def->parent) < 0)
+        goto cleanup;
+
     virNodeDeviceObjEndAPI(&obj);
-    if ((parent_host = virNodeDeviceObjListGetParentHost(driver->devs, def,
-                                                         EXISTING_DEVICE)) < 0)
+
+    if (!(obj = virNodeDeviceObjListFindByName(driver->devs, parent))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("cannot find parent '%s' definition"), parent);
+        goto cleanup;
+    }
+
+    if (virSCSIHostGetNumber(parent, &parent_host) < 0)
         goto cleanup;
 
     if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0)
@@ -626,6 +637,7 @@ nodeDeviceDestroy(virNodeDevicePtr device)
  cleanup:
     nodeDeviceUnlock();
     virNodeDeviceObjEndAPI(&obj);
+    VIR_FREE(parent);
     VIR_FREE(wwnn);
     VIR_FREE(wwpn);
     return ret;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 447cdd44a1..cee7a859cf 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5515,8 +5515,9 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     int ret = 0;
     testDriverPtr driver = dev->conn->privateData;
     virNodeDeviceObjPtr obj = NULL;
+    virNodeDeviceObjPtr parentobj = NULL;
     virNodeDeviceDefPtr def;
-    char *parent_name = NULL, *wwnn = NULL, *wwpn = NULL;
+    char *wwnn = NULL, *wwpn = NULL;
     virObjectEventPtr event = NULL;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
@@ -5526,22 +5527,22 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1)
         goto cleanup;
 
-    if (VIR_STRDUP(parent_name, def->parent) < 0)
-        goto cleanup;
-
-    /* virNodeDeviceGetParentHost will cause the device object's lock to be
-     * taken, so we have to dup the parent's name and drop the lock
-     * before calling it.  We don't need the reference to the object
-     * any more once we have the parent's name.  */
+    /* Unlike the real code we cannot run into the udevAddOneDevice race
+     * which would replace obj->def, so no need to save off the parent,
+     * but do need to drop the @obj lock so that the FindByName code doesn't
+     * deadlock on ourselves */
     virObjectUnlock(obj);
 
-    /* We do this just for basic validation, but also avoid finding a
-     * vport capable HBA if for some reason our vHBA doesn't exist */
-    if (virNodeDeviceObjListGetParentHost(driver->devs, def,
-                                          EXISTING_DEVICE) < 0) {
+    /* We do this just for basic validation and throw away the parentobj
+     * since there's no vport_delete to be run */
+    if (!(parentobj = virNodeDeviceObjListFindByName(driver->devs,
+                                                     def->parent))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("cannot find parent '%s' definition"), def->parent);
         virObjectLock(obj);
         goto cleanup;
     }
+    virNodeDeviceObjEndAPI(&parentobj);
 
     event = virNodeDeviceEventLifecycleNew(dev->name,
                                            VIR_NODE_DEVICE_EVENT_DELETED,
@@ -5555,7 +5556,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
  cleanup:
     virNodeDeviceObjEndAPI(&obj);
     testObjectEventQueue(driver, event);
-    VIR_FREE(parent_name);
     VIR_FREE(wwnn);
     VIR_FREE(wwpn);
     return ret;
-- 
GitLab