From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39082) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8ZqK-0005y8-PR for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:17:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S8ZqI-0005uO-Mk for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:17:28 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:40088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8ZqI-0005tP-G6 for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:17:26 -0400 Received: by mail-pz0-f45.google.com with SMTP id p14so6484436dad.4 for ; Fri, 16 Mar 2012 09:17:25 -0700 (PDT) Message-ID: <4F636792.3010606@codemonkey.ws> Date: Fri, 16 Mar 2012 11:17:22 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1331845217-21705-1-git-send-email-mjt@msgid.tls.msk.ru> <1331845217-21705-4-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: <1331845217-21705-4-git-send-email-mjt@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4 03/11] rewrite iov_* functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: Paolo Bonzini , qemu-devel@nongnu.org On 03/15/2012 04:00 PM, Michael Tokarev wrote: > This changes implementations of all iov_* > functions, completing the previous step. > > All iov_* functions now ensure that this offset > argument is within the iovec (using assertion), > but lets to specify `bytes' value larger than > actual length of the iovec - in this case they > stops at the actual end of iovec. It is also > suggested to use convinient `-1' value as `bytes' > to mean just this -- "up to the end". > > There's one very minor semantic change here: new > requiriment is that `offset' points to inside of > iovec. This is checked just at the end of functions > (assert()), it does not actually need to be enforced, > but using any of these functions with offset pointing > past the end of iovec is wrong anyway. > > Note: the new code in iov.c uses arithmetic with > void pointers. I thought this is not supported > everywhere and is a GCC extension (indeed, the C > standard does not define void arithmetic). However, > the original code already use void arith in > iov_from_buf() function: > (memcpy(..., buf + buf_off,...) > which apparently works well so far (it is this > way in qemu 1.0). So I left it this way and used > it in other places. > > Signed-off-by: Michael Tokarev > --- > iov.c | 91 ++++++++++++++++++++++++++++------------------------------------- > iov.h | 12 +++++++- > 2 files changed, 49 insertions(+), 54 deletions(-) > > diff --git a/iov.c b/iov.c > index bc58cab..fd518dd 100644 > --- a/iov.c > +++ b/iov.c > @@ -7,6 +7,7 @@ > * Author(s): > * Anthony Liguori > * Amit Shah > + * Michael Tokarev > * > * This work is licensed under the terms of the GNU GPL, version 2. See > * the COPYING file in the top-level directory. > @@ -17,75 +18,61 @@ > > #include "iov.h" > > -size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, size_t iov_off, > - const void *buf, size_t size) > +size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, > + size_t offset, const void *buf, size_t bytes) > { > - size_t iovec_off, buf_off; > + size_t done; > unsigned int i; > - > - iovec_off = 0; > - buf_off = 0; > - for (i = 0; i< iov_cnt&& size; i++) { > - if (iov_off< (iovec_off + iov[i].iov_len)) { > - size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off, size); > - > - memcpy(iov[i].iov_base + (iov_off - iovec_off), buf + buf_off, len); > - > - buf_off += len; > - iov_off += len; > - size -= len; > + for (i = 0, done = 0; done< bytes&& i< iov_cnt; i++) { > + if (offset< iov[i].iov_len) { > + size_t len = MIN(iov[i].iov_len - offset, bytes - done); > + memcpy(iov[i].iov_base + offset, buf + done, len); > + done += len; > + offset = 0; > + } else { > + offset -= iov[i].iov_len; > } > - iovec_off += iov[i].iov_len; > } > - return buf_off; > + assert(offset == 0); > + return done; > } It makes me nervous to make a change like this as it has wide impact on the rest of the code base. Could you include a unit test that tests the various boundary conditions of this code? Regards, Anthony Liguori