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

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

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

On Tue, Oct 18, 2022 at 05:06:11AM +0200, Stefano Brivio wrote:
> 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.

Makes sense.  Realized that comment isn't quite correct, because it
was DHCPv6 rather than ICMPv6 traffic that got the flow labels.

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

Done.

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