Re: [PATCH 07/10] tcp: Remove ugly address union from struct tcp_conn

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  4 Nov 2022 19:43:30 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> We now only extract the v4 part of the tcp_conn::a union in one place.
> Make this neater by using a helper to extract the IPv4 address from the
> mapped address in that place.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c  | 40 +++++++++++++++-------------------------
>  util.c | 13 +++++++++++++
>  util.h |  1 +
>  3 files changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 3816a1c..6634abb 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -416,10 +416,7 @@ struct tcp6_l2_head {	/* For MSS6 macro: keep in sync with tcp6_l2_buf_t */
>   * @ws_to_tap:		Window scaling factor advertised to tap/guest
>   * @sndbuf:		Sending buffer in kernel, rounded to 2 ^ SNDBUF_BITS
>   * @seq_dup_ack_approx:	Last duplicate ACK number sent to tap
> - * @a.a6:		IPv6 remote address, can be IPv4-mapped
> - * @a.a4.zero:		Zero prefix for IPv4-mapped, see RFC 6890, Table 20
> - * @a.a4.one:		Ones prefix for IPv4-mapped
> - * @a.a4.a:		IPv4 address
> + * @addr:		IPv6 remote address, can be IPv4-mapped
>   * @tap_port:		Guest-facing tap port
>   * @sock_port:		Remote, socket-facing port
>   * @wnd_from_tap:	Last window size from tap, unscaled (as received)
> @@ -489,15 +486,8 @@ struct tcp_conn {
>  	uint8_t		seq_dup_ack_approx;
>  
>  
> -	union {
> -		struct in6_addr a6;
> -		struct {
> -			uint8_t zero[10];
> -			uint8_t one[2];
> -			struct in_addr a;
> -		} a4;
> -	} a;
> -#define CONN_V4(conn)		IN6_IS_ADDR_V4MAPPED(&conn->a.a6)
> +	struct in6_addr addr;
> +#define CONN_V4(conn)		IN6_IS_ADDR_V4MAPPED(&conn->addr)
>  #define CONN_V6(conn)		(!CONN_V4(conn))
>  
>  	in_port_t	tap_port;
> @@ -960,7 +950,7 @@ static int tcp_rtt_dst_low(const struct tcp_conn *conn)
>  	int i;
>  
>  	for (i = 0; i < LOW_RTT_TABLE_SIZE; i++)
> -		if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i))
> +		if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i))
>  			return 1;
>  
>  	return 0;
> @@ -982,7 +972,7 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn,
>  		return;
>  
>  	for (i = 0; i < LOW_RTT_TABLE_SIZE; i++) {
> -		if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, low_rtt_dst + i))
> +		if (IN6_ARE_ADDR_EQUAL(&conn->addr, low_rtt_dst + i))
>  			return;
>  		if (hole == -1 && IN6_IS_ADDR_UNSPECIFIED(low_rtt_dst + i))
>  			hole = i;
> @@ -994,10 +984,10 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn,
>  	if (hole == -1)
>  		return;
>  
> -	memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6));
> +	memcpy(low_rtt_dst + hole++, &conn->addr, sizeof(conn->addr));
>  	if (hole == LOW_RTT_TABLE_SIZE)
>  		hole = 0;
> -	memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->a.a6));
> +	memcpy(low_rtt_dst + hole, &in6addr_any, sizeof(conn->addr));
>  #else
>  	(void)conn;
>  	(void)tinfo;
> @@ -1285,7 +1275,7 @@ static int tcp_hash_match(const struct tcp_conn *conn,
>  			  const struct in6_addr *addr,
>  			  in_port_t tap_port, in_port_t sock_port)
>  {
> -	if (IN6_ARE_ADDR_EQUAL(&conn->a.a6, addr)		&&
> +	if (IN6_ARE_ADDR_EQUAL(&conn->addr, addr)		&&
>  	    conn->tap_port == tap_port && conn->sock_port == sock_port)
>  		return 1;
>  
> @@ -1330,7 +1320,7 @@ static void tcp_hash_insert(const struct ctx *c, struct tcp_conn *conn)
>  {
>  	int b;
>  
> -	b = tcp_hash(c, &conn->a.a6, conn->tap_port, conn->sock_port);
> +	b = tcp_hash(c, &conn->addr, conn->tap_port, conn->sock_port);
>  	conn->next_index = tc_hash[b] ? tc_hash[b] - tc : -1;
>  	tc_hash[b] = conn;
>  	conn->hash_bucket = b;
> @@ -1660,7 +1650,7 @@ do {									\
>  		ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
>  
>  		b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
> -		b->ip6h.saddr = conn->a.a6;
> +		b->ip6h.saddr = conn->addr;
>  		if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
>  			b->ip6h.daddr = c->ip6.addr_ll_seen;
>  		else
> @@ -1684,7 +1674,7 @@ do {									\
>  
>  		ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
>  		b->iph.tot_len = htons(ip_len);
> -		b->iph.saddr = conn->a.a4.a.s_addr;
> +		b->iph.saddr = decode_ip4mapped_ip6(&conn->addr).s_addr;
>  		b->iph.daddr = c->ip4.addr_seen.s_addr;
>  
>  		if (check)
> @@ -2202,12 +2192,12 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr,
>  		sa = (struct sockaddr *)&addr4;
>  		sl = sizeof(addr4);
>  
> -		encode_ip4mapped_ip6(&conn->a.a6, addr);
> +		encode_ip4mapped_ip6(&conn->addr, addr);
>  	} else {
>  		sa = (struct sockaddr *)&addr6;
>  		sl = sizeof(addr6);
>  
> -		memcpy(&conn->a.a6,      addr, sizeof(conn->a.a6));
> +		memcpy(&conn->addr,      addr, sizeof(conn->addr));
>  	}
>  
>  	conn->sock_port = ntohs(th->dest);
> @@ -2869,7 +2859,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  			memcpy(&sa6.sin6_addr, src, sizeof(*src));
>  		}
>  
> -		memcpy(&conn->a.a6, &sa6.sin6_addr, sizeof(conn->a.a6));
> +		memcpy(&conn->addr, &sa6.sin6_addr, sizeof(conn->addr));
>  
>  		conn->sock_port = ntohs(sa6.sin6_port);
>  		conn->tap_port = ref.r.p.tcp.tcp.index;
> @@ -2888,7 +2878,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
>  		    IN4_ARE_ADDR_EQUAL(&sa4.sin_addr, &c->ip4.addr_seen))
>  			sa4.sin_addr = c->ip4.gw;
>  
> -		encode_ip4mapped_ip6(&conn->a.a6, &sa4.sin_addr);
> +		encode_ip4mapped_ip6(&conn->addr, &sa4.sin_addr);
>  
>  		conn->sock_port = ntohs(sa4.sin_port);
>  		conn->tap_port = ref.r.p.tcp.tcp.index;
> diff --git a/util.c b/util.c
> index 257d0b6..ec7f57f 100644
> --- a/util.c
> +++ b/util.c
> @@ -502,3 +502,16 @@ void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4)
>  	memset(&a->one, 0xff, sizeof(a->one));
>  	memcpy(&a->a4, ip4, sizeof(a->a4));
>  }
> +
> +/**
> + * decode_ip4mapped_ip6() - Decode an IPv4-mapped IPv6 address
> + * @ip6:	IPv6 address
> + *
> + * Returns:	IPv4 address

Return: IPv4 address

> + */
> +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6)
> +{
> +	const struct ip4mapped_ip6 *a = (const struct ip4mapped_ip6 *)ip6;

...if you use struct ip4_mapped_ip6 directly in struct conn, you could
drop this cast, and perhaps this helper too, but I'm not sure if it has
other implications.

> +
> +	return a->a4;
> +}
> diff --git a/util.h b/util.h
> index f7d6a6f..103211d 100644
> --- a/util.h
> +++ b/util.h
> @@ -210,5 +210,6 @@ int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);
>  int write_file(const char *path, const char *buf);
>  void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4);
> +struct in_addr decode_ip4mapped_ip6(const struct in6_addr *ip6);
>  
>  #endif /* UTIL_H */

-- 
Stefano