[libvirt] [PATCH v1 21/31] network_conf: Make virNetworkObj actually virObject

Michal Privoznik mprivozn at redhat.com
Tue Mar 3 13:51:25 UTC 2015


On 03.03.2015 14:10, Peter Krempa wrote:
> On Tue, Mar 03, 2015 at 13:47:32 +0100, Michal Privoznik wrote:
>> On 03.03.2015 12:05, Peter Krempa wrote:
>>> On Thu, Feb 26, 2015 at 15:17:30 +0100, Michal Privoznik wrote:
>>>> So far it's just a structure which happens to have 'Obj' in its
>>>> name, but otherwise it not related to virObject at all. No
>>>> reference counting, not virObjectLock(), nothing.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>  cfg.mk                            |   2 -
>>>>  src/conf/network_conf.c           | 126 ++++++++++++++++++++------------------
>>>>  src/conf/network_conf.h           |   8 +--
>>>>  src/libvirt_private.syms          |   4 +-
>>>>  src/network/bridge_driver.c       |  56 ++++++++---------
>>>>  src/parallels/parallels_network.c |  16 ++---
>>>>  src/test/test_driver.c            |  32 +++++-----
>>>>  tests/networkxml2conftest.c       |   4 +-
>>>>  tests/objectlocking.ml            |   2 -
>>>>  9 files changed, 125 insertions(+), 125 deletions(-)
>>>>
>>>
>>> ...
>>>
>>>> @@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets,
>>>>  {
>>>>      size_t i;
>>>>  
>>>> -    virNetworkObjUnlock(net);
>>>> +    virObjectUnlock(net);
>>>>      for (i = 0; i < nets->count; i++) {
>>>> -        virNetworkObjLock(nets->objs[i]);
>>>> +        virObjectLock(nets->objs[i]);
>>>>          if (nets->objs[i] == net) {
>>>> -            virNetworkObjUnlock(nets->objs[i]);
>>>> -            virNetworkObjFree(nets->objs[i]);
>>>> +            virObjectUnlock(nets->objs[i]);
>>>> +            virObjectUnref(nets->objs[i]);
>>>>  
>>>>              VIR_DELETE_ELEMENT(nets->objs, i, nets->count);
>>>>              break;
>>>>          }
>>>> -        virNetworkObjUnlock(nets->objs[i]);
>>>> +        virObjectUnlock(nets->objs[i]);
>>>>      }
>>>>  }
>>>
>>> The function above is very strange. Your changes should not have any
>>> functional impact, but I think we should fix it right away.
>>>
>>> 1) why do we unlock the object that is going to be deleted ?
>>
>> To get the order of locking right.
> 
> Well, this would make it only simpler to deal with locking in the loop
> below since the code always grabs the network driver lock for every
> operation. And since ...
> 
>>
>>>
>>> 2) why do we bother locking the objects in between? We are comparing
>>> just the pointer to the object so there's no need to wait for the lock.
>>
>> Yep, I've saved that for a separate patch, which is not posted yet though.
> 
> ... locking of the object doesn't make sense there shouldn't be any
> issue.
> 
>>
>>>
>>> If we keep the original object locked there's no need to do any of that.
>>>
>>> 3) the network object is freed _before_ it's removed from the list.
>>> Although the list is always locked before access, I don't think that's a
>>> good practice.
>>
>> Not anymore. Any API that enters the remove function already has at
>> least one reference (obtained via virNetworkObjFind*). So this merely
>> decrease the refcount on the object since the list does no longer hold a
>> reference to the object.
> 
> That is not true at this point. At this point in the series there's only
> one reference ever in the list so that the object gets deleted in this
> function. You add reference counting in patch 28/31.

Okay, considered this squashed in:

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 8f140d2..3ea8975 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -620,17 +620,13 @@ void virNetworkRemoveInactive(virNetworkObjListPtr
nets,
 {
     size_t i;

-    virObjectUnlock(net);
     for (i = 0; i < nets->count; i++) {
-        virObjectLock(nets->objs[i]);
         if (nets->objs[i] == net) {
-            virObjectUnlock(nets->objs[i]);
-            virObjectUnref(nets->objs[i]);
-
             VIR_DELETE_ELEMENT(nets->objs, i, nets->count);
+            virObjectUnlock(net);
+            virObjectUnref(net);
             break;
         }
-        virObjectUnlock(nets->objs[i]);
     }
 }

Michal




More information about the libvir-list mailing list