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