The fact that we're removing a pool object from the list of pools doesn't mean we want to unlock it. It violates locking policy too as object locking and unlocking is not done on the same level. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 5 ++--- src/storage/storage_driver.c | 17 +++-------------- src/test/test_driver.c | 14 +++----------- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 31b5af8e9e..6af4a1a22d 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -514,7 +514,6 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virObjectLock(obj); virHashRemoveEntry(pools->objs, uuidstr); virHashRemoveEntry(pools->objsName, obj->def->name); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(pools); } @@ -1594,13 +1593,13 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, VIR_FREE(obj->configFile); /* for driver reload */ if (VIR_STRDUP(obj->configFile, path) < 0) { virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } VIR_FREE(obj->autostartLink); /* for driver reload */ if (VIR_STRDUP(obj->autostartLink, autostartLink) < 0) { virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 496d51b1e0..c14620765e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -125,8 +125,7 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - *objptr = NULL; + virStoragePoolObjEndAPI(objptr); } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); } @@ -760,12 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, if (build_flags || (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { - if (backend->buildPool(obj, build_flags) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + if (backend->buildPool(obj, build_flags) < 0) + goto error; } } @@ -798,8 +793,6 @@ storagePoolCreateXML(virConnectPtr conn, error: virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -835,8 +828,6 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def) < 0) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; newDef = NULL; goto cleanup; } @@ -903,8 +894,6 @@ storagePoolUndefine(virStoragePoolPtr pool) VIR_INFO("Undefining storage pool '%s'", def->name); virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; ret = 0; cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7fd06fcfa8..a0f19f5c5c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4595,16 +4595,12 @@ testStoragePoolCreateXML(virConnectPtr conn, def->source.adapter.data.fchost.wwnn, def->source.adapter.data.fchost.wwpn) < 0) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } } if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -4662,8 +4658,6 @@ testStoragePoolDefineXML(virConnectPtr conn, if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -4692,7 +4686,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool) 0); virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); virObjectEventStateQueue(privconn->eventState, event); return 0; @@ -4784,11 +4778,9 @@ testStoragePoolDestroy(virStoragePoolPtr pool) VIR_STORAGE_POOL_EVENT_STOPPED, 0); - if (!(virStoragePoolObjGetConfigFile(obj))) { + if (!(virStoragePoolObjGetConfigFile(obj))) virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; - } + ret = 0; cleanup: -- 2.21.0

Turns out there's one callback that might remove a storage pool during its run: storagePoolUpdateAllState() call storagePoolUpdateStateCallback() which may call virStoragePoolUpdateInactive() which in turn may call virStoragePoolObjRemove(). Problem is that the UpdateStateCallback() sees a storage pool object with just two references: one for each hash table holding the object. If the function ends up calling ObjRemove() then upon removing the object from hash tables those references are gone and thus any subsequent call touching the object is invalid. The solution to this problem is to grab reference for the object we are running iterator with. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 6af4a1a22d..286f55fb0c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -411,9 +411,13 @@ virStoragePoolObjListForEachCb(void *payload, virStoragePoolObjPtr obj = payload; struct _virStoragePoolObjListForEachData *data = opaque; + /* Grab a reference so that we don't rely only on references grabbed by + * hash table earlier. Remember, an iterator can remove object from the + * hash table. */ + virObjectRef(obj); virObjectLock(obj); data->iter(obj, data->opaque); - virObjectUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } -- 2.21.0

