On Wed, Apr 02, 2025 at 07:23:27PM +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. Also introduce iov_tail_ptr(), iov_drop() and iov_copy() for later use.Not sure if these belong in the same patch.Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- iov.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++---- iov.h | 53 ++++++++++++++++------ tcp_buf.c | 2 +- 3 files changed, 163 insertions(+), 25 deletions(-) diff --git a/iov.c b/iov.c index 8c63b7ea6e31..d96fc2ab594b 100644 --- a/iov.c +++ b/iov.c @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, * * Returns: The number of bytes successfully copied. */ -/* cppcheck-suppress unusedFunction */ +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, size_t offset, void *buf, size_t bytes) { @@ -126,6 +126,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) */ memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset, len); copied += len; @@ -156,6 +157,45 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) return len; } +/** + * iov_copy - Copy data from one scatter/gather I/O vector (struct iovec) to + * another.I think this function name is slightly misleading and the description is very misleading. What the function does is make a new iov referencing a subset of the data in the original iov. It doesn't copy the reference data - the dst iov will alias (some of) the same memory as the src iov. I'd suggest iov_slice() maybe?+ * @dst_iov: Pointer to the destination array of struct iovec describing + * the scatter/gather I/O vector to copy to. + * @dst_iov_cnt: Number of elements in the destination iov array.Maybe worth emphasising that this is a maximum, we might not use all of them.+ * @iov: Pointer to the source array of struct iovec describing + * the scatter/gather I/O vector to copy from. + * @iov_cnt: Number of elements in the source iov array. + * @offset: Offset within the source iov from where copying should start. + * @bytes: Total number of bytes to copy from iov to dst_iov. + * + * Returns: The number of elements successfully copied to the destination + * iov array.So, this is obviously useful, because it's effectively the "after" value of dst_iov. However, it doesn't necessarily distinguish the (AFAICT) three exit conditions: 1) "success"; iov_size(dst_iov) == bytes afterwards 2) iov_size(dst_iov) < bytes, because we ran out of data in the source iov 3) iov_size(dst_iov) < bytes, because we needed more than @dst_iov_cnt "slots" to reference that much of @iov. On "failure", whether it's (2) or (3) seems like it could matter, so it would be nice if we can deduce that from the return value.+ */ +/* cppcheck-suppress unusedFunction */ +unsigned iov_copy(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; + + i = iov_skip_bytes(iov, iov_cnt, offset, &offset); + + /* copying data */ + for (j = 0; i < iov_cnt && j < dst_iov_cnt && bytes; i++) { + size_t len = MIN(bytes, iov[i].iov_len - offset); + + dst_iov[j].iov_base = (char *)iov[i].iov_base + offset; + dst_iov[j].iov_len = len; + j++; + bytes -= len; + offset = 0; + } + + return j; +} + /** * iov_tail_prune() - Remove any unneeded buffers from an IOV tail * @tail: IO vector tail (modified) @@ -192,7 +232,50 @@ size_t iov_tail_size(struct iov_tail *tail) } /** - * iov_peek_header_() - Get pointer to a header from an IOV tail + * iov_drop() - update head of the tailCute phrasing, but kind of confusing. I think this wants similar phrasing to remove_header, since it's doing basically the same thing, but discarding rather than retreiving the data.+ * @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_drop(struct iov_tail *tail, size_t len) +{ + tail->off = tail->off + len; + + return iov_tail_prune(tail); +} + +/** + * iov_tail_ptr() - get a pointer to the head of the tail + * @tail: IO vector tail + * @off: Byte offset in the tail to skip + * @len: Length of the data to get, in bytes + * + * Returns: pointer to the data in the tail, NULL if the + * tail doesn't contains @len bytes.This is a dangerous interface, because it returns a pointer with no indication of how many bytes after it are actually valid. I can see it being useful internally, but I don't think it should be exported.+ */ +/* cppcheck-suppress unusedFunction */ +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len) +{ + const struct iovec *iov; + size_t cnt, i; + + i = iov_skip_bytes(tail->iov, tail->cnt, tail->off + off, &off); + if (i == tail->cnt) + return NULL; + + iov = &tail->iov[i]; + cnt = tail->cnt - i; + + if (iov_size(iov, cnt) < off + len) + return NULL; + + return (char *)iov[0].iov_base + off; +} + +/** + * 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 @@ -203,8 +286,7 @@ size_t iov_tail_size(struct iov_tail *tail) * 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; @@ -225,7 +307,36 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) } /** - * iov_remove_header_() - Remove a header from an IOV tail + * iov_peek_header_ - Get pointer to a header from an IOV tail + * @tail: IOV tail to get header fromNeeds a description for @v.+ * @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. + * + * Returns: 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)Needs description of @v.* @len: Length of header to remove, in bytes * @align: Required alignment of header, in bytes @@ -233,17 +344,19 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) * On success, @tail is updated so that it longer includes the bytes of the * returned header. * - * Returns: 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. + * Returns: 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; + return p; } diff --git a/iov.h b/iov.h index 9855bf0c0c32..f9b5fdf0803e 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); +unsigned iov_copy(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 @@ -70,38 +73,60 @@ 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_)) + bool iov_tail_prune(struct iov_tail *tail); size_t iov_tail_size(struct iov_tail *tail); -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); +bool iov_drop(struct iov_tail *tail, size_t len); +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); +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len); /** * 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. * - * Returns: Pointer of type (@type_ *) located at the start of @tail_, NULL if - * we can't get a contiguous and aligned pointer. + * Returns: Pointer of type (@type_ *) located at the start of @tail_This is no longer quite accurate: the data is the same, but it's not necessarily a pointer into @tail_ proper. There's also still a NULL return case, just more limited than it was before.*/ -#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. * - * Returns: Pointer of type (@type_ *) located at the old start of @tail_, NULL - * if we can't get a contiguous and aligned pointer. + * Returns: Pointer of type (@type_ *) located at the old start of @tail_ + */ + +#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 + * + * Returns: 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_drop((tail_), sizeof(type_)) #endif /* IOVEC_H */ diff --git a/tcp_buf.c b/tcp_buf.c index 05305636b503..4bcc1acb245a 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 thc, *th = IOV_REMOVE_HEADER(&tail, thc); 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