From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38675) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8ZoL-0004Wx-Q4 for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:15:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S8Zny-0005H1-0C for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:15:25 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:54784) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8Znx-0005G1-RB for qemu-devel@nongnu.org; Fri, 16 Mar 2012 12:15:01 -0400 Received: by yhoo21 with SMTP id o21so5052604yho.4 for ; Fri, 16 Mar 2012 09:15:00 -0700 (PDT) Message-ID: <4F636700.9020201@codemonkey.ws> Date: Fri, 16 Mar 2012 11:14:56 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1331845217-21705-1-git-send-email-mjt@msgid.tls.msk.ru> <1331845217-21705-3-git-send-email-mjt@msgid.tls.msk.ru> In-Reply-To: <1331845217-21705-3-git-send-email-mjt@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4 02/11] change iov_* function prototypes to be more appropriate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: Paolo Bonzini , qemu-devel@nongnu.org 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 > --- > 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