[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv()
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends |
Date: |
Tue, 13 Mar 2012 19:01:49 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 |
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 <address@hidden>
>> ---
>> 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
- Re: [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), (continued)
- [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions, Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent, Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions, Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends, Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 7/9] export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv(), Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying, Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 6/9] change prototypes of qemu_sendv() and qemu_recvv(), Michael Tokarev, 2012/03/12
- [Qemu-devel] [PATCHv3 9/9] rewrite and comment qemu_sendv_recvv(), Michael Tokarev, 2012/03/12
- Re: [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions, Michael Tokarev, 2012/03/13