On Wed, Apr 02, 2025 at 07:23:34PM +0200, Laurent Vivier wrote:Use packet_base() and extract headers using IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() rather than packet_get(). Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- iov.c | 1 - udp.c | 37 ++++++++++++++++++++++++++----------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/iov.c b/iov.c index 508fb6da91fb..0c69379316aa 100644 --- a/iov.c +++ b/iov.c @@ -173,7 +173,6 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) * Returns: The number of elements successfully copied to the destination * iov array. */ -/* 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) diff --git a/udp.c b/udp.c index 80520cbdf188..03906d72e75c 100644 --- a/udp.c +++ b/udp.c @@ -858,15 +858,20 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, struct iovec m[UIO_MAXIOV]; const struct udphdr *uh; struct udp_flow *uflow; - int i, s, count = 0; + int i, j, s, count = 0; + struct iov_tail data; flow_sidx_t tosidx; in_port_t src, dst; + struct udphdr uhc; uint8_t topif; socklen_t sl; ASSERT(!c->no_udp); - uh = packet_get(p, idx, 0, sizeof(*uh), NULL); + if (!packet_base(p, idx, &data)) + return 1; + + uh = IOV_PEEK_HEADER(&data, uhc); if (!uh) return 1; @@ -903,23 +908,33 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif, pif_sockaddr(c, &to_sa, &sl, topif, &toside->eaddr, toside->eport); - for (i = 0; i < (int)p->count - idx; i++) { - struct udphdr *uh_send; - size_t len; + for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) { + const struct udphdr *uh_send; + + if (!packet_base(p, idx + i, &data)) + return p->count - idx; - uh_send = packet_get(p, idx + i, 0, sizeof(*uh), &len); + uh_send = IOV_REMOVE_HEADER(&data, uhc); if (!uh_send) return p->count - idx; + iov_tail_prune(&data);Maybe we should make remove_header always prune, rather than having to do it manually.+ + if (data.cnt + j >= UIO_MAXIOV)Obviously it's equivalent, but this would make more intuitive sense to me as (j + data.cnt): the amount we've already used in our array, plus the extra we're going to need.+ return p->count - idx; + mm[i].msg_hdr.msg_name = &to_sa; mm[i].msg_hdr.msg_namelen = sl; - if (len) { - m[i].iov_base = (char *)(uh_send + 1); - m[i].iov_len = len; + if (data.cnt ) {Stray space.+ unsigned int len; + + len = iov_copy(&m[j], UIO_MAXIOV - j, + &data.iov[0], data.cnt, data.off, SIZE_MAX);So, as I mentioned on iov_copy(), it doesn't obviously report the case where it copies less than expected not because the source is missing data, but because the destination doesn't have enough iovecs. You've avoided this case with a test above, but that's a bit non-obvious, it would be nice if we could just call this and check for an error.- mm[i].msg_hdr.msg_iov = m + i; - mm[i].msg_hdr.msg_iovlen = 1; + mm[i].msg_hdr.msg_iov = &m[j]; + mm[i].msg_hdr.msg_iovlen = len; + j += len; } else { mm[i].msg_hdr.msg_iov = NULL; mm[i].msg_hdr.msg_iovlen = 0;-- 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