The function comment mistakenly refers to 'poolptr' when in fact the variable is named 'objptr'. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c14620765e..4d26c94e66 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -112,7 +112,7 @@ storagePoolRefreshImpl(virStorageBackendPtr backend, /** * virStoragePoolUpdateInactive: - * @poolptr: pointer to a variable holding the pool object pointer + * @objptr: pointer to a variable holding the pool object pointer * * This function is supposed to be called after a pool becomes inactive. The * function switches to the new config object for persistent pools. Inactive -- 2.21.0

There is no need for this function to call virStoragePoolObjEndAPI(). The object is perfectly usable after return from this function. In fact, all callers will call virStoragePoolObjEndAPI() eventually. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/storage/storage_driver.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4d26c94e66..5d3ab1b25f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -112,20 +112,17 @@ storagePoolRefreshImpl(virStorageBackendPtr backend, /** * virStoragePoolUpdateInactive: - * @objptr: pointer to a variable holding the pool object pointer + * @obj: pool object * * 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 obj) { - virStoragePoolObjPtr obj = *objptr; - if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(driver->pools, obj); - virStoragePoolObjEndAPI(objptr); } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); } @@ -176,7 +173,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, active); if (!virStoragePoolObjIsActive(obj)) - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); return; } @@ -1076,7 +1073,7 @@ storagePoolDestroy(virStoragePoolPtr pool) virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); ret = 0; @@ -1194,7 +1191,7 @@ storagePoolRefresh(virStoragePoolPtr pool, 0); virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); goto cleanup; } -- 2.21.0

This function is doing much more than plain assigning pool definition to a pool object. Rename it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 12 ++++++------ src/conf/virstorageobj.h | 6 +++--- src/libvirt_private.syms | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 286f55fb0c..7f25931d05 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1506,7 +1506,7 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, /** - * virStoragePoolObjAssignDef: + * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer * @def: Storage pool definition to add or update * @check_active: If true, ensure that pool is not active @@ -1517,9 +1517,9 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, * Returns locked and reffed object pointer or NULL on error */ virStoragePoolObjPtr -virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def, - bool check_active) +virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + bool check_active) { virStoragePoolObjPtr obj = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1590,7 +1590,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, return NULL; } - if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) + if (!(obj = virStoragePoolObjListAdd(pools, def, false))) return NULL; def = NULL; @@ -1651,7 +1651,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, } /* create the object */ - if (!(obj = virStoragePoolObjAssignDef(pools, def, true))) + if (!(obj = virStoragePoolObjListAdd(pools, def, true))) goto cleanup; def = NULL; diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index c41d4c16ad..090dd6a7e6 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -193,9 +193,9 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStoragePoolVolumeACLFilter filter); virStoragePoolObjPtr -virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def, - bool check_active); +virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + bool check_active); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 909975750c..8fb366d218 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1178,7 +1178,6 @@ virSecretObjSetValueSize; # conf/virstorageobj.h virStoragePoolObjAddVol; -virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDecrAsyncjobs; virStoragePoolObjDefUseNewDef; @@ -1197,6 +1196,7 @@ virStoragePoolObjGetVolumesCount; virStoragePoolObjIncrAsyncjobs; virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; +virStoragePoolObjListAdd; virStoragePoolObjListExport; virStoragePoolObjListForEach; virStoragePoolObjListNew; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5d3ab1b25f..b4a56e54bb 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -743,7 +743,7 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, true))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -818,7 +818,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, false))) goto cleanup; newDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a0f19f5c5c..832943c1fb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1095,7 +1095,7 @@ testParseStorage(testDriverPtr privconn, if (!def) return -1; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, def, false))) { + if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, false))) { virStoragePoolDefFree(def); return -1; } @@ -4581,7 +4581,7 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml))) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, true))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -4647,7 +4647,7 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, false))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); -- 2.21.0

