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

[Takashi Iwai <tiwai@suse.de>]

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