| Message ID | 1466503372-28334-24-git-send-email-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 21, 2016 at 12:02:51PM +0200, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > 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 <marcandre.lureau@redhat.com> > --- > 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? > -- > 2.7.4
Hi ----- Original Message ----- > On Tue, Jun 21, 2016 at 12:02:51PM +0200, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > 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 <marcandre.lureau@redhat.com> > > --- > > 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
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, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > 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 <marcandre.lureau@redhat.com> > > > --- > > > 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 <wency@cn.fujitsu.com> 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.
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);