On 5/24/19 10:35 AM, Michal Privoznik wrote:
> In near future the storage pool object lock will be released
> during startPool and buildPool callback (in some backends). But
> this means that another thread may acquire the pool object lock
> and change its definition rendering the former thread access not
> only stale definition but also access freed memory
> (virStoragePoolObjAssignDef() will free old def when setting a
> new one).
>
> One way out of this would be to have the pool appear as active
> because our code deals with obj->def and obj->newdef just fine.
> But we can't declare a pool as active if it's not started or
> still building up. Therefore, have a boolean flag that is very
> similar and forces virStoragePoolObjAssignDef() to store new
> definition in obj->newdef even for an inactive pool. In turn, we
> have to move the definition to correct place when unsetting the
> flag. But that's as easy as calling
> virStoragePoolUpdateInactive().
>
> Technically speaking, change made to
> storageDriverAutostartCallback() is not needed because until
> storage driver is initialized no storage API can run therefore
> there can't be anyone wanting to change the pool's definition.
> But I'm doing the change there for consistency anyways.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/conf/virstorageobj.c | 26 +++++++++++++++++++++++++-
> src/conf/virstorageobj.h | 6 ++++++
> src/libvirt_private.syms | 2 ++
> src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++-
> 4 files changed, 63 insertions(+), 2 deletions(-)
>
[...]
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index def4123b82..60bfa48e25 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
>
> if (virStoragePoolObjIsAutostart(obj) &&
> !virStoragePoolObjIsActive(obj)) {
> +
> + virStoragePoolObjSetStarting(obj, true);
> if (backend->startPool &&
> backend->startPool(obj) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to autostart storage pool '%s': %s"),
> def->name, virGetLastErrorMessage());
> - return;
> + goto cleanup;
> }
> started = true;
> }
> @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
> virStoragePoolObjSetActive(obj, true);
> }
> }
> +
> + cleanup:
> + if (virStoragePoolObjIsStarting(obj)) {
> + if (!virStoragePoolObjIsActive(obj))
> + virStoragePoolUpdateInactive(obj);
> + virStoragePoolObjSetStarting(obj, false);
> + }
> }
>
>
> @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
> newDef = NULL;
> def = virStoragePoolObjGetDef(obj);
>
Coverity let me know this morning that there's quite a few lines above
here which goto cleanup; however, cleanup expects @obj != NULL (or at
least virStoragePoolObjIsStarting does). Probably need similar logic
like you added in storagePool{Create|Build}.
John
> + virStoragePoolObjSetStarting(obj, true);
> +
> if (backend->buildPool) {
> if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
> build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
> @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn,
> pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
>
> cleanup:
> + if (virStoragePoolObjIsStarting(obj)) {
> + if (!virStoragePoolObjIsActive(obj))
> + virStoragePoolUpdateInactive(obj);
> + virStoragePoolObjSetStarting(obj, false);
> + }
> virObjectEventStateQueue(driver->storageEventState, event);
> virStoragePoolObjEndAPI(&obj);
> return pool;
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
