[Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Marc-André Lureau]

Hi,

In commit 25679e5d58e "chardev: tcp: postpone async connection setup"
(and its follow up 99f2f54174a59), Peter moved chardev socket
connection to machine_done event. However, chardev created later will
no longer attempt to connect, and chardev created in tests do not have
machine_done event (breaking some of vhost-user-test).

The goal was to move the "connect" source to the chardev frontend
context (the monitor thread context in his case). chr->gcontext is set
with qemu_chr_fe_set_handlers(). But there is no guarantee that the
function will be called in general, so we can't delay connection until
then: the chardev should still attempt to connect during open(), using
the main context.

An alternative would be to specify the iothread during chardev
creation. Setting up monitor OOB would be quite different too, it
would take the same iothread as argument.

99f2f54174a595e is also a bit problematic, since it will behave
differently before and after machine_done (the first case gives a
chance to use a different context reliably, the second looks racy)

In the end, I am not sure this is all necessary, as chardev callbacks
are called after qemu_chr_fe_set_handlers(), at which point the
context of sources are updated. In "char-socket: update all ioc
handlers when changing context", I moved also the hup handler to the
updated context. So unless the main thread is already stuck, we can
setup a different context for the chardev at that time. Or not?

Marc-André Lureau (4):
  Revert "chardev: tcp: postpone TLS work until machine done"
  Revert "chardev: tcp: postpone async connection setup"
  char-socket: update all ioc handlers when changing context
  test-char: add socket reconnect test

 chardev/char-socket.c | 86 ++++++++++++++++++-------------------------
 tests/test-char.c     | 18 +++++++--
 2 files changed, 50 insertions(+), 54 deletions(-)

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 1/4] Revert "chardev: tcp: postpone TLS work until machine done"

[Marc-André Lureau]

This reverts commit 99f2f54174a595e3ada6e4332fcd2b37ebb0d55d.

See next commit reverting 25679e5d58e258e9950685ffbd0cae4cd40d9cc2 as
well for rationale.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index efbad6ee7c..e28d2cca4c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -32,7 +32,6 @@
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
-#include "sysemu/sysemu.h"
 
 #include "chardev/char-io.h"
 
@@ -724,11 +723,6 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
-    if (!machine_init_done) {
-        /* This will be postponed to machine_done notifier */
-        return;
-    }
-
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
@@ -1169,10 +1163,6 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
         tcp_chr_connect_async(chr);
     }
 
-    if (s->ioc && s->tls_creds) {
-        tcp_chr_tls_init(chr);
-    }
-
     return 0;
 }
 
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 2/4] Revert "chardev: tcp: postpone async connection setup"

[Marc-André Lureau]

This reverts commit 25679e5d58e258e9950685ffbd0cae4cd40d9cc2.

This commit broke "reconnect socket" chardev that are created after
"machine_done": they no longer try to connect. It broke also
vhost-user-test that uses chardev while there is no "machine_done"
event.

The goal of this patch was to move the "connect" source to the
frontend context. chr->gcontext is set with
qemu_chr_fe_set_handlers(). But there is no guarantee that it will be
called, so we can't delay connection until then: the chardev should
still attempt to connect during open(). qemu_chr_fe_set_handlers() is
eventually called later and will update the context.

Unless there is a good reason to not use initially the default
context, I think we should revert to the previous state to fix the
regressions.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index e28d2cca4c..14f6567f6a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1005,8 +1005,9 @@ static void qmp_chardev_open_socket(Chardev *chr,
         s->reconnect_time = reconnect;
     }
 
-    /* If reconnect_time is set, will do that in chr_machine_done. */
-    if (!s->reconnect_time) {
+    if (s->reconnect_time) {
+        tcp_chr_connect_async(chr);
+    } else {
         if (s->is_listen) {
             char *name;
             s->listener = qio_net_listener_new();
@@ -1155,17 +1156,6 @@ 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);
@@ -1181,7 +1171,6 @@ 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 3/4] char-socket: update all ioc handlers when changing context

[Marc-André Lureau]

So far, tcp_chr_update_read_handler() only updated the read
handler. Let's also update the hup handler.

Factorize the code while at it. (note that s->ioc != NULL when
s->connected)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 59 ++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 14f6567f6a..7cd0ae2824 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -353,6 +353,15 @@ static GSource *tcp_chr_add_watch(Chardev *chr, GIOCondition cond)
     return qio_channel_create_watch(s->ioc, cond);
 }
 
