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
