All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions
@ 2012-03-12 19:14 Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions Michael Tokarev
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

his is a little cleanup/consolidation for some iovec-related
low-level routines in qemu.

The plan is to make library functions more understandable,
consistent and useful, and to drop numerous implementations
of the same thing.

The patch changes prototypes of several iov and qiov functions
to match each other, changes types of arguments for some
functions, _swaps_ some function arguments with each other,
and makes use of common code in r/w path.

The result of all these changes.

1. Most qiov-related (qemu_iovec_*) functions now accepts
 'offset' parameter to specify from which (byte) position to
 start operation.  This is added for _memset (removing
 _memset_skip), _from_buffer (allowing to copy a bounce-
 buffer to a middle of qiov).  Typical:

  void qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, int c, size_t bytes);

2. All functions that accepts this `offset' argument does
 it in a similar manner, following the
   iov, fromwhere, what, bytes
 pattern.  This is consistent with (updated) qemu_sendv()
 and qemu_recvv() and friends, where `offset' and `bytes'
 arguments were _renamed_, with the following prototypes:

   ssize_t qemu_sendv(sockfd, iov, size_t offset, size_t bytes)

 instead of

   int qemu_sendv(sockfd, iov, int len, int iov_offset)

 See how offset & bytes are used in the same way as for iov_*
 and qemu_iovec_*.   A few callers of these are verified and
 converted.

3. Used size_t instead of various variations for byte counts.
 Including qemu_iovec_copy which used uint64_t(!) type.

4. Function arguments are renamed to better match with their
 actual meaning.  Compare new and original prototype of
 qemu_sendv() above: old prototype with `len' does not
 tell if `len' refers to number of iov elements (as
 regular writev() call) or to number of data bytes.
 Ditto for several usages of `count' for some qemu_iovec_*,
 which is also replaced to `bytes'.

5. One implementation of the code remain. For example,
 qemu_iovec_from_buffer() uses iov_from_buf() directly,
 instead of repeating the same code.

The resulting function usage is much more consistent, the
functions themselves are nice and understandable, which
means they're easier to use and less error-prone.

This patchset also consolidates a few low-level send&recv
functions into one, since both versions were exactly
the same (and were finally calling common function anyway).
This is done by exporting a common send_recv function with
one extra bool argument, and making current send&recv to
be just #defines.

And while at it all, also made some implementations shorter,
cleaner and much easier to read/understand, and add some
code comments.

The read&write consolidation has great potential for the
block layer, as has been demonstrated before.
Unification and generalization of qemu_iovec_* functions
will let to optimize/simplify some more code in block/*,
especially qemu_iovec_memset() and _from_buffer() (this
optimization/simplification is already used in qcow2.c
a bit).

The resulting thing should behave like current/existing
code, there should be no behavor change at all, just
some shuffling.  It has been tested slightly, by booting
different guests out of different image formats and doing
some i/o, after each of the 9 patches, and it all works
correctly so far.

The patchset depends on a tiny patch I sent to Amit Shah,
"virtio-serial-bus: use correct lengths in control_out() message"
but the only conflict without that patch is in
hw/virtio-serial-bus.c and it is trivial to resolve it.

Changes since v2:

- covered iov.[ch] with iov_*() functions which contained
  similar functionality
- changed tabs to spaces as per suggestion by Kevin
- explicitly allow to use large sizes for frombuf/tobuf
  functions and friends, stopping at the end of iovec
  in case more bytes requested when available, and
  _returning_ number of actual bytes processed, but
  made it extra clear what a return value will be.
- used ssize_t for sendv/recvv instead of int, to
  be consistent with all system headers and other
  places in qemu
- fix/clarify a case in my qemu_co_sendv_recvv()
  of handling zero reads(recvs) and writes(sends).
- covered qemu_iovec_to_buf() to have the same
  pattern as other too (was missing in v2), and
  use it in qcow2.c right away to simplify things
- spotted and removed wrong usage of qiov instead of
  plain iov in posix-aio-compat.c

What is _not_ covered:

- suggestion by pbonzini to not use macros for
  access methods to call sendv_recvv with an
  extra true/false argument: I still think the
  usage of macros here is better than anything
  else.
- maybe re-shuffling of all this stuff into
  different headers -- we already have iov.h,
  maybe rename it to qemu-iov.h for consistency
  and move all iov-related functions into there
  (and ditto for cutils.c => (qemu-)iov.c).

Michael Tokarev (9):
  refresh iov_* functions
  consolidate qemu_iovec_memset{,_skip}() into single function and use existing iov_memset()
  allow qemu_iovec_from_buffer() to specify offset from which to start copying
  consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent
  change qemu_iovec_to_buf() to match other to,from_buf functions
  change prototypes of qemu_sendv() and qemu_recvv()
  export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()
  cleanup qemu_co_sendv(), qemu_co_recvv() and friends
  rewrite and comment qemu_sendv_recvv()

 block.c                |   12 +-
 block/curl.c           |    6 +-
 block/iscsi.c          |    2 +-
 block/nbd.c            |    4 +-
 block/qcow.c           |    4 +-
 block/qcow2.c          |   19 ++--
 block/qed.c            |   10 +-
 block/rbd.c            |    2 +-
 block/sheepdog.c       |    6 +-
 block/vdi.c            |    4 +-
 cutils.c               |  261 +++++++++++++++++------------------------------
 hw/9pfs/virtio-9p.c    |    8 +-
 hw/rtl8139.c           |    2 +-
 hw/usb.c               |    6 +-
 hw/virtio-balloon.c    |    4 +-
 hw/virtio-net.c        |    4 +-
 hw/virtio-serial-bus.c |    6 +-
 iov.c                  |   89 +++++++----------
 iov.h                  |   47 ++++++++-
 linux-aio.c            |    4 +-
 net.c                  |    2 +-
 posix-aio-compat.c     |    8 +-
 qemu-common.h          |   69 +++++++------
 qemu-coroutine-io.c    |   82 +++++-----------
 24 files changed, 293 insertions(+), 368 deletions(-)

-- 
1.7.9.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 17:44   ` Paolo Bonzini
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() Michael Tokarev
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

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:

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

All iov_* functions now ensure that this offset
argument is within the iovec (using assertion),
but lets to specify `bytes' value larger than
actual length of the iovec - in this case they
stops at the actual end of iovec.  It is also
suggested to use convinient `-1' value as `bytes'
to mean just this -- "up to the end".

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.

There's one very minor semantic change here: new
requiriment is that `offset' points to inside of
iovec.  This is checked just at the end of functions
(assert()), it does not actually need to be enforced,
but using any of these functions with offset pointing
past the end of iovec is wrong anyway.

Note: the new code in iov.c uses arithmetic with void
pointers.  I thought this is not supported everywhere
and is a GCC extension (indeed, the C standard does
not define void arithmetic).  However, the original
code already use void arith in iov_from_buf() function
(memcpy(..., buf + buf_off,...)) which apparently works
well so far (it is this way in 1.0).  So I left it
this way.

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.c               |    6 ++--
 hw/virtio-balloon.c    |    4 +-
 hw/virtio-net.c        |    4 +-
 hw/virtio-serial-bus.c |    6 ++--
 iov.c                  |   89 ++++++++++++++++++++---------------------------
 iov.h                  |   47 ++++++++++++++++++++++---
 net.c                  |    2 +-
 8 files changed, 92 insertions(+), 68 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.c b/hw/usb.c
index 57fc5e3..4148c8d 100644
--- a/hw/usb.c
+++ b/hw/usb.c
@@ -429,10 +429,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);
@@ -446,7 +446,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..fd518dd 100644
--- a/iov.c
+++ b/iov.c
@@ -7,6 +7,7 @@
  * Author(s):
  *  Anthony Liguori <aliguori@us.ibm.com>
  *  Amit Shah <amit.shah@redhat.com>
+ *  Michael Tokarev <mjt@tls.msk.ru>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -18,74 +19,60 @@
 #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 offset, const void *buf, size_t bytes)
 {
-    size_t iovec_off, buf_off;
+    size_t done;
     unsigned int i;
-
-    iovec_off = 0;
-    buf_off = 0;
-    for (i = 0; i < iov_cnt && size; i++) {
-        if (iov_off < (iovec_off + iov[i].iov_len)) {
-            size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off, size);
-
-            memcpy(iov[i].iov_base + (iov_off - iovec_off), buf + buf_off, len);
-
-            buf_off += len;
-            iov_off += len;
-            size -= len;
+    for (i = 0, done = 0; done < bytes && i < iov_cnt; i++) {
+        if (offset < iov[i].iov_len) {
+            size_t len = MIN(iov[i].iov_len - offset, bytes - done);
+            memcpy(iov[i].iov_base + offset, buf + done, len);
+            done += len;
+            offset = 0;
+        } else {
+            offset -= iov[i].iov_len;
         }
-        iovec_off += iov[i].iov_len;
     }
-    return buf_off;
+    assert(offset == 0);
+    return done;
 }
 
 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 offset, void *buf, size_t bytes)
 {
-    uint8_t *ptr;
-    size_t iovec_off, buf_off;
+    size_t done;
     unsigned int i;
-
-    ptr = buf;
-    iovec_off = 0;
-    buf_off = 0;
-    for (i = 0; i < iov_cnt && size; i++) {
-        if (iov_off < (iovec_off + iov[i].iov_len)) {
-            size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size);
-
-            memcpy(ptr + buf_off, iov[i].iov_base + (iov_off - iovec_off), len);
-
-            buf_off += len;
-            iov_off += len;
-            size -= len;
+    for (i = 0, done = 0; done < bytes && i < iov_cnt; i++) {
+        if (offset < iov[i].iov_len) {
+            size_t len = MIN(iov[i].iov_len - offset, bytes - done);
+            memcpy(buf + done, iov[i].iov_base + offset, len);
+            done += len;
+            offset = 0;
+        } else {
+            offset -= iov[i].iov_len;
         }
-        iovec_off += iov[i].iov_len;
     }
-    return buf_off;
+    assert(offset == 0);
+    return done;
 }
 
-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 offset, int fillc, size_t bytes)
 {
-    size_t iovec_off, buf_off;
+    size_t done;
     unsigned int i;
-
-    iovec_off = 0;
-    buf_off = 0;
-    for (i = 0; i < iov_cnt && size; i++) {
-        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);
-
-            buf_off += len;
-            iov_off += len;
-            size -= len;
+    for (i = 0, done = 0; done < bytes && i < iov_cnt; i++) {
+        if (offset < iov[i].iov_len) {
+            size_t len = MIN(iov[i].iov_len - offset, bytes - done);
+            memset(iov[i].iov_base + offset, fillc, len);
+            done += len;
+            offset = 0;
+        } else {
+            offset -= iov[i].iov_len;
         }
-        iovec_off += iov[i].iov_len;
     }
-    return buf_off;
+    assert(offset == 0);
+    return done;
 }
 
 size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt)
diff --git a/iov.h b/iov.h
index 94d2f78..d3815eb 100644
--- a/iov.h
+++ b/iov.h
@@ -12,12 +12,49 @@
 
 #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)
+ * It is okay to use very large value for `bytes' since we're
+ * limited by the size of the iovec anyway, provided that the
+ * buffer pointed to by buf has enough space.  One possible
+ * such "large" value is -1 (sinice size_t is unsigned),
+ * so specifying `-1' as `bytes' means 'up to the end of iovec'.
+ */
 size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
