From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MtNU2-0007X9-Dj for qemu-devel@nongnu.org; Thu, 01 Oct 2009 11:22:18 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MtNTx-0007U9-Cq for qemu-devel@nongnu.org; Thu, 01 Oct 2009 11:22:17 -0400 Received: from [199.232.76.173] (port=59514 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MtNTx-0007U1-5v for qemu-devel@nongnu.org; Thu, 01 Oct 2009 11:22:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15652) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MtNTw-0006sP-On for qemu-devel@nongnu.org; Thu, 01 Oct 2009 11:22:13 -0400 Subject: Re: [Qemu-devel] Questions on audio_atexit(), possibly bugs References: <87y6nw9koq.fsf@pike.pond.sub.org> <871vlo9gz5.fsf@pike.pond.sub.org> From: Markus Armbruster Date: Thu, 01 Oct 2009 17:21:56 +0200 In-Reply-To: (malc's message of "Thu\, 1 Oct 2009 03\:28\:23 +0400 \(MSD\)") Message-ID: <87ws3f3zsb.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: malc Cc: qemu-devel@nongnu.org malc writes: > On Thu, 1 Oct 2009, Markus Armbruster wrote: > >> malc 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); }