mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-08-28 11:31:16 +00:00
storage_driver: Protect pool def during startup and build
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@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
This commit is contained in:
parent
9342bc626b
commit
13284a6b83
@ -86,6 +86,7 @@ struct _virStoragePoolObj {
|
|||||||
char *configFile;
|
char *configFile;
|
||||||
char *autostartLink;
|
char *autostartLink;
|
||||||
bool active;
|
bool active;
|
||||||
|
bool starting;
|
||||||
bool autostart;
|
bool autostart;
|
||||||
unsigned int asyncjobs;
|
unsigned int asyncjobs;
|
||||||
|
|
||||||
@ -312,6 +313,21 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
void
|
||||||
|
virStoragePoolObjSetStarting(virStoragePoolObjPtr obj,
|
||||||
|
bool starting)
|
||||||
|
{
|
||||||
|
obj->starting = starting;
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
bool
|
||||||
|
virStoragePoolObjIsStarting(virStoragePoolObjPtr obj)
|
||||||
|
{
|
||||||
|
return obj->starting;
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
bool
|
bool
|
||||||
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj)
|
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj)
|
||||||
{
|
{
|
||||||
@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
|
|||||||
obj->def->name);
|
obj->def->name);
|
||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("pool '%s' is starting up"),
|
||||||
|
obj->def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
VIR_STEAL_PTR(*objRet, obj);
|
VIR_STEAL_PTR(*objRet, obj);
|
||||||
@ -1510,7 +1533,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj,
|
|||||||
virStoragePoolDefPtr def,
|
virStoragePoolDefPtr def,
|
||||||
unsigned int flags)
|
unsigned int flags)
|
||||||
{
|
{
|
||||||
if (virStoragePoolObjIsActive(obj)) {
|
if (virStoragePoolObjIsActive(obj) ||
|
||||||
|
virStoragePoolObjIsStarting(obj)) {
|
||||||
virStoragePoolDefFree(obj->newDef);
|
virStoragePoolDefFree(obj->newDef);
|
||||||
obj->newDef = def;
|
obj->newDef = def;
|
||||||
} else {
|
} else {
|
||||||
|
@ -94,6 +94,12 @@ void
|
|||||||
virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
|
virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
|
||||||
bool active);
|
bool active);
|
||||||
|
|
||||||
|
void
|
||||||
|
virStoragePoolObjSetStarting(virStoragePoolObjPtr obj,
|
||||||
|
bool starting);
|
||||||
|
bool
|
||||||
|
virStoragePoolObjIsStarting(virStoragePoolObjPtr obj);
|
||||||
|
|
||||||
bool
|
bool
|
||||||
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj);
|
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj);
|
||||||
|
|
||||||
|
@ -1246,6 +1246,7 @@ virStoragePoolObjGetVolumesCount;
|
|||||||
virStoragePoolObjIncrAsyncjobs;
|
virStoragePoolObjIncrAsyncjobs;
|
||||||
virStoragePoolObjIsActive;
|
virStoragePoolObjIsActive;
|
||||||
virStoragePoolObjIsAutostart;
|
virStoragePoolObjIsAutostart;
|
||||||
|
virStoragePoolObjIsStarting;
|
||||||
virStoragePoolObjListAdd;
|
virStoragePoolObjListAdd;
|
||||||
virStoragePoolObjListExport;
|
virStoragePoolObjListExport;
|
||||||
virStoragePoolObjListForEach;
|
virStoragePoolObjListForEach;
|
||||||
@ -1264,6 +1265,7 @@ virStoragePoolObjSetActive;
|
|||||||
virStoragePoolObjSetAutostart;
|
virStoragePoolObjSetAutostart;
|
||||||
virStoragePoolObjSetConfigFile;
|
virStoragePoolObjSetConfigFile;
|
||||||
virStoragePoolObjSetDef;
|
virStoragePoolObjSetDef;
|
||||||
|
virStoragePoolObjSetStarting;
|
||||||
virStoragePoolObjVolumeGetNames;
|
virStoragePoolObjVolumeGetNames;
|
||||||
virStoragePoolObjVolumeListExport;
|
virStoragePoolObjVolumeListExport;
|
||||||
|
|
||||||
|
@ -202,12 +202,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
|
|||||||
|
|
||||||
if (virStoragePoolObjIsAutostart(obj) &&
|
if (virStoragePoolObjIsAutostart(obj) &&
|
||||||
!virStoragePoolObjIsActive(obj)) {
|
!virStoragePoolObjIsActive(obj)) {
|
||||||
|
|
||||||
|
virStoragePoolObjSetStarting(obj, true);
|
||||||
if (backend->startPool &&
|
if (backend->startPool &&
|
||||||
backend->startPool(obj) < 0) {
|
backend->startPool(obj) < 0) {
|
||||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||||
_("Failed to autostart storage pool '%s': %s"),
|
_("Failed to autostart storage pool '%s': %s"),
|
||||||
def->name, virGetLastErrorMessage());
|
def->name, virGetLastErrorMessage());
|
||||||
return;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
started = true;
|
started = true;
|
||||||
}
|
}
|
||||||
@ -226,6 +228,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
|
|||||||
virStoragePoolObjSetActive(obj, true);
|
virStoragePoolObjSetActive(obj, true);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
cleanup:
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
if (!virStoragePoolObjIsActive(obj))
|
||||||
|
virStoragePoolUpdateInactive(obj);
|
||||||
|
virStoragePoolObjSetStarting(obj, false);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@ -761,6 +770,8 @@ storagePoolCreateXML(virConnectPtr conn,
|
|||||||
newDef = NULL;
|
newDef = NULL;
|
||||||
def = virStoragePoolObjGetDef(obj);
|
def = virStoragePoolObjGetDef(obj);
|
||||||
|
|
||||||
|
virStoragePoolObjSetStarting(obj, true);
|
||||||
|
|
||||||
if (backend->buildPool) {
|
if (backend->buildPool) {
|
||||||
if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
|
if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
|
||||||
build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
|
build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
|
||||||
@ -797,6 +808,11 @@ storagePoolCreateXML(virConnectPtr conn,
|
|||||||
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
|
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
|
||||||
|
|
||||||
cleanup:
|
cleanup:
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
if (!virStoragePoolObjIsActive(obj))
|
||||||
|
virStoragePoolUpdateInactive(obj);
|
||||||
|
virStoragePoolObjSetStarting(obj, false);
|
||||||
|
}
|
||||||
virObjectEventStateQueue(driver->storageEventState, event);
|
virObjectEventStateQueue(driver->storageEventState, event);
|
||||||
virStoragePoolObjEndAPI(&obj);
|
virStoragePoolObjEndAPI(&obj);
|
||||||
return pool;
|
return pool;
|
||||||
@ -879,6 +895,13 @@ storagePoolUndefine(virStoragePoolPtr pool)
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
||||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||||
_("pool '%s' has asynchronous jobs running."),
|
_("pool '%s' has asynchronous jobs running."),
|
||||||
@ -923,6 +946,7 @@ storagePoolCreate(virStoragePoolPtr pool,
|
|||||||
int ret = -1;
|
int ret = -1;
|
||||||
unsigned int build_flags = 0;
|
unsigned int build_flags = 0;
|
||||||
VIR_AUTOFREE(char *) stateFile = NULL;
|
VIR_AUTOFREE(char *) stateFile = NULL;
|
||||||
|
bool restoreStarting = false;
|
||||||
|
|
||||||
virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
|
virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
|
||||||
VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
|
VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
|
||||||
@ -948,6 +972,16 @@ storagePoolCreate(virStoragePoolPtr pool,
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
|
virStoragePoolObjSetStarting(obj, true);
|
||||||
|
restoreStarting = true;
|
||||||
|
|
||||||
if (backend->buildPool) {
|
if (backend->buildPool) {
|
||||||
if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
|
if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
|
||||||
build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
|
build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
|
||||||
@ -983,6 +1017,12 @@ storagePoolCreate(virStoragePoolPtr pool,
|
|||||||
ret = 0;
|
ret = 0;
|
||||||
|
|
||||||
cleanup:
|
cleanup:
|
||||||
|
if (restoreStarting &&
|
||||||
|
virStoragePoolObjIsStarting(obj)) {
|
||||||
|
if (!virStoragePoolObjIsActive(obj))
|
||||||
|
virStoragePoolUpdateInactive(obj);
|
||||||
|
virStoragePoolObjSetStarting(obj, false);
|
||||||
|
}
|
||||||
virObjectEventStateQueue(driver->storageEventState, event);
|
virObjectEventStateQueue(driver->storageEventState, event);
|
||||||
virStoragePoolObjEndAPI(&obj);
|
virStoragePoolObjEndAPI(&obj);
|
||||||
return ret;
|
return ret;
|
||||||
@ -996,6 +1036,7 @@ storagePoolBuild(virStoragePoolPtr pool,
|
|||||||
virStoragePoolDefPtr def;
|
virStoragePoolDefPtr def;
|
||||||
virStorageBackendPtr backend;
|
virStorageBackendPtr backend;
|
||||||
virObjectEventPtr event = NULL;
|
virObjectEventPtr event = NULL;
|
||||||
|
bool restoreStarting = false;
|
||||||
int ret = -1;
|
int ret = -1;
|
||||||
|
|
||||||
if (!(obj = virStoragePoolObjFromStoragePool(pool)))
|
if (!(obj = virStoragePoolObjFromStoragePool(pool)))
|
||||||
@ -1015,6 +1056,16 @@ storagePoolBuild(virStoragePoolPtr pool,
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
|
virStoragePoolObjSetStarting(obj, true);
|
||||||
|
restoreStarting = true;
|
||||||
|
|
||||||
if (backend->buildPool &&
|
if (backend->buildPool &&
|
||||||
backend->buildPool(obj, flags) < 0)
|
backend->buildPool(obj, flags) < 0)
|
||||||
goto cleanup;
|
goto cleanup;
|
||||||
@ -1027,6 +1078,11 @@ storagePoolBuild(virStoragePoolPtr pool,
|
|||||||
ret = 0;
|
ret = 0;
|
||||||
|
|
||||||
cleanup:
|
cleanup:
|
||||||
|
if (restoreStarting &&
|
||||||
|
virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virStoragePoolUpdateInactive(obj);
|
||||||
|
virStoragePoolObjSetStarting(obj, false);
|
||||||
|
}
|
||||||
virObjectEventStateQueue(driver->storageEventState, event);
|
virObjectEventStateQueue(driver->storageEventState, event);
|
||||||
virStoragePoolObjEndAPI(&obj);
|
virStoragePoolObjEndAPI(&obj);
|
||||||
return ret;
|
return ret;
|
||||||
@ -1061,6 +1117,13 @@ storagePoolDestroy(virStoragePoolPtr pool)
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
||||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||||
_("pool '%s' has asynchronous jobs running."),
|
_("pool '%s' has asynchronous jobs running."),
|
||||||
@ -1126,6 +1189,13 @@ storagePoolDelete(virStoragePoolPtr pool,
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
||||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||||
_("pool '%s' has asynchronous jobs running."),
|
_("pool '%s' has asynchronous jobs running."),
|
||||||
@ -1189,6 +1259,13 @@ storagePoolRefresh(virStoragePoolPtr pool,
|
|||||||
goto cleanup;
|
goto cleanup;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (virStoragePoolObjIsStarting(obj)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("storage pool '%s' is starting up"),
|
||||||
|
def->name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
|
||||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||||
_("pool '%s' has asynchronous jobs running."),
|
_("pool '%s' has asynchronous jobs running."),
|
||||||
|
Loading…
Reference in New Issue
Block a user