Re: [libvirt] [PATCH v2] There are several cases where we do not handle transient pool destroy and cleanup correctly. For example:
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