[PATCH 0/2] udp_flow: move all udp_flow functions to udp_flow.c

[PATCH 0/2] udp_flow: move all udp_flow functions to udp_flow.c

[Laurent Vivier]

This helps to export them and use them with vhost-user version
of udp.c

There is only one code change: pass s_in rather than udp_meta_t to
udp_flow_from_sock() as udp_meta_t is specific to the socket version
of udp.c

Otherwise, only code move

Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Laurent Vivier (2):
  udp_flow: Remove udp_meta_t from the parameters of
    udp_flow_from_sock()
  udp_flow: move all udp_flow functions to udp_flow.c

 Makefile   |   2 +-
 udp.c      | 264 +--------------------------------------------------
 udp_flow.c | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 udp_flow.h |   9 ++
 4 files changed, 286 insertions(+), 263 deletions(-)
 create mode 100644 udp_flow.c

-- 
2.45.2

[PATCH 1/2] udp_flow: Remove udp_meta_t from the parameters of udp_flow_from_sock()

[Laurent Vivier]

To be used with the vhost-user version of udp.c, we need to export the
udp_flow functions. To avoid to export udp_meta_t too that is specific
to the socket version of udp.c, don't pass udp_meta_t to it,
but the only needed field, s_in.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 udp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/udp.c b/udp.c
index a92014806a73..f27a00bf5610 100644
--- a/udp.c
+++ b/udp.c
@@ -432,7 +432,7 @@ cancel:
  * udp_flow_from_sock() - Find or create UDP flow for "listening" socket
  * @c:		Execution context
  * @ref:	epoll reference of the receiving socket
- * @meta:	Metadata buffer for the datagram
+ * @s_in:	Source socket address, filled in by recvmmsg()
  * @now:	Timestamp
  *
  * #syscalls fcntl
@@ -441,7 +441,7 @@ cancel:
  *         FLOW_SIDX_NONE if we couldn't find or create a flow.
  */
 static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
-				      struct udp_meta_t *meta,
+				      const union sockaddr_inany *s_in,
 				      const struct timespec *now)
 {
 	struct udp_flow *uflow;
@@ -450,7 +450,7 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 
 	ASSERT(ref.type == EPOLL_TYPE_UDP_LISTEN);
 
-	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, &meta->s_in, ref.udp.port);
+	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, s_in, ref.udp.port);
 	if ((uflow = udp_at_sidx(sidx))) {
 		uflow->ts = now->tv_sec;
 		return flow_sidx_opposite(sidx);
@@ -461,11 +461,11 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 
 		debug("Couldn't allocate flow for UDP datagram from %s %s",
 		      pif_name(ref.udp.pif),
-		      sockaddr_ntop(&meta->s_in, sastr, sizeof(sastr)));
+		      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_sa(flow, ref.udp.pif, &meta->s_in, ref.udp.port);
+	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
 	return udp_flow_new(c, flow, ref.fd, now);
 }
 
@@ -712,7 +712,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
 	 * the array, or recalculating tosidx for a single entry, we have to
 	 * populate it one entry *ahead* of the loop counter.
 	 */
-	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0], now);
+	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0].s_in, now);
 	for (i = 0; i < n; ) {
 		flow_sidx_t batchsidx = udp_meta[i].tosidx;
 		uint8_t batchpif = pif_at_sidx(batchsidx);
@@ -730,7 +730,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
 				break;
 
 			udp_meta[i].tosidx = udp_flow_from_sock(c, ref,
-								&udp_meta[i],
+								&udp_meta[i].s_in,
 								now);
 		} while (flow_sidx_eq(udp_meta[i].tosidx, batchsidx));
 
-- 
@@ -432,7 +432,7 @@ cancel:
  * udp_flow_from_sock() - Find or create UDP flow for "listening" socket
  * @c:		Execution context
  * @ref:	epoll reference of the receiving socket
- * @meta:	Metadata buffer for the datagram
+ * @s_in:	Source socket address, filled in by recvmmsg()
  * @now:	Timestamp
  *
  * #syscalls fcntl
@@ -441,7 +441,7 @@ cancel:
  *         FLOW_SIDX_NONE if we couldn't find or create a flow.
  */
 static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
