4:19 p.m.
Current processing requires a "fire dance" unlocking the @obj,
adding an @obj ref, locking the @devs, and relocking @obj in
order to ensure proper proper lock ordering.
This can be avoided by changing virNodeDeviceObjListRemove to
take @name instead of @obj. Then, we can lock the @devs list,
look up the @obj by @name (like we do when adding), and remove
the @obj from the list. This removes the last reference to the
object effectively reaping it.
NB: Since prior to calling we remove the reference to the object
we cannot pass anything contained within the object (such as the
obj->def or obj->def->name) because it's possible that the object
could be reaped by two competing remove threads.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virnodedeviceobj.c | 29 +++++++++++++++++------------
src/conf/virnodedeviceobj.h | 2 +-
src/node_device/node_device_hal.c | 16 +++++++++-------
src/node_device/node_device_udev.c | 13 +++++++++----
src/test/test_driver.c | 28 ++++++++++++++--------------
5 files changed, 50 insertions(+), 38 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index ad0f27ee4..9064a6cfb 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -470,23 +470,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
}
+/*
+ * virNodeDeviceObjListRemove:
+ * @devs: list of node device objects
+ * @name: a node device definition
+ *
+ * Find the object by name in the list, remove the object from the
+ * list hash table, and free the object.
+ *
+ * Upon entry it's expected that prior to entry any locks on
+ * the object related to @name will have been removed.
+ */
void
virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
- virNodeDeviceObjPtr obj)
+ const char *name)
{
- virNodeDeviceDefPtr def;
-
- if (!obj)
- return;
- def = obj->def;
+ virNodeDeviceObjPtr obj;
- virObjectRef(obj);
- virObjectUnlock(obj);
virObjectRWLockWrite(devs);
- virObjectLock(obj);
- virHashRemoveEntry(devs->objs, def->name);
- virObjectUnlock(obj);
- virObjectUnref(obj);
+ if ((obj = virNodeDeviceObjListFindByNameLocked(devs, name))) {
+ virHashRemoveEntry(devs->objs, name);
+ virNodeDeviceObjEndAPI(&obj);
+ }
virObjectRWUnlock(devs);
}
diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
index 87f908369..b9752bc62 100644
--- a/src/conf/virnodedeviceobj.h
+++ b/src/conf/virnodedeviceobj.h
@@ -72,7 +72,7 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
void
virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
- virNodeDeviceObjPtr dev);
+ const char *name);
int
virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs,
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index 6ad56f416..46a419a6e 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -511,8 +511,8 @@ dev_refresh(const char *udi)
/* Simply "rediscover" device -- incrementally handling changes
* to sub-capabilities (like net.80203) is nasty ... so avoid it.
*/
- virNodeDeviceObjListRemove(driver->devs, obj);
- virObjectUnref(obj);
+ virNodeDeviceObjEndAPI(&obj);
+ virNodeDeviceObjListRemoveDef(driver->devs, name);
dev_create(udi);
} else {
VIR_DEBUG("no device named %s", name);
@@ -536,12 +536,14 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED,
virNodeDeviceObjPtr obj;
obj = virNodeDeviceObjListFindByName(driver->devs, name);
- VIR_DEBUG("%s", name);
- if (obj)
- virNodeDeviceObjListRemove(driver->devs, obj);
- else
+ if (!obj) {
VIR_DEBUG("no device named %s", name);
- virObjectUnref(obj);
+ return;
+ }
+
+ VIR_DEBUG("%s", name);
+ virNodeDeviceObjEndAPI(&obj);
+ virNodeDeviceObjListRemove(driver->devs, name);
}
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index e87eb32a8..f6d223fe4 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1282,6 +1282,7 @@ udevRemoveOneDevice(struct udev_device *device)
virNodeDeviceDefPtr def;
virObjectEventPtr event = NULL;
const char *name = NULL;
+ char *defname = NULL;
name = udev_device_get_syspath(device);
if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, name))) {
@@ -1290,15 +1291,19 @@ udevRemoveOneDevice(struct udev_device *device)
return -1;
}
def = virNodeDeviceObjGetDef(obj);
+ if (VIR_STRDUP(defname, def->name) < 0) {
+ virNodeDeviceObjEndAPI(&obj);
+ return -1;
+ }
event = virNodeDeviceEventLifecycleNew(def->name,
VIR_NODE_DEVICE_EVENT_DELETED,
0);
- VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
- def->name, name);
- virNodeDeviceObjListRemove(driver->devs, obj);
- virObjectUnref(obj);
+ VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
defname, name);
+ virNodeDeviceObjEndAPI(&obj);
+ virNodeDeviceObjListRemove(driver->devs, defname);
+ VIR_FREE(defname);
if (event)
virObjectEventStateQueue(driver->nodeDeviceEventState, event);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ddddd8dcb..568d537e9 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4723,8 +4723,8 @@ testDestroyVport(testDriverPtr privconn,
VIR_NODE_DEVICE_EVENT_DELETED,
0);
- virNodeDeviceObjListRemove(privconn->devs, obj);
- virObjectUnref(obj);
+ virNodeDeviceObjEndAPI(&obj);
+ virNodeDeviceObjListRemove(privconn->devs, "scsi_host12");
testObjectEventQueue(privconn, event);
return 0;
@@ -5670,6 +5670,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
virNodeDeviceObjPtr obj = NULL;
virNodeDeviceObjPtr parentobj = NULL;
virNodeDeviceDefPtr def;
+ char *parent = NULL;
char *wwnn = NULL, *wwpn = NULL;
virObjectEventPtr event = NULL;
@@ -5681,18 +5682,19 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
goto cleanup;
/* 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);
+ * which would replace obj->def, but we still need to save off the
+ * parent name since we're about to Unref and Unlock the @obj containing
+ * the @def so that we don't deadlock in virNodeDeviceObjListFindByName. */
+ if (VIR_STRDUP(parent, def->parent) < 0)
+ goto cleanup;
+
+ virNodeDeviceObjEndAPI(&obj);
/* 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))) {
+ if (!(parentobj = virNodeDeviceObjListFindByName(driver->devs, parent))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot find parent '%s' definition"),
def->parent);
- virObjectLock(obj);
+ _("cannot find parent '%s' definition"),
parent);
goto cleanup;
}
virNodeDeviceObjEndAPI(&parentobj);
@@ -5701,16 +5703,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
VIR_NODE_DEVICE_EVENT_DELETED,
0);
- virObjectLock(obj);
- virNodeDeviceObjListRemove(driver->devs, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virNodeDeviceObjListRemove(driver->devs, dev->name);
cleanup:
virNodeDeviceObjEndAPI(&obj);
testObjectEventQueue(driver, event);
VIR_FREE(wwnn);
VIR_FREE(wwpn);
+ VIR_FREE(parent);
return ret;
}
--
2.13.6