Re: [PATCH v5 18/29] ip: Use iov_tail in ipv6_l4hdr()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Apr 2025 18:51:25 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
> and IOV_PEEK_HEADER() rather than packet_get().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  ip.c     | 27 ++++++++++++---------------
>  ip.h     |  3 +--
>  packet.c |  1 +
>  tap.c    |  4 +++-
>  4 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/ip.c b/ip.c
> index 2cc7f6548aff..b63fbe79fb5f 100644
> --- a/ip.c
> +++ b/ip.c
> @@ -23,40 +23,37 @@
>  
>  /**
>   * ipv6_l4hdr() - Find pointer to L4 header in IPv6 packet and extract protocol
> - * @p:		Packet pool, packet number @idx has IPv6 header at @offset
> - * @idx:	Index of packet in pool
> - * @offset:	Pre-calculated IPv6 header offset
> + * @data:	IPv6 packet
>   * @proto:	Filled with L4 protocol number
>   * @dlen:	Data length (payload excluding header extensions), set on return
>   *
>   * Return: pointer to L4 header, NULL if not found
>   */
> -char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
> -		 size_t *dlen)
> +bool ipv6_l4hdr(struct iov_tail *data, uint8_t *proto, size_t *dlen)

Function comment should be updated to reflect this.

>  {
>  	const struct ipv6_opt_hdr *o;
> +	struct ipv6_opt_hdr oc;
>  	const struct ipv6hdr *ip6h;
> -	char *base;
> +	struct ipv6hdr ip6hc;

Nit: partially preexisting, but these should be from longest to
shortest. Usual comment about the 'c' in 'oc'.

>  	int hdrlen;
>  	uint8_t nh;
>  
> -	base = packet_get(p, idx, 0, 0, NULL);
> -	ip6h = packet_get(p, idx, offset, sizeof(*ip6h), dlen);
> +	ip6h = IOV_REMOVE_HEADER(data, ip6hc);
>  	if (!ip6h)
> -		return NULL;
> -
> -	offset += sizeof(*ip6h);
> +		return false;
> +	*dlen = iov_tail_size(data);
>  
>  	nh = ip6h->nexthdr;
>  	if (!IPV6_NH_OPT(nh))
>  		goto found;
>  
> -	while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
> +	while ((o = IOV_PEEK_HEADER(data, oc))) {
> +		*dlen = iov_tail_size(data) - sizeof(*o);
>  		nh = o->nexthdr;
>  		hdrlen = (o->hdrlen + 1) * 8;
>  
>  		if (IPV6_NH_OPT(nh))
> -			offset += hdrlen;
> +			iov_tail_drop(data, hdrlen);
>  		else
>  			goto found;
>  	}
> @@ -65,8 +62,8 @@ char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
>  
>  found:
>  	if (nh == 59)
> -		return NULL;
> +		return false;
>  
>  	*proto = nh;
> -	return base + offset;
> +	return true;
>  }
> diff --git a/ip.h b/ip.h
> index 471c57ee4c71..874cbe476a6b 100644
> --- a/ip.h
> +++ b/ip.h
> @@ -115,8 +115,7 @@ static inline uint32_t ip6_get_flow_lbl(const struct ipv6hdr *ip6h)
>  		ip6h->flow_lbl[2];
>  }
>  
> -char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
> -		 size_t *dlen);
> +bool ipv6_l4hdr(struct iov_tail *data, uint8_t *proto, size_t *dlen);
>  
>  /* IPv6 link-local all-nodes multicast adddress, ff02::1 */
>  static const struct in6_addr in6addr_ll_all_nodes = {
> diff --git a/packet.c b/packet.c
> index b6de869db176..7489010e02eb 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -133,6 +133,7 @@ void packet_add_do(struct pool *p, struct iov_tail *data,
>   *
>   * 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)
>  {
> diff --git a/tap.c b/tap.c
> index 162f19d68115..ca25ff9e90d9 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -909,8 +909,10 @@ resume:
>  		if (plen != check)
>  			continue;
>  
> -		if (!(l4h = ipv6_l4hdr(in, i, sizeof(*eh), &proto, &l4len)))
> +		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];

-- 
Stefano