1:29 p.m.
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: