TCP (both regular and spliced) and ICMP both have macros to retrieve the
relevant protcol specific flow structure from a flow index.  In most cases
what we actually want is to get the specific flow from a sidx.  Replace
those simple macros with a more precise inline, which also asserts that
the flow is of the type we expect.

While we're they're also add a pif_at_sidx() helper to get the interface of
a specific flow & side, which is useful in some places.

Finally, fix some minor style issues in the comments on some of the
existing sidx related helpers.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow_table.h | 26 ++++++++++++++++++++------
 icmp.c       | 22 +++++++++++++++++++---
 tcp.c        | 28 ++++++++++++++++++++++------
 tcp_splice.c | 21 +++++++++++++++++++--
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/flow_table.h b/flow_table.h
index 226ddbdd..ab73e44a 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -42,7 +42,7 @@ extern unsigned flow_first_free;
 extern union flow flowtab[];
 
 
-/** flow_idx - Index of flow from common structure
+/** flow_idx() - Index of flow from common structure
  * @f:	Common flow fields pointer
  *
  * Return: index of @f in the flow table
@@ -52,21 +52,21 @@ static inline unsigned flow_idx(const struct flow_common *f)
 	return (union flow *)f - flowtab;
 }
 
-/** FLOW_IDX - Find the index of a flow
+/** FLOW_IDX() - Find the index of a flow
  * @f_:	Flow pointer, either union flow * or protocol specific
  *
  * Return: index of @f in the flow table
  */
 #define FLOW_IDX(f_)		(flow_idx(&(f_)->f))
 
-/** FLOW - Flow entry at a given index
+/** FLOW() - Flow entry at a given index
  * @idx:	Flow index
  *
  * Return: pointer to entry @idx in the flow table
  */
 #define FLOW(idx)		(&flowtab[(idx)])
 
-/** flow_at_sidx - Flow entry for a given sidx
+/** flow_at_sidx() - Flow entry for a given sidx
  * @sidx:	Flow & side index
  *
  * Return: pointer to the corresponding flow entry, or NULL
@@ -78,7 +78,21 @@ static inline union flow *flow_at_sidx(flow_sidx_t sidx)
 	return FLOW(sidx.flow);
 }
 
-/** flow_sidx_t - Index of one side of a flow from common structure
+/** pif_at_sidx() - Interface for a given flow and side
+ * @sidx:    Flow & side index
+ *
+ * Return: pif for the flow & side given by @sidx
+ */
+static inline uint8_t pif_at_sidx(flow_sidx_t sidx)
+{
+	const union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return PIF_NONE;
+	return flow->f.pif[sidx.side];
+}
+
+/** flow_sidx() - Index of one side of a flow from common structure
  * @f:		Common flow fields pointer
  * @side:	Which side to refer to (0 or 1)
  *
@@ -96,7 +110,7 @@ static inline flow_sidx_t flow_sidx(const struct flow_common *f,
 	};
 }
 
-/** FLOW_SIDX - Find the index of one side of a flow
+/** FLOW_SIDX() - Find the index of one side of a flow
  * @f_:		Flow pointer, either union flow * or protocol specific
  * @side:	Which side to index (0 or 1)
  *
diff --git a/icmp.c b/icmp.c
index d4ccc722..7cf31e60 100644
--- a/icmp.c
+++ b/icmp.c
@@ -45,11 +45,27 @@
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
 #define ICMP_NUM_IDS		(1U << 16)
 
-#define PINGF(idx)		(&(FLOW(idx)->ping))
-
 /* Indexed by ICMP echo identifier */
 static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 
