Re: [PATCH v8 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()

On Tue, Aug 05, 2025 at 05:46:01PM +0200, Laurent Vivier wrote:

Provide a temporary variable of the wanted type to store the header if the memory in the iovec array is not contiguous.

Signed-off-by: Laurent Vivier --- iov.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---------- iov.h | 55 +++++++++++++++++++++++++++++++++++++++++-------------- tcp_buf.c | 2 +- 3 files changed, 87 insertions(+), 25 deletions(-)

diff --git a/iov.c b/iov.c index 9d282d4af461..d39bb099fa69 100644 --- a/iov.c +++ b/iov.c @@ -109,7 +109,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, * * Return: the number of bytes successfully copied. */ -/* cppcheck-suppress unusedFunction */ +/* cppcheck-suppress [staticFunction] */ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, size_t offset, void *buf, size_t bytes) { @@ -127,6 +127,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, /* copying data */ for (copied = 0; copied < bytes && i < iov_cnt; i++) { size_t len = MIN(iov[i].iov_len - offset, bytes - copied); + /* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */

Which parameter was cppcheck complaining about?

memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset, len); copied += len; @@ -208,7 +209,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len) }

/** - * iov_peek_header_() - Get pointer to a header from an IOV tail + * iov_check_header() - Check if a header can be accessed * @tail: IOV tail to get header from * @len: Length of header to get, in bytes * @align: Required alignment of header, in bytes @@ -219,8 +220,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len) * overruns the IO vector, is not contiguous or doesn't have the * requested alignment. */ -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t align) { char *p;

@@ -240,27 +240,62 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) return p; }

+/** + * iov_peek_header_() - Get pointer to a header from an IOV tail + * @tail: IOV tail to get header from + * @v: Temporary memory to use if the memory in @tail + * is discontinuous + * @len: Length of header to get, in bytes + * @align: Required alignment of header, in bytes + * + * @tail may be pruned, but will represent the same bytes as before. + * + * Return: pointer to the first @len logical bytes of the tail, or to + * a copy if that overruns the IO vector, is not contiguous or + * doesn't have the requested alignment. NULL if that overruns the + * IO vector. + */ +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align) +{ + char *p = iov_check_header(tail, len, align); + size_t l; + + if (p) + return p; + + l = iov_to_buf(tail->iov, tail->cnt, tail->off, v, len); + if (l != len) + return NULL; + + return v; +} + /** * iov_remove_header_() - Remove a header from an IOV tail * @tail: IOV tail to remove header from (modified) + * @v: Temporary memory to use if the memory in @tail + * is discontinuous * @len: Length of header to remove, in bytes * @align: Required alignment of header, in bytes * * On success, @tail is updated so that it longer includes the bytes of the * returned header. * - * Return: pointer to the first @len logical bytes of the tail, NULL if that - * overruns the IO vector, is not contiguous or doesn't have the - * requested alignment. + * Return: pointer to the first @len logical bytes of the tail, or to + * a copy if that overruns the IO vector, is not contiguous or + * doesn't have the requested alignment. NULL if that overruns the + * IO vector. */ -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align) +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align) { - char *p = iov_peek_header_(tail, len, align); + char *p = iov_peek_header_(tail, v, len, align);

if (!p) return NULL;

tail->off = tail->off + len;

Do you want to use your new iov_tail_drop() here?

+ return p; }

@@ -275,7 +310,7 @@ void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align) * * Return: the number of elements successfully referenced from the destination * iov array, a negative value if there is not enough room in the - * destination iov array + * destination iov array */ /* cppcheck-suppress unusedFunction */ ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt, diff --git a/iov.h b/iov.h index bf9820ac52ab..ccdb690ef3f1 100644 --- a/iov.h +++ b/iov.h @@ -70,41 +70,68 @@ struct iov_tail { #define IOV_TAIL(iov_, cnt_, off_) \ (struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) }

+/** + * IOV_TAIL_FROM_BUF() - Create a new IOV tail from a buffer + * @buf_: Buffer address to use in the iovec + * @len_: Buffer size + * @off_: Byte offset in the buffer where the tail begins + */ +#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \ + IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), \ + .iov_len = (len_) }), \ + 1, \ + (off_)) +

This seems unrelated to the rest of the patch, does it belong in a different one? Also, given that you're constructing the iovec entries yourself, why not set tail->off to zero and update iov_base and len accordingly.

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); +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align); +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align); ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt, struct iov_tail *tail);

/** * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail * @tail_: IOV tail to get header from - * @type_: Data type of the header + * @var_: Temporary buffer of the type of the header to use if + * the memory in the iovec array is not contiguous. * * @tail_ may be pruned, but will represent the same bytes as before. * - * Return: pointer of type (@type_ *) located at the start of @tail_, NULL if - * we can't get a contiguous and aligned pointer. + * Return: pointer of type (@type_ *) located at the start of @tail_ + * or to @var_ if iovec memory is not contiguous, NULL if + * that overruns the iovec.

The NULL case can't happen if given a variable of the right type.

*/ -#define IOV_PEEK_HEADER(tail_, type_) \ - ((type_ *)(iov_peek_header_((tail_), \ - sizeof(type_), __alignof__(type_)))) + +#define IOV_PEEK_HEADER(tail_, var_) \ + ((__typeof__(var_) *)(iov_peek_header_((tail_), &(var_), \ + sizeof(var_), \ + __alignof__(var_))))

/** * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail * @tail_: IOV tail to remove header from (modified) - * @type_: Data type of the header to remove + * @var_: Temporary buffer of the type of the header to use if + * the memory in the iovec array is not contiguous. * * On success, @tail_ is updated so that it longer includes the bytes of the * returned header. * - * Return: pointer of type (@type_ *) located at the old start of @tail_, NULL - * if we can't get a contiguous and aligned pointer. + * Return: pointer of type (@type_ *) located at the start of @tail_ + * or to @var_ if iovec memory is not contiguous, NULL if + * that overruns the iovec.

Again, NULL case can't happen.

+ */ + +#define IOV_REMOVE_HEADER(tail_, var_) \ + ((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_), \ + sizeof(var_), __alignof__(var_)))) + +/** IOV_DROP_HEADER() - Remove a typed header from an IOV tail + * @tail_: IOV tail to remove header from (modified) + * @type_: Data type of the header to remove + * + * Return: true if the tail still contains any bytes, otherwise false */ -#define IOV_REMOVE_HEADER(tail_, type_) \ - ((type_ *)(iov_remove_header_((tail_), \ - sizeof(type_), __alignof__(type_)))) +#define IOV_DROP_HEADER(tail_, type_) iov_tail_drop((tail_), sizeof(type_))

#endif /* IOVEC_H */ diff --git a/tcp_buf.c b/tcp_buf.c index d1fca676c9a7..bc898de86919 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -160,7 +160,7 @@ static void tcp_l2_buf_fill_headers(const struct tcp_tap_conn *conn, uint32_t seq, bool no_tcp_csum) { struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); - struct tcphdr *th = IOV_REMOVE_HEADER(&tail, struct tcphdr); + struct tcphdr th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage); struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; const struct flowside *tapside = TAPFLOW(conn); const struct in_addr *a4 = inany_v4(&tapside->oaddr);

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson