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

On Thu, 17 Apr 2025 18:51:09 +0200 Laurent Vivier 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 --- 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