diff mbox

[v3] storage: Fix several issues with transient pool cleanup

Message ID 1da71ee8-d380-a2e2-a238-fbbd421c15f9@redhat.com
State New
Headers show

Commit Message

Cole Robinson June 22, 2016, 5:37 p.m. UTC 
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



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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

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;