| Message ID | 20180301084438.13594-9-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qio: general non-default GMainContext support | expand |
On Thu, Mar 01, 2018 at 04:44:31PM +0800, Peter Xu wrote: > It was originally created by qio_channel_add_watch() so it's always > assigning the task to main context. Now we use the new API called > qio_channel_add_watch_source() so that we get the GSource handle rather > than the tag ID. > > Meanwhile, caching the gsource in SocketChardev.telnet_source so that we > can also do dynamic context switch when update read handlers. I don't see why we would ever want to dynamically switch the GMainContext in use while in middle of reading the telnet greeting. Regards, Daniel
On 01/03/2018 16:46, Daniel P. Berrangé wrote: > On Thu, Mar 01, 2018 at 04:44:31PM +0800, Peter Xu wrote: >> It was originally created by qio_channel_add_watch() so it's always >> assigning the task to main context. Now we use the new API called >> qio_channel_add_watch_source() so that we get the GSource handle rather >> than the tag ID. >> >> Meanwhile, caching the gsource in SocketChardev.telnet_source so that we >> can also do dynamic context switch when update read handlers. > I don't see why we would ever want to dynamically switch the > GMainContext in use while in middle of reading the telnet greeting. Maybe because the remote client hangs in the middle of the telnet greeting? The user of the Chardev can't know that the initial handshake hasn't been done yet. Paolo
On Thu, Mar 01, 2018 at 06:16:53PM +0100, Paolo Bonzini wrote: > On 01/03/2018 16:46, Daniel P. Berrangé wrote: > > On Thu, Mar 01, 2018 at 04:44:31PM +0800, Peter Xu wrote: > >> It was originally created by qio_channel_add_watch() so it's always > >> assigning the task to main context. Now we use the new API called > >> qio_channel_add_watch_source() so that we get the GSource handle rather > >> than the tag ID. > >> > >> Meanwhile, caching the gsource in SocketChardev.telnet_source so that we > >> can also do dynamic context switch when update read handlers. > > I don't see why we would ever want to dynamically switch the > > GMainContext in use while in middle of reading the telnet greeting. > > Maybe because the remote client hangs in the middle of the telnet > greeting? The user of the Chardev can't know that the initial handshake > hasn't been done yet. Ah, this reminded me that I should better cache the TCPChardevTelnetInit struct, otherwise when context changes we'll possibly restart a telnet handshake. Actually if only considering the monitor-OOB series (which is the only one now who may change the context) it's not really necessary, since the context will only be changed once when init monitors. But it's easy to even achieve a higher goal to support real dynamic switch for telnet connections. So I think I'll try that in my next post. Thanks,
On Thu, Mar 01, 2018 at 06:16:53PM +0100, Paolo Bonzini wrote: > On 01/03/2018 16:46, Daniel P. Berrangé wrote: > > On Thu, Mar 01, 2018 at 04:44:31PM +0800, Peter Xu wrote: > >> It was originally created by qio_channel_add_watch() so it's always > >> assigning the task to main context. Now we use the new API called > >> qio_channel_add_watch_source() so that we get the GSource handle rather > >> than the tag ID. > >> > >> Meanwhile, caching the gsource in SocketChardev.telnet_source so that we > >> can also do dynamic context switch when update read handlers. > > I don't see why we would ever want to dynamically switch the > > GMainContext in use while in middle of reading the telnet greeting. > > Maybe because the remote client hangs in the middle of the telnet > greeting? The user of the Chardev can't know that the initial handshake > hasn't been done yet. NB, the chardev will emit the CHR_EVENT_OPENED event once the connection is successfully established, which includes completion of the telnet and/or TLS handshake. I'm just not seeing what code in QEMU would actually trigger a decision to change the GMainContext, in between the accept() of the socket, and the CHR_EVENT_OPENED, since we don't tell anyone that the accept() has actually taken place - they first they'll know of a new client being connected is when the CHR_EVENT_OPENED is emitted. Regards, Daniel
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 5cd20cc932..6c3f1de013 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -59,6 +59,7 @@ typedef struct { bool is_listen; bool is_telnet; bool is_tn3270; + GSource *telnet_source; GSource *reconnect_timer; int64_t reconnect_time; @@ -69,6 +70,7 @@ typedef struct { OBJECT_CHECK(SocketChardev, (obj), TYPE_CHARDEV_SOCKET) static gboolean socket_reconnect_timeout(gpointer opaque); +static void tcp_chr_telnet_init(Chardev *chr); static void tcp_chr_reconn_timer_cancel(SocketChardev *s) { @@ -567,6 +569,15 @@ static void tcp_chr_connect(void *opaque) qemu_chr_be_event(chr, CHR_EVENT_OPENED); } +static void tcp_chr_telnet_destroy(SocketChardev *s) +{ + if (s->telnet_source) { + g_source_destroy(s->telnet_source); + g_source_unref(s->telnet_source); + s->telnet_source = NULL; + } +} + static void tcp_chr_update_read_handler(Chardev *chr) { SocketChardev *s = SOCKET_CHARDEV(chr); @@ -580,6 +591,11 @@ static void tcp_chr_update_read_handler(Chardev *chr) tcp_chr_net_listener_setup(s, true); } + if (s->telnet_source) { + tcp_chr_telnet_destroy(s); + tcp_chr_telnet_init(CHARDEV(s)); + } + if (!s->connected) { return; } @@ -604,6 +620,7 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc, gpointer user_data) { TCPChardevTelnetInit *init = user_data; + SocketChardev *s = SOCKET_CHARDEV(init->chr); ssize_t ret; ret = qio_channel_write(ioc, init->buf, init->buflen, NULL); @@ -628,6 +645,8 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc, end: g_free(init); + g_source_unref(s->telnet_source); + s->telnet_source = NULL; return G_SOURCE_REMOVE; } @@ -667,10 +686,10 @@ static void tcp_chr_telnet_init(Chardev *chr) #undef IACSET - qio_channel_add_watch( - s->ioc, G_IO_OUT, - tcp_chr_telnet_init_io, - init, NULL); + s->telnet_source = qio_channel_add_watch_source(s->ioc, G_IO_OUT, + tcp_chr_telnet_init_io, + init, NULL, + chr->gcontext); } @@ -843,6 +862,7 @@ static void char_socket_finalize(Object *obj) tcp_chr_free_connection(chr); tcp_chr_reconn_timer_cancel(s); qapi_free_SocketAddress(s->addr); + tcp_chr_telnet_destroy(s); if (s->listener) { tcp_chr_net_listener_setup(s, false); object_unref(OBJECT(s->listener));
It was originally created by qio_channel_add_watch() so it's always assigning the task to main context. Now we use the new API called qio_channel_add_watch_source() so that we get the GSource handle rather than the tag ID. Meanwhile, caching the gsource in SocketChardev.telnet_source so that we can also do dynamic context switch when update read handlers. Signed-off-by: Peter Xu <peterx@redhat.com> --- chardev/char-socket.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)