[Qemu-devel] Re: [PATCH 2/3] NBD library: add aio-compatible read/write function

[Kevin Wolf]

Am 18.02.2011 13:55, schrieb Nick Thomas:
> Signed-off-by: Nicholas Thomas <address@hidden>
> ---
>  nbd.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  nbd.h |    2 ++
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index abe0ecb..83d3342 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -107,6 +107,57 @@ size_t nbd_wr_sync(int fd, void *buffer, size_t size, 
> bool do_read)
>      return offset;
>  }
>  
> +int nbd_wr_aio(int sockfd, struct iovec *iov, size_t len,  off_t offset,
> +               bool do_read)
> +{
> +    struct msghdr msg;
> +    int ret, diff;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = iov;
> +    msg.msg_iovlen = 1;
> +
> +    len += offset;
> +
> +    while (iov->iov_len < len) {
> +        len -= iov->iov_len;
> +
> +        iov++;
> +        msg.msg_iovlen++;
> +    }

Is there a reason why you don't use a QEMUIOVector here? It already has
alls the information like the number of entries. Throwing this
information away only to recalculate it here seems a bit pointless.

> +
> +    diff = iov->iov_len - len;
> +    iov->iov_len -= diff;
> +
> +    while (msg.msg_iov->iov_len <= offset) {
> +        offset -= msg.msg_iov->iov_len;
> +
> +        msg.msg_iov++;
> +        msg.msg_iovlen--;
> +    }
> +
> +    msg.msg_iov->iov_base = (char *) msg.msg_iov->iov_base + offset;
> +    msg.msg_iov->iov_len -= offset;

You could use qemu_iovec_copy to create a qiov that covers exactly the
part that you want to use.

> +
> +retry:
> +    if (do_read) {
> +        ret = recvmsg(sockfd, &msg, 0);
> +    } else {
> +        ret = sendmsg(sockfd, &msg, 0);
> +    }
> +
> +    /* recoverable error */
> +    if (ret == -1 && (errno == EAGAIN || errno == EINTR)) {
> +        goto retry;
> +    }

Make this a do...while loop, no reason for goto here.

> +
> +    msg.msg_iov->iov_base = (char *) msg.msg_iov->iov_base - offset;
> +    msg.msg_iov->iov_len += offset;
> +
> +    iov->iov_len += diff;
> +    return ret;

This wouldn't be necessary with a copied qiov either (but you'd need to
destroy the copy)

Kevin