There will be more boolean information that we want to pass to this function. Instead of having them in separate arguments per each one, use @flags. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 16 +++++++++++----- src/conf/virstorageobj.h | 6 +++++- src/storage/storage_driver.c | 5 +++-- src/test/test_driver.c | 7 ++++--- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 7f25931d05..7515b5d107 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1509,17 +1509,20 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer * @def: Storage pool definition to add or update - * @check_active: If true, ensure that pool is not active + * @flags: bitwise-OR of VIR_STORAGE_POOL_OBJ_LIST_* flags * * Lookup the @def to see if it already exists in the @pools in order * to either update or add if it does not exist. * + * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags + * then this will fail if the pool exists and is active. + * * Returns locked and reffed object pointer or NULL on error */ virStoragePoolObjPtr virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - bool check_active) + unsigned int flags) { virStoragePoolObjPtr obj = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1530,7 +1533,9 @@ virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, if (virStoragePoolObjSourceFindDuplicate(pools, def) < 0) goto error; - rc = virStoragePoolObjIsDuplicate(pools, def, check_active, &obj); + rc = virStoragePoolObjIsDuplicate(pools, def, + !!(flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE), + &obj); if (rc < 0) goto error; @@ -1590,7 +1595,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, return NULL; } - if (!(obj = virStoragePoolObjListAdd(pools, def, false))) + if (!(obj = virStoragePoolObjListAdd(pools, def, 0))) return NULL; def = NULL; @@ -1651,7 +1656,8 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, } /* create the object */ - if (!(obj = virStoragePoolObjListAdd(pools, def, true))) + if (!(obj = virStoragePoolObjListAdd(pools, def, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; def = NULL; diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 090dd6a7e6..fe62515b50 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -192,10 +192,14 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStorageVolPtr **vols, virStoragePoolVolumeACLFilter filter); +enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), +}; + virStoragePoolObjPtr virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - bool check_active); + unsigned int flags); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b4a56e54bb..38b83a77b7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -743,7 +743,8 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -818,7 +819,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, 0))) goto cleanup; newDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 832943c1fb..39f5557fe1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1095,7 +1095,7 @@ testParseStorage(testDriverPtr privconn, if (!def) return -1; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, false))) { + if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, 0))) { virStoragePoolDefFree(def); return -1; } @@ -4581,7 +4581,8 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml))) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -4647,7 +4648,7 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, 0))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); -- 2.21.0

