[PATCH v2] aplay: Fix error message when writing captured data

[PATCH v2] aplay: Fix error message when writing captured data

[Daniel Baluta]

Read can return less then requested bytes, but we treat
this as an error.

Anyhow, errno is not updated in this case and we can end
up with a confusing error message.

For example, when there is no room to write data into the
output file we receive:

$ arecord -d 2000 -c 2 -r 192000 -f S16_LE -Dplughw:0,0 audio.wav
Recording WAVE '/mnt/msc/audio.wav' : Signed 16 bit Little Endian, Rate
192000 Hz, Stereo
audio.wav: No such file or directory

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 aplay/aplay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/aplay/aplay.c b/aplay/aplay.c
index ee480f2..96f5e1b 100644
--- a/aplay/aplay.c
+++ b/aplay/aplay.c
@@ -3079,7 +3079,7 @@ static void capture(char *orig_name)
 				break;
 			}
 			if (write(fd, audiobuf, c) != c) {
-				perror(name);
+				fprintf(stderr, _("Couldn't write all data to %s\n"), name);
 				in_aborting = 1;
 				break;
 			}
-- 
2.7.4

Re: [PATCH v2] aplay: Fix error message when writing captured data

[Takashi Iwai]

On Fri, 07 Apr 2017 15:20:36 +0200,
Daniel Baluta wrote:
> 
> Read can return less then requested bytes, but we treat
> this as an error.

Actually that's the bug -- we should loop the write until it reaches
to the real error.  Once when we get the proper errno, the error
message via perror() itself will be enough.


thanks,

Takashi

> Anyhow, errno is not updated in this case and we can end
> up with a confusing error message.
> 
> For example, when there is no room to write data into the
> output file we receive:
> 
> $ arecord -d 2000 -c 2 -r 192000 -f S16_LE -Dplughw:0,0 audio.wav
> Recording WAVE '/mnt/msc/audio.wav' : Signed 16 bit Little Endian, Rate
> 192000 Hz, Stereo
> audio.wav: No such file or directory
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  aplay/aplay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index ee480f2..96f5e1b 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -3079,7 +3079,7 @@ static void capture(char *orig_name)
>  				break;
>  			}
>  			if (write(fd, audiobuf, c) != c) {
> -				perror(name);
> +				fprintf(stderr, _("Couldn't write all data to %s\n"), name);
>  				in_aborting = 1;
>  				break;
>  			}
> -- 
> 2.7.4
> 

Re: [PATCH v2] aplay: Fix error message when writing captured data

[Daniel Baluta]

On Fri, Apr 7, 2017 at 4:27 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 07 Apr 2017 15:20:36 +0200,
> Daniel Baluta wrote:
>>
>> Read can return less then requested bytes, but we treat
>> this as an error.
>
s/read/write here :)).

> Actually that's the bug -- we should loop the write until it reaches
> to the real error.  Once when we get the proper errno, the error
> message via perror() itself will be enough.

Correct. But I think aplay decided to keep things simple and fail in case
couldn't write all requested bytes.

This pattern can be noticed all over the places where write is used.

Given the fact that write is blocking (meaning that it must write
at least one byte until return) this should work most of the time,
excepting corner cases like disk full, etc.

thanks,
Daniel.

Re: [PATCH v2] aplay: Fix error message when writing captured data

[Takashi Iwai]

On Fri, 07 Apr 2017 15:45:42 +0200,
Daniel Baluta wrote:
> 
> On Fri, Apr 7, 2017 at 4:27 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 07 Apr 2017 15:20:36 +0200,
> > Daniel Baluta wrote:
> >>
> >> Read can return less then requested bytes, but we treat
> >> this as an error.
> >
> s/read/write here :)).
> 
> > Actually that's the bug -- we should loop the write until it reaches
> > to the real error.  Once when we get the proper errno, the error
> > message via perror() itself will be enough.
> 
> Correct. But I think aplay decided to keep things simple and fail in case
> couldn't write all requested bytes.

And that's wrong.  We should loop instead.

> This pattern can be noticed all over the places where write is used.

All wrong :)

We can implement a simple helper function and replace each write call
with it.

> Given the fact that write is blocking (meaning that it must write
> at least one byte until return) this should work most of the time,
> excepting corner cases like disk full, etc.

Yes, and such corner cases must be handled properly, too -- in the
end, it's what you wanted to achieve by your patch, but into a
different direction.


Takashi

Re: [PATCH v2] aplay: Fix error message when writing captured data

[Daniel Baluta]

On Fri, Apr 7, 2017 at 5:30 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 07 Apr 2017 15:45:42 +0200,
> Daniel Baluta wrote:
>>
>> On Fri, Apr 7, 2017 at 4:27 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Fri, 07 Apr 2017 15:20:36 +0200,
>> > Daniel Baluta wrote:
>> >>
>> >> Read can return less then requested bytes, but we treat
>> >> this as an error.
>> >
>> s/read/write here :)).
>>
>> > Actually that's the bug -- we should loop the write until it reaches
>> > to the real error.  Once when we get the proper errno, the error
>> > message via perror() itself will be enough.
>>
>> Correct. But I think aplay decided to keep things simple and fail in case
>> couldn't write all requested bytes.
>
> And that's wrong.  We should loop instead.
>
>> This pattern can be noticed all over the places where write is used.
>
> All wrong :)
>
> We can implement a simple helper function and replace each write call
> with it.
>
>> Given the fact that write is blocking (meaning that it must write
>> at least one byte until return) this should work most of the time,
>> excepting corner cases like disk full, etc.
>
> Yes, and such corner cases must be handled properly, too -- in the
> end, it's what you wanted to achieve by your patch, but into a
> different direction.

:), all right. Will fix this the correct way and resend.

thanks,
Daniel.