+static void remove_hup_source(SocketChardev *s)
+{
+    if (s->hup_source != NULL) {
+        g_source_destroy(s->hup_source);
+        g_source_unref(s->hup_source);
+        s->hup_source = NULL;
+    }
+}
+
 static void tcp_chr_free_connection(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -367,11 +376,7 @@ static void tcp_chr_free_connection(Chardev *chr)
         s->read_msgfds_num = 0;
     }
 
-    if (s->hup_source != NULL) {
-        g_source_destroy(s->hup_source);
-        g_source_unref(s->hup_source);
-        s->hup_source = NULL;
-    }
+    remove_hup_source(s);
 
     tcp_set_msgfds(chr, NULL, 0);
     remove_fd_in_watch(chr);
@@ -540,6 +545,27 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len,
     }
 }
 
+static void update_ioc_handlers(SocketChardev *s)
+{
+    Chardev *chr = CHARDEV(s);
+
+    if (!s->connected) {
+        return;
+    }
+
+    remove_fd_in_watch(chr);
+    chr->gsource = io_add_watch_poll(chr, s->ioc,
+                                     tcp_chr_read_poll,
+                                     tcp_chr_read, chr,
+                                     chr->gcontext);
+
+    remove_hup_source(s);
+    s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
+    g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
+                          chr, NULL);
+    g_source_attach(s->hup_source, chr->gcontext);
+}
+
 static void tcp_chr_connect(void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
@@ -552,16 +578,7 @@ static void tcp_chr_connect(void *opaque)
         s->is_listen, s->is_telnet);
 
     s->connected = 1;
-    chr->gsource = io_add_watch_poll(chr, s->ioc,
-                                       tcp_chr_read_poll,
-                                       tcp_chr_read,
-                                       chr, chr->gcontext);
-
-    s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
-    g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
-                          chr, NULL);
-    g_source_attach(s->hup_source, chr->gcontext);
-
+    update_ioc_handlers(s);
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
@@ -592,17 +609,7 @@ static void tcp_chr_update_read_handler(Chardev *chr)
         tcp_chr_telnet_init(CHARDEV(s));
     }
 
-    if (!s->connected) {
-        return;
-    }
-
-    remove_fd_in_watch(chr);
-    if (s->ioc) {
-        chr->gsource = io_add_watch_poll(chr, s->ioc,
-                                           tcp_chr_read_poll,
-                                           tcp_chr_read, chr,
-                                           chr->gcontext);
-    }
+    update_ioc_handlers(s);
 }
 
 static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 4/4] test-char: add socket reconnect test

[Marc-André Lureau]

This test exhibits a regression fixed by the previous reverts.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-char.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/test-char.c b/tests/test-char.c
index 5905d31441..d99742d86d 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -309,7 +309,7 @@ static int socket_can_read_hello(void *opaque)
     return 10;
 }
 
-static void char_socket_test_common(Chardev *chr)
+static void char_socket_test_common(Chardev *chr, bool reconnect)
 {
     Chardev *chr_client;
     QObject *addr;
@@ -329,7 +329,8 @@ static void char_socket_test_common(Chardev *chr)
     addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
     qdict = qobject_to(QDict, addr);
     port = qdict_get_str(qdict, "port");
-    tmp = g_strdup_printf("tcp:127.0.0.1:%s", port);
+    tmp = g_strdup_printf("tcp:127.0.0.1:%s%s", port,
+                          reconnect ? ",reconnect=1" : "");
     qobject_unref(qdict);
 
     qemu_chr_fe_init(&be, chr, &error_abort);
@@ -370,7 +371,15 @@ static void char_socket_basic_test(void)
 {
     Chardev *chr = qemu_chr_new("server", "tcp:127.0.0.1:0,server,nowait");
 
-    char_socket_test_common(chr);
+    char_socket_test_common(chr, false);
+}
+
+
+static void char_socket_reconnect_test(void)
+{
+    Chardev *chr = qemu_chr_new("server", "tcp:127.0.0.1:0,server,nowait");
+
+    char_socket_test_common(chr, true);
 }
 
 
@@ -402,7 +411,7 @@ static void char_socket_fdpass_test(void)
 
     qemu_opts_del(opts);
 
-    char_socket_test_common(chr);
+    char_socket_test_common(chr, false);
 }
 
 
