Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> With refcnt, NetClientState's user can run agaist deleter.

Please split this into two patches:

1. net_clients lock
2. NetClientState refcount

> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/core/qdev-properties-system.c | 14 ++++++++++++
>  include/net/net.h                |  3 +++
>  net/hub.c                        |  3 +++
>  net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
>  net/slirp.c                      |  3 ++-
>  5 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 0eada32..41cc7e6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -302,6 +302,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>  
> +    /* inc ref, released when unset property */
>      hubport = net_hub_port_find(id);
>      if (!hubport) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> @@ -311,11 +312,24 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>      *ptr = hubport;
>  }
>  
> +static void release_vlan(Object *obj, const char *name, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
> +    NetClientState **ptr = &peers_ptr->ncs[0];
> +
> +    if (*ptr) {
> +        netclient_unref(*ptr);
> +    }
> +}
> +
>  PropertyInfo qdev_prop_vlan = {
>      .name  = "vlan",
>      .print = print_vlan,
>      .get   = get_vlan,
>      .set   = set_vlan,
> +    .release = release_vlan,
>  };
>  
>  int qdev_prop_set_drive(DeviceState *dev, const char *name,

What about the netdev property?  I don't see any refcount code there.

> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>              qemu_del_net_client(nc);
>          }
>      }
> +    qemu_mutex_destroy(&net_clients_lock);

Why is it okay to iterate over net_clients here without the lock?