+/**
+ * ping_at_sidx() - Get ping specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: ping specific flow at @sidx, or NULL of @sidx is invalid.  Asserts if
+ *         the flow at @sidx is not FLOW_PING4 or FLOW_PING6
+ */
+static struct icmp_ping_flow *ping_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_PING4 || flow->f.type == FLOW_PING6);
+	return &flow->ping;
+}
+
 /**
  * icmp_sock_handler() - Handle new data from ICMP or ICMPv6 socket
  * @c:		Execution context
@@ -57,7 +73,7 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
  */
 void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
 {
-	struct icmp_ping_flow *pingf = PINGF(ref.flowside.flow);
+	struct icmp_ping_flow *pingf = ping_at_sidx(ref.flowside);
 	union sockaddr_inany sr;
 	socklen_t sl = sizeof(sr);
 	char buf[USHRT_MAX];
diff --git a/tcp.c b/tcp.c
index de3f9f64..04ea1f91 100644
--- a/tcp.c
+++ b/tcp.c
@@ -376,8 +376,6 @@ char		tcp_buf_discard		[MAX_WINDOW];
 /* sendmsg() to socket */
 static struct iovec	tcp_iov			[UIO_MAXIOV];
 
-#define CONN(idx)		(&(FLOW(idx)->tcp))
-
 /* Table for lookup from remote address, local port, remote port */
 static flow_sidx_t tc_hash[TCP_HASH_TABLE_SIZE];
 
@@ -388,6 +386,24 @@ static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX,
 int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
 int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
 
+/**
+ * conn_at_sidx() - Get TCP connection specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: TCP connection at @sidx, or NULL of @sidx is invalid.  Asserts if the
+ *         flow at @sidx is not FLOW_TCP.
+ */
+static struct tcp_tap_conn *conn_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_TCP);
+	return &flow->tcp;
+}
+
 /**
  * tcp_conn_epoll_events() - epoll events mask for given connection state
  * @events:	Current connection events
@@ -2339,9 +2355,10 @@ cancel:
 void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 {
 	struct itimerspec check_armed = { { 0 }, { 0 } };
-	struct tcp_tap_conn *conn = CONN(ref.flow);
+	struct tcp_tap_conn *conn = &FLOW(ref.flow)->tcp;
 
 	ASSERT(!c->no_tcp);
+	ASSERT(conn->f.type == FLOW_TCP);
 
 	/* We don't reset timers on ~ACK_FROM_TAP_DUE, ~ACK_TO_TAP_DUE. If the
 	 * timer is currently armed, this event came from a previous setting,
@@ -2397,11 +2414,10 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
  */
 void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
 {
-	struct tcp_tap_conn *conn = CONN(ref.flowside.flow);
+	struct tcp_tap_conn *conn = conn_at_sidx(ref.flowside);
 
 	ASSERT(!c->no_tcp);
-	ASSERT(conn->f.type == FLOW_TCP);
-	ASSERT(conn->f.pif[ref.flowside.side] != PIF_TAP);
+	ASSERT(pif_at_sidx(ref.flowside) != PIF_TAP);
 
 	if (conn->events == CLOSED)
 		return;
diff --git a/tcp_splice.c b/tcp_splice.c
index f2d4fc60..9fc56565 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -76,7 +76,6 @@ static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2];
 #define CONN_V6(x)			((x)->flags & SPLICE_V6)
 #define CONN_V4(x)			(!CONN_V6(x))
 #define CONN_HAS(conn, set)		(((conn)->events & (set)) == (set))
