* [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail
2019-06-07 12:23 [PATCH 0/2] pcm_file write file assert case handling Adam Miartus
@ 2019-06-07 12:23 ` Adam Miartus
2019-06-08 18:38 ` Cezary Rojewski
0 siblings, 1 reply; 10+ messages in thread
From: Adam Miartus @ 2019-06-07 12:23 UTC (permalink / raw
To: patch; +Cc: alsa-devel
previously playback could be interrupted by snd_pcm_file_add_frames:
assert(file->wbuf_used_bytes < file->wbuf_size_bytes)
in case snd_pcm_file_write_bytes fails to write full amount of bytes
to file, variable wbuf_used_bytes would not be fully decremented by
requested amount of bytes function was called with
for the assert to trigger, multiple write fails need to happen, so
that wbuf_used_bytes overflows wbuf_size_bytes,
this patch will allow application to report error code to api user
who might have an idea how to recover, before assert is triggered,
also reporting error along with the print out message might give user
a better idea of what is going on, where previously reason for
mentioned assert was not immediately clear
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
---
src/pcm/pcm_file.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index 1ef80b5..a1d15d6 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -381,27 +381,31 @@ static void fixup_wav_header(snd_pcm_t *pcm)
-static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
+/* return error code in case write failed */
+static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
{
snd_pcm_file_t *file = pcm->private_data;
+ snd_pcm_sframes_t err = 0;
assert(bytes <= file->wbuf_used_bytes);
if (file->format == SND_PCM_FILE_FORMAT_WAV &&
!file->wav_header.fmt) {
- if (write_wav_header(pcm) < 0)
- return;
+ err = write_wav_header(pcm);
+ if (err < 0) {
+ SYSERR("%s write failed, file data may be corrupt", file->fname);
+ return err;
+ }
}
while (bytes > 0) {
- snd_pcm_sframes_t err;
size_t n = bytes;
size_t cont = file->wbuf_size_bytes - file->file_ptr_bytes;
if (n > cont)
n = cont;
err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
if (err < 0) {
- SYSERR("write failed");
- break;
+ SYSERR("%s write failed, file data may be corrupt", file->fname);
+ return err;
}
bytes -= err;
file->wbuf_used_bytes -= err;
@@ -412,15 +416,18 @@ static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
if ((snd_pcm_uframes_t)err != n)
break;
}
+
+ return 0;
}
-static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
- const snd_pcm_channel_area_t *areas,
- snd_pcm_uframes_t offset,
- snd_pcm_uframes_t frames)
+static int snd_pcm_file_add_frames(snd_pcm_t *pcm,
+ const snd_pcm_channel_area_t *areas,
+ snd_pcm_uframes_t offset,
+ snd_pcm_uframes_t frames)
{
snd_pcm_file_t *file = pcm->private_data;
while (frames > 0) {
+ int err = 0;
snd_pcm_uframes_t n = frames;
snd_pcm_uframes_t cont = file->wbuf_size - file->appl_ptr;
snd_pcm_uframes_t avail = file->wbuf_size - snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
@@ -437,10 +444,15 @@ static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
if (file->appl_ptr == file->wbuf_size)
file->appl_ptr = 0;
file->wbuf_used_bytes += snd_pcm_frames_to_bytes(pcm, n);
- if (file->wbuf_used_bytes > file->buffer_bytes)
- snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (file->wbuf_used_bytes > file->buffer_bytes) {
+ err = snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (err < 0) {
+ return err;
+ }
+ }
assert(file->wbuf_used_bytes < file->wbuf_size_bytes);
}
+ return 0;
}
static int snd_pcm_file_close(snd_pcm_t *pcm)
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail
2019-06-07 12:23 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
@ 2019-06-08 18:38 ` Cezary Rojewski
2019-06-11 7:52 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
0 siblings, 1 reply; 10+ messages in thread
From: Cezary Rojewski @ 2019-06-08 18:38 UTC (permalink / raw
To: Adam Miartus, patch; +Cc: alsa-devel
On 2019-06-07 14:23, Adam Miartus wrote:
> previously playback could be interrupted by snd_pcm_file_add_frames:
> assert(file->wbuf_used_bytes < file->wbuf_size_bytes)
>
> in case snd_pcm_file_write_bytes fails to write full amount of bytes
> to file, variable wbuf_used_bytes would not be fully decremented by
> requested amount of bytes function was called with
>
> for the assert to trigger, multiple write fails need to happen, so
> that wbuf_used_bytes overflows wbuf_size_bytes,
>
> this patch will allow application to report error code to api user
> who might have an idea how to recover, before assert is triggered,
> also reporting error along with the print out message might give user
> a better idea of what is going on, where previously reason for
> mentioned assert was not immediately clear
>
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> ---
> src/pcm/pcm_file.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
> index 1ef80b5..a1d15d6 100644
> --- a/src/pcm/pcm_file.c
> +++ b/src/pcm/pcm_file.c
> @@ -381,27 +381,31 @@ static void fixup_wav_header(snd_pcm_t *pcm)
>
>
>
> -static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
> +/* return error code in case write failed */
> +static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
> {
> snd_pcm_file_t *file = pcm->private_data;
> + snd_pcm_sframes_t err = 0;
> assert(bytes <= file->wbuf_used_bytes);
>
> if (file->format == SND_PCM_FILE_FORMAT_WAV &&
> !file->wav_header.fmt) {
> - if (write_wav_header(pcm) < 0)
> - return;
> + err = write_wav_header(pcm);
> + if (err < 0) {
> + SYSERR("%s write failed, file data may be corrupt", file->fname);
> + return err;
> + }
write_wav_header already dumps "Write error" on failure. Both messages
are quite similar. Your message dumps filename though, so it's clearly
more descriptive - consider updating write_wav_header SYSERR message all
along while simplifying code here.
> }
>
> while (bytes > 0) {
> - snd_pcm_sframes_t err;
> size_t n = bytes;
> size_t cont = file->wbuf_size_bytes - file->file_ptr_bytes;
> if (n > cont)
> n = cont;
> err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
> if (err < 0) {
> - SYSERR("write failed");
> - break;
> + SYSERR("%s write failed, file data may be corrupt", file->fname);
> + return err;
> }
> bytes -= err;
> file->wbuf_used_bytes -= err;
> @@ -412,15 +416,18 @@ static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
> if ((snd_pcm_uframes_t)err != n)
> break;
> }
> +
> + return 0;
> }
>
> -static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
> - const snd_pcm_channel_area_t *areas,
> - snd_pcm_uframes_t offset,
> - snd_pcm_uframes_t frames)
> +static int snd_pcm_file_add_frames(snd_pcm_t *pcm,
> + const snd_pcm_channel_area_t *areas,
> + snd_pcm_uframes_t offset,
> + snd_pcm_uframes_t frames)
> {
> snd_pcm_file_t *file = pcm->private_data;
> while (frames > 0) {
> + int err = 0;
> snd_pcm_uframes_t n = frames;
> snd_pcm_uframes_t cont = file->wbuf_size - file->appl_ptr;
> snd_pcm_uframes_t avail = file->wbuf_size - snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
> @@ -437,10 +444,15 @@ static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
> if (file->appl_ptr == file->wbuf_size)
> file->appl_ptr = 0;
> file->wbuf_used_bytes += snd_pcm_frames_to_bytes(pcm, n);
> - if (file->wbuf_used_bytes > file->buffer_bytes)
> - snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
> + if (file->wbuf_used_bytes > file->buffer_bytes) {
> + err = snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
> + if (err < 0) {
> + return err;
> + }
Suggestion: drop unnecessary brackets.
> + }
> assert(file->wbuf_used_bytes < file->wbuf_size_bytes);
> }
> + return 0;
Hmm. For snd_pcm_file_write_bytes you've chosen a different form:
newline + return. Code looks more cohesive if it's "formatted" in the
same fashion.
> }
>
> static int snd_pcm_file_close(snd_pcm_t *pcm)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail
2019-06-08 18:38 ` Cezary Rojewski
@ 2019-06-11 7:52 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
2019-06-11 16:03 ` Takashi Iwai
0 siblings, 1 reply; 10+ messages in thread
From: Miartus, Adam (Arion Recruitment; ADITG/ESM) @ 2019-06-11 7:52 UTC (permalink / raw
To: Cezary Rojewski, patch@alsa-project.org; +Cc: alsa-devel@alsa-project.org
> -----Original Message-----
> From: Cezary Rojewski <cezary.rojewski@intel.com>
> Sent: Samstag, 8. Juni 2019 20:38
> To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@de.adit-
> jv.com>; patch@alsa-project.org
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH 1/2] pcm_file: do not disrupt playback on
> output file write fail
>
> On 2019-06-07 14:23, Adam Miartus wrote:
> > previously playback could be interrupted by snd_pcm_file_add_frames:
> > assert(file->wbuf_used_bytes < file->wbuf_size_bytes)
> >
> > in case snd_pcm_file_write_bytes fails to write full amount of bytes
> > to file, variable wbuf_used_bytes would not be fully decremented by
> > requested amount of bytes function was called with
> >
> > for the assert to trigger, multiple write fails need to happen, so
> > that wbuf_used_bytes overflows wbuf_size_bytes,
> >
> > this patch will allow application to report error code to api user
> > who might have an idea how to recover, before assert is triggered,
> > also reporting error along with the print out message might give user
> > a better idea of what is going on, where previously reason for
> > mentioned assert was not immediately clear
> >
> > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > ---
> > src/pcm/pcm_file.c | 36 ++++++++++++++++++++++++------------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
> > index 1ef80b5..a1d15d6 100644
> > --- a/src/pcm/pcm_file.c
> > +++ b/src/pcm/pcm_file.c
> > @@ -381,27 +381,31 @@ static void fixup_wav_header(snd_pcm_t *pcm)
> >
> >
> >
> > -static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
> > +/* return error code in case write failed */
> > +static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
> > {
> > snd_pcm_file_t *file = pcm->private_data;
> > + snd_pcm_sframes_t err = 0;
> > assert(bytes <= file->wbuf_used_bytes);
> >
> > if (file->format == SND_PCM_FILE_FORMAT_WAV &&
> > !file->wav_header.fmt) {
> > - if (write_wav_header(pcm) < 0)
> > - return;
> > + err = write_wav_header(pcm);
> > + if (err < 0) {
> > + SYSERR("%s write failed, file data may be corrupt",
> file->fname);
> > + return err;
> > + }
>
> write_wav_header already dumps "Write error" on failure. Both messages
> are quite similar. Your message dumps filename though, so it's clearly
> more descriptive - consider updating write_wav_header SYSERR message all
> along while simplifying code here.
>
> > }
> >
> > while (bytes > 0) {
> > - snd_pcm_sframes_t err;
> > size_t n = bytes;
> > size_t cont = file->wbuf_size_bytes - file->file_ptr_bytes;
> > if (n > cont)
> > n = cont;
> > err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
> > if (err < 0) {
> > - SYSERR("write failed");
> > - break;
> > + SYSERR("%s write failed, file data may be corrupt",
> file->fname);
> > + return err;
> > }
> > bytes -= err;
> > file->wbuf_used_bytes -= err;
> > @@ -412,15 +416,18 @@ static void snd_pcm_file_write_bytes(snd_pcm_t
> *pcm, size_t bytes)
> > if ((snd_pcm_uframes_t)err != n)
> > break;
> > }
> > +
> > + return 0;
> > }
> >
> > -static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
> > - const snd_pcm_channel_area_t *areas,
> > - snd_pcm_uframes_t offset,
> > - snd_pcm_uframes_t frames)
> > +static int snd_pcm_file_add_frames(snd_pcm_t *pcm,
> > + const snd_pcm_channel_area_t *areas,
> > + snd_pcm_uframes_t offset,
> > + snd_pcm_uframes_t frames)
> > {
> > snd_pcm_file_t *file = pcm->private_data;
> > while (frames > 0) {
> > + int err = 0;
> > snd_pcm_uframes_t n = frames;
> > snd_pcm_uframes_t cont = file->wbuf_size - file->appl_ptr;
> > snd_pcm_uframes_t avail = file->wbuf_size -
> snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
> > @@ -437,10 +444,15 @@ static void snd_pcm_file_add_frames(snd_pcm_t
> *pcm,
> > if (file->appl_ptr == file->wbuf_size)
> > file->appl_ptr = 0;
> > file->wbuf_used_bytes += snd_pcm_frames_to_bytes(pcm,
> n);
> > - if (file->wbuf_used_bytes > file->buffer_bytes)
> > - snd_pcm_file_write_bytes(pcm, file-
> >wbuf_used_bytes - file->buffer_bytes);
> > + if (file->wbuf_used_bytes > file->buffer_bytes) {
> > + err = snd_pcm_file_write_bytes(pcm, file-
> >wbuf_used_bytes - file->buffer_bytes);
> > + if (err < 0) {
> > + return err;
> > + }
>
> Suggestion: drop unnecessary brackets.
>
> > + }
> > assert(file->wbuf_used_bytes < file->wbuf_size_bytes);
> > }
> > + return 0;
>
> Hmm. For snd_pcm_file_write_bytes you've chosen a different form:
> newline + return. Code looks more cohesive if it's "formatted" in the
> same fashion.
>
> > }
> >
> > static int snd_pcm_file_close(snd_pcm_t *pcm)
> >
Thank you for the review, I removed the whitespace, brackets and moved the log to be printed at first error occurrence.
Should I send a new patch request or copy paste here is ok?
>From 5f347b41ed78b5f033c6e5d62ae9bef944dbb1b0 Mon Sep 17 00:00:00 2001
From: Adam Miartus <amiartus@de.adit-jv.com>
Date: Tue, 11 Jun 2019 09:42:21 +0200
Subject: [PATCH] pcm_file: do not disrupt playback on output file write fail
previously playback could be interrupted by snd_pcm_file_add_frames:
assert(file->wbuf_used_bytes < file->wbuf_size_bytes)
in case snd_pcm_file_write_bytes fails to write full amount of bytes
to file, variable wbuf_used_bytes would not be fully decremented by
requested amount of bytes function was called with
for the assert to trigger, multiple write fails need to happen, so
that wbuf_used_bytes overflows wbuf_size_bytes,
this patch will allow application to report error code to api user
who might have an idea how to recover, before assert is triggered,
also reporting error along with the print out message might give user
a better idea of what is going on, where previously reason for
mentioned assert was not immediately clear
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
---
src/pcm/pcm_file.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index 1ef80b5..0a8d98d 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -346,7 +346,7 @@ static int write_wav_header(snd_pcm_t *pcm)
sizeof(file->wav_header) ||
write(file->fd, header2, sizeof(header2)) != sizeof(header2)) {
int err = errno;
- SYSERR("Write error.\n");
+ SYSERR("%s write header failed, file data may be corrupt", file->fname);
return -err;
}
return 0;
@@ -381,27 +381,29 @@ static void fixup_wav_header(snd_pcm_t *pcm)
-static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
+/* return error code in case write failed */
+static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
{
snd_pcm_file_t *file = pcm->private_data;
+ snd_pcm_sframes_t err = 0;
assert(bytes <= file->wbuf_used_bytes);
if (file->format == SND_PCM_FILE_FORMAT_WAV &&
!file->wav_header.fmt) {
- if (write_wav_header(pcm) < 0)
- return;
+ err = write_wav_header(pcm);
+ if (err < 0)
+ return err;
}
while (bytes > 0) {
- snd_pcm_sframes_t err;
size_t n = bytes;
size_t cont = file->wbuf_size_bytes - file->file_ptr_bytes;
if (n > cont)
n = cont;
err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
if (err < 0) {
- SYSERR("write failed");
- break;
+ SYSERR("%s write failed, file data may be corrupt", file->fname);
+ return err;
}
bytes -= err;
file->wbuf_used_bytes -= err;
@@ -412,15 +414,17 @@ static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
if ((snd_pcm_uframes_t)err != n)
break;
}
+ return 0;
}
-static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
- const snd_pcm_channel_area_t *areas,
- snd_pcm_uframes_t offset,
- snd_pcm_uframes_t frames)
+static int snd_pcm_file_add_frames(snd_pcm_t *pcm,
+ const snd_pcm_channel_area_t *areas,
+ snd_pcm_uframes_t offset,
+ snd_pcm_uframes_t frames)
{
snd_pcm_file_t *file = pcm->private_data;
while (frames > 0) {
+ int err = 0;
snd_pcm_uframes_t n = frames;
snd_pcm_uframes_t cont = file->wbuf_size - file->appl_ptr;
snd_pcm_uframes_t avail = file->wbuf_size - snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
@@ -437,10 +441,14 @@ static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
if (file->appl_ptr == file->wbuf_size)
file->appl_ptr = 0;
file->wbuf_used_bytes += snd_pcm_frames_to_bytes(pcm, n);
- if (file->wbuf_used_bytes > file->buffer_bytes)
- snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (file->wbuf_used_bytes > file->buffer_bytes) {
+ err = snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (err < 0)
+ return err;
+ }
assert(file->wbuf_used_bytes < file->wbuf_size_bytes);
}
+ return 0;
}
static int snd_pcm_file_close(snd_pcm_t *pcm)
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail
2019-06-11 7:52 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
@ 2019-06-11 16:03 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-06-11 16:03 UTC (permalink / raw
To: Miartus, Adam (Arion Recruitment; ADITG/ESM)
Cc: Cezary Rojewski, alsa-devel@alsa-project.org
On Tue, 11 Jun 2019 09:52:19 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
>
> Thank you for the review, I removed the whitespace, brackets and moved the log to be printed at first error occurrence.
>
> Should I send a new patch request or copy paste here is ok?
Please resubmit the patchset as v2 later.
But it's better if you guys take internal reviews and get reviewed-by
tag before resubmission.
thanks,
Takashi
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/2] v2 pcm_file propagate write error instead of an assert
@ 2019-06-12 6:48 Adam Miartus
2019-06-12 6:48 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Adam Miartus @ 2019-06-12 6:48 UTC (permalink / raw
To: patch; +Cc: alsa-devel
patch possible case whenre file write causes assert in pcm_file
plugin, by reporting error to user and printing a message
user has a chance to recover error state, or capture
correct logs before assert happens, and may have an idea how
to recover
Adam Miartus (2):
pcm_file: do not disrupt playback on output file write fail
pcm_file: report write output file error to api user
src/pcm/pcm_file.c | 61 ++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 43 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail
2019-06-12 6:48 [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Adam Miartus
@ 2019-06-12 6:48 ` Adam Miartus
2019-06-12 6:48 ` [PATCH 2/2] pcm_file: report write output file error to api user Adam Miartus
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Adam Miartus @ 2019-06-12 6:48 UTC (permalink / raw
To: patch; +Cc: alsa-devel
previously playback could be interrupted by snd_pcm_file_add_frames:
assert(file->wbuf_used_bytes < file->wbuf_size_bytes)
in case snd_pcm_file_write_bytes fails to write full amount of bytes
to file, variable wbuf_used_bytes would not be fully decremented by
requested amount of bytes function was called with
for the assert to trigger, multiple write fails need to happen, so
that wbuf_used_bytes overflows wbuf_size_bytes,
this patch will allow application to report error code to api user
who might have an idea how to recover, before assert is triggered,
also reporting error along with the print out message might give user
a better idea of what is going on, where previously reason for
mentioned assert was not immediately clear
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
---
src/pcm/pcm_file.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index 1ef80b5..0a8d98d 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -346,7 +346,7 @@ static int write_wav_header(snd_pcm_t *pcm)
sizeof(file->wav_header) ||
write(file->fd, header2, sizeof(header2)) != sizeof(header2)) {
int err = errno;
- SYSERR("Write error.\n");
+ SYSERR("%s write header failed, file data may be corrupt", file->fname);
return -err;
}
return 0;
@@ -381,27 +381,29 @@ static void fixup_wav_header(snd_pcm_t *pcm)
-static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
+/* return error code in case write failed */
+static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
{
snd_pcm_file_t *file = pcm->private_data;
+ snd_pcm_sframes_t err = 0;
assert(bytes <= file->wbuf_used_bytes);
if (file->format == SND_PCM_FILE_FORMAT_WAV &&
!file->wav_header.fmt) {
- if (write_wav_header(pcm) < 0)
- return;
+ err = write_wav_header(pcm);
+ if (err < 0)
+ return err;
}
while (bytes > 0) {
- snd_pcm_sframes_t err;
size_t n = bytes;
size_t cont = file->wbuf_size_bytes - file->file_ptr_bytes;
if (n > cont)
n = cont;
err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
if (err < 0) {
- SYSERR("write failed");
- break;
+ SYSERR("%s write failed, file data may be corrupt", file->fname);
+ return err;
}
bytes -= err;
file->wbuf_used_bytes -= err;
@@ -412,15 +414,17 @@ static void snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
if ((snd_pcm_uframes_t)err != n)
break;
}
+ return 0;
}
-static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
- const snd_pcm_channel_area_t *areas,
- snd_pcm_uframes_t offset,
- snd_pcm_uframes_t frames)
+static int snd_pcm_file_add_frames(snd_pcm_t *pcm,
+ const snd_pcm_channel_area_t *areas,
+ snd_pcm_uframes_t offset,
+ snd_pcm_uframes_t frames)
{
snd_pcm_file_t *file = pcm->private_data;
while (frames > 0) {
+ int err = 0;
snd_pcm_uframes_t n = frames;
snd_pcm_uframes_t cont = file->wbuf_size - file->appl_ptr;
snd_pcm_uframes_t avail = file->wbuf_size - snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
@@ -437,10 +441,14 @@ static void snd_pcm_file_add_frames(snd_pcm_t *pcm,
if (file->appl_ptr == file->wbuf_size)
file->appl_ptr = 0;
file->wbuf_used_bytes += snd_pcm_frames_to_bytes(pcm, n);
- if (file->wbuf_used_bytes > file->buffer_bytes)
- snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (file->wbuf_used_bytes > file->buffer_bytes) {
+ err = snd_pcm_file_write_bytes(pcm, file->wbuf_used_bytes - file->buffer_bytes);
+ if (err < 0)
+ return err;
+ }
assert(file->wbuf_used_bytes < file->wbuf_size_bytes);
}
+ return 0;
}
static int snd_pcm_file_close(snd_pcm_t *pcm)
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] pcm_file: report write output file error to api user
2019-06-12 6:48 [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Adam Miartus
2019-06-12 6:48 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
@ 2019-06-12 6:48 ` Adam Miartus
2019-06-15 8:35 ` [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Takashi Iwai
2019-06-18 6:30 ` [ALSA patch] " Takashi Iwai
3 siblings, 0 replies; 10+ messages in thread
From: Adam Miartus @ 2019-06-12 6:48 UTC (permalink / raw
To: patch; +Cc: alsa-devel
when writing to output file fails, api user is notified and can handle
recovery
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
---
src/pcm/pcm_file.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index 0a8d98d..ca8e0c8 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -572,7 +572,10 @@ static snd_pcm_sframes_t snd_pcm_file_writei(snd_pcm_t *pcm, const void *buffer,
if (n > 0) {
snd_pcm_areas_from_buf(pcm, areas, (void*) buffer);
__snd_pcm_lock(pcm);
- snd_pcm_file_add_frames(pcm, areas, 0, n);
+ if (snd_pcm_file_add_frames(pcm, areas, 0, n) < 0) {
+ __snd_pcm_unlock(pcm);
+ return -EPIPE;
+ }
__snd_pcm_unlock(pcm);
}
return n;
@@ -587,7 +590,10 @@ static snd_pcm_sframes_t snd_pcm_file_writen(snd_pcm_t *pcm, void **bufs, snd_pc
if (n > 0) {
snd_pcm_areas_from_bufs(pcm, areas, bufs);
__snd_pcm_lock(pcm);
- snd_pcm_file_add_frames(pcm, areas, 0, n);
+ if (snd_pcm_file_add_frames(pcm, areas, 0, n) < 0) {
+ __snd_pcm_unlock(pcm);
+ return -EPIPE;
+ }
__snd_pcm_unlock(pcm);
}
return n;
@@ -608,6 +614,11 @@ static snd_pcm_sframes_t snd_pcm_file_readi(snd_pcm_t *pcm, void *buffer, snd_pc
snd_pcm_file_areas_read_infile(pcm, areas, 0, frames);
__snd_pcm_lock(pcm);
snd_pcm_file_add_frames(pcm, areas, 0, frames);
+ if (snd_pcm_file_add_frames(pcm, areas, 0, frames) < 0) {
+ __snd_pcm_unlock(pcm);
+ return -EPIPE;
+ }
+
__snd_pcm_unlock(pcm);
return frames;
@@ -627,7 +638,11 @@ static snd_pcm_sframes_t snd_pcm_file_readn(snd_pcm_t *pcm, void **bufs, snd_pcm
snd_pcm_areas_from_bufs(pcm, areas, bufs);
snd_pcm_file_areas_read_infile(pcm, areas, 0, frames);
__snd_pcm_lock(pcm);
- snd_pcm_file_add_frames(pcm, areas, 0, frames);
+ if (snd_pcm_file_add_frames(pcm, areas, 0, frames) < 0) {
+ __snd_pcm_unlock(pcm);
+ return -EPIPE;
+ }
+
__snd_pcm_unlock(pcm);
return frames;
@@ -649,8 +664,10 @@ static snd_pcm_sframes_t snd_pcm_file_mmap_commit(snd_pcm_t *pcm,
if (result >= 0) {
assert(ofs == offset && siz == size);
result = snd_pcm_mmap_commit(file->gen.slave, ofs, siz);
- if (result > 0)
- snd_pcm_file_add_frames(pcm, areas, ofs, result);
+ if (result > 0) {
+ if (snd_pcm_file_add_frames(pcm, areas, ofs, result) < 0)
+ return -EPIPE;
+ }
}
return result;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] v2 pcm_file propagate write error instead of an assert
2019-06-12 6:48 [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Adam Miartus
2019-06-12 6:48 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
2019-06-12 6:48 ` [PATCH 2/2] pcm_file: report write output file error to api user Adam Miartus
@ 2019-06-15 8:35 ` Takashi Iwai
2019-06-18 6:30 ` [ALSA patch] " Takashi Iwai
3 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-06-15 8:35 UTC (permalink / raw
To: Adam Miartus; +Cc: alsa-devel
On Wed, 12 Jun 2019 08:48:26 +0200,
Adam Miartus wrote:
>
> patch possible case whenre file write causes assert in pcm_file
> plugin, by reporting error to user and printing a message
>
> user has a chance to recover error state, or capture
> correct logs before assert happens, and may have an idea how
> to recover
>
> Adam Miartus (2):
> pcm_file: do not disrupt playback on output file write fail
> pcm_file: report write output file error to api user
Applied both patches. Thanks.
Takashi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ALSA patch] [PATCH 0/2] v2 pcm_file propagate write error instead of an assert
2019-06-12 6:48 [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Adam Miartus
` (2 preceding siblings ...)
2019-06-15 8:35 ` [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Takashi Iwai
@ 2019-06-18 6:30 ` Takashi Iwai
2019-06-18 6:49 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
3 siblings, 1 reply; 10+ messages in thread
From: Takashi Iwai @ 2019-06-18 6:30 UTC (permalink / raw
To: Adam Miartus; +Cc: alsa-devel
On Wed, 12 Jun 2019 08:48:26 +0200,
Adam Miartus wrote:
>
> patch possible case whenre file write causes assert in pcm_file
> plugin, by reporting error to user and printing a message
>
> user has a chance to recover error state, or capture
> correct logs before assert happens, and may have an idea how
> to recover
>
> Adam Miartus (2):
> pcm_file: do not disrupt playback on output file write fail
> pcm_file: report write output file error to api user
The patches have been already merged. Any reason for resend?
thanks,
Takashi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ALSA patch] [PATCH 0/2] v2 pcm_file propagate write error instead of an assert
2019-06-18 6:30 ` [ALSA patch] " Takashi Iwai
@ 2019-06-18 6:49 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
0 siblings, 0 replies; 10+ messages in thread
From: Miartus, Adam (Arion Recruitment; ADITG/ESM) @ 2019-06-18 6:49 UTC (permalink / raw
To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Dienstag, 18. Juni 2019 08:31
> To: Miartus, Adam (Arion Recruitment; ADITG/ESM) <amiartus@de.adit-
> jv.com>
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [ALSA patch] [PATCH 0/2] v2 pcm_file propagate write error
> instead of an assert
>
> On Wed, 12 Jun 2019 08:48:26 +0200,
> Adam Miartus wrote:
> >
> > patch possible case whenre file write causes assert in pcm_file
> > plugin, by reporting error to user and printing a message
> >
> > user has a chance to recover error state, or capture
> > correct logs before assert happens, and may have an idea how
> > to recover
> >
> > Adam Miartus (2):
> > pcm_file: do not disrupt playback on output file write fail
> > pcm_file: report write output file error to api user
>
> The patches have been already merged. Any reason for resend?
>
>
> thanks,
>
> Takashi
This is request v2, Im not aware I sent it two times. If so, sorry for spam.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-06-18 6:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-12 6:48 [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Adam Miartus
2019-06-12 6:48 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
2019-06-12 6:48 ` [PATCH 2/2] pcm_file: report write output file error to api user Adam Miartus
2019-06-15 8:35 ` [PATCH 0/2] v2 pcm_file propagate write error instead of an assert Takashi Iwai
2019-06-18 6:30 ` [ALSA patch] " Takashi Iwai
2019-06-18 6:49 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
-- strict thread matches above, loose matches on Subject: below --
2019-06-07 12:23 [PATCH 0/2] pcm_file write file assert case handling Adam Miartus
2019-06-07 12:23 ` [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail Adam Miartus
2019-06-08 18:38 ` Cezary Rojewski
2019-06-11 7:52 ` Miartus, Adam (Arion Recruitment; ADITG/ESM)
2019-06-11 16:03 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.