| Message ID | 20180301084438.13594-8-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qio: general non-default GMainContext support | expand |
On Thu, Mar 01, 2018 at 04:44:30PM +0800, Peter Xu wrote: > TCP chardevs can be using QIO network listeners working in the > background when in listening mode. However the network listeners are > always running in main context. This can race with chardevs that are > running in non-main contexts. > > To solve this, we need to re-setup the net listeners in > tcp_chr_update_read_handler() with the newly cached gcontext. > > Since at it, generalize a tcp_chr_net_listener_setup() helper function > and clean up the old code a bit. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > chardev/char-socket.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 43a2cc2c1c..5cd20cc932 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev *s) > s->is_listen, s->is_telnet); > } > > +/* Set enable=true to start net listeners, false to stop them. */ > +static void tcp_chr_net_listener_setup(SocketChardev *s, bool enable) > +{ > + Chardev *chr = CHARDEV(s); > + > + /* Net listeners' context will follow the Chardev's. */ > + qio_net_listener_set_client_func_full(s->listener, > + enable ? tcp_chr_accept : NULL, > + enable ? chr : NULL, > + NULL, > + chr->gcontext); I don't think this helper method is really a benefit. In fact I think it makes understanding the code harder, because when you see tcp_chr_net_listener_setup(s, true), you've no idea what 'true' means without going to finding the impl of tcp_chr_net_listener_setup(). Just leave the direct calls to qio_net_listener_set_client_func_full as they are IMHO. > +} > + > /* NB may be called even if tcp_chr_connect has not been > * reached, due to TLS or telnet initialization failure, > * so can *not* assume s->connected == true > @@ -422,8 +435,7 @@ static void tcp_chr_disconnect(Chardev *chr) > tcp_chr_free_connection(chr); > > if (s->listener) { > - qio_net_listener_set_client_func(s->listener, tcp_chr_accept, > - chr, NULL); > + tcp_chr_net_listener_setup(s, true); > } > update_disconnected_filename(s); > if (emit_close) { > @@ -559,6 +571,15 @@ static void tcp_chr_update_read_handler(Chardev *chr) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > > + if (s->listener) { > + /* > + * It's possible that chardev context is changed in > + * qemu_chr_be_update_read_handlers(). Reset it for QIO net > + * listener if there is. > + */ > + tcp_chr_net_listener_setup(s, true); > + } > + > if (!s->connected) { > return; > } > @@ -742,7 +763,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc) > qio_channel_set_delay(s->ioc, false); > } > if (s->listener) { > - qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL); > + tcp_chr_net_listener_setup(s, false); > } > > if (s->tls_creds) { > @@ -823,7 +844,7 @@ static void char_socket_finalize(Object *obj) > tcp_chr_reconn_timer_cancel(s); > qapi_free_SocketAddress(s->addr); > if (s->listener) { > - qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL); > + tcp_chr_net_listener_setup(s, false); > object_unref(OBJECT(s->listener)); > } > if (s->tls_creds) { > @@ -979,8 +1000,7 @@ static void qmp_chardev_open_socket(Chardev *chr, > return; > } > if (!s->ioc) { > - qio_net_listener_set_client_func(s->listener, tcp_chr_accept, > - chr, NULL); > + tcp_chr_net_listener_setup(s, true); > } > } else if (qemu_chr_wait_connected(chr, errp) < 0) { > goto error; > -- > 2.14.3 > Regards, Daniel
On Thu, Mar 01, 2018 at 03:43:31PM +0000, Daniel P. Berrangé wrote: > On Thu, Mar 01, 2018 at 04:44:30PM +0800, Peter Xu wrote: > > TCP chardevs can be using QIO network listeners working in the > > background when in listening mode. However the network listeners are > > always running in main context. This can race with chardevs that are > > running in non-main contexts. > > > > To solve this, we need to re-setup the net listeners in > > tcp_chr_update_read_handler() with the newly cached gcontext. > > > > Since at it, generalize a tcp_chr_net_listener_setup() helper function > > and clean up the old code a bit. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > chardev/char-socket.c | 32 ++++++++++++++++++++++++++------ > > 1 file changed, 26 insertions(+), 6 deletions(-) > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > > index 43a2cc2c1c..5cd20cc932 100644 > > --- a/chardev/char-socket.c > > +++ b/chardev/char-socket.c > > @@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev *s) > > s->is_listen, s->is_telnet); > > } > > > > +/* Set enable=true to start net listeners, false to stop them. */ > > +static void tcp_chr_net_listener_setup(SocketChardev *s, bool enable) > > +{ > > + Chardev *chr = CHARDEV(s); > > + > > + /* Net listeners' context will follow the Chardev's. */ > > + qio_net_listener_set_client_func_full(s->listener, > > + enable ? tcp_chr_accept : NULL, > > + enable ? chr : NULL, > > + NULL, > > + chr->gcontext); > > I don't think this helper method is really a benefit. In fact I think > it makes understanding the code harder, because when you see > tcp_chr_net_listener_setup(s, true), you've no idea what 'true' means > without going to finding the impl of tcp_chr_net_listener_setup(). > > Just leave the direct calls to qio_net_listener_set_client_func_full > as they are IMHO. Frankly speaking I was a bit confused when I started to read chardev/qio codes with so many hooks, e.g., when I saw: qio_net_listener_set_client_func(s->listener, tcp_chr_accept, chr, NULL); I totally have no idea on what happened. I need to go deeper into the net listener code to know that, hmm, it's setting up something to accept connections! If I can have something like: tcp_chr_net_listener_setup(s, true); It may be easier for me to understand that there's something either registered for the listening ports, and I don't need to care about which function will be called when accept happened. Basically it "hides" some logic inside, that's IMHO where functions/macros help. (Here the naming of function is discussible for sure, along with how to define the parameters) I think it may be a flavor issue. In that case, I'm always fine with either way. I assume the previous cleanup patch 5 is similarly a flavor issue too, so I'll follow your final judgement on what you would prefer. Thanks,
On 02/03/2018 05:26, Peter Xu wrote: > Frankly speaking I was a bit confused when I started to read > chardev/qio codes with so many hooks, e.g., when I saw: > > qio_net_listener_set_client_func(s->listener, tcp_chr_accept, > chr, NULL); > > I totally have no idea on what happened. I need to go deeper into the > net listener code to know that, hmm, it's setting up something to > accept connections! > > If I can have something like: > > tcp_chr_net_listener_setup(s, true); > > It may be easier for me to understand that there's something either > registered for the listening ports, and I don't need to care about > which function will be called when accept happened. Basically it > "hides" some logic inside, that's IMHO where functions/macros help. I tend to agree with Daniel, but I probably would be convinced if you had a pair of functions tcp_chr_{start,stop}_listen instead of a bool argument. Paolo
On Fri, Mar 02, 2018 at 12:17:30PM +0100, Paolo Bonzini wrote: > On 02/03/2018 05:26, Peter Xu wrote: > > Frankly speaking I was a bit confused when I started to read > > chardev/qio codes with so many hooks, e.g., when I saw: > > > > qio_net_listener_set_client_func(s->listener, tcp_chr_accept, > > chr, NULL); > > > > I totally have no idea on what happened. I need to go deeper into the > > net listener code to know that, hmm, it's setting up something to > > accept connections! > > > > If I can have something like: > > > > tcp_chr_net_listener_setup(s, true); > > > > It may be easier for me to understand that there's something either > > registered for the listening ports, and I don't need to care about > > which function will be called when accept happened. Basically it > > "hides" some logic inside, that's IMHO where functions/macros help. > > I tend to agree with Daniel, but I probably would be convinced if you > had a pair of functions tcp_chr_{start,stop}_listen instead of a bool > argument. Thanks Paolo for your suggestion. Let me just keep the code untouched so that we can be more focused on the topic of the series. I'll try to avoid unecessary clean ups there. Thanks,
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 43a2cc2c1c..5cd20cc932 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev *s) s->is_listen, s->is_telnet); } +/* Set enable=true to start net listeners, false to stop them. */ +static void tcp_chr_net_listener_setup(SocketChardev *s, bool enable) +{ + Chardev *chr = CHARDEV(s); + + /* Net listeners' context will follow the Chardev's. */ + qio_net_listener_set_client_func_full(s->listener, + enable ? tcp_chr_accept : NULL, + enable ? chr : NULL, + NULL, + chr->gcontext); +} + /* NB may be called even if tcp_chr_connect has not been * reached, due to TLS or telnet initialization failure, * so can *not* assume s->connected == true @@ -422,8 +435,7 @@ static void tcp_chr_disconnect(Chardev *chr) tcp_chr_free_connection(chr); if (s->listener) { - qio_net_listener_set_client_func(s->listener, tcp_chr_accept, - chr, NULL); + tcp_chr_net_listener_setup(s, true); } update_disconnected_filename(s); if (emit_close) { @@ -559,6 +571,15 @@ static void tcp_chr_update_read_handler(Chardev *chr) { SocketChardev *s = SOCKET_CHARDEV(chr); + if (s->listener) { + /* + * It's possible that chardev context is changed in + * qemu_chr_be_update_read_handlers(). Reset it for QIO net + * listener if there is. + */ + tcp_chr_net_listener_setup(s, true); + } + if (!s->connected) { return; } @@ -742,7 +763,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc) qio_channel_set_delay(s->ioc, false); } if (s->listener) { - qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL); + tcp_chr_net_listener_setup(s, false); } if (s->tls_creds) { @@ -823,7 +844,7 @@ static void char_socket_finalize(Object *obj) tcp_chr_reconn_timer_cancel(s); qapi_free_SocketAddress(s->addr); if (s->listener) { - qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL); + tcp_chr_net_listener_setup(s, false); object_unref(OBJECT(s->listener)); } if (s->tls_creds) { @@ -979,8 +1000,7 @@ static void qmp_chardev_open_socket(Chardev *chr, return; } if (!s->ioc) { - qio_net_listener_set_client_func(s->listener, tcp_chr_accept, - chr, NULL); + tcp_chr_net_listener_setup(s, true); } } else if (qemu_chr_wait_connected(chr, errp) < 0) { goto error;
TCP chardevs can be using QIO network listeners working in the background when in listening mode. However the network listeners are always running in main context. This can race with chardevs that are running in non-main contexts. To solve this, we need to re-setup the net listeners in tcp_chr_update_read_handler() with the newly cached gcontext. Since at it, generalize a tcp_chr_net_listener_setup() helper function and clean up the old code a bit. Signed-off-by: Peter Xu <peterx@redhat.com> --- chardev/char-socket.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)