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