[PATCH v2] virnodedeviceobj: Don't unlock virNodeDeviceObj in virNodeDeviceObjListRemove()

Daniel Henrique Barboza danielhb413 at gmail.com
Wed Feb 2 20:16:50 UTC 2022



On 2/2/22 06:00, Michal Privoznik wrote:
> When virNodeDeviceObjListRemove() is called, the passed
> virNodeDeviceObj is removed from internal list of node devices
> and then unrefed and unlocked. While the former is warranted (the
> object was refed at the beginning of the function) the unlock is
> not. In fact, it's wrong from conceptual POV. We still want
> threads working on the object tu mutually exclude each other.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>

> 
> v2 of:
> 
> https://listman.redhat.com/archives/libvir-list/2022-February/msg00036.html
> 
> diff to v1:
> - Fixed test driver which worked around broken logic
> 
> Note, there is similar problem with virNodeDeviceObjListForEachRemove()
> which removes objects from the list, even without lock, but that happens
> only with mdevs and I have none to test with.
> 
>   src/conf/virnodedeviceobj.c | 2 +-
>   src/test/test_driver.c      | 4 +---
>   2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 2e4ef2df3c..7a560349d4 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -524,7 +524,7 @@ virNodeDeviceObjListRemove(virNodeDeviceObjList *devs,
>       virObjectRWLockWrite(devs);
>       virObjectLock(obj);
>       virNodeDeviceObjListRemoveLocked(devs, obj);
> -    virNodeDeviceObjEndAPI(&obj);
> +    virObjectUnref(obj);
>       virObjectRWUnlock(devs);
>   }
>   
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 9e1fc65972..f900123941 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -6810,7 +6810,7 @@ testDestroyVport(testDriver *privconn,
>                                              0);
>   
>       virNodeDeviceObjListRemove(privconn->devs, obj);
> -    virObjectUnref(obj);
> +    virNodeDeviceObjEndAPI(&obj);
>   
>       virObjectEventStateQueue(privconn->eventState, event);
>       return 0;
> @@ -7797,8 +7797,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
>   
>       virObjectLock(obj);
>       virNodeDeviceObjListRemove(driver->devs, obj);
> -    virObjectUnref(obj);
> -    obj = NULL;
>   
>    cleanup:
>       virNodeDeviceObjEndAPI(&obj);




More information about the libvir-list mailing list