[Qemu-devel] [PATCH] ossaudio: do not enable by default

[Qemu-devel] [PATCH] ossaudio: do not enable by default

[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 <aliguori@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

[Andreas Färber]

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 <aliguori@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 <afaerber@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

[Anthony Liguori]

On Sun, Nov 3, 2013 at 9:12 AM, Andreas Färber <afaerber@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 <aliguori@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 <afaerber@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

Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default

[Gerd Hoffmann]

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

[Peter Maydell]

On 3 November 2013 16:45, Anthony Liguori <anthony@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 <aliguori@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

Re: [Qemu-devel] [PATCH] ossaudio: do not enable by default

[Anthony Liguori]

Gerd Hoffmann <kraxel@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

[Peter Maydell]

On 5 November 2013 19:57, Anthony Liguori <anthony@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

[Peter Maydell]

On 5 November 2013 19:57, Anthony Liguori <anthony@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

[Gerd Hoffmann]

On Di, 2013-11-05 at 20:34 +0000, Peter Maydell wrote:
> On 5 November 2013 19:57, Anthony Liguori <anthony@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

[Peter Maydell]

On 6 November 2013 09:18, Gerd Hoffmann <kraxel@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

[Gerd Hoffmann]

On Mi, 2013-11-06 at 10:48 +0000, 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

[Anthony Liguori]

On Wed, Nov 6, 2013 at 3:15 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mi, 2013-11-06 at 10:48 +0000, 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

[Peter Maydell]

On 6 November 2013 14:54, Anthony Liguori <anthony@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

[Gerd Hoffmann]

  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

[Anthony Liguori]

On Wed, Nov 6, 2013 at 7:20 AM, Gerd Hoffmann <kraxel@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]

  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