-#define CONN(idx)			(&FLOW(idx)->tcp_splice)
 
 /* Display strings for connection events */
 static const char *tcp_splice_event_str[] __attribute((__unused__)) = {
@@ -94,6 +93,24 @@ static const char *tcp_splice_flag_str[] __attribute((__unused__)) = {
 static int tcp_sock_refill_ns(void *arg);
 static int tcp_conn_sock_ns(const struct ctx *c, sa_family_t af);
 
+/**
+ * conn_at_sidx() - Get spliced TCP connection specific flow at given sidx
+ * @sidx:	Flow and side to retrieve
+ *
+ * Return: Spliced TCP connection at @sidx, or NULL of @sidx is invalid.
+ *         Asserts if the flow at @sidx is not FLOW_TCP_SPLICE.
+ */
+static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
+{
+	union flow *flow = flow_at_sidx(sidx);
+
+	if (!flow)
+		return NULL;
+
+	ASSERT(flow->f.type == FLOW_TCP_SPLICE);
+	return &flow->tcp_splice;
+}
+
 /**
  * tcp_splice_conn_epoll_events() - epoll events masks for given state
  * @events:	Connection event flags
@@ -502,7 +519,7 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
 void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			     uint32_t events)
 {
-	struct tcp_splice_conn *conn = CONN(ref.flowside.flow);
+	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
 	unsigned side = ref.flowside.side, fromside;
 	uint8_t lowat_set_flag, lowat_act_flag;
 	int eof, never_read;
-- 
2.45.2

Both the events and flags fields in tcp_splice_conn have several bits
which are per-side, e.g. OUT_WAIT_0 for side 0 and OUT_WAIT_1 for side 1.
This necessitates some rather awkward ternary expressions when we need
to get the relevant bit for a particular side.

Simplify this by using a parameterised macro for the bit values.  This
needs a ternary expression inside the macros, but makes the places we use
it substantially clearer.

That simplification in turn allows us to use a loop across each side to
implement several things which are currently open coded to do equivalent
things for each side in turn.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp_conn.h   | 15 +++++--------
 tcp_splice.c | 60 +++++++++++++++++++++++++---------------------------
 2 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/tcp_conn.h b/tcp_conn.h
index 5f8c8fb6..f80ef67b 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -129,19 +129,14 @@ struct tcp_splice_conn {
 #define SPLICE_CLOSED			0
 #define SPLICE_CONNECT			BIT(0)
 #define SPLICE_ESTABLISHED		BIT(1)
-#define OUT_WAIT_0			BIT(2)
-#define OUT_WAIT_1			BIT(3)
-#define FIN_RCVD_0			BIT(4)
-#define FIN_RCVD_1			BIT(5)
-#define FIN_SENT_0			BIT(6)
-#define FIN_SENT_1			BIT(7)
+#define OUT_WAIT(sidei_)		((sidei_) ? BIT(3) : BIT(2))
+#define FIN_RCVD(sidei_)		((sidei_) ? BIT(5) : BIT(4))
+#define FIN_SENT(sidei_)		((sidei_) ? BIT(7) : BIT(6))
 
 	uint8_t flags;
 #define SPLICE_V6			BIT(0)
-#define RCVLOWAT_SET_0			BIT(1)
-#define RCVLOWAT_SET_1			BIT(2)
-#define RCVLOWAT_ACT_0			BIT(3)
-#define RCVLOWAT_ACT_1			BIT(4)
+#define RCVLOWAT_SET(sidei_)		((sidei_) ? BIT(2) : BIT(1))
+#define RCVLOWAT_ACT(sidei_)		((sidei_) ? BIT(4) : BIT(3))
 #define CLOSING				BIT(5)
 
 	uint32_t read[SIDES];
diff --git a/tcp_splice.c b/tcp_splice.c
index 5a9325b1..ffddc853 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -119,19 +119,22 @@ static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
 static void tcp_splice_conn_epoll_events(uint16_t events,
 					 struct epoll_event ev[])
 {
-	ev[0].events = ev[1].events = 0;
+	unsigned sidei;
+
+	flow_foreach_sidei(sidei)
+		ev[sidei].events = 0;
 
 	if (events & SPLICE_ESTABLISHED) {
-		if (!(events & FIN_SENT_1))
-			ev[0].events = EPOLLIN | EPOLLRDHUP;
-		if (!(events & FIN_SENT_0))
-			ev[1].events = EPOLLIN | EPOLLRDHUP;
+		flow_foreach_sidei(sidei) {
+			if (!(events & FIN_SENT(!sidei)))
+				ev[sidei].events = EPOLLIN | EPOLLRDHUP;
+		}
 	} else if (events & SPLICE_CONNECT) {
 		ev[1].events = EPOLLOUT;
 	}
 
-	ev[0].events |= (events & OUT_WAIT_0) ? EPOLLOUT : 0;
-	ev[1].events |= (events & OUT_WAIT_1) ? EPOLLOUT : 0;
+	flow_foreach_sidei(sidei)
+		ev[sidei].events |= (events & OUT_WAIT(sidei)) ? EPOLLOUT : 0;
 }
 
 /**
@@ -553,21 +556,21 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 
 	if (events & EPOLLOUT) {
 		fromsidei = !evsidei;
-		conn_event(c, conn, evsidei == 0 ? ~OUT_WAIT_0 : ~OUT_WAIT_1);
+		conn_event(c, conn, ~OUT_WAIT(evsidei));
 	} else {
 		fromsidei = evsidei;
 	}
 
 	if (events & EPOLLRDHUP)
 		/* For side 0 this is fake, but implied */
-		conn_event(c, conn, evsidei == 0 ? FIN_RCVD_0 : FIN_RCVD_1);
+		conn_event(c, conn, FIN_RCVD(evsidei));
 
 swap:
 	eof = 0;
 	never_read = 1;
 
-	lowat_set_flag = fromsidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-	lowat_act_flag = fromsidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
+	lowat_set_flag = RCVLOWAT_SET(fromsidei);
+	lowat_act_flag = RCVLOWAT_ACT(fromsidei);
 
 	while (1) {
 		ssize_t readlen, to_write = 0, written;
@@ -644,8 +647,7 @@ eintr:
 			if (conn->read[fromsidei] == conn->written[fromsidei])
 				break;
 
-			conn_event(c, conn,
-				   fromsidei == 0 ? OUT_WAIT_1 : OUT_WAIT_0);
+			conn_event(c, conn, OUT_WAIT(fromsidei));
 			break;
 		}
 
@@ -661,21 +663,19 @@ eintr:
 			break;
 	}
 
-	if ((conn->events & FIN_RCVD_0) && !(conn->events & FIN_SENT_1))
{
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[1], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_1);
-		}
-	}
+	if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
+		unsigned sidei;
 
