public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH v2 0/6] udp: Small cleanups
@ 2024-03-06  5:34 David Gibson
  2024-03-06  5:34 ` [PATCH v2 1/6] udp: Refactor udp_sock[46]_iov_init() David Gibson
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

Following on from the recent set of small fixes for UDP, here are a
number of small cleanups to the UDP code, which will simplify later
more complex fixes and improvements.

Laurent, I expect this will have some conflicts with part 2 of your
vhost-user work, though I hope they won't be too bad.

This is now based on part 1 of Laurent's vhost-user series
(specifically the version I posted with the static checker regressions
fixed).

Changes since v1:
 * Rebased on Laurent's vhost-user part 1 patches
 * Some improved commit messages

David Gibson (6):
  udp: Refactor udp_sock[46]_iov_init()
  udp: Consistent port variable names in udp_update_hdr[46]
  udp: Pass data length explicitly to to udp_update_hdr[46]
  udp: Re-order udp_update_hdr[46] for clarity and brevity
  udp: Avoid unnecessary pointer in udp_update_hdr4()
  udp: Use existing helper for UDP checksum on inbound IPv6 packets

 udp.c | 211 +++++++++++++++++++++++++++-------------------------------
 1 file changed, 99 insertions(+), 112 deletions(-)

-- 
2.44.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/6] udp: Refactor udp_sock[46]_iov_init()
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-06  5:34 ` [PATCH v2 2/6] udp: Consistent port variable names in udp_update_hdr[46] David Gibson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

Each of these functions have 3 essentially identical loops in a row.
Merge the loops into a single common udp_sock_iov_init() function, calling
udp_sock[46]_iov_init_one() helpers to initialize each "slot" in the
various parallel arrays.  This is slightly neater now, and more naturally
allows changes we want to make where more initialization will become common
between IPv4 and IPv6.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 102 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 50 insertions(+), 52 deletions(-)

diff --git a/udp.c b/udp.c
index 1f46afb2..df5f3c48 100644
--- a/udp.c
+++ b/udp.c
@@ -294,72 +294,74 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
 }
 
 /**
- * udp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
+ * udp_sock4_iov_init_one() - Initialise a scatter-gather L2 buffer for IPv4
  * @c:		Execution context
+ * @i:		Index of buffer to initialize
  */
-static void udp_sock4_iov_init(const struct ctx *c)
+static void udp_sock4_iov_init_one(const struct ctx *c, size_t i)
 {
-	struct mmsghdr *h;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(udp4_l2_buf); i++) {
-		udp4_l2_buf[i] = (struct udp4_l2_buf_t) {
-			.taph = TAP_HDR_INIT(ETH_P_IP),
-			.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
-		};
-	}
-
-	for (i = 0, h = udp4_l2_mh_sock; i < UDP_MAX_FRAMES; i++, h++) {
-		struct msghdr *mh = &h->msg_hdr;
+	struct msghdr *mh = &udp4_l2_mh_sock[i].msg_hdr;
+	struct udp4_l2_buf_t *buf = &udp4_l2_buf[i];
+	struct iovec *siov = &udp4_l2_iov_sock[i];
+	struct iovec *tiov = &udp4_l2_iov_tap[i];
 
-		mh->msg_name			= &udp4_l2_buf[i].s_in;
-		mh->msg_namelen			= sizeof(udp4_l2_buf[i].s_in);
+	*buf = (struct udp4_l2_buf_t) {
+		.taph = TAP_HDR_INIT(ETH_P_IP),
+		.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
+	};
 
-		udp4_l2_iov_sock[i].iov_base	= udp4_l2_buf[i].data;
-		udp4_l2_iov_sock[i].iov_len	= sizeof(udp4_l2_buf[i].data);
-		mh->msg_iov			= &udp4_l2_iov_sock[i];
-		mh->msg_iovlen			= 1;
-	}
+	siov->iov_base	= buf->data;
+	siov->iov_len	= sizeof(buf->data);
 
-	for (i = 0; i < UDP_MAX_FRAMES; i++) {
-		struct iovec *iov = &udp4_l2_iov_tap[i];
+	mh->msg_name	= &buf->s_in;
+	mh->msg_namelen	= sizeof(buf->s_in);
+	mh->msg_iov	= siov;
+	mh->msg_iovlen	= 1;
 
-		iov->iov_base = tap_iov_base(c, &udp4_l2_buf[i].taph);
-	}
+	tiov->iov_base	= tap_iov_base(c, &buf->taph);
 }
 
 /**
- * udp_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
+ * udp_sock6_iov_init_one() - Initialise a scatter-gather L2 buffer for IPv6
  * @c:		Execution context
+ * @i:		Index of buffer to initialize
  */
