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

[Peter Krempa]

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

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.

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150303/3959c523/attachment-0001.sig>