[Qemu-devel] [PATCH for-2.7 0/2] Fix spice audio crash regression

[Qemu-devel] [PATCH for-2.7 0/2] Fix spice audio crash regression

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

The monitor is being used atexit, during spice audio clean up, and
this leads to a crash since the chardev are now being cleaned up at
the end of main, before atexit handlers.

Fix the crash by cleaning up the monitor when leaving, and restore the
original event behaviour when leaving by cleaning up audio before
that.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1355704

Marc-André Lureau (2):
  monitor: fix crash when leaving qemu with spice audio
  audio: clean up before monitor clean up

 monitor.c                 | 20 ++++++++++++++++++++
 audio/audio.c             | 26 ++++++++++++++++++--------
 audio/audio.h             |  3 +++
 audio/coreaudio.c         | 12 ++----------
 include/monitor/monitor.h |  1 +
 vl.c                      |  2 ++
 6 files changed, 46 insertions(+), 18 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 1/2] monitor: fix crash when leaving qemu with spice audio

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

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=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
 #1  0x00007fffdb1912fa in __GI_abort () at abort.c:89
 #2  0x0000555555c263e7 in error_exit (err=22, msg=0x555555d47980 <__func__.13537> "qemu_mutex_lock") at util/qemu-thread-posix.c:39
 #3  0x0000555555c26488 in qemu_mutex_lock (mutex=0x5555567a2420) at util/qemu-thread-posix.c:66
 #4  0x00005555558c52db in qemu_chr_fe_write (s=0x5555567a2420, buf=0x55555740dc40 "{\"timestamp\": {\"seconds\": 1470041716, \"microseconds\": 989699}, \"event\": \"SPICE_DISCONNECTED\", \"data\": {\"server\": {\"port\": \"5900\", \"family\": \"ipv4\", \"host\": \"127.0.0.1\"}, \"client\": {\"port\": \"40272\", \"f"..., len=240) at qemu-char.c:280
 #5  0x0000555555787cad in monitor_flush_locked (mon=0x5555567bd9e0) at /home/elmarco/src/qemu/monitor.c:311
 #6  0x0000555555787e46 in monitor_puts (mon=0x5555567bd9e0, str=0x5555567a44ef "") at /home/elmarco/src/qemu/monitor.c:353
 #7  0x00005555557880fe in monitor_json_emitter (mon=0x5555567bd9e0, data=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:401
 #8  0x00005555557882d2 in monitor_qapi_event_emit (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:472
 #9  0x000055555578838f in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0, errp=0x7fffffffca88) at /home/elmarco/src/qemu/monitor.c:497
 #10 0x0000555555c15541 in qapi_event_send_spice_disconnected (server=0x5555571139d0, client=0x5555570d0db0, errp=0x5555566c0428 <error_abort>) at qapi-event.c:1038
 #11 0x0000555555b11bc6 in channel_event (event=3, info=0x5555570d6c00) at ui/spice-core.c:248
 #12 0x00007fffdcc9983a in adapter_channel_event (event=3, info=0x5555570d6c00) at reds.c:120
 #13 0x00007fffdcc99a25 in reds_handle_channel_event (reds=0x5555567a9d60, event=3, info=0x5555570d6c00) at reds.c:324
 #14 0x00007fffdcc7d4c4 in main_dispatcher_self_handle_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:175
 #15 0x00007fffdcc7d5b1 in main_dispatcher_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:194
 #16 0x00007fffdcca7674 in reds_stream_push_channel_event (s=0x5555570d9910, event=3) at reds-stream.c:354
 #17 0x00007fffdcca749b in reds_stream_free (s=0x5555570d9910) at reds-stream.c:323
 #18 0x00007fffdccb5dad in snd_disconnect_channel (channel=0x5555576a89a0) at sound.c:229
 #19 0x00007fffdccb9e57 in snd_detach_common (worker=0x555557739720) at sound.c:1589
 #20 0x00007fffdccb9f0e in snd_detach_playback (sin=0x5555569fe3f8) at sound.c:1602
 #21 0x00007fffdcca3373 in spice_server_remove_interface (sin=0x5555569fe3f8) at reds.c:3387
 #22 0x00005555558ff6e2 in line_out_fini (hw=0x5555569fe370) at audio/spiceaudio.c:152
 #23 0x00005555558f909e in audio_atexit () at audio/audio.c:1754
 #24 0x00007fffdb1941e8 in __run_exit_handlers (status=0, listp=0x7fffdb5175d8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
 #25 0x00007fffdb194235 in __GI_exit (status=<optimized out>) at exit.c:104
 #26 0x00007fffdb17b738 in __libc_start_main (main=0x5555558d7874 <main>, argc=67, argv=0x7fffffffcf48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffcf38) at ../csu/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é Lureau <marcandre.lureau@redhat.com>
