[PATCHv3,8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

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 <mjt@tls.msk.ru>
---
 block/nbd.c         |    4 +-
 block/sheepdog.c    |    6 ++--
 qemu-common.h       |   39 ++++++++++++------------
 qemu-coroutine-io.c |   82 +++++++++++++++-----------------------------------
 4 files changed, 49 insertions(+), 82 deletions(-)

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 <mjt@tls.msk.ru>
> ---
>  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.

Paolo

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 <mjt@tls.msk.ru>
>> ---
>>  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

On 13.03.2012 22:01, Paolo Bonzini wrote:

>>>  ssize_t qemu_sendv_recvv(int sockfd, struct iovec *iov,
>>>                           size_t offset, size_t bytes, bool do_sendv);

> 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.

Yes such an argument may be needed here, and this function
looks differently from the usual iov_* family in this very
place: lack of iov_cnt.

>    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. ;)

Heh for the #defines.  Defines are fine when used properly,
really.  In this case, the #define just adds an argument
and nothing more.

For qemu_sendv and qemu_recvv, I'll just go for renaming these
to be iov_send and iov_recv, and move them to iov.[ch].

And for qemu_co_sendv()&Co, which actually _are_ used, this
wont help anyway.  Or I can rename them to qemu_iovec_co_send(),
to catch the change.  But it becomes longer... :(

What do you think?

Thank you!

/mjt