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