@@ -823,6 +832,7 @@ int main(int argc, char **argv)
     g_test_add_func("/char/file-fifo", char_file_fifo_test);
 #endif
     g_test_add_func("/char/socket/basic", char_socket_basic_test);
+    g_test_add_func("/char/socket/reconnect", char_socket_reconnect_test);
     g_test_add_func("/char/socket/fdpass", char_socket_fdpass_test);
     g_test_add_func("/char/udp", char_udp_test);
 #ifdef HAVE_CHARDEV_SERIAL
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Peter Xu]

On Fri, Aug 17, 2018 at 03:52:20PM +0200, Marc-André Lureau wrote:
> Hi,
> 
> In commit 25679e5d58e "chardev: tcp: postpone async connection setup"
> (and its follow up 99f2f54174a59), Peter moved chardev socket
> connection to machine_done event. However, chardev created later will
> no longer attempt to connect, and chardev created in tests do not have
> machine_done event (breaking some of vhost-user-test).
> 
> The goal was to move the "connect" source to the chardev frontend
> context (the monitor thread context in his case). chr->gcontext is set
> with qemu_chr_fe_set_handlers(). But there is no guarantee that the
> function will be called in general,

Could you hint a case where we didn't use qemu_chr_fe_set_handlers()
upon a chardev backend?  I thought it was always used in chardev
frontends, and what the backend could do if without a frontend?

[1]

> so we can't delay connection until
> then: the chardev should still attempt to connect during open(), using
> the main context.
> 
> An alternative would be to specify the iothread during chardev
> creation. Setting up monitor OOB would be quite different too, it
> would take the same iothread as argument.
> 
> 99f2f54174a595e is also a bit problematic, since it will behave
> differently before and after machine_done (the first case gives a
> chance to use a different context reliably, the second looks racy)
> 
> In the end, I am not sure this is all necessary, as chardev callbacks
> are called after qemu_chr_fe_set_handlers(), at which point the
> context of sources are updated. In "char-socket: update all ioc
> handlers when changing context", I moved also the hup handler to the
> updated context. So unless the main thread is already stuck, we can
> setup a different context for the chardev at that time. Or not?

IMHO the two patches that you reverted are special-cases for reasons.

The TLS handshake is carried out with an TLS internal GSource which is
not owned by the chardev code, so the qemu_chr_fe_set_handlers() won't
update that GSource (please refer to qio_channel_tls_handshake_task).

The async connection is carried out in a standalone thread that calls
connect().  IMHO we'd better not update the gcontext bound to the
async task since otherwise there'll be a race (IIRC I proposed
something before using a mutex to update the gcontext, but Dan would
prefer not to, and I followed with the suggestion which makes sense to
me).

Could we just postpone these machine done tasks into
qemu_chr_fe_set_handlers() (or say, chr_update_read_handler() hook,
just like what I mentioned in the other thread)?  Though we'll be sure
qemu_chr_fe_set_handlers() will be called for all chardev backends
hence I asked question [1] above.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Marc-André Lureau]

Hi

On Mon, Aug 20, 2018 at 4:48 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 17, 2018 at 03:52:20PM +0200, Marc-André Lureau wrote:
> > Hi,
> >
> > In commit 25679e5d58e "chardev: tcp: postpone async connection setup"
> > (and its follow up 99f2f54174a59), Peter moved chardev socket
> > connection to machine_done event. However, chardev created later will
> > no longer attempt to connect, and chardev created in tests do not have
> > machine_done event (breaking some of vhost-user-test).
> >
> > The goal was to move the "connect" source to the chardev frontend
> > context (the monitor thread context in his case). chr->gcontext is set
> > with qemu_chr_fe_set_handlers(). But there is no guarantee that the
> > function will be called in general,
>
> Could you hint a case where we didn't use qemu_chr_fe_set_handlers()
> upon a chardev backend?  I thought it was always used in chardev
> frontends, and what the backend could do if without a frontend?

