[libvirt] [PATCH] storage: Fix several issues with transient pool cleanup

[Cole Robinson]

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