[Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions

[Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions

[Michael Tokarev]

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.

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,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:

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

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 isn't done in this series)

Michael Tokarev (7):
  Consolidate qemu_iovec_memset{,_skip}() into single, simplified function
  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 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             |    8 +-
 block/curl.c        |    4 +-
 block/nbd.c         |    4 +-
 block/qcow.c        |    2 +-
 block/qcow2.c       |   14 ++--
 block/qed.c         |   10 +-
 block/sheepdog.c    |    6 +-
 block/vdi.c         |    2 +-
 cutils.c            |  275 +++++++++++++++++++++------------------------------
 hw/9pfs/virtio-9p.c |    8 +-
 linux-aio.c         |    4 +-
 posix-aio-compat.c  |    2 +-
 qemu-common.h       |   64 +++++++-----
 qemu-coroutine-io.c |   83 ++++-----------
 14 files changed, 205 insertions(+), 281 deletions(-)

-- 
1.7.9.1

[Qemu-devel] [PATCHv2 1/7] Consolidate qemu_iovec_memset{, _skip}() into single, simplified function

[Michael Tokarev]

This patch combines two functions into one, simplifies the
implementation and adds some assert()s into place.

The new prototype of qemu_iovec_memset():
  void 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.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index eb5ea48..6d11bc0 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..9451c86 100644
--- a/cutils.c
+++ b/cutils.c
@@ -260,46 +260,24 @@ 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(QEMUIOVector *qiov, size_t offset, int c, size_t bytes)
 {
-    size_t n;
+    struct iovec *iov = qiov->iov;
     int i;
+    assert(qiov->size >= offset);
+    assert(qiov->size - offset >= bytes);
 
-    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;
+    /* first skip initial full-sized elements */
+    for(i = 0; offset >= iov[i].iov_len; ++i) {
+	offset -= iov[i].iov_len;
     }
-}
-
-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;
+    /* skip/memset partial element and memset the rest */
+    while(bytes) {
+	size_t n = MIN(bytes, iov[i].iov_len - offset);
+	memset((char*)iov[i].iov_base + offset, c, n);
+	bytes -= n;
+	++i;
+	offset = 0;
     }
 }
 
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..f8efe98 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -355,7 +355,7 @@ static void *aio_thread(void *unused)
 
                 qemu_iovec_init_external(&qiov, aiocb->aio_iov,
                                          aiocb->aio_niov);
-                qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - ret, ret);
+                qemu_iovec_memset(&qiov, ret, 0, aiocb->aio_nbytes - ret);
 
                 ret = aiocb->aio_nbytes;
             }
diff --git a/qemu-common.h b/qemu-common.h
index dbfce6f..a1ff126 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -340,9 +340,7 @@ 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);
+void 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

[Qemu-devel] [PATCHv2 2/7] allow qemu_iovec_from_buffer() to specify offset from which to start copying

[Michael Tokarev]

Similar to
 qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, int c, size_t bytes);
