9:08 a.m.
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.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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 0a19908..f56ff34 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 83ab9cc..076b17a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5618,8 +5618,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)))
@@ -5629,22 +5630,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,
@@ -5658,7 +5659,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
cleanup:
virNodeDeviceObjEndAPI(&obj);
testObjectEventQueue(driver, event);
- VIR_FREE(parent_name);
VIR_FREE(wwnn);
VIR_FREE(wwpn);
return ret;
--
2.9.4