-				      struct udp_meta_t *meta,
+				      const union sockaddr_inany *s_in,
 				      const struct timespec *now)
 {
 	struct udp_flow *uflow;
@@ -450,7 +450,7 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 
 	ASSERT(ref.type == EPOLL_TYPE_UDP_LISTEN);
 
-	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, &meta->s_in, ref.udp.port);
+	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, s_in, ref.udp.port);
 	if ((uflow = udp_at_sidx(sidx))) {
 		uflow->ts = now->tv_sec;
 		return flow_sidx_opposite(sidx);
@@ -461,11 +461,11 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
 
 		debug("Couldn't allocate flow for UDP datagram from %s %s",
 		      pif_name(ref.udp.pif),
-		      sockaddr_ntop(&meta->s_in, sastr, sizeof(sastr)));
+		      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
 		return FLOW_SIDX_NONE;
 	}
 
-	flow_initiate_sa(flow, ref.udp.pif, &meta->s_in, ref.udp.port);
+	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
 	return udp_flow_new(c, flow, ref.fd, now);
 }
 
@@ -712,7 +712,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
 	 * the array, or recalculating tosidx for a single entry, we have to
 	 * populate it one entry *ahead* of the loop counter.
 	 */
-	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0], now);
+	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0].s_in, now);
 	for (i = 0; i < n; ) {
 		flow_sidx_t batchsidx = udp_meta[i].tosidx;
 		uint8_t batchpif = pif_at_sidx(batchsidx);
@@ -730,7 +730,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
 				break;
 
 			udp_meta[i].tosidx = udp_flow_from_sock(c, ref,
-								&udp_meta[i],
+								&udp_meta[i].s_in,
 								now);
 		} while (flow_sidx_eq(udp_meta[i].tosidx, batchsidx));
 
-- 
2.45.2

[PATCH 2/2] udp_flow: move all udp_flow functions to udp_flow.c

[Laurent Vivier]

No code change.

They need to be exported to be available by the vhost-user version of
passt.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 Makefile   |   2 +-
 udp.c      | 260 --------------------------------------------------
 udp_flow.c | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 udp_flow.h |   9 ++
 4 files changed, 284 insertions(+), 261 deletions(-)
 create mode 100644 udp_flow.c

diff --git a/Makefile b/Makefile
index bd504d23da3c..b6329e35f884 100644
--- a/Makefile
+++ b/Makefile
@@ -47,7 +47,7 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
 PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \
 	icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \
 	ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \
-	tcp_buf.c tcp_splice.c udp.c util.c
+	tcp_buf.c tcp_splice.c udp.c udp_flow.c util.c
 QRAP_SRCS = qrap.c
 SRCS = $(PASST_SRCS) $(QRAP_SRCS)
 
diff --git a/udp.c b/udp.c
index f27a00bf5610..7731257292e1 100644
--- a/udp.c
+++ b/udp.c
@@ -95,7 +95,6 @@
 #include <sys/socket.h>
 #include <sys/uio.h>
 #include <time.h>
-#include <fcntl.h>
 #include <arpa/inet.h>
 #include <linux/errqueue.h>
 
@@ -111,7 +110,6 @@
 #include "log.h"
 #include "flow_table.h"
 
-#define UDP_CONN_TIMEOUT	180 /* s, timeout for ephemeral or local bind */
 #define UDP_MAX_FRAMES		32  /* max # of frames to receive at once */
 
 /* "Spliced" sockets indexed by bound port (host order) */
@@ -276,199 +274,6 @@ static void udp_iov_init(const struct ctx *c)
 		udp_iov_init_one(c, i);
 }
 
