Re: [Qemu-devel] [PATCHv4 10/11] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/15/2012 04:00 PM, Michael Tokarev wrote:
> 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 iov_send_recv() 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 and extra
> `iov_cnt' argument.
>
> Cc: MORITA Kazutaka<morita.kazutaka@lab.ntt.co.jp>
> Cc: Paolo Bonzini<pbonzini@redhat.com>
> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>

Same comments here re: macros.

Regards,

Anthony Liguori

> ---
>   block/nbd.c         |   16 +++++-----
>   block/sheepdog.c    |    6 ++--
>   qemu-common.h       |   37 ++++++++++------------
>   qemu-coroutine-io.c |   82 +++++++++++++++-----------------------------------
>   4 files changed, 53 insertions(+), 88 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 161b299..c451a25 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -184,7 +184,7 @@ static void nbd_restart_write(void *opaque)
>   }
>
>   static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
> -                               struct iovec *iov, int offset)
> +                               QEMUIOVector *qiov, int offset)
>   {
>       int rc, ret;
>
> @@ -193,8 +193,8 @@ static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
>       qemu_aio_set_fd_handler(s->sock, nbd_reply_ready, nbd_restart_write,
>                               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);
> +    if (rc != -1&&  qiov) {
> +        ret = qemu_co_sendv(s->sock, qiov->iov, qiov->niov, offset, request->len);
>           if (ret != request->len) {
>               errno = -EIO;
>               rc = -1;
> @@ -209,7 +209,7 @@ static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
>
>   static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
>                                    struct nbd_reply *reply,
> -                                 struct iovec *iov, int offset)
> +                                 QEMUIOVector *qiov, int offset)
>   {
>       int ret;
>
> @@ -220,8 +220,8 @@ static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
>       if (reply->handle != request->handle) {
>           reply->error = EIO;
>       } else {
> -        if (iov&&  reply->error == 0) {
> -            ret = qemu_co_recvv(s->sock, iov, request->len, offset);
> +        if (qiov&&  reply->error == 0) {
> +            ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov, offset, request->len);
>               if (ret != request->len) {
>                   reply->error = EIO;
>               }
> @@ -336,7 +336,7 @@ static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
>       if (nbd_co_send_request(s,&request, NULL, 0) == -1) {
>           reply.error = errno;
>       } else {
> -        nbd_co_receive_reply(s,&request,&reply, qiov->iov, offset);
> +        nbd_co_receive_reply(s,&request,&reply, qiov, offset);
>       }
>       nbd_coroutine_end(s,&request);
>       return -reply.error;
> @@ -360,7 +360,7 @@ static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
>       request.len = nb_sectors * 512;
>
>       nbd_coroutine_start(s,&request);
> -    if (nbd_co_send_request(s,&request, qiov->iov, offset) == -1) {
> +    if (nbd_co_send_request(s,&request, qiov, offset) == -1) {
>           reply.error = errno;
>       } else {
>           nbd_co_receive_reply(s,&request,&reply, NULL, 0);
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 00276f6f..e688e49 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, acb->qiov->niov,
> +                            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, niov, 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 d4a0243..d9858d1 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -297,32 +297,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, unsigned iov_cnt,
> +                            size_t offset, size_t bytes, bool do_send);
> +#define qemu_co_recvv(sockfd, iov, iov_cnt, offset, bytes) \
> +  qemu_co_sendv_recvv(sockfd, iov, iov_cnt, offset, bytes, false)
> +#define qemu_co_sendv(sockfd, iov, iov_cnt, offset, bytes) \
> +  qemu_co_sendv_recvv(sockfd, iov, iov_cnt, 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_send);
> +#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 0461a9a..6693c78 100644
> --- a/qemu-coroutine-io.c
> +++ b/qemu-coroutine-io.c
> @@ -27,71 +27,39 @@
>   #include "qemu-coroutine.h"
>   #include "iov.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, unsigned iov_cnt,
> +                    size_t offset, size_t bytes, bool do_send)
>   {
> -    int total = 0;
> -    int ret;
> -    while (len) {
> -        ret = iov_recv(sockfd, iov, iov_offset + total, len);
> -        if (ret<  0) {
> +    size_t done = 0;
> +    ssize_t ret;
> +    while (done<  bytes) {
> +        ret = iov_send_recv(sockfd, iov,
> +                            offset + done, bytes - done, do_send);
> +        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 = iov_send(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_send) {
> +            /* 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_send)
>   {
> -    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, 1, 0, bytes, do_send);
>   }