-static void udp_sock6_iov_init(const struct ctx *c)
+static void udp_sock6_iov_init_one(const struct ctx *c, size_t i)
 {
-	struct mmsghdr *h;
-	int i;
+	struct msghdr *mh = &udp6_l2_mh_sock[i].msg_hdr;
+	struct udp6_l2_buf_t *buf = &udp6_l2_buf[i];
+	struct iovec *siov = &udp6_l2_iov_sock[i];
+	struct iovec *tiov = &udp6_l2_iov_tap[i];
 
-	for (i = 0; i < ARRAY_SIZE(udp6_l2_buf); i++) {
-		udp6_l2_buf[i] = (struct udp6_l2_buf_t) {
-			.taph = TAP_HDR_INIT(ETH_P_IPV6),
-			.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
-		};
-	}
+	*buf = (struct udp6_l2_buf_t) {
+		.taph = TAP_HDR_INIT(ETH_P_IPV6),
+		.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
+	};
 
-	for (i = 0, h = udp6_l2_mh_sock; i < UDP_MAX_FRAMES; i++, h++) {
-		struct msghdr *mh = &h->msg_hdr;
+	siov->iov_base	= buf->data;
+	siov->iov_len	= sizeof(buf->data);
 
-		mh->msg_name			= &udp6_l2_buf[i].s_in6;
-		mh->msg_namelen			= sizeof(struct sockaddr_in6);
+	mh->msg_name	= &buf->s_in6;
+	mh->msg_namelen	= sizeof(buf->s_in6);
+	mh->msg_iov	= siov;
+	mh->msg_iovlen	= 1;
 
-		udp6_l2_iov_sock[i].iov_base	= udp6_l2_buf[i].data;
-		udp6_l2_iov_sock[i].iov_len	= sizeof(udp6_l2_buf[i].data);
-		mh->msg_iov			= &udp6_l2_iov_sock[i];
-		mh->msg_iovlen			= 1;
-	}
+	tiov->iov_base	= tap_iov_base(c, &buf->taph);
+}
 
-	for (i = 0; i < UDP_MAX_FRAMES; i++) {
-		struct iovec *iov = &udp6_l2_iov_tap[i];
+/**
+ * udp_sock_iov_init() - Initialise scatter-gather L2 buffers
+ * @c:		Execution context
+ */
+static void udp_sock_iov_init(const struct ctx *c)
+{
+	size_t i;
 
-		iov->iov_base = tap_iov_base(c, &udp6_l2_buf[i].taph);
+	for (i = 0; i < UDP_MAX_FRAMES; i++) {
+		if (c->ifi4)
+			udp_sock4_iov_init_one(c, i);
+		if (c->ifi6)
+			udp_sock6_iov_init_one(c, i);
 	}
 }
 
@@ -1230,11 +1232,7 @@ v6:
  */
 int udp_init(struct ctx *c)
 {
-	if (c->ifi4)
-		udp_sock4_iov_init(c);
-
-	if (c->ifi6)
-		udp_sock6_iov_init(c);
+	udp_sock_iov_init(c);
 
 	udp_invert_portmap(&c->udp.fwd_in);
 	udp_invert_portmap(&c->udp.fwd_out);
-- 
@@ -294,72 +294,74 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
 }
 
 /**
- * udp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
+ * udp_sock4_iov_init_one() - Initialise a scatter-gather L2 buffer for IPv4
  * @c:		Execution context
+ * @i:		Index of buffer to initialize
  */
-static void udp_sock4_iov_init(const struct ctx *c)
+static void udp_sock4_iov_init_one(const struct ctx *c, size_t i)
 {
-	struct mmsghdr *h;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(udp4_l2_buf); i++) {
-		udp4_l2_buf[i] = (struct udp4_l2_buf_t) {
-			.taph = TAP_HDR_INIT(ETH_P_IP),
-			.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
-		};
-	}
-
-	for (i = 0, h = udp4_l2_mh_sock; i < UDP_MAX_FRAMES; i++, h++) {
-		struct msghdr *mh = &h->msg_hdr;
+	struct msghdr *mh = &udp4_l2_mh_sock[i].msg_hdr;
+	struct udp4_l2_buf_t *buf = &udp4_l2_buf[i];
+	struct iovec *siov = &udp4_l2_iov_sock[i];
+	struct iovec *tiov = &udp4_l2_iov_tap[i];
 
-		mh->msg_name			= &udp4_l2_buf[i].s_in;
-		mh->msg_namelen			= sizeof(udp4_l2_buf[i].s_in);
+	*buf = (struct udp4_l2_buf_t) {
+		.taph = TAP_HDR_INIT(ETH_P_IP),
+		.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
+	};
 
-		udp4_l2_iov_sock[i].iov_base	= udp4_l2_buf[i].data;
-		udp4_l2_iov_sock[i].iov_len	= sizeof(udp4_l2_buf[i].data);
-		mh->msg_iov			= &udp4_l2_iov_sock[i];
-		mh->msg_iovlen			= 1;
-	}
+	siov->iov_base	= buf->data;
+	siov->iov_len	= sizeof(buf->data);
 
-	for (i = 0; i < UDP_MAX_FRAMES; i++) {
-		struct iovec *iov = &udp4_l2_iov_tap[i];
+	mh->msg_name	= &buf->s_in;
+	mh->msg_namelen	= sizeof(buf->s_in);
+	mh->msg_iov	= siov;
+	mh->msg_iovlen	= 1;
 
-		iov->iov_base = tap_iov_base(c, &udp4_l2_buf[i].taph);
-	}
+	tiov->iov_base	= tap_iov_base(c, &buf->taph);
 }
 
 /**
- * udp_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
+ * udp_sock6_iov_init_one() - Initialise a scatter-gather L2 buffer for IPv6
  * @c:		Execution context
+ * @i:		Index of buffer to initialize
  */
