| Message ID | 1491571236-23476-1-git-send-email-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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
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.
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; }
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(-)