1. Patchew
  2. Libvirt
  3. View series

[PATCH] Unlock the storage pool objects after looking it up

Yi Li posted 1 patch 5 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200507104855.2868736-1-yili@winhong.com
There is a newer version of this series
src/conf/virstorageobj.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] Unlock the storage pool objects after looking it up
Posted by Yi Li 5 years, 9 months ago 
The lock should be released.

Signed-off-by: Yi Li <yili@winhong.com>
---
 src/conf/virstorageobj.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 13b75b648d..9f24ae67ca 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -2077,8 +2077,10 @@ virStoragePoolObjListExport(virConnectPtr conn,
 
     virObjectRWLockRead(poolobjs);
 
-    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0)
-        goto error;
+    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0) {
+        virObjectRWUnlock(poolobjs);
+        return -1;
+    }
 
     virHashForEach(poolobjs->objs, virStoragePoolObjListExportCallback, &data);
     virObjectRWUnlock(poolobjs);
-- 
2.25.3
Re: [PATCH] Unlock the storage pool objects after looking it up
Posted by Peter Krempa 5 years, 9 months ago 
On Thu, May 07, 2020 at 18:48:55 +0800, Yi Li wrote:
> The lock should be released.
> 
> Signed-off-by: Yi Li <yili@winhong.com>
> ---
>  src/conf/virstorageobj.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 13b75b648d..9f24ae67ca 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -2077,8 +2077,10 @@ virStoragePoolObjListExport(virConnectPtr conn,
>  
>      virObjectRWLockRead(poolobjs);
>  
> -    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0)
> -        goto error;
> +    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0) {
> +        virObjectRWUnlock(poolobjs);
> +        return -1;
> +    }

In this instance, I'd prefer if VIR_ALLOC_N is replaced by g_new0 which
will not fail and thus the unlock will not be necessary as there will be
no cleanup path.
Re: [PATCH] Unlock the storage pool objects after looking it up
Posted by 李义 5 years, 9 months ago 
On 5/7/20, Peter Krempa <pkrempa@redhat.com> wrote:
> On Thu, May 07, 2020 at 18:48:55 +0800, Yi Li wrote:
>> The lock should be released.
>>
>> Signed-off-by: Yi Li <yili@winhong.com>
>> ---
>>  src/conf/virstorageobj.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
>> index 13b75b648d..9f24ae67ca 100644
>> --- a/src/conf/virstorageobj.c
>> +++ b/src/conf/virstorageobj.c
>> @@ -2077,8 +2077,10 @@ virStoragePoolObjListExport(virConnectPtr conn,
>>
>>      virObjectRWLockRead(poolobjs);
>>
>> -    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1)
>> < 0)
>> -        goto error;
>> +    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1)
>> < 0) {
>> +        virObjectRWUnlock(poolobjs);
>> +        return -1;
>> +    }
>
> In this instance, I'd prefer if VIR_ALLOC_N is replaced by g_new0 which
> will not fail and thus the unlock will not be necessary as there will be
> no cleanup path.
>
>
Thanks, i will redo it.
[PATCH v2] Unlock the storage pool objects after looking it up
Posted by Yi Li 5 years, 9 months ago 
Use g_new0 to allocate and remove NULL checks from callers
and the lock will release properly

Signed-off-by: Yi Li <yili@winhong.com>
---
 src/conf/virstorageobj.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 13b75b648d..f3c54d0c52 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1036,10 +1036,7 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
         return ret;
     }
 
-    if (VIR_ALLOC_N(data.vols, virHashSize(volumes->objsName) + 1) < 0) {
-        virObjectRWUnlock(volumes);
-        return -1;
-    }
+    data.vols = g_new0(virStorageVolPtr, virHashSize(volumes->objsName) + 1);
 
     virHashForEach(volumes->objsName, virStoragePoolObjVolumeListExportCallback, &data);
     virObjectRWUnlock(volumes);
@@ -2077,8 +2074,13 @@ virStoragePoolObjListExport(virConnectPtr conn,
 
     virObjectRWLockRead(poolobjs);
 
-    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0)
-        goto error;
+    if (!pools) {
+        int ret = virHashSize(poolobjs->objs);
+        virObjectRWUnlock(poolobjs);
+        return ret;
+    }
+
+    data.pools = g_new0(virStoragePoolPtr, virHashSize(poolobjs->objs) + 1);
 
     virHashForEach(poolobjs->objs, virStoragePoolObjListExportCallback, &data);
     virObjectRWUnlock(poolobjs);
-- 
2.25.3
Re: [PATCH v2] Unlock the storage pool objects after looking it up
Posted by 李义 5 years, 8 months ago 
ping

On Fri, May 8, 2020 at 12:23 AM Yi Li <yili@winhong.com> wrote:
>
> Use g_new0 to allocate and remove NULL checks from callers
> and the lock will release properly
>
> Signed-off-by: Yi Li <yili@winhong.com>
> ---
>  src/conf/virstorageobj.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 13b75b648d..f3c54d0c52 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -1036,10 +1036,7 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
>          return ret;
>      }
>
> -    if (VIR_ALLOC_N(data.vols, virHashSize(volumes->objsName) + 1) < 0) {
> -        virObjectRWUnlock(volumes);
> -        return -1;
> -    }
> +    data.vols = g_new0(virStorageVolPtr, virHashSize(volumes->objsName) + 1);
>
>      virHashForEach(volumes->objsName, virStoragePoolObjVolumeListExportCallback, &data);
>      virObjectRWUnlock(volumes);
> @@ -2077,8 +2074,13 @@ virStoragePoolObjListExport(virConnectPtr conn,
>
>      virObjectRWLockRead(poolobjs);
>
> -    if (pools && VIR_ALLOC_N(data.pools, virHashSize(poolobjs->objs) + 1) < 0)
> -        goto error;
> +    if (!pools) {
> +        int ret = virHashSize(poolobjs->objs);
> +        virObjectRWUnlock(poolobjs);
> +        return ret;
> +    }
> +
> +    data.pools = g_new0(virStoragePoolPtr, virHashSize(poolobjs->objs) + 1);
>
>      virHashForEach(poolobjs->objs, virStoragePoolObjListExportCallback, &data);
>      virObjectRWUnlock(poolobjs);
> --
> 2.25.3
>
>
>
Re: [PATCH v2] Unlock the storage pool objects after looking it up
Posted by Michal Privoznik 5 years, 8 months ago 
On 5/7/20 6:23 PM, Yi Li wrote:
> Use g_new0 to allocate and remove NULL checks from callers
> and the lock will release properly
> 
> Signed-off-by: Yi Li <yili@winhong.com>
> ---
>   src/conf/virstorageobj.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And pushed.

Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.