Re: [libvirt] [PATCH] storage: Fix several issues with transient pool cleanup
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