-static void udp_sock6_iov_init(const struct ctx *c)
+static void udp_sock6_iov_init_one(const struct ctx *c, size_t i)
 {
-	struct mmsghdr *h;
-	int i;
+	struct msghdr *mh = &udp6_l2_mh_sock[i].msg_hdr;
+	struct udp6_l2_buf_t *buf = &udp6_l2_buf[i];
+	struct iovec *siov = &udp6_l2_iov_sock[i];
+	struct iovec *tiov = &udp6_l2_iov_tap[i];
 
-	for (i = 0; i < ARRAY_SIZE(udp6_l2_buf); i++) {
-		udp6_l2_buf[i] = (struct udp6_l2_buf_t) {
-			.taph = TAP_HDR_INIT(ETH_P_IPV6),
-			.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
-		};
-	}
+	*buf = (struct udp6_l2_buf_t) {
+		.taph = TAP_HDR_INIT(ETH_P_IPV6),
+		.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
+	};
 
-	for (i = 0, h = udp6_l2_mh_sock; i < UDP_MAX_FRAMES; i++, h++) {
-		struct msghdr *mh = &h->msg_hdr;
+	siov->iov_base	= buf->data;
+	siov->iov_len	= sizeof(buf->data);
 
-		mh->msg_name			= &udp6_l2_buf[i].s_in6;
-		mh->msg_namelen			= sizeof(struct sockaddr_in6);
+	mh->msg_name	= &buf->s_in6;
+	mh->msg_namelen	= sizeof(buf->s_in6);
+	mh->msg_iov	= siov;
+	mh->msg_iovlen	= 1;
 
-		udp6_l2_iov_sock[i].iov_base	= udp6_l2_buf[i].data;
-		udp6_l2_iov_sock[i].iov_len	= sizeof(udp6_l2_buf[i].data);
-		mh->msg_iov			= &udp6_l2_iov_sock[i];
-		mh->msg_iovlen			= 1;
-	}
+	tiov->iov_base	= tap_iov_base(c, &buf->taph);
+}
 
-	for (i = 0; i < UDP_MAX_FRAMES; i++) {
-		struct iovec *iov = &udp6_l2_iov_tap[i];
+/**
+ * udp_sock_iov_init() - Initialise scatter-gather L2 buffers
+ * @c:		Execution context
+ */
+static void udp_sock_iov_init(const struct ctx *c)
+{
+	size_t i;
 
-		iov->iov_base = tap_iov_base(c, &udp6_l2_buf[i].taph);
+	for (i = 0; i < UDP_MAX_FRAMES; i++) {
+		if (c->ifi4)
+			udp_sock4_iov_init_one(c, i);
+		if (c->ifi6)
+			udp_sock6_iov_init_one(c, i);
 	}
 }
 
@@ -1230,11 +1232,7 @@ v6:
  */
 int udp_init(struct ctx *c)
 {
-	if (c->ifi4)
-		udp_sock4_iov_init(c);
-
-	if (c->ifi6)
-		udp_sock6_iov_init(c);
+	udp_sock_iov_init(c);
 
 	udp_invert_portmap(&c->udp.fwd_in);
 	udp_invert_portmap(&c->udp.fwd_out);
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/6] udp: Consistent port variable names in udp_update_hdr[46]
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
  2024-03-06  5:34 ` [PATCH v2 1/6] udp: Refactor udp_sock[46]_iov_init() David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-06  5:34 ` [PATCH v2 3/6] udp: Pass data length explicitly to to udp_update_hdr[46] David Gibson
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

In these functions we have 'dstport' for the destination port, but
'src_port' for the source port.  Change the latter to 'srcport' for
consistency.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/udp.c b/udp.c
index df5f3c48..79f0a63c 100644
--- a/udp.c
+++ b/udp.c
@@ -574,7 +574,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 {
 	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
 	const struct in_addr *src;
-	in_port_t src_port;
+	in_port_t srcport;
 	size_t ip_len;
 
 	ip_len = udp4_l2_mh_sock[n].msg_len + sizeof(b->iph) + sizeof(b->uh);
@@ -583,22 +583,22 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
 
 	src = &b->s_in.sin_addr;
-	src_port = ntohs(b->s_in.sin_port);
+	srcport = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && src_port == 53) {
+	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
 		src = &c->ip4.dns_match;
 	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
 		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
-		udp_tap_map[V4][src_port].ts = now->tv_sec;
-		udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
+		udp_tap_map[V4][srcport].ts = now->tv_sec;
+		udp_tap_map[V4][srcport].flags |= PORT_LOCAL;
 
 		if (IN4_IS_ADDR_LOOPBACK(src))
-			udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
+			udp_tap_map[V4][srcport].flags |= PORT_LOOPBACK;
 		else
-			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
+			udp_tap_map[V4][srcport].flags &= ~PORT_LOOPBACK;
 
-		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
+		bitmap_set(udp_act[V4][UDP_ACT_TAP], srcport);
 
 		src = &c->ip4.gw;
 	}
@@ -628,12 +628,12 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 	struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
 	const struct in6_addr *src, *dst;
 	uint16_t payload_len;
-	in_port_t src_port;
+	in_port_t srcport;
 	size_t ip_len;
 
 	dst = &c->ip6.addr_seen;
 	src = &b->s_in6.sin6_addr;
-	src_port = ntohs(b->s_in6.sin6_port);
+	srcport = ntohs(b->s_in6.sin6_port);
 
 	ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
 
@@ -644,25 +644,25 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 		dst = &c->ip6.addr_ll_seen;
 	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match) &&
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_host) &&
-		   src_port == 53) {
+		   srcport == 53) {
 		src = &c->ip6.dns_match;
 	} else if (IN6_IS_ADDR_LOOPBACK(src)			||
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr_seen)	||
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr)) {
-		udp_tap_map[V6][src_port].ts = now->tv_sec;
-		udp_tap_map[V6][src_port].flags |= PORT_LOCAL;
+		udp_tap_map[V6][srcport].ts = now->tv_sec;
+		udp_tap_map[V6][srcport].flags |= PORT_LOCAL;
 
 		if (IN6_IS_ADDR_LOOPBACK(src))
-			udp_tap_map[V6][src_port].flags |= PORT_LOOPBACK;
+			udp_tap_map[V6][srcport].flags |= PORT_LOOPBACK;
 		else
-			udp_tap_map[V6][src_port].flags &= ~PORT_LOOPBACK;
+			udp_tap_map[V6][srcport].flags &= ~PORT_LOOPBACK;
 
 		if (IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr))
