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

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.

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