---
 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)
 
 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);
 }
 
+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)
 {
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);
 
 void monitor_init(CharDriverState *chr, int flags);
+void monitor_cleanup(void);
 
 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)
 
     /* vhost-user must be cleaned up before chardevs.  */
     net_cleanup();
+    monitor_cleanup();
     qemu_chr_cleanup();
 
     return 0;
-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Since aa5cb7f5e, the chardevs are being cleaned up when leaving qemu,
before the atexit() handlers. audio_cleanup() may use the monitor to
notify of changes. For compatibility reasons, let's clean up audio
before the monitor so it keeps emitting monitor events.

The audio_atexit() function is made idempotent (so it can be called
multiple times), and renamed to audio_cleanup(). Since coreaudio
backend is using a 'isAtexit' code path, change it to check
audio_is_cleaning_up() instead, so the path is taken during normal
exit.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 audio/audio.c     | 26 ++++++++++++++++++--------
 audio/audio.h     |  3 +++
 audio/coreaudio.c | 12 ++----------
 vl.c              |  1 +
 4 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 9d4dcc7..c845a44 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1739,13 +1739,21 @@ static void audio_vm_change_state_handler (void *opaque, int running,
     audio_reset_timer (s);
 }
 
-static void audio_atexit (void)
+static bool is_cleaning_up;
+
+bool audio_is_cleaning_up(void)
+{
+    return is_cleaning_up;
+}
+
+void audio_cleanup(void)
 {
     AudioState *s = &glob_audio_state;
-    HWVoiceOut *hwo = NULL;
-    HWVoiceIn *hwi = NULL;
+    HWVoiceOut *hwo, *hwon;
+    HWVoiceIn *hwi, *hwin;
 
-    while ((hwo = audio_pcm_hw_find_any_out (hwo))) {
+    is_cleaning_up = true;
+    QLIST_FOREACH_SAFE(hwo, &glob_audio_state.hw_head_out, entries, hwon) {
         SWVoiceCap *sc;
 
         if (hwo->enabled) {
@@ -1761,17 +1769,20 @@ static void audio_atexit (void)
                 cb->ops.destroy (cb->opaque);
             }
         }
+        QLIST_REMOVE(hwo, entries);
     }
 
-    while ((hwi = audio_pcm_hw_find_any_in (hwi))) {
+    QLIST_FOREACH_SAFE(hwi, &glob_audio_state.hw_head_in, entries, hwin) {
         if (hwi->enabled) {
             hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
         }
         hwi->pcm_ops->fini_in (hwi);
+        QLIST_REMOVE(hwi, entries);
     }
 
     if (s->drv) {
         s->drv->fini (s->drv_opaque);
+        s->drv = NULL;
     }
 }
 
@@ -1799,7 +1810,7 @@ static void audio_init (void)
     QLIST_INIT (&s->hw_head_out);
     QLIST_INIT (&s->hw_head_in);
     QLIST_INIT (&s->cap_head);
-    atexit (audio_atexit);
+    atexit(audio_cleanup);
 
     s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
 
@@ -1966,8 +1977,7 @@ CaptureVoiceOut *AUD_add_capture (
         QLIST_INSERT_HEAD (&s->cap_head, cap, entries);
         QLIST_INSERT_HEAD (&cap->cb_head, cb, entries);
 
-        hw = NULL;
-        while ((hw = audio_pcm_hw_find_any_out (hw))) {
+        QLIST_FOREACH(hw, &glob_audio_state.hw_head_out, entries) {
             audio_attach_capture (hw);
         }
         return cap;
diff --git a/audio/audio.h b/audio/audio.h
index 11e56c9..c3c5198 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -163,4 +163,7 @@ static inline void *advance (void *p, int incr)
 int wav_start_capture (CaptureState *s, const char *path, int freq,
                        int bits, int nchannels);
 
+bool audio_is_cleaning_up(void);
+void audio_cleanup(void);
+
 #endif /* QEMU_AUDIO_H */
diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index d4ad224..c751420 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -36,8 +36,6 @@
 #define MAC_OS_X_VERSION_10_6 1060
 #endif
 
-static int isAtexit;
-
 typedef struct {
     int buffer_frames;
     int nbuffers;
@@ -378,11 +376,6 @@ static inline UInt32 isPlaying (AudioDeviceID outputDeviceID)
     return result;
 }
 
-static void coreaudio_atexit (void)
-{
-    isAtexit = 1;
-}
-
 static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
 {
     int err;
@@ -630,7 +623,7 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
     int err;
     coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
 
-    if (!isAtexit) {
+    if (!audio_is_cleaning_up()) {
         /* stop playback */
         if (isPlaying(core->outputDeviceID)) {
             status = AudioDeviceStop(core->outputDeviceID, core->ioprocid);
@@ -673,7 +666,7 @@ static int coreaudio_ctl_out (HWVoiceOut *hw, int cmd, ...)
 
     case VOICE_DISABLE:
         /* stop playback */
-        if (!isAtexit) {
+        if (!audio_is_cleaning_up()) {
             if (isPlaying(core->outputDeviceID)) {
                 status = AudioDeviceStop(core->outputDeviceID,
                                          core->ioprocid);
@@ -697,7 +690,6 @@ static void *coreaudio_audio_init (void)
     CoreaudioConf *conf = g_malloc(sizeof(CoreaudioConf));
     *conf = glob_conf;
 
-    atexit(coreaudio_atexit);
     return conf;
 }
 
diff --git a/vl.c b/vl.c
index a14c438..c4eeaff 100644
--- a/vl.c
+++ b/vl.c
@@ -4612,6 +4612,7 @@ int main(int argc, char **argv, char **envp)
 
     /* vhost-user must be cleaned up before chardevs.  */
     net_cleanup();
+    audio_cleanup();
     monitor_cleanup();
     qemu_chr_cleanup();
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[Paolo Bonzini]

On 01/08/2016 13:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Since aa5cb7f5e, the chardevs are being cleaned up when leaving qemu,
> before the atexit() handlers. audio_cleanup() may use the monitor to
> notify of changes. For compatibility reasons, let's clean up audio
> before the monitor so it keeps emitting monitor events.
> 
> The audio_atexit() function is made idempotent (so it can be called
> multiple times),

That's a very good idea, as it avoids having to establish exit notifiers
as we did for net/.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

The two patches conflict with each other, so it's probably easiest if
one of Gerd or Markus takes both.

Thanks,

Paolo

> and renamed to audio_cleanup(). Since coreaudio
> backend is using a 'isAtexit' code path, change it to check
> audio_is_cleaning_up() instead, so the path is taken during normal
> exit.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  audio/audio.c     | 26 ++++++++++++++++++--------
>  audio/audio.h     |  3 +++
>  audio/coreaudio.c | 12 ++----------
>  vl.c              |  1 +
>  4 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 9d4dcc7..c845a44 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1739,13 +1739,21 @@ static void audio_vm_change_state_handler (void *opaque, int running,
>      audio_reset_timer (s);
>  }
>  
> -static void audio_atexit (void)
> +static bool is_cleaning_up;
> +
> +bool audio_is_cleaning_up(void)
> +{
> +    return is_cleaning_up;
> +}
> +
> +void audio_cleanup(void)
>  {
>      AudioState *s = &glob_audio_state;
> -    HWVoiceOut *hwo = NULL;
> -    HWVoiceIn *hwi = NULL;
> +    HWVoiceOut *hwo, *hwon;
> +    HWVoiceIn *hwi, *hwin;
>  
> -    while ((hwo = audio_pcm_hw_find_any_out (hwo))) {
> +    is_cleaning_up = true;
> +    QLIST_FOREACH_SAFE(hwo, &glob_audio_state.hw_head_out, entries, hwon) {
>          SWVoiceCap *sc;
>  
>          if (hwo->enabled) {
> @@ -1761,17 +1769,20 @@ static void audio_atexit (void)
>                  cb->ops.destroy (cb->opaque);
>              }
>          }
> +        QLIST_REMOVE(hwo, entries);
>      }
>  
> -    while ((hwi = audio_pcm_hw_find_any_in (hwi))) {
> +    QLIST_FOREACH_SAFE(hwi, &glob_audio_state.hw_head_in, entries, hwin) {
>          if (hwi->enabled) {
>              hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
>          }
>          hwi->pcm_ops->fini_in (hwi);
> +        QLIST_REMOVE(hwi, entries);
>      }
>  
>      if (s->drv) {
>          s->drv->fini (s->drv_opaque);
> +        s->drv = NULL;
>      }
>  }
>  
> @@ -1799,7 +1810,7 @@ static void audio_init (void)
>      QLIST_INIT (&s->hw_head_out);
>      QLIST_INIT (&s->hw_head_in);
>      QLIST_INIT (&s->cap_head);
> -    atexit (audio_atexit);
> +    atexit(audio_cleanup);
>  
>      s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
>  
> @@ -1966,8 +1977,7 @@ CaptureVoiceOut *AUD_add_capture (
>          QLIST_INSERT_HEAD (&s->cap_head, cap, entries);
>          QLIST_INSERT_HEAD (&cap->cb_head, cb, entries);
>  
> -        hw = NULL;
> -        while ((hw = audio_pcm_hw_find_any_out (hw))) {
> +        QLIST_FOREACH(hw, &glob_audio_state.hw_head_out, entries) {
>              audio_attach_capture (hw);
>          }
>          return cap;
> diff --git a/audio/audio.h b/audio/audio.h
> index 11e56c9..c3c5198 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -163,4 +163,7 @@ static inline void *advance (void *p, int incr)
>  int wav_start_capture (CaptureState *s, const char *path, int freq,
>                         int bits, int nchannels);
>  
> +bool audio_is_cleaning_up(void);
> +void audio_cleanup(void);
> +
>  #endif /* QEMU_AUDIO_H */
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index d4ad224..c751420 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -36,8 +36,6 @@
>  #define MAC_OS_X_VERSION_10_6 1060
>  #endif
>  
> -static int isAtexit;
> -
>  typedef struct {
>      int buffer_frames;
>      int nbuffers;
> @@ -378,11 +376,6 @@ static inline UInt32 isPlaying (AudioDeviceID outputDeviceID)
>      return result;
>  }
>  
> -static void coreaudio_atexit (void)
> -{
> -    isAtexit = 1;
> -}
> -
>  static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
>  {
>      int err;
> @@ -630,7 +623,7 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
>      int err;
>      coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
>  
> -    if (!isAtexit) {
> +    if (!audio_is_cleaning_up()) {
>          /* stop playback */
>          if (isPlaying(core->outputDeviceID)) {
>              status = AudioDeviceStop(core->outputDeviceID, core->ioprocid);
> @@ -673,7 +666,7 @@ static int coreaudio_ctl_out (HWVoiceOut *hw, int cmd, ...)
>  
>      case VOICE_DISABLE:
>          /* stop playback */
> -        if (!isAtexit) {
> +        if (!audio_is_cleaning_up()) {
>              if (isPlaying(core->outputDeviceID)) {
>                  status = AudioDeviceStop(core->outputDeviceID,
>                                           core->ioprocid);
> @@ -697,7 +690,6 @@ static void *coreaudio_audio_init (void)
>      CoreaudioConf *conf = g_malloc(sizeof(CoreaudioConf));
>      *conf = glob_conf;
>  
> -    atexit(coreaudio_atexit);
>      return conf;
>  }
>  
> diff --git a/vl.c b/vl.c
> index a14c438..c4eeaff 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4612,6 +4612,7 @@ int main(int argc, char **argv, char **envp)
>  
>      /* vhost-user must be cleaned up before chardevs.  */
>      net_cleanup();
> +    audio_cleanup();
>      monitor_cleanup();
>      qemu_chr_cleanup();
>  
> 

Re: [Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[Marc-André Lureau]

Hi

----- Original Message -----
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Since aa5cb7f5e, the chardevs are being cleaned up when leaving qemu,
> before the atexit() handlers. audio_cleanup() may use the monitor to
> notify of changes. For compatibility reasons, let's clean up audio
> before the monitor so it keeps emitting monitor events.
> 
> The audio_atexit() function is made idempotent (so it can be called
> multiple times), and renamed to audio_cleanup(). Since coreaudio
> backend is using a 'isAtexit' code path, change it to check
> audio_is_cleaning_up() instead, so the path is taken during normal
> exit.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  audio/audio.c     | 26 ++++++++++++++++++--------
>  audio/audio.h     |  3 +++
>  audio/coreaudio.c | 12 ++----------
>  vl.c              |  1 +
>  4 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 9d4dcc7..c845a44 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1739,13 +1739,21 @@ static void audio_vm_change_state_handler (void
> *opaque, int running,
>      audio_reset_timer (s);
>  }
>  
> -static void audio_atexit (void)
> +static bool is_cleaning_up;
> +
> +bool audio_is_cleaning_up(void)
> +{
> +    return is_cleaning_up;
> +}
> +
> +void audio_cleanup(void)
>  {
>      AudioState *s = &glob_audio_state;
> -    HWVoiceOut *hwo = NULL;
> -    HWVoiceIn *hwi = NULL;
> +    HWVoiceOut *hwo, *hwon;
> +    HWVoiceIn *hwi, *hwin;
>  
> -    while ((hwo = audio_pcm_hw_find_any_out (hwo))) {
> +    is_cleaning_up = true;
> +    QLIST_FOREACH_SAFE(hwo, &glob_audio_state.hw_head_out, entries, hwon) {
>          SWVoiceCap *sc;
>  
>          if (hwo->enabled) {
> @@ -1761,17 +1769,20 @@ static void audio_atexit (void)
>                  cb->ops.destroy (cb->opaque);
>              }
>          }
> +        QLIST_REMOVE(hwo, entries);
>      }
>  
> -    while ((hwi = audio_pcm_hw_find_any_in (hwi))) {
> +    QLIST_FOREACH_SAFE(hwi, &glob_audio_state.hw_head_in, entries, hwin) {
>          if (hwi->enabled) {
>              hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
>          }
>          hwi->pcm_ops->fini_in (hwi);
> +        QLIST_REMOVE(hwi, entries);
>      }
>  
>      if (s->drv) {
>          s->drv->fini (s->drv_opaque);
> +        s->drv = NULL;
>      }
>  }
>  
> @@ -1799,7 +1810,7 @@ static void audio_init (void)
>      QLIST_INIT (&s->hw_head_out);
>      QLIST_INIT (&s->hw_head_in);
>      QLIST_INIT (&s->cap_head);
> -    atexit (audio_atexit);
> +    atexit(audio_cleanup);
>  
>      s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
>  
> @@ -1966,8 +1977,7 @@ CaptureVoiceOut *AUD_add_capture (
>          QLIST_INSERT_HEAD (&s->cap_head, cap, entries);
>          QLIST_INSERT_HEAD (&cap->cb_head, cb, entries);
>  
> -        hw = NULL;
> -        while ((hw = audio_pcm_hw_find_any_out (hw))) {
> +        QLIST_FOREACH(hw, &glob_audio_state.hw_head_out, entries) {
>              audio_attach_capture (hw);
>          }

That hunk is unnecessary (although it removes the usage of an unpleasant glue function)

>          return cap;
> diff --git a/audio/audio.h b/audio/audio.h
> index 11e56c9..c3c5198 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -163,4 +163,7 @@ static inline void *advance (void *p, int incr)
>  int wav_start_capture (CaptureState *s, const char *path, int freq,
>                         int bits, int nchannels);
>  
> +bool audio_is_cleaning_up(void);
> +void audio_cleanup(void);
> +
>  #endif /* QEMU_AUDIO_H */
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index d4ad224..c751420 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -36,8 +36,6 @@
>  #define MAC_OS_X_VERSION_10_6 1060
>  #endif
>  
> -static int isAtexit;
> -
>  typedef struct {
>      int buffer_frames;
>      int nbuffers;
> @@ -378,11 +376,6 @@ static inline UInt32 isPlaying (AudioDeviceID
> outputDeviceID)
>      return result;
>  }
>  
> -static void coreaudio_atexit (void)
> -{
> -    isAtexit = 1;
> -}
> -
>  static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
>  {
>      int err;
> @@ -630,7 +623,7 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
>      int err;
>      coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
>  
> -    if (!isAtexit) {
> +    if (!audio_is_cleaning_up()) {
>          /* stop playback */
>          if (isPlaying(core->outputDeviceID)) {
>              status = AudioDeviceStop(core->outputDeviceID, core->ioprocid);
> @@ -673,7 +666,7 @@ static int coreaudio_ctl_out (HWVoiceOut *hw, int cmd,
> ...)
>  
>      case VOICE_DISABLE:
>          /* stop playback */
> -        if (!isAtexit) {
> +        if (!audio_is_cleaning_up()) {
>              if (isPlaying(core->outputDeviceID)) {
>                  status = AudioDeviceStop(core->outputDeviceID,
>                                           core->ioprocid);
> @@ -697,7 +690,6 @@ static void *coreaudio_audio_init (void)
>      CoreaudioConf *conf = g_malloc(sizeof(CoreaudioConf));
>      *conf = glob_conf;
>  
> -    atexit(coreaudio_atexit);
>      return conf;
>  }
>  
> diff --git a/vl.c b/vl.c
> index a14c438..c4eeaff 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4612,6 +4612,7 @@ int main(int argc, char **argv, char **envp)
>  
>      /* vhost-user must be cleaned up before chardevs.  */
>      net_cleanup();
> +    audio_cleanup();
>      monitor_cleanup();
>      qemu_chr_cleanup();
>  
> --
> 2.9.0
> 
> 

Re: [Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/08/2016 13:23, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> Since aa5cb7f5e, the chardevs are being cleaned up when leaving qemu,
>> before the atexit() handlers. audio_cleanup() may use the monitor to
>> notify of changes. For compatibility reasons, let's clean up audio
>> before the monitor so it keeps emitting monitor events.
>> 
>> The audio_atexit() function is made idempotent (so it can be called
>> multiple times),
>
> That's a very good idea, as it avoids having to establish exit notifiers
> as we did for net/.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Does the R-by apply to PATCH 1/2 as well?

> The two patches conflict with each other, so it's probably easiest if
> one of Gerd or Markus takes both.

I can squeeze in review and a pull request today.  Gerd, if you'd prefer
to do it yourself, let me know.

Re: [Qemu-devel] [PATCH for-2.7 1/2] monitor: fix crash when leaving qemu with spice audio

[Paolo Bonzini]

On 01/08/2016 13:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
>  #1  0x00007fffdb1912fa in __GI_abort () at abort.c:89
>  #2  0x0000555555c263e7 in error_exit (err=22, msg=0x555555d47980 <__func__.13537> "qemu_mutex_lock") at util/qemu-thread-posix.c:39
>  #3  0x0000555555c26488 in qemu_mutex_lock (mutex=0x5555567a2420) at util/qemu-thread-posix.c:66
>  #4  0x00005555558c52db in qemu_chr_fe_write (s=0x5555567a2420, buf=0x55555740dc40 "{\"timestamp\": {\"seconds\": 1470041716, \"microseconds\": 989699}, \"event\": \"SPICE_DISCONNECTED\", \"data\": {\"server\": {\"port\": \"5900\", \"family\": \"ipv4\", \"host\": \"127.0.0.1\"}, \"client\": {\"port\": \"40272\", \"f"..., len=240) at qemu-char.c:280
>  #5  0x0000555555787cad in monitor_flush_locked (mon=0x5555567bd9e0) at /home/elmarco/src/qemu/monitor.c:311
>  #6  0x0000555555787e46 in monitor_puts (mon=0x5555567bd9e0, str=0x5555567a44ef "") at /home/elmarco/src/qemu/monitor.c:353
>  #7  0x00005555557880fe in monitor_json_emitter (mon=0x5555567bd9e0, data=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:401
>  #8  0x00005555557882d2 in monitor_qapi_event_emit (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:472
>  #9  0x000055555578838f in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0, errp=0x7fffffffca88) at /home/elmarco/src/qemu/monitor.c:497
>  #10 0x0000555555c15541 in qapi_event_send_spice_disconnected (server=0x5555571139d0, client=0x5555570d0db0, errp=0x5555566c0428 <error_abort>) at qapi-event.c:1038
>  #11 0x0000555555b11bc6 in channel_event (event=3, info=0x5555570d6c00) at ui/spice-core.c:248
>  #12 0x00007fffdcc9983a in adapter_channel_event (event=3, info=0x5555570d6c00) at reds.c:120
>  #13 0x00007fffdcc99a25 in reds_handle_channel_event (reds=0x5555567a9d60, event=3, info=0x5555570d6c00) at reds.c:324
>  #14 0x00007fffdcc7d4c4 in main_dispatcher_self_handle_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:175
>  #15 0x00007fffdcc7d5b1 in main_dispatcher_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:194
>  #16 0x00007fffdcca7674 in reds_stream_push_channel_event (s=0x5555570d9910, event=3) at reds-stream.c:354
>  #17 0x00007fffdcca749b in reds_stream_free (s=0x5555570d9910) at reds-stream.c:323
>  #18 0x00007fffdccb5dad in snd_disconnect_channel (channel=0x5555576a89a0) at sound.c:229
>  #19 0x00007fffdccb9e57 in snd_detach_common (worker=0x555557739720) at sound.c:1589
>  #20 0x00007fffdccb9f0e in snd_detach_playback (sin=0x5555569fe3f8) at sound.c:1602
>  #21 0x00007fffdcca3373 in spice_server_remove_interface (sin=0x5555569fe3f8) at reds.c:3387
>  #22 0x00005555558ff6e2 in line_out_fini (hw=0x5555569fe370) at audio/spiceaudio.c:152
>  #23 0x00005555558f909e in audio_atexit () at audio/audio.c:1754
>  #24 0x00007fffdb1941e8 in __run_exit_handlers (status=0, listp=0x7fffdb5175d8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
>  #25 0x00007fffdb194235 in __GI_exit (status=<optimized out>) at exit.c:104
>  #26 0x00007fffdb17b738 in __libc_start_main (main=0x5555558d7874 <main>, argc=67, argv=0x7fffffffcf48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffcf38) at ../csu/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é Lureau <marcandre.lureau@redhat.com>
> ---
>  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)
>  
>  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);
>  }
>  
> +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)
>  {
> 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);
>  
>  void monitor_init(CharDriverState *chr, int flags);
> +void monitor_cleanup(void);
>  
>  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)
>  
>      /* vhost-user must be cleaned up before chardevs.  */
>      net_cleanup();
> +    monitor_cleanup();
>      qemu_chr_cleanup();
>  
>      return 0;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[Paolo Bonzini]

On 08/08/2016 11:45, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 01/08/2016 13:23, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Since aa5cb7f5e, the chardevs are being cleaned up when leaving qemu,
>>> before the atexit() handlers. audio_cleanup() may use the monitor to
>>> notify of changes. For compatibility reasons, let's clean up audio
>>> before the monitor so it keeps emitting monitor events.
>>>
>>> The audio_atexit() function is made idempotent (so it can be called
>>> multiple times),
>>
>> That's a very good idea, as it avoids having to establish exit notifiers
>> as we did for net/.
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Does the R-by apply to PATCH 1/2 as well?

I've now sent a separate one. :)

Paolo

>> The two patches conflict with each other, so it's probably easiest if
>> one of Gerd or Markus takes both.
> 
> I can squeeze in review and a pull request today.  Gerd, if you'd prefer
> to do it yourself, let me know.
> 
> 

Re: [Qemu-devel] [PATCH for-2.7 2/2] audio: clean up before monitor clean up

[Gerd Hoffmann]

  Hi,

> > The two patches conflict with each other, so it's probably easiest if
> > one of Gerd or Markus takes both.
> 
> I can squeeze in review and a pull request today.  Gerd, if you'd prefer
> to do it yourself, let me know.

Fine with me, go ahead.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-devel] [PATCH for-2.7 1/2] monitor: fix crash when leaving qemu with spice audio

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
>  #1  0x00007fffdb1912fa in __GI_abort () at abort.c:89
>  #2  0x0000555555c263e7 in error_exit (err=22, msg=0x555555d47980 <__func__.13537> "qemu_mutex_lock") at util/qemu-thread-posix.c:39
>  #3  0x0000555555c26488 in qemu_mutex_lock (mutex=0x5555567a2420) at util/qemu-thread-posix.c:66
>  #4  0x00005555558c52db in qemu_chr_fe_write (s=0x5555567a2420, buf=0x55555740dc40 "{\"timestamp\": {\"seconds\": 1470041716, \"microseconds\": 989699}, \"event\": \"SPICE_DISCONNECTED\", \"data\": {\"server\": {\"port\": \"5900\", \"family\": \"ipv4\", \"host\": \"127.0.0.1\"}, \"client\": {\"port\": \"40272\", \"f"..., len=240) at qemu-char.c:280
>  #5  0x0000555555787cad in monitor_flush_locked (mon=0x5555567bd9e0) at /home/elmarco/src/qemu/monitor.c:311
>  #6  0x0000555555787e46 in monitor_puts (mon=0x5555567bd9e0, str=0x5555567a44ef "") at /home/elmarco/src/qemu/monitor.c:353
>  #7  0x00005555557880fe in monitor_json_emitter (mon=0x5555567bd9e0, data=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:401
>  #8  0x00005555557882d2 in monitor_qapi_event_emit (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0) at /home/elmarco/src/qemu/monitor.c:472
>  #9  0x000055555578838f in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x5555567c73a0, errp=0x7fffffffca88) at /home/elmarco/src/qemu/monitor.c:497
>  #10 0x0000555555c15541 in qapi_event_send_spice_disconnected (server=0x5555571139d0, client=0x5555570d0db0, errp=0x5555566c0428 <error_abort>) at qapi-event.c:1038
>  #11 0x0000555555b11bc6 in channel_event (event=3, info=0x5555570d6c00) at ui/spice-core.c:248
>  #12 0x00007fffdcc9983a in adapter_channel_event (event=3, info=0x5555570d6c00) at reds.c:120
>  #13 0x00007fffdcc99a25 in reds_handle_channel_event (reds=0x5555567a9d60, event=3, info=0x5555570d6c00) at reds.c:324
>  #14 0x00007fffdcc7d4c4 in main_dispatcher_self_handle_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:175
>  #15 0x00007fffdcc7d5b1 in main_dispatcher_channel_event (self=0x5555567b28b0, event=3, info=0x5555570d6c00) at main-dispatcher.c:194
>  #16 0x00007fffdcca7674 in reds_stream_push_channel_event (s=0x5555570d9910, event=3) at reds-stream.c:354
>  #17 0x00007fffdcca749b in reds_stream_free (s=0x5555570d9910) at reds-stream.c:323
>  #18 0x00007fffdccb5dad in snd_disconnect_channel (channel=0x5555576a89a0) at sound.c:229
>  #19 0x00007fffdccb9e57 in snd_detach_common (worker=0x555557739720) at sound.c:1589
>  #20 0x00007fffdccb9f0e in snd_detach_playback (sin=0x5555569fe3f8) at sound.c:1602
>  #21 0x00007fffdcca3373 in spice_server_remove_interface (sin=0x5555569fe3f8) at reds.c:3387
>  #22 0x00005555558ff6e2 in line_out_fini (hw=0x5555569fe370) at audio/spiceaudio.c:152
>  #23 0x00005555558f909e in audio_atexit () at audio/audio.c:1754
>  #24 0x00007fffdb1941e8 in __run_exit_handlers (status=0, listp=0x7fffdb5175d8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
>  #25 0x00007fffdb194235 in __GI_exit (status=<optimized out>) at exit.c:104
>  #26 0x00007fffdb17b738 in __libc_start_main (main=0x5555558d7874 <main>, argc=67, argv=0x7fffffffcf48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffcf38) at ../csu/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é Lureau <marcandre.lureau@redhat.com>
> ---
>  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)
>  
>  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);
>  }
>  
> +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);
>  
>  void monitor_init(CharDriverState *chr, int flags);
> +void monitor_cleanup(void);
>  
>  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)
>  
>      /* vhost-user must be cleaned up before chardevs.  */
>      net_cleanup();
> +    monitor_cleanup();
>      qemu_chr_cleanup();
>  
>      return 0;

Reviewed-by: Markus Armbruster <armbru@redhat.com>