diff mbox

storage: Fix several issues with transient pool cleanup

Message ID 0bde0948-4da2-5ef6-857b-c1c88cfa4f98@redhat.com
State New
Headers show

Commit Message

Cole Robinson June 20, 2016, 6:16 p.m. UTC 
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:



>      }

> @@ -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

--
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 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);
     }