-			udp_tap_map[V6][src_port].flags |= PORT_GUA;
+			udp_tap_map[V6][srcport].flags |= PORT_GUA;
 		else
-			udp_tap_map[V6][src_port].flags &= ~PORT_GUA;
+			udp_tap_map[V6][srcport].flags &= ~PORT_GUA;
 
-		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
+		bitmap_set(udp_act[V6][UDP_ACT_TAP], srcport);
 
 		dst = &c->ip6.addr_ll_seen;
 
-- 
@@ -574,7 +574,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 {
 	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
 	const struct in_addr *src;
-	in_port_t src_port;
+	in_port_t srcport;
 	size_t ip_len;
 
 	ip_len = udp4_l2_mh_sock[n].msg_len + sizeof(b->iph) + sizeof(b->uh);
@@ -583,22 +583,22 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
 
 	src = &b->s_in.sin_addr;
-	src_port = ntohs(b->s_in.sin_port);
+	srcport = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && src_port == 53) {
+	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
 		src = &c->ip4.dns_match;
 	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
 		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
-		udp_tap_map[V4][src_port].ts = now->tv_sec;
-		udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
+		udp_tap_map[V4][srcport].ts = now->tv_sec;
+		udp_tap_map[V4][srcport].flags |= PORT_LOCAL;
 
 		if (IN4_IS_ADDR_LOOPBACK(src))
-			udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
+			udp_tap_map[V4][srcport].flags |= PORT_LOOPBACK;
 		else
-			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
+			udp_tap_map[V4][srcport].flags &= ~PORT_LOOPBACK;
 
-		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
+		bitmap_set(udp_act[V4][UDP_ACT_TAP], srcport);
 
 		src = &c->ip4.gw;
 	}
@@ -628,12 +628,12 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 	struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
 	const struct in6_addr *src, *dst;
 	uint16_t payload_len;
-	in_port_t src_port;
+	in_port_t srcport;
 	size_t ip_len;
 
 	dst = &c->ip6.addr_seen;
 	src = &b->s_in6.sin6_addr;
-	src_port = ntohs(b->s_in6.sin6_port);
+	srcport = ntohs(b->s_in6.sin6_port);
 
 	ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
 
@@ -644,25 +644,25 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 		dst = &c->ip6.addr_ll_seen;
 	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match) &&
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_host) &&
-		   src_port == 53) {
+		   srcport == 53) {
 		src = &c->ip6.dns_match;
 	} else if (IN6_IS_ADDR_LOOPBACK(src)			||
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr_seen)	||
 		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr)) {
-		udp_tap_map[V6][src_port].ts = now->tv_sec;
-		udp_tap_map[V6][src_port].flags |= PORT_LOCAL;
+		udp_tap_map[V6][srcport].ts = now->tv_sec;
+		udp_tap_map[V6][srcport].flags |= PORT_LOCAL;
 
 		if (IN6_IS_ADDR_LOOPBACK(src))
-			udp_tap_map[V6][src_port].flags |= PORT_LOOPBACK;
+			udp_tap_map[V6][srcport].flags |= PORT_LOOPBACK;
 		else
-			udp_tap_map[V6][src_port].flags &= ~PORT_LOOPBACK;
+			udp_tap_map[V6][srcport].flags &= ~PORT_LOOPBACK;
 
 		if (IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr))
-			udp_tap_map[V6][src_port].flags |= PORT_GUA;
+			udp_tap_map[V6][srcport].flags |= PORT_GUA;
 		else
