Re: [Qemu-devel] [PATCHv5 03/11] rewrite iov_* functions

[Paolo Bonzini]

Il 16/03/2012 22:34, Michael Tokarev ha scritto:
> 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.
> 
> While at it, add a unit-test file test-iov.c,
> to check various corner cases with iov_from_buf()
> and iov_to_buf() (all 3 functions uses exactly
> the same logic so iov_memset() should be covered
> too).
> 
> Signed-off-by: Michael Tokarev <address@hidden>
> ---
>  iov.c      |   91 +++++++++++++++--------------------
>  iov.h      |   12 ++++-
>  test-iov.c |  158 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Makefile changes missing.

Also, please use g_test_rand_int_range to generate random numbers inside
GTester.  The .xml output will include the seed so that failures are
reproducible.

http://developer.gnome.org/glib/2.30/glib-Testing.html#g-test-rand-int

Paolo

>  3 files changed, 207 insertions(+), 54 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index bc58cab..9657d28 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -7,6 +7,7 @@
>   * Author(s):
>   *  Anthony Liguori <address@hidden>
>   *  Amit Shah <address@hidden>
> + *  Michael Tokarev <address@hidden>
>   *
>   * 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; (offset || 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; (offset || 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; (offset || 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 <address@hidden>
> + *  Michael Tokarev <address@hidden>
>   *
>   * 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);
> diff --git a/test-iov.c b/test-iov.c
> new file mode 100644
> index 0000000..fd8f9fe
> --- /dev/null
> +++ b/test-iov.c
> @@ -0,0 +1,158 @@
> +#include <glib.h>
> +#include "qemu-common.h"
> +#include "iov.h"
> +
> +/* return a (pseudo-)random number in [min, max] range */
> +
> +static inline int rannum(int min, int max)
> +{
> +  return min + random() % (max - min + 1);
> +}
> +
> +/* create a randomly-sized iovec with random vectors */
> +static void iov_random(struct iovec **iovp, unsigned *iov_cntp)
> +{
> +     unsigned niov = rannum(3, 8);
> +     struct iovec *iov = g_malloc(niov * sizeof(*iov));
> +     unsigned i;
> +     for (i = 0; i < niov; ++i) {
> +         iov[i].iov_len = rannum(2, 10);
> +         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_free(ibuf-4);
> +    g_assert(!memcmp(obuf-4, "xxxx", 4) && !memcmp(obuf+sz, "yyyy", 4));
> +    g_free(obuf-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)
> +{
> +    srandom(getpid());
> +    g_test_init(&argc, &argv, NULL);
> +    g_test_add_func("/basic/iov/from-to-buf", test_to_from_buf);
> +    return g_test_run();
> +}