Re: [PATCH v5 02/29] iov: Introduce iov_slice(), iov_tail_slice() and iov_tail_drop().

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Apr 2025 18:51:09 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> iov_tail_drop() discards a header from the iov_tail,
> 
> iov_slice() makes a new iov referencing a subset of the data
> in the original iov.
> 
> iov_tail_slice() is a wrapper for iov_slice() using iov_tail
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  iov.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  iov.h |  6 ++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/iov.c b/iov.c
> index 8c63b7ea6e31..047fcbce7fcd 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -156,6 +156,59 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt)
>  	return len;
>  }
>  
> +/**
> + * iov_slice -  Make a new iov referencing a subset of the data in the original iov

Nit: iov_slice() - Make ...

> + *
> + * @dst_iov:	 Pointer to the destination array of struct iovec describing
> + *		 the scatter/gather I/O vector to copy to.

From here on, you start mentioning "copy", but, I realised later,
there's no "deep" copy done here. Perhaps a mix of "shallow copy" or
"to duplicate references to [...]" etc. would be less confusing?

> + * @dst_iov_cnt: Maximum number of elements in the destination iov array.
> + * @iov:	 Pointer to the source array of struct iovec describing
> + *		 the scatter/gather I/O vector to copy from.
> + * @iov_cnt:	 Maximum number of elements in the source iov array.
> + * @offset:	 Offset within the source iov from where copying should start.

I would mention explicitly this is only for the first iov, otherwise it
might look like it's a fixed offset that applies to all elements
(...until you find the offset = 0 assignment below), say:

 * @offset:	Offset of the first source iov from where copying should start

> + * @bytes:	 On input, total number of bytes to copy from @iov to @dst_iov,
> + * 		 if @bytes is NULL, copy all the content of @iov,
> + * 		 on output actual number of bytes copied.

It's nice to avoid one additional argument, but to me it looks quite
convoluted, especially as I started reviewing the next patches. What
about:

 * @len:	Maximum byte count of @iov to map onto @dst_iov, 0 means unlimited
 * @copied:	If given, number of bytes referenced by @dst_iov, set on return

> + *
> + * Returns:	 The number of elements successfully copied to the destination
> + *		 iov array, a negative value if there is not enough room in the
> + *		 destination iov array
> + */
> +/* cppcheck-suppress staticFunction */
> +ssize_t iov_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
> +		  const struct iovec *iov, size_t iov_cnt,
> +		  size_t offset, size_t *bytes)
> +{
> +	unsigned int i, j;
> +	size_t remaining = bytes ? *bytes : SIZE_MAX;

Declarations from longest to shortest.

> +
> +	i = iov_skip_bytes(iov, iov_cnt, offset, &offset);
> +
> +	/* create a new iov array referencing a subset of the source one  */

...that's what I would have expected, but actually, the destination iov
is already there. Maybe (also in the function description) use "assign
iov references" rather than "create" or "make a new iov"?

> +	for (j = 0; i < iov_cnt && j < dst_iov_cnt && remaining; i++) {

'j' progresses with 'i'. Strictly speaking, you could use a single
counter, but it might complicate things later. Still, why shouldn't j
be post-decremented together with i, say:

	for (j = 0; i < iov_cnt && j < dst_iov_cnt && remaining; i++, j++) {

?

> +		size_t len = MIN(remaining, iov[i].iov_len - offset);
> +
> +		dst_iov[j].iov_base = (char *)iov[i].iov_base + offset;
> +		dst_iov[j].iov_len = len;
> +		j++;
> +		remaining -= len;
> +		offset = 0;
> +	}
> +	if (j == dst_iov_cnt) {
> +		if (bytes) {
> +			if (remaining)
> +				return -1;
> +		} else if (i != iov_cnt) {
> +			return -1;

...this looks subtle. I'm wondering: what if we skipped enough bytes,
so that i != iov_cnt but we're now referencing everything from dst_iov
as expected? Say that we get as input:

- iov[0]: abcd, iov[1]: efgh
- offset: 4
- dst_iov_cnt: 1
- iov_cnt: 2 (it's iov[0] and iov[1])

now we have:

- iov_dst[0]: efgh
- j == 1
- i == 1, iov_cnt == 2

...but we did "copy" everything, right? Unless I'm missing something,
we should store the result from iov_skip_bytes() (say, 'skipped'), and
then subtract it here.

Actually, the whole "error checking" here looks a bit fragile, and I
wonder if we shouldn't rather always set 'remaining' to the remaining
bytes (even if it's a bit of a waste, when bytes is NULL), then signal
failure whenever remaining != 0.

> +		}
> +	}
> +
> +	if (bytes)
> +		*bytes -= remaining;

So, assuming it's desired (I'm fairly sure it is from the next
patches), 'bytes' is an upper limit, rather than the number of bytes we
_must_ copy (as the current function comment would seem to imply).

If it's not desired, then we should also return error if we "copied"
less than 'bytes'.

> +
> +	return j;
> +}
> +
>  /**
>   * iov_tail_prune() - Remove any unneeded buffers from an IOV tail
>   * @tail:	IO vector tail (modified)
> @@ -191,6 +244,21 @@ size_t iov_tail_size(struct iov_tail *tail)
>  	return iov_size(tail->iov, tail->cnt) - tail->off;
>  }
>  
> +/**
> + * iov_tail_drop() - Discard a header from an IOV tail

Mixing 'header' with 'tail' makes this a bit confusing. Yes, these
vectors are used for headers, but what about calling it simply 'item',
to match the context of this function?

> + * @tail:	IO vector tail
> + * @len:	length to move the head of the tail
> + *
> + * Returns:	true if the tail still contains any bytes, otherwise false
> + */
> +/* cppcheck-suppress unusedFunction */
> +bool iov_tail_drop(struct iov_tail *tail, size_t len)
> +{
> +	tail->off = tail->off + len;
> +
> +	return iov_tail_prune(tail);
> +}
> +
>  /**
>   * iov_peek_header_() - Get pointer to a header from an IOV tail
>   * @tail:	IOV tail to get header from
> @@ -247,3 +315,27 @@ void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align)
>  	tail->off = tail->off + len;
>  	return p;
>  }
> +
> +/**
> + * iov_tail_slice -  Make a new iov referencing a subset of the data

iov_tail_slice() - ...

Same comment as above: we're just setting references in a passed iov,
not really "making" one.

> + *		     in an iov_tail
> + *
> + * @dst_iov:     Pointer to the destination array of struct iovec describing
> + *		 the scatter/gather I/O vector to copy to.
> + * @dst_iov_cnt: Maximum number of elements in the destination iov array.
> + * @tail:	 Pointer to the source iov_tail
> + * @bytes:	 On input, total number of bytes to copy from @iov to @dst_iov,
> + * 		 if @bytes is NULL, copy all the content of @iov,
> + * 		 on output actual number of bytes copied.
> + *
> + * Returns:      The number of elements successfully copied to the destination
> + *		 iov array, a negative value if there is not enough room in the
> + *		 destination iov array
> + */
> +/* cppcheck-suppress unusedFunction */
> +int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
> +                   struct iov_tail *tail, size_t *bytes)
> +{
> +	return iov_slice(dst_iov, dst_iov_cnt, &tail->iov[0], tail->cnt,
> +			 tail->off, bytes);
> +}
> diff --git a/iov.h b/iov.h
> index 9855bf0c0c32..809f84a2c0a0 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -28,6 +28,9 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>  size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>                    size_t offset, void *buf, size_t bytes);
>  size_t iov_size(const struct iovec *iov, size_t iov_cnt);
> +ssize_t iov_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
> +		  const struct iovec *iov, size_t iov_cnt,
> +		  size_t offset, size_t *bytes);
>  
>  /*
>   * DOC: Theory of Operation, struct iov_tail
> @@ -72,8 +75,11 @@ struct iov_tail {
>  
>  bool iov_tail_prune(struct iov_tail *tail);
>  size_t iov_tail_size(struct iov_tail *tail);
> +bool iov_tail_drop(struct iov_tail *tail, size_t len);
>  void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align);
>  void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align);
> +int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
> +		   struct iov_tail *tail, size_t *bytes);
>  
>  /**
>   * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail

-- 
Stefano