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; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list