-                    const void *buf, size_t iov_off, size_t size);
+                    size_t offset, const void *buf, size_t bytes);
 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_size(const struct iovec *iov, const unsigned int iov_cnt);
-size_t iov_clear(const struct iovec *iov, const unsigned int iov_cnt,
-                 size_t iov_off, size_t size);
+                  size_t offset, void *buf, size_t bytes);
+
+/**
+ * Set data bytes pointed out by iovec `iov' of size `iov_cnt' elements,
+ * starting at byte offset `start', to value `fillc', repeating it
+ * `bytes' number of times.  `Offset' must point inside of iovec.
+ * If `bytes' is large enough, only last bytes portion of iovec,
+ * up to the end of it, will be filled with the specified value.
+ * Function return actual number of bytes processed, which is
+ * min(size, iov_size(iov) - offset).
+ */
+size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
+                  size_t offset, int fillc, size_t bytes);
+
+/**
+ * Produce a text hexdump of iovec `iov' with `iov_cnt' number of elements
+ * in file `fp', prefixing each line with `prefix' and processing not more
+ * than `limit' data bytes.
+ */
 void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
                  FILE *fp, const char *prefix, size_t limit);
diff --git a/net.c b/net.c
index c34474f..ffd9ac8 100644
--- a/net.c
+++ b/net.c
@@ -544,7 +544,7 @@ static ssize_t vc_sendv_compat(VLANClientState *vc, const struct iovec *iov,
     uint8_t buffer[4096];
     size_t offset;
 
-    offset = iov_to_buf(iov, iovcnt, buffer, 0, sizeof(buffer));
+    offset = iov_to_buf(iov, iovcnt, 0, buffer, sizeof(buffer));
 
     return vc->info->receive(vc, buffer, offset);
 }
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset()
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 17:46   ` Paolo Bonzini
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying Michael Tokarev
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

This patch combines two functions into one, and replaces
the implementation with already existing iov_memset() from
iov.c.

The new prototype of qemu_iovec_memset():
  size_t qemu_iovec_memset(qiov, size_t offset, int c, size_t bytes)
It is different from former qemu_iovec_memset_skip(), and
I want to make other functions to be consistent with it
too: first how much to skip, second what, and 3rd how many
of it.  It also returns actual number of bytes filled in,
which may be less than the requested `bytes' if qiov is
smaller than offset+bytes, in the same way iov_memset()
does.

While at it, use utility function iov_memset() from
iov.h in posix-aio-compat.c, where qiov was used.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block/qcow2.c      |    4 ++--
 block/qed.c        |    4 ++--
 cutils.c           |   44 ++++----------------------------------------
 linux-aio.c        |    4 ++--
 posix-aio-compat.c |    8 +++-----
 qemu-common.h      |    5 ++---
 6 files changed, 15 insertions(+), 54 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index eb5ea48..d46ca70 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -406,7 +406,7 @@ int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
     else
         n1 = bs->total_sectors - sector_num;
 
-    qemu_iovec_memset_skip(qiov, 0, 512 * (nb_sectors - n1), 512 * n1);
+    qemu_iovec_memset(qiov, 512 * n1, 0, 512 * (nb_sectors - n1));
 
     return n1;
 }
@@ -466,7 +466,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 }
             } else {
                 /* Note: in this case, no need to wait */
-                qemu_iovec_memset(&hd_qiov, 0, 512 * cur_nr_sectors);
+                qemu_iovec_memset(&hd_qiov, 0, 0, 512 * cur_nr_sectors);
             }
         } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
             /* add AIO support for compressed blocks ? */
diff --git a/block/qed.c b/block/qed.c
index a041d31..6f9325b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -738,7 +738,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     /* Zero all sectors if reading beyond the end of the backing file */
     if (pos >= backing_length ||
         pos + qiov->size > backing_length) {
-        qemu_iovec_memset(qiov, 0, qiov->size);
+        qemu_iovec_memset(qiov, 0, 0, qiov->size);
     }
 
     /* Complete now if there are no backing file sectors to read */
@@ -1253,7 +1253,7 @@ static void qed_aio_read_data(void *opaque, int ret,
 
     /* Handle zero cluster and backing file reads */
     if (ret == QED_CLUSTER_ZERO) {
-        qemu_iovec_memset(&acb->cur_qiov, 0, acb->cur_qiov.size);
+        qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size);
         qed_aio_next_io(acb, 0);
         return;
     } else if (ret != QED_CLUSTER_FOUND) {
diff --git a/cutils.c b/cutils.c
index af308cd..53ff825 100644
--- a/cutils.c
+++ b/cutils.c
@@ -26,6 +26,7 @@
 #include <math.h>
 
 #include "qemu_socket.h"
+#include "iov.h"
 
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
@@ -260,47 +261,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
     }
 }
 
-void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count)
+size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
+                         int c, size_t bytes)
 {
-    size_t n;
-    int i;
-
-    for (i = 0; i < qiov->niov && count; ++i) {
-        n = MIN(count, qiov->iov[i].iov_len);
-        memset(qiov->iov[i].iov_base, c, n);
-        count -= n;
-    }
-}
-
-void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
-                            size_t skip)
-{
-    int i;
-    size_t done;
-    void *iov_base;
-    uint64_t iov_len;
-
-    done = 0;
-    for (i = 0; (i < qiov->niov) && (done != count); i++) {
-        if (skip >= qiov->iov[i].iov_len) {
-            /* Skip the whole iov */
-            skip -= qiov->iov[i].iov_len;
-            continue;
-        } else {
-            /* Skip only part (or nothing) of the iov */
-            iov_base = (uint8_t*) qiov->iov[i].iov_base + skip;
-            iov_len = qiov->iov[i].iov_len - skip;
-            skip = 0;
-        }
-
-        if (done + iov_len > count) {
-            memset(iov_base, c, count - done);
-            break;
-        } else {
-            memset(iov_base, c, iov_len);
-        }
-        done += iov_len;
-    }
+    return iov_memset(qiov->iov, qiov->niov, offset, c, bytes);
 }
 
 /*
diff --git a/linux-aio.c b/linux-aio.c
index d2fc2e7..5a46c13 100644
--- a/linux-aio.c
+++ b/linux-aio.c
@@ -64,8 +64,8 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,
         } else if (ret >= 0) {
             /* Short reads mean EOF, pad with zeros. */
             if (laiocb->is_read) {
-                qemu_iovec_memset_skip(laiocb->qiov, 0,
-                    laiocb->qiov->size - ret, ret);
+                qemu_iovec_memset(laiocb->qiov, ret, 0,
+                    laiocb->qiov->size - ret);
             } else {
                 ret = -EINVAL;
             }
diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index d311d13..1ab4a18 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -29,6 +29,7 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "iov.h"
 
 #include "block/raw-posix-aio.h"
 
@@ -351,11 +352,8 @@ static void *aio_thread(void *unused)
             if (ret >= 0 && ret < aiocb->aio_nbytes && aiocb->common.bs->growable) {
                 /* A short read means that we have reached EOF. Pad the buffer
                  * with zeros for bytes after EOF. */
-                QEMUIOVector qiov;
-
-                qemu_iovec_init_external(&qiov, aiocb->aio_iov,
-                                         aiocb->aio_niov);
-                qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - ret, ret);
+                iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret,
+                           0, aiocb->aio_nbytes - ret);
 
                 ret = aiocb->aio_nbytes;
             }
diff --git a/qemu-common.h b/qemu-common.h
index dbfce6f..90f17fe 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -340,9 +340,8 @@ void qemu_iovec_destroy(QEMUIOVector *qiov);
 void qemu_iovec_reset(QEMUIOVector *qiov);
 void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
 void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
-void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count);
-void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
-                            size_t skip);
+size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
+                         int c, size_t bytes);
 
 bool buffer_is_zero(const void *buf, size_t len);
 
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 11:10   ` Kevin Wolf
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent Michael Tokarev
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Michael Tokarev

Similar to
 qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
                   int c, size_t bytes);
the new prototype is:
 qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
                     const void *buf, size_t bytes);

The processing starts at offset bytes within qiov.

This way, we may copy a bounce buffer directly to
a middle of qiov.

This is exactly the same function as iov_from_buf() from
iov.c, so use the existing implementation and rename it
to qemu_iovec_from_buf() to be shorter and to match the
utility function.

As with utility implementation, we now assert that the
offset is inside actual iovec.  Nothing changed for
current callers, because `offset' parameter is new.

While at it, stop using "bounce-qiov" in block/qcow2.c
and copy decrypted data directly from cluster_data
instead of recreating a temp qiov for doing that
(Cc'ing kwolf for this change).

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: Kevin Wolf <kwolf@redhat.com>
---
 block.c       |    6 +++---
 block/curl.c  |    6 +++---
 block/qcow.c  |    2 +-
 block/qcow2.c |    9 +++------
 block/vdi.c   |    2 +-
 cutils.c      |   16 +++-------------
 qemu-common.h |    3 ++-
 7 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index 52ffe14..f4320be 100644
--- a/block.c
+++ b/block.c
@@ -1692,8 +1692,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
     }
 
     skip_bytes = (sector_num - cluster_sector_num) * BDRV_SECTOR_SIZE;
-    qemu_iovec_from_buffer(qiov, bounce_buffer + skip_bytes,
-                           nb_sectors * BDRV_SECTOR_SIZE);
+    qemu_iovec_from_buf(qiov, 0, bounce_buffer + skip_bytes,
+                        nb_sectors * BDRV_SECTOR_SIZE);
 
 err:
     qemu_vfree(bounce_buffer);
@@ -3240,7 +3240,7 @@ static void bdrv_aio_bh_cb(void *opaque)
     BlockDriverAIOCBSync *acb = opaque;
 
     if (!acb->is_write)
-        qemu_iovec_from_buffer(acb->qiov, acb->bounce, acb->qiov->size);
+        qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
     qemu_vfree(acb->bounce);
     acb->common.cb(acb->common.opaque, acb->ret);
     qemu_bh_delete(acb->bh);