the new prototype is:
 qemu_iovec_from_buffer(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.

While at it, add some asserts to ensure that buffer content fits
within qiov: offset + bytes <= size.
---
 block.c       |    4 ++--
 block/curl.c  |    4 ++--
 block/qcow.c  |    2 +-
 block/qcow2.c |    4 ++--
 block/vdi.c   |    2 +-
 cutils.c      |   27 +++++++++++++++++----------
 qemu-common.h |    3 ++-
 7 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index 52ffe14..cbbedfa 100644
--- a/block.c
+++ b/block.c
@@ -1692,7 +1692,7 @@ 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,
+    qemu_iovec_from_buffer(qiov, 0, bounce_buffer + skip_bytes,
                            nb_sectors * BDRV_SECTOR_SIZE);
 
 err:
@@ -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_buffer(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..e772912 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -142,7 +142,7 @@ 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,
+            qemu_iovec_from_buffer(acb->qiov, 0, s->orig_buf + acb->start,
                                    acb->end - acb->start);
             acb->common.cb(acb->common.opaque, 0);
             qemu_aio_release(acb);
@@ -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_buffer(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..f95b93f 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_buffer(qiov, 0, orig_buf, qiov->size);
         qemu_vfree(orig_buf);
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 6d11bc0..668e2e2 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_buffer(&hd_qiov, 0,
                 s->cluster_cache + index_in_cluster * 512,
                 512 * cur_nr_sectors);
         } else {
@@ -516,7 +516,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 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,
+                qemu_iovec_from_buffer(&hd_qiov, 0, cluster_data,
                     512 * cur_nr_sectors);
             }
         }
diff --git a/block/vdi.c b/block/vdi.c
index 6a0011f..db0650e 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_buffer(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 9451c86..5353f89 100644
--- a/cutils.c
+++ b/cutils.c
@@ -244,19 +244,26 @@ 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_from_buffer(QEMUIOVector *qiov, size_t offset,
+			    const void *buf, size_t bytes)
 {
-    const uint8_t *p = (const uint8_t *)buf;
-    size_t copy;
+    const struct iovec *iov = qiov->iov;
     int i;
+    assert(qiov->size >= offset);
+    assert(qiov->size - offset >= bytes);
 
-    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;
+    /* first skip initial full-sized elements */
+    for(i = 0; offset >= iov[i].iov_len; ++i) {
+	offset -= iov[i].iov_len;
+    }
+    /* skip/copy partial element and copy the rest */
+    while(bytes) {
+	size_t n = MIN(bytes, iov[i].iov_len - offset);
+	memcpy((char*)iov[i].iov_base + offset, buf, n);
+	bytes -= n;
+        buf = (const char*)buf + n;
+	++i;
+	offset = 0;
     }
 }
 
diff --git a/qemu-common.h b/qemu-common.h
index a1ff126..249f70d 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);
+void qemu_iovec_from_buffer(QEMUIOVector *qiov, size_t offset,
+			    const void *buf, size_t bytes);
 void 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

[Qemu-devel] [PATCHv2 3/7] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent

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

Semantic change in the meaning of `count' (now renamed to `sbytes')
argument.  Initial comment said that src is copied to dst until
_total_ size is less than specified, so it might be interpreted
as maximum size of the _dst_ vector.  Actual meaning of if was
that total amount of skipped and copied bytes should not exceed
`count'.  Make it just the amount of bytes to _copy_, without
counting skipped bytes.  This makes it consistent with other
iovec functions, and also matches actual _usage_ of this function.

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
paramere to allow resetting dst in one go.
---
 block.c             |    4 +-
 block/qcow2.c       |    6 ++--
 block/qed.c         |    6 ++--
 cutils.c            |   56 +++++++++++++++++++-------------------------------
 hw/9pfs/virtio-9p.c |    8 +++---
 qemu-common.h       |    5 +--
 6 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/block.c b/block.c
index cbbedfa..3baa182 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 668e2e2..1723275 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) {
@@ -514,7 +514,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 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,
+                qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
                     cur_nr_sectors * 512);
                 qemu_iovec_from_buffer(&hd_qiov, 0, cluster_data,
                     512 * cur_nr_sectors);
@@ -596,7 +596,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 5353f89..04e51f7 100644
--- a/cutils.c
+++ b/cutils.c
@@ -171,48 +171,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 copies `sbytes' bytes from that point.
+ * 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.
  */
-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);
+    assert(src->size >= soffset);
+    assert(src->size - soffset >= sbytes);
 
-    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;
-        } 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;
-        }
-
-        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;
+    /* first skip initial full-sized source elements */
+    for(i = 0; soffset >= siov[i].iov_len; ++i) {
+	soffset -= siov[i].iov_len;
+    }
+    /* skip/copy partial element and copy the rest */
+    while(sbytes) {
+	size_t n = MIN(sbytes, siov[i].iov_len - soffset);
+	qemu_iovec_add(dst, (char*)siov[i].iov_base + soffset, n);
+	sbytes -= n;
+	++i;
+	soffset = 0;
     }
-}
-
-void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t size)
-{
-    qemu_iovec_copy(dst, src, 0, size);
 }
 
 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 249f70d..e8d1429 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

[Qemu-devel] [PATCHv2 4/7] change prototypes of qemu_sendv() and qemu_recvv()

[Michael Tokarev]

Rename arguments and use size_t for sizes instead of int

-qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset)
+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.

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.

