[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] char-socket: hold chr_write_lock during tcp
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-devel] [RFC PATCH] char-socket: hold chr_write_lock during tcp_chr_free_connection() |
Date: |
Wed, 06 Feb 2019 14:23:34 +0100 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Wed 06 Feb 2019 02:00:03 PM CET, Paolo Bonzini wrote:
> On 06/02/19 13:43, Alberto Garcia wrote:
>> }
>> @@ -449,7 +451,9 @@ static void tcp_chr_disconnect(Chardev *chr)
>> SocketChardev *s = SOCKET_CHARDEV(chr);
>> bool emit_close = s->connected;
>>
>> + qemu_mutex_lock(&chr->chr_write_lock);
>> tcp_chr_free_connection(chr);
>> + qemu_mutex_unlock(&chr->chr_write_lock);
>>
>> if (s->listener) {
>> qio_net_listener_set_client_func_full(s->listener, tcp_chr_accept,
>
> Should operations on the listener also be protected? I think that apart
> from emitting the closed event itself everything in this function should
> be protected by the lock. The closed event should be pushed to the
> GMainContext using an idle source (perhaps it's worth writing a wrapper
> qemu_idle_add that takes a GMainContext, as the same idiom is already
> present in pty_chr_state and qio_task_thread_worker).
Now that you mention this it seems that we are leaking the GSources in
those two cases? GLib's g_idle_add_full() implementation does unref the
source after attaching it to the GMainContext.
https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gmain.c#L5684
Berto