diff mbox

[PATCHv4,03/11] rewrite iov_* functions

Message ID 1331845217-21705-4-git-send-email-mjt@msgid.tls.msk.ru
State New
Headers show

Commit Message

Michael Tokarev March 15, 2012, 9 p.m. UTC
This changes implementations of all iov_*
functions, completing the previous step.

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

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 qemu 1.0).  So I left it this way and used
it in other places.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 iov.c |   91 ++++++++++++++++++++++++++++-------------------------------------
 iov.h |   12 +++++++-
 2 files changed, 49 insertions(+), 54 deletions(-)

Comments

Anthony Liguori March 16, 2012, 4:17 p.m. UTC | #1
On 03/15/2012 04:00 PM, Michael Tokarev wrote:
> This changes implementations of all iov_*
> functions, completing the previous step.
>
> 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".
>
> 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 qemu 1.0).  So I left it this way and used
> it in other places.
>
> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
> ---
>   iov.c |   91 ++++++++++++++++++++++++++++-------------------------------------
>   iov.h |   12 +++++++-
>   2 files changed, 49 insertions(+), 54 deletions(-)
>
> diff --git a/iov.c b/iov.c
> index bc58cab..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.
> @@ -17,75 +18,61 @@
>
>   #include "iov.h"
>
> -size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, size_t iov_off,
> -                    const void *buf, size_t size)
> +size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
> +                    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;
>   }

It makes me nervous to make a change like this as it has wide impact on the rest 
of the code base.  Could you include a unit test that tests the various boundary 
conditions of this code?

Regards,

Anthony Liguori
Anthony Liguori March 16, 2012, 4:17 p.m. UTC | #2
On 03/15/2012 04:00 PM, Michael Tokarev wrote:
> This changes implementations of all iov_*
> functions, completing the previous step.
>
> 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".
>
> 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 qemu 1.0).  So I left it this way and used
> it in other places.
>
> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
> ---
>   iov.c |   91 ++++++++++++++++++++++++++++-------------------------------------
>   iov.h |   12 +++++++-
>   2 files changed, 49 insertions(+), 54 deletions(-)
>
> diff --git a/iov.c b/iov.c
> index bc58cab..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.
> @@ -17,75 +18,61 @@
>
>   #include "iov.h"
>
> -size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, size_t iov_off,
> -                    const void *buf, size_t size)
> +size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
> +                    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;
>   }

It makes me nervous to make a change like this as it has wide impact on the rest 
of the code base.  Could you include a unit test that tests the various boundary 
conditions of this code?

Regards,

Anthony Liguori
Michael Tokarev March 16, 2012, 7:30 p.m. UTC | #3
On 16.03.2012 20:17, Anthony Liguori wrote:

> It makes me nervous to make a change like this as it has wide impact on the rest of the code base.  Could you include a unit test that tests the various boundary conditions of this code?

Is the below code a good unit test?

Thanks,

/mjt

------------------------

#include <glib.h>
#include "iov.h"
#include <sys/socket.h>
#include <netinet/in.h>

/* return a (pseudo-)random number in [min, max) range */

static inline int rannum(int min, int max)
{
  return min + random() % (max - min);
}

/* create a randomly-sized iovec with random vectors */
static void iov_random(struct iovec **iovp, unsigned *iov_cntp)
{
     unsigned niov = rannum(10, 20);
     struct iovec *iov = g_malloc(niov * sizeof(*iov));
     unsigned i;
     for (i = 0; i < niov; ++i) {
         iov[i].iov_len = rannum(20,40);
         iov[i].iov_base = g_malloc(iov[i].iov_len);
     }
     *iovp = iov;
     *iov_cntp = niov;
}

static void iov_free(struct iovec *iov, unsigned niov)
{
    unsigned i;
    for(i = 0; i < niov; ++i) {
        g_free(iov[i].iov_base);
    }
    g_free(iov);
}

static void test_iov_bytes(struct iovec *iov, unsigned niov,
                           size_t offset, size_t bytes)
{
    unsigned i;
    size_t j, o;
    unsigned char *b;
    o = 0;

    /* we walk over all elements, */
    for (i = 0; i < niov; ++i) {
        b = iov[i].iov_base;
        /* over each char of each element, */
        for(j = 0; j < iov[i].iov_len; ++j) {
            /* counting each of them and
             * verifying that the ones within [offset,offset+bytes)
             * range are equal to the position number (o) */
            if (o >= offset && o < offset + bytes) {
                g_assert(b[j] == (o & 255));
            }
            ++o;
        }
    }
}