-/**
- * udp_at_sidx() - Get UDP specific flow at given sidx
- * @sidx:    Flow and side to retrieve
- *
- * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
- *         the flow at @sidx is not FLOW_UDP.
- */
-struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
-{
-	union flow *flow = flow_at_sidx(sidx);
-
-	if (!flow)
-		return NULL;
-
-	ASSERT(flow->f.type == FLOW_UDP);
-	return &flow->udp;
-}
-
-/*
- * udp_flow_close() - Close and clean up UDP flow
- * @c:		Execution context
- * @uflow:	UDP flow
- */
-static void udp_flow_close(const struct ctx *c, struct udp_flow *uflow)
-{
-	if (uflow->s[INISIDE] >= 0) {
-		/* The listening socket needs to stay in epoll */
-		close(uflow->s[INISIDE]);
-		uflow->s[INISIDE] = -1;
-	}
-
-	if (uflow->s[TGTSIDE] >= 0) {
-		/* But the flow specific one needs to be removed */
-		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, uflow->s[TGTSIDE], NULL);
-		close(uflow->s[TGTSIDE]);
-		uflow->s[TGTSIDE] = -1;
-	}
-	flow_hash_remove(c, FLOW_SIDX(uflow, INISIDE));
-	if (!pif_is_socket(uflow->f.pif[TGTSIDE]))
-		flow_hash_remove(c, FLOW_SIDX(uflow, TGTSIDE));
-}
-
-/**
- * udp_flow_new() - Common setup for a new UDP flow
- * @c:		Execution context
- * @flow:	Initiated flow
- * @s_ini:	Initiating socket (or -1)
- * @now:	Timestamp
- *
- * Return: UDP specific flow, if successful, NULL on failure
- */
-static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
-				int s_ini, const struct timespec *now)
-{
-	const struct flowside *ini = &flow->f.side[INISIDE];
-	struct udp_flow *uflow = NULL;
-	const struct flowside *tgt;
-	uint8_t tgtpif;
-
-	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0) {
-		flow_trace(flow, "Invalid endpoint to initiate UDP flow");
-		goto cancel;
-	}
-
-	if (!(tgt = flow_target(c, flow, IPPROTO_UDP)))
-		goto cancel;
-	tgtpif = flow->f.pif[TGTSIDE];
-
-	uflow = FLOW_SET_TYPE(flow, FLOW_UDP, udp);
-	uflow->ts = now->tv_sec;
-	uflow->s[INISIDE] = uflow->s[TGTSIDE] = -1;
-
-	if (s_ini >= 0) {
-		/* When using auto port-scanning the listening port could go
-		 * away, so we need to duplicate the socket
-		 */
-		uflow->s[INISIDE] = fcntl(s_ini, F_DUPFD_CLOEXEC, 0);
-		if (uflow->s[INISIDE] < 0) {
-			flow_err(uflow,
-				 "Couldn't duplicate listening socket: %s",
-				 strerror(errno));
-			goto cancel;
-		}
-	}
-
-	if (pif_is_socket(tgtpif)) {
-		struct mmsghdr discard[UIO_MAXIOV] = { 0 };
-		union {
-			flow_sidx_t sidx;
-			uint32_t data;
-		} fref = {
-			.sidx = FLOW_SIDX(flow, TGTSIDE),
-		};
-		int rc;
-
-		uflow->s[TGTSIDE] = flowside_sock_l4(c, EPOLL_TYPE_UDP_REPLY,
-						     tgtpif, tgt, fref.data);
-		if (uflow->s[TGTSIDE] < 0) {
-			flow_dbg(uflow,
-				 "Couldn't open socket for spliced flow: %s",
-				 strerror(errno));
-			goto cancel;
-		}
-
-		if (flowside_connect(c, uflow->s[TGTSIDE], tgtpif, tgt) < 0) {
-			flow_dbg(uflow,
-				 "Couldn't connect flow socket: %s",
-				 strerror(errno));
-			goto cancel;
-		}
-
-		/* It's possible, if unlikely, that we could receive some
-		 * unrelated packets in between the bind() and connect() of this
-		 * socket.  For now we just discard these.  We could consider
-		 * trying to redirect these to an appropriate handler, if we
-		 * need to.
-		 */
-		rc = recvmmsg(uflow->s[TGTSIDE], discard, ARRAY_SIZE(discard),
-			      MSG_DONTWAIT, NULL);
-		if (rc >= ARRAY_SIZE(discard)) {
-			flow_dbg(uflow,
-				 "Too many (%d) spurious reply datagrams", rc);
-			goto cancel;
-		} else if (rc > 0) {
-			flow_trace(uflow,
-				   "Discarded %d spurious reply datagrams", rc);
-		} else if (errno != EAGAIN) {
-			flow_err(uflow,
-				 "Unexpected error discarding datagrams: %s",
-				 strerror(errno));
-		}
-	}
-
-	flow_hash_insert(c, FLOW_SIDX(uflow, INISIDE));
-
-	/* If the target side is a socket, it will be a reply socket that knows
-	 * its own flowside.  But if it's tap, then we need to look it up by
-	 * hash.
-	 */
-	if (!pif_is_socket(tgtpif))
-		flow_hash_insert(c, FLOW_SIDX(uflow, TGTSIDE));
-	FLOW_ACTIVATE(uflow);
-
-	return FLOW_SIDX(uflow, TGTSIDE);
-
-cancel:
-	if (uflow)
-		udp_flow_close(c, uflow);
-	flow_alloc_cancel(flow);
-	return FLOW_SIDX_NONE;
-}
-
-/**
- * udp_flow_from_sock() - Find or create UDP flow for "listening" socket
- * @c:		Execution context
- * @ref:	epoll reference of the receiving socket
- * @s_in:	Source socket address, filled in by recvmmsg()
- * @now:	Timestamp
- *
- * #syscalls fcntl
- *
- * Return: sidx for the destination side of the flow for this packet, or
- *         FLOW_SIDX_NONE if we couldn't find or create a flow.
- */
-static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
-				      const union sockaddr_inany *s_in,
-				      const struct timespec *now)
-{
-	struct udp_flow *uflow;
-	union flow *flow;
-	flow_sidx_t sidx;
-
-	ASSERT(ref.type == EPOLL_TYPE_UDP_LISTEN);
-
-	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, s_in, ref.udp.port);
-	if ((uflow = udp_at_sidx(sidx))) {
-		uflow->ts = now->tv_sec;
-		return flow_sidx_opposite(sidx);
-	}
-
-	if (!(flow = flow_alloc())) {
-		char sastr[SOCKADDR_STRLEN];
-
-		debug("Couldn't allocate flow for UDP datagram from %s %s",
-		      pif_name(ref.udp.pif),
-		      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
-		return FLOW_SIDX_NONE;
-	}
-
-	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
-	return udp_flow_new(c, flow, ref.fd, now);
-}
-
 /**
  * udp_splice_prepare() - Prepare one datagram for splicing
  * @mmh:	Receiving mmsghdr array
@@ -804,53 +609,6 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref,
 	}
 }
 
-/**
- * udp_flow_from_tap() - Find or create UDP flow for tap packets
- * @c:		Execution context
- * @pif:	pif on which the packet is arriving
- * @af:		Address family, AF_INET or AF_INET6
- * @saddr:	Source address on guest side
- * @daddr:	Destination address guest side
- * @srcport:	Source port on guest side
- * @dstport:	Destination port on guest side
- *
- * Return: sidx for the destination side of the flow for this packet, or
- *         FLOW_SIDX_NONE if we couldn't find or create a flow.
- */
-static flow_sidx_t udp_flow_from_tap(const struct ctx *c,
-				     uint8_t pif, sa_family_t af,
-				     const void *saddr, const void *daddr,
-				     in_port_t srcport, in_port_t dstport,
-				     const struct timespec *now)
-{
-	struct udp_flow *uflow;
-	union flow *flow;
-	flow_sidx_t sidx;
-
-	ASSERT(pif == PIF_TAP);
-
-	sidx = flow_lookup_af(c, IPPROTO_UDP, pif, af, saddr, daddr,
-			      srcport, dstport);
-	if ((uflow = udp_at_sidx(sidx))) {
-		uflow->ts = now->tv_sec;
-		return flow_sidx_opposite(sidx);
-	}
-
-	if (!(flow = flow_alloc())) {
-		char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
-
-		debug("Couldn't allocate flow for UDP datagram from %s %s:%hu -> %s:%hu",
-		      pif_name(pif),
-		      inet_ntop(af, saddr, sstr, sizeof(sstr)), srcport,
-		      inet_ntop(af, daddr, dstr, sizeof(dstr)), dstport);
-		return FLOW_SIDX_NONE;
-	}
-
-	flow_initiate_af(flow, PIF_TAP, af, saddr, srcport, daddr, dstport);
-
-	return udp_flow_new(c, flow, -1, now);
-}
-
 /**
  * udp_tap_handler() - Handle packets from tap
  * @c:		Execution context
@@ -1098,24 +856,6 @@ static int udp_port_rebind_outbound(void *arg)
 	return 0;
 }
 
-/**
- * udp_flow_timer() - Handler for timed events related to a given flow
- * @c:		Execution context
- * @uflow:	UDP flow
- * @now:	Current timestamp
- *
- * Return: true if the flow is ready to free, false otherwise
- */
-bool udp_flow_timer(const struct ctx *c, struct udp_flow *uflow,
-		    const struct timespec *now)
-{
-	if (now->tv_sec - uflow->ts <= UDP_CONN_TIMEOUT)
-		return false;
-
-	udp_flow_close(c, uflow);
-	return true;
-}
-
 /**
  * udp_timer() - Scan activity bitmaps for ports with associated timed events
  * @c:		Execution context
diff --git a/udp_flow.c b/udp_flow.c
new file mode 100644
index 000000000000..8b25ad162eaa
--- /dev/null
+++ b/udp_flow.c
@@ -0,0 +1,274 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ *
+ * UDP flow tracking functions
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/uio.h>
+
+#include "util.h"
+#include "passt.h"
+#include "flow_table.h"
+
+#define UDP_CONN_TIMEOUT	180 /* s, timeout for ephemeral or local bind */
+
+/**
+ * udp_at_sidx() - Get UDP specific flow at given sidx
+ * @sidx:    Flow and side to retrieve
+ *
+ * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
+ *         the flow at @sidx is not FLOW_UDP.
+ */
+struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_UDP);
+	return &flow->udp;
+}
+
+/*
+ * udp_flow_close() - Close and clean up UDP flow
+ * @c:		Execution context
+ * @uflow:	UDP flow
+ */
+static void udp_flow_close(const struct ctx *c, struct udp_flow *uflow)
+{
+	if (uflow->s[INISIDE] >= 0) {
+		/* The listening socket needs to stay in epoll */
+		close(uflow->s[INISIDE]);
+		uflow->s[INISIDE] = -1;
+	}
+
+	if (uflow->s[TGTSIDE] >= 0) {
+		/* But the flow specific one needs to be removed */
+		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, uflow->s[TGTSIDE], NULL);
+		close(uflow->s[TGTSIDE]);
+		uflow->s[TGTSIDE] = -1;
+	}
+	flow_hash_remove(c, FLOW_SIDX(uflow, INISIDE));
+	if (!pif_is_socket(uflow->f.pif[TGTSIDE]))
+		flow_hash_remove(c, FLOW_SIDX(uflow, TGTSIDE));
+}
+
+/**
+ * udp_flow_new() - Common setup for a new UDP flow
+ * @c:		Execution context
+ * @flow:	Initiated flow
+ * @s_ini:	Initiating socket (or -1)
+ * @now:	Timestamp
+ *
+ * Return: UDP specific flow, if successful, NULL on failure
+ */
+static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
+				int s_ini, const struct timespec *now)
+{
+	const struct flowside *ini = &flow->f.side[INISIDE];
+	struct udp_flow *uflow = NULL;
+	const struct flowside *tgt;
+	uint8_t tgtpif;
+
+	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0) {
+		flow_trace(flow, "Invalid endpoint to initiate UDP flow");
+		goto cancel;
+	}
+
+	if (!(tgt = flow_target(c, flow, IPPROTO_UDP)))
+		goto cancel;
+	tgtpif = flow->f.pif[TGTSIDE];
+
+	uflow = FLOW_SET_TYPE(flow, FLOW_UDP, udp);
+	uflow->ts = now->tv_sec;
+	uflow->s[INISIDE] = uflow->s[TGTSIDE] = -1;
+
+	if (s_ini >= 0) {
+		/* When using auto port-scanning the listening port could go
+		 * away, so we need to duplicate the socket
+		 */
+		uflow->s[INISIDE] = fcntl(s_ini, F_DUPFD_CLOEXEC, 0);
+		if (uflow->s[INISIDE] < 0) {
+			flow_err(uflow,
+				 "Couldn't duplicate listening socket: %s",
+				 strerror(errno));
+			goto cancel;
+		}
+	}
+
+	if (pif_is_socket(tgtpif)) {
+		struct mmsghdr discard[UIO_MAXIOV] = { 0 };
+		union {
+			flow_sidx_t sidx;
+			uint32_t data;
+		} fref = {
+			.sidx = FLOW_SIDX(flow, TGTSIDE),
+		};
+		int rc;
+
+		uflow->s[TGTSIDE] = flowside_sock_l4(c, EPOLL_TYPE_UDP_REPLY,
+						     tgtpif, tgt, fref.data);
+		if (uflow->s[TGTSIDE] < 0) {
+			flow_dbg(uflow,
+				 "Couldn't open socket for spliced flow: %s",
+				 strerror(errno));
+			goto cancel;
+		}
+
+		if (flowside_connect(c, uflow->s[TGTSIDE], tgtpif, tgt) < 0) {
+			flow_dbg(uflow,
+				 "Couldn't connect flow socket: %s",
+				 strerror(errno));
+			goto cancel;
+		}
+
+		/* It's possible, if unlikely, that we could receive some
+		 * unrelated packets in between the bind() and connect() of this
+		 * socket.  For now we just discard these.  We could consider
+		 * trying to redirect these to an appropriate handler, if we
+		 * need to.
+		 */
+		rc = recvmmsg(uflow->s[TGTSIDE], discard, ARRAY_SIZE(discard),
+			      MSG_DONTWAIT, NULL);
+		if (rc >= ARRAY_SIZE(discard)) {
+			flow_dbg(uflow,
+				 "Too many (%d) spurious reply datagrams", rc);
+			goto cancel;
+		} else if (rc > 0) {
+			flow_trace(uflow,
+				   "Discarded %d spurious reply datagrams", rc);
+		} else if (errno != EAGAIN) {
+			flow_err(uflow,
+				 "Unexpected error discarding datagrams: %s",
+				 strerror(errno));
+		}
+	}
+
+	flow_hash_insert(c, FLOW_SIDX(uflow, INISIDE));
+
+	/* If the target side is a socket, it will be a reply socket that knows
+	 * its own flowside.  But if it's tap, then we need to look it up by
+	 * hash.
+	 */
+	if (!pif_is_socket(tgtpif))
+		flow_hash_insert(c, FLOW_SIDX(uflow, TGTSIDE));
+	FLOW_ACTIVATE(uflow);
+
+	return FLOW_SIDX(uflow, TGTSIDE);
+
+cancel:
+	if (uflow)
+		udp_flow_close(c, uflow);
+	flow_alloc_cancel(flow);
+	return FLOW_SIDX_NONE;
+}
+
+/**
+ * udp_flow_from_sock() - Find or create UDP flow for "listening" socket
+ * @c:		Execution context
+ * @ref:	epoll reference of the receiving socket
+ * @s_in:	Source socket address, filled in by recvmmsg()
+ * @now:	Timestamp
+ *
+ * #syscalls fcntl
+ *
+ * Return: sidx for the destination side of the flow for this packet, or
+ *         FLOW_SIDX_NONE if we couldn't find or create a flow.
+ */
+flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
+			       const union sockaddr_inany *s_in,
+			       const struct timespec *now)
+{
+	struct udp_flow *uflow;
+	union flow *flow;
+	flow_sidx_t sidx;
+
+	ASSERT(ref.type == EPOLL_TYPE_UDP_LISTEN);
+
+	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, s_in, ref.udp.port);
+	if ((uflow = udp_at_sidx(sidx))) {
+		uflow->ts = now->tv_sec;
+		return flow_sidx_opposite(sidx);
+	}
+
+	if (!(flow = flow_alloc())) {
+		char sastr[SOCKADDR_STRLEN];
+
+		debug("Couldn't allocate flow for UDP datagram from %s %s",
+		      pif_name(ref.udp.pif),
+		      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
+		return FLOW_SIDX_NONE;
+	}
+
+	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
+	return udp_flow_new(c, flow, ref.fd, now);
+}
+
+/**
+ * udp_flow_from_tap() - Find or create UDP flow for tap packets
+ * @c:		Execution context
+ * @pif:	pif on which the packet is arriving
+ * @af:		Address family, AF_INET or AF_INET6
+ * @saddr:	Source address on guest side
+ * @daddr:	Destination address guest side
+ * @srcport:	Source port on guest side
+ * @dstport:	Destination port on guest side
+ *
+ * Return: sidx for the destination side of the flow for this packet, or
+ *         FLOW_SIDX_NONE if we couldn't find or create a flow.
+ */
+flow_sidx_t udp_flow_from_tap(const struct ctx *c,
+			      uint8_t pif, sa_family_t af,
+			      const void *saddr, const void *daddr,
+			      in_port_t srcport, in_port_t dstport,
+			      const struct timespec *now)
+{
+	struct udp_flow *uflow;
+	union flow *flow;
+	flow_sidx_t sidx;
+
+	ASSERT(pif == PIF_TAP);
+
+	sidx = flow_lookup_af(c, IPPROTO_UDP, pif, af, saddr, daddr,
+			      srcport, dstport);
+	if ((uflow = udp_at_sidx(sidx))) {
+		uflow->ts = now->tv_sec;
+		return flow_sidx_opposite(sidx);
+	}
+
+	if (!(flow = flow_alloc())) {
+		char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
+
+		debug("Couldn't allocate flow for UDP datagram from %s %s:%hu -> %s:%hu",
+		      pif_name(pif),
+		      inet_ntop(af, saddr, sstr, sizeof(sstr)), srcport,
+		      inet_ntop(af, daddr, dstr, sizeof(dstr)), dstport);
+		return FLOW_SIDX_NONE;
+	}
+
+	flow_initiate_af(flow, PIF_TAP, af, saddr, srcport, daddr, dstport);
+
+	return udp_flow_new(c, flow, -1, now);
+}
+
+/**
+ * udp_flow_timer() - Handler for timed events related to a given flow
+ * @c:		Execution context
+ * @uflow:	UDP flow
+ * @now:	Current timestamp
+ *
+ * Return: true if the flow is ready to free, false otherwise
+ */
+bool udp_flow_timer(const struct ctx *c, struct udp_flow *uflow,
+		    const struct timespec *now)
+{
+	if (now->tv_sec - uflow->ts <= UDP_CONN_TIMEOUT)
+		return false;
+
+	udp_flow_close(c, uflow);
+	return true;
+}
diff --git a/udp_flow.h b/udp_flow.h
index e0736f8f60bc..12ddf0394cce 100644
--- a/udp_flow.h
+++ b/udp_flow.h
@@ -21,6 +21,15 @@ struct udp_flow {
 	int s[SIDES];
 };
 
