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

[Anthony Liguori <anthony@codemonkey.ws>]

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