From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60097) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8ZwR-00072f-OR for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:24:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S8ZwL-00074R-Qb for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:23:47 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:32822) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8ZwL-00074A-LX for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:23:41 -0400 Received: by yhoo21 with SMTP id o21so5063891yho.4 for ; Fri, 16 Mar 2012 09:23:40 -0700 (PDT) Message-ID: <4F636908.4020300@redhat.com> Date: Fri, 16 Mar 2012 11:23:36 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1331845217-21705-1-git-send-email-mjt@msgid.tls.msk.ru> <1331845217-21705-12-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: <1331845217-21705-12-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 11/11] rewrite iov_send_recv() and move it to iov.c 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: > Make it much more understandable, add a missing > iov_cnt argument (number of iovs in the iov), and > add comments to it. > > The new implementation has been extensively tested > by splitting a large buffer into many small > randomly-sized chunks, sending it over socket to > another, slow process and verifying the receiving > data is the same. > > Signed-off-by: Michael Tokarev > --- > cutils.c | 83 ---------------------------------------- > iov.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ > iov.h | 15 ++++--- > qemu-coroutine-io.c | 2 +- > 4 files changed, 115 insertions(+), 90 deletions(-) > > diff --git a/cutils.c b/cutils.c > index cb6f638..e2bc1b8 100644 > --- a/cutils.c > +++ b/cutils.c > @@ -375,86 +375,3 @@ int qemu_parse_fd(const char *param) > } > return fd; > } > - > -ssize_t iov_send_recv(int sockfd, struct iovec *iov, > - size_t offset, size_t bytes, > - bool do_sendv) > -{ > - int iovlen; > - ssize_t ret; > - size_t diff; > - struct iovec *last_iov; > - > - /* last_iov is inclusive, so count from one. */ > - iovlen = 1; > - last_iov = iov; > - bytes += offset; > - > - while (last_iov->iov_len< bytes) { > - bytes -= last_iov->iov_len; > - > - last_iov++; > - iovlen++; > - } > - > - diff = last_iov->iov_len - bytes; > - last_iov->iov_len -= diff; > - > - while (iov->iov_len<= offset) { > - offset -= iov->iov_len; > - > - iov++; > - iovlen--; > - } > - > - iov->iov_base = (char *) iov->iov_base + offset; > - iov->iov_len -= offset; > - > - { > -#if defined CONFIG_IOVEC&& defined CONFIG_POSIX > - struct msghdr msg; > - memset(&msg, 0, sizeof(msg)); > - msg.msg_iov = iov; > - msg.msg_iovlen = iovlen; > - > - do { > - if (do_sendv) { > - ret = sendmsg(sockfd,&msg, 0); > - } else { > - ret = recvmsg(sockfd,&msg, 0); > - } > - } while (ret == -1&& errno == EINTR); > -#else > - struct iovec *p = iov; > - ret = 0; > - while (iovlen> 0) { > - int rc; > - if (do_sendv) { > - rc = send(sockfd, p->iov_base, p->iov_len, 0); > - } else { > - rc = qemu_recv(sockfd, p->iov_base, p->iov_len, 0); > - } > - if (rc == -1) { > - if (errno == EINTR) { > - continue; > - } > - if (ret == 0) { > - ret = -1; > - } > - break; > - } > - if (rc == 0) { > - break; > - } > - ret += rc; > - iovlen--, p++; > - } > -#endif > - } > - > - /* Undo the changes above */ > - iov->iov_base = (char *) iov->iov_base - offset; > - iov->iov_len += offset; > - last_iov->iov_len += diff; > - return ret; > -} > diff --git a/iov.c b/iov.c > index fd518dd..6c79b68 100644 > --- a/iov.c > +++ b/iov.c > @@ -18,6 +18,14 @@ > > #include "iov.h" > > +#ifdef _WIN32 > +# include > +# include > +#else > +# include > +# include > +#endif > + > size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, > size_t offset, const void *buf, size_t bytes) > { > @@ -87,6 +95,103 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt) > return len; > } > > +/* helper function for iov_send_recv() */ > +static ssize_t > +do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send) > +{ > +#if defined CONFIG_IOVEC&& defined CONFIG_POSIX > + ssize_t ret; > + struct msghdr msg; > + memset(&msg, 0, sizeof(msg)); > + msg.msg_iov = iov; > + msg.msg_iovlen = iov_cnt; > + do { > + ret = do_send > + ? sendmsg(sockfd,&msg, 0) > + : recvmsg(sockfd,&msg, 0); > + } while (ret< 0&& errno == EINTR); > + return ret; > +#else > + /* else send piece-by-piece */ > + /*XXX Note: windows has WSASend() and WSARecv() */ > + unsigned i; > + size_t count = 0; > + for(i = 0; i< iov_cnt; ++i) { > + ssize_t r = do_send > + ? send(sockfd, iov[i].iov_base, iov[i].iov_len, 0) > + : recv(sockfd, iov[i].iov_base, iov[i].iov_len, 0); > + if (r> 0) { > + ret += r; > + } else if (!r) { > + break; > + } else if (errno == EINTR) { > + continue; > + } else { > + /* else it is some "other" error, > + * only return if there was no data processed. */ > + if (ret == 0) { > + return -1; > + } > + break; > + } > + } > + return count; > +#endif > +} > + > +ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, > + size_t offset, size_t bytes, > + bool do_send) > +{ > + ssize_t ret; > + size_t lastlen; > + unsigned si, ei; /* start and end indexes */ > + > + /* Find the start position, skipping `offset' bytes: > + * first, skip all full-sized vector elements, */ > + for(si = 0; si< iov_cnt&& offset>= iov[si].iov_len; ++si) { > + offset -= iov[si].iov_len; > + } > + if (offset) { > + assert(si< iov_cnt); > + /* second, skip `offset' bytes from the (now) first element, > + * undo it on exit */ > + iov[si].iov_base += offset; > + iov[si].iov_len -= offset; > + } > + /* Find the end position skipping `bytes' bytes: */ > + /* first, skip all full-sized elements */ > + for(ei = si; ei< iov_cnt&& iov[ei].iov_len<= bytes; ++ei) { > + bytes -= iov[ei].iov_len; > + } > + if (bytes) { > + /* second, fixup the last element, and remember > + * the length we've cut from the end of it in `bytes' */ > + assert(ei< iov_cnt); > + assert(iov[ei].iov_len> bytes); > + lastlen = iov[ei].iov_len; > + iov[ei].iov_len = bytes; > + ++ei; > + } else { > + lastlen = 0; > + } > + > + ret = do_send_recv(sockfd, iov + si, ei - si, do_send); > + > + /* Undo the changes above */ > + if (offset) { > + iov[si].iov_base -= offset; > + iov[si].iov_len += offset; > + } > + if (lastlen) { > + --ei; > + iov[ei].iov_len = lastlen; > + } > + > + return ret; > +} > + > + Coding style is off in this code. I'd really like to see unit tests if we're rewriting core functions... Regards, Anthony Liguori > void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt, > FILE *fp, const char *prefix, size_t limit) > { > diff --git a/iov.h b/iov.h > index 9b6a883..381f37a 100644 > --- a/iov.h > +++ b/iov.h > @@ -60,7 +60,7 @@ size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt, > * `offset' bytes in the beginning of iovec buffer are skipped and > * next `bytes' bytes are used, which must be within data of iovec. > * > - * r = iov_send_recv(sockfd, iov, offset, bytes, true); > + * r = iov_send_recv(sockfd, iov, iovcnt, offset, bytes, true); > * > * is logically equivalent to > * > @@ -68,13 +68,16 @@ size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt, > * iov_to_buf(iov, iovcnt, offset, buf, bytes); > * r = send(sockfd, buf, bytes, 0); > * free(buf); > + * > + * For iov_send_recv() _whole_ area being sent or received > + * should be within the iovec, not only beginning of it. > */ > -ssize_t iov_send_recv(int sockfd, struct iovec *iov, > +ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, > size_t offset, size_t bytes, bool do_send); > -#define iov_recv(sockfd, iov, offset, bytes) \ > - iov_send_recv(sockfd, iov, offset, bytes, false) > -#define iov_send(sockfd, iov, offset, bytes) \ > - iov_send_recv(sockfd, iov, offset, bytes, true) > +#define iov_recv(sockfd, iov, iov_cnt, offset, bytes) \ > + iov_send_recv(sockfd, iov, iov_cnt, offset, bytes, false) > +#define iov_send(sockfd, iov, iov_cnt, offset, bytes) \ > + iov_send_recv(sockfd, iov, iov_cnt, offset, bytes, true) > > /** > * Produce a text hexdump of iovec `iov' with `iov_cnt' number of elements > diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c > index 6693c78..5734965 100644 > --- a/qemu-coroutine-io.c > +++ b/qemu-coroutine-io.c > @@ -34,7 +34,7 @@ qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt, > size_t done = 0; > ssize_t ret; > while (done< bytes) { > - ret = iov_send_recv(sockfd, iov, > + ret = iov_send_recv(sockfd, iov, iov_cnt, > offset + done, bytes - done, do_send); > if (ret> 0) { > done += ret;