Re: [PATCH v8 09/30] icmp: Convert to iov_tail

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On Tue, Aug 05, 2025 at 05:46:07PM +0200, Laurent Vivier wrote:
> Use packet_data() and extract headers using IOV_PEEK_HEADER()
> rather than packet_get().
> 
> Introduce iov_tail_msghdr() to convert iov_tail to msghdr
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  icmp.c | 25 ++++++++++++++-----------
>  iov.c  | 23 +++++++++++++++++++++++
>  iov.h  |  2 ++
>  3 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 95f38c1e2a3a..fdfc857b5ae8 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -241,25 +241,27 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	struct icmp_ping_flow *pingf;
>  	const struct flowside *tgt;
>  	union sockaddr_inany sa;
> -	size_t dlen, l4len;
> +	struct iov_tail data;
> +	struct msghdr msh;
>  	uint16_t id, seq;
>  	union flow *flow;
>  	uint8_t proto;
>  	socklen_t sl;
> -	void *pkt;
>  
>  	(void)saddr;
>  	ASSERT(pif == PIF_TAP);
>  
> +	if (!packet_data(p, 0, &data))
> +		return -1;
> +
>  	if (af == AF_INET) {
> +		struct icmphdr ih_storage;
>  		const struct icmphdr *ih;
>  
> -		if (!(pkt = packet_get(p, 0, 0, sizeof(*ih), &dlen)))
> +		ih = IOV_PEEK_HEADER(&data, ih_storage);
> +		if (!ih)
>  			return 1;
>  
> -		ih =  (struct icmphdr *)pkt;
> -		l4len = dlen + sizeof(*ih);
> -
>  		if (ih->type != ICMP_ECHO)
>  			return 1;
>  
> @@ -267,14 +269,13 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  		id = ntohs(ih->un.echo.id);
>  		seq = ntohs(ih->un.echo.sequence);
>  	} else if (af == AF_INET6) {
> +		struct icmp6hdr ih_storage;
>  		const struct icmp6hdr *ih;
>  
> -		if (!(pkt = packet_get(p, 0, 0, sizeof(*ih), &dlen)))
> +		ih = IOV_PEEK_HEADER(&data, ih_storage);
> +		if (!ih)
>  			return 1;
>  
> -		ih = (struct icmp6hdr *)pkt;
> -		l4len = dlen + sizeof(*ih);
> -
>  		if (ih->icmp6_type != ICMPV6_ECHO_REQUEST)
>  			return 1;
>  
> @@ -298,8 +299,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	ASSERT(flow_proto[pingf->f.type] == proto);
>  	pingf->ts = now->tv_sec;
>  
> +

Bogus extra blank line.

>  	pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0);
> -	if (sendto(pingf->sock, pkt, l4len, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
> +	iov_tail_msghdr(&msh, &data, &sa, sl);
> +	if (sendmsg(pingf->sock, &msh, MSG_NOSIGNAL) < 0) {
>  		flow_dbg_perror(pingf, "failed to relay request to socket");
>  	} else {
>  		flow_dbg(pingf,
> diff --git a/iov.c b/iov.c
> index 97e4ea733540..9d99beb32532 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -158,6 +158,29 @@ 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);

Oof, this is a pretty nasty non-obvious constraint on calling this
function.  The whole point of iov_tails is the offset, but this
function won't work in that case.

> +	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 ccdb690ef3f1..75c3b07a87e3 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -82,6 +82,8 @@ 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);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]