Separate storage pool definition assignment into a function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 7515b5d107..2b9ad6fc98 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1505,6 +1505,21 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, } +static void +virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, + virStoragePoolDefPtr def, + unsigned int flgs ATTRIBUTE_UNUSED) +{ + if (!virStoragePoolObjIsActive(obj)) { + virStoragePoolDefFree(obj->def); + obj->def = def; + } else { + virStoragePoolDefFree(obj->newDef); + obj->newDef = def; + } +} + + /** * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer @@ -1540,14 +1555,8 @@ virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, if (rc < 0) goto error; if (rc > 0) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { - virStoragePoolDefFree(obj->newDef); - obj->newDef = def; - } virObjectRWUnlock(pools); + virStoragePoolObjAssignDef(obj, def, flags); return obj; } -- 2.21.0

On 5/24/19 10:35 AM, Michal Privoznik wrote: Was it intentional to swap the order? John

This flag can be used to denote that the definition we're trying to assign to a pool object is live definition and thus the inactive definition should be saved into ->newDef. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 19 ++++++++++++++----- src/conf/virstorageobj.h | 1 + 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2b9ad6fc98..bdb167e9e2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1508,14 +1508,18 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, static void virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, virStoragePoolDefPtr def, - unsigned int flgs ATTRIBUTE_UNUSED) + unsigned int flags) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { + if (virStoragePoolObjIsActive(obj)) { virStoragePoolDefFree(obj->newDef); obj->newDef = def; + } else { + if (!obj->newDef && + flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE) + VIR_STEAL_PTR(obj->newDef, obj->def); + + virStoragePoolDefFree(obj->def); + obj->def = def; } } @@ -1529,6 +1533,11 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, * Lookup the @def to see if it already exists in the @pools in order * to either update or add if it does not exist. * + * Use VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE to denote that @def + * refers to an active definition and thus any possible inactive + * definition found should be saved to ->newDef (in case of + * future restore). + * * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags * then this will fail if the pool exists and is active. * diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index fe62515b50..df699a84c5 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -193,6 +193,7 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStoragePoolVolumeACLFilter filter); enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE= (1 << 0), VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), }; -- 2.21.0

If there's a persistent storage and user tries to start a new one with the same name and UUID (e.g. to test new configuration) it may happen that upon failure we lose the persistent defintion. Fortunately, we don't remove it from the disk only from the internal list of the pools. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/storage/storage_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 38b83a77b7..def4123b82 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -744,6 +744,7 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE | VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; @@ -790,7 +791,7 @@ storagePoolCreateXML(virConnectPtr conn, return pool; error: - virStoragePoolObjRemove(driver->pools, obj); + virStoragePoolUpdateInactive(obj); goto cleanup; } -- 2.21.0

In near future the storage pool object lock will be released during startPool and buildPool callback (in some backends). But this means that another thread may acquire the pool object lock and change its definition rendering the former thread access not only stale definition but also access freed memory (virStoragePoolObjAssignDef() will free old def when setting a new one). One way out of this would be to have the pool appear as active because our code deals with obj->def and obj->newdef just fine. But we can't declare a pool as active if it's not started or still building up. Therefore, have a boolean flag that is very similar and forces virStoragePoolObjAssignDef() to store new definition in obj->newdef even for an inactive pool. In turn, we have to move the definition to correct place when unsetting the flag. But that's as easy as calling virStoragePoolUpdateInactive(). Technically speaking, change made to storageDriverAutostartCallback() is not needed because until storage driver is initialized no storage API can run therefore there can't be anyone wanting to change the pool's definition. But I'm doing the change there for consistency anyways. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virstorageobj.c | 26 +++++++++++++++++++++++++- src/conf/virstorageobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++- 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index bdb167e9e2..9abcac479e 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -86,6 +86,7 @@ struct _virStoragePoolObj { char *configFile; char *autostartLink; bool active; + bool starting; bool autostart; unsigned int asyncjobs; @@ -312,6 +313,21 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj, } +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting) +{ + obj->starting = starting; +} + + +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj) +{ + return obj->starting; +} + + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj) { @@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, obj->def->name); goto cleanup; } + + if (virStoragePoolObjIsStarting(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pool '%s' is starting up"), + obj->def->name); + goto cleanup; + } } VIR_STEAL_PTR(*objRet, obj); @@ -1510,7 +1533,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, virStoragePoolDefPtr def, unsigned int flags) { - if (virStoragePoolObjIsActive(obj)) { + if (virStoragePoolObjIsActive(obj) || + virStoragePoolObjIsStarting(obj)) { virStoragePoolDefFree(obj->newDef); obj->newDef = def; } else { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index df699a84c5..7dfdf42b26 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -92,6 +92,12 @@ void virStoragePoolObjSetActive(virStoragePoolObjPtr obj, bool active); +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting); +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj); + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8fb366d218..243e3179cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1196,6 +1196,7 @@ virStoragePoolObjGetVolumesCount; virStoragePoolObjIncrAsyncjobs; virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; +virStoragePoolObjIsStarting; virStoragePoolObjListAdd; virStoragePoolObjListExport; virStoragePoolObjListForEach; @@ -1214,6 +1215,7 @@ virStoragePoolObjSetActive; virStoragePoolObjSetAutostart; virStoragePoolObjSetConfigFile; virStoragePoolObjSetDef; +virStoragePoolObjSetStarting; virStoragePoolObjVolumeGetNames; virStoragePoolObjVolumeListExport; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index def4123b82..60bfa48e25 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, if (virStoragePoolObjIsAutostart(obj) && !virStoragePoolObjIsActive(obj)) { + + virStoragePoolObjSetStarting(obj, true); if (backend->startPool && backend->startPool(obj) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); - return; + goto cleanup; } started = true; } @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, true); } } + + cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } } @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, newDef = NULL; def = virStoragePoolObjGetDef(obj); + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; @@ -937,6 +953,8 @@ storagePoolCreate(virStoragePoolPtr pool, goto cleanup; } + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -972,6 +990,11 @@ storagePoolCreate(virStoragePoolPtr pool, ret = 0; cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; @@ -1004,6 +1027,8 @@ storagePoolBuild(virStoragePoolPtr pool, goto cleanup; } + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool && backend->buildPool(obj, flags) < 0) goto cleanup; @@ -1016,6 +1041,10 @@ storagePoolBuild(virStoragePoolPtr pool, ret = 0; cleanup: + if (virStoragePoolObjIsStarting(obj)) { + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; -- 2.21.0

On 5/24/19 10:35 AM, Michal Privoznik wrote: [...] Coverity let me know this morning that there's quite a few lines above here which goto cleanup; however, cleanup expects @obj != NULL (or at least virStoragePoolObjIsStarting does). Probably need similar logic like you added in storagePool{Create|Build}. John [...]