[alsa-devel] [PATCH alsa-lib 0/1] bug fix and comment on "switch"
Hi,
this is a very simple one line patch but the issue was driving me crazy for few days.
After this finding, I have reviewed all the "switch" statements in alsa-lib. All well done with proper comment when a "case" falls through the next "case".
But there is one "switch" that does not follow the usual scheme. In file src/pcm/pcm_dsnoop.c procedure snd_pcm_dsnoop_delay() following code fragment:
switch(dsnoop->state) { case SNDRV_PCM_STATE_DRAINING: case SNDRV_PCM_STATE_RUNNING: err = snd_pcm_dsnoop_sync_ptr(pcm); if (err < 0) return err; case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_SUSPENDED: *delayp = snd_pcm_mmap_capture_hw_avail(pcm); return 0;
If snd_pcm_dsnoop_sync_ptr() returns no error, execution falls through next "case". It's missing the usual comment /* Fall through */ to highlight such code behavior. I think the code is correct, but I do not have enough experience on alsa-lib to be sure. Please confirm. In such case, I would suggest adding the comment above.
Best Regards, Antonio Borneo
A missing "break" in procedure snd_pcm_write_mmap() causes execution of "case SND_PCM_ACCESS_MMAP_NONINTERLEAVED" to fall through next "default" case of the "switch" statement. Since "default" handles error cases, the procedure returns error.
The error fixed by this patch blocks transfer of capture data from kernel to application. Execution get stuck in alsa-lib, that discards all received data.
Signed-off-by: Antonio Borneo borneo.antonio@gmail.com --- src/pcm/pcm_mmap.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/pcm/pcm_mmap.c b/src/pcm/pcm_mmap.c index 6b44050..83e74e5 100644 --- a/src/pcm/pcm_mmap.c +++ b/src/pcm/pcm_mmap.c @@ -622,6 +622,7 @@ snd_pcm_sframes_t snd_pcm_read_mmap(snd_pcm_t *pcm, snd_pcm_uframes_t offset, err = _snd_pcm_readn(pcm->fast_op_arg, bufs, frames); if (err >= 0) frames = err; + break; } default: SNDMSG("invalid access type %d", pcm->access);
At Sun, 13 May 2012 00:06:22 +0800, Antonio Borneo wrote:
Hi,
this is a very simple one line patch but the issue was driving me crazy for few days.
After this finding, I have reviewed all the "switch" statements in alsa-lib. All well done with proper comment when a "case" falls through the next "case".
Thanks, applied the patch now.
But there is one "switch" that does not follow the usual scheme. In file src/pcm/pcm_dsnoop.c procedure snd_pcm_dsnoop_delay() following code fragment:
switch(dsnoop->state) { case SNDRV_PCM_STATE_DRAINING: case SNDRV_PCM_STATE_RUNNING: err = snd_pcm_dsnoop_sync_ptr(pcm); if (err < 0) return err; case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_SUSPENDED: *delayp = snd_pcm_mmap_capture_hw_avail(pcm); return 0;If snd_pcm_dsnoop_sync_ptr() returns no error, execution falls through next "case". It's missing the usual comment /* Fall through */ to highlight such code behavior. I think the code is correct, but I do not have enough experience on alsa-lib to be sure. Please confirm. In such case, I would suggest adding the comment above.
Yes, the comment should be added. Care to send a patch?
Takashi
On Mon, May 14, 2012 at 10:58 PM, Takashi Iwai tiwai@suse.de wrote:
At Sun, 13 May 2012 00:06:22 +0800, Antonio Borneo wrote:
Hi,
this is a very simple one line patch but the issue was driving me crazy for few days.
After this finding, I have reviewed all the "switch" statements in alsa-lib. All well done with proper comment when a "case" falls through the next "case".
Thanks, applied the patch now.
Thanks!
But there is one "switch" that does not follow the usual scheme. In file src/pcm/pcm_dsnoop.c procedure snd_pcm_dsnoop_delay() following code fragment:
switch(dsnoop->state) { case SNDRV_PCM_STATE_DRAINING: case SNDRV_PCM_STATE_RUNNING: err = snd_pcm_dsnoop_sync_ptr(pcm); if (err < 0) return err; case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_SUSPENDED: *delayp = snd_pcm_mmap_capture_hw_avail(pcm); return 0;
If snd_pcm_dsnoop_sync_ptr() returns no error, execution falls through next "case". It's missing the usual comment /* Fall through */ to highlight such code behavior. I think the code is correct, but I do not have enough experience on alsa-lib to be sure. Please confirm. In such case, I would suggest adding the comment above.
Yes, the comment should be added. Care to send a patch?
I'll send it shortly.
Best Regards Antonio Borneo
participants (2)
-
Antonio Borneo -
Takashi Iwai