-			udp_tap_map[V6][src_port].flags &= ~PORT_GUA;
+			udp_tap_map[V6][srcport].flags &= ~PORT_GUA;
 
-		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
+		bitmap_set(udp_act[V6][UDP_ACT_TAP], srcport);
 
 		dst = &c->ip6.addr_ll_seen;
 
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/6] udp: Pass data length explicitly to to udp_update_hdr[46]
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
  2024-03-06  5:34 ` [PATCH v2 1/6] udp: Refactor udp_sock[46]_iov_init() David Gibson
  2024-03-06  5:34 ` [PATCH v2 2/6] udp: Consistent port variable names in udp_update_hdr[46] David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-06  5:34 ` [PATCH v2 4/6] udp: Re-order udp_update_hdr[46] for clarity and brevity David Gibson
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

These functions take an index to the L2 buffer whose header information to
update.  They use that for two things: to locate the buffer pointer itself,
and to retrieve the length of the received message from the paralllel
udp[46]_l2_mh_sock array.  The latter is arguably a failure to separate
concerns.  Change these functions to explicitly take a buffer pointer and
payload length as parameters.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/udp.c b/udp.c
index 79f0a63c..e20f537a 100644
--- a/udp.c
+++ b/udp.c
@@ -563,21 +563,22 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
 /**
  * udp_update_hdr4() - Update headers for one IPv4 datagram
  * @c:		Execution context
- * @n:		Index of buffer in udp4_l2_buf pool
+ * @b:		Pointer to udp4_l2_buf to update
  * @dstport:	Destination port number
+ * @datalen:	Length of UDP payload
  * @now:	Current timestamp
  *
  * Return: size of tap frame with headers
  */
-static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
+static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
+			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
 	const struct in_addr *src;
 	in_port_t srcport;
 	size_t ip_len;
 
-	ip_len = udp4_l2_mh_sock[n].msg_len + sizeof(b->iph) + sizeof(b->uh);
+	ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
 
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
@@ -608,7 +609,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 				       *src, c->ip4.addr_seen);
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
-	b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
+	b->uh.len = htons(datalen + sizeof(b->uh));
 
 	return tap_iov_len(c, &b->taph, ip_len);
 }
@@ -616,16 +617,17 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 /**
  * udp_update_hdr6() - Update headers for one IPv6 datagram
  * @c:		Execution context
- * @n:		Index of buffer in udp6_l2_buf pool
+ * @b:		Pointer to udp6_l2_buf to update
  * @dstport:	Destination port number
+ * @datalen:	Length of UDP payload
  * @now:	Current timestamp
  *
  * Return: size of tap frame with headers
  */
-static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
+static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
+			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
 	const struct in6_addr *src, *dst;
 	uint16_t payload_len;
 	in_port_t srcport;
@@ -635,9 +637,9 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 	src = &b->s_in6.sin6_addr;
 	srcport = ntohs(b->s_in6.sin6_port);
 
-	ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
+	ip_len = datalen + sizeof(b->ip6h) + sizeof(b->uh);
 
-	payload_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->uh);
+	payload_len = datalen + sizeof(b->uh);
 	b->ip6h.payload_len = htons(payload_len);
 
 	if (IN6_IS_ADDR_LINKLOCAL(src)) {
@@ -716,9 +718,11 @@ static void udp_tap_send(const struct ctx *c,
 		size_t buf_len;
 
 		if (v6)
-			buf_len = udp_update_hdr6(c, i, dstport, now);
+			buf_len = udp_update_hdr6(c, &udp6_l2_buf[i], dstport,
+						  udp6_l2_mh_sock[i].msg_len, now);
 		else
-			buf_len = udp_update_hdr4(c, i, dstport, now);
+			buf_len = udp_update_hdr4(c, &udp4_l2_buf[i], dstport,
+						  udp4_l2_mh_sock[i].msg_len, now);
 
 		tap_iov[i].iov_len = buf_len;
 	}
-- 
@@ -563,21 +563,22 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
 /**
  * udp_update_hdr4() - Update headers for one IPv4 datagram
  * @c:		Execution context
- * @n:		Index of buffer in udp4_l2_buf pool
+ * @b:		Pointer to udp4_l2_buf to update
  * @dstport:	Destination port number
+ * @datalen:	Length of UDP payload
  * @now:	Current timestamp
  *
  * Return: size of tap frame with headers
  */
-static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
+static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
+			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
 	const struct in_addr *src;
 	in_port_t srcport;
 	size_t ip_len;
 
-	ip_len = udp4_l2_mh_sock[n].msg_len + sizeof(b->iph) + sizeof(b->uh);
+	ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
 
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
@@ -608,7 +609,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 				       *src, c->ip4.addr_seen);
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
-	b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
+	b->uh.len = htons(datalen + sizeof(b->uh));
 
 	return tap_iov_len(c, &b->taph, ip_len);
 }
@@ -616,16 +617,17 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 /**
  * udp_update_hdr6() - Update headers for one IPv6 datagram
  * @c:		Execution context
- * @n:		Index of buffer in udp6_l2_buf pool
+ * @b:		Pointer to udp6_l2_buf to update
  * @dstport:	Destination port number
+ * @datalen:	Length of UDP payload
  * @now:	Current timestamp
  *
  * Return: size of tap frame with headers
  */
-static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
+static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
+			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
 	const struct in6_addr *src, *dst;
 	uint16_t payload_len;
 	in_port_t srcport;
@@ -635,9 +637,9 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
 	src = &b->s_in6.sin6_addr;
 	srcport = ntohs(b->s_in6.sin6_port);
 
-	ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
+	ip_len = datalen + sizeof(b->ip6h) + sizeof(b->uh);
 
-	payload_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->uh);
+	payload_len = datalen + sizeof(b->uh);
 	b->ip6h.payload_len = htons(payload_len);
 
 	if (IN6_IS_ADDR_LINKLOCAL(src)) {
@@ -716,9 +718,11 @@ static void udp_tap_send(const struct ctx *c,
 		size_t buf_len;
 
 		if (v6)
-			buf_len = udp_update_hdr6(c, i, dstport, now);
+			buf_len = udp_update_hdr6(c, &udp6_l2_buf[i], dstport,
+						  udp6_l2_mh_sock[i].msg_len, now);
 		else
-			buf_len = udp_update_hdr4(c, i, dstport, now);
+			buf_len = udp_update_hdr4(c, &udp4_l2_buf[i], dstport,
+						  udp4_l2_mh_sock[i].msg_len, now);
 
 		tap_iov[i].iov_len = buf_len;
 	}
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/6] udp: Re-order udp_update_hdr[46] for clarity and brevity
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
                   ` (2 preceding siblings ...)
  2024-03-06  5:34 ` [PATCH v2 3/6] udp: Pass data length explicitly to to udp_update_hdr[46] David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-06  5:34 ` [PATCH v2 5/6] udp: Avoid unnecessary pointer in udp_update_hdr4() David Gibson
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