+struct udp_flow *udp_at_sidx(flow_sidx_t sidx);
+flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
+			       const union sockaddr_inany *s_in,
+			       const struct timespec *now);
+flow_sidx_t udp_flow_from_tap(const struct ctx *c,
+			      uint8_t pif, sa_family_t af,
+			      const void *saddr, const void *daddr,
+			      in_port_t srcport, in_port_t dstport,
+			      const struct timespec *now);
 bool udp_flow_timer(const struct ctx *c, struct udp_flow *uflow,
 		    const struct timespec *now);
 
-- 
@@ -21,6 +21,15 @@ struct udp_flow {
 	int s[SIDES];
 };
 
+struct udp_flow *udp_at_sidx(flow_sidx_t sidx);
+flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
+			       const union sockaddr_inany *s_in,
+			       const struct timespec *now);
+flow_sidx_t udp_flow_from_tap(const struct ctx *c,
+			      uint8_t pif, sa_family_t af,
+			      const void *saddr, const void *daddr,
+			      in_port_t srcport, in_port_t dstport,
+			      const struct timespec *now);
 bool udp_flow_timer(const struct ctx *c, struct udp_flow *uflow,
 		    const struct timespec *now);
 
-- 
2.45.2

Re: [PATCH 1/2] udp_flow: Remove udp_meta_t from the parameters of udp_flow_from_sock()

