On Thu, Jan 10, 2019 at 09:19:41PM +0800, Yongji Xie wrote:
> On Thu, 10 Jan 2019 at 20:50, Daniel P. Berrangé <berra...@redhat.com> wrote:
> >
> > On Wed, Jan 09, 2019 at 07:27:22PM +0800, elohi...@gmail.com wrote:
> > > From: Xie Yongji <xieyon...@baidu.com>
> > >
> > > Enable "nowait" option to make QEMU not do a connect
> > > on client sockets during initialization of the chardev.
> > > Then we can use qemu_chr_fe_wait_connected() to connect
> > > when necessary. Now it would be used for unix domain
> > > socket of vhost-user-blk device to support reconnect.
> > >
> > > Signed-off-by: Xie Yongji <xieyon...@baidu.com>
> > > Signed-off-by: Zhang Yu <zhangy...@baidu.com>
> > > ---
> > > chardev/char-socket.c | 56 +++++++++++++++++++++----------------------
> > > qapi/char.json | 3 +--
> > > qemu-options.hx | 9 ++++---
> > > 3 files changed, 35 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index eaa8e8b68f..f803f4f7d3 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -1072,37 +1072,37 @@ static void qmp_chardev_open_socket(Chardev *chr,
> > > s->reconnect_time = reconnect;
> > > }
> > >
> > > - if (s->reconnect_time) {
> > > - tcp_chr_connect_async(chr);
> > > - } else {
> > > - if (s->is_listen) {
> > > - char *name;
> > > - s->listener = qio_net_listener_new();
> > > + if (s->is_listen) {
> > > + char *name;
> > > + s->listener = qio_net_listener_new();
> > >
> > > - name = g_strdup_printf("chardev-tcp-listener-%s",
> > > chr->label);
> > > - qio_net_listener_set_name(s->listener, name);
> > > - g_free(name);
> > > + name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
> > > + qio_net_listener_set_name(s->listener, name);
> > > + g_free(name);
> > >
> > > - if (qio_net_listener_open_sync(s->listener, s->addr, errp) <
> > > 0) {
> > > - object_unref(OBJECT(s->listener));
> > > - s->listener = NULL;
> > > - goto error;
> > > - }
> > > + if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
> > > + object_unref(OBJECT(s->listener));
> > > + s->listener = NULL;
> > > + goto error;
> > > + }
> > >
> > > - qapi_free_SocketAddress(s->addr);
> > > - s->addr = socket_local_address(s->listener->sioc[0]->fd,
> > > errp);
> > > - update_disconnected_filename(s);
> > > + qapi_free_SocketAddress(s->addr);
> > > + s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
> > > + update_disconnected_filename(s);
> > >
> > > - if (is_waitconnect &&
> > > - qemu_chr_wait_connected(chr, errp) < 0) {
> > > - return;
> > > - }
> > > - if (!s->ioc) {
> > > - qio_net_listener_set_client_func_full(s->listener,
> > > - tcp_chr_accept,
> > > - chr, NULL,
> > > - chr->gcontext);
> > > - }
> > > + if (is_waitconnect &&
> > > + qemu_chr_wait_connected(chr, errp) < 0) {
> > > + return;
> > > + }
> > > + if (!s->ioc) {
> > > + qio_net_listener_set_client_func_full(s->listener,
> > > + tcp_chr_accept,
> > > + chr, NULL,
> > > + chr->gcontext);
> > > + }
> > > + } else if (is_waitconnect) {
> > > + if (s->reconnect_time) {
> > > + tcp_chr_connect_async(chr);
> > > } else if (qemu_chr_wait_connected(chr, errp) < 0) {
> > > goto error;
> > > }
> >
> > This skips everything when 'is_waitconnect' is false.
> >
> > This combines with a bug in tests/libqtest.c which adds the 'nowait'
> > flag to the -chardevs it cteates. This mistake was previously ignored
> > because the chardevs were socket clients, but now we honour it.
> >
> > We shoul remove 'nowait' from the qtest chardevs, but separately
> > from that this code should also still attempt a non-blocking
> > connect when is_waitconnect is false.
> >
>
> Do you mean we still need to connect server in background with
> "nowait" option? But my purpose is not to connect server until we
> manually call qemu_chr_fe_wait_connected() in other place.
I don't see a need to delay the connect. We can start a
background connect right away. The later code you have
merely needs to wait for that background connect to
finish, which qemu_chr_fe_wait_connected still accomplishes.
This keeps the chardev code clearer only having 2 distinct
code paths to worry about - blocking or non-blocking connect.
>
> > ie
> >
> > } else if (is_waitconnect) {
> > if (s->reconnect_time || !is_waitconnect) {
>
> "!is_waitconnect" should always be false here?
Opps, I meant
} else {
if (s->reconnect_time || !is_waitconnect) {
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