The order of things in these functions is a bit odd for historical reasons.
We initialise some IP header fields early, the more later after making
some tests.  Likewise we declare some variables without initialisation,
but then unconditionally set them to values we could calculate at the
start of the function.

Previous cleanups have removed the reasons for some of these choices, so
reorder for clarity, and where possible move the first assignment into an
initialiser.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/udp.c b/udp.c
index e20f537a..d3b9f5f9 100644
--- a/udp.c
+++ b/udp.c
@@ -574,17 +574,9 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	const struct in_addr *src;
-	in_port_t srcport;
-	size_t ip_len;
-
-	ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
-
-	b->iph.tot_len = htons(ip_len);
-	b->iph.daddr = c->ip4.addr_seen.s_addr;
-
-	src = &b->s_in.sin_addr;
-	srcport = ntohs(b->s_in.sin_port);
+	size_t ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
+	const struct in_addr *src = &b->s_in.sin_addr;
+	in_port_t srcport = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
 	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
@@ -603,10 +595,13 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 
 		src = &c->ip4.gw;
 	}
-	b->iph.saddr = src->s_addr;
 
+	b->iph.tot_len = htons(ip_len);
+	b->iph.daddr = c->ip4.addr_seen.s_addr;
+	b->iph.saddr = src->s_addr;
 	b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
 				       *src, c->ip4.addr_seen);
+
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
 	b->uh.len = htons(datalen + sizeof(b->uh));
@@ -628,19 +623,10 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	const struct in6_addr *src, *dst;
-	uint16_t payload_len;
-	in_port_t srcport;
-	size_t ip_len;
-
-	dst = &c->ip6.addr_seen;
-	src = &b->s_in6.sin6_addr;
-	srcport = ntohs(b->s_in6.sin6_port);
-
-	ip_len = datalen + sizeof(b->ip6h) + sizeof(b->uh);
-
-	payload_len = datalen + sizeof(b->uh);
-	b->ip6h.payload_len = htons(payload_len);
+	const struct in6_addr *src = &b->s_in6.sin6_addr;
+	const struct in6_addr *dst = &c->ip6.addr_seen;
+	uint16_t payload_len = datalen + sizeof(b->uh);
+	in_port_t srcport = ntohs(b->s_in6.sin6_port);
 
 	if (IN6_IS_ADDR_LINKLOCAL(src)) {
 		dst = &c->ip6.addr_ll_seen;
@@ -674,6 +660,8 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			src = &c->ip6.addr_ll;
 
 	}
+
+	b->ip6h.payload_len = htons(payload_len);
 	b->ip6h.daddr = *dst;
 	b->ip6h.saddr = *src;
 	b->ip6h.version = 6;
@@ -688,7 +676,7 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			   proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
 						  src, dst));
 