-	if ((conn->events & FIN_RCVD_1) && !(conn->events & FIN_SENT_0))
{
-		if (conn->read[fromsidei] == conn->written[fromsidei] && eof) {
-			shutdown(conn->s[0], SHUT_WR);
-			conn_event(c, conn, FIN_SENT_0);
+		flow_foreach_sidei(sidei) {
+			if ((conn->events & FIN_RCVD(sidei)) &&
+			    !(conn->events & FIN_SENT(!sidei))) {
+				shutdown(conn->s[!sidei], SHUT_WR);
+				conn_event(c, conn, FIN_SENT(!sidei));
+			}
 		}
 	}
 
-	if (CONN_HAS(conn, FIN_SENT_0 | FIN_SENT_1))
+	if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1)))
 		goto close;
 
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
@@ -821,19 +821,17 @@ void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn
*conn)
 	ASSERT(!(conn->flags & CLOSING));
 
 	flow_foreach_sidei(sidei) {
-		uint8_t set = sidei == 0 ? RCVLOWAT_SET_0 : RCVLOWAT_SET_1;
-		uint8_t act = sidei == 0 ? RCVLOWAT_ACT_0 : RCVLOWAT_ACT_1;
-
-		if ((conn->flags & set) && !(conn->flags & act)) {
+		if ((conn->flags & RCVLOWAT_SET(sidei)) &&
+		    !(conn->flags & RCVLOWAT_ACT(sidei))) {
 			if (setsockopt(conn->s[sidei], SOL_SOCKET, SO_RCVLOWAT,
 				       &((int){ 1 }), sizeof(int))) {
 				flow_trace(conn, "can't set SO_RCVLOWAT on %d",
 					   conn->s[sidei]);
 			}
-			conn_flag(c, conn, ~set);
+			conn_flag(c, conn, ~RCVLOWAT_SET(sidei));
 		}
 	}
 
-	conn_flag(c, conn, ~RCVLOWAT_ACT_0);
-	conn_flag(c, conn, ~RCVLOWAT_ACT_1);
+	flow_foreach_sidei(sidei)
+		conn_flag(c, conn, ~RCVLOWAT_ACT(sidei));
 }