Well, you don't have to have a front-end to have side effects. Connect
will be attempted even without frontend. We may have users expecting
that behaviour, that might be considered a break if we change it.

(and unlikely, there might be frontends that are write only)

>
> [1]
>
> > so we can't delay connection until
> > then: the chardev should still attempt to connect during open(), using
> > the main context.
> >
> > An alternative would be to specify the iothread during chardev
> > creation. Setting up monitor OOB would be quite different too, it
> > would take the same iothread as argument.
> >
> > 99f2f54174a595e is also a bit problematic, since it will behave
> > differently before and after machine_done (the first case gives a
> > chance to use a different context reliably, the second looks racy)
> >
> > In the end, I am not sure this is all necessary, as chardev callbacks
> > are called after qemu_chr_fe_set_handlers(), at which point the
> > context of sources are updated. In "char-socket: update all ioc
> > handlers when changing context", I moved also the hup handler to the
> > updated context. So unless the main thread is already stuck, we can
> > setup a different context for the chardev at that time. Or not?
>
> IMHO the two patches that you reverted are special-cases for reasons.
>
> The TLS handshake is carried out with an TLS internal GSource which is
> not owned by the chardev code, so the qemu_chr_fe_set_handlers() won't
> update that GSource (please refer to qio_channel_tls_handshake_task).

What can go wrong by using the default context for initial connection
and TLS handshake?

Presumably, you have a case where the mainloop is no longer processed
and that will hang the chardev?

> The async connection is carried out in a standalone thread that calls
> connect().  IMHO we'd better not update the gcontext bound to the
> async task since otherwise there'll be a race (IIRC I proposed
> something before using a mutex to update the gcontext, but Dan would
> prefer not to, and I followed with the suggestion which makes sense to
> me).
>
> Could we just postpone these machine done tasks into
> qemu_chr_fe_set_handlers() (or say, chr_update_read_handler() hook,
> just like what I mentioned in the other thread)?  Though we'll be sure
> qemu_chr_fe_set_handlers() will be called for all chardev backends
> hence I asked question [1] above.

I would rather not to, if possible. unless we take the risk of
breaking current behaviour and review chardev usage in qemu.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Peter Xu]

On Mon, Aug 20, 2018 at 05:37:55PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Aug 20, 2018 at 4:48 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 17, 2018 at 03:52:20PM +0200, Marc-André Lureau wrote:
> > > Hi,
> > >
> > > In commit 25679e5d58e "chardev: tcp: postpone async connection setup"
> > > (and its follow up 99f2f54174a59), Peter moved chardev socket
> > > connection to machine_done event. However, chardev created later will
> > > no longer attempt to connect, and chardev created in tests do not have
> > > machine_done event (breaking some of vhost-user-test).
> > >
> > > The goal was to move the "connect" source to the chardev frontend
> > > context (the monitor thread context in his case). chr->gcontext is set
> > > with qemu_chr_fe_set_handlers(). But there is no guarantee that the
> > > function will be called in general,
> >
> > Could you hint a case where we didn't use qemu_chr_fe_set_handlers()
> > upon a chardev backend?  I thought it was always used in chardev
> > frontends, and what the backend could do if without a frontend?
> 
> Well, you don't have to have a front-end to have side effects. Connect
> will be attempted even without frontend. We may have users expecting
> that behaviour, that might be considered a break if we change it.
> 
> (and unlikely, there might be frontends that are write only)

My understanding is that qemu_chr_fe_set_handlers() is not only for
port read, but also for the rest.  For example, we need to pass in the
correct IOEventHandler* to handle chardev backend events even if the
frontend only writes.