[David Gibson]

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

On Fri, Aug 02, 2024 at 06:10:35PM +0200, Laurent Vivier wrote:
> To be used with the vhost-user version of udp.c, we need to export the
> udp_flow functions. To avoid to export udp_meta_t too that is specific
> to the socket version of udp.c, don't pass udp_meta_t to it,
> but the only needed field, s_in.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  udp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index a92014806a73..f27a00bf5610 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -432,7 +432,7 @@ cancel:
>   * udp_flow_from_sock() - Find or create UDP flow for "listening" socket
>   * @c:		Execution context
>   * @ref:	epoll reference of the receiving socket
> - * @meta:	Metadata buffer for the datagram
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   * @now:	Timestamp
>   *
>   * #syscalls fcntl
> @@ -441,7 +441,7 @@ cancel:
>   *         FLOW_SIDX_NONE if we couldn't find or create a flow.
>   */
>  static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
> -				      struct udp_meta_t *meta,
> +				      const union sockaddr_inany *s_in,
>  				      const struct timespec *now)
>  {
>  	struct udp_flow *uflow;
> @@ -450,7 +450,7 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
>  
>  	ASSERT(ref.type == EPOLL_TYPE_UDP_LISTEN);
>  
> -	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, &meta->s_in, ref.udp.port);
> +	sidx = flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, s_in, ref.udp.port);
>  	if ((uflow = udp_at_sidx(sidx))) {
>  		uflow->ts = now->tv_sec;
>  		return flow_sidx_opposite(sidx);
> @@ -461,11 +461,11 @@ static flow_sidx_t udp_flow_from_sock(const struct ctx *c, union epoll_ref ref,
>  
>  		debug("Couldn't allocate flow for UDP datagram from %s %s",
>  		      pif_name(ref.udp.pif),
> -		      sockaddr_ntop(&meta->s_in, sastr, sizeof(sastr)));
> +		      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
>  		return FLOW_SIDX_NONE;
>  	}
>  
> -	flow_initiate_sa(flow, ref.udp.pif, &meta->s_in, ref.udp.port);
> +	flow_initiate_sa(flow, ref.udp.pif, s_in, ref.udp.port);
>  	return udp_flow_new(c, flow, ref.fd, now);
>  }
>  
> @@ -712,7 +712,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
>  	 * the array, or recalculating tosidx for a single entry, we have to
>  	 * populate it one entry *ahead* of the loop counter.
>  	 */
> -	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0], now);
> +	udp_meta[0].tosidx = udp_flow_from_sock(c, ref, &udp_meta[0].s_in, now);
>  	for (i = 0; i < n; ) {
>  		flow_sidx_t batchsidx = udp_meta[i].tosidx;
>  		uint8_t batchpif = pif_at_sidx(batchsidx);
> @@ -730,7 +730,7 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref,
>  				break;
>  
>  			udp_meta[i].tosidx = udp_flow_from_sock(c, ref,
> -								&udp_meta[i],
> +								&udp_meta[i].s_in,
>  								now);
>  		} while (flow_sidx_eq(udp_meta[i].tosidx, batchsidx));
>  

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH 2/2] udp_flow: move all udp_flow functions to udp_flow.c

