Current processing requires a "fire dance" unlocking the @obj,
adding an @obj ref, locking the @pools, and relocking @obj in
order to ensure proper lock ordering.
This can be avoided by changing virStoragePoolObjRemove to
take a @name instead of @obj. Then, we can lock the @pools
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@xxxxxxxxxx>
---
src/conf/virstorageobj.c | 41 +++++++++++++++++++++++++++++------------
src/conf/virstorageobj.h | 2 +-
src/storage/storage_driver.c | 44 +++++++++++++++++++++++++++++---------------
src/test/test_driver.c | 30 +++++++++++++++++++-----------
4 files changed, 78 insertions(+), 39 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index f48f08a64..df6febfd0 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1034,21 +1034,31 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
}
+/*
+ * virStoragePoolObjRemove:
+ * @pools: list of storage pool objects
+ * @name: name of storage pool to remove
+ *
+ * Find the object by name in the list, remove the object from
+ * each hash table in the list, 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
virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
- virStoragePoolObjPtr obj)
+ const char *name)
{
+ virStoragePoolObjPtr obj;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(obj->def->uuid, uuidstr);
- virObjectRef(obj);
- virObjectUnlock(obj);
virObjectRWLockWrite(pools);
- virObjectLock(obj);
- virHashRemoveEntry(pools->objs, uuidstr);
- virHashRemoveEntry(pools->objsName, obj->def->name);
- virObjectUnlock(obj);
- virObjectUnref(obj);
+ if ((obj = virStoragePoolObjFindByNameLocked(pools, name))) {
+ virUUIDFormat(obj->def->uuid, uuidstr);
+ virHashRemoveEntry(pools->objs, uuidstr);
+ virHashRemoveEntry(pools->objsName, name);
+ virStoragePoolObjEndAPI(&obj);
+ }
virObjectRWUnlock(pools);
}
@@ -1117,6 +1127,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
{
virStoragePoolDefPtr def = NULL;
virStoragePoolObjPtr obj = NULL;
+ char *name = NULL;
if (!(def = virStoragePoolDefParseFile(path)))
return NULL;
@@ -1129,6 +1140,9 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
goto error;
}
+ if (VIR_STRDUP(name, def->name) < 0)
+ goto error;
+
if (!(obj = virStoragePoolObjAssignDef(pools, def)))
goto error;
def = NULL;
@@ -1144,13 +1158,16 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
obj->autostart = virFileLinkPointsTo(obj->autostartLink,
obj->configFile);
+ VIR_FREE(name);
+
return obj;
error:
- if (obj) {
- virStoragePoolObjRemove(pools, obj);
- virObjectUnref(obj);
+ if (obj && name) {
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(pools, name);
}
+ VIR_FREE(name);
virStoragePoolDefFree(def);
return NULL;
}
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index dd7001c4b..047b08a92 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -242,7 +242,7 @@ virStoragePoolObjListNew(void);
void
virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
- virStoragePoolObjPtr obj);
+ const char *name);
int
virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 6276545eb..814e5cb97 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -82,19 +82,21 @@ static void storageDriverUnlock(void)
/**
* virStoragePoolUpdateInactive:
* @poolptr: pointer to a variable holding the pool object pointer
+ * @name: Name of the pool
*
* This function is supposed to be called after a pool becomes inactive. The
* function switches to the new config object for persistent pools. Inactive
* pools are removed.
*/
static void
-virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr)
+virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr,
+ const char *name)
{
virStoragePoolObjPtr obj = *objptr;
if (!virStoragePoolObjGetConfigFile(obj)) {
- virStoragePoolObjRemove(driver->pools, obj);
- virObjectUnref(obj);
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(driver->pools, name);
*objptr = NULL;
} else if (virStoragePoolObjGetNewDef(obj)) {
virStoragePoolObjDefUseNewDef(obj);
@@ -110,6 +112,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
bool active = false;
virStorageBackendPtr backend;
char *stateFile;
+ char *name = NULL;
if (!(stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml")))
goto cleanup;
@@ -120,6 +123,9 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
goto cleanup;
}
+ if (VIR_STRDUP(name, def->name) < 0)
+ goto cleanup;
+
/* Backends which do not support 'checkPool' are considered
* inactive by default. */
if (backend->checkPool &&
@@ -149,12 +155,13 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj,
virStoragePoolObjSetActive(obj, active);
if (!virStoragePoolObjIsActive(obj))
- virStoragePoolUpdateInactive(&obj);
+ virStoragePoolUpdateInactive(&obj, name);
cleanup:
if (!active && stateFile)
ignore_value(unlink(stateFile));
VIR_FREE(stateFile);
+ VIR_FREE(name);
return;
}
@@ -707,6 +714,7 @@ storagePoolCreateXML(virConnectPtr conn,
virStorageBackendPtr backend;
virObjectEventPtr event = NULL;
char *stateFile = NULL;
+ char *name = NULL;
unsigned int build_flags = 0;
virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
@@ -731,6 +739,9 @@ storagePoolCreateXML(virConnectPtr conn,
if ((backend = virStorageBackendForType(newDef->type)) == NULL)
goto cleanup;
+ if (VIR_STRDUP(name, newDef->name) < 0)
+ goto cleanup;
+
if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
goto cleanup;
newDef = NULL;
@@ -776,6 +787,7 @@ storagePoolCreateXML(virConnectPtr conn,
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
+ VIR_FREE(name);
VIR_FREE(stateFile);
virStoragePoolDefFree(newDef);
if (event)
@@ -784,9 +796,8 @@ storagePoolCreateXML(virConnectPtr conn,
return pool;
error:
- virStoragePoolObjRemove(driver->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(driver->pools, name);
goto cleanup;
}
@@ -800,6 +811,7 @@ storagePoolDefineXML(virConnectPtr conn,
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
virObjectEventPtr event = NULL;
+ char *name = NULL;
virCheckFlags(0, NULL);
@@ -821,15 +833,17 @@ storagePoolDefineXML(virConnectPtr conn,
if (virStorageBackendForType(newDef->type) == NULL)
goto cleanup;
+ if (VIR_STRDUP(name, newDef->name) < 0)
+ goto cleanup;
+
if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
goto cleanup;
newDef = NULL;
def = virStoragePoolObjGetDef(obj);
if (virStoragePoolObjSaveDef(driver, obj, def) < 0) {
- virStoragePoolObjRemove(driver->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(driver->pools, name);
goto cleanup;
}
@@ -841,6 +855,7 @@ storagePoolDefineXML(virConnectPtr conn,
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
+ VIR_FREE(name);
if (event)
virObjectEventStateQueue(driver->storageEventState, event);
virStoragePoolDefFree(newDef);
@@ -895,9 +910,8 @@ storagePoolUndefine(virStoragePoolPtr pool)
0);
VIR_INFO("Undefining storage pool '%s'", def->name);
- virStoragePoolObjRemove(driver->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(driver->pools, pool->name);
ret = 0;
cleanup:
@@ -1089,7 +1103,7 @@ storagePoolDestroy(virStoragePoolPtr pool)
virStoragePoolObjSetActive(obj, false);
- virStoragePoolUpdateInactive(&obj);
+ virStoragePoolUpdateInactive(&obj, pool->name);
ret = 0;
@@ -1212,7 +1226,7 @@ storagePoolRefresh(virStoragePoolPtr pool,
0);
virStoragePoolObjSetActive(obj, false);
- virStoragePoolUpdateInactive(&obj);
+ virStoragePoolUpdateInactive(&obj, pool->name);
goto cleanup;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 0a284e3d1..70509a204 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4540,6 +4540,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
virObjectEventPtr event = NULL;
+ char *name = NULL;
virCheckFlags(0, NULL);
@@ -4550,6 +4551,9 @@ testStoragePoolCreateXML(virConnectPtr conn,
if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, true) < 0)
goto cleanup;
+ if (VIR_STRDUP(name, newDef->name) < 0)
+ goto cleanup;
+
if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef)))
goto cleanup;
newDef = NULL;
@@ -4583,6 +4587,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
+ VIR_FREE(name);
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
@@ -4590,9 +4595,8 @@ testStoragePoolCreateXML(virConnectPtr conn,
return pool;
error:
- virStoragePoolObjRemove(privconn->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(privconn->pools, name);
goto cleanup;
}
@@ -4608,6 +4612,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
virObjectEventPtr event = NULL;
+ char *name = NULL;
virCheckFlags(0, NULL);
@@ -4622,6 +4627,9 @@ testStoragePoolDefineXML(virConnectPtr conn,
if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, false) < 0)
goto cleanup;
+ if (VIR_STRDUP(name, newDef->name) < 0)
+ goto cleanup;
+
if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef)))
goto cleanup;
newDef = NULL;
@@ -4632,15 +4640,15 @@ testStoragePoolDefineXML(virConnectPtr conn,
0);
if (testStoragePoolObjSetDefaults(obj) == -1) {
- virStoragePoolObjRemove(privconn->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(privconn->pools, name);
goto cleanup;
}
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
+ VIR_FREE(name);
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
@@ -4663,8 +4671,8 @@ testStoragePoolUndefine(virStoragePoolPtr pool)
VIR_STORAGE_POOL_EVENT_UNDEFINED,
0);
- virStoragePoolObjRemove(privconn->pools, obj);
- virObjectUnref(obj);
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(privconn->pools, pool->name);
testObjectEventQueue(privconn, event);
return 0;
@@ -4757,10 +4765,10 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
0);
if (!(virStoragePoolObjGetConfigFile(obj))) {
- virStoragePoolObjRemove(privconn->pools, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virStoragePoolObjEndAPI(&obj);
+ virStoragePoolObjRemove(privconn->pools, pool->name);
}
+
ret = 0;
cleanup:
--
2.13.6
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
