[libvirt] [PATCH 6/6] storage: Introduce storagePoolUpdateAllState function

[John Ferlan]

On 04/02/2015 06:10 AM, Erik Skultety wrote:
> The 'checkPool' callback was originally part of the storageDriverAutostart function,
> but the pools need to be checked earlier during initialization phase,
> otherwise we can't start a domain which mountsa volume after daemon's

s/mountsa/mounts a/

s/after daemon's/after the libvirtd daemon/

> restarted. This is because qemuProcessReconnect is called earlier than
> storageDriverAutostart. Therefore the 'checkPool' logic has been moved to
> storagePoolUpdateAllState which is called inside storageDriverInitialize.
> 
> We also need a valid 'conn' reference to be able to execute 'refreshPool'
> during initialization phase. Though it isn't available until storageDriverAutostart
> all of our storage backends do ignore 'conn' pointer, except for RBD,
> but RBD doesn't support 'checkPool' callback, so it's safe to pass
> conn = NULL in this case.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733

yes, this does belong in this one

Also, as I'm going through my list of backlog bugzillas, I think that
https://bugzilla.redhat.com/show_bug.cgi?id=1125805 can be duplicated to
this bz.

> ---
>  src/storage/storage_driver.c | 74 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 36c05b3..12e94ad 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -75,6 +75,64 @@ static void storageDriverUnlock(void)
>  }
>  
>  static void
> +storagePoolUpdateAllState(void)
> +{
> +    size_t i;
> +    bool active = false;

Instead of initializing it here - do it in the code...

> +
> +    for (i = 0; i < driver->pools.count; i++) {
> +        virStoragePoolObjPtr pool = driver->pools.objs[i];
> +        virStorageBackendPtr backend;
> +
> +        virStoragePoolObjLock(pool);
> +        if (!virStoragePoolObjIsActive(pool)) {
> +            virStoragePoolObjUnlock(pool);
> +            continue;
> +        }
> +
> +        if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> +            VIR_ERROR(_("Missing backend %d"), pool->def->type);
> +            virStoragePoolObjUnlock(pool);
> +            continue;
> +        }
> +
> +        /* Backends which do not support 'checkPool' are considered
> +         * inactive by default.
> +         */
           active = false;


So here's my reasoning for resetting active each time through the
pool... What happens if pool [1] has a check and is determined to be
active... then pool [2] doesn't have a check function, but active is
still true from [1] - thus the refreshPool will happen.

ACK  with that adjustment.

John
> +        if (backend->checkPool &&
> +            backend->checkPool(pool, &active) < 0) {
> +            virErrorPtr err = virGetLastError();
> +            VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
> +                      pool->def->name, err ? err->message :
> +                      _("no error message found"));
> +            virStoragePoolObjUnlock(pool);
> +            continue;
> +        }
> +
> +        /* We can pass NULL as connection, most backends do not use
> +         * it anyway, but if they do and fail, we want to log error and
> +         * continue with other pools.
> +         */
> +        if (active) {
> +            virStoragePoolObjClearVols(pool);
> +            if (backend->refreshPool(NULL, pool) < 0) {
> +                virErrorPtr err = virGetLastError();
> +                if (backend->stopPool)
> +                    backend->stopPool(NULL, pool);
> +                VIR_ERROR(_("Failed to restart storage pool '%s': %s"),
> +                          pool->def->name, err ? err->message :
> +                          _("no error message found"));
> +                virStoragePoolObjUnlock(pool);
> +                continue;
> +            }
> +        }
> +
> +        pool->active = active;
> +        virStoragePoolObjUnlock(pool);
> +    }
> +}
> +
> +static void
>  storageDriverAutostart(void)
>  {
>      size_t i;
> @@ -95,23 +153,11 @@ storageDriverAutostart(void)
>  
>          virStoragePoolObjLock(pool);
>          if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> -            VIR_ERROR(_("Missing backend %d"), pool->def->type);
>              virStoragePoolObjUnlock(pool);
>              continue;
>          }
>  
> -        if (backend->checkPool &&
> -            backend->checkPool(pool, &started) < 0) {
> -            virErrorPtr err = virGetLastError();
> -            VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
> -                      pool->def->name, err ? err->message :
> -                      _("no error message found"));
> -            virStoragePoolObjUnlock(pool);
> -            continue;
> -        }
> -
> -        if (!started &&
> -            pool->autostart &&
> +        if (pool->autostart &&
>              !virStoragePoolObjIsActive(pool)) {
>              if (backend->startPool &&
>                  backend->startPool(conn, pool) < 0) {
> @@ -215,6 +261,8 @@ storageStateInitialize(bool privileged,
>                                       driver->autostartDir) < 0)
>          goto error;
>  
> +    storagePoolUpdateAllState();
> +
>      storageDriverUnlock();
>  
>      ret = 0;
>