From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact() Date: Thu, 03 Jul 2014 18:41:08 +0100 Message-ID: <53B595B4.5000701@citrix.com> References: <1404324299-29286-1-git-send-email-andrew.cooper3@citrix.com> <21429.36759.826159.277629@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21429.36759.826159.277629@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 03/07/2014 18:15, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()"): >> This implementation of writev_exact() will cope with an iovcnt greater than >> IOV_MAX because glibc will actually let this work anyway, and it is very >> useful not to have to work about this in the caller of writev_exact(). The >> caller is still required to ensure that the sum of iov_len's doesn't overflow >> a ssize_t. > ... >> +int writev_exact(int fd, struct iovec *iov, int iovcnt) >> +{ >> + int iov_idx = 0; >> + ssize_t len; >> + >> + while ( iov_idx < iovcnt ) >> + { >> + /* Skip over iov[] enties with 0 length. */ >> + while ( iov[iov_idx].iov_len == 0 ) >> + if ( ++iov_idx == iovcnt ) >> + return 0; >> + >> + len = writev(fd, &iov[iov_idx], MIN(iovcnt - iov_idx, IOV_MAX)); >> + >> + if ( (len == -1) && (errno == EINTR) ) >> + continue; >> + if ( len <= 0 ) >> + return -1; > If writev does return 0, you at the very least need to set errno > before changing the return value to -1. Hmm - that should probably be "return len", to have the same EOF behaviour as write_exact(). In that case, errno should be correct without further modification. Errno does however need to be set for the previous return 0. > >> + /* Check iov[] to see whether we had a partial or complete write. */ >> + while ( len > 0 && (iov_idx < iovcnt) ) >> + len -= iov[iov_idx++].iov_len; >> + >> + if ( len < 0 ) /* Partial write of iov[iov_idx - 1]. */ >> + { >> + iov_idx--; > This logic is rather wtf! Isn't there a way of expressing it that > doesn't involve len becoming negative and unwinding iov_idx ? > >> +int writev_exact(int fd, struct iovec *iov, int iovcnt); >> +/* Note - writev_exact() might modify iov. Whether it does so in practice >> + * depends on whether your system implementation of writev() returns from a >> + * partial write in the middle of an iov element. */ > The second sentence should be removed. No-one is allowed to assume > that writev doesn't do so. Also, you should mention that your > writev_exact lacks the atomicity guarantee of proper writev. Actually I wonder. Rethinking this somewhat, the atomicity guarentee of the proper writev() wrt individual iov[] elements constitute a guarentee that it shall never return having performed a partial write of an individual iov[] element. I came to this conclusion first, then somehow argued myself out of it. Can I have a second opinion? ~Andrew