From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 2/3] pcm_file: improve error checking in write_wav_header function Date: Wed, 03 Jul 2019 14:21:45 +0200 Message-ID: References: <1561987518-2828-1-git-send-email-amiartus@de.adit-jv.com> <1561987518-2828-3-git-send-email-amiartus@de.adit-jv.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 4D3D1F80058 for ; Wed, 3 Jul 2019 14:21:45 +0200 (CEST) In-Reply-To: <1561987518-2828-3-git-send-email-amiartus@de.adit-jv.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Adam Miartus Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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 > Reviewed-by: Timo Wischer 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