[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcont
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext |
Date: |
Thu, 1 Mar 2018 15:43:31 +0000 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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 <address@hidden>
> ---
> 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
--
|: 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 05/15] qio: refactor net listener source operations, (continued)
[Qemu-devel] [PATCH v2 06/15] qio: store gsources for net listeners, Peter Xu, 2018/03/01
[Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext, Peter Xu, 2018/03/01
- Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext,
Daniel P . Berrangé <=
[Qemu-devel] [PATCH v2 08/15] chardev: allow telnet gsource to switch gcontext, Peter Xu, 2018/03/01
[Qemu-devel] [PATCH v2 10/15] qio: non-default context for async conn, Peter Xu, 2018/03/01