On 21.07.2016 11:57, Marc-André Lureau wrote: > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > Many code paths assume get_vhost_net() returns non-null. > > Keep VhostUserState.vhost_net after a successful vhost_net_init(), > instead of freeing it in vhost_net_cleanup(). > > VhostUserState.vhost_net is thus freed before after being recreated or > on final vhost_user_cleanup() and there is no need to save the acked > features. > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > hw/net/vhost_net.c | 1 - > net/tap.c | 1 + > net/vhost-user.c | 36 ++++++++++++++++-------------------- > 3 files changed, 17 insertions(+), 21 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index c11f69c..7b76591 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -378,7 +378,6 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState > *ncs, > void vhost_net_cleanup(struct vhost_net *net) > { > vhost_dev_cleanup(&net->dev); > - g_free(net); > } > > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr) > diff --git a/net/tap.c b/net/tap.c > index 40a8c74..6abb962 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -312,6 +312,7 @@ static void tap_cleanup(NetClientState *nc) > > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net = NULL; > } > > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 2af212b..60fab9a 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -23,7 +23,6 @@ typedef struct VhostUserState { > CharDriverState *chr; > VHostNetState *vhost_net; > guint watch; > - uint64_t acked_features; > } VhostUserState; > > typedef struct VhostUserChardevProps { > @@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc) > { > VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); > assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER); > - return s->acked_features; > -} > - > -static int vhost_user_running(VhostUserState *s) > -{ > - return (s->vhost_net) ? 1 : 0; > + return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0; > } > > static void vhost_user_stop(int queues, NetClientState *ncs[]) > @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientState > *ncs[]) > assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER); > > s = DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (!vhost_user_running(s)) { > - continue; > - } > > if (s->vhost_net) { > - /* save acked features */ > - s->acked_features = vhost_net_get_acked_features(s->vhost_net); > vhost_net_cleanup(s->vhost_net); > - s->vhost_net = NULL; > } > } > } > @@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClientState > *ncs[]) > static int vhost_user_start(int queues, NetClientState *ncs[]) > { > VhostNetOptions options; > + struct vhost_net *net = NULL; > VhostUserState *s; > int max_queues; > int i; > @@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClientState > *ncs[]) > assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER); > > s = DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (vhost_user_running(s)) { > - continue; > - } > > options.net_backend = ncs[i]; > options.opaque = s->chr; > options.busyloop_timeout = 0; > - s->vhost_net = vhost_net_init(&options); > - if (!s->vhost_net) { > + net = vhost_net_init(&options); > + if (!net) { > error_report("failed to init vhost_net for queue %d", i); > goto err; > } > > if (i == 0) { > - max_queues = vhost_net_get_max_queues(s->vhost_net); > + max_queues = vhost_net_get_max_queues(net); > if (queues > max_queues) { > error_report("you are asking more queues than supported: %d", > max_queues); > goto err; > } > } > + > + if (s->vhost_net) { > + vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > + } > + s->vhost_net = net; > } > > return 0; > > err: > - vhost_user_stop(i + 1, ncs); > + if (net) { > + vhost_net_cleanup(net); > + } > + vhost_user_stop(i, ncs); > return -1; > } > > @@ -150,6 +145,7 @@ static void vhost_user_cleanup(NetClientState *nc) > > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net = NULL; > } > if (s->chr) { >
In patch number 7 of this patch set was introduced 'memset()' inside 'vhost_dev_cleanup()' which clears 'acked_features' field of 'vhost_dev' structure. This patch uses already zeroed value of this field for features restore after reconnection. You should not remove 'acked_features' from 'struct VhostUserState' or avoid cleaning of this field in 'vhost_dev'. I'm suggesting following fixup (mainly, just a partial revert): ---------------------------------------------------------------------- diff --git a/net/vhost-user.c b/net/vhost-user.c index 60fab9a..3100a5e 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -23,6 +23,7 @@ typedef struct VhostUserState { CharDriverState *chr; VHostNetState *vhost_net; guint watch; + uint64_t acked_features; } VhostUserState; typedef struct VhostUserChardevProps { @@ -41,7 +42,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc) { VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER); - return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0; + return s->acked_features; } static void vhost_user_stop(int queues, NetClientState *ncs[]) @@ -55,6 +56,11 @@ static void vhost_user_stop(int queues, NetClientState *ncs[]) s = DO_UPCAST(VhostUserState, nc, ncs[i]); if (s->vhost_net) { + /* save acked features */ + uint64_t features = vhost_net_get_acked_features(s->vhost_net); + if (features) { + s->acked_features = features; + } vhost_net_cleanup(s->vhost_net); } } ---------------------------------------------------------------------- Best regards, Ilya Maximets.