[09/10] alsabat: use variable for thread return value

Commit Message

[e-mail] March 2, 2016, 8:53 a.m. UTC

From: "Lu, Han" <[e-mail]>

Replace fixed "1" to variable for thread return value.

Signed-off-by: Lu, Han <[e-mail]>

Comments

Takashi Iwai March 11, 2016, 1:34 p.m. UTC | #1

On Wed, 02 Mar 2016 09:53:19 +0100,
[e-mail] wrote:
> 
> From: "Lu, Han" <[e-mail]>
> 
> Replace fixed "1" to variable for thread return value.
> 
> Signed-off-by: Lu, Han <[e-mail]>
> 
> 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...?


Takashi

[e-mail] March 14, 2016, 9:15 a.m. UTC | #2

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Friday, March 11, 2016 9:34 PM
> To: Lu, Han <[e-mail]>
> Cc: [e-mail]; Gautier, Bernard
> <[e-mail]>; Popescu, Edward C
> <[e-mail]>; [e-mail]
> Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for thread
> return value
> 
> On Wed, 02 Mar 2016 09:53:19 +0100,
> [e-mail] wrote:
> >
> > From: "Lu, Han" <[e-mail]>
> >
> > Replace fixed "1" to variable for thread return value.
> >
> > Signed-off-by: Lu, Han <[e-mail]>
> >
> > 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.

BR,
Han

> 
> 
> Takashi

Takashi Iwai March 14, 2016, 9:21 a.m. UTC | #3

On Mon, 14 Mar 2016 10:15:20 +0100,
Lu, Han wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Friday, March 11, 2016 9:34 PM
> > To: Lu, Han <[e-mail]>
> > Cc: [e-mail]; Gautier, Bernard
> > <[e-mail]>; Popescu, Edward C
> > <[e-mail]>; [e-mail]
> > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for thread
> > return value
> > 
> > On Wed, 02 Mar 2016 09:53:19 +0100,
> > [e-mail] wrote:
> > >
> > > From: "Lu, Han" <[e-mail]>
> > >
> > > Replace fixed "1" to variable for thread return value.
> > >
> > > Signed-off-by: Lu, Han <[e-mail]>
> > >
> > > 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

[e-mail] March 14, 2016, 9:36 a.m. UTC | #4

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Monday, March 14, 2016 5:21 PM
> To: Lu, Han <[e-mail]>
> Cc: [e-mail]; Gautier, Bernard
> <[e-mail]>; Popescu, Edward C
> <[e-mail]>; [e-mail]
> 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:[e-mail]]
> > > Sent: Friday, March 11, 2016 9:34 PM
> > > To: Lu, Han <[e-mail]>
> > > Cc: [e-mail]; Gautier, Bernard
> > > <[e-mail]>; Popescu, Edward C
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for
> > > thread return value
> > >
> > > On Wed, 02 Mar 2016 09:53:19 +0100,
> > > [e-mail] wrote:
> > > >
> > > > From: "Lu, Han" <[e-mail]>
> > > >
> > > > Replace fixed "1" to variable for thread return value.
> > > >
> > > > Signed-off-by: Lu, Han <[e-mail]>
> > > >
> > > > 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.

BR,
Han

Takashi Iwai March 14, 2016, 9:43 a.m. UTC | #5

On Mon, 14 Mar 2016 10:36:53 +0100,
Lu, Han wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Monday, March 14, 2016 5:21 PM
> > To: Lu, Han <[e-mail]>
> > Cc: [e-mail]; Gautier, Bernard
> > <[e-mail]>; Popescu, Edward C
> > <[e-mail]>; [e-mail]
> > 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:[e-mail]]
> > > > Sent: Friday, March 11, 2016 9:34 PM
> > > > To: Lu, Han <[e-mail]>
> > > > Cc: [e-mail]; Gautier, Bernard
> > > > <[e-mail]>; Popescu, Edward C
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for
> > > > thread return value
> > > >
> > > > On Wed, 02 Mar 2016 09:53:19 +0100,
> > > > [e-mail] wrote:
> > > > >
> > > > > From: "Lu, Han" <[e-mail]>
> > > > >
> > > > > Replace fixed "1" to variable for thread return value.
> > > > >
> > > > > Signed-off-by: Lu, Han <[e-mail]>
> > > > >
> > > > > 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

[e-mail] March 14, 2016, 9:53 a.m. UTC | #6

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Monday, March 14, 2016 5:44 PM
> To: Lu, Han <[e-mail]>
> Cc: [e-mail]; Gautier, Bernard
> <[e-mail]>; Popescu, Edward C
> <[e-mail]>; [e-mail]
> Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable for thread
> return value
> 
> On Mon, 14 Mar 2016 10:36:53 +0100,
> Lu, Han wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: Monday, March 14, 2016 5:21 PM
> > > To: Lu, Han <[e-mail]>
> > > Cc: [e-mail]; Gautier, Bernard
> > > <[e-mail]>; Popescu, Edward C
> > > <[e-mail]>; [e-mail]
> > > 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:[e-mail]]
> > > > > Sent: Friday, March 11, 2016 9:34 PM
> > > > > To: Lu, Han <[e-mail]>
> > > > > Cc: [e-mail]; Gautier, Bernard
> > > > > <[e-mail]>; Popescu, Edward C
> > > > > <[e-mail]>; [e-mail]
> > > > > Subject: Re: [alsa-devel] [PATCH 09/10] alsabat: use variable
> > > > > for thread return value
> > > > >
> > > > > On Wed, 02 Mar 2016 09:53:19 +0100, [e-mail] wrote:
> > > > > >
> > > > > > From: "Lu, Han" <[e-mail]>
> > > > > >
> > > > > > Replace fixed "1" to variable for thread return value.
> > > > > >
> > > > > > Signed-off-by: Lu, Han <[e-mail]>
> > > > > >
> > > > > > 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

[han] OK. I'd like to send one patchset for 4 features: default name,
standalone, tinyalsa and shell script, and send another patchset for trivial
fixes.

BR,
Han

Patch

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;
 			goto exit3;
 		}
 		/* Skip header */
 		err = read_wav_header(bat, bat->playback.file, bat->fp, true);
 		if (err != 0) {
-			retval_play = 1;
+			retval_play = err;
 			goto exit4;
 		}
 	}
@@ -348,7 +348,7 @@  void *playback_alsa(struct bat *bat)
 
 	err = write_to_pcm_loop(&sndpcm, bat);
 	if (err < 0) {
-		retval_play = 1;
+		retval_play = err;
 		goto exit4;
 	}
 
@@ -471,13 +471,13 @@  void *record_alsa(struct bat *bat)
 	if (err != 0) {
 		fprintf(bat->err, _("Cannot open PCM capture device: "));
 		fprintf(bat->err, _("%s(%d)\n"), snd_strerror(err), err);
-		retval_record = 1;
+		retval_record = err;
 		goto exit1;
 	}
 
 	err = set_snd_pcm_params(bat, &sndpcm);
 	if (err != 0) {
-		retval_record = 1;
+		retval_record = err;
 		goto exit2;
 	}