[alsa-devel] [PATCH 09/10] alsabat: use variable for thread return value

[Takashi Iwai]

On Mon, 14 Mar 2016 10:36:53 +0100,
Lu, Han wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Monday, March 14, 2016 5:21 PM
> > To: Lu, Han <han.lu at intel.com>
> > Cc: liam.r.girdwood at linux.intel.com; Gautier, Bernard
> > <bernard.gautier at intel.com>; Popescu, Edward C
> > <edward.c.popescu at intel.com>; alsa-devel at alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for thread
> > return value
> > 
> > On Mon, 14 Mar 2016 10:15:20 +0100,
> > Lu, Han wrote:
> > >
> > > Hi Takashi,
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > > Sent: Friday, March 11, 2016 9:34 PM
> > > > To: Lu, Han <han.lu at intel.com>
> > > > Cc: liam.r.girdwood at linux.intel.com; Gautier, Bernard
> > > > <bernard.gautier at intel.com>; Popescu, Edward C
> > > > <edward.c.popescu at intel.com>; alsa-devel at alsa-project.org
> > > > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for
> > > > thread return value
> > > >
> > > > On Wed, 02 Mar 2016 09:53:19 +0100,
> > > > han.lu at intel.com wrote:
> > > > >
> > > > > From: "Lu, Han" <han.lu at intel.com>
> > > > >
> > > > > Replace fixed "1" to variable for thread return value.
> > > > >
> > > > > Signed-off-by: Lu, Han <han.lu at intel.com>
> > > > >
> > > > > diff --git a/bat/alsa.c b/bat/alsa.c index 0a5f899..189b0e9 100644
> > > > > --- a/bat/alsa.c
> > > > > +++ b/bat/alsa.c
> > > > > @@ -309,13 +309,13 @@ void *playback_alsa(struct bat *bat)
> > > > >  	if (err != 0) {
> > > > >  		fprintf(bat->err, _("Cannot open PCM playback device: "));
> > > > >  		fprintf(bat->err, _("%s(%d)\n"), snd_strerror(err), err);
> > > > > -		retval_play = 1;
> > > > > +		retval_play = err;
> > > > >  		goto exit1;
> > > > >  	}
> > > > >
> > > > >  	err = set_snd_pcm_params(bat, &sndpcm);
> > > > >  	if (err != 0) {
> > > > > -		retval_play = 1;
> > > > > +		retval_play = err;
> > > > >  		goto exit2;
> > > > >  	}
> > > > >
> > > > > @@ -332,13 +332,13 @@ void *playback_alsa(struct bat *bat)
> > > > >  			fprintf(bat->err, _("Cannot open file for playback: "));
> > > > >  			fprintf(bat->err, _("%s %d\n"),
> > > > >  					bat->playback.file, -errno);
> > > > > -			retval_play = 1;
> > > > > +			retval_play = -errno;
> > > >
> > > > Is the original errno still preserved at this point...?
> > >
> > > [han] I think so, the complete code section here is
> > > 	...
> > > 	bat->fp = fopen(bat->playback.file, "rb");
> > > 	if (bat->fp == NULL) {
> > > 		fprintf(bat->err, _("Cannot open file for playback: "));
> > > 		fprintf(bat->err, _("%s %d\n"),
> > > 				bat->playback.file, -errno);
> > > -		retval_play = 1;
> > > +		retval_play = -errno;
> > > 		goto exit3;
> > > 	}
> > > 	...
> > > So the use of errno should be safe.
> > 
> > You call a bunch of functions between the fopen() error and the reference of
> > errno.  The errno reference should be done immediately after the error.
> > 
> > 
> > Takashi
> 
> [han] sorry! I ignored the errno could be changed by the printf(). I'll rewrite the
> patch and fix other similar errors.

While we're at it: at the next respin, could you split the patchset,
one for trivial fixes that can be applied immediately, and another
patchset for extending the feature?  Mixing up both fixes and
enhancements made it difficult to pick up each patch.  In that way,
I'll be able to apply the fix patches more quickly while reviewing the
enhancement patches more intensively.


thanks,

Takashi