Re: [PATCH v9 20/30] tap: Convert tap6_handler() to iov_tail

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

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

On Fri, Aug 08, 2025 at 04:01:32PM +0200, Laurent Vivier wrote:
> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
> and IOV_PEEK_HEADER() rather than packet_get().
> 
> Remove packet_get() as it is not used anymore.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  packet.c | 70 --------------------------------------------------------
>  packet.h | 11 ---------
>  tap.c    | 25 ++++++++++++--------
>  3 files changed, 16 insertions(+), 90 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index 014b353cdf8b..5da18bafa576 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -121,76 +121,6 @@ void packet_add_do(struct pool *p, struct iov_tail *data,
>  	p->count++;
>  }
>  
> -/**
> - * packet_get_try_do() - Get data range from packet descriptor from given pool
> - * @p:		Packet pool
> - * @idx:	Index of packet descriptor in pool
> - * @offset:	Offset of data range in packet descriptor
> - * @len:	Length of desired data range
> - * @left:	Length of available data after range, set on return, can be NULL
> - * @func:	For tracing: name of calling function
> - * @line:	For tracing: caller line of function call
> - *
> - * Return: pointer to start of data range, NULL on invalid range or descriptor
> - */
> -/* cppcheck-suppress [staticFunction] */
> -void *packet_get_try_do(const struct pool *p, size_t idx, size_t offset,
> -			size_t len, size_t *left, const char *func, int line)
> -{
> -	char *ptr;
> -
> -	ASSERT_WITH_MSG(p->count <= p->size,
> -			"Corrupt pool count: %zu, size: %zu, %s:%i",
> -			p->count, p->size, func, line);
> -
> -	if (idx >= p->count) {
> -		debug("packet %zu from pool count: %zu, %s:%i",
> -		      idx, p->count, func, line);
> -		return NULL;
> -	}
> -
> -	if (offset > p->pkt[idx].iov_len ||
> -	    len > (p->pkt[idx].iov_len - offset))
> -		return NULL;
> -
> -	ptr = (char *)p->pkt[idx].iov_base + offset;
> -
> -	ASSERT_WITH_MSG(!packet_check_range(p, ptr, len, func, line),
> -			"Corrupt packet pool, %s:%i", func, line);
> -
> -	if (left)
> -		*left = p->pkt[idx].iov_len - offset - len;
> -
> -	return ptr;
> -}
> -
> -/**
> - * packet_get_do() - Get data range from packet descriptor from given pool
> - * @p:		Packet pool
> - * @idx:	Index of packet descriptor in pool
> - * @offset:	Offset of data range in packet descriptor
> - * @len:	Length of desired data range
> - * @left:	Length of available data after range, set on return, can be NULL
> - * @func:	For tracing: name of calling function
> - * @line:	For tracing: caller line of function call
> - *
> - * Return: as packet_get_try_do() but log a trace message when returning NULL
> - */
> -void *packet_get_do(const struct pool *p, const size_t idx,
> -		    size_t offset, size_t len, size_t *left,
> -		    const char *func, int line)
> -{
> -	void *r = packet_get_try_do(p, idx, offset, len, left, func, line);
> -
> -	if (!r) {
> -		trace("missing packet data length %zu, offset %zu from "
> -		      "length %zu, %s:%i",
> -		      len, offset, p->pkt[idx].iov_len, func, line);
> -	}
> -
> -	return r;
> -}
> -
>  /**
>   * packet_data_do() - Get data range from packet descriptor from given pool
>   * @p:		Packet pool
> diff --git a/packet.h b/packet.h
> index 062afb978124..dab8274fa5c5 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -33,12 +33,6 @@ struct pool {
>  int vu_packet_check_range(void *buf, const char *ptr, size_t len);
>  void packet_add_do(struct pool *p, struct iov_tail *data,
>  		   const char *func, int line);
> -void *packet_get_try_do(const struct pool *p, const size_t idx,
> -			size_t offset, size_t len, size_t *left,
> -			const char *func, int line);
> -void *packet_get_do(const struct pool *p, const size_t idx,
> -		    size_t offset, size_t len, size_t *left,
> -		    const char *func, int line);
>  bool packet_data_do(const struct pool *p, const size_t idx,
>  		    struct iov_tail *data,
>  		    const char *func, int line);
> @@ -47,11 +41,6 @@ void pool_flush(struct pool *p);
>  
>  #define packet_add(p, data)					\
>  	packet_add_do(p, data, __func__, __LINE__)
> -
> -#define packet_get_try(p, idx, offset, len, left)			\
> -	packet_get_try_do(p, idx, offset, len, left, __func__, __LINE__)
> -#define packet_get(p, idx, offset, len, left)				\
> -	packet_get_do(p, idx, offset, len, left, __func__, __LINE__)
>  #define packet_data(p, idx, data)					\
>  	packet_data_do(p, idx, data, __func__, __LINE__)
>  
> diff --git a/tap.c b/tap.c
> index 4fbcad3b385f..983f39ee8ee8 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -896,21 +896,28 @@ resume:
>  	for (seq_count = 0, seq = NULL; i < in->count; i++) {
>  		size_t l4len, plen, check;
>  		struct in6_addr *saddr, *daddr;
> +		struct ipv6hdr ip6h_storage;
> +		struct ethhdr eh_storage;
> +		struct udphdr uh_storage;
>  		const struct ethhdr *eh;
>  		const struct udphdr *uh;
>  		struct iov_tail data;
>  		struct ipv6hdr *ip6h;
>  		uint8_t proto;
> -		char *l4h;
>  
> -		eh =   packet_get(in, i, 0,		sizeof(*eh), NULL);
> +		if (!packet_data(in, i, &data))
> +			return -1;
> +
> +		eh = IOV_REMOVE_HEADER(&data, eh_storage);
>  		if (!eh)
>  			continue;
>  
> -		ip6h = packet_get(in, i, sizeof(*eh),	sizeof(*ip6h), &check);
> +		ip6h = IOV_PEEK_HEADER(&data, ip6h_storage);
>  		if (!ip6h)
>  			continue;
>  
> +		check = iov_tail_size(&data) - sizeof(*ip6h);
> +
>  		saddr = &ip6h->saddr;
>  		daddr = &ip6h->daddr;
>  
> @@ -918,10 +925,8 @@ resume:
>  		if (plen != check)
>  			continue;
>  
> -		data = IOV_TAIL_FROM_BUF(ip6h, sizeof(*ip6h) + check, 0);
>  		if (!ipv6_l4hdr(&data, &proto, &l4len))
>  			continue;
> -		l4h = (char *)data.iov[0].iov_base + data.off;
>  
>  		if (IN6_IS_ADDR_LOOPBACK(saddr) || IN6_IS_ADDR_LOOPBACK(daddr)) {
>  			char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
> @@ -946,6 +951,8 @@ resume:
>  		}
>  
>  		if (proto == IPPROTO_ICMPV6) {
> +			struct icmp6hdr l4h_storage;
> +			const struct icmp6hdr *l4h;
>  			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			if (c->no_icmp)
> @@ -954,9 +961,9 @@ resume:
>  			if (l4len < sizeof(struct icmp6hdr))
>  				continue;
>  
> -			data = IOV_TAIL_FROM_BUF(l4h, l4len, 0);
>  			packet_add(pkt, &data);
>  
> +			l4h = IOV_PEEK_HEADER(&data, l4h_storage);
>  			if (ndp(c, (struct icmp6hdr *)l4h, saddr, pkt))
>  				continue;
>  
> @@ -969,12 +976,13 @@ resume:
>  
>  		if (l4len < sizeof(*uh))
>  			continue;
> -		uh = (struct udphdr *)l4h;
> +		uh = IOV_PEEK_HEADER(&data, uh_storage);
> +		if (!uh)
> +			continue;
>  
>  		if (proto == IPPROTO_UDP) {
>  			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
> -			data = IOV_TAIL_FROM_BUF(l4h, l4len, 0);
>  			packet_add(pkt, &data);
>  
>  			if (dhcpv6(c, pkt, saddr, daddr))
> @@ -1031,7 +1039,6 @@ resume:
>  #undef L4_SET
>  
>  append:
> -		data = IOV_TAIL_FROM_BUF(l4h, l4len, 0);
>  		packet_add((struct pool *)&seq->p, &data);
>  	}
>  

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