[David Gibson]

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

On Fri, Aug 02, 2024 at 06:10:36PM +0200, Laurent Vivier wrote:
> No code change.
> 
> They need to be exported to be available by the vhost-user version of
> passt.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

LGTM, with one nit:

> +/**
> + * udp_at_sidx() - Get UDP specific flow at given sidx
> + * @sidx:    Flow and side to retrieve
> + *
> + * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
> + *         the flow at @sidx is not FLOW_UDP.
> + */
> +struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
> +{
> +	union flow *flow = flow_at_sidx(sidx);
> +
> +	if (!flow)
> +		return NULL;
> +
> +	ASSERT(flow->f.type == FLOW_UDP);
> +	return &flow->udp;
> +}

udp_at_sidx() is so simple it probably makes more sense to have it as
an inline in the header file, rather than a regular function.  When it
was a local function, I was pretty much assuming the compiler would
inline it anyways.

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH 2/2] udp_flow: move all udp_flow functions to udp_flow.c

[Laurent Vivier]

On 03/08/2024 09:51, David Gibson wrote:
> On Fri, Aug 02, 2024 at 06:10:36PM +0200, Laurent Vivier wrote:
>> No code change.
>>
>> They need to be exported to be available by the vhost-user version of
>> passt.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> LGTM, with one nit:
> 
>> +/**
>> + * udp_at_sidx() - Get UDP specific flow at given sidx
>> + * @sidx:    Flow and side to retrieve
>> + *
>> + * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
>> + *         the flow at @sidx is not FLOW_UDP.
>> + */
>> +struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
>> +{
>> +	union flow *flow = flow_at_sidx(sidx);
>> +
>> +	if (!flow)
>> +		return NULL;
>> +
>> +	ASSERT(flow->f.type == FLOW_UDP);
>> +	return &flow->udp;
>> +}
> 
> udp_at_sidx() is so simple it probably makes more sense to have it as
> an inline in the header file, rather than a regular function.  When it
> was a local function, I was pretty much assuming the compiler would
> inline it anyways.
> 

