On 03/24/2015 06:06 AM, Erik Skultety wrote: > Once we introduced virStoragePoolSaveStatus function, create a status > XML every time a pool is created (virStoragePoolCreate, > storageDriverAutostart) > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 > --- > src/storage/storage_driver.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > I still think this could be merged into 2 and 4... Commit message should indicate we're not creating a stateFile for transient storage pools which use the PoolCreateXML... Deleting the file occurs at PoolDestroy or PoolDelete time > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index 2899521..4ce3d34 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -136,6 +136,7 @@ static void > storageDriverAutostart(void) > { > size_t i; > + char *stateFile = NULL; > virConnectPtr conn = NULL; > > /* XXX Remove hardcoding of QEMU URI */ > @@ -183,6 +184,12 @@ storageDriverAutostart(void) > virStoragePoolObjUnlock(pool); > continue; > } > + > + if (!(stateFile = virFileBuildPath(driver->stateDir, > + pool->def->name, ".xml"))) > + continue; > + > + ignore_value(virStoragePoolSaveStatus(stateFile, pool->def)); The 'stateFile' is leaked multiple times... Strange Coverity didn't pick up on it! > pool->active = 1; > } > virStoragePoolObjUnlock(pool); > @@ -812,6 +819,7 @@ storagePoolCreate(virStoragePoolPtr obj, > virStoragePoolObjPtr pool; > virStorageBackendPtr backend; > int ret = -1; > + char *stateFile = NULL; > > virCheckFlags(0, -1); > > @@ -840,11 +848,21 @@ storagePoolCreate(virStoragePoolPtr obj, > goto cleanup; > } > > + /* save pool state */ > + if (!(stateFile = virFileBuildPath(driver->stateDir, > + pool->def->name, ".xml"))) > + goto cleanup; > + > + if ((ret = virStoragePoolSaveStatus(stateFile, > + pool->def)) < 0) > + goto cleanup; > + > VIR_INFO("Starting up storage pool '%s'", pool->def->name); Ironically we already started... So just because we cannot create the pool state file, we're failing startup, but the pool is left running on failure... You'll need to stop the pool... maybe even move to in between start and refresh... Both failures need to stop the pool, but you could combine into 1 if stmt. > pool->active = 1; > ret = 0; > > cleanup: > + VIR_FREE(stateFile); > virStoragePoolObjUnlock(pool); > return ret; > } > @@ -889,6 +907,7 @@ storagePoolDestroy(virStoragePoolPtr obj) > { > virStoragePoolObjPtr pool; > virStorageBackendPtr backend; > + char *stateFile = NULL; > int ret = -1; > > storageDriverLock(); > @@ -937,6 +956,15 @@ storagePoolDestroy(virStoragePoolPtr obj) Ironically the VIR_INFO above here says "Shutting down...", but in reality it's already stopped. > pool->def = pool->newDef; > pool->newDef = NULL; > } > + > + if (!(stateFile = virFileBuildPath(driver->stateDir, > + pool->def->name, > + ".xml"))) The args are unaligned. Futhermore, Coverity points out the pool = NULL a few lines back when pool->configFile == NULL, thus we'd have a bad dereference. We've already stopped the pool - so failing and leaving a stopped pool is unusual. So the question comes - do we attempt to delete the status file first and if any part of that fails, we cause failure or do we silently fail. > + goto cleanup; > + > + unlink(stateFile); > + VIR_FREE(stateFile); > + > ret = 0; > > cleanup: > @@ -952,6 +980,7 @@ storagePoolDelete(virStoragePoolPtr obj, > { > virStoragePoolObjPtr pool; > virStorageBackendPtr backend; > + char *stateFile = NULL; > int ret = -1; > > if (!(pool = virStoragePoolObjFromStoragePool(obj))) > @@ -985,6 +1014,14 @@ storagePoolDelete(virStoragePoolPtr obj, > if (backend->deletePool(obj->conn, pool, flags) < 0) > goto cleanup; > VIR_INFO("Deleting storage pool '%s'", pool->def->name); > + > + if (!(stateFile = virFileBuildPath(driver->stateDir, > + pool->def->name, > + ".xml"))) args are unaligned Again I'll point at the wrong tense used in the VIR_INFO message... We're not deleting - we deleted! Also like the destroy path - do we attempt the state file deletion first, then fail if that fails or do we silently just accept that we cannot delete the file. John > + goto cleanup; > + > + unlink(stateFile); > + VIR_FREE(stateFile); > ret = 0; > > cleanup: > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list