-- 
2.45.2

This test program verifies that we can receive and discard datagrams by
using recv() with a NULL buffer and zero-length.  Extend it to verify it
also works using recvmsg() and either an iov with a zero-length NULL
buffer or an iov that itself is NULL and zero-length.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 doc/platform-requirements/recv-zero.c | 60 +++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 8 deletions(-)

diff --git a/doc/platform-requirements/recv-zero.c
b/doc/platform-requirements/recv-zero.c
index f161e5c2..ab27704d 100644
--- a/doc/platform-requirements/recv-zero.c
+++ b/doc/platform-requirements/recv-zero.c
@@ -23,11 +23,27 @@
 
 #define DSTPORT	13257U
 
+enum discard_method {
+	DISCARD_NULL_BUF,
+	DISCARD_ZERO_IOV,
+	DISCARD_NULL_IOV,
+	NUM_METHODS,
+};
+
 /* 127.0.0.1:DSTPORT */
 static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
 
-static void test_discard(void)
+static void test_discard(enum discard_method method)
 {
+	struct iovec zero_iov = { .iov_base = NULL, .iov_len = 0, };
+	struct msghdr mh_zero = {
+		.msg_iov = &zero_iov,
+		.msg_iovlen = 1,
+	};
+	struct msghdr mh_null = {
+		.msg_iov = NULL,
+		.msg_iovlen = 0,
+	};
 	long token1, token2;
 	int recv_s, send_s;
 	ssize_t rc;
@@ -46,11 +62,36 @@ static void test_discard(void)
 	send_token(send_s, token1);
 	send_token(send_s, token2);
 
-	/* cppcheck-suppress nullPointer */
-	rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
-	if (rc < 0)
-		die("discarding recv(): %s\n", strerror(errno));
-	
+	switch (method) {
+	case DISCARD_NULL_BUF:
+		/* cppcheck-suppress nullPointer */
+		rc = recv(recv_s, NULL, 0, MSG_DONTWAIT);
+		if (rc < 0)
+			die("discarding recv(): %s\n", strerror(errno));
+		break;
+
+	case DISCARD_ZERO_IOV:
+		rc = recvmsg(recv_s, &mh_zero, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length buffer: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_zero.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	case DISCARD_NULL_IOV:
+		rc = recvmsg(recv_s, &mh_null, MSG_DONTWAIT);
+		if (rc < 0)
+			die("recvmsg() with zero-length iov: %s\n",
+			    strerror(errno));
+		if (!((unsigned)mh_null.msg_flags & MSG_TRUNC))
+			die("Missing MSG_TRUNC flag\n");
+		break;
+
+	default:
+		die("Bad method\n");
+	}
+
 	recv_token(recv_s, token2);
 
 	/* cppcheck-suppress nullPointer */
@@ -63,12 +104,15 @@ static void test_discard(void)
 
 int main(int argc, char *argv[])
 {
+	enum discard_method method;
+
 	(void)argc;
 	(void)argv;
 
-	test_discard();
+	for (method = 0; method < NUM_METHODS; method++)
+		test_discard(method);
 
-	printf("Discarding datagrams with a 0-length recv() seems to work\n");
+	printf("Discarding datagrams with a 0-length receives seems to work\n");
 
 	exit(0);
 }
-- 
2.45.2

On Wed, 17 Jul 2024 14:52:17 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  This series has some further preliminaries for the
flow table.
 Specifically this is focused on some clean ups to handling of indicies
 for flows and sides.  We also add some additional test programs for
 platform requirements.
 
 This is based on the series adding UDP error handling.
 
 David Gibson (6):
   flow, icmp, tcp: Clean up helpers for getting flow from index
   flow, tcp_splice: Prefer 'sidei' for variables referring to side index
   flow: Introduce flow_foreach_sidei() macro
   tcp_splice: Use parameterised macros for per-side event/flag bits
   doc: Test behaviour of closing duplicate UDP sockets
   doc: Extend zero-recv test with methods using msghdr 
Applied.

-- 
Stefano