diff --git a/block/curl.c b/block/curl.c
index e9102e3..cfc2ced 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -142,8 +142,8 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
             continue;
 
         if ((s->buf_off >= acb->end)) {
-            qemu_iovec_from_buffer(acb->qiov, s->orig_buf + acb->start,
-                                   acb->end - acb->start);
+            qemu_iovec_from_buf(acb->qiov, 0, s->orig_buf + acb->start,
+                                acb->end - acb->start);
             acb->common.cb(acb->common.opaque, 0);
             qemu_aio_release(acb);
             s->acb[i] = NULL;
@@ -178,7 +178,7 @@ static int curl_find_buf(BDRVCURLState *s, size_t start, size_t len,
         {
             char *buf = state->orig_buf + (start - state->buf_start);
 
-            qemu_iovec_from_buffer(acb->qiov, buf, len);
+            qemu_iovec_from_buf(acb->qiov, 0, buf, len);
             acb->common.cb(acb->common.opaque, 0);
 
             return FIND_RET_OK;
diff --git a/block/qcow.c b/block/qcow.c
index b1cfe1f..562a19c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -540,7 +540,7 @@ done:
     qemu_co_mutex_unlock(&s->lock);
 
     if (qiov->niov > 1) {
-        qemu_iovec_from_buffer(qiov, orig_buf, qiov->size);
+        qemu_iovec_from_buf(qiov, 0, orig_buf, qiov->size);
         qemu_vfree(orig_buf);
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index d46ca70..106f2ce 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -475,7 +475,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 goto fail;
             }
 
-            qemu_iovec_from_buffer(&hd_qiov,
+            qemu_iovec_from_buf(&hd_qiov, 0,
                 s->cluster_cache + index_in_cluster * 512,
                 512 * cur_nr_sectors);
         } else {
@@ -513,11 +513,8 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
             if (s->crypt_method) {
                 qcow2_encrypt_sectors(s, sector_num,  cluster_data,
                     cluster_data, cur_nr_sectors, 0, &s->aes_decrypt_key);
-                qemu_iovec_reset(&hd_qiov);
-                qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
-                    cur_nr_sectors * 512);
-                qemu_iovec_from_buffer(&hd_qiov, cluster_data,
-                    512 * cur_nr_sectors);
+                qemu_iovec_from_buf(qiov, bytes_done,
+                    cluster_data, 512 * cur_nr_sectors);
             }
         }
 
diff --git a/block/vdi.c b/block/vdi.c
index 6a0011f..24f4027 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -635,7 +635,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
     return;
 done:
     if (acb->qiov->niov > 1) {
-        qemu_iovec_from_buffer(acb->qiov, acb->orig_buf, acb->qiov->size);
+        qemu_iovec_from_buf(acb->qiov, 0, acb->orig_buf, acb->qiov->size);
         qemu_vfree(acb->orig_buf);
     }
     acb->common.cb(acb->common.opaque, ret);
diff --git a/cutils.c b/cutils.c
index 53ff825..1c93ed4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -245,20 +245,10 @@ void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf)
     }
 }
 
-void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
+size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
+                           const void *buf, size_t bytes)
 {
-    const uint8_t *p = (const uint8_t *)buf;
-    size_t copy;
-    int i;
-
-    for (i = 0; i < qiov->niov && count; ++i) {
-        copy = count;
-        if (copy > qiov->iov[i].iov_len)
-            copy = qiov->iov[i].iov_len;
-        memcpy(qiov->iov[i].iov_base, p, copy);
-        p     += copy;
-        count -= copy;
-    }
+    return iov_from_buf(qiov->iov, qiov->niov, offset, buf, bytes);
 }
 
 size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
diff --git a/qemu-common.h b/qemu-common.h
index 90f17fe..2cbb513 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -339,7 +339,8 @@ void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size);
 void qemu_iovec_destroy(QEMUIOVector *qiov);
 void qemu_iovec_reset(QEMUIOVector *qiov);
 void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
