[PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[David Gibson]

As discussed on our recent calls, the C11 standard introduces
anonymous structure and union members and static assertions, amongst
other things.  Both of these could be useful in a few places in
passt/pasta to make the code more readable and safer.

However, at the moment, the compiler flags we use only allow C99 code.
This series allows C11 code, and makes some fairly obvious cleanups by
using it.

It would be nice to get an opinion on this reasonably quickly, because
I have other patches in the works that will look different depending
on whether or not they can use C11 features.

David Gibson (3):
  Allow C11 code, not just C99 code
  Use C11 anonymous members to make poll refs less verbose to use
  Use static assertion to verify that union epoll_ref is the right size

 Makefile     |  4 ++--
 icmp.c       | 22 +++++++++++-----------
 icmp.h       |  2 +-
 passt.c      |  8 ++++----
 passt.h      |  8 ++++++--
 tcp.c        | 46 +++++++++++++++++++++++-----------------------
 tcp.h        |  2 +-
 tcp_splice.c | 11 +++++------
 udp.c        | 50 +++++++++++++++++++++++---------------------------
 udp.h        |  2 +-
 util.c       |  4 ++--
 11 files changed, 79 insertions(+), 80 deletions(-)

-- 
2.41.0

[PATCH 1/3] Allow C11 code, not just C99 code

[David Gibson]

C11 has some features that will allow us to make some things a bit cleaner.
Alter the Makefile to tell the compiler to allow us to use C11 code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index a5256f5..4435bd6 100644
--- a/Makefile
+++ b/Makefile
@@ -33,7 +33,7 @@ AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/MIPS64EL/MIPSEL64/')
 AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/HPPA/PARISC/')
 AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/SH4/SH/')
 
-FLAGS := -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE
+FLAGS := -Wall -Wextra -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE
 FLAGS += -D_FORTIFY_SOURCE=2 -O2 -pie -fPIE
 FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
 FLAGS += -DNETNS_RUN_DIR=\"/run/netns\"
@@ -284,7 +284,7 @@ VER := $(shell $(CC) -dumpversion)
 SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include
 endif
 cppcheck: $(SRCS) $(HEADERS)
-	cppcheck --std=c99 --error-exitcode=1 --enable=all --force	\
+	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
 	--inconclusive --library=posix --quiet				\
 	$(SYSTEM_INCLUDES:%=-I%)					\
 	$(SYSTEM_INCLUDES:%=--config-exclude=%)				\
-- 
@@ -33,7 +33,7 @@ AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/MIPS64EL/MIPSEL64/')
 AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/HPPA/PARISC/')
 AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/SH4/SH/')
 
-FLAGS := -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE
+FLAGS := -Wall -Wextra -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE
 FLAGS += -D_FORTIFY_SOURCE=2 -O2 -pie -fPIE
 FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
 FLAGS += -DNETNS_RUN_DIR=\"/run/netns\"
@@ -284,7 +284,7 @@ VER := $(shell $(CC) -dumpversion)
 SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include
 endif
 cppcheck: $(SRCS) $(HEADERS)
-	cppcheck --std=c99 --error-exitcode=1 --enable=all --force	\
+	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
 	--inconclusive --library=posix --quiet				\
 	$(SYSTEM_INCLUDES:%=-I%)					\
 	$(SYSTEM_INCLUDES:%=--config-exclude=%)				\
-- 
2.41.0

[PATCH 2/3] Use C11 anonymous members to make poll refs less verbose to use

[David Gibson]

union epoll_ref has a deeply nested set of structs and unions to let us
subdivide it into the various different fields we want.  This means that
referencing elements can involve an awkward long string of intermediate
fields.

Using C11 anonymous structs and unions lets us do this less clumsily.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 icmp.c       | 22 +++++++++++-----------
 icmp.h       |  2 +-
 passt.c      |  8 ++++----
 passt.h      |  4 ++--
 tcp.c        | 46 +++++++++++++++++++++++-----------------------
 tcp.h        |  2 +-
 tcp_splice.c | 11 +++++------
 udp.c        | 50 +++++++++++++++++++++++---------------------------
 udp.h        |  2 +-
 util.c       |  4 ++--
 10 files changed, 73 insertions(+), 78 deletions(-)

diff --git a/icmp.c b/icmp.c
index 44f73c8..676fa64 100644
--- a/icmp.c
+++ b/icmp.c
@@ -69,7 +69,7 @@ static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
 void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
 		       uint32_t events, const struct timespec *now)
 {
-	union icmp_epoll_ref *iref = &ref.r.p.icmp;
+	union icmp_epoll_ref *iref = &ref.icmp;
 	struct sockaddr_storage sr;
 	socklen_t sl = sizeof(sr);
 	char buf[USHRT_MAX];
@@ -79,11 +79,11 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
 	(void)events;
 	(void)now;
 
-	n = recvfrom(ref.r.s, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
+	n = recvfrom(ref.s, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
 	if (n < 0)
 		return;
 
-	if (iref->icmp.v6) {
+	if (iref->v6) {
 		struct sockaddr_in6 *sr6 = (struct sockaddr_in6 *)&sr;
 		struct icmp6hdr *ih = (struct icmp6hdr *)buf;
 
@@ -93,8 +93,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
 		/* If bind() fails e.g. because of a broken SELinux policy, this
 		 * might happen. Fix up the identifier to match the sent one.
 		 */
-		if (id != iref->icmp.id)
-			ih->icmp6_identifier = htons(iref->icmp.id);
+		if (id != iref->id)
+			ih->icmp6_identifier = htons(iref->id);
 
 		/* In PASTA mode, we'll get any reply we send, discard them. */
 		if (c->mode == MODE_PASTA) {
@@ -116,8 +116,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
 		id = ntohs(ih->un.echo.id);
 		seq = ntohs(ih->un.echo.sequence);
 
-		if (id != iref->icmp.id)
-			ih->un.echo.id = htons(iref->icmp.id);
+		if (id != iref->id)
+			ih->un.echo.id = htons(iref->id);
 
 		if (c->mode == MODE_PASTA) {
 
@@ -150,7 +150,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 	size_t plen;
 
 	if (af == AF_INET) {
-		union icmp_epoll_ref iref = { .icmp.v6 = 0 };
+		union icmp_epoll_ref iref = { .v6 = 0 };
 		struct sockaddr_in sa = {
 			.sin_family = AF_INET,
 			.sin_addr = { .s_addr = htonl(INADDR_ANY) },
@@ -167,7 +167,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 
 		sa.sin_port = ih->un.echo.id;
 
-		iref.icmp.id = id = ntohs(ih->un.echo.id);
+		iref.id = id = ntohs(ih->un.echo.id);
 
 		if ((s = icmp_id_map[V4][id].sock) <= 0) {
 			const struct in_addr *bind_addr = NULL;
@@ -204,7 +204,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 			      id, ntohs(ih->un.echo.sequence));
 		}
 	} else if (af == AF_INET6) {
-		union icmp_epoll_ref iref = { .icmp.v6 = 1 };
+		union icmp_epoll_ref iref = { .v6 = 1 };
 		struct sockaddr_in6 sa = {
 			.sin6_family = AF_INET6,
 			.sin6_addr = IN6ADDR_ANY_INIT,
@@ -222,7 +222,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 
 		sa.sin6_port = ih->icmp6_identifier;
 
-		iref.icmp.id = id = ntohs(ih->icmp6_identifier);
+		iref.id = id = ntohs(ih->icmp6_identifier);
 		if ((s = icmp_id_map[V6][id].sock) <= 0) {
 			const struct in6_addr *bind_addr = NULL;
 			const char *bind_if;
diff --git a/icmp.h b/icmp.h
index f899e6d..29c7829 100644
--- a/icmp.h
+++ b/icmp.h
@@ -27,7 +27,7 @@ union icmp_epoll_ref {
 	struct {
 		uint32_t	v6:1,
 				id:16;
-	} icmp;
+	};
 	uint32_t u32;
 };
 
diff --git a/passt.c b/passt.c
index 3b9b36b..9123868 100644
--- a/passt.c
+++ b/passt.c
@@ -75,14 +75,14 @@ static void sock_handler(struct ctx *c, union epoll_ref ref,
 {
 	trace("%s: %s packet from socket %i (events: 0x%08x)",
 	      c->mode == MODE_PASST ? "passt" : "pasta",
-	      IP_PROTO_STR(ref.r.proto), ref.r.s, events);
+	      IP_PROTO_STR(ref.proto), ref.s, events);
 
-	if (!c->no_tcp && ref.r.proto == IPPROTO_TCP)
+	if (!c->no_tcp && ref.proto == IPPROTO_TCP)
 		tcp_sock_handler( c, ref, events, now);
-	else if (!c->no_udp && ref.r.proto == IPPROTO_UDP)
+	else if (!c->no_udp && ref.proto == IPPROTO_UDP)
 		udp_sock_handler( c, ref, events, now);
 	else if (!c->no_icmp &&
-		 (ref.r.proto == IPPROTO_ICMP || ref.r.proto == IPPROTO_ICMPV6))
+		 (ref.proto == IPPROTO_ICMP || ref.proto == IPPROTO_ICMPV6))
 		icmp_sock_handler(c, ref, events, now);
 }
 
diff --git a/passt.h b/passt.h
index 96fd27b..8b7235a 100644
--- a/passt.h
+++ b/passt.h
@@ -60,8 +60,8 @@ union epoll_ref {
 			union udp_epoll_ref udp;
 			union icmp_epoll_ref icmp;
 			uint32_t data;
-		} p;
-	} r;
+		};
+	};
 	uint64_t u64;
 };
 
diff --git a/tcp.c b/tcp.c
index 0ed9bfa..d9ecee5 100644
--- a/tcp.c
+++ b/tcp.c
@@ -643,8 +643,8 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn,
 static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
 {
 	int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD;
-	union epoll_ref ref = { .r.proto = IPPROTO_TCP, .r.s = conn->sock,
-				.r.p.tcp.tcp.index = CONN_IDX(conn) };
+	union epoll_ref ref = { .proto = IPPROTO_TCP, .s = conn->sock,
+				.tcp.index = CONN_IDX(conn) };
 	struct epoll_event ev = { .data.u64 = ref.u64 };
 
 	if (conn->events == CLOSED) {
@@ -663,10 +663,10 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
 	conn->c.in_epoll = true;
 
 	if (conn->timer != -1) {
-		union epoll_ref ref_t = { .r.proto = IPPROTO_TCP,
-					  .r.s = conn->sock,
-					  .r.p.tcp.tcp.timer = 1,
-					  .r.p.tcp.tcp.index = CONN_IDX(conn) };
+		union epoll_ref ref_t = { .proto = IPPROTO_TCP,
+					  .s = conn->sock,
+					  .tcp.timer = 1,
+					  .tcp.index = CONN_IDX(conn) };
 		struct epoll_event ev_t = { .data.u64 = ref_t.u64,
 					    .events = EPOLLIN | EPOLLET };
 
@@ -692,10 +692,10 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
 		return;
 
 	if (conn->timer == -1) {
-		union epoll_ref ref = { .r.proto = IPPROTO_TCP,
-					.r.s = conn->sock,
-					.r.p.tcp.tcp.timer = 1,
-					.r.p.tcp.tcp.index = CONN_IDX(conn) };
+		union epoll_ref ref = { .proto = IPPROTO_TCP,
+					.s = conn->sock,
+					.tcp.timer = 1,
+					.tcp.index = CONN_IDX(conn) };
 		struct epoll_event ev = { .data.u64 = ref.u64,
 					  .events = EPOLLIN | EPOLLET };
 		int fd;
@@ -2749,7 +2749,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	conn_event(c, conn, SOCK_ACCEPTED);
 
 	inany_from_sockaddr(&conn->addr, &conn->sock_port, sa);
-	conn->tap_port = ref.r.p.tcp.tcp.index;
+	conn->tap_port = ref.tcp.index;
 
 	tcp_snat_inbound(c, &conn->addr);
 
@@ -2780,7 +2780,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	socklen_t sl;
 	int s;
 
-	ASSERT(ref.r.p.tcp.tcp.listen);
+	ASSERT(ref.tcp.listen);
 
 	if (c->tcp.conn_count >= TCP_MAX_CONNS)
 		return;
@@ -2791,7 +2791,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	 * https://github.com/llvm/llvm-project/issues/58992
 	 */
 	memset(&sa, 0, sizeof(struct sockaddr_in6));
-	s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);
+	s = accept4(ref.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);
 	if (s < 0)
 		return;
 
@@ -2815,7 +2815,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
  */
 static void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 {
-	struct tcp_tap_conn *conn = conn_at_idx(ref.r.p.tcp.tcp.index);
+	struct tcp_tap_conn *conn = conn_at_idx(ref.tcp.index);
 	struct itimerspec check_armed = { { 0 }, { 0 } };
 
 	if (!conn)
@@ -2935,20 +2935,20 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
 {
 	union tcp_conn *conn;
 
-	if (ref.r.p.tcp.tcp.timer) {
+	if (ref.tcp.timer) {
 		tcp_timer_handler(c, ref);
 		return;
 	}
 
-	if (ref.r.p.tcp.tcp.listen) {
+	if (ref.tcp.listen) {
 		tcp_conn_from_sock(c, ref, now);
 		return;
 	}
 
-	conn = tc + ref.r.p.tcp.tcp.index;
+	conn = tc + ref.tcp.index;
 
 	if (conn->c.spliced)
-		tcp_splice_sock_handler(c, &conn->splice, ref.r.s, events);
+		tcp_splice_sock_handler(c, &conn->splice, ref.s, events);
 	else
 		tcp_tap_sock_handler(c, &conn->tap, events);
 }
@@ -2967,7 +2967,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
 			    const struct in_addr *addr, const char *ifname)
 {
 	in_port_t idx = port + c->tcp.fwd_in.delta[port];
-	union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.index = idx };
+	union tcp_epoll_ref tref = { .listen = 1, .index = idx };
 	int s;
 
 	s = sock_l4(c, af, IPPROTO_TCP, addr, ifname, port, tref.u32);
@@ -3027,8 +3027,8 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
 static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
 {
 	in_port_t idx = port + c->tcp.fwd_out.delta[port];
-	union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = 1,
-				     .tcp.index = idx };
+	union tcp_epoll_ref tref = { .listen = 1, .outbound = 1,
+				     .index = idx };
 	struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
 	int s;
 
@@ -3052,8 +3052,8 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
 static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port)
 {
 	in_port_t idx = port + c->tcp.fwd_out.delta[port];
-	union tcp_epoll_ref tref = { .tcp.listen = 1, .tcp.outbound = 1,
-				     .tcp.index = idx };
+	union tcp_epoll_ref tref = { .listen = 1, .outbound = 1,
+				     .index = idx };
 	int s;
 
 	ASSERT(c->mode == MODE_PASTA);
diff --git a/tcp.h b/tcp.h
index a9e3425..5499127 100644
--- a/tcp.h
+++ b/tcp.h
@@ -41,7 +41,7 @@ union tcp_epoll_ref {
 				outbound:1,
 				timer:1,
 				index:20;
-	} tcp;
+	};
 	uint32_t u32;
 };
 
diff --git a/tcp_splice.c b/tcp_splice.c
index 71256b0..03e14c1 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -173,10 +173,10 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
 				struct tcp_splice_conn *conn)
 {
 	int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD;
-	union epoll_ref ref_a = { .r.proto = IPPROTO_TCP, .r.s = conn->a,
-				  .r.p.tcp.tcp.index = CONN_IDX(conn) };
-	union epoll_ref ref_b = { .r.proto = IPPROTO_TCP, .r.s = conn->b,
-				  .r.p.tcp.tcp.index = CONN_IDX(conn) };
+	union epoll_ref ref_a = { .proto = IPPROTO_TCP, .s = conn->a,
+				  .tcp.index = CONN_IDX(conn) };
+	union epoll_ref ref_b = { .proto = IPPROTO_TCP, .s = conn->b,
+				  .tcp.index = CONN_IDX(conn) };
 	struct epoll_event ev_a = { .data.u64 = ref_a.u64 };
 	struct epoll_event ev_b = { .data.u64 = ref_b.u64 };
 	uint32_t events_a, events_b;
@@ -516,8 +516,7 @@ bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	c->tcp.splice_conn_count++;
 	conn->a = s;
 
-	if (tcp_splice_new(c, conn, ref.r.p.tcp.tcp.index,
-			   ref.r.p.tcp.tcp.outbound))
+	if (tcp_splice_new(c, conn, ref.tcp.index, ref.tcp.outbound))
 		conn_flag(c, conn, CLOSING);
 
 	return true;
diff --git a/udp.c b/udp.c
index 39c59d4..f749f5f 100644
--- a/udp.c
+++ b/udp.c
@@ -388,9 +388,9 @@ static void udp_sock6_iov_init(const struct ctx *c)
 int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
 {
 	struct epoll_event ev = { .events = EPOLLIN | EPOLLRDHUP | EPOLLHUP };
-	union epoll_ref ref = { .r.proto = IPPROTO_UDP,
-				.r.p.udp.udp = { .splice = true, .ns = ns,
-						 .v6 = v6, .port = src }
+	union epoll_ref ref = { .proto = IPPROTO_UDP,
+				.udp = { .splice = true, .ns = ns,
+					     .v6 = v6, .port = src }
 			      };
 	struct udp_splice_port *sp;
 	int act, s;
@@ -414,7 +414,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
 	if (s < 0)
 		return s;
 
-	ref.r.s = s;
+	ref.s = s;
 
 	if (v6) {
 		struct sockaddr_in6 addr6 = {
@@ -752,8 +752,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
 	 * for pasta mode.
 	 */
 	ssize_t n = (c->mode == MODE_PASST ? UDP_MAX_FRAMES : 1);
-	in_port_t dstport = ref.r.p.udp.udp.port;
-	bool v6 = ref.r.p.udp.udp.v6;
+	in_port_t dstport = ref.udp.port;
+	bool v6 = ref.udp.v6;
 	struct mmsghdr *mmh_recv;
 	int i, m;
 
@@ -768,7 +768,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
 		udp4_localname.sin_port = htons(dstport);
 	}
 
-	n = recvmmsg(ref.r.s, mmh_recv, n, 0, NULL);
+	n = recvmmsg(ref.s, mmh_recv, n, 0, NULL);
 	if (n <= 0)
 		return;
 
@@ -776,7 +776,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
 		int splicefrom = -1;
 		m = n;
 
-		if (ref.r.p.udp.udp.splice) {
+		if (ref.udp.splice) {
 			splicefrom = udp_mmh_splice_port(v6, mmh_recv + i);
 
 			for (m = 1; i + m < n; m++) {
@@ -790,8 +790,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
 
 		if (splicefrom >= 0)
 			udp_splice_sendfrom(c, i, m, splicefrom, dstport,
-					    v6, ref.r.p.udp.udp.ns,
-					    ref.r.p.udp.udp.orig, now);
+					    v6, ref.udp.ns, ref.udp.orig, now);
 		else
 			udp_tap_send(c, i, m, dstport, v6, now);
 	}
@@ -857,7 +856,7 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
 		}
 
 		if (!(s = udp_tap_map[V4][src].sock)) {
-			union udp_epoll_ref uref = { .udp.port = src };
+			union udp_epoll_ref uref = { .port = src };
 			in_addr_t bind_addr = { 0 };
 			const char *bind_if = NULL;
 
@@ -907,8 +906,7 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
 		}
 
 		if (!(s = udp_tap_map[V6][src].sock)) {
-			union udp_epoll_ref uref = { .udp.v6 = 1,
-						     .udp.port = src };
+			union udp_epoll_ref uref = { .v6 = 1, .port = src };
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
@@ -986,27 +984,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
 
 	if (ns) {
-		uref.udp.port = (in_port_t)(port +
-					    c->udp.fwd_out.f.delta[port]);
+		uref.port = (in_port_t)(port + c->udp.fwd_out.f.delta[port]);
 	} else {
-		uref.udp.port = (in_port_t)(port +
-					    c->udp.fwd_in.f.delta[port]);
+		uref.port = (in_port_t)(port + c->udp.fwd_in.f.delta[port]);
 	}
 
 	if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) {
-		uref.udp.v6 = 0;
-		uref.udp.splice = (c->mode == MODE_PASTA);
-		uref.udp.orig = true;
+		uref.v6 = 0;
+		uref.splice = (c->mode == MODE_PASTA);
+		uref.orig = true;
 
 		if (!ns) {
 			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
 					 ifname, port, uref.u32);
 
-			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
+			udp_tap_map[V4][uref.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
 		} else {
 			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
-			uref.udp.ns = true;
+			uref.ns = true;
 
 			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
 					 ifname, port, uref.u32);
@@ -1015,18 +1011,18 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 	}
 
 	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
-		uref.udp.v6 = 1;
-		uref.udp.splice = (c->mode == MODE_PASTA);
-		uref.udp.orig = true;
+		uref.v6 = 1;
+		uref.splice = (c->mode == MODE_PASTA);
+		uref.orig = true;
 
 		if (!ns) {
 			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
 					 ifname, port, uref.u32);
 
-			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
+			udp_tap_map[V6][uref.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
 		} else {
-			uref.udp.ns = true;
+			uref.ns = true;
 
 			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
 					 &in6addr_loopback,
diff --git a/udp.h b/udp.h
index e64ef18..56bcd78 100644
--- a/udp.h
+++ b/udp.h
@@ -36,7 +36,7 @@ union udp_epoll_ref {
 				ns:1,
 				v6:1;
 		uint32_t	port:16;
-	} udp;
+	};
 	uint32_t u32;
 };
 
diff --git a/util.c b/util.c
index 1d00404..b9f4e7d 100644
--- a/util.c
+++ b/util.c
@@ -102,7 +102,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
 	    uint32_t data)
 {
-	union epoll_ref ref = { .r.proto = proto, .r.p.data = data };
+	union epoll_ref ref = { .proto = proto, .data = data };
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = htons(port),
@@ -145,7 +145,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		return -EBADF;
 	}
 
-	ref.r.s = fd;
+	ref.s = fd;
 
 	if (af == AF_INET) {
 		if (bind_addr)
-- 
@@ -102,7 +102,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
 	    uint32_t data)
 {
-	union epoll_ref ref = { .r.proto = proto, .r.p.data = data };
+	union epoll_ref ref = { .proto = proto, .data = data };
 	struct sockaddr_in addr4 = {
 		.sin_family = AF_INET,
 		.sin_port = htons(port),
@@ -145,7 +145,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		return -EBADF;
 	}
 
-	ref.r.s = fd;
+	ref.s = fd;
 
 	if (af == AF_INET) {
 		if (bind_addr)
-- 
2.41.0

[PATCH 3/3] Use static assertion to verify that union epoll_ref is the right size

[David Gibson]

union epoll_ref is used to subdivide the 64-bit data field in struct
epoll_event.  Thus it *must* fit within that field or we're likely to get
very subtle and nasty bugs.  C11 introduces the notion of static assertions
which we can use to verify this is the case at compile time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/passt.h b/passt.h
index 8b7235a..edc4841 100644
--- a/passt.h
+++ b/passt.h
@@ -32,6 +32,8 @@ struct tap_l4_msg {
 union epoll_ref;
 
 #include <stdbool.h>
+#include <assert.h>
+#include <sys/epoll.h>
 
 #include "packet.h"
 #include "icmp.h"
@@ -64,6 +66,8 @@ union epoll_ref {
 	};
 	uint64_t u64;
 };
+static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data),
+	      "epoll_ref must have same size as epoll_data");
 
 #define TAP_BUF_BYTES							\
 	ROUND_DOWN(((ETH_MAX_MTU + sizeof(uint32_t)) * 128), PAGE_SIZE)
-- 
@@ -32,6 +32,8 @@ struct tap_l4_msg {
 union epoll_ref;
 
 #include <stdbool.h>
+#include <assert.h>
+#include <sys/epoll.h>
 
 #include "packet.h"
 #include "icmp.h"
@@ -64,6 +66,8 @@ union epoll_ref {
 	};
 	uint64_t u64;
 };
+static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data),
+	      "epoll_ref must have same size as epoll_data");
 
 #define TAP_BUF_BYTES							\
 	ROUND_DOWN(((ETH_MAX_MTU + sizeof(uint32_t)) * 128), PAGE_SIZE)
-- 
2.41.0

Re: [PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[Stefano Brivio]

On Tue,  1 Aug 2023 13:36:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> As discussed on our recent calls, the C11 standard introduces
> anonymous structure and union members and static assertions, amongst
> other things.  Both of these could be useful in a few places in
> passt/pasta to make the code more readable and safer.
> 
> However, at the moment, the compiler flags we use only allow C99 code.
> This series allows C11 code, and makes some fairly obvious cleanups by
> using it.
> 
> It would be nice to get an opinion on this reasonably quickly, because
> I have other patches in the works that will look different depending
> on whether or not they can use C11 features.

...then let me start with this one, as it's straightforward: I think
anonymous unions and structures are great. :)

The series (especially 2/3) looks good to me, I'll push it in a bit.

We also need to check for issues with reasonably older gcc (perhaps
those we have in the test/distro tests, at least) and clang versions,
unless you already did that.

-- 
Stefano

Re: [PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[David Gibson]

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

On Tue, Aug 01, 2023 at 10:15:27AM +0200, Stefano Brivio wrote:
> On Tue,  1 Aug 2023 13:36:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > As discussed on our recent calls, the C11 standard introduces
> > anonymous structure and union members and static assertions, amongst
> > other things.  Both of these could be useful in a few places in
> > passt/pasta to make the code more readable and safer.
> > 
> > However, at the moment, the compiler flags we use only allow C99 code.
> > This series allows C11 code, and makes some fairly obvious cleanups by
> > using it.
> > 
> > It would be nice to get an opinion on this reasonably quickly, because
> > I have other patches in the works that will look different depending
> > on whether or not they can use C11 features.
> 
> ...then let me start with this one, as it's straightforward: I think
> anonymous unions and structures are great. :)
> 
> The series (especially 2/3) looks good to me, I'll push it in a bit.
> 
> We also need to check for issues with reasonably older gcc (perhaps
> those we have in the test/distro tests, at least) and clang versions,
> unless you already did that.

So I tried building with this series in some container images of old
debian versions.

In buster (Debian 10), it builds ok with both gcc 8sh and clang 7ish,
although the latter unsurprisingly gives warnings for the gcc specific
__attribute__((optimize("-fno-strict-aliasing"))) we have on the
siphash functions.

In stretch (Debian 9), it builds ok with gcc 6ish, though not with
clang-3.8ish.  However the latter doesn't appear to be because of the
C11 changes - it's complaining about initializers which only list some
of the structure fields.

In jessie (Debian 8) it doesn't build with gcc 4ish or clang-3.5ish,
but again it appears to be the incomplete initializers rather than the
C11 changes.


In short, it appears that before we hit compilers that won't cope with
the C11 changes, we hit compilers that object to incomplete
initializers that we're already using.  So.. I don't think there's any
reason not to apply the C11 changes.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[Stefano Brivio]

On Wed, 2 Aug 2023 14:47:07 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Aug 01, 2023 at 10:15:27AM +0200, Stefano Brivio wrote:
> > On Tue,  1 Aug 2023 13:36:44 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > As discussed on our recent calls, the C11 standard introduces
> > > anonymous structure and union members and static assertions, amongst
> > > other things.  Both of these could be useful in a few places in
> > > passt/pasta to make the code more readable and safer.
> > > 
> > > However, at the moment, the compiler flags we use only allow C99 code.
> > > This series allows C11 code, and makes some fairly obvious cleanups by
> > > using it.
> > > 
> > > It would be nice to get an opinion on this reasonably quickly, because
> > > I have other patches in the works that will look different depending
> > > on whether or not they can use C11 features.  
> > 
> > ...then let me start with this one, as it's straightforward: I think
> > anonymous unions and structures are great. :)
> > 
> > The series (especially 2/3) looks good to me, I'll push it in a bit.
> > 
> > We also need to check for issues with reasonably older gcc (perhaps
> > those we have in the test/distro tests, at least) and clang versions,
> > unless you already did that.  
> 
> So I tried building with this series in some container images of old
> debian versions.
> 
> In buster (Debian 10), it builds ok with both gcc 8sh and clang 7ish,
> although the latter unsurprisingly gives warnings for the gcc specific
> __attribute__((optimize("-fno-strict-aliasing"))) we have on the
> siphash functions.
> 
> In stretch (Debian 9), it builds ok with gcc 6ish, though not with
> clang-3.8ish.  However the latter doesn't appear to be because of the
> C11 changes - it's complaining about initializers which only list some
> of the structure fields.
> 
> In jessie (Debian 8) it doesn't build with gcc 4ish or clang-3.5ish,
> but again it appears to be the incomplete initializers rather than the
> C11 changes.

Oops.

> In short, it appears that before we hit compilers that won't cope with
> the C11 changes, we hit compilers that object to incomplete
> initializers that we're already using.  So.. I don't think there's any
> reason not to apply the C11 changes.

Thanks for checking. And yes, all the "new" features used in this series
should be supported starting from gcc 4.7 (March 2012) anyway, which
seems to be a reasonable target. I'll go ahead and apply this.

-- 
Stefano

Re: [PATCH 0/3] RFC: Allow C11 extensions in the passt/pasta code

[Stefano Brivio]

On Tue,  1 Aug 2023 13:36:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> As discussed on our recent calls, the C11 standard introduces
> anonymous structure and union members and static assertions, amongst
> other things.  Both of these could be useful in a few places in
> passt/pasta to make the code more readable and safer.
> 
> However, at the moment, the compiler flags we use only allow C99 code.
> This series allows C11 code, and makes some fairly obvious cleanups by
> using it.
> 
> It would be nice to get an opinion on this reasonably quickly, because
> I have other patches in the works that will look different depending
> on whether or not they can use C11 features.
> 
> David Gibson (3):
>   Allow C11 code, not just C99 code
>   Use C11 anonymous members to make poll refs less verbose to use
>   Use static assertion to verify that union epoll_ref is the right size

Applied.

-- 
Stefano