[libvirt] [PATCH v1 01/11] virStoragePoolObjRemove: Don't unlock pool object upon return

Michal Privoznik posted 11 patches 5 years, 6 months ago
[libvirt] [PATCH v1 01/11] virStoragePoolObjRemove: Don't unlock pool object upon return
Posted by Michal Privoznik 5 years, 6 months ago
The fact that we're removing a pool object from the list of pools
doesn't mean we want to unlock it. It violates locking policy
too as object locking and unlocking is not done on the same
level.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/virstorageobj.c     |  5 ++---
 src/storage/storage_driver.c | 17 +++--------------
 src/test/test_driver.c       | 14 +++-----------
 3 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 31b5af8e9e..6af4a1a22d 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -514,7 +514,6 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
     virObjectLock(obj);
     virHashRemoveEntry(pools->objs, uuidstr);
     virHashRemoveEntry(pools->objsName, obj->def->name);
-    virObjectUnlock(obj);
     virObjectUnref(obj);
     virObjectRWUnlock(pools);
 }
@@ -1594,13 +1593,13 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
     VIR_FREE(obj->configFile);  /* for driver reload */
     if (VIR_STRDUP(obj->configFile, path) < 0) {
         virStoragePoolObjRemove(pools, obj);
-        virObjectUnref(obj);
+        virStoragePoolObjEndAPI(&obj);
         return NULL;
     }
     VIR_FREE(obj->autostartLink); /* for driver reload */
     if (VIR_STRDUP(obj->autostartLink, autostartLink) < 0) {
         virStoragePoolObjRemove(pools, obj);
-        virObjectUnref(obj);
+        virStoragePoolObjEndAPI(&obj);
         return NULL;
     }
 
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 496d51b1e0..c14620765e 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -125,8 +125,7 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr)
 
     if (!virStoragePoolObjGetConfigFile(obj)) {
         virStoragePoolObjRemove(driver->pools, obj);
-        virObjectUnref(obj);
-        *objptr = NULL;
+        virStoragePoolObjEndAPI(objptr);
     } else if (virStoragePoolObjGetNewDef(obj)) {
         virStoragePoolObjDefUseNewDef(obj);
     }
@@ -760,12 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
 
         if (build_flags ||
             (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) {
-            if (backend->buildPool(obj, build_flags) < 0) {
-                virStoragePoolObjRemove(driver->pools, obj);
-                virObjectUnref(obj);
-                obj = NULL;
-                goto cleanup;
-            }
+            if (backend->buildPool(obj, build_flags) < 0)
+                goto error;
         }
     }
 
@@ -798,8 +793,6 @@ storagePoolCreateXML(virConnectPtr conn,
 
  error:
     virStoragePoolObjRemove(driver->pools, obj);
-    virObjectUnref(obj);
-    obj = NULL;
     goto cleanup;
 }
 
@@ -835,8 +828,6 @@ storagePoolDefineXML(virConnectPtr conn,
 
     if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def) < 0) {
         virStoragePoolObjRemove(driver->pools, obj);
-        virObjectUnref(obj);
-        obj = NULL;
         newDef = NULL;
         goto cleanup;
     }
@@ -903,8 +894,6 @@ storagePoolUndefine(virStoragePoolPtr pool)
 
     VIR_INFO("Undefining storage pool '%s'", def->name);
     virStoragePoolObjRemove(driver->pools, obj);
-    virObjectUnref(obj);
-    obj = NULL;
     ret = 0;
 
  cleanup:
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 7fd06fcfa8..a0f19f5c5c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4595,16 +4595,12 @@ testStoragePoolCreateXML(virConnectPtr conn,
                             def->source.adapter.data.fchost.wwnn,
                             def->source.adapter.data.fchost.wwpn) < 0) {
             virStoragePoolObjRemove(privconn->pools, obj);
-            virObjectUnref(obj);
-            obj = NULL;
             goto cleanup;
         }
     }
 
     if (testStoragePoolObjSetDefaults(obj) == -1) {
         virStoragePoolObjRemove(privconn->pools, obj);
-        virObjectUnref(obj);
-        obj = NULL;
         goto cleanup;
     }
 
@@ -4662,8 +4658,6 @@ testStoragePoolDefineXML(virConnectPtr conn,
 
     if (testStoragePoolObjSetDefaults(obj) == -1) {
         virStoragePoolObjRemove(privconn->pools, obj);
-        virObjectUnref(obj);
-        obj = NULL;
         goto cleanup;
     }
 
@@ -4692,7 +4686,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool)
                                             0);
 
     virStoragePoolObjRemove(privconn->pools, obj);
-    virObjectUnref(obj);
+    virStoragePoolObjEndAPI(&obj);
 
     virObjectEventStateQueue(privconn->eventState, event);
     return 0;
@@ -4784,11 +4778,9 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
                                             VIR_STORAGE_POOL_EVENT_STOPPED,
                                             0);
 
-    if (!(virStoragePoolObjGetConfigFile(obj))) {
+    if (!(virStoragePoolObjGetConfigFile(obj)))
         virStoragePoolObjRemove(privconn->pools, obj);
-        virObjectUnref(obj);
-        obj = NULL;
-    }
+
     ret = 0;
 
  cleanup:
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/11] virStoragePoolObjRemove: Don't unlock pool object upon return
Posted by Ján Tomko 5 years, 3 months ago
On Fri, May 24, 2019 at 04:35:37PM +0200, Michal Privoznik wrote:
>The fact that we're removing a pool object from the list of pools
>doesn't mean we want to unlock it. It violates locking policy
>too as object locking and unlocking is not done on the same
>level.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/virstorageobj.c     |  5 ++---
> src/storage/storage_driver.c | 17 +++--------------
> src/test/test_driver.c       | 14 +++-----------
> 3 files changed, 8 insertions(+), 28 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

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