-void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
+size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
+                           const void *buf, size_t bytes);
 size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
                          int c, size_t bytes);
 
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (2 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 18:02   ` Paolo Bonzini
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions Michael Tokarev
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

qemu_iovec_concat() is currently a wrapper for
qemu_iovec_copy(), use the former (with extra
"0" arg) in a few places where it is used.

Change skip argument of qemu_iovec_copy() from
uint64_t to size_t, since size of qiov itself
is size_t, so there's no way to skip larger
sizes.  Rename it to soffset, to make it clear
that the offset is applied to src.

Also change the only usage of uint64_t in
hw/9pfs/virtio-9p.c, in v9fs_init_qiov_from_pdu() -
all callers of it actually uses size_t too,
not uint64_t.

One added restriction: as for all other iovec-related
functions, soffset must point inside src.

Order of argumens is already good:
 qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
                   int c, size_t bytes)
vs:
 qemu_iovec_concat(QEMUIOVector *dst,
                   QEMUIOVector *src,
                   size_t soffset, size_t sbytes)
(note soffset is after _src_ not dst, since it applies to src;
for memset it applies to qiov).

Note that in many places where this function is used,
the previous call is qemu_iovec_reset(), which means
many callers actually want copy (replacing dst content),
not concat.  So we may want to add a wrapper like
qemu_iovec_copy() with the same arguments but which
calls qemu_iovec_reset() before _concat().  This needs
to be done later, so that any current out-of-tree code
which uses _copy().

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c             |    4 +-
 block/qcow2.c       |    4 +-
 block/qed.c         |    6 ++--
 cutils.c            |   54 ++++++++++++++++++--------------------------------
 hw/9pfs/virtio-9p.c |    8 +++---
 qemu-common.h       |    5 +--
 6 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/block.c b/block.c
index f4320be..9246518 100644
--- a/block.c
+++ b/block.c
@@ -2959,13 +2959,13 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
             // Add the first request to the merged one. If the requests are
             // overlapping, drop the last sectors of the first request.
             size = (reqs[i].sector - reqs[outidx].sector) << 9;
-            qemu_iovec_concat(qiov, reqs[outidx].qiov, size);
+            qemu_iovec_concat(qiov, reqs[outidx].qiov, 0, size);
 
             // We should need to add any zeros between the two requests
             assert (reqs[i].sector <= oldreq_last);
 
             // Add the second request
-            qemu_iovec_concat(qiov, reqs[i].qiov, reqs[i].qiov->size);
+            qemu_iovec_concat(qiov, reqs[i].qiov, 0, reqs[i].qiov->size);
 
             reqs[outidx].nb_sectors = qiov->size >> 9;
             reqs[outidx].qiov = qiov;
diff --git a/block/qcow2.c b/block/qcow2.c
index 106f2ce..894fb4d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -445,7 +445,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
             cur_nr_sectors * 512);
 
         if (!cluster_offset) {
@@ -593,7 +593,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
         assert((cluster_offset & 511) == 0);
 
         qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
             cur_nr_sectors * 512);
 
         if (s->crypt_method) {
diff --git a/block/qed.c b/block/qed.c
index 6f9325b..a4ef12c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1133,7 +1133,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 
     acb->cur_nclusters = qed_bytes_to_clusters(s,
             qed_offset_into_cluster(s, acb->cur_pos) + len);
-    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
+    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     if (acb->flags & QED_AIOCB_ZERO) {
         /* Skip ahead if the clusters are already zero */
@@ -1179,7 +1179,7 @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
 
     /* Calculate the I/O vector */
     acb->cur_cluster = offset;
-    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
+    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     /* Do the actual write */
     qed_aio_write_main(acb, 0);
@@ -1249,7 +1249,7 @@ static void qed_aio_read_data(void *opaque, int ret,
         goto err;
     }
 
-    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
+    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     /* Handle zero cluster and backing file reads */
     if (ret == QED_CLUSTER_ZERO) {
diff --git a/cutils.c b/cutils.c
index 1c93ed4..b8fc954 100644
--- a/cutils.c
+++ b/cutils.c
@@ -172,48 +172,34 @@ void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len)
 }
 
 /*
- * Copies iovecs from src to the end of dst. It starts copying after skipping
- * the given number of bytes in src and copies until src is completely copied
- * or the total size of the copied iovec reaches size.The size of the last
- * copied iovec is changed in order to fit the specified total size if it isn't
- * a perfect fit already.
+ * Concatenates (partial) iovecs from src to the end of dst.
+ * It starts copying after skipping `soffset' bytes at the
+ * beginning of src and adds individual vectors from src to
+ * dst copies up to `sbytes' bytes total, or up to the end
+ * of src if it comes first.  This way, it is okay to specify
+ * very large value for `sbytes' to indicate "up to the end
+ * of src".
+ * Only vector pointers are processed, not the actual data buffers.
  */
-void qemu_iovec_copy(QEMUIOVector *dst, QEMUIOVector *src, uint64_t skip,
-    size_t size)
+void qemu_iovec_concat(QEMUIOVector *dst,
+                       QEMUIOVector *src, size_t soffset, size_t sbytes)
 {
     int i;
     size_t done;
-    void *iov_base;
-    uint64_t iov_len;
-
+    struct iovec *siov = src->iov;
     assert(dst->nalloc != -1);
-
-    done = 0;
-    for (i = 0; (i < src->niov) && (done != size); i++) {
-        if (skip >= src->iov[i].iov_len) {
-            /* Skip the whole iov */
-            skip -= src->iov[i].iov_len;
-            continue;
+    assert(src->size >= soffset);
+    for (i = 0, done = 0; done < sbytes && i < src->niov; i++) {
+        if (soffset < siov[i].iov_len) {
+            size_t len = MIN(siov[i].iov_len - soffset, sbytes - done);
+            qemu_iovec_add(dst, siov[i].iov_base + soffset, len);
+            done += len;
+            soffset = 0;
         } else {
-            /* Skip only part (or nothing) of the iov */
-            iov_base = (uint8_t*) src->iov[i].iov_base + skip;
-            iov_len = src->iov[i].iov_len - skip;
-            skip = 0;
+            soffset -= siov[i].iov_len;
         }
-
-        if (done + iov_len > size) {
-            qemu_iovec_add(dst, iov_base, size - done);
-            break;
-        } else {
-            qemu_iovec_add(dst, iov_base, iov_len);
-        }
-        done += iov_len;
     }
-}
-
-void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size)
-{
-    qemu_iovec_copy(dst, src, 0, size);
+    /* return done; */
 }
 
 void qemu_iovec_destroy(QEMUIOVector *qiov)
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index c633fb9..f4a7026 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1648,7 +1648,7 @@ out:
  * with qemu_iovec_destroy().
  */
 static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
-                                    uint64_t skip, size_t size,
+                                    size_t skip, size_t size,
                                     bool is_write)
 {
     QEMUIOVector elem;
@@ -1665,7 +1665,7 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
 
     qemu_iovec_init_external(&elem, iov, niov);
     qemu_iovec_init(qiov, niov);
-    qemu_iovec_copy(qiov, &elem, skip, size);
+    qemu_iovec_concat(qiov, &elem, skip, size);
 }
 
 static void v9fs_read(void *opaque)
@@ -1715,7 +1715,7 @@ static void v9fs_read(void *opaque)
         qemu_iovec_init(&qiov, qiov_full.niov);
         do {
             qemu_iovec_reset(&qiov);
-            qemu_iovec_copy(&qiov, &qiov_full, count, qiov_full.size - count);
+            qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count);
             if (0) {
                 print_sg(qiov.iov, qiov.niov);
             }
@@ -1970,7 +1970,7 @@ static void v9fs_write(void *opaque)
     qemu_iovec_init(&qiov, qiov_full.niov);
     do {
         qemu_iovec_reset(&qiov);
-        qemu_iovec_copy(&qiov, &qiov_full, total, qiov_full.size - total);
+        qemu_iovec_concat(&qiov, &qiov_full, total, qiov_full.size - total);
         if (0) {
             print_sg(qiov.iov, qiov.niov);
         }
diff --git a/qemu-common.h b/qemu-common.h
index 2cbb513..31b30b5 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -333,9 +333,8 @@ typedef struct QEMUIOVector {
 void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
 void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
-void qemu_iovec_copy(QEMUIOVector *dst, QEMUIOVector *src, uint64_t skip,
-    size_t size);
-void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size);
+void qemu_iovec_concat(QEMUIOVector *dst,
+                       QEMUIOVector *src, size_t soffset, size_t sbytes);
 void qemu_iovec_destroy(QEMUIOVector *qiov);
 void qemu_iovec_reset(QEMUIOVector *qiov);
 void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (3 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 18:02   ` Paolo Bonzini
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 6/9] change prototypes of qemu_sendv() and qemu_recvv() Michael Tokarev
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

It now allows specifying offset within qiov to start from and
amount of bytes to copy.  Actual implementation is just a call
to iov_to_buf().

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c       |    2 +-
 block/iscsi.c |    2 +-
 block/qcow.c  |    2 +-
 block/qcow2.c |    2 +-
 block/rbd.c   |    2 +-
 block/vdi.c   |    2 +-
 cutils.c      |   11 +++--------
 qemu-common.h |    3 ++-
 8 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 9246518..4bb5f92 100644
--- a/block.c
+++ b/block.c
@@ -3266,7 +3266,7 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
     acb->bh = qemu_bh_new(bdrv_aio_bh_cb, acb);
 
     if (is_write) {
-        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
+        qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
         acb->ret = bs->drv->bdrv_write(bs, sector_num, acb->bounce, nb_sectors);
     } else {
         acb->ret = bs->drv->bdrv_read(bs, sector_num, acb->bounce, nb_sectors);
diff --git a/block/iscsi.c b/block/iscsi.c
index bd3ca11..db589f5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -223,7 +223,7 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
     /* this will allow us to get rid of 'buf' completely */
     size = nb_sectors * BDRV_SECTOR_SIZE;
     acb->buf = g_malloc(size);
-    qemu_iovec_to_buffer(acb->qiov, acb->buf);
+    qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
     acb->task = iscsi_write10_task(iscsi, iscsilun->lun, acb->buf, size,
                               sector_qemu2lun(sector_num, iscsilun),
                               fua, 0, iscsilun->block_size,
diff --git a/block/qcow.c b/block/qcow.c
index 562a19c..a367459 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -569,7 +569,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
 
     if (qiov->niov > 1) {
         buf = orig_buf = qemu_blockalign(bs, qiov->size);
-        qemu_iovec_to_buffer(qiov, buf);
+        qemu_iovec_to_buf(qiov, 0, buf, qiov->size);
     } else {
         orig_buf = NULL;
         buf = (uint8_t *)qiov->iov->iov_base;
diff --git a/block/qcow2.c b/block/qcow2.c
index 894fb4d..88fbd73 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -604,7 +604,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 
             assert(hd_qiov.size <=
                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
-            qemu_iovec_to_buffer(&hd_qiov, cluster_data);
+            qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
 
             qcow2_encrypt_sectors(s, sector_num, cluster_data,
                 cluster_data, cur_nr_sectors, 1, &s->aes_encrypt_key);
diff --git a/block/rbd.c b/block/rbd.c
index 46a8579..0191f86 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -644,7 +644,7 @@ static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
     acb->bh = NULL;
 
     if (write) {
-        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
+        qemu_iovec_to_buffer(acb->qiov, 0, acb->bounce, qiov->size);
     }
 
     buf = acb->bounce;
diff --git a/block/vdi.c b/block/vdi.c
index 24f4027..30a5e27 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -524,7 +524,7 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
         acb->buf = qemu_blockalign(bs, qiov->size);
         acb->orig_buf = acb->buf;
         if (is_write) {
-            qemu_iovec_to_buffer(qiov, acb->buf);
+            qemu_iovec_to_buf(qiov, 0, acb->buf, qiov->size);
         }
     } else {
         acb->buf = (uint8_t *)qiov->iov->iov_base;
diff --git a/cutils.c b/cutils.c
index b8fc954..df26f72 100644
--- a/cutils.c
+++ b/cutils.c
@@ -220,15 +220,10 @@ void qemu_iovec_reset(QEMUIOVector *qiov)
     qiov->size = 0;
 }
 
-void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf)
+size_t qemu_iovec_to_buf(QEMUIOVector *qiov, size_t offset,
+                         void *buf, size_t bytes)
 {
-    uint8_t *p = (uint8_t *)buf;
-    int i;
-
-    for (i = 0; i < qiov->niov; ++i) {
-        memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
-        p += qiov->iov[i].iov_len;
-    }
+    return iov_to_buf(qiov->iov, qiov->niov, offset, buf, bytes);
 }
 
 size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
diff --git a/qemu-common.h b/qemu-common.h
index 31b30b5..ec9655b 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -337,7 +337,8 @@ void qemu_iovec_concat(QEMUIOVector *dst,
                        QEMUIOVector *src, size_t soffset, size_t sbytes);
 void qemu_iovec_destroy(QEMUIOVector *qiov);
 void qemu_iovec_reset(QEMUIOVector *qiov);
-void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
+size_t qemu_iovec_to_buf(QEMUIOVector *qiov, size_t offset,
+                         void *buf, size_t bytes);
 size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
                            const void *buf, size_t bytes);
 size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 6/9] change prototypes of qemu_sendv() and qemu_recvv()
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (4 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 7/9] export qemu_sendv_recvv() and use it in " Michael Tokarev
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

Rename arguments and use size_t for sizes instead of int,
from
 int
 qemu_sendv(int sockfd, struct iovec *iov,
            int len, int iov_offset)
to
 ssize_t
 qemu_sendv(int sockfd, struct iovec *iov,
            size_t offset, size_t bytes)

The main motivation was to make it clear that length
and offset are in _bytes_, not in iov elements: it was
very confusing before, because all standard functions
which deals with iovecs expects number of iovs, not
bytes, even the fact that struct iovec has iov_len and
iov_ prefix does not help.  With "bytes" and "offset",
especially since they're now size_t, it is much more
explicit.  Also change the return type to be ssize_t
instead of int.

This also changes it to match other iov-related functons,
but not _quite_: there's still no argument indicating
where iovec ends, ie, no iov_cnt parameter as used
in iov_size() and friends.  If it were to be added,
it should go before offset.

All callers of qemu_sendv() and qemu_recvv() and
related, like qemu_co_sendv() and qemu_co_recvv(),
were checked to verify that it is safe to use unsigned
datatype instead of int.

Note that the order of arguments is changed to: offset
and bytes (len and iov_offset) are swapped with each
other.  This is to make them consistent with very similar
functions from qemu_iovec family, where offset always
follows qiov, to mean the place in it to start from.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 cutils.c            |   37 +++++++++++++++++++------------------
 qemu-common.h       |    4 ++--
 qemu-coroutine-io.c |    4 ++--
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/cutils.c b/cutils.c
index df26f72..e9052fc 100644
--- a/cutils.c
+++ b/cutils.c
@@ -381,38 +381,39 @@ int qemu_parse_fd(const char *param)
  *
  * This function send/recv data from/to the iovec buffer directly.
  * The first `offset' bytes in the iovec buffer are skipped and next
- * `len' bytes are used.
+ * `bytes' bytes are used, which must be within data of iovec.
  *
- * For example,
+ *   r = do_sendv_recvv(sockfd, iov, offset, bytes, 1);
  *
- *   do_sendv_recvv(sockfd, iov, len, offset, 1);
+ * is logically equivalent to
  *
- * is equal to
- *
- *   char *buf = malloc(size);
- *   iov_to_buf(iov, iovcnt, buf, offset, size);
- *   send(sockfd, buf, size, 0);
+ *   char *buf = malloc(bytes);
+ *   iov_to_buf(iov, iovcnt, offset, buf, size);
+ *   r = send(sockfd, buf, size, 0);
  *   free(buf);
  */
-static int do_sendv_recvv(int sockfd, struct iovec *iov, int len, int offset,
+static ssize_t do_sendv_recvv(int sockfd, struct iovec *iov,
+                          size_t offset, size_t bytes,
                           int do_sendv)
 {
-    int ret, diff, iovlen;
+    int iovlen;
+    ssize_t ret;
+    size_t diff;
     struct iovec *last_iov;
 
     /* last_iov is inclusive, so count from one.  */
     iovlen = 1;
     last_iov = iov;
-    len += offset;
+    bytes += offset;
 
-    while (last_iov->iov_len < len) {
-        len -= last_iov->iov_len;
+    while (last_iov->iov_len < bytes) {
+        bytes -= last_iov->iov_len;
 
         last_iov++;
         iovlen++;
     }
 
-    diff = last_iov->iov_len - len;
+    diff = last_iov->iov_len - bytes;
     last_iov->iov_len -= diff;
 
     while (iov->iov_len <= offset) {
@@ -474,13 +475,13 @@ static int do_sendv_recvv(int sockfd, struct iovec *iov, int len, int offset,
     return ret;
 }
 
-int qemu_recvv(int sockfd, struct iovec *iov, int len, int iov_offset)
+ssize_t qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes)
 {
-    return do_sendv_recvv(sockfd, iov, len, iov_offset, 0);
+    return do_sendv_recvv(sockfd, iov, offset, bytes, 0);
 }
 
-int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset)
+ssize_t qemu_sendv(int sockfd, struct iovec *iov, size_t offset, size_t bytes)
 {
-    return do_sendv_recvv(sockfd, iov, len, iov_offset, 1);
+    return do_sendv_recvv(sockfd, iov, offset, bytes, 1);
 }
 
diff --git a/qemu-common.h b/qemu-common.h
index ec9655b..8d12fca 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -199,8 +199,8 @@ int qemu_pipe(int pipefd[2]);
 #define qemu_recv(sockfd, buf, len, flags) recv(sockfd, buf, len, flags)
 #endif
 
-int qemu_recvv(int sockfd, struct iovec *iov, int len, int iov_offset);
-int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset);
+ssize_t qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
+ssize_t qemu_sendv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
 
 /* Error handling.  */
 
diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c
index 40fd514..df8ff21 100644
--- a/qemu-coroutine-io.c
+++ b/qemu-coroutine-io.c
@@ -32,7 +32,7 @@ int coroutine_fn qemu_co_recvv(int sockfd, struct iovec *iov,
     int total = 0;
     int ret;
     while (len) {
-        ret = qemu_recvv(sockfd, iov, len, iov_offset + total);
+        ret = qemu_recvv(sockfd, iov, iov_offset + total, len);
         if (ret < 0) {
             if (errno == EAGAIN) {
                 qemu_coroutine_yield();
@@ -58,7 +58,7 @@ int coroutine_fn qemu_co_sendv(int sockfd, struct iovec *iov,
     int total = 0;
     int ret;
     while (len) {
-        ret = qemu_sendv(sockfd, iov, len, iov_offset + total);
+        ret = qemu_sendv(sockfd, iov, iov_offset + total, len);
         if (ret < 0) {
             if (errno == EAGAIN) {
                 qemu_coroutine_yield();
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 7/9] export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (5 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 6/9] change prototypes of qemu_sendv() and qemu_recvv() Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends Michael Tokarev
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

Rename do_sendv_recvv() to qemu_sendv_recvv(),
change its last arg (do_sendv) from int to bool,
export it in qemu-common.h, and made the two
callers of it (qemu_sendv() and qemu_recvv())
to be trivial #defines just adding 5th arg.

qemu_sendv_recvv() will be used later.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 cutils.c      |   19 ++++---------------
 qemu-common.h |   14 ++++++++++++--
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/cutils.c b/cutils.c
index e9052fc..be2173f 100644
--- a/cutils.c
+++ b/cutils.c
@@ -383,7 +383,7 @@ int qemu_parse_fd(const char *param)
  * The first `offset' bytes in the iovec buffer are skipped and next
  * `bytes' bytes are used, which must be within data of iovec.
  *
- *   r = do_sendv_recvv(sockfd, iov, offset, bytes, 1);
+ *   r = qemu_sendv_recvv(sockfd, iov, offset, butes, true);
  *
  * is logically equivalent to
  *
@@ -392,9 +392,9 @@ int qemu_parse_fd(const char *param)
  *   r = send(sockfd, buf, size, 0);
  *   free(buf);
  */
-static ssize_t do_sendv_recvv(int sockfd, struct iovec *iov,
-                          size_t offset, size_t bytes,
-                          int do_sendv)
+ssize_t qemu_sendv_recvv(int sockfd, struct iovec *iov,
+                         size_t offset, size_t bytes,
+                         bool do_sendv)
 {
     int iovlen;
     ssize_t ret;
@@ -474,14 +474,3 @@ static ssize_t do_sendv_recvv(int sockfd, struct iovec *iov,
     last_iov->iov_len += diff;
     return ret;
 }
-
-ssize_t qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes)
-{
-    return do_sendv_recvv(sockfd, iov, offset, bytes, 0);
-}
-
-ssize_t qemu_sendv(int sockfd, struct iovec *iov, size_t offset, size_t bytes)
-{
-    return do_sendv_recvv(sockfd, iov, offset, bytes, 1);
-}
-
diff --git a/qemu-common.h b/qemu-common.h
index 8d12fca..b01d84c 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -199,8 +199,18 @@ int qemu_pipe(int pipefd[2]);
 #define qemu_recv(sockfd, buf, len, flags) recv(sockfd, buf, len, flags)
 #endif
 
-ssize_t qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
-ssize_t qemu_sendv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
+/**
+ * Send or receive data from/to an (optionally partial) iovec.
+ * Instead of processing whole iovector, this routine can process
+ * not more than a specified number of bytes, and start not at
+ * the beginning of iovec but at byte position `offset'.
+ */
+ssize_t qemu_sendv_recvv(int sockfd, struct iovec *iov,
+                         size_t offset, size_t bytes, bool do_sendv);
+#define qemu_recvv(sockfd, iov, offset, bytes) \
+  qemu_sendv_recvv(sockfd, iov, offset, bytes, false)
+#define qemu_sendv(sockfd, iov, offset, bytes) \
+  qemu_sendv_recvv(sockfd, iov, offset, bytes, true)
 
 /* Error handling.  */
 
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (6 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 7/9] export qemu_sendv_recvv() and use it in " Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 17:52   ` Paolo Bonzini
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 9/9] rewrite and comment qemu_sendv_recvv() Michael Tokarev
  2012-03-13 16:09 ` [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
  9 siblings, 1 reply; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

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);
 }
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCHv3 9/9] rewrite and comment qemu_sendv_recvv()
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (7 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends Michael Tokarev
@ 2012-03-12 19:14 ` Michael Tokarev
  2012-03-13 16:09 ` [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
  9 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-12 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Michael Tokarev

Make it much more understandable, and add
comments to it.

The new implementation has been extensively
tested by splitting a large buffer into
many small randomly-sized chunks, sending
it over socket to another, slow process
and verifying the receiving data is the
same.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 cutils.c |  106 ++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/cutils.c b/cutils.c
index be2173f..7a19874 100644
--- a/cutils.c
+++ b/cutils.c
@@ -396,81 +396,85 @@ ssize_t qemu_sendv_recvv(int sockfd, struct iovec *iov,
                          size_t offset, size_t bytes,
                          bool do_sendv)
 {
-    int iovlen;
     ssize_t ret;
-    size_t diff;
-    struct iovec *last_iov;
+    struct iovec *end;
 
-    /* last_iov is inclusive, so count from one.  */
-    iovlen = 1;
-    last_iov = iov;
-    bytes += offset;
-
-    while (last_iov->iov_len < bytes) {
-        bytes -= last_iov->iov_len;
-
-        last_iov++;
-        iovlen++;
-    }
-
-    diff = last_iov->iov_len - bytes;
-    last_iov->iov_len -= diff;
-
-    while (iov->iov_len <= offset) {
+    /* Find the start position, skipping `offset' bytes:
+     * first, skip all full-sized vector elements, */
+    while(offset >= iov->iov_len) {
         offset -= iov->iov_len;
-
-        iov++;
-        iovlen--;
+        ++iov;
+    }
+    if (offset) {
+        /* second, skip `offset' bytes from the (now) first element,
+         * undo it on exit */
+        iov->iov_base += offset;
+        iov->iov_len -= offset;
+    }
+    /* Find the end position skipping `bytes' bytes: */
+    /* first, skip all full-sized elements */
+    end = iov;
+    while(end->iov_len <= bytes) {
+        bytes -= end->iov_len;
+        ++end;
+    }
+    if (bytes) {
+        /* second, fixup the last element, and remember
+         * the length we've cut from the end of it in `bytes' */
+        size_t tail = end->iov_len - bytes;
+        end->iov_len = bytes;
+        ++end;
+        bytes = tail;
     }
-
-    iov->iov_base = (char *) iov->iov_base + offset;
-    iov->iov_len -= offset;
 
     {
 #if defined CONFIG_IOVEC && defined CONFIG_POSIX
         struct msghdr msg;
         memset(&msg, 0, sizeof(msg));
         msg.msg_iov = iov;
-        msg.msg_iovlen = iovlen;
-
+        msg.msg_iovlen = end - iov;;
         do {
-            if (do_sendv) {
-                ret = sendmsg(sockfd, &msg, 0);
-            } else {
-                ret = recvmsg(sockfd, &msg, 0);
-            }
-        } while (ret == -1 && errno == EINTR);
+            ret = do_sendv
+                ? sendmsg(sockfd, &msg, 0)
+                : recvmsg(sockfd, &msg, 0);
+        } while (ret < 0 && errno == EINTR);
 #else
+        /* else send piece-by-piece */
+        /*XXX Note: windows has WSASend() and WSARecv() */
         struct iovec *p = iov;
         ret = 0;
-        while (iovlen > 0) {
-            int rc;
-            if (do_sendv) {
-                rc = send(sockfd, p->iov_base, p->iov_len, 0);
+        while(p < end) {
+            int r = do_sendv
+                ? send(sockfd, p->iov_base, p->iov_len, 0)
+                : recv(sockfd, p->iov_base, p->iov_len, 0);
+            if (r > 0) {
+                ret += r;
+                ++i;
+            } else if (!r) {
+                break;
+            } else if (errno == EINTR) {
+                continue;
             } else {
-                rc = qemu_recv(sockfd, p->iov_base, p->iov_len, 0);
-            }
-            if (rc == -1) {
-                if (errno == EINTR) {
-                    continue;
-                }
+                /* else it is some "other" error,
+                 * only return if there was no data processed. */
                 if (ret == 0) {
                     ret = -1;
                 }
                 break;
             }
-            if (rc == 0) {
-                break;
-            }
-            ret += rc;
-            iovlen--, p++;
         }
 #endif
     }
 
     /* Undo the changes above */
-    iov->iov_base = (char *) iov->iov_base - offset;
-    iov->iov_len += offset;
-    last_iov->iov_len += diff;
+    if (offset) {
+        iov->iov_base -= offset;
+        iov->iov_len += offset;
+    }
+    if (bytes) {
+        --end;
+        end->iov_len += bytes;
+    }
+
     return ret;
 }
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying Michael Tokarev
@ 2012-03-13 11:10   ` Kevin Wolf
  0 siblings, 0 replies; 20+ messages in thread
From: Kevin Wolf @ 2012-03-13 11:10 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: Paolo Bonzini, qemu-devel

Am 12.03.2012 20:14, schrieb Michael Tokarev:
> Similar to
>  qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
>                    int c, size_t bytes);
> the new prototype is:
>  qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
>                      const void *buf, size_t bytes);
> 
> The processing starts at offset bytes within qiov.
> 
> This way, we may copy a bounce buffer directly to
> a middle of qiov.
> 
> This is exactly the same function as iov_from_buf() from
> iov.c, so use the existing implementation and rename it
> to qemu_iovec_from_buf() to be shorter and to match the
> utility function.
> 
> As with utility implementation, we now assert that the
> offset is inside actual iovec.  Nothing changed for
> current callers, because `offset' parameter is new.
> 
> While at it, stop using "bounce-qiov" in block/qcow2.c
> and copy decrypted data directly from cluster_data
> instead of recreating a temp qiov for doing that
> (Cc'ing kwolf for this change).

Looks good to me.

Kevin

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions
  2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
                   ` (8 preceding siblings ...)
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 9/9] rewrite and comment qemu_sendv_recvv() Michael Tokarev
@ 2012-03-13 16:09 ` Michael Tokarev
  9 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-13 16:09 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: Paolo Bonzini, qemu-devel

I created a git tree with these patches,
  git://git.corpit.ru/qemu.git branch mjt-iov

It includes one extra patch:

 6856d2ef3e72753aaa3bcdfc274ee6ae6c112dc6 virtio-serial-bus: use correct lengths in control_out() message

which has been discussed with Amit Shah and he suggested
me to include it in this series.

Thanks,

/mjt

On 12.03.2012 23:14, Michael Tokarev wrote:
> This is a little cleanup/consolidation for some iovec-related
> low-level routines in qemu.
> 
> The plan is to make library functions more understandable,
> consistent and useful, and to drop numerous implementations
> of the same thing.
> 
> The patch changes prototypes of several iov and qiov functions
> to match each other, changes types of arguments for some
> functions, _swaps_ some function arguments with each other,
> and makes use of common code in r/w path.
> 
> The result of all these changes.
> 
> 1. Most qiov-related (qemu_iovec_*) functions now accepts
>  'offset' parameter to specify from which (byte) position to
>  start operation.  This is added for _memset (removing
>  _memset_skip), _from_buffer (allowing to copy a bounce-
>  buffer to a middle of qiov).  Typical:
> 
>   void qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, int c, size_t bytes);
> 
> 2. All functions that accepts this `offset' argument does
>  it in a similar manner, following the
>    iov, fromwhere, what, bytes
>  pattern.  This is consistent with (updated) qemu_sendv()
>  and qemu_recvv() and friends, where `offset' and `bytes'
>  arguments were _renamed_, with the following prototypes:
> 
>    ssize_t qemu_sendv(sockfd, iov, size_t offset, size_t bytes)
> 
>  instead of
> 
>    int qemu_sendv(sockfd, iov, int len, int iov_offset)
> 
>  See how offset & bytes are used in the same way as for iov_*
>  and qemu_iovec_*.   A few callers of these are verified and
>  converted.
> 
> 3. Used size_t instead of various variations for byte counts.
>  Including qemu_iovec_copy which used uint64_t(!) type.
> 
> 4. Function arguments are renamed to better match with their
>  actual meaning.  Compare new and original prototype of
>  qemu_sendv() above: old prototype with `len' does not
>  tell if `len' refers to number of iov elements (as
>  regular writev() call) or to number of data bytes.
>  Ditto for several usages of `count' for some qemu_iovec_*,
>  which is also replaced to `bytes'.
> 
> 5. One implementation of the code remain. For example,
>  qemu_iovec_from_buffer() uses iov_from_buf() directly,
>  instead of repeating the same code.
> 
> The resulting function usage is much more consistent, the
> functions themselves are nice and understandable, which
> means they're easier to use and less error-prone.
> 
> This patchset also consolidates a few low-level send&recv
> functions into one, since both versions were exactly
> the same (and were finally calling common function anyway).
> This is done by exporting a common send_recv function with
> one extra bool argument, and making current send&recv to
> be just #defines.
> 
> And while at it all, also made some implementations shorter,
> cleaner and much easier to read/understand, and add some
> code comments.
> 
> The read&write consolidation has great potential for the
> block layer, as has been demonstrated before.
> Unification and generalization of qemu_iovec_* functions
> will let to optimize/simplify some more code in block/*,
> especially qemu_iovec_memset() and _from_buffer() (this
> optimization/simplification is already used in qcow2.c
> a bit).
> 
> The resulting thing should behave like current/existing
> code, there should be no behavor change at all, just
> some shuffling.  It has been tested slightly, by booting
> different guests out of different image formats and doing
> some i/o, after each of the 9 patches, and it all works
> correctly so far.
> 
> The patchset depends on a tiny patch I sent to Amit Shah,
> "virtio-serial-bus: use correct lengths in control_out() message"
> but the only conflict without that patch is in
> hw/virtio-serial-bus.c and it is trivial to resolve it.
> 
> Changes since v2:
> 
> - covered iov.[ch] with iov_*() functions which contained
>   similar functionality
> - changed tabs to spaces as per suggestion by Kevin
> - explicitly allow to use large sizes for frombuf/tobuf
>   functions and friends, stopping at the end of iovec
>   in case more bytes requested when available, and
>   _returning_ number of actual bytes processed, but
>   made it extra clear what a return value will be.
> - used ssize_t for sendv/recvv instead of int, to
>   be consistent with all system headers and other
>   places in qemu
> - fix/clarify a case in my qemu_co_sendv_recvv()
>   of handling zero reads(recvs) and writes(sends).
> - covered qemu_iovec_to_buf() to have the same
>   pattern as other too (was missing in v2), and
>   use it in qcow2.c right away to simplify things
> - spotted and removed wrong usage of qiov instead of
>   plain iov in posix-aio-compat.c
> 
> What is _not_ covered:
> 
> - suggestion by pbonzini to not use macros for
>   access methods to call sendv_recvv with an
>   extra true/false argument: I still think the
>   usage of macros here is better than anything
>   else.
> - maybe re-shuffling of all this stuff into
>   different headers -- we already have iov.h,
>   maybe rename it to qemu-iov.h for consistency
>   and move all iov-related functions into there
>   (and ditto for cutils.c => (qemu-)iov.c).
> 
> Michael Tokarev (9):
>   refresh iov_* functions
>   consolidate qemu_iovec_memset{,_skip}() into single function and use existing iov_memset()
>   allow qemu_iovec_from_buffer() to specify offset from which to start copying
>   consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent
>   change qemu_iovec_to_buf() to match other to,from_buf functions
>   change prototypes of qemu_sendv() and qemu_recvv()
>   export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()
>   cleanup qemu_co_sendv(), qemu_co_recvv() and friends
>   rewrite and comment qemu_sendv_recvv()
> 
>  block.c                |   12 +-
>  block/curl.c           |    6 +-
>  block/iscsi.c          |    2 +-
>  block/nbd.c            |    4 +-
>  block/qcow.c           |    4 +-
>  block/qcow2.c          |   19 ++--
>  block/qed.c            |   10 +-
>  block/rbd.c            |    2 +-
>  block/sheepdog.c       |    6 +-
>  block/vdi.c            |    4 +-
>  cutils.c               |  261 +++++++++++++++++------------------------------
>  hw/9pfs/virtio-9p.c    |    8 +-
>  hw/rtl8139.c           |    2 +-
>  hw/usb.c               |    6 +-
>  hw/virtio-balloon.c    |    4 +-
>  hw/virtio-net.c        |    4 +-
>  hw/virtio-serial-bus.c |    6 +-
>  iov.c                  |   89 +++++++----------
>  iov.h                  |   47 ++++++++-
>  linux-aio.c            |    4 +-
>  net.c                  |    2 +-
>  posix-aio-compat.c     |    8 +-
>  qemu-common.h          |   69 +++++++------
>  qemu-coroutine-io.c    |   82 +++++-----------
>  24 files changed, 293 insertions(+), 368 deletions(-)
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions Michael Tokarev
@ 2012-03-13 17:44   ` Paolo Bonzini
  2012-03-13 18:28     ` Michael Tokarev
  0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 17:44 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

Il 12/03/2012 20:14, Michael Tokarev ha scritto:
> +    for (i = 0, done = 0; done < bytes && i < iov_cnt; i++) {
> +        if (offset < iov[i].iov_len) {
> +            size_t len = MIN(iov[i].iov_len - offset, bytes - done);
> +            memcpy(iov[i].iov_base + offset, buf + done, len);
> +            done += len;
> +            offset = 0;
> +        } else {
> +            offset -= iov[i].iov_len;
>          }
> -        iovec_off += iov[i].iov_len;
>      }
> -    return buf_off;
> +    assert(offset == 0);

This needs to be assert(offset == 0 || done == 0).

> +    return done;

Otherwise looks good, but I must say I do not like changing the API
*and* the implementation in the same patch.  It seems like a
bisectability nightmare (and reviewing nightmare, too).  I do prefer
your new code though.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset()
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() Michael Tokarev
@ 2012-03-13 17:46   ` Paolo Bonzini
  0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 17:46 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

Il 12/03/2012 20:14, Michael Tokarev ha scritto:
> This patch combines two functions into one, and replaces
> the implementation with already existing iov_memset() from
> iov.c.
> 
> The new prototype of qemu_iovec_memset():
>   size_t qemu_iovec_memset(qiov, size_t offset, int c, size_t bytes)
> It is different from former qemu_iovec_memset_skip(), and
> I want to make other functions to be consistent with it
> too: first how much to skip, second what, and 3rd how many
> of it.  It also returns actual number of bytes filled in,
> which may be less than the requested `bytes' if qiov is
> smaller than offset+bytes, in the same way iov_memset()
> does.
> 
> While at it, use utility function iov_memset() from
> iov.h in posix-aio-compat.c, where qiov was used.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block/qcow2.c      |    4 ++--
>  block/qed.c        |    4 ++--
>  cutils.c           |   44 ++++----------------------------------------
>  linux-aio.c        |    4 ++--
>  posix-aio-compat.c |    8 +++-----
>  qemu-common.h      |    5 ++---
>  6 files changed, 15 insertions(+), 54 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index eb5ea48..d46ca70 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -406,7 +406,7 @@ int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
>      else
>          n1 = bs->total_sectors - sector_num;
>  
> -    qemu_iovec_memset_skip(qiov, 0, 512 * (nb_sectors - n1), 512 * n1);
> +    qemu_iovec_memset(qiov, 512 * n1, 0, 512 * (nb_sectors - n1));
>  
>      return n1;
>  }
> @@ -466,7 +466,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
>                  }
>              } else {
>                  /* Note: in this case, no need to wait */
> -                qemu_iovec_memset(&hd_qiov, 0, 512 * cur_nr_sectors);
> +                qemu_iovec_memset(&hd_qiov, 0, 0, 512 * cur_nr_sectors);
>              }
>          } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
>              /* add AIO support for compressed blocks ? */
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..6f9325b 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -738,7 +738,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
>      /* Zero all sectors if reading beyond the end of the backing file */
>      if (pos >= backing_length ||
>          pos + qiov->size > backing_length) {
> -        qemu_iovec_memset(qiov, 0, qiov->size);
> +        qemu_iovec_memset(qiov, 0, 0, qiov->size);
>      }
>  
>      /* Complete now if there are no backing file sectors to read */
> @@ -1253,7 +1253,7 @@ static void qed_aio_read_data(void *opaque, int ret,
>  
>      /* Handle zero cluster and backing file reads */
>      if (ret == QED_CLUSTER_ZERO) {
> -        qemu_iovec_memset(&acb->cur_qiov, 0, acb->cur_qiov.size);
> +        qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size);
>          qed_aio_next_io(acb, 0);
>          return;
>      } else if (ret != QED_CLUSTER_FOUND) {
> diff --git a/cutils.c b/cutils.c
> index af308cd..53ff825 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -26,6 +26,7 @@
>  #include <math.h>
>  
>  #include "qemu_socket.h"
> +#include "iov.h"
>  
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
> @@ -260,47 +261,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
>      }
>  }
>  
> -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count)
> +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
> +                         int c, size_t bytes)
>  {
> -    size_t n;
> -    int i;
> -
> -    for (i = 0; i < qiov->niov && count; ++i) {
> -        n = MIN(count, qiov->iov[i].iov_len);
> -        memset(qiov->iov[i].iov_base, c, n);
> -        count -= n;
> -    }
> -}
> -
> -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
> -                            size_t skip)
> -{
> -    int i;
> -    size_t done;
> -    void *iov_base;
> -    uint64_t iov_len;
> -
> -    done = 0;
> -    for (i = 0; (i < qiov->niov) && (done != count); i++) {
> -        if (skip >= qiov->iov[i].iov_len) {
> -            /* Skip the whole iov */
> -            skip -= qiov->iov[i].iov_len;
> -            continue;
> -        } else {
> -            /* Skip only part (or nothing) of the iov */
> -            iov_base = (uint8_t*) qiov->iov[i].iov_base + skip;
> -            iov_len = qiov->iov[i].iov_len - skip;
> -            skip = 0;
> -        }
> -
> -        if (done + iov_len > count) {
> -            memset(iov_base, c, count - done);
> -            break;
> -        } else {
> -            memset(iov_base, c, iov_len);
> -        }
> -        done += iov_len;
> -    }
> +    return iov_memset(qiov->iov, qiov->niov, offset, c, bytes);
>  }
>  
>  /*
> diff --git a/linux-aio.c b/linux-aio.c
> index d2fc2e7..5a46c13 100644
> --- a/linux-aio.c
> +++ b/linux-aio.c
> @@ -64,8 +64,8 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,
>          } else if (ret >= 0) {
>              /* Short reads mean EOF, pad with zeros. */
>              if (laiocb->is_read) {
> -                qemu_iovec_memset_skip(laiocb->qiov, 0,
> -                    laiocb->qiov->size - ret, ret);
> +                qemu_iovec_memset(laiocb->qiov, ret, 0,
> +                    laiocb->qiov->size - ret);
>              } else {
>                  ret = -EINVAL;
>              }
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index d311d13..1ab4a18 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -29,6 +29,7 @@
>  #include "qemu-common.h"
>  #include "trace.h"
>  #include "block_int.h"
> +#include "iov.h"
>  
>  #include "block/raw-posix-aio.h"
>  
> @@ -351,11 +352,8 @@ static void *aio_thread(void *unused)
>              if (ret >= 0 && ret < aiocb->aio_nbytes && aiocb->common.bs->growable) {
>                  /* A short read means that we have reached EOF. Pad the buffer
>                   * with zeros for bytes after EOF. */
> -                QEMUIOVector qiov;
> -
> -                qemu_iovec_init_external(&qiov, aiocb->aio_iov,
> -                                         aiocb->aio_niov);
> -                qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - ret, ret);
> +                iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret,
> +                           0, aiocb->aio_nbytes - ret);
>  
>                  ret = aiocb->aio_nbytes;
>              }
> diff --git a/qemu-common.h b/qemu-common.h
> index dbfce6f..90f17fe 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -340,9 +340,8 @@ void qemu_iovec_destroy(QEMUIOVector *qiov);
>  void qemu_iovec_reset(QEMUIOVector *qiov);
>  void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
>  void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
> -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count);
> -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
> -                            size_t skip);
> +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
> +                         int c, size_t bytes);
>  
>  bool buffer_is_zero(const void *buf, size_t len);
>  

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends Michael Tokarev
@ 2012-03-13 17:52   ` Paolo Bonzini
  2012-03-13 18:01     ` Paolo Bonzini
  0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 17:52 UTC (permalink / raw)
  To: qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
  2012-03-13 17:52   ` Paolo Bonzini
@ 2012-03-13 18:01     ` Paolo Bonzini
  2012-03-13 19:35       ` Michael Tokarev
  0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 18:01 UTC (permalink / raw)
  Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent Michael Tokarev
@ 2012-03-13 18:02   ` Paolo Bonzini
  0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 18:02 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

