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

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(a)redhat.com&gt; >>> --- >>> 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.