> 
> >
> > [1]
> >
> > > so we can't delay connection until
> > > then: the chardev should still attempt to connect during open(), using
> > > the main context.
> > >
> > > An alternative would be to specify the iothread during chardev
> > > creation. Setting up monitor OOB would be quite different too, it
> > > would take the same iothread as argument.
> > >
> > > 99f2f54174a595e is also a bit problematic, since it will behave
> > > differently before and after machine_done (the first case gives a
> > > chance to use a different context reliably, the second looks racy)
> > >
> > > In the end, I am not sure this is all necessary, as chardev callbacks
> > > are called after qemu_chr_fe_set_handlers(), at which point the
> > > context of sources are updated. In "char-socket: update all ioc
> > > handlers when changing context", I moved also the hup handler to the
> > > updated context. So unless the main thread is already stuck, we can
> > > setup a different context for the chardev at that time. Or not?
> >
> > IMHO the two patches that you reverted are special-cases for reasons.
> >
> > The TLS handshake is carried out with an TLS internal GSource which is
> > not owned by the chardev code, so the qemu_chr_fe_set_handlers() won't
> > update that GSource (please refer to qio_channel_tls_handshake_task).
> 
> What can go wrong by using the default context for initial connection
> and TLS handshake?
> 
> Presumably, you have a case where the mainloop is no longer processed
> and that will hang the chardev?

Yeah I don't see a big problem now, but I'm not sure. Actually it
should not be very hard to even migrate this one just like other
GSources, however the async one below should be a bit harder.

> 
> > The async connection is carried out in a standalone thread that calls
> > connect().  IMHO we'd better not update the gcontext bound to the
> > async task since otherwise there'll be a race (IIRC I proposed
> > something before using a mutex to update the gcontext, but Dan would
> > prefer not to, and I followed with the suggestion which makes sense to
> > me).
> >
> > Could we just postpone these machine done tasks into
> > qemu_chr_fe_set_handlers() (or say, chr_update_read_handler() hook,
> > just like what I mentioned in the other thread)?  Though we'll be sure
> > qemu_chr_fe_set_handlers() will be called for all chardev backends
> > hence I asked question [1] above.
> 
> I would rather not to, if possible. unless we take the risk of
> breaking current behaviour and review chardev usage in qemu.

Yeah, I'd be glad to know any of the behavior breakage if there is,
but I can't figure any out.  AFAIU there should be none since we
should always be pairing a backend with a frontend.

I fully agree that current way is not ideal since basically the
backend should not depend on the frontend, but now we have the
gcontext as an exception then the backend will somehow depend on the
frontend.  If you don't like the way I proposed, another thing I am
thinking is that whether we can assign the gcontext for the chardev
backend before initialization of it (or by parsing the backend &
frontend relationships before init of backends), then we assure that
we never change the gcontext of any chardev backends.  Though that
will require that we need to setup all possible gcontexts before hand
(e.g., the monitor gcontext).  Then we can drop all these dynamic
binding magics (but just to hope we will never need the flexibility in
the future).

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Marc-André Lureau]

Hi