Il 12/03/2012 20:14, Michael Tokarev ha scritto:
> qemu_iovec_concat() is currently a wrapper for
> qemu_iovec_copy(), use the former (with extra
> "0" arg) in a few places where it is used.
> 
> Change skip argument of qemu_iovec_copy() from
> uint64_t to size_t, since size of qiov itself
> is size_t, so there's no way to skip larger
> sizes.  Rename it to soffset, to make it clear
> that the offset is applied to src.
> 
> Also change the only usage of uint64_t in
> hw/9pfs/virtio-9p.c, in v9fs_init_qiov_from_pdu() -
> all callers of it actually uses size_t too,
> not uint64_t.
> 
> One added restriction: as for all other iovec-related
> functions, soffset must point inside src.
> 
> Order of argumens is already good:
>  qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
>                    int c, size_t bytes)
> vs:
>  qemu_iovec_concat(QEMUIOVector *dst,
>                    QEMUIOVector *src,
>                    size_t soffset, size_t sbytes)
> (note soffset is after _src_ not dst, since it applies to src;
> for memset it applies to qiov).
> 
> Note that in many places where this function is used,
> the previous call is qemu_iovec_reset(), which means
> many callers actually want copy (replacing dst content),
> not concat.  So we may want to add a wrapper like
> qemu_iovec_copy() with the same arguments but which
> calls qemu_iovec_reset() before _concat().  This needs
> to be done later, so that any current out-of-tree code
> which uses _copy().
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block.c             |    4 +-
>  block/qcow2.c       |    4 +-
>  block/qed.c         |    6 ++--
>  cutils.c            |   54 ++++++++++++++++++--------------------------------
>  hw/9pfs/virtio-9p.c |    8 +++---
>  qemu-common.h       |    5 +--
>  6 files changed, 33 insertions(+), 48 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f4320be..9246518 100644
> --- a/block.c
> +++ b/block.c
> @@ -2959,13 +2959,13 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>              // Add the first request to the merged one. If the requests are
>              // overlapping, drop the last sectors of the first request.
>              size = (reqs[i].sector - reqs[outidx].sector) << 9;
> -            qemu_iovec_concat(qiov, reqs[outidx].qiov, size);
> +            qemu_iovec_concat(qiov, reqs[outidx].qiov, 0, size);
>  
>              // We should need to add any zeros between the two requests
>              assert (reqs[i].sector <= oldreq_last);
>  
>              // Add the second request
> -            qemu_iovec_concat(qiov, reqs[i].qiov, reqs[i].qiov->size);
> +            qemu_iovec_concat(qiov, reqs[i].qiov, 0, reqs[i].qiov->size);
>  
>              reqs[outidx].nb_sectors = qiov->size >> 9;
>              reqs[outidx].qiov = qiov;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 106f2ce..894fb4d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -445,7 +445,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>  
>          qemu_iovec_reset(&hd_qiov);
> -        qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
> +        qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
>              cur_nr_sectors * 512);
>  
>          if (!cluster_offset) {
> @@ -593,7 +593,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>          assert((cluster_offset & 511) == 0);
>  
>          qemu_iovec_reset(&hd_qiov);
> -        qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
> +        qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
>              cur_nr_sectors * 512);
>  
>          if (s->crypt_method) {
> diff --git a/block/qed.c b/block/qed.c
> index 6f9325b..a4ef12c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1133,7 +1133,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
>  
>      acb->cur_nclusters = qed_bytes_to_clusters(s,
>              qed_offset_into_cluster(s, acb->cur_pos) + len);
> -    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
> +    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
>  
>      if (acb->flags & QED_AIOCB_ZERO) {
>          /* Skip ahead if the clusters are already zero */
> @@ -1179,7 +1179,7 @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
>  
>      /* Calculate the I/O vector */
>      acb->cur_cluster = offset;
> -    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
> +    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
>  
>      /* Do the actual write */
>      qed_aio_write_main(acb, 0);
> @@ -1249,7 +1249,7 @@ static void qed_aio_read_data(void *opaque, int ret,
>          goto err;
>      }
>  
> -    qemu_iovec_copy(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
> +    qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
>  
>      /* Handle zero cluster and backing file reads */
>      if (ret == QED_CLUSTER_ZERO) {
> diff --git a/cutils.c b/cutils.c
> index 1c93ed4..b8fc954 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -172,48 +172,34 @@ void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len)
>  }
>  
>  /*
> - * Copies iovecs from src to the end of dst. It starts copying after skipping
> - * the given number of bytes in src and copies until src is completely copied
> - * or the total size of the copied iovec reaches size.The size of the last
> - * copied iovec is changed in order to fit the specified total size if it isn't
> - * a perfect fit already.
> + * Concatenates (partial) iovecs from src to the end of dst.
> + * It starts copying after skipping `soffset' bytes at the
> + * beginning of src and adds individual vectors from src to
> + * dst copies up to `sbytes' bytes total, or up to the end
> + * of src if it comes first.  This way, it is okay to specify
> + * very large value for `sbytes' to indicate "up to the end
> + * of src".
> + * Only vector pointers are processed, not the actual data buffers.
>   */
> -void qemu_iovec_copy(QEMUIOVector *dst, QEMUIOVector *src, uint64_t skip,
> -    size_t size)
> +void qemu_iovec_concat(QEMUIOVector *dst,
> +                       QEMUIOVector *src, size_t soffset, size_t sbytes)
>  {
>      int i;
>      size_t done;
> -    void *iov_base;
> -    uint64_t iov_len;
> -
> +    struct iovec *siov = src->iov;
>      assert(dst->nalloc != -1);
> -
> -    done = 0;
> -    for (i = 0; (i < src->niov) && (done != size); i++) {
> -        if (skip >= src->iov[i].iov_len) {
> -            /* Skip the whole iov */
> -            skip -= src->iov[i].iov_len;
> -            continue;
> +    assert(src->size >= soffset);
> +    for (i = 0, done = 0; done < sbytes && i < src->niov; i++) {
> +        if (soffset < siov[i].iov_len) {
> +            size_t len = MIN(siov[i].iov_len - soffset, sbytes - done);
> +            qemu_iovec_add(dst, siov[i].iov_base + soffset, len);
> +            done += len;
> +            soffset = 0;
>          } else {
> -            /* Skip only part (or nothing) of the iov */
> -            iov_base = (uint8_t*) src->iov[i].iov_base + skip;
> -            iov_len = src->iov[i].iov_len - skip;
> -            skip = 0;
> +            soffset -= siov[i].iov_len;
>          }
> -
> -        if (done + iov_len > size) {
> -            qemu_iovec_add(dst, iov_base, size - done);
> -            break;
> -        } else {
> -            qemu_iovec_add(dst, iov_base, iov_len);
> -        }
> -        done += iov_len;
>      }
> -}
> -
> -void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size)
> -{
> -    qemu_iovec_copy(dst, src, 0, size);
> +    /* return done; */
>  }
>  
>  void qemu_iovec_destroy(QEMUIOVector *qiov)
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index c633fb9..f4a7026 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1648,7 +1648,7 @@ out:
>   * with qemu_iovec_destroy().
>   */
>  static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
> -                                    uint64_t skip, size_t size,
> +                                    size_t skip, size_t size,
>                                      bool is_write)
>  {
>      QEMUIOVector elem;
> @@ -1665,7 +1665,7 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
>  
>      qemu_iovec_init_external(&elem, iov, niov);
>      qemu_iovec_init(qiov, niov);
> -    qemu_iovec_copy(qiov, &elem, skip, size);
> +    qemu_iovec_concat(qiov, &elem, skip, size);
>  }
>  
>  static void v9fs_read(void *opaque)
> @@ -1715,7 +1715,7 @@ static void v9fs_read(void *opaque)
>          qemu_iovec_init(&qiov, qiov_full.niov);
>          do {
>              qemu_iovec_reset(&qiov);
> -            qemu_iovec_copy(&qiov, &qiov_full, count, qiov_full.size - count);
> +            qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count);
>              if (0) {
>                  print_sg(qiov.iov, qiov.niov);
>              }
> @@ -1970,7 +1970,7 @@ static void v9fs_write(void *opaque)
>      qemu_iovec_init(&qiov, qiov_full.niov);
>      do {
>          qemu_iovec_reset(&qiov);
> -        qemu_iovec_copy(&qiov, &qiov_full, total, qiov_full.size - total);
> +        qemu_iovec_concat(&qiov, &qiov_full, total, qiov_full.size - total);
>          if (0) {
>              print_sg(qiov.iov, qiov.niov);
>          }
> diff --git a/qemu-common.h b/qemu-common.h
> index 2cbb513..31b30b5 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -333,9 +333,8 @@ typedef struct QEMUIOVector {
>  void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
>  void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
>  void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
> -void qemu_iovec_copy(QEMUIOVector *dst, QEMUIOVector *src, uint64_t skip,
> -    size_t size);
> -void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size);
> +void qemu_iovec_concat(QEMUIOVector *dst,
> +                       QEMUIOVector *src, size_t soffset, size_t sbytes);
>  void qemu_iovec_destroy(QEMUIOVector *qiov);
>  void qemu_iovec_reset(QEMUIOVector *qiov);
>  void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);

Looks good.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions
  2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions Michael Tokarev
@ 2012-03-13 18:02   ` Paolo Bonzini
  0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2012-03-13 18:02 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel

Il 12/03/2012 20:14, Michael Tokarev ha scritto:
> It now allows specifying offset within qiov to start from and
> amount of bytes to copy.  Actual implementation is just a call
> to iov_to_buf().
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block.c       |    2 +-
>  block/iscsi.c |    2 +-
>  block/qcow.c  |    2 +-
>  block/qcow2.c |    2 +-
>  block/rbd.c   |    2 +-
>  block/vdi.c   |    2 +-
>  cutils.c      |   11 +++--------
>  qemu-common.h |    3 ++-
>  8 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9246518..4bb5f92 100644
> --- a/block.c
> +++ b/block.c
> @@ -3266,7 +3266,7 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
>      acb->bh = qemu_bh_new(bdrv_aio_bh_cb, acb);
>  
>      if (is_write) {
> -        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
> +        qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
>          acb->ret = bs->drv->bdrv_write(bs, sector_num, acb->bounce, nb_sectors);
>      } else {
>          acb->ret = bs->drv->bdrv_read(bs, sector_num, acb->bounce, nb_sectors);
> diff --git a/block/iscsi.c b/block/iscsi.c
> index bd3ca11..db589f5 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -223,7 +223,7 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
>      /* this will allow us to get rid of 'buf' completely */
>      size = nb_sectors * BDRV_SECTOR_SIZE;
>      acb->buf = g_malloc(size);
> -    qemu_iovec_to_buffer(acb->qiov, acb->buf);
> +    qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>      acb->task = iscsi_write10_task(iscsi, iscsilun->lun, acb->buf, size,
>                                sector_qemu2lun(sector_num, iscsilun),
>                                fua, 0, iscsilun->block_size,
> diff --git a/block/qcow.c b/block/qcow.c
> index 562a19c..a367459 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -569,7 +569,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>  
>      if (qiov->niov > 1) {
>          buf = orig_buf = qemu_blockalign(bs, qiov->size);
> -        qemu_iovec_to_buffer(qiov, buf);
> +        qemu_iovec_to_buf(qiov, 0, buf, qiov->size);
>      } else {
>          orig_buf = NULL;
>          buf = (uint8_t *)qiov->iov->iov_base;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 894fb4d..88fbd73 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -604,7 +604,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  
>              assert(hd_qiov.size <=
>                     QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
> -            qemu_iovec_to_buffer(&hd_qiov, cluster_data);
> +            qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
>  
>              qcow2_encrypt_sectors(s, sector_num, cluster_data,
>                  cluster_data, cur_nr_sectors, 1, &s->aes_encrypt_key);
> diff --git a/block/rbd.c b/block/rbd.c
> index 46a8579..0191f86 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -644,7 +644,7 @@ static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
>      acb->bh = NULL;
>  
>      if (write) {
> -        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
> +        qemu_iovec_to_buffer(acb->qiov, 0, acb->bounce, qiov->size);
>      }
>  
>      buf = acb->bounce;
> diff --git a/block/vdi.c b/block/vdi.c
> index 24f4027..30a5e27 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -524,7 +524,7 @@ static VdiAIOCB *vdi_aio_setup(BlockDriverState *bs, int64_t sector_num,
>          acb->buf = qemu_blockalign(bs, qiov->size);
>          acb->orig_buf = acb->buf;
>          if (is_write) {
> -            qemu_iovec_to_buffer(qiov, acb->buf);
> +            qemu_iovec_to_buf(qiov, 0, acb->buf, qiov->size);
>          }
>      } else {
>          acb->buf = (uint8_t *)qiov->iov->iov_base;
> diff --git a/cutils.c b/cutils.c
> index b8fc954..df26f72 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -220,15 +220,10 @@ void qemu_iovec_reset(QEMUIOVector *qiov)
>      qiov->size = 0;
>  }
>  
> -void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf)
> +size_t qemu_iovec_to_buf(QEMUIOVector *qiov, size_t offset,
> +                         void *buf, size_t bytes)
>  {
> -    uint8_t *p = (uint8_t *)buf;
> -    int i;
> -
> -    for (i = 0; i < qiov->niov; ++i) {
> -        memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
> -        p += qiov->iov[i].iov_len;
> -    }
> +    return iov_to_buf(qiov->iov, qiov->niov, offset, buf, bytes);
>  }
>  
>  size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
> diff --git a/qemu-common.h b/qemu-common.h
> index 31b30b5..ec9655b 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -337,7 +337,8 @@ void qemu_iovec_concat(QEMUIOVector *dst,
>                         QEMUIOVector *src, size_t soffset, size_t sbytes);
>  void qemu_iovec_destroy(QEMUIOVector *qiov);
>  void qemu_iovec_reset(QEMUIOVector *qiov);
> -void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
> +size_t qemu_iovec_to_buf(QEMUIOVector *qiov, size_t offset,
> +                         void *buf, size_t bytes);
>  size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
>                             const void *buf, size_t bytes);
>  size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,

Looks good.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions
  2012-03-13 17:44   ` Paolo Bonzini
@ 2012-03-13 18:28     ` Michael Tokarev
  0 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-13 18:28 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

On 13.03.2012 21:44, Paolo Bonzini wrote:
> Il 12/03/2012 20:14, Michael Tokarev ha scritto:
>> +    for (i = 0, done = 0; done < bytes && i < iov_cnt; i++) {
>> +        if (offset < iov[i].iov_len) {
>> +            size_t len = MIN(iov[i].iov_len - offset, bytes - done);
>> +            memcpy(iov[i].iov_base + offset, buf + done, len);
>> +            done += len;
>> +            offset = 0;
>> +        } else {
>> +            offset -= iov[i].iov_len;
>>          }
>> -        iovec_off += iov[i].iov_len;
>>      }
>> -    return buf_off;
>> +    assert(offset == 0);
> 
> This needs to be assert(offset == 0 || done == 0).

Nope.  All these functions actually allow to specify very
large value for `bytes' (and I suggested using -1 for it
to mean "up to the end").  Only offset must be within
the iovec.

>> +    return done;
> 
> Otherwise looks good, but I must say I do not like changing the API
> *and* the implementation in the same patch.  It seems like a
> bisectability nightmare (and reviewing nightmare, too).  I do prefer
> your new code though.

I changed it all to be the same and actually verified.
Sure I can split it into two patches -- that should be
easy, will do that in a moment...  Just like I did with
qemu_sendv_recvv().  The whole thing is just trivial but.. ;)

