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 Di, 2013-11-05 at 20:34 +, Peter Maydell wrote: On 5 November 2013 19:57, Anthony Liguori anth...@codemonkey.ws 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 6 November 2013 09:18, Gerd Hoffmann kra...@redhat.com 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 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 Wed, Nov 6, 2013 at 3:15 AM, Gerd Hoffmann kra...@redhat.com 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 6 November 2013 14:54, Anthony Liguori anth...@codemonkey.ws 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
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 Wed, Nov 6, 2013 at 7:20 AM, Gerd Hoffmann kra...@redhat.com 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
Gerd Hoffmann kra...@redhat.com 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 5 November 2013 19:57, Anthony Liguori anth...@codemonkey.ws 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
On 5 November 2013 19:57, Anthony Liguori anth...@codemonkey.ws 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 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 3 November 2013 16:45, Anthony Liguori anth...@codemonkey.ws 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 aligu...@amazon.com
---
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
[Qemu-devel] [PATCH] ossaudio: do not enable by default
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 aligu...@amazon.com
---
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),
--
1.8.3.1
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 aligu...@amazon.com --- 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 afaer...@suse.de 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
Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default
On Sun, Nov 3, 2013 at 9:12 AM, Andreas Färber afaer...@suse.de 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 aligu...@amazon.com --- 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 afaer...@suse.de 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