-	return tap_iov_len(c, &b->taph, ip_len);
+	return tap_iov_len(c, &b->taph, payload_len + sizeof(b->ip6h));
 }
 
 /**
-- 
@@ -574,17 +574,9 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	const struct in_addr *src;
-	in_port_t srcport;
-	size_t ip_len;
-
-	ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
-
-	b->iph.tot_len = htons(ip_len);
-	b->iph.daddr = c->ip4.addr_seen.s_addr;
-
-	src = &b->s_in.sin_addr;
-	srcport = ntohs(b->s_in.sin_port);
+	size_t ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
+	const struct in_addr *src = &b->s_in.sin_addr;
+	in_port_t srcport = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
 	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
@@ -603,10 +595,13 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 
 		src = &c->ip4.gw;
 	}
-	b->iph.saddr = src->s_addr;
 
+	b->iph.tot_len = htons(ip_len);
+	b->iph.daddr = c->ip4.addr_seen.s_addr;
+	b->iph.saddr = src->s_addr;
 	b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
 				       *src, c->ip4.addr_seen);
+
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
 	b->uh.len = htons(datalen + sizeof(b->uh));
@@ -628,19 +623,10 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			      in_port_t dstport, size_t datalen,
 			      const struct timespec *now)
 {
-	const struct in6_addr *src, *dst;
-	uint16_t payload_len;
-	in_port_t srcport;
-	size_t ip_len;
-
-	dst = &c->ip6.addr_seen;
-	src = &b->s_in6.sin6_addr;
-	srcport = ntohs(b->s_in6.sin6_port);
-
-	ip_len = datalen + sizeof(b->ip6h) + sizeof(b->uh);
-
-	payload_len = datalen + sizeof(b->uh);
-	b->ip6h.payload_len = htons(payload_len);
+	const struct in6_addr *src = &b->s_in6.sin6_addr;
+	const struct in6_addr *dst = &c->ip6.addr_seen;
+	uint16_t payload_len = datalen + sizeof(b->uh);
+	in_port_t srcport = ntohs(b->s_in6.sin6_port);
 
 	if (IN6_IS_ADDR_LINKLOCAL(src)) {
 		dst = &c->ip6.addr_ll_seen;
@@ -674,6 +660,8 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			src = &c->ip6.addr_ll;
 
 	}
+
+	b->ip6h.payload_len = htons(payload_len);
 	b->ip6h.daddr = *dst;
 	b->ip6h.saddr = *src;
 	b->ip6h.version = 6;
@@ -688,7 +676,7 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 			   proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
 						  src, dst));
 
-	return tap_iov_len(c, &b->taph, ip_len);
+	return tap_iov_len(c, &b->taph, payload_len + sizeof(b->ip6h));
 }
 
 /**
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/6] udp: Avoid unnecessary pointer in udp_update_hdr4()
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
                   ` (3 preceding siblings ...)
  2024-03-06  5:34 ` [PATCH v2 4/6] udp: Re-order udp_update_hdr[46] for clarity and brevity David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-06  5:34 ` [PATCH v2 6/6] udp: Use existing helper for UDP checksum on inbound IPv6 packets David Gibson
  2024-03-13 14:21 ` [PATCH v2 0/6] udp: Small cleanups Stefano Brivio
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

We carry around the source address as a pointer to a constant struct
in_addr.  But it's silly to carry around a 4 or 8 byte pointer to a 4 byte
IPv4 address.  Just copy the IPv4 address around by value.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/udp.c b/udp.c
index d3b9f5f9..fe3a6cd8 100644
--- a/udp.c
+++ b/udp.c
@@ -575,32 +575,32 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 			      const struct timespec *now)
 {
 	size_t ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
-	const struct in_addr *src = &b->s_in.sin_addr;
 	in_port_t srcport = ntohs(b->s_in.sin_port);
+	struct in_addr src = b->s_in.sin_addr;
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
-		src = &c->ip4.dns_match;
-	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
-		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
+	    IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport == 53) {
+		src = c->ip4.dns_match;
+	} else if (IN4_IS_ADDR_LOOPBACK(&src) ||
+		   IN4_ARE_ADDR_EQUAL(&src, &c->ip4.addr_seen)) {
 		udp_tap_map[V4][srcport].ts = now->tv_sec;
 		udp_tap_map[V4][srcport].flags |= PORT_LOCAL;
 
-		if (IN4_IS_ADDR_LOOPBACK(src))
+		if (IN4_IS_ADDR_LOOPBACK(&src))
 			udp_tap_map[V4][srcport].flags |= PORT_LOOPBACK;
 		else
 			udp_tap_map[V4][srcport].flags &= ~PORT_LOOPBACK;
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], srcport);
 
-		src = &c->ip4.gw;
+		src = c->ip4.gw;
 	}
 
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
-	b->iph.saddr = src->s_addr;
+	b->iph.saddr = src.s_addr;
 	b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
-				       *src, c->ip4.addr_seen);
+				       src, c->ip4.addr_seen);
 
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
-- 
@@ -575,32 +575,32 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
 			      const struct timespec *now)
 {
 	size_t ip_len = datalen + sizeof(b->iph) + sizeof(b->uh);
-	const struct in_addr *src = &b->s_in.sin_addr;
 	in_port_t srcport = ntohs(b->s_in.sin_port);
+	struct in_addr src = b->s_in.sin_addr;
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && srcport == 53) {
-		src = &c->ip4.dns_match;
-	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
-		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
+	    IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport == 53) {
+		src = c->ip4.dns_match;
+	} else if (IN4_IS_ADDR_LOOPBACK(&src) ||
+		   IN4_ARE_ADDR_EQUAL(&src, &c->ip4.addr_seen)) {
 		udp_tap_map[V4][srcport].ts = now->tv_sec;
 		udp_tap_map[V4][srcport].flags |= PORT_LOCAL;
 
-		if (IN4_IS_ADDR_LOOPBACK(src))
+		if (IN4_IS_ADDR_LOOPBACK(&src))
 			udp_tap_map[V4][srcport].flags |= PORT_LOOPBACK;
 		else
 			udp_tap_map[V4][srcport].flags &= ~PORT_LOOPBACK;
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], srcport);
 
-		src = &c->ip4.gw;
+		src = c->ip4.gw;
 	}
 
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
-	b->iph.saddr = src->s_addr;
+	b->iph.saddr = src.s_addr;
 	b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
-				       *src, c->ip4.addr_seen);
+				       src, c->ip4.addr_seen);
 
 	b->uh.source = b->s_in.sin_port;
 	b->uh.dest = htons(dstport);
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 6/6] udp: Use existing helper for UDP checksum on inbound IPv6 packets
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
                   ` (4 preceding siblings ...)
  2024-03-06  5:34 ` [PATCH v2 5/6] udp: Avoid unnecessary pointer in udp_update_hdr4() David Gibson
@ 2024-03-06  5:34 ` David Gibson
  2024-03-13 14:21 ` [PATCH v2 0/6] udp: Small cleanups Stefano Brivio
  6 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2024-03-06  5:34 UTC (permalink / raw)
  To: Stefano Brivio, passt-dev; +Cc: David Gibson

Currently we open code the calculation of the UDP checksum in
udp_update_hdr6().  We calling a helper to handle the IPv6 pseudo-header,
and preset the checksum field to 0 so an uninitialised value doesn't get
folded in.  We already have a helper to do this: csum_udp6() which we use
in some slow paths.  Use it here as well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/udp.c b/udp.c
index fe3a6cd8..45b7cc96 100644
--- a/udp.c
+++ b/udp.c
@@ -671,10 +671,7 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 	b->uh.source = b->s_in6.sin6_port;
 	b->uh.dest = htons(dstport);
 	b->uh.len = b->ip6h.payload_len;
-	b->uh.check = 0;
-	b->uh.check = csum(&b->uh, payload_len,
-			   proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
-						  src, dst));
+	csum_udp6(&b->uh, src, dst, b->data, datalen);
 
 	return tap_iov_len(c, &b->taph, payload_len + sizeof(b->ip6h));
 }
-- 
@@ -671,10 +671,7 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
 	b->uh.source = b->s_in6.sin6_port;
 	b->uh.dest = htons(dstport);
 	b->uh.len = b->ip6h.payload_len;
-	b->uh.check = 0;
-	b->uh.check = csum(&b->uh, payload_len,
-			   proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
-						  src, dst));
+	csum_udp6(&b->uh, src, dst, b->data, datalen);
 
 	return tap_iov_len(c, &b->taph, payload_len + sizeof(b->ip6h));
 }
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/6] udp: Small cleanups
  2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
                   ` (5 preceding siblings ...)
  2024-03-06  5:34 ` [PATCH v2 6/6] udp: Use existing helper for UDP checksum on inbound IPv6 packets David Gibson
@ 2024-03-13 14:21 ` Stefano Brivio
  6 siblings, 0 replies; 8+ messages in thread
From: Stefano Brivio @ 2024-03-13 14:21 UTC (permalink / raw)
  To: David Gibson; +Cc: passt-dev

On Wed,  6 Mar 2024 16:34:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Following on from the recent set of small fixes for UDP, here are a
> number of small cleanups to the UDP code, which will simplify later
> more complex fixes and improvements.
> 
> Laurent, I expect this will have some conflicts with part 2 of your
> vhost-user work, though I hope they won't be too bad.
> 
> This is now based on part 1 of Laurent's vhost-user series
> (specifically the version I posted with the static checker regressions
> fixed).
> 
> Changes since v1:
>  * Rebased on Laurent's vhost-user part 1 patches
>  * Some improved commit messages

Applied.

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-03-13 14:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-06  5:34 [PATCH v2 0/6] udp: Small cleanups David Gibson
2024-03-06  5:34 ` [PATCH v2 1/6] udp: Refactor udp_sock[46]_iov_init() David Gibson
2024-03-06  5:34 ` [PATCH v2 2/6] udp: Consistent port variable names in udp_update_hdr[46] David Gibson
2024-03-06  5:34 ` [PATCH v2 3/6] udp: Pass data length explicitly to to udp_update_hdr[46] David Gibson
2024-03-06  5:34 ` [PATCH v2 4/6] udp: Re-order udp_update_hdr[46] for clarity and brevity David Gibson
2024-03-06  5:34 ` [PATCH v2 5/6] udp: Avoid unnecessary pointer in udp_update_hdr4() David Gibson
2024-03-06  5:34 ` [PATCH v2 6/6] udp: Use existing helper for UDP checksum on inbound IPv6 packets David Gibson
2024-03-13 14:21 ` [PATCH v2 0/6] udp: Small cleanups Stefano Brivio

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).