While at it, clean up misleading comment near do_sendv_recvv().

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            |   34 ++++++++++++----------------------
 qemu-common.h       |    4 ++--
 qemu-coroutine-io.c |    4 ++--
 3 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/cutils.c b/cutils.c
index 04e51f7..b3b9cfb 100644
--- a/cutils.c
+++ b/cutils.c
@@ -417,38 +417,28 @@ 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.
- *
- * For example,
- *
- *   do_sendv_recvv(sockfd, iov, len, offset, 1);
- *
- * is equal to
- *
- *   char *buf = malloc(size);
- *   iov_to_buf(iov, iovcnt, buf, offset, size);
- *   send(sockfd, buf, size, 0);
- *   free(buf);
+ * `bytes' bytes are used.
  */
-static int do_sendv_recvv(int sockfd, struct iovec *iov, int len, int offset,
+static int do_sendv_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes,
                           int do_sendv)
 {
-    int ret, diff, iovlen;
+    int ret, iovlen;
+    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) {
@@ -510,13 +500,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)
+int 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)
+int 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 e8d1429..bb75dd3 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);
+int qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
+int 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

[Qemu-devel] [PATCHv2 5/7] Export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()

[Michael Tokarev]

Rename do_sendv_recvv() to qemu_sendv_recvv(),
change its last arg (do_send) 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      |   15 ++-------------
 qemu-common.h |   14 ++++++++++++--
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/cutils.c b/cutils.c
index b3b9cfb..b6f8eb8 100644
--- a/cutils.c
+++ b/cutils.c
@@ -419,8 +419,8 @@ int qemu_parse_fd(const char *param)
  * The first `offset' bytes in the iovec buffer are skipped and next
  * `bytes' bytes are used.
  */
-static int do_sendv_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes,
-                          int do_sendv)
+int qemu_sendv_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes,
+		     bool do_sendv)
 {
     int ret, iovlen;
     size_t diff;
@@ -499,14 +499,3 @@ static int do_sendv_recvv(int sockfd, struct iovec *iov, size_t offset, size_t b
     last_iov->iov_len += diff;
     return ret;
 }
-
-int qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes)
-{
-    return do_sendv_recvv(sockfd, iov, offset, bytes, 0);
-}
-
-int 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 bb75dd3..b8190e9 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
 
-int qemu_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes);
-int 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'.
+ */
+int 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

[Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[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.
Also constify buf arg of qemu_co_send().

qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now
trivial #define's merely adding one extra arg.  qemu_co_send()
is an inline function due to `buf' arg de-constification.

qemu_co_sendv() and qemu_co_recvv() callers are converted to
different argument order.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block/nbd.c         |    4 +-
 block/sheepdog.c    |    6 ++--
 qemu-common.h       |   38 +++++++++++-----------
 qemu-coroutine-io.c |   83 +++++++++++++-------------------------------------
 4 files changed, 46 insertions(+), 85 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..fed436b 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 b8190e9..110ec06 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -306,31 +306,31 @@ 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.
- */
-int qemu_co_recvv(int sockfd, struct iovec *iov,
-                  int len, int iov_offset);
-
 
 /**
- * Sends a buffer down a socket, yielding when the socket is full.
+ * 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_send(int sockfd, void *buf, int len);
+int qemu_co_sendv_recvv(int sockfd, struct iovec *iov,
+			size_t offset, size_t bytes, bool do_send);
+#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)
 
 /**
- * Receives data into a buffer from a socket, yielding when there
- * is no data in the socket.
+ * The same as above, but with just a single buffer
  */
