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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list