Re: [PATCH 09/14] Split tap_ip_send() into IPv4 and IPv6 specific functions

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 17 Oct 2022 19:58:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The IPv4 and IPv6 paths in tap_ip_send() have very little in common, and
> it turns out that every caller (statically) knows if it is using IPv4 or
> IPv6.  So split into separate tap_ip4_send() and tap_ip6_send() functions.
> Use a new tap_l2_hdr() function for the very small common part.
> 
> While we're there, make some minor cleanups:
>   - We were double writing some fields in the IPv6 header, so that it
>     temporary matched the pseudo-header for checksum calculation.  With
>     recent checksum reworks, this isn't neccessary any more.
>   - We don't use any IPv4 header options, so use some sizeof() constructs
>     instead of some open coded values for header length.
>   - The comment used to say that the flow label was for TCP over IPv6, but
>     in fact the only thing we used it for was ICMPv6

...right, this used to be the data path.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  dhcpv6.c |   6 +-
>  icmp.c   |  10 +---
>  tap.c    | 176 +++++++++++++++++++++++++++++--------------------------
>  tap.h    |   6 +-
>  4 files changed, 102 insertions(+), 96 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index e7640ce..7829968 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -531,8 +531,8 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  
>  			resp_not_on_link.hdr.xid = mh->xid;
>  
> -			tap_ip_send(c, src, IPPROTO_UDP,
> -				    (char *)&resp_not_on_link, n, mh->xid);
> +			tap_ip6_send(c, src, IPPROTO_UDP,
> +				     (char *)&resp_not_on_link, n, mh->xid);
>  
>  			return 1;
>  		}
> @@ -580,7 +580,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  
>  	resp.hdr.xid = mh->xid;
>  
> -	tap_ip_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid);
> +	tap_ip6_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid);
>  	c->ip6.addr_seen = c->ip6.addr;
>  
>  	return 1;
> diff --git a/icmp.c b/icmp.c
> index 21ea2d7..61c2d90 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -69,10 +69,6 @@ static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
>  void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  		       uint32_t events, const struct timespec *now)
>  {
> -	struct in6_addr a6 = { .s6_addr = {    0,    0,    0,    0,
> -					       0,    0,    0,    0,
> -					       0,    0, 0xff, 0xff,
> -					       0,    0,    0,    0 } };
>  	union icmp_epoll_ref *iref = &ref.r.p.icmp;
>  	struct sockaddr_storage sr;
>  	socklen_t sl = sizeof(sr);
> @@ -109,7 +105,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  			icmp_id_map[V6][id].seq = seq;
>  		}
>  
> -		tap_ip_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0);
> +		tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0);
>  	} else {
>  		struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr;
>  		struct icmphdr *ih = (struct icmphdr *)buf;
> @@ -127,9 +123,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  			icmp_id_map[V4][id].seq = seq;
>  		}
>  
> -		memcpy(&a6.s6_addr[12], &sr4->sin_addr, sizeof(sr4->sin_addr));
> -
> -		tap_ip_send(c, &a6, IPPROTO_ICMP, buf, n, 0);
> +		tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n);
>  	}
>  }
>  
> diff --git a/tap.c b/tap.c
> index ae75fac..45547ac 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -109,100 +109,110 @@ const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
>  }
>  
>  /**
> - * tap_ip_send() - Send IP packet, with L2 headers, calculating L3/L4 checksums
> + * tap_l2_hdr() - Build an L2 header for an inbound packet
>   * @c:		Execution context
> - * @src:	IPv6 source address, IPv4-mapped for IPv4 sources
> - * @proto:	L4 protocol number
> - * @in:		Payload
> - * @len:	L4 payload length
> - * @flow:	Flow label for TCP over IPv6
> + * @buf:	Buffer address at which to generate header
> + * @proto:	Ethernet protocol number for L3
> + *
> + * Returns a pointer at which to write the packet's payload

 * Return: ...

-- 
Stefano