Thank you for the review!

> Paolo

/mjt

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
  2012-03-13 18:01     ` Paolo Bonzini
@ 2012-03-13 19:35       ` Michael Tokarev
  0 siblings, 0 replies; 20+ messages in thread
From: Michael Tokarev @ 2012-03-13 19:35 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2012-03-13 19:35 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-12 19:14 [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 1/9] refresh iov_* functions Michael Tokarev
2012-03-13 17:44   ` Paolo Bonzini
2012-03-13 18:28     ` Michael Tokarev
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 2/9] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() Michael Tokarev
2012-03-13 17:46   ` Paolo Bonzini
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 3/9] allow qemu_iovec_from_buffer() to specify offset from which to start copying Michael Tokarev
2012-03-13 11:10   ` Kevin Wolf
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 4/9] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent Michael Tokarev
2012-03-13 18:02   ` Paolo Bonzini
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 5/9] change qemu_iovec_to_buf() to match other to, from_buf functions Michael Tokarev
2012-03-13 18:02   ` Paolo Bonzini
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 6/9] change prototypes of qemu_sendv() and qemu_recvv() Michael Tokarev
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 7/9] export qemu_sendv_recvv() and use it in " Michael Tokarev
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 8/9] cleanup qemu_co_sendv(), qemu_co_recvv() and friends Michael Tokarev
2012-03-13 17:52   ` Paolo Bonzini
2012-03-13 18:01     ` Paolo Bonzini
2012-03-13 19:35       ` Michael Tokarev
2012-03-12 19:14 ` [Qemu-devel] [PATCHv3 9/9] rewrite and comment qemu_sendv_recvv() Michael Tokarev
2012-03-13 16:09 ` [Qemu-devel] [PATCHv3 0/9] cleanup/consolidate some iovec functions Michael Tokarev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.