[Qemu-devel] Questions on audio_atexit(), possibly bugs

[Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

Excuse my ignorance on all things audio, but I stumbled over something
that could be wrong.

audio_vm_change_state_handler() stops voices when the VM stops, and
starts them when it continues.

audio_atexit() unconditionally stops them.  When a stopped VM exits,
this stops voices that are already stopped.

Does the audio driver contract allow stopping a stopped voice?  If yes,
I figured starting a running voice is fine, too.  If no, we have a bug
in audio_atexit().

Why is audio_atexit() needed at all?  Doesn't the OS clean up just fine
all by itself?  If we do need manual cleanup, why do we have to stop
voices before we run fini_out() and fini_in()?


Unrelated, but nearby: audio_vm_change_state_handler() calls the
ctl_out() callback with three arguments:

        hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);

(op is either VOICE_ENABLE or VOICE_DISABLE here), while audio_atexit()
calls it with two:

        hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);

Same for ctl_in().  Doesn't look kosher.  A quick check of oss_ctl_out()
and oss_ctl_in() shows use of three parameters.

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Wed, 30 Sep 2009, Markus Armbruster wrote:

> Excuse my ignorance on all things audio, but I stumbled over something
> that could be wrong.
> 
> audio_vm_change_state_handler() stops voices when the VM stops, and
> starts them when it continues.
> 
> audio_atexit() unconditionally stops them.  When a stopped VM exits,
> this stops voices that are already stopped.
> 
> Does the audio driver contract allow stopping a stopped voice?  If yes,
> I figured starting a running voice is fine, too.  If no, we have a bug
> in audio_atexit().

