[PATCH 0/3] pcm_file report EIO when fail to write to debug file

[PATCH 0/3] pcm_file report EIO when fail to write to debug file

[Adam Miartus]

Previously, I introduced a patch that may return EPIPE error when pcm_file
plugin fails to write to output file for some reason.

The purpose of reporting the error was to evade assert in code to allow
API user error handling and recovery, or more clarity in debugging.

Failing to write to a file usually means bad file descriptor, running
out of free memory, insufficient file permissions or in case file
is a pipe there may be other reasons preventing the write operation.
All these are not easily recoverable by restaring playback as EPIPE
error which is used to signal XRUN might suggest. Therefore, use EIO
to signal a different kind of error.

write_wav_header function is refactored a bit for more detailed
error handling in case of failed write operation.

Adam Miartus (3):
  pcm_file: use EIO instead of EPIPE when failing to write output file
  pcm_file: improve error checking in write_wav_header function
  pcm_file: in case of failed write clear file buffer variables

 src/pcm/pcm_file.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/3] pcm_file: use EIO instead of EPIPE when failing to write output file

[Adam Miartus]

EPIPE is defined as XRUN which is not entirely correct in this condition

failing to write to a file in pcm_file plugin can not be simply recovered
by a retry as user of the api might be led to believe when receiving EPIPE

use EIO instead to indicate a different kid of error that may not be
recoverable by retry

Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
---
 src/pcm/pcm_file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index ab47da8..d83f97a 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -575,7 +575,7 @@ static snd_pcm_sframes_t snd_pcm_file_writei(snd_pcm_t *pcm, const void *buffer,
 		__snd_pcm_lock(pcm);
 		if (snd_pcm_file_add_frames(pcm, areas, 0, n) < 0) {
 			__snd_pcm_unlock(pcm);
-			return -EPIPE;
+			return -EIO;
 		}
 		__snd_pcm_unlock(pcm);
 	}
@@ -593,7 +593,7 @@ static snd_pcm_sframes_t snd_pcm_file_writen(snd_pcm_t *pcm, void **bufs, snd_pc
 		__snd_pcm_lock(pcm);
 		if (snd_pcm_file_add_frames(pcm, areas, 0, n) < 0) {
 			__snd_pcm_unlock(pcm);
-			return -EPIPE;
+			return -EIO;
 		}
 		__snd_pcm_unlock(pcm);
 	}
