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>

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 4af8f5eb0ab6c3202907eb308284c979f8f38dd1..286192172d8ccf80b55ee992789660ea54584df0 100644 (file)
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -86,6 +86,7 @@ struct _virStoragePoolObj {
     char *configFile;
     char *autostartLink;
     bool active;
+    bool starting;
     bool autostart;
     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
 virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj)
 {
@@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
                                obj->def->name);
                 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);
@@ -1510,7 +1533,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj,
                            virStoragePoolDefPtr def,
                            unsigned int flags)
 {
-    if (virStoragePoolObjIsActive(obj)) {
+    if (virStoragePoolObjIsActive(obj) ||
+        virStoragePoolObjIsStarting(obj)) {
         virStoragePoolDefFree(obj->newDef);
         obj->newDef = def;
     } else {

diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index 0af4b2c8218d2660808c2ee0fde008731085d531..54100ac22a3ec2a1056ec71a3436871ce8363eca 100644 (file)
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -94,6 +94,12 @@ void
 virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
                            bool active);
 
+void
+virStoragePoolObjSetStarting(virStoragePoolObjPtr obj,
+                             bool starting);
+bool
+virStoragePoolObjIsStarting(virStoragePoolObjPtr obj);
+
 bool
 virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj);
 

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 73cc2916bd185f98304e63f7f20220d7cbaee645..fc749e76c3e8132070a91916b86d24a29e1b5965 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1246,6 +1246,7 @@ virStoragePoolObjGetVolumesCount;
 virStoragePoolObjIncrAsyncjobs;
 virStoragePoolObjIsActive;
 virStoragePoolObjIsAutostart;
+virStoragePoolObjIsStarting;
 virStoragePoolObjListAdd;
 virStoragePoolObjListExport;
 virStoragePoolObjListForEach;
@@ -1264,6 +1265,7 @@ virStoragePoolObjSetActive;
 virStoragePoolObjSetAutostart;
 virStoragePoolObjSetConfigFile;
 virStoragePoolObjSetDef;
+virStoragePoolObjSetStarting;
 virStoragePoolObjVolumeGetNames;
 virStoragePoolObjVolumeListExport;
 

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ebf3f78752949ae9e2033e674014e5c01dc5f420..cd9f14a2c0b3afb633f4b6e682e21775cb304700 100644 (file)
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -202,12 +202,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;
     }
@@ -226,6 +228,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
             virStoragePoolObjSetActive(obj, true);
         }
     }
+
+ cleanup:
+    if (virStoragePoolObjIsStarting(obj)) {
+        if (!virStoragePoolObjIsActive(obj))
+            virStoragePoolUpdateInactive(obj);
+        virStoragePoolObjSetStarting(obj, false);
+    }
 }
 
 
@@ -761,6 +770,8 @@ storagePoolCreateXML(virConnectPtr conn,
     newDef = NULL;
     def = virStoragePoolObjGetDef(obj);
 
+    virStoragePoolObjSetStarting(obj, true);
+
     if (backend->buildPool) {
         if (flags & VIR_STORAGE_POOL_CREATE_WITH_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);
 
  cleanup:
+    if (virStoragePoolObjIsStarting(obj)) {
+        if (!virStoragePoolObjIsActive(obj))
+            virStoragePoolUpdateInactive(obj);
+        virStoragePoolObjSetStarting(obj, false);
+    }
     virObjectEventStateQueue(driver->storageEventState, event);
     virStoragePoolObjEndAPI(&obj);
     return pool;
@@ -879,6 +895,13 @@ storagePoolUndefine(virStoragePoolPtr pool)
         goto cleanup;
     }
 
+    if (virStoragePoolObjIsStarting(obj)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("storage pool '%s' is starting up"),
+                       def->name);
+        goto cleanup;
+    }
+
     if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("pool '%s' has asynchronous jobs running."),
@@ -923,6 +946,7 @@ storagePoolCreate(virStoragePoolPtr pool,
     int ret = -1;
     unsigned int build_flags = 0;
     VIR_AUTOFREE(char *) stateFile = NULL;
+    bool restoreStarting = false;
 
     virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
                   VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
@@ -948,6 +972,16 @@ storagePoolCreate(virStoragePoolPtr pool,
         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 (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
             build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
@@ -983,6 +1017,12 @@ storagePoolCreate(virStoragePoolPtr pool,
     ret = 0;
 
  cleanup:
+    if (restoreStarting &&
+        virStoragePoolObjIsStarting(obj)) {
+        if (!virStoragePoolObjIsActive(obj))
+            virStoragePoolUpdateInactive(obj);
+        virStoragePoolObjSetStarting(obj, false);
+    }
     virObjectEventStateQueue(driver->storageEventState, event);
     virStoragePoolObjEndAPI(&obj);
     return ret;
@@ -996,6 +1036,7 @@ storagePoolBuild(virStoragePoolPtr pool,
     virStoragePoolDefPtr def;
     virStorageBackendPtr backend;
     virObjectEventPtr event = NULL;
+    bool restoreStarting = false;
     int ret = -1;
 
     if (!(obj = virStoragePoolObjFromStoragePool(pool)))
@@ -1015,6 +1056,16 @@ storagePoolBuild(virStoragePoolPtr pool,
         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 &&
         backend->buildPool(obj, flags) < 0)
         goto cleanup;
@@ -1027,6 +1078,11 @@ storagePoolBuild(virStoragePoolPtr pool,
     ret = 0;
 
  cleanup:
+    if (restoreStarting &&
+        virStoragePoolObjIsStarting(obj)) {
+        virStoragePoolUpdateInactive(obj);
+        virStoragePoolObjSetStarting(obj, false);
+    }
     virObjectEventStateQueue(driver->storageEventState, event);
     virStoragePoolObjEndAPI(&obj);
     return ret;
@@ -1061,6 +1117,13 @@ storagePoolDestroy(virStoragePoolPtr pool)
         goto cleanup;
     }
 
+    if (virStoragePoolObjIsStarting(obj)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("storage pool '%s' is starting up"),
+                       def->name);
+        goto cleanup;
+    }
+
     if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("pool '%s' has asynchronous jobs running."),
@@ -1126,6 +1189,13 @@ storagePoolDelete(virStoragePoolPtr pool,
         goto cleanup;
     }
 
+    if (virStoragePoolObjIsStarting(obj)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("storage pool '%s' is starting up"),
+                       def->name);
+        goto cleanup;
+    }
+
     if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("pool '%s' has asynchronous jobs running."),
@@ -1189,6 +1259,13 @@ storagePoolRefresh(virStoragePoolPtr pool,
         goto cleanup;
     }
 
+    if (virStoragePoolObjIsStarting(obj)) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("storage pool '%s' is starting up"),
+                       def->name);
+        goto cleanup;
+    }
+
     if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("pool '%s' has asynchronous jobs running."),