From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36946) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWjPA-0003vO-W9 for qemu-devel@nongnu.org; Mon, 08 Aug 2016 08:11:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bWjP5-0008Dv-Sd for qemu-devel@nongnu.org; Mon, 08 Aug 2016 08:11:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59650) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWjP5-0008Dd-KE for qemu-devel@nongnu.org; Mon, 08 Aug 2016 08:11:35 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 312DF3F724 for ; Mon, 8 Aug 2016 12:11:34 +0000 (UTC) From: Markus Armbruster References: <20160801112343.29082-1-marcandre.lureau@redhat.com> <20160801112343.29082-2-marcandre.lureau@redhat.com> Date: Mon, 08 Aug 2016 14:11:31 +0200 In-Reply-To: <20160801112343.29082-2-marcandre.lureau@redhat.com> (marcandre lureau's message of "Mon, 1 Aug 2016 15:23:42 +0400") Message-ID: <8760rbcvf0.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-2.7 1/2] monitor: fix crash when leaving qemu with spice audio List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com Cc: qemu-devel@nongnu.org, pbonzini@redhat.com marcandre.lureau@redhat.com writes: > From: Marc-Andr=C3=A9 Lureau > > Since aa5cb7f5e, the chardevs are being cleaned up when leaving > qemu. However, the monitor has still references to them, which may > lead to crashes when running atexit() and trying to send monitor > events: > > #0 0x00007fffdb18f6f5 in __GI_raise (sig=3Dsig@entry=3D6) at ../sysdeps= /unix/sysv/linux/raise.c:54 > #1 0x00007fffdb1912fa in __GI_abort () at abort.c:89 > #2 0x0000555555c263e7 in error_exit (err=3D22, msg=3D0x555555d47980 <__= func__.13537> "qemu_mutex_lock") at util/qemu-thread-posix.c:39 > #3 0x0000555555c26488 in qemu_mutex_lock (mutex=3D0x5555567a2420) at ut= il/qemu-thread-posix.c:66 > #4 0x00005555558c52db in qemu_chr_fe_write (s=3D0x5555567a2420, buf=3D0= x55555740dc40 "{\"timestamp\": {\"seconds\": 1470041716, \"microseconds\": = 989699}, \"event\": \"SPICE_DISCONNECTED\", \"data\": {\"server\": {\"port\= ": \"5900\", \"family\": \"ipv4\", \"host\": \"127.0.0.1\"}, \"client\": {\= "port\": \"40272\", \"f"..., len=3D240) at qemu-char.c:280 > #5 0x0000555555787cad in monitor_flush_locked (mon=3D0x5555567bd9e0) at= /home/elmarco/src/qemu/monitor.c:311 > #6 0x0000555555787e46 in monitor_puts (mon=3D0x5555567bd9e0, str=3D0x55= 55567a44ef "") at /home/elmarco/src/qemu/monitor.c:353 > #7 0x00005555557880fe in monitor_json_emitter (mon=3D0x5555567bd9e0, da= ta=3D0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:401 > #8 0x00005555557882d2 in monitor_qapi_event_emit (event=3DQAPI_EVENT_SP= ICE_DISCONNECTED, qdict=3D0x5555567c73a0) at /home/elmarco/src/qemu/monitor= .c:472 > #9 0x000055555578838f in monitor_qapi_event_queue (event=3DQAPI_EVENT_S= PICE_DISCONNECTED, qdict=3D0x5555567c73a0, errp=3D0x7fffffffca88) at /home/= elmarco/src/qemu/monitor.c:497 > #10 0x0000555555c15541 in qapi_event_send_spice_disconnected (server=3D0= x5555571139d0, client=3D0x5555570d0db0, errp=3D0x5555566c0428 = ) at qapi-event.c:1038 > #11 0x0000555555b11bc6 in channel_event (event=3D3, info=3D0x5555570d6c0= 0) at ui/spice-core.c:248 > #12 0x00007fffdcc9983a in adapter_channel_event (event=3D3, info=3D0x555= 5570d6c00) at reds.c:120 > #13 0x00007fffdcc99a25 in reds_handle_channel_event (reds=3D0x5555567a9d= 60, event=3D3, info=3D0x5555570d6c00) at reds.c:324 > #14 0x00007fffdcc7d4c4 in main_dispatcher_self_handle_channel_event (sel= f=3D0x5555567b28b0, event=3D3, info=3D0x5555570d6c00) at main-dispatcher.c:= 175 > #15 0x00007fffdcc7d5b1 in main_dispatcher_channel_event (self=3D0x555556= 7b28b0, event=3D3, info=3D0x5555570d6c00) at main-dispatcher.c:194 > #16 0x00007fffdcca7674 in reds_stream_push_channel_event (s=3D0x5555570d= 9910, event=3D3) at reds-stream.c:354 > #17 0x00007fffdcca749b in reds_stream_free (s=3D0x5555570d9910) at reds-= stream.c:323 > #18 0x00007fffdccb5dad in snd_disconnect_channel (channel=3D0x5555576a89= a0) at sound.c:229 > #19 0x00007fffdccb9e57 in snd_detach_common (worker=3D0x555557739720) at= sound.c:1589 > #20 0x00007fffdccb9f0e in snd_detach_playback (sin=3D0x5555569fe3f8) at = sound.c:1602 > #21 0x00007fffdcca3373 in spice_server_remove_interface (sin=3D0x5555569= fe3f8) at reds.c:3387 > #22 0x00005555558ff6e2 in line_out_fini (hw=3D0x5555569fe370) at audio/s= piceaudio.c:152 > #23 0x00005555558f909e in audio_atexit () at audio/audio.c:1754 > #24 0x00007fffdb1941e8 in __run_exit_handlers (status=3D0, listp=3D0x7ff= fdb5175d8 <__exit_funcs>, run_list_atexit=3Drun_list_atexit@entry=3Dtrue) a= t exit.c:82 > #25 0x00007fffdb194235 in __GI_exit (status=3D) at exit.c= :104 > #26 0x00007fffdb17b738 in __libc_start_main (main=3D0x5555558d7874 , argc=3D67, argv=3D0x7fffffffcf48, init=3D, fini=3D, rtld_fini=3D, stack_end=3D0x7fffffffcf38) at ../cs= u/libc-start.c:323 > > Add a monitor_cleanup() functions to remove all the monitors before > cleaning up the chardev. Note that we are "losing" some events that > used to be sent during atexit(). > > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > monitor.c | 20 ++++++++++++++++++++ > include/monitor/monitor.h | 1 + > vl.c | 1 + > 3 files changed, 22 insertions(+) > > diff --git a/monitor.c b/monitor.c > index 5d68a5d..5c00373 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -635,6 +635,13 @@ static void monitor_data_init(Monitor *mon) >=20=20 > static void monitor_data_destroy(Monitor *mon) > { > + if (mon->chr) { > + qemu_chr_add_handlers(mon->chr, NULL, NULL, NULL, NULL); > + } > + if (monitor_is_qmp(mon)) { > + json_message_parser_destroy(&mon->qmp.parser); > + } > + g_free(mon->rs); > QDECREF(mon->outbuf); > qemu_mutex_destroy(&mon->out_lock); > } > @@ -4196,6 +4203,19 @@ void monitor_init(CharDriverState *chr, int flags) > qemu_mutex_unlock(&monitor_lock); > } >=20=20 > +void monitor_cleanup(void) > +{ > + Monitor *mon, *next; > + > + qemu_mutex_lock(&monitor_lock); > + QLIST_FOREACH_SAFE(mon, &mon_list, entry, next) { > + QLIST_REMOVE(mon, entry); > + monitor_data_destroy(mon); > + g_free(mon); > + } > + qemu_mutex_unlock(&monitor_lock); > +} > + > static void bdrv_password_cb(void *opaque, const char *password, > void *readline_opaque) > { Before the patch, monitor_data_destroy() destroys what monitor_data_init() creates. After the patch, the symmetry is gone: the additional destructions are for monitor_init(). Let's take this patch as is to get the bug fix into the next RC. Cosmetic surgery can be done on top, without time pressure. > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h > index c5c9ea2..a714d8e 100644 > --- a/include/monitor/monitor.h > +++ b/include/monitor/monitor.h > @@ -17,6 +17,7 @@ extern Monitor *cur_mon; > bool monitor_cur_is_qmp(void); >=20=20 > void monitor_init(CharDriverState *chr, int flags); > +void monitor_cleanup(void); >=20=20 > int monitor_suspend(Monitor *mon); > void monitor_resume(Monitor *mon); > diff --git a/vl.c b/vl.c > index e7c2c62..a14c438 100644 > --- a/vl.c > +++ b/vl.c > @@ -4612,6 +4612,7 @@ int main(int argc, char **argv, char **envp) >=20=20 > /* vhost-user must be cleaned up before chardevs. */ > net_cleanup(); > + monitor_cleanup(); > qemu_chr_cleanup(); >=20=20 > return 0; Reviewed-by: Markus Armbruster