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

[Michael Tokarev]

On 17.03.2012 02:12, Paolo Bonzini wrote:
[]
>> 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.

*sigh* Which changes?

I'm sorry but i ran past any sane time limits for this long ago.

Come on, which ANOTHER changes do you guys need for all this TRIVIAL stuff ???
Can you describe in a few words please?

Thanks,

/mjt

> 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();
>> +}
>