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

[Markus Armbruster <armbru@redhat.com>]

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);
     }