Re: [Qemu-devel] [PATCH 23/24] vhost-user: wait until link is up

[Michael S. Tsirkin]

On Thu, Jun 23, 2016 at 05:11:37AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Jun 21, 2016 at 12:02:51PM +0200, address@hidden wrote:
> > > From: Marc-André Lureau <address@hidden>
> > > 
> > > The chardev waits for an initial connection before starting qemu,
> > > vhost-user wants the backend negotiation to be completed. vhost-user is
> > > started in the net_vhost_user_event callback, which is synchronously
> > > called after the socket is connected.
> > > 
> > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > ---
> > >  net/vhost-user.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > index 95ed2d2..bb4a253 100644
> > > --- a/net/vhost-user.c
> > > +++ b/net/vhost-user.c
> > > @@ -248,7 +248,15 @@ static int net_vhost_user_init(NetClientState *peer,
> > > const char *device,
> > >          s->chr = chr;
> > >      }
> > >  
> > > -    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event,
> > > nc[0].name);
> > > +    do {
> > > +        Error *err = NULL;
> > > +        if (qemu_chr_wait_connected(chr, &err) < 0) {
> > > +            error_report_err(err);
> > > +            return -1;
> > > +        }
> > > +        qemu_chr_add_handlers(chr, NULL, NULL,
> > > +                              net_vhost_user_event, nc[0].name);
> > > +    } while (nc->link_down);
> > >  
> > >      assert(s->vhost_net != NULL);
> > 
> > 
> > I don't get why this makes sense.
> > Why should vhost user poke at link down at all?
> 
> It should wait for an initial valid connection (see the test case). Only 
> after vhost_user_start() should it keep going. We could have 
> VhostUserState.ready set to true when done, or we could check after 
> qmp_set_link(.., true) status, like I did here. Does that make sense?
> 
> thanks

I'd prefer something well contained. A dedicated flags sounds
reasonable.

        commit d39c87d70763f2755d1d7a719817b06f0281fb01
        Author: Wen Congyang <address@hidden>
        Date:   Wed Nov 11 14:53:29 2015 +0800

            vhost-user: set link down when the char device is closed

was a quick hack. Disconnect should be transparent to guest and not
cause dhcp renegotiation and stuff.

The reason it's there is because we do not yet handle tx packets
when disconnected, so it reduces the chance of tx watchdog if we tell guest it
should not transmit anything new.

Once we fix that we will drop the link down thing, so don't rely on link
state please.

-- 
MST