The current mechanism doesn't lock each element in devs->objs as it's
looking at it, rather it keeps locking/unlocking the passed element for
which the removal is being attempted.  Fix things to lock each element
as we're looking at them.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnodedeviceobj.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 02ac544..1352720 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -281,16 +281,16 @@ virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs,
     virNodeDeviceObjUnlock(*dev);
 
     for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjLock(*dev);
+        virNodeDeviceObjLock(devs->objs[i]);
         if (devs->objs[i] == *dev) {
-            virNodeDeviceObjUnlock(*dev);
+            virNodeDeviceObjUnlock(devs->objs[i]);
             virNodeDeviceObjFree(devs->objs[i]);
             *dev = NULL;
 
             VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
             break;
         }
-        virNodeDeviceObjUnlock(*dev);
+        virNodeDeviceObjUnlock(devs->objs[i]);
     }
 }
 
-- 
2.9.4

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

On Thu, May 25, 2017 at 15:56:59 -0400, John Ferlan wrote:
> The current mechanism doesn't lock each element in devs->objs as it's
> looking at it, rather it keeps locking/unlocking the passed element for
> which the removal is being attempted.  Fix things to lock each element
> as we're looking at them.

Since you are comparing only the pointer of the item locking it is
really not necessary.

Any other justification for this change?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/26/2017 02:57 AM, Peter Krempa wrote:
> On Thu, May 25, 2017 at 15:56:59 -0400, John Ferlan wrote:
>> The current mechanism doesn't lock each element in devs->objs as it's
>> looking at it, rather it keeps locking/unlocking the passed element for
>> which the removal is being attempted.  Fix things to lock each element
>> as we're looking at them.
> 
> Since you are comparing only the pointer of the item locking it is
> really not necessary.
> 
> Any other justification for this change?
> 

Beyond making this like other vir.*ObjRemove APIs, no and fixing what
would be an "obvious" coding error.

I suppose the argument is that the driver/test lock wouldn't allow
change to the list so true technically it doesn't matter. I can drop
this if that's what you'd prefer.  Eventually it goes away completely,
but that's code I have in my branches.


John

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