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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list