[Qemu-devel] Re: SDL audio and AIO hogging each other's signals

["andrzej zaborowski" <balrog@zabor.org>]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

On 04/04/07, malc <av1474@comtv.ru> wrote:
> On Wed, 4 Apr 2007, andrzej zaborowski wrote:
>
> > Hi, thanks for quick response!
> >
> > On 03/04/07, malc <av1474@comtv.ru> wrote:
> >> On Tue, 3 Apr 2007, andrzej zaborowski wrote:
>
> [..snip..]
>
> > It should be using ALSA.
> >
> >>
> >> Here's my theory: signal will be delivered to the arbitrary thread
> >> that happens to not block it, for whatever reason, the thread SDL
> >> created to do audio processing is picked up, which leads to some
> >> system call being interrupted(eventually) and -1 (errno == EINTR), SDL
> >> happily continues calling stuff.
>
> Actually the signal can be just handled (by whatever signal handler
> QEMU installed with sigaction) in a SDL created thread, so things can,
> and mostlikely will, be silently wrong, i.e. EINTR while possible will
> not necessarily happen.
>
> >
> > Yes, reading the PTHREAD_SIGNAL(3) note, this sounds very likely.
> >
> >>
> >> One solution would be to explicitly block everything upon entering
> >> sdl_callback for the first time. This is ugly and can have any
> >> consequences one cares to imagine, but that's SDL for you (any
> >> particular reason why you are using it?)
> >
> > Not really - just had only OSS and SDL compiled into qemu at this moment.
> >
> > Yes, the suggested solution works. Unfortunately it's neither pretty
> > nor correct, because at the time sdl_callback runs, the signal which
> > was supposed to wake up sigwait() is already lost and I can't find any
> > way to prevent that - we can add a kill(0, SIGUSR2) but this is even
> > uglier and again we don't know when sdl_callback is called as a result
> > of this signal and when legally. (That's also why I didn't put a
> > "return" after pthread_sigmask().)
>
> [..snip..]
>
> > What POSIX needs is a way to set the default signal mask for new threads :-/
>
> http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_create.html
> in part reads
> <quote>
> The signal state of the new thread is initialised as follows:
>
>      * The signal mask is inherited from the creating thread.
> </quote>

Oh, right.

>
> Hence sequence of:
>
> sigfillset (newset)
> pthread_sigmask (SIG_BLOCK, newset, oldset)
> SDL_OpenAudio (...)
> pthread_sigmask (SGI_SETMASK, oldset, NULL)
>
> Will probably achieve the desired effect.

Thanks, this works!

Attached is a patch, should be safe to apply. We're still making the
assumption that the thread is created precisely in SDL_OpenAudio and
not later, but this should not be a problem because all signals in the
main thread should be blocked throughout the entire runtime.

Regards,
Andrzej

[-- Attachment #2: 0001-Ensure-signals-are-properly-masked-for-new-SDL-Audio-threads.txt --]
[-- Type: text/plain, Size: 1270 bytes --]

From 6843399e5e751e999f33de09e4476f7c96839974 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <balrog@zabor.org>
Date: Wed, 4 Apr 2007 15:18:20 +0200
Subject: [PATCH] Ensure signals are properly masked for new SDL Audio threads.

---
 audio/sdlaudio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/audio/sdlaudio.c b/audio/sdlaudio.c
index f2a6896..11edab0 100644
--- a/audio/sdlaudio.c
+++ b/audio/sdlaudio.c
@@ -25,6 +25,13 @@
 #include <SDL_thread.h>
 #include "vl.h"
 
+#ifndef _WIN32
+#ifdef __sun__
+#define _POSIX_PTHREAD_SEMANTICS 1
+#endif
+#include <signal.h>
+#endif
+
 #define AUDIO_CAP "sdl"
 #include "audio_int.h"
 
@@ -177,11 +184,22 @@ static int sdl_to_audfmt (int sdlfmt, audfmt_e *fmt, int *endianess)
 static int sdl_open (SDL_AudioSpec *req, SDL_AudioSpec *obt)
 {
     int status;
+#ifndef _WIN32
+    sigset_t new, old;
+
+    /* Make sure potential threads created by SDL don't hog signals.  */
+    sigfillset (&new);
+    pthread_sigmask (SIG_BLOCK, &new, &old);
+#endif
 
     status = SDL_OpenAudio (req, obt);
     if (status) {
         sdl_logerr ("SDL_OpenAudio failed\n");
     }
+
+#ifndef _WIN32
+    pthread_sigmask (SIG_SETMASK, &old, 0);
+#endif
     return status;
 }
 
-- 
1.4.4.3