-int qemu_co_recv(int sockfd, void *buf, int len);
+int qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send);
+#define qemu_co_recv(sockfd, buf, bytes) \
+  qemu_co_send_recv(sockfd, buf, bytes, false)
+static inline int qemu_co_send(int sockfd, const void *buf, size_t bytes)
+{
+    return qemu_co_send_recv(sockfd, (void*)buf, bytes, true);
+}
 
 
 typedef struct QEMUIOVector {
diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c
index df8ff21..75a4117 100644
--- a/qemu-coroutine-io.c
+++ b/qemu-coroutine-io.c
@@ -26,71 +26,32 @@
 #include "qemu_socket.h"
 #include "qemu-coroutine.h"
 
-int coroutine_fn qemu_co_recvv(int sockfd, struct iovec *iov,
-                               int len, int iov_offset)
+int coroutine_fn
+qemu_co_sendv_recvv(int sockfd, struct iovec *iov,
+		        size_t offset, size_t bytes, bool do_send)
 {
-    int total = 0;
+    size_t done = 0;
     int ret;
-    while (len) {
-        ret = qemu_recvv(sockfd, iov, iov_offset + total, len);
-        if (ret < 0) {
-            if (errno == EAGAIN) {
-                qemu_coroutine_yield();
-                continue;
-            }
-            if (total == 0) {
-                total = -1;
-            }
-            break;
-        }
-        if (ret == 0) {
-            break;
-        }
-        total += ret, len -= ret;
+    while (done < bytes) {
+        ret = qemu_sendv_recvv(sockfd, iov, offset + done, bytes - done, do_send);
+	if (ret > 0) {
+	    done += ret;
+	} else if (!ret) {
+	    break;
+	} else if (errno == EAGAIN) {
+	    qemu_coroutine_yield();
+	} else if (done == 0) {
+	    return -1;
+	} else {
+	    break;
+	}
     }
-
-    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;
-            }
-            break;
-        }
-        total += ret, len -= ret;
-    }
-
-    return total;
+    return done;
 }
 
-int coroutine_fn qemu_co_recv(int sockfd, void *buf, int len)
+int coroutine_fn
+qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
 {
-    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 = { buf, bytes };
+    return qemu_co_sendv_recvv(sockfd, &iov, 0, bytes, do_send);
 }
-- 
1.7.9.1

[Qemu-devel] [PATCHv2 7/7] rewrite and comment qemu_sendv_recvv()

[Michael Tokarev]

Make it much more understandable, and add comments to it.
This is in order to prepare similar function for writev_readv().

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      |  119 +++++++++++++++++++++++++++++---------------------------
 qemu-common.h |    2 +-
 2 files changed, 63 insertions(+), 58 deletions(-)

diff --git a/cutils.c b/cutils.c
index b6f8eb8..3d1b2ea 100644
--- a/cutils.c
+++ b/cutils.c
@@ -419,83 +419,88 @@ int qemu_parse_fd(const char *param)
  * The first `offset' bytes in the iovec buffer are skipped and next
  * `bytes' bytes are used.
  */
-int qemu_sendv_recvv(int sockfd, struct iovec *iov, size_t offset, size_t bytes,
-		     bool do_sendv)
+int qemu_sendv_recvv(int sockfd, struct iovec *iov,
+		     size_t offset, size_t bytes, bool do_send)
 {
-    int ret, iovlen;
-    size_t diff;
-    struct iovec *last_iov;
-
-    /* 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++;
+    int ret;
+    struct iovec *end;
+
+    /* Find the start position, skipping `offset' bytes:
+     * first, skip all full-sized vector elements, */
+    while(offset >= iov->iov_len) {
+	offset -= iov->iov_len;
+	++iov;
     }
-
-    diff = last_iov->iov_len - bytes;
-    last_iov->iov_len -= diff;
-
-    while (iov->iov_len <= offset) {
-        offset -= iov->iov_len;
-
-        iov++;
-        iovlen--;
+    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_send
+		? sendmsg(sockfd, &msg, 0)
+                : recvmsg(sockfd, &msg, 0);
+        } while (ret < 0 && errno == EINTR);
 #else
-        struct iovec *p = iov;
+	/* 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);
-            } else {
-                rc = qemu_recv(sockfd, p->iov_base, p->iov_len, 0);
-            }
-            if (rc == -1) {
-                if (errno == EINTR) {
-                    continue;
-                }
+	while(p < end) {
+	    int r = do_send
+		? send(sockfd, p->iov_base, p->iov_len, 0)
+		: qemu_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 {
+		/* 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;
 }
diff --git a/qemu-common.h b/qemu-common.h
index 110ec06..2097fa4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -206,7 +206,7 @@ int qemu_pipe(int pipefd[2]);
  * the beginning of iovec but at byte position `offset'.
  */
 int qemu_sendv_recvv(int sockfd, struct iovec *iov,
-		     size_t offset, size_t bytes, bool do_sendv);
+		     size_t offset, size_t bytes, bool do_send);
 #define qemu_recvv(sockfd, iov, offset, bytes) \
   qemu_sendv_recvv(sockfd, iov, offset, bytes, false)
 #define qemu_sendv(sockfd, iov, offset, bytes) \
