On Tue, Aug 05, 2025 at 05:46:23PM +0200, Laurent Vivier wrote: > As the iov_tail has a non zero offset (because of the presence of > packet headers in the iov array), we must copy it to a new > iov array (using iov_tail_splice()) to pass it to sendmsg(). > > We cannot use anymore iov_tail_msghdr(), so remove it. Right, as I mentioned on that patch, it was always kind of a foot gun. Was this the only user? Maybe just open code it earlier in the series rather than introducing then removing the helper. > > Signed-off-by: Laurent Vivier Reviewed-by: David Gibson > --- > icmp.c | 30 +++++++++++++++++++----------- > icmp.h | 2 +- > iov.c | 23 ----------------------- > iov.h | 2 -- > tap.c | 7 ++----- > 5 files changed, 22 insertions(+), 42 deletions(-) > > diff --git a/icmp.c b/icmp.c > index 71c496540310..be800e30c369 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -44,6 +44,7 @@ > > #define ICMP_ECHO_TIMEOUT 60 /* s, timeout for ICMP socket activity */ > #define ICMP_NUM_IDS (1U << 16) > +#define MAX_IOV_ICMP 16 /* Arbitrary, should be enough */ > > /** > * ping_at_sidx() - Get ping specific flow at given sidx > @@ -229,36 +230,33 @@ cancel: > * @af: Address family, AF_INET or AF_INET6 > * @saddr: Source address > * @daddr: Destination address > - * @p: Packet pool, single packet with ICMP/ICMPv6 header > + * @data: Single packet with ICMP/ICMPv6 header > * @now: Current timestamp > * > * Return: count of consumed packets (always 1, even if malformed) > */ > int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > const void *saddr, const void *daddr, > - const struct pool *p, const struct timespec *now) > + struct iov_tail *data, const struct timespec *now) > { > + struct iovec iov[MAX_IOV_ICMP]; > struct icmp_ping_flow *pingf; > const struct flowside *tgt; > union sockaddr_inany sa; > - struct iov_tail data; > struct msghdr msh; > uint16_t id, seq; > union flow *flow; > uint8_t proto; > - socklen_t sl; > + int cnt; > > (void)saddr; > ASSERT(pif == PIF_TAP); > > - if (!packet_get(p, 0, &data)) > - return -1; > - > if (af == AF_INET) { > struct icmphdr ih_storage; > const struct icmphdr *ih; > > - ih = IOV_PEEK_HEADER(&data, ih_storage); > + ih = IOV_PEEK_HEADER(data, ih_storage); > if (!ih) > return 1; > > @@ -272,7 +270,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > struct icmp6hdr ih_storage; > const struct icmp6hdr *ih; > > - ih = IOV_PEEK_HEADER(&data, ih_storage); > + ih = IOV_PEEK_HEADER(data, ih_storage); > if (!ih) > return 1; > > @@ -286,6 +284,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > ASSERT(0); > } > > + cnt = iov_tail_clone(&iov[0], MAX_IOV_ICMP, data); > + if (cnt < 0) > + return 1; > + > flow = flow_at_sidx(flow_lookup_af(c, proto, PIF_TAP, > af, saddr, daddr, id, id)); > > @@ -300,8 +302,14 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > pingf->ts = now->tv_sec; > > > - pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0); > - iov_tail_msghdr(&msh, &data, &sa, sl); > + pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0); > + msh.msg_name = &sa; > + msh.msg_iov = iov; > + msh.msg_iovlen = cnt; > + msh.msg_control = NULL; > + msh.msg_controllen = 0; > + msh.msg_flags = 0; > + > if (sendmsg(pingf->sock, &msh, MSG_NOSIGNAL) < 0) { > flow_dbg_perror(pingf, "failed to relay request to socket"); > } else { > diff --git a/icmp.h b/icmp.h > index 5ce22b5eca1f..d1cecb20e29d 100644 > --- a/icmp.h > +++ b/icmp.h > @@ -14,7 +14,7 @@ struct icmp_ping_flow; > void icmp_sock_handler(const struct ctx *c, union epoll_ref ref); > int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > const void *saddr, const void *daddr, > - const struct pool *p, const struct timespec *now); > + struct iov_tail *data, const struct timespec *now); > void icmp_init(void); > > /** > diff --git a/iov.c b/iov.c > index d17d4dd3da09..1d734acdfea6 100644 > --- a/iov.c > +++ b/iov.c > @@ -157,29 +157,6 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) > return len; > } > > -/** > - * iov_tail_msghdr - Initialize a msghdr from an IOV tail structure > - * @msh: msghdr to initialize > - * @tail: iov_tail to use to set msg_iov and msg_iovlen > - * @msg_name: Pointer to set to msg_name > - * @msg_namelen: Size of @msg_name > - */ > -void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail, > - void *msg_name, socklen_t msg_namelen) > -{ > - iov_tail_prune(tail); > - > - ASSERT(tail->off == 0); > - > - msh->msg_name = msg_name; > - msh->msg_namelen = msg_namelen; > - msh->msg_iov = (struct iovec *)tail->iov; > - msh->msg_iovlen = tail->cnt; > - msh->msg_control = NULL; > - msh->msg_controllen = 0; > - msh->msg_flags = 0; > -} > - > /** > * iov_tail_prune() - Remove any unneeded buffers from an IOV tail > * @tail: IO vector tail (modified) > diff --git a/iov.h b/iov.h > index 75c3b07a87e3..ccdb690ef3f1 100644 > --- a/iov.h > +++ b/iov.h > @@ -82,8 +82,6 @@ struct iov_tail { > 1, \ > (off_)) > > -void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail, > - void *msg_name, socklen_t msg_namelen); > 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); > diff --git a/tap.c b/tap.c > index 3262b44c4287..48152a84674c 100644 > --- a/tap.c > +++ b/tap.c > @@ -764,17 +764,14 @@ resume: > continue; > > if (iph->protocol == IPPROTO_ICMP) { > - PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > - > if (c->no_icmp) > continue; > > tap_packet_debug(iph, NULL, NULL, 0, NULL, 1); > > - packet_add(pkt, &data); > icmp_tap_handler(c, PIF_TAP, AF_INET, > &iph->saddr, &iph->daddr, > - pkt, now); > + &data, now); > continue; > } > > @@ -964,7 +961,7 @@ resume: > tap_packet_debug(NULL, ip6h, NULL, proto, NULL, 1); > > icmp_tap_handler(c, PIF_TAP, AF_INET6, > - saddr, daddr, pkt, now); > + saddr, daddr, &data, now); > continue; > } > -- 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