static void test_to_from_buf_1(void)
{
     struct iovec *iov;
     unsigned niov;
     size_t sz;
     unsigned char *ibuf, *obuf;
     unsigned i, j, n;

     iov_random(&iov, &niov);
     sz = iov_size(iov, niov);

     ibuf = g_malloc(sz + 8) + 4;
     memcpy(ibuf-4, "aaaa", 4); memcpy(ibuf + sz, "bbbb", 4);
     obuf = g_malloc(sz + 8) + 4;
     memcpy(obuf-4, "xxxx", 4); memcpy(obuf + sz, "yyyy", 4);

     /* fill in ibuf with 0123456... */
     for (i = 0; i < sz; ++i) {
         ibuf[i] = i & 255;
     }

     for (i = 0; i <= sz; ++i) {

         /* Test from/to buf for offset(i) in [0..sz] up to the end of buffer.
          * For last iteration with offset == sz, the procedure should
          * skip whole vector and process exactly 0 bytes */

         /* first set bytes [i..sz) to some "random" value */
         n = iov_memset(iov, niov, 0, i&255, -1);
         g_assert(n == sz);

         /* next copy bytes [i..sz) from ibuf to iovec */
         n = iov_from_buf(iov, niov, i, ibuf + i, -1);
         g_assert(n == sz - i);

         /* clear part of obuf */
         memset(obuf + i, 0, sz - i);
         /* and set this part of obuf to values from iovec */
         n = iov_to_buf(iov, niov, i, obuf + i, -1);
         g_assert(n == sz - i);

         /* now compare resulting buffers */
         g_assert(memcmp(ibuf, obuf, sz) == 0);

         /* test just one char */
         n = iov_to_buf(iov, niov, i, obuf + i, 1);
         g_assert(n == (i < sz));
         if (n) {
             g_assert(obuf[i] == (i & 255));
         }

         for (j = i; j <= sz; ++j) {
             /* now test num of bytes cap up to byte no. j,
              * with j in [i..sz]. */

             /* clear iovec */
             n = iov_memset(iov, niov, 0, j&255, -1);
             g_assert(n == sz);

             /* copy bytes [i..j) from ibuf to iovec */
             n = iov_from_buf(iov, niov, i, ibuf + i, j - i);
             g_assert(n == j - i);

             /* clear part of obuf */
             memset(obuf + i, 0, j - i);

             /* copy bytes [i..j) from iovec to obuf */
             n = iov_to_buf(iov, niov, i, obuf + i, j - i);
             g_assert(n == j - i);

             /* verify result */
             g_assert(memcmp(ibuf, obuf, sz) == 0);

             /* now actually check if the iovec contains the right data */
             test_iov_bytes(iov, niov, i, j - i);
         }
    }
    g_assert(!memcmp(ibuf-4, "aaaa", 4) && !memcmp(ibuf+sz, "bbbb", 4));
    g_assert(!memcmp(obuf-4, "xxxx", 4) && !memcmp(obuf+sz, "yyyy", 4));
    iov_free(iov, niov);
}

static void test_to_from_buf(void)
{
    int x;
    /* repeat it several times with different (random) values */
    for(x = 0; x < 4; ++x) {
        test_to_from_buf_1();
    }
}

int main(int argc, char **argv)
{

  g_test_init(&argc, &argv, NULL);
  g_test_add_func("/basic/iov/from-to-buf", test_to_from_buf);

  return g_test_run();
}
diff mbox

Patch

diff --git a/iov.c b/iov.c
index bc58cab..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.
@@ -17,75 +18,61 @@ 
 
 #include "iov.h"
 
-size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt, size_t iov_off,
-                    const void *buf, size_t size)
+size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
+                    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, size_t iov_off,
-                  void *buf, size_t size)
+size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
+                  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_memset(const struct iovec *iov, const unsigned int iov_cnt,
-                 size_t iov_off, int fillc, size_t size)
+                  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), fillc, 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 0b4acf4..19ee3b3 100644
--- a/iov.h
+++ b/iov.h
@@ -1,10 +1,11 @@ 
 /*
- * Helpers for getting linearized buffers from iov / filling buffers into iovs
+ * Helpers for using (partial) iovecs.
  *
  * Copyright (C) 2010 Red Hat, Inc.
  *
  * Author(s):
  *  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.
@@ -28,6 +29,12 @@  size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
  * 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)
+ * `Offset' must point to the inside of iovec.
+ * 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,
                     size_t offset, const void *buf, size_t bytes);
@@ -37,11 +44,12 @@  size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
 /**
  * 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.
+ * `bytes' number of times.  `Offset' must point to the 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).
+ * Again, it is okay to use large value for `bytes' to mean "up to the end".
  */
 size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, int fillc, size_t bytes);