On Tue, Aug 21, 2018 at 8:29 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Aug 20, 2018 at 05:37:55PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Aug 20, 2018 at 4:48 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 17, 2018 at 03:52:20PM +0200, Marc-André Lureau wrote:
> > > > Hi,
> > > >
> > > > In commit 25679e5d58e "chardev: tcp: postpone async connection setup"
> > > > (and its follow up 99f2f54174a59), Peter moved chardev socket
> > > > connection to machine_done event. However, chardev created later will
> > > > no longer attempt to connect, and chardev created in tests do not have
> > > > machine_done event (breaking some of vhost-user-test).
> > > >
> > > > The goal was to move the "connect" source to the chardev frontend
> > > > context (the monitor thread context in his case). chr->gcontext is set
> > > > with qemu_chr_fe_set_handlers(). But there is no guarantee that the
> > > > function will be called in general,
> > >
> > > Could you hint a case where we didn't use qemu_chr_fe_set_handlers()
> > > upon a chardev backend?  I thought it was always used in chardev
> > > frontends, and what the backend could do if without a frontend?
> >
> > Well, you don't have to have a front-end to have side effects. Connect
> > will be attempted even without frontend. We may have users expecting
> > that behaviour, that might be considered a break if we change it.
> >
> > (and unlikely, there might be frontends that are write only)
>
> My understanding is that qemu_chr_fe_set_handlers() is not only for
> port read, but also for the rest.  For example, we need to pass in the
> correct IOEventHandler* to handle chardev backend events even if the
> frontend only writes.
>
> >
> > >
> > > [1]
> > >
> > > > so we can't delay connection until
> > > > then: the chardev should still attempt to connect during open(), using
> > > > the main context.
> > > >
> > > > An alternative would be to specify the iothread during chardev
> > > > creation. Setting up monitor OOB would be quite different too, it
> > > > would take the same iothread as argument.
> > > >
> > > > 99f2f54174a595e is also a bit problematic, since it will behave
> > > > differently before and after machine_done (the first case gives a
> > > > chance to use a different context reliably, the second looks racy)
> > > >
> > > > In the end, I am not sure this is all necessary, as chardev callbacks
> > > > are called after qemu_chr_fe_set_handlers(), at which point the
> > > > context of sources are updated. In "char-socket: update all ioc
> > > > handlers when changing context", I moved also the hup handler to the
> > > > updated context. So unless the main thread is already stuck, we can
> > > > setup a different context for the chardev at that time. Or not?
> > >
> > > IMHO the two patches that you reverted are special-cases for reasons.
> > >
> > > The TLS handshake is carried out with an TLS internal GSource which is
> > > not owned by the chardev code, so the qemu_chr_fe_set_handlers() won't
> > > update that GSource (please refer to qio_channel_tls_handshake_task).
> >
> > What can go wrong by using the default context for initial connection
> > and TLS handshake?
> >
> > Presumably, you have a case where the mainloop is no longer processed
> > and that will hang the chardev?
>
> Yeah I don't see a big problem now, but I'm not sure. Actually it
> should not be very hard to even migrate this one just like other
> GSources, however the async one below should be a bit harder.
>
> >
> > > The async connection is carried out in a standalone thread that calls
> > > connect().  IMHO we'd better not update the gcontext bound to the
> > > async task since otherwise there'll be a race (IIRC I proposed
> > > something before using a mutex to update the gcontext, but Dan would
> > > prefer not to, and I followed with the suggestion which makes sense to
> > > me).
> > >
> > > Could we just postpone these machine done tasks into
> > > qemu_chr_fe_set_handlers() (or say, chr_update_read_handler() hook,
> > > just like what I mentioned in the other thread)?  Though we'll be sure
> > > qemu_chr_fe_set_handlers() will be called for all chardev backends
> > > hence I asked question [1] above.
> >
> > I would rather not to, if possible. unless we take the risk of
> > breaking current behaviour and review chardev usage in qemu.
>
> Yeah, I'd be glad to know any of the behavior breakage if there is,
> but I can't figure any out.  AFAIU there should be none since we
> should always be pairing a backend with a frontend.

Even if we check all usage of chardev in internal qemu, there might be
external users that expect that creating a chardev will attempt the
connection immediately.

>
> I fully agree that current way is not ideal since basically the
> backend should not depend on the frontend, but now we have the
> gcontext as an exception then the backend will somehow depend on the
> frontend.  If you don't like the way I proposed, another thing I am
> thinking is that whether we can assign the gcontext for the chardev
> backend before initialization of it (or by parsing the backend &
> frontend relationships before init of backends), then we assure that
> we never change the gcontext of any chardev backends.  Though that

Yes, I think that's a cleaner solution. I suggested to use an iothread
argument in the cover letter.

Paolo, Daniel, any opinion?

> will require that we need to setup all possible gcontexts before hand
> (e.g., the monitor gcontext).  Then we can drop all these dynamic
> binding magics (but just to hope we will never need the flexibility in
> the future).
>
> Regards,
>
> --
> Peter Xu



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Daniel P. Berrangé]

On Tue, Aug 21, 2018 at 04:04:45PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 21, 2018 at 8:29 AM Peter Xu <peterx@redhat.com> wrote:
> > I fully agree that current way is not ideal since basically the
> > backend should not depend on the frontend, but now we have the
> > gcontext as an exception then the backend will somehow depend on the
> > frontend.  If you don't like the way I proposed, another thing I am
> > thinking is that whether we can assign the gcontext for the chardev
> > backend before initialization of it (or by parsing the backend &
> > frontend relationships before init of backends), then we assure that
> > we never change the gcontext of any chardev backends.  Though that
> 
> Yes, I think that's a cleaner solution. I suggested to use an iothread
> argument in the cover letter.
> 
> Paolo, Daniel, any opinion?

