Hi, I got it, thanks for your quick reply :)
Regards, -Gonglei > -----Original Message----- > From: Daniel P. Berrange [mailto:berra...@redhat.com] > Sent: Monday, July 18, 2016 5:07 PM > To: Gonglei (Arei) > Cc: Paolo Bonzini; Lilijun (Jerry); qemu-devel@nongnu.org > Subject: Re: A question about 9894dc0cdcc397ee5b26370bc53da6d360a363c2 > > On Mon, Jul 18, 2016 at 08:41:32AM +0000, Gonglei (Arei) wrote: > > Hi Daniel & Paolo, > > > > Commit 9894dc0c "char: convert from GIOChannel to QIOChannel", > > about the below code segment: > > > > static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp) > > { > > TCPCharDriver *s = chr->opaque; > > - int fd; > > + QIOChannelSocket *sioc = qio_channel_socket_new(); > > > > if (s->is_listen) { > > - fd = socket_listen(s->addr, errp); > > + if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) { > > + goto fail; > > + } > > + qemu_chr_finish_socket_connection(chr, sioc); > > } else if (s->reconnect_time) { > > - fd = socket_connect(s->addr, errp, qemu_chr_socket_connected, > chr); > > - return fd >= 0; > > + qio_channel_socket_connect_async(sioc, s->addr, > > + > qemu_chr_socket_connected, > > + chr, NULL); > > } else { > > - fd = socket_connect(s->addr, errp, NULL, NULL); > > - } > > - if (fd < 0) { > > - return false; > > + if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) { > > + goto fail; > > + } > > + qemu_chr_finish_socket_connection(chr, sioc); > > } > > > > - qemu_chr_finish_socket_connection(chr, fd); > > return true; > > + > > + fail: > > + object_unref(OBJECT(sioc)); > > + return false; > > } > > > > Why did you change socket_connect() to qio_channel_socket_connect_async > > but not qio_channel_socket_connect_sync when s->reconnect_time is ture? > Thanks. > > I can't get the reason from the commit message. > > When the socket_connect() function is called with a non-null value for > the NonBlockingConnectHandler parameter, it'll put the socket into > non-blocking mode before doing the connection. Therefore socket_connect() > method is no longer guaranteed to actually connect to the socket - it > may simply start the connection and leave an event handler to finish > it. This avoids blocking the caller waiting for the connectionb to > finish which may take non-negligible time with TCP sockets. > > The qio_channel_socket_connect_sync() method will always block until > completion of the connect, so we use qio_channel_socket_connect_async > which always does the connection attempt in the background. > > The old code which may or may not finish the connect() in the background > was a recipe for bugs because it forced callers to have 2 completely > separate codepaths for the same API call and chances are only one of > those code paths would be tested by the developers. With the code code > we're guaranteed to always complete in the background, so the callers > only have 1 codepath to worry about and so they'll be forced to ensure > that codepath works. > > > PS: We encountered a problem that when config vhost-user-net reconneticon > function, > > the virtio_net_get_features() didn't get a correct value after this changed, > and it was > > fixed when we change the async to sync. > > It seems that you were mostly lucky with the old code in that presumably > socket_connect() would complete the connection fully. There was never any > guarantee of this though - it was perfectly feasible that socket_connect() > would finish the connect in the background. So IMHO the vhost user code > was already buggy if it doesn't cope with the connect finishing in the > background. > > > Regards, > Daniel > -- > |: http://berrange.com -o- > http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- > http://virt-manager.org :| > |: http://autobuild.org -o- > http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- > http://live.gnome.org/gtk-vnc :|
