[Qemu-devel] A question about 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Qemu-devel] A question about 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Gonglei (Arei)]

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.

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.

[2016-07-17 21:18:55] virtio_net_get_features:551 virtio_net_get_features 5 n->host_features=12582855, features=2042626023, nc->peer=0x7f11c4b80820
[2016-07-17 21:18:55] get_vhost_net:400 get_vhost_net nc=0x7f11c4b80820
[2016-07-17 21:18:55] get_vhost_net:405 get_vhost_net nc type=11, NET_CLIENT_OPTIONS_KIND_TAP=3, NET_CLIENT_OPTIONS_KIND_VHOST_USER=11
[2016-07-17 21:18:55] get_vhost_net:417 get_vhost_net vhost_net=(nil)


Regards,
-Gonglei

Re: [Qemu-devel] A question about 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Daniel P. Berrange]

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 :|

Re: [Qemu-devel] A question about 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Gonglei (Arei)]

Hi,

I got it, thanks for your quick reply :)


Regards,
-Gonglei


> -----Original Message-----
> From: Daniel P. Berrange [mailto:berrange@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 :|