Re: [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended

[Stefan Hajnoczi]

On Tue, May 28, 2019 at 11:45:43AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/util/iov.c b/util/iov.c
> index 74e6ca8ed7..6bfd609998 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -353,6 +353,95 @@ void qemu_iovec_concat(QEMUIOVector *dst,
>      qemu_iovec_concat_iov(dst, src->iov, src->niov, soffset, sbytes);
>  }
>  
> +/*
> + * qiov_find_iov
> + *
> + * Return iov, where byte at @offset (in @qiov) is.
> + * Update @offset to be offset inside that iov to the smae byte.

s/smae/same/

> + */
> +static struct iovec *qiov_find_iov(QEMUIOVector *qiov, size_t *offset)
> +{
> +    struct iovec *iov = qiov->iov;
> +
> +    assert(*offset < qiov->size);
> +
> +    while (*offset >= iov->iov_len) {
> +        *offset -= iov->iov_len;
> +        iov++;
> +    }
> +
> +    return iov;
> +}
> +
> +/*
> + * qiov_slice
> + *
> + * Fund subarray of iovec's, containing requested range. @head would

s/Fund/Find/

> + * be offset in first iov (retruned by the function), @tail would be

s/retruned/returned/

> + * count of extra bytes in last iov (returned iov + @niov - 1).
> + */
> +static struct iovec *qiov_slice(QEMUIOVector *qiov,
> +                                size_t offset, size_t len,
> +                                size_t *head, size_t *tail, int *niov)
> +{
> +    struct iovec *iov = qiov_find_iov(qiov, &offset), *end_iov;
> +    size_t end_offset;
> +
> +    assert(offset + len <= qiov->size);

offset has already been modified by qiov_find_iov() in iov's
initializer so this comparison is meaningless.  Fix:

  struct iovec *iov;
  struct iovec *end_iov;
  size_t end_offset;

  assert(offset + len <= qiov->size);

  iov = qiov_find_iov(qiov, &offset);

Perhaps qiov_find_iov() shouldn't reuse the offset argument for two
different things (the offset from the beginning of qiov and the offset
from the beginning of the returned iovec).  This would eliminate this
class of bugs.

> +
> +    end_offset = iov->iov_len;
> +    end_iov = iov + 1;
> +
> +    while (end_offset - offset < len) {
> +        end_offset += end_iov->iov_len;
> +        end_iov++;
> +    }

Hmm...this looks like qiov_find_iov().  Can this function be implemented
in less code using two calls to qiov_find_iov() to find the first and
last iovecs?

signature.asc
Description: PGP signature