I can't move it to udp_flow.h because udp_at_sidx() uses flow_at_sidx() that is defined in 
flow_table.h after udp_flow.h has already been included. flow_table.h includes udp_flow.h 
at its top because it needs "struct udp_flow" in union flow.

Thanks,
Laurent

Re: [PATCH 2/2] udp_flow: move all udp_flow functions to udp_flow.c

[David Gibson]

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

On Mon, Aug 05, 2024 at 09:11:36AM +0200, Laurent Vivier wrote:
> On 03/08/2024 09:51, David Gibson wrote:
> > On Fri, Aug 02, 2024 at 06:10:36PM +0200, Laurent Vivier wrote:
> > > No code change.
> > > 
> > > They need to be exported to be available by the vhost-user version of
> > > passt.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > LGTM, with one nit:
> > 
> > > +/**
> > > + * udp_at_sidx() - Get UDP specific flow at given sidx
> > > + * @sidx:    Flow and side to retrieve
> > > + *
> > > + * Return: UDP specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
> > > + *         the flow at @sidx is not FLOW_UDP.
> > > + */
> > > +struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
> > > +{
> > > +	union flow *flow = flow_at_sidx(sidx);
> > > +
> > > +	if (!flow)
> > > +		return NULL;
> > > +
> > > +	ASSERT(flow->f.type == FLOW_UDP);
> > > +	return &flow->udp;
> > > +}
> > 
> > udp_at_sidx() is so simple it probably makes more sense to have it as
> > an inline in the header file, rather than a regular function.  When it
> > was a local function, I was pretty much assuming the compiler would
> > inline it anyways.
> > 
> 
> I can't move it to udp_flow.h because udp_at_sidx() uses flow_at_sidx() that
> is defined in flow_table.h after udp_flow.h has already been included.
> flow_table.h includes udp_flow.h at its top because it needs "struct
> udp_flow" in union flow.

Ah, I didn't think of that.  Never mind, then.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH 0/2] udp_flow: move all udp_flow functions to udp_flow.c

[Stefano Brivio]

On Fri,  2 Aug 2024 18:10:34 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> This helps to export them and use them with vhost-user version
> of udp.c
> 
> There is only one code change: pass s_in rather than udp_meta_t to
> udp_flow_from_sock() as udp_meta_t is specific to the socket version
> of udp.c
> 
> Otherwise, only code move
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Laurent Vivier (2):
>   udp_flow: Remove udp_meta_t from the parameters of
>     udp_flow_from_sock()
>   udp_flow: move all udp_flow functions to udp_flow.c

Applied.

-- 
Stefano