This should answer the question audio_atexit existed long before vm
change state handlers. Those were actually added to stop the host from
looping the same audio fragment over and over again (can/will happen
with DirectSound, mmapped OSS, fmod too if i'm not mistaken).

> 
> Why is audio_atexit() needed at all?  Doesn't the OS clean up just fine
> all by itself?  If we do need manual cleanup, why do we have to stop
> voices before we run fini_out() and fini_in()?

audio_atexit is needed for WAV, the OS will not write the final
information for us.

> 
> Unrelated, but nearby: audio_vm_change_state_handler() calls the
> ctl_out() callback with three arguments:
> 
>         hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
> 
> (op is either VOICE_ENABLE or VOICE_DISABLE here), while audio_atexit()
> calls it with two:
> 
>         hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
> 
> Same for ctl_in().  Doesn't look kosher.  A quick check of oss_ctl_out()
> and oss_ctl_in() shows use of three parameters.

Yes, not kosher, but harmless, conf.try_poll_out is only applicable to
VOICE_ENABLE and is simply ignored by the handler of VOICE_DISABLE, this
is a vararg function, so it's okay, though i'd probably change this to
avoid further confusion.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Wed, 30 Sep 2009, Markus Armbruster wrote:
>
>> Excuse my ignorance on all things audio, but I stumbled over something
>> that could be wrong.
>> 
>> audio_vm_change_state_handler() stops voices when the VM stops, and
>> starts them when it continues.
>> 
>> audio_atexit() unconditionally stops them.  When a stopped VM exits,
>> this stops voices that are already stopped.
>> 
>> Does the audio driver contract allow stopping a stopped voice?  If yes,
>> I figured starting a running voice is fine, too.  If no, we have a bug
>> in audio_atexit().
>
> This should answer the question audio_atexit existed long before vm
> change state handlers. Those were actually added to stop the host from
> looping the same audio fragment over and over again (can/will happen
> with DirectSound, mmapped OSS, fmod too if i'm not mistaken).

Just to make sure: Does this mean implementations of audio_pcm_ops need
to cope with stopping a stopped voice?

>> Why is audio_atexit() needed at all?  Doesn't the OS clean up just fine
>> all by itself?  If we do need manual cleanup, why do we have to stop
>> voices before we run fini_out() and fini_in()?
>
> audio_atexit is needed for WAV, the OS will not write the final
> information for us.
>
>> 
>> Unrelated, but nearby: audio_vm_change_state_handler() calls the
>> ctl_out() callback with three arguments:
>> 
>>         hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
>> 
>> (op is either VOICE_ENABLE or VOICE_DISABLE here), while audio_atexit()
>> calls it with two:
>> 
>>         hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
>> 
>> Same for ctl_in().  Doesn't look kosher.  A quick check of oss_ctl_out()
>> and oss_ctl_in() shows use of three parameters.
>
> Yes, not kosher, but harmless, conf.try_poll_out is only applicable to
> VOICE_ENABLE and is simply ignored by the handler of VOICE_DISABLE, this
> is a vararg function, so it's okay, though i'd probably change this to
> avoid further confusion.

Appreciated.

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Thu, 1 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
> >
> >> Excuse my ignorance on all things audio, but I stumbled over something
> >> that could be wrong.
> >> 
> >> audio_vm_change_state_handler() stops voices when the VM stops, and
> >> starts them when it continues.
> >> 
> >> audio_atexit() unconditionally stops them.  When a stopped VM exits,
> >> this stops voices that are already stopped.
> >> 
> >> Does the audio driver contract allow stopping a stopped voice?  If yes,
> >> I figured starting a running voice is fine, too.  If no, we have a bug
> >> in audio_atexit().
> >
> > This should answer the question audio_atexit existed long before vm
> > change state handlers. Those were actually added to stop the host from
> > looping the same audio fragment over and over again (can/will happen
> > with DirectSound, mmapped OSS, fmod too if i'm not mistaken).
> 
> Just to make sure: Does this mean implementations of audio_pcm_ops need
> to cope with stopping a stopped voice?

Yes.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Thu, 1 Oct 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
>> >
>> >> Excuse my ignorance on all things audio, but I stumbled over something
>> >> that could be wrong.
>> >> 
>> >> audio_vm_change_state_handler() stops voices when the VM stops, and
>> >> starts them when it continues.
>> >> 
>> >> audio_atexit() unconditionally stops them.  When a stopped VM exits,
>> >> this stops voices that are already stopped.
>> >> 
>> >> Does the audio driver contract allow stopping a stopped voice?  If yes,
>> >> I figured starting a running voice is fine, too.  If no, we have a bug
>> >> in audio_atexit().
>> >
>> > This should answer the question audio_atexit existed long before vm
>> > change state handlers. Those were actually added to stop the host from
>> > looping the same audio fragment over and over again (can/will happen
>> > with DirectSound, mmapped OSS, fmod too if i'm not mistaken).
>> 
>> Just to make sure: Does this mean implementations of audio_pcm_ops need
>> to cope with stopping a stopped voice?
>
> Yes.
>
> [..snip..]

Thanks.  Next question: I'm having difficulties understanding how
HWVoiceIn / HWVoiceOut member enabled works.

AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
to maintain hw->enabled reflecting the state of the voice.  They also
use it to avoid changing the state uselessly.

audio_vm_change_state_handler() uses hw->enabled the same way.  But it
doesn't update it.  Why?  Same for audio_atexit().

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Fri, 2 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Thu, 1 Oct 2009, Markus Armbruster wrote:
> >
> >> malc <av1474@comtv.ru> writes:
> >> 
> >> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
> >> >
> >> >> Excuse my ignorance on all things audio, but I stumbled over something
> >> >> that could be wrong.
> >> >> 
> >> >> audio_vm_change_state_handler() stops voices when the VM stops, and
> >> >> starts them when it continues.
> >> >> 
> >> >> audio_atexit() unconditionally stops them.  When a stopped VM exits,
> >> >> this stops voices that are already stopped.
> >> >> 
> >> >> Does the audio driver contract allow stopping a stopped voice?  If yes,
> >> >> I figured starting a running voice is fine, too.  If no, we have a bug
> >> >> in audio_atexit().
> >> >
> >> > This should answer the question audio_atexit existed long before vm
> >> > change state handlers. Those were actually added to stop the host from
> >> > looping the same audio fragment over and over again (can/will happen
> >> > with DirectSound, mmapped OSS, fmod too if i'm not mistaken).
> >> 
> >> Just to make sure: Does this mean implementations of audio_pcm_ops need
> >> to cope with stopping a stopped voice?
> >
> > Yes.
> >
> > [..snip..]
> 
> Thanks.  Next question: I'm having difficulties understanding how
> HWVoiceIn / HWVoiceOut member enabled works.
> 
> AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
> to maintain hw->enabled reflecting the state of the voice.  They also
> use it to avoid changing the state uselessly.
> 
> audio_vm_change_state_handler() uses hw->enabled the same way.  But it
> doesn't update it.  Why?  Same for audio_atexit().
> 

Because it's more or less a hack, just to make sure host doesn't loop
the sample it has. Cleaner approach would be to have a member named
something like tmporarily_disabled or paused, but that's an overkill.

After some thinking i believe not calling VOICE_DISABLE in atexit is
possible given that s->vm_running is zero.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Fri, 2 Oct 2009, Markus Armbruster wrote:
[...]
>> Thanks.  Next question: I'm having difficulties understanding how
>> HWVoiceIn / HWVoiceOut member enabled works.
>> 
>> AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
>> to maintain hw->enabled reflecting the state of the voice.  They also
>> use it to avoid changing the state uselessly.
>> 
>> audio_vm_change_state_handler() uses hw->enabled the same way.  But it
>> doesn't update it.  Why?  Same for audio_atexit().
>> 
>
> Because it's more or less a hack, just to make sure host doesn't loop
> the sample it has. Cleaner approach would be to have a member named
> something like tmporarily_disabled or paused, but that's an overkill.

I think I understand why we disable voices on stop.  My question is why
we don't record the fact in hw->enabled.  Care to explain?

> After some thinking i believe not calling VOICE_DISABLE in atexit is
> possible given that s->vm_running is zero.

Is it?  We call exit() in many, many places...

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Sat, 3 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Fri, 2 Oct 2009, Markus Armbruster wrote:
> [...]
> >> Thanks.  Next question: I'm having difficulties understanding how
> >> HWVoiceIn / HWVoiceOut member enabled works.
> >> 
> >> AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
> >> to maintain hw->enabled reflecting the state of the voice.  They also
> >> use it to avoid changing the state uselessly.
> >> 
> >> audio_vm_change_state_handler() uses hw->enabled the same way.  But it
> >> doesn't update it.  Why?  Same for audio_atexit().
> >> 
> >
> > Because it's more or less a hack, just to make sure host doesn't loop
> > the sample it has. Cleaner approach would be to have a member named
> > something like tmporarily_disabled or paused, but that's an overkill.
> 
> I think I understand why we disable voices on stop.  My question is why
> we don't record the fact in hw->enabled.  Care to explain?

Because of overloaded meaning, here we are only pausing, what enabled
means in other contexts is that all the soft voices are gone or inactive,
so the host should stop.

> > After some thinking i believe not calling VOICE_DISABLE in atexit is
> > possible given that s->vm_running is zero.
> 
> Is it?  We call exit() in many, many places...

Yes. The logic goes like this, vm_stop can be zero only if we saw
vm_change_state_handler going from running to stopped and consequently all 
the voices were already stopped/paused - no need to do it at exit.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Sat, 3 Oct 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Fri, 2 Oct 2009, Markus Armbruster wrote:
>> [...]
>> >> Thanks.  Next question: I'm having difficulties understanding how
>> >> HWVoiceIn / HWVoiceOut member enabled works.
>> >> 
>> >> AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
>> >> to maintain hw->enabled reflecting the state of the voice.  They also
>> >> use it to avoid changing the state uselessly.
>> >> 
>> >> audio_vm_change_state_handler() uses hw->enabled the same way.  But it
>> >> doesn't update it.  Why?  Same for audio_atexit().
>> >> 
>> >
>> > Because it's more or less a hack, just to make sure host doesn't loop
>> > the sample it has. Cleaner approach would be to have a member named
>> > something like tmporarily_disabled or paused, but that's an overkill.
>> 
>> I think I understand why we disable voices on stop.  My question is why
>> we don't record the fact in hw->enabled.  Care to explain?
>
> Because of overloaded meaning, here we are only pausing, what enabled
> means in other contexts is that all the soft voices are gone or inactive,
> so the host should stop.
>
>> > After some thinking i believe not calling VOICE_DISABLE in atexit is
>> > possible given that s->vm_running is zero.
>> 
>> Is it?  We call exit() in many, many places...
>
> Yes. The logic goes like this, vm_stop can be zero only if we saw
> vm_change_state_handler going from running to stopped and consequently all 
> the voices were already stopped/paused - no need to do it at exit.

I fear I don't understand.  vm_stop is a function, and as such can't "be
zero".

Consider monitor command "quit".  All it does is exit(0).  I don't see
it stopping the VM.

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Sat, 3 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Sat, 3 Oct 2009, Markus Armbruster wrote:
> >
> >> malc <av1474@comtv.ru> writes:
> >> 
> >> > On Fri, 2 Oct 2009, Markus Armbruster wrote:
> >> [...]
> >> >> Thanks.  Next question: I'm having difficulties understanding how
> >> >> HWVoiceIn / HWVoiceOut member enabled works.
> >> >> 
> >> >> AUD_set_active_out(), AUD_set_active_in() and audio_run_out() take care
> >> >> to maintain hw->enabled reflecting the state of the voice.  They also
> >> >> use it to avoid changing the state uselessly.
> >> >> 
> >> >> audio_vm_change_state_handler() uses hw->enabled the same way.  But it
> >> >> doesn't update it.  Why?  Same for audio_atexit().
> >> >> 
> >> >
> >> > Because it's more or less a hack, just to make sure host doesn't loop
> >> > the sample it has. Cleaner approach would be to have a member named
> >> > something like tmporarily_disabled or paused, but that's an overkill.
> >> 
> >> I think I understand why we disable voices on stop.  My question is why
> >> we don't record the fact in hw->enabled.  Care to explain?
> >
> > Because of overloaded meaning, here we are only pausing, what enabled
> > means in other contexts is that all the soft voices are gone or inactive,
> > so the host should stop.
> >
> >> > After some thinking i believe not calling VOICE_DISABLE in atexit is
> >> > possible given that s->vm_running is zero.
> >> 
> >> Is it?  We call exit() in many, many places...
> >
> > Yes. The logic goes like this, vm_stop can be zero only if we saw
> > vm_change_state_handler going from running to stopped and consequently all 
> > the voices were already stopped/paused - no need to do it at exit.
> 
> I fear I don't understand.  vm_stop is a function, and as such can't "be
> zero".

My bad, i meant AudioState's vm_running field.

> 
> Consider monitor command "quit".  All it does is exit(0).  I don't see
> it stopping the VM.

Yes, vm_running would still be 1 (audio's vm_change_state_handler didn't
run, so we will proceed with disabling). 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Thu, 1 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
> >

[..snip..]

> >> 
> >> Unrelated, but nearby: audio_vm_change_state_handler() calls the
> >> ctl_out() callback with three arguments:
> >> 
> >>         hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
> >> 
> >> (op is either VOICE_ENABLE or VOICE_DISABLE here), while audio_atexit()
> >> calls it with two:
> >> 
> >>         hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
> >> 
> >> Same for ctl_in().  Doesn't look kosher.  A quick check of oss_ctl_out()
> >> and oss_ctl_in() shows use of three parameters.
> >
> > Yes, not kosher, but harmless, conf.try_poll_out is only applicable to
> > VOICE_ENABLE and is simply ignored by the handler of VOICE_DISABLE, this
> > is a vararg function, so it's okay, though i'd probably change this to
> > avoid further confusion.
> 
> Appreciated.
> 

Just coded it and not sure whether it's worth it, what say you?

 audio.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 80a717b..977261c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1739,15 +1739,24 @@ static void audio_vm_change_state_handler (void *opaque, int running,
     AudioState *s = opaque;
     HWVoiceOut *hwo = NULL;
     HWVoiceIn *hwi = NULL;
-    int op = running ? VOICE_ENABLE : VOICE_DISABLE;
 
     s->vm_running = running;
     while ((hwo = audio_pcm_hw_find_any_enabled_out (hwo))) {
-        hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
+        if (running) {
+            hwo->pcm_ops->ctl_out (hwo, VOICE_ENABLE, conf.try_poll_out);
+        }
+        else {
+            hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
+        }
     }
 
     while ((hwi = audio_pcm_hw_find_any_enabled_in (hwi))) {
-        hwi->pcm_ops->ctl_in (hwi, op, conf.try_poll_in);
+        if (running) {
+            hwi->pcm_ops->ctl_in (hwi, VOICE_ENABLE, conf.try_poll_in);
+        }
+        else {
+            hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
+        }
     }
     audio_reset_timer ();
 }

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Thu, 1 Oct 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
>> >
>
> [..snip..]
>
>> >> 
>> >> Unrelated, but nearby: audio_vm_change_state_handler() calls the
>> >> ctl_out() callback with three arguments:
>> >> 
>> >>         hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
>> >> 
>> >> (op is either VOICE_ENABLE or VOICE_DISABLE here), while audio_atexit()
>> >> calls it with two:
>> >> 
>> >>         hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
>> >> 
>> >> Same for ctl_in().  Doesn't look kosher.  A quick check of oss_ctl_out()
>> >> and oss_ctl_in() shows use of three parameters.
>> >
>> > Yes, not kosher, but harmless, conf.try_poll_out is only applicable to
>> > VOICE_ENABLE and is simply ignored by the handler of VOICE_DISABLE, this
>> > is a vararg function, so it's okay, though i'd probably change this to
>> > avoid further confusion.
>> 
>> Appreciated.
>> 
>
> Just coded it and not sure whether it's worth it, what say you?
>
>  audio.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 80a717b..977261c 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1739,15 +1739,24 @@ static void audio_vm_change_state_handler (void *opaque, int running,
>      AudioState *s = opaque;
>      HWVoiceOut *hwo = NULL;
>      HWVoiceIn *hwi = NULL;
> -    int op = running ? VOICE_ENABLE : VOICE_DISABLE;
>  
>      s->vm_running = running;
>      while ((hwo = audio_pcm_hw_find_any_enabled_out (hwo))) {
> -        hwo->pcm_ops->ctl_out (hwo, op, conf.try_poll_out);
> +        if (running) {
> +            hwo->pcm_ops->ctl_out (hwo, VOICE_ENABLE, conf.try_poll_out);
> +        }
> +        else {
> +            hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
> +        }
>      }
>  
>      while ((hwi = audio_pcm_hw_find_any_enabled_in (hwi))) {
> -        hwi->pcm_ops->ctl_in (hwi, op, conf.try_poll_in);
> +        if (running) {
> +            hwi->pcm_ops->ctl_in (hwi, VOICE_ENABLE, conf.try_poll_in);
> +        }
> +        else {
> +            hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
> +        }
>      }
>      audio_reset_timer ();
>  }

The ctl_out() and ctl_in() callbacks I checked (ALSA, OSS), get the
variable argument unconditionally.

Getting more variable arguments than the caller passed is undefined
behavior (C99 7.15.1.1), although in practice is usually gets some
unpredictable value, which is harmless as long as it's not actually
used.

Not getting all variable arguments the caller passed is fine.

So, the calls you fixed up weren't broken in the first place, and the
change isn't worth the trouble, in my opinion.

However, there are calls with undefined behavior, because they pass two
arguments (two fixed, zero variable), and at least some callees use
three (two fixed, one variable).  Fix for that below.


diff --git a/audio/audio.c b/audio/audio.c
index 80a717b..874933c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1243,7 +1243,7 @@ void AUD_set_active_in (SWVoiceIn *sw, int on)
 
                 if (nb_active == 1) {
                     hw->enabled = 0;
-                    hw->pcm_ops->ctl_in (hw, VOICE_DISABLE);
+                    hw->pcm_ops->ctl_in (hw, VOICE_DISABLE, 0);
                 }
             }
         }
@@ -1363,7 +1363,7 @@ static void audio_run_out (AudioState *s)
 #endif
             hw->enabled = 0;
             hw->pending_disable = 0;
-            hw->pcm_ops->ctl_out (hw, VOICE_DISABLE);
+            hw->pcm_ops->ctl_out (hw, VOICE_DISABLE, 0);
             for (sc = hw->cap_head.lh_first; sc; sc = sc->entries.le_next) {
                 sc->sw.active = 0;
                 audio_recalc_and_notify_capture (sc->cap);
@@ -1761,7 +1761,7 @@ static void audio_atexit (void)
     while ((hwo = audio_pcm_hw_find_any_enabled_out (hwo))) {
         SWVoiceCap *sc;
 
-        hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
+        hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE, 0);
         hwo->pcm_ops->fini_out (hwo);
 
         for (sc = hwo->cap_head.lh_first; sc; sc = sc->entries.le_next) {
@@ -1775,7 +1775,7 @@ static void audio_atexit (void)
     }
 
     while ((hwi = audio_pcm_hw_find_any_enabled_in (hwi))) {
-        hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
+        hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE, 0);
         hwi->pcm_ops->fini_in (hwi);
     }
 

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Thu, 1 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Thu, 1 Oct 2009, Markus Armbruster wrote:
> >
> >> malc <av1474@comtv.ru> writes:
> >> 
> >> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
> >> >
> >
> > [..snip..]
> >
[..snip..]

> 
> The ctl_out() and ctl_in() callbacks I checked (ALSA, OSS), get the
> variable argument unconditionally.
> 
> Getting more variable arguments than the caller passed is undefined
> behavior (C99 7.15.1.1), although in practice is usually gets some
> unpredictable value, which is harmless as long as it's not actually
> used.

I wasn't aware of that, thank you for pointing that out.

> 
> Not getting all variable arguments the caller passed is fine.
> 
> So, the calls you fixed up weren't broken in the first place, and the
> change isn't worth the trouble, in my opinion.
> 
> However, there are calls with undefined behavior, because they pass two
> arguments (two fixed, zero variable), and at least some callees use
> three (two fixed, one variable).  Fix for that below.
> 

I'd much rather fix oss and alsa, they should look for poll_mode only in
case of enable, one more time thanks for heads up.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Thu, 1 Oct 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Thu, 1 Oct 2009, Markus Armbruster wrote:
>> >
>> >> malc <av1474@comtv.ru> writes:
>> >> 
>> >> > On Wed, 30 Sep 2009, Markus Armbruster wrote:
>> >> >
>> >
>> > [..snip..]
>> >
> [..snip..]
>
>> 
>> The ctl_out() and ctl_in() callbacks I checked (ALSA, OSS), get the
>> variable argument unconditionally.
>> 
>> Getting more variable arguments than the caller passed is undefined
>> behavior (C99 7.15.1.1), although in practice is usually gets some
>> unpredictable value, which is harmless as long as it's not actually
>> used.
>
> I wasn't aware of that, thank you for pointing that out.
>
>> 
>> Not getting all variable arguments the caller passed is fine.
>> 
>> So, the calls you fixed up weren't broken in the first place, and the
>> change isn't worth the trouble, in my opinion.
>> 
>> However, there are calls with undefined behavior, because they pass two
>> arguments (two fixed, zero variable), and at least some callees use
>> three (two fixed, one variable).  Fix for that below.
>> 
>
> I'd much rather fix oss and alsa, they should look for poll_mode only in
> case of enable, one more time thanks for heads up.

Makes sense.

Better check the other drivers as well, because I didn't.

Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs

[malc]

On Fri, 2 Oct 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 

[..snip..]

> 
> Makes sense.
>
> Better check the other drivers as well, because I didn't.

Nothing else implements polled mode.

And one more thing audio_at_exit tears down captures, those might need
some finalizations of their own, much like WAV does (in fact apart
from VNC audio the only other capture that exits now is also a WAV
renderer, which can be started/stopped from the monitor)

-- 
mailto:av1474@comtv.ru