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

Michal Privoznik mprivozn at redhat.com
Tue Mar 3 12:47:32 UTC 2015


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.

> 
> 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.

> 
> 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.

> 
> I'd like to see the function fixed either separately or as a follow up.
> I'd also like to see that patch before.
> 
> ACK to the rest though, this can be fixed in a individual patch.
> 
> Peter
> 

Michal




More information about the libvir-list mailing list