Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
Hi, > And that is actually simple enough that we can consider it for 1.7: > > > > static void *oss_audio_init (void) > > { > > +if (access(conf.devpath_in, R_OK | W_OK) < 0 || > > +access(conf.devpath_out, R_OK | W_OK) < 0) { > > +return NULL; > > That would be reasonable. Can you add a SoB and submit as a patch? Pull req sent. cheers, Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Wed, Nov 6, 2013 at 7:20 AM, Gerd Hoffmann wrote: > Hi, > >> > static void *oss_audio_init (void) >> > { >> > return &conf; >> > } >> > >> > It never fails. >> >> OK, that's a bug. (I'd misread the calling function >> audio_driver_init() as also checking that the init_in >> and init_out functions succeeded, which it does not.) >> >> > So audio is broken on Linux by default today. This >> > patch unbreaks it. >> >> No, this patch is papering over the problem by giving us >> a default config where audio works for nobody. >> >> If you want to fix that problem you need to do it by >> making the oss_audio_init() function return failure >> on init. > > And that is actually simple enough that we can consider it for 1.7: > > static void *oss_audio_init (void) > { > +if (access(conf.devpath_in, R_OK | W_OK) < 0 || > +access(conf.devpath_out, R_OK | W_OK) < 0) { > +return NULL; That would be reasonable. Can you add a SoB and submit as a patch? Regards, Anthony Liguori > +} > return &conf; > } > > cheers, > Gerd > >
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
Hi, > > static void *oss_audio_init (void) > > { > > return &conf; > > } > > > > It never fails. > > OK, that's a bug. (I'd misread the calling function > audio_driver_init() as also checking that the init_in > and init_out functions succeeded, which it does not.) > > > So audio is broken on Linux by default today. This > > patch unbreaks it. > > No, this patch is papering over the problem by giving us > a default config where audio works for nobody. > > If you want to fix that problem you need to do it by > making the oss_audio_init() function return failure > on init. And that is actually simple enough that we can consider it for 1.7: static void *oss_audio_init (void) { +if (access(conf.devpath_in, R_OK | W_OK) < 0 || +access(conf.devpath_out, R_OK | W_OK) < 0) { +return NULL; +} return &conf; } cheers, Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On 6 November 2013 14:54, Anthony Liguori wrote: > I don't think you guys understand what is happening. > > As ossaudio is able to be default, it *will be selected* as the audio > output backend unconditionally. You aren't seeing errors during > probing, you're seeing errors post-initialization. > > The ossaudio init function is simply: > > static void *oss_audio_init (void) > { > return &conf; > } > > It never fails. OK, that's a bug. (I'd misread the calling function audio_driver_init() as also checking that the init_in and init_out functions succeeded, which it does not.) > So audio is broken on Linux by default today. This > patch unbreaks it. No, this patch is papering over the problem by giving us a default config where audio works for nobody. If you want to fix that problem you need to do it by making the oss_audio_init() function return failure on init. I think the major point is still the same: * there is a bug here * this patch doesn't fix it * this bug has been present for years * we should leave 1.7 behaving like 1.6 & earlier, and fix properly for 1.8 -- PMM
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Wed, Nov 6, 2013 at 3:15 AM, Gerd Hoffmann wrote: > On Mi, 2013-11-06 at 10:48 +, Peter Maydell wrote: > >> > That is clearly 1.8 material though. I think for 1.7 we should simply >> > leave things as-is. >> >> Do you mean "as-is with Anthony's patch applied", or "as it was >> before that patch was applied" ? > > Oh, it is in? > >> I would suggest the latter >> (ie revert this patch), because that's the safest choice this >> close to release. > > Agree. I don't think you guys understand what is happening. As ossaudio is able to be default, it *will be selected* as the audio output backend unconditionally. You aren't seeing errors during probing, you're seeing errors post-initialization. The ossaudio init function is simply: static void *oss_audio_init (void) { return &conf; } It never fails. So audio is broken on Linux by default today. This patch unbreaks it. If you compare this to the pulseaudio backend where init can actually fail, you can see why it can be default but ossaudio really can't. Regards, Anthony Liguori > cheers, > Gerd > >
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Mi, 2013-11-06 at 10:48 +, Peter Maydell wrote: > > That is clearly 1.8 material though. I think for 1.7 we should simply > > leave things as-is. > > Do you mean "as-is with Anthony's patch applied", or "as it was > before that patch was applied" ? Oh, it is in? > I would suggest the latter > (ie revert this patch), because that's the safest choice this > close to release. Agree. cheers, Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On 6 November 2013 09:18, Gerd Hoffmann wrote: > I think the audio backend handling needs a serious makeover. Detect > sound libraries (pulse, esd, alsa, ...) via configure like any other > library. Zap the whole can_be_default logic. Replace it by walking the > list of backends, sorted by priority, take the first which initializes > successfully. I agree on the need for a serious overhaul. I think the thing we're actually hitting at the moment is that none of the backends are quiet when they fail to initialize, which is bad since we already have "loop through and take first which initializes" logic. > That is clearly 1.8 material though. I think for 1.7 we should simply > leave things as-is. Do you mean "as-is with Anthony's patch applied", or "as it was before that patch was applied" ? I would suggest the latter (ie revert this patch), because that's the safest choice this close to release. thanks -- PMM
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Di, 2013-11-05 at 20:34 +, Peter Maydell wrote: > On 5 November 2013 19:57, Anthony Liguori wrote: > > This patch just requires that you explicitly select oss so it's not > > breaking audio on BSD. > > That sounds to me like it's breaking audio for all the > users for whom it previously worked out of the box > without any particular configure or command line options. > In fact I suspect it also breaks audio for all the *linux* > users for whom it previously worked without special options. Yes, it does. Sound is broken by default now. Only oss is compiled by default, but it isn't used any more unless you explicitly request that. My suggestion has it problems too, when adding alsa that way without having alsa-devel package installed the build breaks. I think the audio backend handling needs a serious makeover. Detect sound libraries (pulse, esd, alsa, ...) via configure like any other library. Zap the whole can_be_default logic. Replace it by walking the list of backends, sorted by priority, take the first which initializes successfully. That is clearly 1.8 material though. I think for 1.7 we should simply leave things as-is. I have a custom --audio-drv-list in my build scripts for ages, and I suspect all distros and anyone who compiles itself and cares about audio has. cheers, Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On 5 November 2013 19:57, Anthony Liguori wrote: > This patch just requires that you explicitly select oss so it's not > breaking audio on BSD. That sounds to me like it's breaking audio for all the users for whom it previously worked out of the box without any particular configure or command line options. In fact I suspect it also breaks audio for all the *linux* users for whom it previously worked without special options. -- PMM
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On 5 November 2013 19:57, Anthony Liguori wrote: > Since the oss code can fail to initialize without handling it > gracefully, it really cannot be default on any platform. Can you describe what the actual problem is we're trying to fix here, please? I can't see a description of it in any of the mailing list threads (there are references to irc conversations but it's really not clear what the actual symptoms are). I have a system with no /dev/dsp, and it can build and run systems fine (including passing 'make check') with the oss backend as the (default-by-configure) only backend. The only thing that happens is that there are a bunch of warnings from ossaudio, like this: oss: Could not initialize ADC oss: Failed to open `/dev/dsp' oss: Reason: No such file or directory oss: Could not initialize ADC oss: Failed to open `/dev/dsp' oss: Reason: No such file or directory audio: Failed to create voice `mm_ac97.in' but they don't prevent make check from working, they don't prevent systems from booting, and they have been present for *years*. I really don't see why you suddenly needed to apply this patch despite the fact that most of the response you got to it was negative. thanks -- PMM
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
Gerd Hoffmann writes: > On So, 2013-11-03 at 08:45 -0800, Anthony Liguori wrote: >> Modern Linux's no longer support /dev/dsp so enabling it by >> default causes audio failures on newer Linux distros. > > That will break sound on BSD. > > I think we should do something like this instead: > > --- a/configure > +++ b/configure > @@ -554,7 +554,7 @@ Haiku) >LIBS="-lposix_error_mapper -lnetwork $LIBS" > ;; > *) > - audio_drv_list="oss" > + audio_drv_list="pa alsa oss" >audio_possible_drivers="oss alsa sdl esd pa" >linux="yes" >linux_user="yes" > > i.e. build pulseaudio and alsa by default on linux and prioritize them > over oss. This patch just requires that you explicitly select oss so it's not breaking audio on BSD. Since the oss code can fail to initialize without handling it gracefully, it really cannot be default on any platform. Same problem would occur on BSD if the permissions on /dev/dsp were restrictive. Regards, Anthony Liguori > > cheers, > Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On 3 November 2013 16:45, Anthony Liguori wrote: > Modern Linux's no longer support /dev/dsp so enabling it by > default causes audio failures on newer Linux distros. > > Signed-off-by: Anthony Liguori > --- > audio/ossaudio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/audio/ossaudio.c b/audio/ossaudio.c > index 007c641..3e04a58 100644 > --- a/audio/ossaudio.c > +++ b/audio/ossaudio.c > @@ -932,7 +932,7 @@ struct audio_driver oss_audio_driver = { > .init = oss_audio_init, > .fini = oss_audio_fini, > .pcm_ops= &oss_pcm_ops, > -.can_be_default = 1, > +.can_be_default = 0, > .max_voices_out = INT_MAX, > .max_voices_in = INT_MAX, > .voice_size_out = sizeof (OSSVoiceOut), This doesn't look like the right fix for the problem given in the commit message. If the oss init function doesn't cleanly return a failure so we can loop round and try another driver then the init function should be fixed. If QEMU code itself is printing warnings then we should silence them. If you're just seeing warnings from some system audio library (ALSA/pulse/etc) then either file upstream bugs or just pass configure the correct audio driver option for your distro. thanks -- PMM
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On So, 2013-11-03 at 08:45 -0800, Anthony Liguori wrote: > Modern Linux's no longer support /dev/dsp so enabling it by > default causes audio failures on newer Linux distros. That will break sound on BSD. I think we should do something like this instead: --- a/configure +++ b/configure @@ -554,7 +554,7 @@ Haiku) LIBS="-lposix_error_mapper -lnetwork $LIBS" ;; *) - audio_drv_list="oss" + audio_drv_list="pa alsa oss" audio_possible_drivers="oss alsa sdl esd pa" linux="yes" linux_user="yes" i.e. build pulseaudio and alsa by default on linux and prioritize them over oss. cheers, Gerd
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Sun, Nov 3, 2013 at 9:12 AM, Andreas Färber wrote: > Am 03.11.2013 17:45, schrieb Anthony Liguori: >> Modern Linux's no longer support /dev/dsp so enabling it by >> default causes audio failures on newer Linux distros. >> >> Signed-off-by: Anthony Liguori >> --- >> audio/ossaudio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > With my current config, this patch is not making a change. make > check-qtest-arm succeeds on qom-next branch with and without it without > any annoying output. So FWIW > > Tested-by: Andreas Färber > > Note that author and Sob differ, probably not intentionally. Nope, thanks. Regards, Anthony Liguori > > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
Am 03.11.2013 17:45, schrieb Anthony Liguori: > Modern Linux's no longer support /dev/dsp so enabling it by > default causes audio failures on newer Linux distros. > > Signed-off-by: Anthony Liguori > --- > audio/ossaudio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) With my current config, this patch is not making a change. make check-qtest-arm succeeds on qom-next branch with and without it without any annoying output. So FWIW Tested-by: Andreas Färber Note that author and Sob differ, probably not intentionally. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg