[PATCH] ALSA: pcm_native: remove unused label

[PATCH] ALSA: pcm_native: remove unused label

[Marcin Slusarz]

gcc warns about it:
sound/core/pcm_native.c: In function 'snd_pcm_fasync':
sound/core/pcm_native.c:3262: warning: label 'out' defined but not used

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jaroslav Kysela <perex@perex.cz>
---
 sound/core/pcm_native.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c49b9d9..333cff6 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3259,7 +3259,6 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
 	runtime = substream->runtime;
 
 	err = fasync_helper(fd, file, on, &runtime->fasync);
-out:
 	unlock_kernel();
 	if (err < 0)
 		return err;
-- 
1.5.4.5

Re: [PATCH] ALSA: pcm_native: remove unused label

[Sven Wegener]

On Sun, 3 Aug 2008, Marcin Slusarz wrote:

> gcc warns about it:
> sound/core/pcm_native.c: In function 'snd_pcm_fasync':
> sound/core/pcm_native.c:3262: warning: label 'out' defined but not used
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> ---
>  sound/core/pcm_native.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c49b9d9..333cff6 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3259,7 +3259,6 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
>  	runtime = substream->runtime;
>  
>  	err = fasync_helper(fd, file, on, &runtime->fasync);
> -out:
>  	unlock_kernel();
>  	if (err < 0)
>  		return err;

Uhm, no, there's

snd_assert(substream != NULL, goto out);

one line above your context. The snd_assert() macro will only include the 
second argument with CONFIG_SND_DEBUG, without CONFIG_SND_DEBUG it just 
evaulates the first parameter and triggers the warning.

Sven

Re: [PATCH] ALSA: pcm_native: remove unused label

[Sven Wegener]

On Sun, 3 Aug 2008, Sven Wegener wrote:

> On Sun, 3 Aug 2008, Marcin Slusarz wrote:
> 
> > gcc warns about it:
> > sound/core/pcm_native.c: In function 'snd_pcm_fasync':
> > sound/core/pcm_native.c:3262: warning: label 'out' defined but not used
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Jaroslav Kysela <perex@perex.cz>
> > ---
> >  sound/core/pcm_native.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index c49b9d9..333cff6 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -3259,7 +3259,6 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
> >  	runtime = substream->runtime;
> >  
> >  	err = fasync_helper(fd, file, on, &runtime->fasync);
> > -out:
> >  	unlock_kernel();
> >  	if (err < 0)
> >  		return err;
> 
> Uhm, no, there's
> 
> snd_assert(substream != NULL, goto out);
> 
> one line above your context.

And that is broken. That's not what an assert is for, we access 
substream->runtime a line later and that will blow up. That should be a 
simpe if, instead of an assert.

Sven

Re: [PATCH] ALSA: pcm_native: remove unused label

[Marcin Slusarz]

On Sun, Aug 03, 2008 at 07:33:19PM +0200, Sven Wegener wrote:
> On Sun, 3 Aug 2008, Sven Wegener wrote:
> 
> > On Sun, 3 Aug 2008, Marcin Slusarz wrote:
> > 
> > > gcc warns about it:
> > > sound/core/pcm_native.c: In function 'snd_pcm_fasync':
> > > sound/core/pcm_native.c:3262: warning: label 'out' defined but not used
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Cc: Jaroslav Kysela <perex@perex.cz>
> > > ---
> > >  sound/core/pcm_native.c |    1 -
> > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > > index c49b9d9..333cff6 100644
> > > --- a/sound/core/pcm_native.c
> > > +++ b/sound/core/pcm_native.c
> > > @@ -3259,7 +3259,6 @@ static int snd_pcm_fasync(int fd, struct file * file, int on)
> > >  	runtime = substream->runtime;
> > >  
> > >  	err = fasync_helper(fd, file, on, &runtime->fasync);
> > > -out:
> > >  	unlock_kernel();
> > >  	if (err < 0)
> > >  		return err;
> > 
> > Uhm, no, there's
> > 
> > snd_assert(substream != NULL, goto out);
> > 
> > one line above your context.

Brown paper bag for me, please.
But for my defense, here's the code:

static int snd_pcm_fasync(int fd, struct file * file, int on)
{
	struct snd_pcm_file * pcm_file;
	struct snd_pcm_substream *substream;
	struct snd_pcm_runtime *runtime;
	int err = -ENXIO;

	lock_kernel();
	pcm_file = file->private_data;
	substream = pcm_file->substream;
	snd_assert(substream != NULL, goto out);
	runtime = substream->runtime;

	err = fasync_helper(fd, file, on, &runtime->fasync);
out:
	unlock_kernel();
	if (err < 0)
		return err;
	return 0;
}

It's a bit weird to have constructs like snd_assert which look
like function call, but actually changes program flow...

> And that is broken. That's not what an assert is for, we access 
> substream->runtime a line later and that will blow up. That should be a 
> simpe if, instead of an assert.

I would say this whole snd_assert macro is broken...

$ git grep snd_assert|wc -l
829
$ git grep snd_assert|grep return|wc -l
722
$ git grep snd_assert|grep return|grep NULL|wc -l
381

Marcin