From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7W2n-0002tY-HW for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:02:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S7W2k-0006bi-PI for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:01:57 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:38994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S7W2k-0006bJ-Cr for qemu-devel@nongnu.org; Tue, 13 Mar 2012 14:01:54 -0400 Received: by eaaf11 with SMTP id f11so545073eaa.4 for ; Tue, 13 Mar 2012 11:01:52 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4F5F8B8D.9010202@redhat.com> Date: Tue, 13 Mar 2012 19:01:49 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1331579663-29950-1-git-send-email-mjt@msgid.tls.msk.ru> <1331579663-29950-9-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-devel@nongnu.org Il 13/03/2012 18:52, Paolo Bonzini ha scritto: > Il 12/03/2012 20:14, Michael Tokarev ha scritto: >> The same as for non-coroutine versions in previous >> patches: rename arguments to be more obvious, change >> type of arguments from int to size_t where appropriate, >> and use common code for send and receive paths (with >> one extra argument) since these are exactly the same. >> Use common qemu_sendv_recvv() directly. >> >> qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() >> are now trivial #define's merely adding one extra arg. >> >> qemu_co_sendv() and qemu_co_recvv() callers are >> converted to different argument order. >> >> Also add comment near qemu_sendv() and qemu_recvv() >> stating that these are now unused. >> >> Signed-off-by: Michael Tokarev >> --- >> block/nbd.c | 4 +- >> block/sheepdog.c | 6 ++-- >> qemu-common.h | 39 ++++++++++++------------ >> qemu-coroutine-io.c | 82 +++++++++++++++----------------------------------- >> 4 files changed, 49 insertions(+), 82 deletions(-) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 161b299..9919acc 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -194,7 +194,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request, >> nbd_have_request, NULL, s); >> rc = nbd_send_request(s->sock, request); >> if (rc != -1 && iov) { >> - ret = qemu_co_sendv(s->sock, iov, request->len, offset); >> + ret = qemu_co_sendv(s->sock, iov, offset, request->len); >> if (ret != request->len) { >> errno = -EIO; >> rc = -1; >> @@ -221,7 +221,7 @@ static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request, >> reply->error = EIO; >> } else { >> if (iov && reply->error == 0) { >> - ret = qemu_co_recvv(s->sock, iov, request->len, offset); >> + ret = qemu_co_recvv(s->sock, iov, offset, request->len); >> if (ret != request->len) { >> reply->error = EIO; >> } >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 00276f6f..1083f27 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -657,8 +657,8 @@ static void coroutine_fn aio_read_response(void *opaque) >> } >> break; >> case AIOCB_READ_UDATA: >> - ret = qemu_co_recvv(fd, acb->qiov->iov, rsp.data_length, >> - aio_req->iov_offset); >> + ret = qemu_co_recvv(fd, acb->qiov->iov, >> + aio_req->iov_offset, rsp.data_length); >> if (ret < 0) { >> error_report("failed to get the data, %s", strerror(errno)); >> goto out; >> @@ -924,7 +924,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req, >> } >> >> if (wlen) { >> - ret = qemu_co_sendv(s->fd, iov, wlen, aio_req->iov_offset); >> + ret = qemu_co_sendv(s->fd, iov, aio_req->iov_offset, wlen); >> if (ret < 0) { >> qemu_co_mutex_unlock(&s->lock); >> error_report("failed to send a data, %s", strerror(errno)); >> diff --git a/qemu-common.h b/qemu-common.h >> index b01d84c..eae1bde 100644 >> --- a/qemu-common.h >> +++ b/qemu-common.h >> @@ -207,6 +207,8 @@ int qemu_pipe(int pipefd[2]); >> */ >> ssize_t qemu_sendv_recvv(int sockfd, struct iovec *iov, >> size_t offset, size_t bytes, bool do_sendv); >> +/* these are basically unused now when everything >> + * switched is using coroutine versions */ >> #define qemu_recvv(sockfd, iov, offset, bytes) \ >> qemu_sendv_recvv(sockfd, iov, offset, bytes, false) >> #define qemu_sendv(sockfd, iov, offset, bytes) \ >> @@ -306,32 +308,29 @@ struct qemu_work_item { >> void qemu_init_vcpu(void *env); >> #endif >> >> -/** >> - * Sends an iovec (or optionally a part of it) down a socket, yielding >> - * when the socket is full. >> - */ >> -int qemu_co_sendv(int sockfd, struct iovec *iov, >> - int len, int iov_offset); >> >> /** >> - * Receives data into an iovec (or optionally into a part of it) from >> - * a socket, yielding when there is no data in the socket. >> + * Sends a (part of) iovec down a socket, yielding when the socket is full, or >> + * Receives data into a (part of) iovec from a socket, >> + * yielding when there is no data in the socket. >> + * The same interface as qemu_sendv_recvv(), with added yielding. >> + * XXX should mark these as coroutine_fn >> */ >> -int qemu_co_recvv(int sockfd, struct iovec *iov, >> - int len, int iov_offset); >> - >> +ssize_t qemu_co_sendv_recvv(int sockfd, struct iovec *iov, >> + size_t offset, size_t bytes, bool do_sendv); >> +#define qemu_co_recvv(sockfd, iov, offset, bytes) \ >> + qemu_co_sendv_recvv(sockfd, iov, offset, bytes, false) >> +#define qemu_co_sendv(sockfd, iov, offset, bytes) \ >> + qemu_co_sendv_recvv(sockfd, iov, offset, bytes, true) >> >> /** >> - * Sends a buffer down a socket, yielding when the socket is full. >> + * The same as above, but with just a single buffer >> */ >> -int qemu_co_send(int sockfd, void *buf, int len); >> - >> -/** >> - * Receives data into a buffer from a socket, yielding when there >> - * is no data in the socket. >> - */ >> -int qemu_co_recv(int sockfd, void *buf, int len); >> - >> +ssize_t qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_sendv); >> +#define qemu_co_recv(sockfd, buf, bytes) \ >> + qemu_co_send_recv(sockfd, buf, bytes, false) >> +#define qemu_co_send(sockfd, buf, bytes) \ >> + qemu_co_send_recv(sockfd, buf, bytes, true) >> >> typedef struct QEMUIOVector { >> struct iovec *iov; >> diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c >> index df8ff21..7c978ea 100644 >> --- a/qemu-coroutine-io.c >> +++ b/qemu-coroutine-io.c >> @@ -26,71 +26,39 @@ >> #include "qemu_socket.h" >> #include "qemu-coroutine.h" >> >> -int coroutine_fn qemu_co_recvv(int sockfd, struct iovec *iov, >> - int len, int iov_offset) >> +ssize_t coroutine_fn >> +qemu_co_sendv_recvv(int sockfd, struct iovec *iov, >> + size_t offset, size_t bytes, bool do_sendv) >> { >> - int total = 0; >> - int ret; >> - while (len) { >> - ret = qemu_recvv(sockfd, iov, iov_offset + total, len); >> - if (ret < 0) { >> + size_t done = 0; >> + ssize_t ret; >> + while (done < bytes) { >> + ret = qemu_sendv_recvv(sockfd, iov, >> + offset + done, bytes - done, do_sendv); >> + if (ret > 0) { >> + done += ret; >> + } else if (ret < 0) { >> if (errno == EAGAIN) { >> qemu_coroutine_yield(); >> - continue; >> - } >> - if (total == 0) { >> - total = -1; >> - } >> - break; >> - } >> - if (ret == 0) { >> - break; >> - } >> - total += ret, len -= ret; >> - } >> - >> - return total; >> -} >> - >> -int coroutine_fn qemu_co_sendv(int sockfd, struct iovec *iov, >> - int len, int iov_offset) >> -{ >> - int total = 0; >> - int ret; >> - while (len) { >> - ret = qemu_sendv(sockfd, iov, iov_offset + total, len); >> - if (ret < 0) { >> - if (errno == EAGAIN) { >> - qemu_coroutine_yield(); >> - continue; >> - } >> - if (total == 0) { >> - total = -1; >> + } else if (done == 0) { >> + return -1; >> + } else { >> + break; >> } >> + } else if (ret == 0 && !do_sendv) { >> + /* write (send) should never return 0. >> + * read (recv) returns 0 for end-of-file (-data). >> + * In both cases there's little point retrying, >> + * but we do for write anyway, just in case */ >> break; >> } >> - total += ret, len -= ret; >> } >> - >> - return total; >> + return done; >> } >> >> -int coroutine_fn qemu_co_recv(int sockfd, void *buf, int len) >> +ssize_t coroutine_fn >> +qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_sendv) >> { >> - struct iovec iov; >> - >> - iov.iov_base = buf; >> - iov.iov_len = len; >> - >> - return qemu_co_recvv(sockfd, &iov, len, 0); >> -} >> - >> -int coroutine_fn qemu_co_send(int sockfd, void *buf, int len) >> -{ >> - struct iovec iov; >> - >> - iov.iov_base = buf; >> - iov.iov_len = len; >> - >> - return qemu_co_sendv(sockfd, &iov, len, 0); >> + struct iovec iov = { .iov_base = buf, .iov_len = bytes }; >> + return qemu_co_sendv_recvv(sockfd, &iov, 0, bytes, do_sendv); >> } > > Looks good. Hmm, since you are at it however, perhaps you could add an argument to these functions and qemu_sendv_recvv for the length of the iovec, so that it can detect out-of-bounds access. This would also "fix" the fact that you changed the signature of the function in a compatible way, while making the API incompatible. As you prefer, but we can call it a deal for the #defines. ;) Paolo