[libvirt] [PATCH v3] storage: Fix several issues with transient pool cleanup
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 | 45 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..930ae5a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -77,6 +77,23 @@ static void storageDriverUnlock(void) } static void +storagePoolSetInactive(virStoragePoolObjPtr *poolptr) +{ + virStoragePoolObjPtr pool = *poolptr; + + pool->active = false; + + if (pool->configFile == NULL) { + virStoragePoolObjRemove(&driver->pools, pool); + *poolptr = NULL; + } else if (pool->newDef) { + virStoragePoolDefFree(pool->def); + pool->def = pool->newDef; + pool->newDef = NULL; + } +} + +static void storagePoolUpdateState(virStoragePoolObjPtr pool) { bool active; @@ -115,16 +132,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); @@ -201,6 +221,8 @@ storageDriverAutostart(void) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), pool->def->name, virGetLastErrorMessage()); + storagePoolSetInactive(&pool); + continue; } else { pool->active = true; } @@ -737,8 +759,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 +1089,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,12 +1209,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; - } + storagePoolSetInactive(&pool); goto cleanup; } -- 2.5.5
On 06/22/2016 11:24 AM, 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 | 45 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e2d729f..930ae5a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -77,6 +77,23 @@ static void storageDriverUnlock(void) }
static void +storagePoolSetInactive(virStoragePoolObjPtr *poolptr) +{ + virStoragePoolObjPtr pool = *poolptr; + + pool->active = false; + + if (pool->configFile == NULL) { + virStoragePoolObjRemove(&driver->pools, pool); + *poolptr = NULL; + } else if (pool->newDef) { + virStoragePoolDefFree(pool->def); + pool->def = pool->newDef; + pool->newDef = NULL; + } +} + +static void storagePoolUpdateState(virStoragePoolObjPtr pool) { bool active; @@ -115,16 +132,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;
This bit differs from my suggested diff, and as such pool->active is never activated if active = true; so it needs this diff diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 930ae5a..08b2385 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -138,7 +138,7 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) pool->def->name, virGetLastErrorMessage()); goto error; } - active = true; + pool->active = true; } ret = 0; But actually now that I'm playing with this I'm seeing several other problems tied up in this with related to driver startup and state files. And this particular code path here has some subtle misbehavior because we can remove a storage pool while we are in the middle of iterating over the list of pools... I'm going to hold off on pushing this, but I'm going to work on a series of other cleanups in this area that will include your patch Thanks, Cole
participants (2)
-
Cole Robinson -
Jovanka Gulicoska