On Mon, Nov 07, 2022 at 07:08:19PM +0100, Stefano Brivio wrote:
> On Fri,  4 Nov 2022 19:43:25 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In the tcp_conn structure we have space for an IPv6 address, and use
> > IPv4-mapped IPv6 addresses to describe IPv4 connections.  We open code
> > the construction of those IPv4-mapped address in two places.
> > 
> > Avoid the duplication with a helper function.
> 
> Much nicer, indeed.
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c  |  9 ++-------
> >  util.c | 20 ++++++++++++++++++++
> >  util.h |  1 +
> >  3 files changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 3d48d6e..26dd268 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2217,9 +2217,7 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr,
> >  		sa = (struct sockaddr *)&addr4;
> >  		sl = sizeof(addr4);
> >  
> > -		memset(&conn->a.a4.zero, 0,    sizeof(conn->a.a4.zero));
> > -		memset(&conn->a.a4.one,  0xff, sizeof(conn->a.a4.one));
> > -		memcpy(&conn->a.a4.a,    addr, sizeof(conn->a.a4.a));
> > +		encode_ip4mapped_ip6(&conn->a.a6, addr);
> >  	} else {
> >  		sa = (struct sockaddr *)&addr6;
> >  		sl = sizeof(addr6);
> > @@ -2902,15 +2900,12 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
> >  
> >  		memcpy(&sa4, &sa, sizeof(sa4));
> >  
> > -		memset(&conn->a.a4.zero,   0, sizeof(conn->a.a4.zero));
> > -		memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one));
> > -
> >  		if (IN4_IS_ADDR_LOOPBACK(&sa4.sin_addr) ||
> >  		    IN4_IS_ADDR_UNSPECIFIED(&sa4.sin_addr) ||
> >  		    IN4_ARE_ADDR_EQUAL(&sa4.sin_addr, &c->ip4.addr_seen))
> >  			sa4.sin_addr = c->ip4.gw;
> >  
> > -		conn->a.a4.a = sa4.sin_addr;
> > +		encode_ip4mapped_ip6(&conn->a.a6, &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 514bd44..257d0b6 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -482,3 +482,23 @@ int write_file(const char *path, const char *buf)
> >  	close(fd);
> >  	return len == 0 ? 0 : -1;
> >  }
> > +
> > +struct ip4mapped_ip6 {
> > +	uint8_t zero[10];
> > +	uint8_t one[2];
> > +	struct in_addr a4;
> > +};
> 
> Document fields even if they're obvious. Can we reuse this part in
> struct conn instead of using struct in6_addr there as you do later in
> 7/10? I don't have a strong preference though.

Yeah, we could.  Originally I was trying to make this even more
hidden, with the decode function returning a struct in_addr *, that
would be NULL if the IPv6 address wasn't v4-mapped.  Unfortunately I
seemed to hit more weird compiler aliasing issues that caused hard to
predict failures.

Hrm... your comment gives me some thoughts on maybe a better way to do
this, I'll look into it.

> 
> > +
> > +/**
> > + * encode_ip4mapped_ip6() - Convert an IPv4 address to an IPv4-mapped IPv6 address
> > + * @ip6:	Buffer to store the IPv4-mapped IPv6 address
> > + * @ip4:	IPv4 address, network order
> > + */
> > +void encode_ip4mapped_ip6(struct in6_addr *ip6, const void *ip4)
> > +{
> > +	struct ip4mapped_ip6 *a = (struct ip4mapped_ip6 *)ip6;
> > +
> > +	memset(&a->zero, 0, sizeof(a->zero));
> > +	memset(&a->one, 0xff, sizeof(a->one));
> > +	memcpy(&a->a4, ip4, sizeof(a->a4));
> > +}
> > diff --git a/util.h b/util.h
> > index 2d4e1ff..f7d6a6f 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -209,5 +209,6 @@ void write_pidfile(int fd, pid_t pid);
> >  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);
> >  
> >  #endif /* UTIL_H */
> 

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