On 06/21/2016 12:13 PM, Jovanka Gulicoska wrote:

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

Move the pool cleanup logic to a new function storagePoolSetInactive and use it consistently.

Looks like the subject line from the original patch was dropped, please bring it back.

--- src/storage/storage_driver.c | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..2f29292 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; +

Hmm, you can probably make this a bit nicer looking by doing storagePoolSetInactive(virStoragePoolObjPtr *poolptr) { virStoragePoolObjPtr pool = *poolptr; ... That will save having to all the (*pool) stuff, except for the actual bit that matters, *poolptr = NULL; A couple other comments below

+ if ((*pool)->configFile == NULL) { + virStoragePoolObjRemove(&driver->pools, (*pool)); + *pool = NULL; + } else if ((*pool)->newDef) { + virStoragePoolDefFree((*pool)->def); + (*pool)->def = (*pool)->newDef; + (*pool)->newDef = NULL; + } +} + +static void storagePoolUpdateState(virStoragePoolObjPtr pool) { bool active; @@ -115,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; } + active = true; }

- pool->active = active; ret = 0; error: + if (!active) + storagePoolSetInactive(&pool); if (ret < 0) { if (stateFile) unlink(stateFile); @@ -198,6 +216,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());

Since this can set pool=NULL; it needs to come after the virReportError call which derefences pool. Also a few lines below there's a virStoragePoolObjUnlock, so maybe stick a continue; after virReportError to avoid issues with that

@@ -737,8 +756,7 @@ storagePoolCreateXML(virConnectPtr conn, unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); - virStoragePoolObjRemove(&driver->pools, pool); - pool = NULL; + storagePoolSetInactive(&pool); goto cleanup; }

@@ -1068,16 +1086,7 @@ storagePoolDestroy(virStoragePoolPtr obj) VIR_STORAGE_POOL_EVENT_STOPPED, 0);

- 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; - } + storagePoolSetInactive(&pool);

ret = 0;

@@ -1197,13 +1206,7 @@ storagePoolRefresh(virStoragePoolPtr obj, pool->def->uuid, VIR_STORAGE_POOL_EVENT_STOPPED, 0); - pool->active = false; - - if (pool->configFile == NULL) { - virStoragePoolObjRemove(&driver->pools, pool); - pool = NULL; - } - goto cleanup; + storagePoolSetInactive(&pool); }

I missed this before, but you need to preserve the goto cleanup; here Thanks, Cole