Questions on audio_atexit(), possibly bugs

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.

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.

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.

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)