[alsa-devel] [PATCH 1/2] pcm_file: do not disrupt playback on output file write fail

[Cezary Rojewski]

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 at 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)
>