diff mbox

[2/6] storage: Add storagePoolSetInactive

Message ID 731c614cd8cc8af3a6b5dc760bdb2bcbce0c7f4b.1466640165.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson June 23, 2016, 12:12 a.m. UTC 
This function handles newDef assignment and transient pool removal
when an object is set inactive. The return value notifies callers
if the pool was removed, so they know not to try to access the
pool object anymore.

Some users don't gain anything from it at this point, but future
patches will improve that.
---
 src/storage/storage_driver.c | 54 +++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 16 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson June 24, 2016, 1:36 p.m. UTC | #1 
On 06/24/2016 09:26 AM, Ján Tomko wrote:
> On Wed, Jun 22, 2016 at 08:12:12PM -0400, Cole Robinson wrote:

>> This function handles newDef assignment and transient pool removal

>> when an object is set inactive. The return value notifies callers

>> if the pool was removed, so they know not to try to access the

>> pool object anymore.

>>

>> Some users don't gain anything from it at this point, but future

>> patches will improve that.

>> ---

>> src/storage/storage_driver.c | 54 +++++++++++++++++++++++++++++++-------------

>> 1 file changed, 38 insertions(+), 16 deletions(-)

>>

>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c

>> index 3bdc13f..c7ffea8 100644

>> --- a/src/storage/storage_driver.c

>> +++ b/src/storage/storage_driver.c

>> @@ -76,6 +76,32 @@ static void storageDriverUnlock(void)

>>     virMutexUnlock(&driver->lock);

>> }

>>

>> +/*

>> + * storagePoolSetInactive:

>> + * Helper for setting a pool object as 'inactive'. Handles reassigning

>> + * newDef for persistent pools, and removing and freeing the object

>> + * for transient pools.

>> + *

>> + * Returns true if pool was removed from driver->pools

>> + */

>> +static bool

>> +storagePoolSetInactive(virStoragePoolObjPtr pool)

> 

> Can be void (virStoragePoolObjPtr *pool) and clear the original pointer

> if it was freed.


That is what Jovanka's similar patch did, but after playing with it some I
changed it to this. IMO it's safer to require the caller to set pool=NULL,
otherwise it isn't exactly clear just scanning the caller's code that pool can
become NULL, possibly leading to accidental NULL dereference if the code is
extended in the future, or new uses of the storagePoolSetInactive are added.
(I probably should have strapped on an ATTRIBUTE_RETURN_CHECK to be extra safe)

But I'm not married to the idea, so if you still prefer the other way I can
change it.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 3bdc13f..c7ffea8 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -76,6 +76,32 @@  static void storageDriverUnlock(void)
     virMutexUnlock(&driver->lock);
 }
 
+/*
+ * storagePoolSetInactive:
+ * Helper for setting a pool object as 'inactive'. Handles reassigning
+ * newDef for persistent pools, and removing and freeing the object
+ * for transient pools.
+ *
+ * Returns true if pool was removed from driver->pools
+ */
+static bool
+storagePoolSetInactive(virStoragePoolObjPtr pool)
+{
+    bool ret = false;
+    pool->active = false;
+
+    if (pool->configFile == NULL) {
+        virStoragePoolObjRemove(&driver->pools, pool);
+        ret = true;
+    } else if (pool->newDef) {
+        virStoragePoolDefFree(pool->def);
+        pool->def = pool->newDef;
+        pool->newDef = NULL;
+    }
+
+    return ret;
+}
+
 static void
 storagePoolUpdateState(virStoragePoolObjPtr pool)
 {
@@ -182,6 +208,10 @@  storageDriverAutostartPool(virConnectPtr conn,
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to autostart storage pool '%s': %s"),
                        pool->def->name, virGetLastErrorMessage());
+
+        /* Don't check the return value, it should never be 'true' here
+         * since this function requires a non-transient pool */
+        storagePoolSetInactive(pool);
         goto cleanup;
     }
 
@@ -739,8 +769,8 @@  storagePoolCreateXML(virConnectPtr conn,
             unlink(stateFile);
         if (backend->stopPool)
             backend->stopPool(conn, pool);
-        virStoragePoolObjRemove(&driver->pools, pool);
-        pool = NULL;
+        if (storagePoolSetInactive(pool))
+            pool = NULL;
         goto cleanup;
     }
 
@@ -956,6 +986,10 @@  storagePoolCreate(virStoragePoolPtr obj,
             unlink(stateFile);
         if (backend->stopPool)
             backend->stopPool(obj->conn, pool);
+
+        /* Don't check the return value, it should never be 'true' here
+         * since this function requires a non-transient pool */
+        storagePoolSetInactive(pool);
         goto cleanup;
     }
 
@@ -1070,16 +1104,8 @@  storagePoolDestroy(virStoragePoolPtr obj)
                                             VIR_STORAGE_POOL_EVENT_STOPPED,
                                             0);
 
-    pool->active = false;
-
-    if (pool->configFile == NULL) {
-        virStoragePoolObjRemove(&driver->pools, pool);
+    if (storagePoolSetInactive(pool))
         pool = NULL;
-    } else if (pool->newDef) {
-        virStoragePoolDefFree(pool->def);
-        pool->def = pool->newDef;
-        pool->newDef = NULL;
-    }
 
     ret = 0;
 
@@ -1199,12 +1225,8 @@  storagePoolRefresh(virStoragePoolPtr obj,
                                                 pool->def->uuid,
                                                 VIR_STORAGE_POOL_EVENT_STOPPED,
                                                 0);
-        pool->active = false;
-
-        if (pool->configFile == NULL) {
-            virStoragePoolObjRemove(&driver->pools, pool);
+        if (storagePoolSetInactive(pool))
             pool = NULL;
-        }
         goto cleanup;
     }