-- 
1.7.9.1

Re: [Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions

[Michael Tokarev]

On 11.03.2012 05:49, 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.
> 
> 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,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:
> 
>    int 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 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'.
> 
> 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 isn't done in this series)

And ofcourse I forgot to mention: the difference from
initial patch:

- picked 2 more qemu_iovec_* functions (minor), and
- changed order of arguments of sendv() and recvv().

The second point is relatively major, since it changes quite
some innocent places, and has a potential of breaking things
due to someone not noticing the args has been reordered.

But this is something I was wanted to do initially, because
this is how the args should has been initially (IMHO anyway),
but wasn't brave enough to actually change it.  But it is
better to do it now while we don't have lots of callers,
instead of not changing them at all because it's "too late"
and everyone got used to it.

Thanks,

/mjt

Re: [Qemu-devel] [PATCHv2 3/7] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent

[Paolo Bonzini]

Il 11/03/2012 02:49, 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.
> 
> Semantic change in the meaning of `count' (now renamed to `sbytes')
> argument.  Initial comment said that src is copied to dst until
> _total_ size is less than specified, so it might be interpreted
> as maximum size of the _dst_ vector.  Actual meaning of if was
> that total amount of skipped and copied bytes should not exceed
> `count'.  Make it just the amount of bytes to _copy_, without
> counting skipped bytes.  This makes it consistent with other
> iovec functions, and also matches actual _usage_ of this function.
> 
> 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
> paramere to allow resetting dst in one go.

Yes, this initially left me a bit confused.

Let's add a new function qemu_iovec_copy that does reset+concat.

Paolo

Re: [Qemu-devel] [PATCHv2 5/7] Export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()

[Paolo Bonzini]

Il 11/03/2012 02:49, Michael Tokarev ha scritto:
> Rename do_sendv_recvv() to qemu_sendv_recvv(),
> change its last arg (do_send) 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.

GCC is smart and knows how to do tail calls in many cases.  Thus, I
don't see very much the point of this patch.

Paolo

Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Paolo Bonzini]

Il 11/03/2012 02:49, 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.
> Also constify buf arg of qemu_co_send().
> 
> qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now
> trivial #define's merely adding one extra arg.  qemu_co_send()
> is an inline function due to `buf' arg de-constification.

Again, I don't see the point in using #defines.  Either leave the
function static, or you can export it but then inlines are preferrable.

> qemu_co_sendv() and qemu_co_recvv() callers are converted to
> different argument order.

Paolo

Re: [Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions

[Paolo Bonzini]

Il 11/03/2012 02:49, Michael Tokarev ha scritto:
> 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.
> 
> 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,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:
> 
>    int 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 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'.
> 
> 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 isn't done in this series)
> 
> Michael Tokarev (7):
>   Consolidate qemu_iovec_memset{,_skip}() into single, simplified function
>   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 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             |    8 +-
>  block/curl.c        |    4 +-
>  block/nbd.c         |    4 +-
>  block/qcow.c        |    2 +-
>  block/qcow2.c       |   14 ++--
>  block/qed.c         |   10 +-
>  block/sheepdog.c    |    6 +-
>  block/vdi.c         |    2 +-
>  cutils.c            |  275 +++++++++++++++++++++------------------------------
>  hw/9pfs/virtio-9p.c |    8 +-
>  linux-aio.c         |    4 +-
>  posix-aio-compat.c  |    2 +-
>  qemu-common.h       |   64 +++++++-----
>  qemu-coroutine-io.c |   83 ++++-----------
>  14 files changed, 205 insertions(+), 281 deletions(-)
> 

Looks good, except that I don't like #defines.  (I also don't like
exporting qemu_sendv_recvv, but I can live with it. :))  I commented on
the single patches.

I'm happy to take 4-7 via the NBD tree.

Paolo

Re: [Qemu-devel] [PATCHv2 5/7] Export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()

[Michael Tokarev]

