[Qemu-devel] [PATCH v2 14/15] chardev: tcp: postpone async connection setup

Peter Xu posted 15 patches 7 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 14/15] chardev: tcp: postpone async connection setup
Posted by Peter Xu 7 years, 7 months ago
This patch allows the socket chardev async connection be setup with
non-default gcontext.  We do it by postponing the setup to machine done,
since until then we can know which context we should run the async
operation on.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index cd9db123f2..2b355fc7a8 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -997,9 +997,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
         s->reconnect_time = reconnect;
     }
 
-    if (s->reconnect_time) {
-        tcp_chr_connect_async(chr);
-    } else {
+    /* If reconnect_time is set, will do that in chr_machine_done. */
+    if (!s->reconnect_time) {
         if (s->is_listen) {
             char *name;
             s->listener = qio_net_listener_new();
@@ -1128,6 +1127,17 @@ char_socket_get_connected(Object *obj, Error **errp)
     return s->connected;
 }
 
+static int tcp_chr_machine_done_hook(Chardev *chr)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->reconnect_time) {
+        tcp_chr_connect_async(chr);
+    }
+
+    return 0;
+}
+
 static void char_socket_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -1143,6 +1153,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
     cc->chr_add_client = tcp_chr_add_client;
     cc->chr_add_watch = tcp_chr_add_watch;
     cc->chr_update_read_handler = tcp_chr_update_read_handler;
+    cc->chr_machine_done = tcp_chr_machine_done_hook;
 
     object_class_property_add(oc, "addr", "SocketAddress",
                               char_socket_get_addr, NULL,
-- 
2.14.3


Re: [Qemu-devel] [PATCH v2 14/15] chardev: tcp: postpone async connection setup
Posted by Daniel P. Berrangé 7 years, 7 months ago
On Thu, Mar 01, 2018 at 04:44:37PM +0800, Peter Xu wrote:
> This patch allows the socket chardev async connection be setup with
> non-default gcontext.  We do it by postponing the setup to machine done,
> since until then we can know which context we should run the async
> operation on.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

I don't like this as it is special casing behaviour wrt GMainContext
only the the case where the chardev is configured as a client with
non-blocking connect. So any code that uses chardevs and wants to
set a different GMainContext may or may not work, depending on
whether the user gave the ',wait' option to the chardev. I'm struggling
to see why this is really needed at all.

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

Re: [Qemu-devel] [PATCH v2 14/15] chardev: tcp: postpone async connection setup
Posted by Peter Xu 7 years, 7 months ago
On Thu, Mar 01, 2018 at 04:01:38PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:37PM +0800, Peter Xu wrote:
> > This patch allows the socket chardev async connection be setup with
> > non-default gcontext.  We do it by postponing the setup to machine done,
> > since until then we can know which context we should run the async
> > operation on.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> I don't like this as it is special casing behaviour wrt GMainContext
> only the the case where the chardev is configured as a client with
> non-blocking connect. So any code that uses chardevs and wants to
> set a different GMainContext may or may not work, depending on
> whether the user gave the ',wait' option to the chardev. I'm struggling
> to see why this is really needed at all.

Not sure whether I fully got your point, but IMHO when "wait" is there
we should be perfectly fine too if with all the TLS/TELNET patches in
the this series.  And for sure this patch only solves the problem when
"wait" is specified.

Or say, with this series, all configuration of chardev _should_ work
with non-default context now.  If not, then I must have missed
something else (which may be possible), then I would be very glad that
anyone can give me a hint on where.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 14/15] chardev: tcp: postpone async connection setup
Posted by Paolo Bonzini 7 years, 7 months ago
On 01/03/2018 09:44, Peter Xu wrote:
> This patch allows the socket chardev async connection be setup with
> non-default gcontext.  We do it by postponing the setup to machine done,
> since until then we can know which context we should run the async
> operation on.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index cd9db123f2..2b355fc7a8 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -997,9 +997,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          s->reconnect_time = reconnect;
>      }
>  
> -    if (s->reconnect_time) {
> -        tcp_chr_connect_async(chr);
> -    } else {
> +    /* If reconnect_time is set, will do that in chr_machine_done. */
> +    if (!s->reconnect_time) {
>          if (s->is_listen) {
>              char *name;
>              s->listener = qio_net_listener_new();
> @@ -1128,6 +1127,17 @@ char_socket_get_connected(Object *obj, Error **errp)
>      return s->connected;
>  }
>  
> +static int tcp_chr_machine_done_hook(Chardev *chr)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +
> +    if (s->reconnect_time) {
> +        tcp_chr_connect_async(chr);
> +    }
> +
> +    return 0;
> +}
> +
>  static void char_socket_class_init(ObjectClass *oc, void *data)
>  {
>      ChardevClass *cc = CHARDEV_CLASS(oc);
> @@ -1143,6 +1153,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
>      cc->chr_add_client = tcp_chr_add_client;
>      cc->chr_add_watch = tcp_chr_add_watch;
>      cc->chr_update_read_handler = tcp_chr_update_read_handler;
> +    cc->chr_machine_done = tcp_chr_machine_done_hook;
>  
>      object_class_property_add(oc, "addr", "SocketAddress",
>                                char_socket_get_addr, NULL,
> 


Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>