[libvirt] [PATCH v2] There are several cases where we do not handle transient pool destroy and cleanup correctly. For example:

[Cole Robinson]

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