On 06/20/2016 12:18 PM, Jovanka Gulicoska wrote:

There are several cases where we do not handle transient pool destroy and cleanup correctly. For example:

https://bugzilla.redhat.com/show_bug.cgi?id=1227475

Move the pool cleanup logic to a new function storagePoolSetInactive and use it consistently. --- src/storage/storage_driver.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..74af35d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -77,6 +77,21 @@ static void storageDriverUnlock(void) }

static void +storagePoolSetInactive(virStoragePoolObjPtr pool) +{ + pool->active = false; + + if (pool->configFile == NULL) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + } else if (pool->newDef) { + virStoragePoolDefFree(pool->def); + pool->def = pool->newDef; + pool->newDef = NULL; + } +} +

Hmm. I'm testing this, and I see one problem here. The pool = NULL bit is not propagated to the caller in any way, which is required to not crash in cleanup paths if the pool disappears. I think this function needs to take virStoragePoolObjPtr *pool, and callers pass in &pool, so the *pool = NULL; bit affects the caller.

+static void storagePoolUpdateState(virStoragePoolObjPtr pool) { bool active; @@ -143,6 +158,7 @@ storagePoolUpdateAllState(void) virStoragePoolObjPtr pool = driver->pools.objs[i];

virStoragePoolObjLock(pool); + storagePoolSetInactive(pool); storagePoolUpdateState(pool); virStoragePoolObjUnlock(pool);

I see this bit here is required to fix the reported bug, but I think it should be pushed into storagePoolUpdateState(pool), since there are cases there where the pool may be marked as 'inactive' as well. This is the diff I came up with: diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 74af35d..ae965ee 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -130,16 +130,19 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) if (backend->refreshPool(NULL, pool) < 0) { if (backend->stopPool) backend->stopPool(NULL, pool); + active = false; virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to restart storage pool '%s': %s"), pool->def->name, virGetLastErrorMessage()); goto error; } + pool->active = true; } - pool->active = active; ret = 0; error: + if (!active) + storagePoolSetInactive(pool); if (ret < 0) { if (stateFile) unlink(stateFile); @@ -158,7 +161,6 @@ storagePoolUpdateAllState(void) virStoragePoolObjPtr pool = driver->pools.objs[i]; virStoragePoolObjLock(pool); - storagePoolSetInactive(pool); storagePoolUpdateState(pool); virStoragePoolObjUnlock(pool); }

} @@ -198,6 +214,7 @@ storageDriverAutostart(void) unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); + storagePoolSetInactive(pool); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), pool->def->name, virGetLastErrorMessage()); @@ -737,7 +754,7 @@ storagePoolCreateXML(virConnectPtr conn, unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); - virStoragePoolObjRemove(&driver->pools, pool); + storagePoolSetInactive(pool); pool = NULL;

This pool = NULL; line can be dropped if the above first suggestion is implemented. Otherwise this looks correct to me Thanks, Cole