On 11.03.2012 19:00, Paolo Bonzini wrote:
> Il 11/03/2012 02:49, Michael Tokarev ha scritto:
>> Rename do_sendv_recvv() to qemu_sendv_recvv(),
>> change its last arg (do_send) 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.
> 
> GCC is smart and knows how to do tail calls in many cases.  Thus, I
> don't see very much the point of this patch.

The point is to allow qemu_sendv_recvv() to be used
directly, see the next patch
 [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
for an example, and see my previous attempt to address
all this with bdrv_* methods where reads and writes
are implemented in common functions and are split
back on layer boundary, just to go to a common
routine on the next layer.  Or worse yet, repeating
exactly the same code like in this 6/7 patch for
qemu_co_recvv() and qemu_co_sendv().

It is not about tail calls at all.

Thanks,

/mjt

Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Michael Tokarev]

On 11.03.2012 19:01, Paolo Bonzini wrote:
> Il 11/03/2012 02:49, 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.
>> Also constify buf arg of qemu_co_send().
>>
>> qemu_co_sendv(), qemu_co_recvv(), and qemu_co_recv() are now
>> trivial #define's merely adding one extra arg.  qemu_co_send()
>> is an inline function due to `buf' arg de-constification.
> 
> Again, I don't see the point in using #defines.  Either leave the
> function static, or you can export it but then inlines are preferrable.

When you're debugging in gdb, it always enters all inline functions.
For a very simple #define there's nothing to enter -- hence the
#define.

Note that - I still hope - in the end there will be no sendv or
recv calls at all, only common sendv_recvv with is_write passed
as an argument from upper layer.  It will be easier to remove
that #define - just two lines of code instead of minimum 5 :)

Also, in such cases like these, #defines are more compact and
does not clutter the header too much.

>> qemu_co_sendv() and qemu_co_recvv() callers are converted to
>> different argument order.
> 
> Paolo

Thanks,

/mjt

Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Paolo Bonzini]

Il 11/03/2012 16:26, Michael Tokarev ha scritto:
> Note that - I still hope - in the end there will be no sendv or
> recv calls at all, only common sendv_recvv with is_write passed
> as an argument from upper layer.  It will be easier to remove
> that #define - just two lines of code instead of minimum 5 :)

This is what I don't really like in the second part of these patches.
You are doing changes for the sake of other changes which are not
upstream yet, for which there is no clear vision, and for which there is
no clear benefit.

While I agree that there is a lot of duplicated code in block.c and
block/*, I don't think that what we need is more parameters to the
functions.  We have places where we need to know the request flags, for
example, but the methods are already quite unwieldy and have a lot of
arguments.  So I'm not sure that this kind of unification buys anything.

On top of this, your patches unify things that are similar but not quite
identical, and you do not provide hints in the commit messages that you
did so.  For example, qemu_co_recvv has handling for receiving 0, but
qemu_co_sendv does not.

Can you please separate the changes to make the argument order uniform?
 Those should be easy to get in.

Paolo

Re: [Qemu-devel] [PATCHv2 1/7] Consolidate qemu_iovec_memset{, _skip}() into single, simplified function

[Kevin Wolf]

Am 11.03.2012 02:49, schrieb Michael Tokarev:
> This patch combines two functions into one, simplifies the
> implementation and adds some assert()s into place.
> 
> The new prototype of qemu_iovec_memset():
>   void 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.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block/qcow2.c      |    4 ++--
>  block/qed.c        |    4 ++--
>  cutils.c           |   50 ++++++++++++++------------------------------------
>  linux-aio.c        |    4 ++--
>  posix-aio-compat.c |    2 +-
>  qemu-common.h      |    4 +---
>  6 files changed, 22 insertions(+), 46 deletions(-)

> diff --git a/cutils.c b/cutils.c
> index af308cd..9451c86 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -260,46 +260,24 @@ 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(QEMUIOVector *qiov, size_t offset, int c, size_t bytes)
>  {
> -    size_t n;
> +    struct iovec *iov = qiov->iov;
>      int i;
> +    assert(qiov->size >= offset);
> +    assert(qiov->size - offset >= bytes);
>  
> -    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;
> +    /* first skip initial full-sized elements */
> +    for(i = 0; offset >= iov[i].iov_len; ++i) {
> +	offset -= iov[i].iov_len;
>      }

This doesn't check i < qiov->niov any more. It's probably safe because
you added the assertions above. I didn't check if the assertions can
trigger anywhere, but they do constitute an interface change.

Also, indentation is off (tabs instead of spaces).

> -}
> -
> -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;
> +    /* skip/memset partial element and memset the rest */
> +    while(bytes) {
> +	size_t n = MIN(bytes, iov[i].iov_len - offset);
> +	memset((char*)iov[i].iov_base + offset, c, n);
> +	bytes -= n;
> +	++i;
> +	offset = 0;
>      }
>  }

The memsetting logic looks correct, but again indentation is off.

Kevin

Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Michael Tokarev]

On 12.03.2012 17:30, Paolo Bonzini wrote:
> Il 11/03/2012 16:26, Michael Tokarev ha scritto:
>> Note that - I still hope - in the end there will be no sendv or
>> recv calls at all, only common sendv_recvv with is_write passed
>> as an argument from upper layer.  It will be easier to remove
>> that #define - just two lines of code instead of minimum 5 :)
> 
> This is what I don't really like in the second part of these patches.
> You are doing changes for the sake of other changes which are not
> upstream yet, for which there is no clear vision, and for which there is
> no clear benefit.

I already posted the example of this.  I can complete whole thing
and send it all in one huge chunk if you prefer that :)

> While I agree that there is a lot of duplicated code in block.c and
> block/*, I don't think that what we need is more parameters to the
> functions.  We have places where we need to know the request flags, for
> example, but the methods are already quite unwieldy and have a lot of
> arguments.  So I'm not sure that this kind of unification buys anything.

Which places are these?

Also, how about dropping nr_sectors?

If you need flags, well, the extra argument being
added can really be used for that if necessary.

> On top of this, your patches unify things that are similar but not quite
> identical, and you do not provide hints in the commit messages that you
> did so.  For example, qemu_co_recvv has handling for receiving 0, but
> qemu_co_sendv does not.

This is a bug, which I dind't notice, it shouldn't
be there.  Somehow I overlooked this difference, but
I really wondered how they're differ!  By using common
code here it becomes much more obvious (whith the bug
actually fixed).

I'll take a look at other similar things here and
in my block layer changes.

> Can you please separate the changes to make the argument order uniform?
>  Those should be easy to get in.

While at it I found another "library", iov.[ch], which
has another implementation of the same thing.  So that's
where it all should have been started.

I'll resend a v3 covering iov_* functions too.

Thank you!

/mjt

Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Paolo Bonzini]

Il 12/03/2012 17:29, Michael Tokarev ha scritto:
>> For example, qemu_co_recvv has handling for receiving 0, but
>> > qemu_co_sendv does not.
> This is a bug, which I dind't notice, it shouldn't
> be there.  Somehow I overlooked this difference, but
> I really wondered how they're differ!  By using common
> code here it becomes much more obvious (whith the bug
> actually fixed).

write should never return 0, read does for end-of-file.

So your code was actually correct in some sense.

>> This is what I don't really like in the second part of these patches.
>> You are doing changes for the sake of other changes which are not
>> upstream yet, for which there is no clear vision, and for which there is
>> no clear benefit.
> I already posted the example of this.  I can complete whole thing
> and send it all in one huge chunk if you prefer that 
> 
>> While I agree that there is a lot of duplicated code in block.c and
>> block/*, I don't think that what we need is more parameters to the
>> functions.  We have places where we need to know the request flags, for
>> example, but the methods are already quite unwieldy and have a lot of
>> arguments.  So I'm not sure that this kind of unification buys anything.
> Which places are these?

If we turn zero-write into a special case of discard, we will need it as
a flag in discard.  Block mirroring would like to have access to
copy-on-read flags.

> Also, how about dropping nr_sectors?
> 
> If you need flags, well, the extra argument being
> added can really be used for that if necessary.

Or we can actually clean up everything, and create a real "request"
structure that can be passed along.  See how flush and discard are
really similar (which do not have all the accumulated stuff for
throttling, copy-on-read, bounce buffering, etc.).  Perhaps that's the
place to start.

Paolo