@@ -616,7 +616,7 @@ static snd_pcm_sframes_t snd_pcm_file_readi(snd_pcm_t *pcm, void *buffer, snd_pc
 	__snd_pcm_lock(pcm);
 	if (snd_pcm_file_add_frames(pcm, areas, 0, frames) < 0) {
 		__snd_pcm_unlock(pcm);
-		return -EPIPE;
+		return -EIO;
 	}
 
 	__snd_pcm_unlock(pcm);
@@ -640,7 +640,7 @@ static snd_pcm_sframes_t snd_pcm_file_readn(snd_pcm_t *pcm, void **bufs, snd_pcm
 	__snd_pcm_lock(pcm);
 	if (snd_pcm_file_add_frames(pcm, areas, 0, frames) < 0) {
 		__snd_pcm_unlock(pcm);
-		return -EPIPE;
+		return -EIO;
 	}
 
 	__snd_pcm_unlock(pcm);
@@ -666,7 +666,7 @@ static snd_pcm_sframes_t snd_pcm_file_mmap_commit(snd_pcm_t *pcm,
 		result = snd_pcm_mmap_commit(file->gen.slave, ofs, siz);
 		if (result > 0) {
 			if (snd_pcm_file_add_frames(pcm, areas, ofs, result) < 0)
-				return -EPIPE;
+				return -EIO;
 		}
 	}
 	return result;
-- 
2.7.4

Re: [PATCH 1/3] pcm_file: use EIO instead of EPIPE when failing to write output file

[Takashi Iwai]

On Mon, 01 Jul 2019 15:25:16 +0200,
Adam Miartus wrote:
> 
> EPIPE is defined as XRUN which is not entirely correct in this condition
> 
> failing to write to a file in pcm_file plugin can not be simply recovered
> by a retry as user of the api might be led to believe when receiving EPIPE
> 
> use EIO instead to indicate a different kid of error that may not be
> recoverable by retry
> 
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>

Applied, thanks.


Takashi

[PATCH 2/3] pcm_file: improve error checking in write_wav_header function

[Adam Miartus]

previously errno would be returned even for cases where it may have
not been populated, for example one of the write functions failing,
or writing only partial buffer,

now progress through write operations separately and report errno when
appropriate

Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
---
 src/pcm/pcm_file.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index d83f97a..f800e51 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -327,6 +327,8 @@ static void setup_wav_header(snd_pcm_t *pcm, struct wav_fmt *fmt)
 static int write_wav_header(snd_pcm_t *pcm)
 {
 	snd_pcm_file_t *file = pcm->private_data;
+	ssize_t res;
+
 	static const char header[] = {
 		'R', 'I', 'F', 'F',
 		0x24, 0, 0, 0,
@@ -341,15 +343,36 @@ static int write_wav_header(snd_pcm_t *pcm)
 	
 	setup_wav_header(pcm, &file->wav_header);
 
-	if (write(file->fd, header, sizeof(header)) != sizeof(header) ||
-	    write(file->fd, &file->wav_header, sizeof(file->wav_header)) !=
-	    sizeof(file->wav_header) ||
-	    write(file->fd, header2, sizeof(header2)) != sizeof(header2)) {
-		int err = errno;
-		SYSERR("%s write header failed, file data may be corrupt", file->fname);
-		return -err;
+	res = write(file->fd, header, sizeof(header));
+	if (res != sizeof(header)) {
+		goto write_error;
+	}
+
+	res = write(file->fd, &file->wav_header, sizeof(file->wav_header));
+	if (res != sizeof(file->wav_header)) {
+		goto write_error;
 	}
+
+	res = write(file->fd, header2, sizeof(header2));
+	if (res != sizeof(header2)) {
+		goto write_error;
+	}
+
 	return 0;
+
+write_error:
+	// print real errno if available and return EIO, reason for this is to block possible
+	// EPIPE in case file->fd is a pipe. EPIPE from file->fd conflicts with EPIPE from
+	// playback stream which should be used to signal XRUN on playback device
+	if (res < 0) {
+		SYSERR("%s write header failed, file data may be corrupt", file->fname);
+	} else {
+		SNDERR("%s write header incomplete, file data may be corrupt", file->fname);
+	}
+
+	memset(&file->wav_header, 0, sizeof(struct wav_fmt));
+
+	return -EIO;
 }
 
 /* fix up the length fields in WAV header */
-- 
2.7.4

Re: [PATCH 2/3] pcm_file: improve error checking in write_wav_header function

[Takashi Iwai]

On Mon, 01 Jul 2019 15:25:17 +0200,
Adam Miartus wrote:
> 
> previously errno would be returned even for cases where it may have
> not been populated, for example one of the write functions failing,
> or writing only partial buffer,
> 
> now progress through write operations separately and report errno when
> appropriate
> 
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>

The code changes look OK, but just some cosmetic things:

> @@ -341,15 +343,36 @@ static int write_wav_header(snd_pcm_t *pcm)
>  	
>  	setup_wav_header(pcm, &file->wav_header);
>  
> -	if (write(file->fd, header, sizeof(header)) != sizeof(header) ||
> -	    write(file->fd, &file->wav_header, sizeof(file->wav_header)) !=
> -	    sizeof(file->wav_header) ||
> -	    write(file->fd, header2, sizeof(header2)) != sizeof(header2)) {
> -		int err = errno;
> -		SYSERR("%s write header failed, file data may be corrupt", file->fname);
> -		return -err;
> +	res = write(file->fd, header, sizeof(header));
> +	if (res != sizeof(header)) {
> +		goto write_error;
> +	}

Please drop the unnecessary braces here (and in other lines).
We follow the Linux kernel coding style in general.

> +write_error:
> +	// print real errno if available and return EIO, reason for this is to block possible
> +	// EPIPE in case file->fd is a pipe. EPIPE from file->fd conflicts with EPIPE from
> +	// playback stream which should be used to signal XRUN on playback devic
e

Avoid C++ comments but keep in C-style.
Also, try to keep the line in 80chars as much as possible.


thanks,

Takashi

[PATCH 3/3] pcm_file: in case of failed write clear file buffer variables

[Adam Miartus]

previously, in case of failed write to output file, error is returned
from snd_pcm_writei/read APIs, user could run pcm_drain as fallback and
encounter an assert, since drain would try to write remaining file
buffer to a file

if failed to write to output file in first place, it makes sense to clear
current internal pmc_file file buffer variables

Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
---
 src/pcm/pcm_file.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index f800e51..bf6e064 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -414,8 +414,11 @@ static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
 	if (file->format == SND_PCM_FILE_FORMAT_WAV &&
 	    !file->wav_header.fmt) {
 		err = write_wav_header(pcm);
-		if (err < 0)
+		if (err < 0) {
+			file->wbuf_used_bytes = 0;
+			file->file_ptr_bytes = 0;
 			return err;
+		}
 	}
 
 	while (bytes > 0) {
@@ -426,6 +429,8 @@ static int snd_pcm_file_write_bytes(snd_pcm_t *pcm, size_t bytes)
 		err = write(file->fd, file->wbuf + file->file_ptr_bytes, n);
 		if (err < 0) {
 			err = -errno;
+			file->wbuf_used_bytes = 0;
+			file->file_ptr_bytes = 0;
 			SYSERR("%s write failed, file data may be corrupt", file->fname);
 			return err;
 		}
-- 
2.7.4

Re: [PATCH 3/3] pcm_file: in case of failed write clear file buffer variables

[Takashi Iwai]

On Mon, 01 Jul 2019 15:25:18 +0200,
Adam Miartus wrote:
> 
> previously, in case of failed write to output file, error is returned
> from snd_pcm_writei/read APIs, user could run pcm_drain as fallback and
> encounter an assert, since drain would try to write remaining file
> buffer to a file
> 
> if failed to write to output file in first place, it makes sense to clear
> current internal pmc_file file buffer variables
> 
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>

Applied, thanks.


Takashi

Re: [PATCH 0/3] pcm_file report EIO when fail to write to debug file

[Takashi Iwai]

On Mon, 01 Jul 2019 15:25:15 +0200,
Adam Miartus wrote:
> 
> Previously, I introduced a patch that may return EPIPE error when pcm_file
> plugin fails to write to output file for some reason.
> 
> The purpose of reporting the error was to evade assert in code to allow
> API user error handling and recovery, or more clarity in debugging.
> 
> Failing to write to a file usually means bad file descriptor, running
> out of free memory, insufficient file permissions or in case file
> is a pipe there may be other reasons preventing the write operation.
> All these are not easily recoverable by restaring playback as EPIPE
> error which is used to signal XRUN might suggest. Therefore, use EIO
> to signal a different kind of error.
> 
> write_wav_header function is refactored a bit for more detailed
> error handling in case of failed write operation.
> 
> Adam Miartus (3):
>   pcm_file: use EIO instead of EPIPE when failing to write output file
>   pcm_file: improve error checking in write_wav_header function
>   pcm_file: in case of failed write clear file buffer variables

I applied patches 1 and 3.  Could you resubmit patch 2 after
correction?


thanks,

Takashi