Re: [Qemu-devel] [PATCHv4 02/11] change iov_* function prototypes to be more appropriate

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/15/2012 04:00 PM, Michael Tokarev wrote:
> Reorder arguments to be more natural, readable and
> consistent with other iov_* functions, and change
> argument names, from:
>   iov_from_buf(iov, iov_cnt, buf, iov_off, size)
> to
>   iov_from_buf(iov, iov_cnt, offset, buf, bytes)
>
> The result becomes natural English:

I don't think this is a good idea.  This is code churn for nothing but cosmetic 
reasons.  I don't think there's a lot of value in that.

>
>   copy data to this `iov' vector with `iov_cnt'
>   elements starting at byte offset `offset'
>   from memory buffer `buf', processing `bytes'
>   bytes max.
>
> (Try to read the original prototype this way).
>
> Also change iov_clear() to more general iov_memset()
> (it uses memset() internally anyway).
>
> While at it, add comments to the header file
> describing what the routines actually does.
>
> The patch only renames argumens in the header, but
> keeps old names in the implementation.  The next
> patch will touch actual code to match.
>
> Now, it might look wrong to pay so much attention
> to so small things.  But we've so many badly designed
> interfaces already so the whole thing becomes rather
> confusing or error prone.  One example of this is
> previous commit and small discussion which emerged
> from it, with an outcome that the utility functions
> like these aren't well-understdandable, leading to
> strange usage cases.  That's why I paid quite some
> attention to this set of functions and a few
> others in subsequent patches.
>
> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
> ---
>   hw/rtl8139.c           |    2 +-
>   hw/usb/core.c          |    6 +++---
>   hw/virtio-balloon.c    |    4 ++--
>   hw/virtio-net.c        |    4 ++--
>   hw/virtio-serial-bus.c |    6 +++---
>   iov.c                  |   14 +++++++-------
>   iov.h                  |   42 +++++++++++++++++++++++++++++++++++++-----
>   net.c                  |    2 +-
>   8 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 05b8e1e..e837079 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -1799,7 +1799,7 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>           if (iov) {
>               buf2_size = iov_size(iov, 3);
>               buf2 = g_malloc(buf2_size);
> -            iov_to_buf(iov, 3, buf2, 0, buf2_size);
> +            iov_to_buf(iov, 3, 0, buf2, buf2_size);
>               buf = buf2;
>           }
>
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index a4048fe..4a213aa 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -516,10 +516,10 @@ void usb_packet_copy(USBPacket *p, void *ptr, size_t bytes)
>       switch (p->pid) {
>       case USB_TOKEN_SETUP:
>       case USB_TOKEN_OUT:
> -        iov_to_buf(p->iov.iov, p->iov.niov, ptr, p->result, bytes);
> +        iov_to_buf(p->iov.iov, p->iov.niov, p->result, ptr, bytes);
>           break;
>       case USB_TOKEN_IN:
> -        iov_from_buf(p->iov.iov, p->iov.niov, ptr, p->result, bytes);
> +        iov_from_buf(p->iov.iov, p->iov.niov, p->result, ptr, bytes);
>           break;
>       default:
>           fprintf(stderr, "%s: invalid pid: %x\n", __func__, p->pid);
> @@ -533,7 +533,7 @@ void usb_packet_skip(USBPacket *p, size_t bytes)
>       assert(p->result>= 0);
>       assert(p->result + bytes<= p->iov.size);
>       if (p->pid == USB_TOKEN_IN) {
> -        iov_clear(p->iov.iov, p->iov.niov, p->result, bytes);
> +        iov_memset(p->iov.iov, p->iov.niov, p->result, 0, bytes);
>       }
>       p->result += bytes;
>   }
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index ce9d2c9..8f0cf33 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -77,7 +77,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>           size_t offset = 0;
>           uint32_t pfn;
>
> -        while (iov_to_buf(elem.out_sg, elem.out_num,&pfn, offset, 4) == 4) {
> +        while (iov_to_buf(elem.out_sg, elem.out_num, offset,&pfn, 4) == 4) {
>               ram_addr_t pa;
>               ram_addr_t addr;
>
> @@ -118,7 +118,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>        */
>       reset_stats(s);
>
> -    while (iov_to_buf(elem->out_sg, elem->out_num,&stat, offset, sizeof(stat))
> +    while (iov_to_buf(elem->out_sg, elem->out_num, offset,&stat, sizeof(stat))
>              == sizeof(stat)) {
>           uint16_t tag = tswap16(stat.tag);
>           uint64_t val = tswap64(stat.val);
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index bc5e3a8..65a516f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -656,8 +656,8 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>           }
>
>           /* copy in packet.  ugh */
> -        len = iov_from_buf(sg, elem.in_num,
> -                           buf + offset, 0, size - offset);
> +        len = iov_from_buf(sg, elem.in_num, 0,
> +                           buf + offset, size - offset);
>           total += len;
>           offset += len;
>           /* If buffers can't be merged, at this point we
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index abe48ec..41a62d1 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -106,8 +106,8 @@ static size_t write_to_port(VirtIOSerialPort *port,
>               break;
>           }
>
> -        len = iov_from_buf(elem.in_sg, elem.in_num,
> -                           buf + offset, 0, size - offset);
> +        len = iov_from_buf(elem.in_sg, elem.in_num, 0,
> +                           buf + offset, size - offset);
>           offset += len;
>
>           virtqueue_push(vq,&elem, len);
> @@ -467,7 +467,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>               buf = g_malloc(cur_len);
>               len = cur_len;
>           }
> -        iov_to_buf(elem.out_sg, elem.out_num, buf, 0, cur_len);
> +        iov_to_buf(elem.out_sg, elem.out_num, 0, buf, cur_len);
>
>           handle_control_message(vser, buf, cur_len);
>           virtqueue_push(vq,&elem, 0);
> diff --git a/iov.c b/iov.c
> index 0f96493..bc58cab 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -17,8 +17,8 @@
>
>   #include "iov.h"
>
> -size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
> -                    const void *buf, size_t iov_off, size_t size)
> +size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, size_t iov_off,
> +                    const void *buf, size_t size)
>   {
>       size_t iovec_off, buf_off;
>       unsigned int i;
> @@ -40,8 +40,8 @@ size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
>       return buf_off;
>   }
>
> -size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> -                  void *buf, size_t iov_off, size_t size)
> +size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt, size_t iov_off,
> +                  void *buf, size_t size)
>   {
>       uint8_t *ptr;
>       size_t iovec_off, buf_off;
> @@ -65,8 +65,8 @@ size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
>       return buf_off;
>   }
>
> -size_t iov_clear(const struct iovec *iov, const unsigned int iov_cnt,
> -                 size_t iov_off, size_t size)
> +size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
> +                 size_t iov_off, int fillc, size_t size)
>   {
>       size_t iovec_off, buf_off;
>       unsigned int i;
> @@ -77,7 +77,7 @@ size_t iov_clear(const struct iovec *iov, const unsigned int iov_cnt,
>           if (iov_off<  (iovec_off + iov[i].iov_len)) {
>               size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size);
>
> -            memset(iov[i].iov_base + (iov_off - iovec_off), 0, len);
> +            memset(iov[i].iov_base + (iov_off - iovec_off), fillc, len);
>
>               buf_off += len;
>               iov_off += len;
> diff --git a/iov.h b/iov.h
> index 94d2f78..0b4acf4 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -12,12 +12,44 @@
>
>   #include "qemu-common.h"
>
> +/**
> + * count and return data size, in bytes, of an iovec
> + * starting at `iov' of `iov_cnt' number of elements.
> + */
> +size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
> +
> +/**
> + * Copy from single continuous buffer to scatter-gather vector of buffers
> + * (iovec) and back like memcpy() between two continuous memory regions.
> + * Data in single continuous buffer starting at address `buf' and
> + * `bytes' bytes long will be copied to/from an iovec `iov' with
> + * `iov_cnt' number of elements, starting at byte position `offset'
> + * within the iovec.  If the iovec does not contain enough space,
> + * only part of data will be copied, up to the end of the iovec.
> + * Number of bytes actually copied will be returned, which is
> + *  min(bytes, iov_size(iov)-offset)
> + */

But if you're adding documentation (especially to ever function: hint) then I'd 
be willing to take the above change as a compromise.

However, this should be in gtk-doc format.

Regards,

Anthony Liguori