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

[Marcin Slusarz <marcin.slusarz@gmail.com>]

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