Never changing the GContext once initialized is much nicer. I think I
had suggested that before when this code was first proposed.

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 0/4] Fix socket chardev regression

[Paolo Bonzini]

On 21/08/2018 16:04, Marc-André Lureau wrote:
>> If you don't like the way I proposed, another thing I am
>> thinking is that whether we can assign the gcontext for the chardev
>> backend before initialization of it (or by parsing the backend &
>> frontend relationships before init of backends), then we assure that
>> we never change the gcontext of any chardev backends.  Though that
> Yes, I think that's a cleaner solution. I suggested to use an iothread
> argument in the cover letter.

That would be nice, but isn't it already too late for the monitor chardev?

In any case, I don't see a reason to dislike this patch, especially
since it comes with a testcase.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Peter Xu]

On Tue, Aug 21, 2018 at 04:16:27PM +0200, Paolo Bonzini wrote:
> On 21/08/2018 16:04, Marc-André Lureau wrote:
> >> If you don't like the way I proposed, another thing I am
> >> thinking is that whether we can assign the gcontext for the chardev
> >> backend before initialization of it (or by parsing the backend &
> >> frontend relationships before init of backends), then we assure that
> >> we never change the gcontext of any chardev backends.  Though that
> > Yes, I think that's a cleaner solution. I suggested to use an iothread
> > argument in the cover letter.
> 
> That would be nice, but isn't it already too late for the monitor chardev?

I think, yes, if we want to do this automatically. Though if as
Marc-André suggested (which I didn't really notice first when reading
the cover letter), then maybe that's not a problem since user need to
manually specify the iothread for a chardev backend, then chardev
context will not depend on monitor code any more.

Marc-André, do you want to propose your iothread interface?  That
should be the easy way AFAIU, though that'll make the command line for
monitor out-of-band much longer (but it seems fine at least to me).

Adding Markus too.

> 
> In any case, I don't see a reason to dislike this patch, especially
> since it comes with a testcase.

AFAIU the test case didn't really test the non-NULL gcontext case, so
it fixed A (vhost-user reconnect) however it might break B (non-NULL
gcontext with a potential race).

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] Fix socket chardev regression

[Peter Xu]

On Wed, Aug 22, 2018 at 11:46:32AM +0800, Peter Xu wrote:
> On Tue, Aug 21, 2018 at 04:16:27PM +0200, Paolo Bonzini wrote:
> > On 21/08/2018 16:04, Marc-André Lureau wrote:
> > >> If you don't like the way I proposed, another thing I am
> > >> thinking is that whether we can assign the gcontext for the chardev
> > >> backend before initialization of it (or by parsing the backend &
> > >> frontend relationships before init of backends), then we assure that
> > >> we never change the gcontext of any chardev backends.  Though that
> > > Yes, I think that's a cleaner solution. I suggested to use an iothread
> > > argument in the cover letter.
> > 
> > That would be nice, but isn't it already too late for the monitor chardev?
> 
> I think, yes, if we want to do this automatically.

Sorry I forgot to mention some more details here.  If to do it
automatically, IMHO we can just split the mon_init_func() into parsing
and init, then we do:

(1) parse monitors, setup gcontext/... for chardev
(2) init chardevs with the gcontext/... setup
(3) init monitors

Benefit is that we don't need to introduce new interface then.

> Though if as
> Marc-André suggested (which I didn't really notice first when reading
> the cover letter), then maybe that's not a problem since user need to
> manually specify the iothread for a chardev backend, then chardev
> context will not depend on monitor code any more.
> 
> Marc-André, do you want to propose your iothread interface?  That
> should be the easy way AFAIU, though that'll make the command line for
> monitor out-of-band much longer (but it seems fine at least to me).
> 
> Adding Markus too.
> 
> > 
> > In any case, I don't see a reason to dislike this patch, especially
> > since it comes with a testcase.
> 
> AFAIU the test case didn't really test the non-NULL gcontext case, so
> it fixed A (vhost-user reconnect) however it might break B (non-NULL
> gcontext with a potential race).
> 
> Regards,
> 
> -- 
> Peter Xu

Regards,

-- 
Peter Xu