[PATCH 0/4] Assorted minor socket creation cleanups

[PATCH 0/4] Assorted minor socket creation cleanups

[David Gibson]

As discussed on our recent call, I was looking again at bug 167.  I
discovered it's still fairly fiddly to address this, but while
investigating spotted a number of cleanups to make in the vicinity.  I
think they make sense even without fixing bug 167 (yet), so here they
are.

David Gibson (4):
  flow: Correct misleading signature of flowside_sock_l4()
  Makefile: Remove unused DUAL_STACK_SOCKETS define
  fwd, pif: Remove duplicated logic between tcp_listen() and
    udp_listen()
  pif, util: Move listen(2) call from sock_l4_() to pif_listen()

 Makefile |  5 -----
 flow.c   | 11 +++++------
 fwd.c    |  8 +-------
 pif.c    | 45 ++++++++++++++++++++++++++++++++++++---------
 pif.h    |  2 +-
 tcp.c    | 38 --------------------------------------
 tcp.h    |  2 --
 udp.c    | 38 --------------------------------------
 udp.h    |  2 --
 util.c   |  7 -------
 10 files changed, 43 insertions(+), 115 deletions(-)

-- 
2.54.0

[PATCH 1/4] flow: Correct misleading signature of flowside_sock_l4()

[David Gibson]

flowside_sock_l4() takes a @tgt parameter with the side to create a socket
for.  The name is misleading, however, although ICMP only uses it for the
target side of the flow, UDP can use it for either or both sides.  TCP
doesn't use it at all.  Rename it to @side.

While we're there remove the stale comment for the @data paremeter which
was removed in 05972c7c4daf.

Fixes: 05972c7c4daf ("util: Move epoll registration out of sock_l4_sa()")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/flow.c b/flow.c
index 565ed2b2..7a5c81c2 100644
--- a/flow.c
+++ b/flow.c
@@ -207,25 +207,24 @@ static int flowside_sock_splice(void *arg)
  * @c:		Execution context
  * @type:	Socket epoll type
  * @pif:	Interface for this socket
- * @tgt:	Target flowside
- * @data:	epoll reference portion for protocol handlers
+ * @side:	Flowside to create a socket for
  *
- * Return: socket fd of protocol @proto bound to our address and port from @tgt
+ * Return: socket fd of protocol @proto bound to our address and port from @side
  *         (if specified).
  */
 int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
-		     const struct flowside *tgt)
+		     const struct flowside *side)
 {
 	const char *ifname = NULL;
 	union sockaddr_inany sa;
 
 	assert(pif_is_socket(pif));
 
-	pif_sockaddr(c, &sa, pif, &tgt->oaddr, tgt->oport);
+	pif_sockaddr(c, &sa, pif, &side->oaddr, side->oport);
 
 	switch (pif) {
 	case PIF_HOST:
-		if (inany_is_loopback(&tgt->oaddr))
+		if (inany_is_loopback(&side->oaddr))
 			ifname = NULL;
 		else if (sa.sa_family == AF_INET)
 			ifname = c->ip4.ifname_out;
-- 
2.54.0

[PATCH 2/4] Makefile: Remove unused DUAL_STACK_SOCKETS define

[David Gibson]

We have a make variable DUAL_STACK_SOCKETS which used to determine whether
we'd use dual stack sockets.  When we introduced it we were concerned that
we might have future ports to systems which did not support them.

We've since discovered that the dual stack interface is described in
RFC 3493, and supported by both Windows and BSD.  Platforms sometimes
differ in the default setting for the IPV6_V6ONLY socket option, but the
feature itself is widely supported.  So, since b8d4fac6a2e7 ("util, pif:
Replace sock_l4() with pif_sock_l4()") we've simply assumed its presence.

The makefile still defines the now unused variable, though.  Remove it.

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

diff --git a/Makefile b/Makefile
index 0a0a60b0..a8c09815 100644
--- a/Makefile
+++ b/Makefile
@@ -11,10 +11,6 @@
 
 VERSION ?= $(shell git describe --tags HEAD 2>/dev/null || echo "unknown\ version")
 
-# Does the target platform allow IPv4 connections to be handled via
-# the IPv6 socket API? (Linux does)
-DUAL_STACK_SOCKETS := 1
-
 TARGET ?= $(shell $(CC) -dumpmachine)
 $(if $(TARGET),,$(error Failed to get target architecture))
 # Get 'uname -m'-like architecture description for target
@@ -33,7 +29,6 @@ endif
 BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE $(FORTIFY_FLAG)
 BASE_CPPFLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
 BASE_CPPFLAGS += -DVERSION=\"$(VERSION)\"
-BASE_CPPFLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
 
 BASE_CFLAGS := -std=c11 -pie -fPIE -O2
 BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security
-- 
2.54.0

[PATCH 3/4] fwd, pif: Remove duplicated logic between tcp_listen() and udp_listen()

[David Gibson]

tcp_listen() and udp_listen() have only some simple logic around a call to
pif_listen(), and it's basically identical in each case.  If we move the
common logic into pif_listen() we can then remove {tcp,udp}_listen()
entirely, with their caller in fwd_sync_one() calling pif_listen()
directly.

We also move the logic converting from a protocol id to an epoll type from
fwd_sync_one() into pif_listen().  It's a bit arbitrary, but seems slightly
nicer that way.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c |  8 +-------
 pif.c | 31 ++++++++++++++++++++++++++-----
 pif.h |  2 +-
 tcp.c | 38 --------------------------------------
 tcp.h |  2 --
 udp.c | 38 --------------------------------------
 udp.h |  2 --
 7 files changed, 28 insertions(+), 93 deletions(-)

diff --git a/fwd.c b/fwd.c
index c0a6adac..042158cf 100644
--- a/fwd.c
+++ b/fwd.c
@@ -389,13 +389,7 @@ static int fwd_sync_one(const struct ctx *c, uint8_t pif, unsigned idx,
 			continue;
 		}
 
-		if (rule->proto == IPPROTO_TCP)
-			fd = tcp_listen(c, pif, idx, addr, ifname, port);
-		else if (rule->proto == IPPROTO_UDP)
-			fd = udp_listen(c, pif, idx, addr, ifname, port);
-		else
-			assert(0);
-
+		fd = pif_listen(c, rule->proto, pif, addr, ifname, port, idx);
 		if (fd < 0) {
 			char astr[INANY_ADDRSTRLEN];
 
diff --git a/pif.c b/pif.c
index d5e31613..ede2f903 100644
--- a/pif.c
+++ b/pif.c
@@ -64,7 +64,7 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
 
 /** pif_listen() - Open a listening socket on a specified pif
  * @c:		Execution context
- * @type:	Socket epoll type
+ * @proto:	Socket protocol (IPPROTO_TCP or IPPROTO_UDP)
  * @pif:	Interface for this socket
  * @addr:	Address to bind to, or NULL for dual-stack any
  * @ifname:	Interface for binding, NULL for any
@@ -76,7 +76,7 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
  *
  * Return: newly created socket, negative error code on failure
  */
-int pif_listen(const struct ctx *c, enum epoll_type type, uint8_t pif,
+int pif_listen(const struct ctx *c, uint8_t proto, uint8_t pif,
 	       const union inany_addr *addr, const char *ifname,
 	       in_port_t port, unsigned rule)
 {
@@ -85,19 +85,40 @@ int pif_listen(const struct ctx *c, enum epoll_type type, uint8_t pif,
 
 	assert(pif_is_socket(pif));
 
+	if (!c->ifi4) {
+		if (!addr)
+			/* Restrict to v6 only */
+			addr = &inany_any6;
+		else if (inany_v4(addr))
+			return -EAFNOSUPPORT;
+	}
+	if (!c->ifi6) {
+		if (!addr)
+			/* Restrict to v4 only */
+			addr = &inany_any4;
+		else if (!inany_v4(addr))
+			return -EAFNOSUPPORT;
+	}
+
+	if (proto == IPPROTO_TCP)
+		ref.type = EPOLL_TYPE_TCP_LISTEN;
+	else if (proto == IPPROTO_UDP)
+		ref.type = EPOLL_TYPE_UDP_LISTEN;
+	else
+		return -EPROTONOSUPPORT;
+
 	if (!addr) {
-		ref.fd = sock_l4_dualstack_any(c, type, port, ifname);
+		ref.fd = sock_l4_dualstack_any(c, ref.type, port, ifname);
 	} else {
 		union sockaddr_inany sa;
 
 		pif_sockaddr(c, &sa, pif, addr, port);
-		ref.fd = sock_l4(c, type, &sa, ifname);
+		ref.fd = sock_l4(c, ref.type, &sa, ifname);
 	}
 
 	if (ref.fd < 0)
 		return ref.fd;
 
-	ref.type = type;
 	ref.listen.port = port;
 	ref.listen.pif = pif;
 	ref.listen.rule = rule;
diff --git a/pif.h b/pif.h
index 62223d12..3a1e2e57 100644
--- a/pif.h
+++ b/pif.h
@@ -66,7 +66,7 @@ static inline bool pif_is_socket(uint8_t pif)
 
 void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
 		  uint8_t pif, const union inany_addr *addr, in_port_t port);
-int pif_listen(const struct ctx *c, enum epoll_type type, uint8_t pif,
+int pif_listen(const struct ctx *c, uint8_t proto, uint8_t pif,
 	       const union inany_addr *addr, const char *ifname,
 	       in_port_t port, unsigned rule);
 
diff --git a/tcp.c b/tcp.c
index c4000754..36add423 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2739,44 +2739,6 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
 	}
 }
 
-/**
- * tcp_listen() - Create listening socket
- * @c:		Execution context
- * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
- * @rule:	Index of relevant forwarding rule
- * @addr:	Pointer to address for binding, NULL for any
- * @ifname:	Name of interface to bind to, NULL for any
- * @port:	Port, host order
- *
- * Return: socket fd on success, negative error code on failure
- */
-int tcp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
-	       const union inany_addr *addr, const char *ifname, in_port_t port)
-{
-	int s;
-
-	assert(!c->no_tcp);
-
-	if (!c->ifi4) {
-		if (!addr)
-			/* Restrict to v6 only */
-			addr = &inany_any6;
-		else if (inany_v4(addr))
-			return -EAFNOSUPPORT;
-	}
-	if (!c->ifi6) {
-		if (!addr)
-			/* Restrict to v4 only */
-			addr = &inany_any4;
-		else if (!inany_v4(addr))
-			return -EAFNOSUPPORT;
-	}
-
-	s = pif_listen(c, EPOLL_TYPE_TCP_LISTEN, pif, addr, ifname, port, rule);
-
-	return s;
-}
-
 /**
  * tcp_sock_refill_pool() - Refill one pool of pre-opened sockets
  * @pool:	Pool of sockets to refill
diff --git a/tcp.h b/tcp.h
index 3262a807..57deef57 100644
--- a/tcp.h
+++ b/tcp.h
@@ -26,8 +26,6 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
 int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 		    const void *saddr, const void *daddr, uint32_t flow_lbl,
 		    const struct pool *p, int idx, const struct timespec *now);
-int tcp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
-	       const union inany_addr *addr, const char *ifname, in_port_t port);
 int tcp_init(struct ctx *c);
 void tcp_defer_handler(struct ctx *c, const struct timespec *now);
 
diff --git a/udp.c b/udp.c
index c28d6ee2..375befa7 100644
--- a/udp.c
+++ b/udp.c
@@ -1124,44 +1124,6 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 	return count;
 }
 
-/**
- * udp_listen() - Initialise listening socket for a given port
- * @c:		Execution context
- * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
- * @rule:	Index of relevant forwarding rule
- * @addr:	Pointer to address for binding, NULL if not configured
- * @ifname:	Name of interface to bind to, NULL if not configured
- * @port:	Port, host order
- *
- * Return: socket fd on success, negative error code on failure
- */
-int udp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
-	       const union inany_addr *addr, const char *ifname, in_port_t port)
-{
-	int s;
-
-	assert(!c->no_udp);
-
-	if (!c->ifi4) {
-		if (!addr)
-			/* Restrict to v6 only */
-			addr = &inany_any6;
-		else if (inany_v4(addr))
-			return -EAFNOSUPPORT;
-	}
-	if (!c->ifi6) {
-		if (!addr)
-			/* Restrict to v4 only */
-			addr = &inany_any4;
-		else if (!inany_v4(addr))
-			return -EAFNOSUPPORT;
-	}
-
-	s = pif_listen(c, EPOLL_TYPE_UDP_LISTEN, pif, addr, ifname, port, rule);
-
-	return s;
-}
-
 /**
  * udp_splice_iov_init() - Set up buffers and descriptors for recvmmsg/sendmmsg
  */
diff --git a/udp.h b/udp.h
index a75d5aea..9ea7de68 100644
--- a/udp.h
+++ b/udp.h
@@ -19,8 +19,6 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 		    sa_family_t af, const void *saddr, const void *daddr,
 		    uint8_t ttl, const struct pool *p, int idx,
 		    const struct timespec *now);
-int udp_listen(const struct ctx *c, uint8_t pif, unsigned rule,
-	       const union inany_addr *addr, const char *ifname, in_port_t port);
 int udp_init(struct ctx *c);
 void udp_update_l2_buf(const unsigned char *eth_d);
 
-- 
2.54.0

[PATCH 4/4] pif, util: Move listen(2) call from sock_l4_() to pif_listen()

[David Gibson]

It's a bit odd to have the listen(2) call for TCP listening sockets, down
deep in sock_l4_() conditional upon the epoll type passed in.  Move it to
pif_listen(), which is at least about listening, although it does still
need to be conditional on TCP.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 pif.c  | 14 ++++++++++----
 util.c |  7 -------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/pif.c b/pif.c
index ede2f903..f99f33e5 100644
--- a/pif.c
+++ b/pif.c
@@ -123,11 +123,17 @@ int pif_listen(const struct ctx *c, uint8_t proto, uint8_t pif,
 	ref.listen.pif = pif;
 	ref.listen.rule = rule;
 
-	ret = epoll_add(c->epollfd, EPOLLIN, ref);
-	if (ret < 0) {
-		close(ref.fd);
-		return ret;
+	if (proto == IPPROTO_TCP && listen(ref.fd, 128) < 0) {
+		ret = -errno;
+		goto fail;
 	}
 
+	ret = epoll_add(c->epollfd, EPOLLIN, ref);
+	if (ret < 0)
+		goto fail;
+
 	return ref.fd;
+fail:
+	close(ref.fd);
+	return ret;
 }
diff --git a/util.c b/util.c
index b64c29ed..e22696e3 100644
--- a/util.c
+++ b/util.c
@@ -169,13 +169,6 @@ static int sock_l4_(const struct ctx *c, enum epoll_type type,
 		}
 	}
 
-	if (type == EPOLL_TYPE_TCP_LISTEN && listen(fd, 128) < 0) {
-		ret = -errno;
-		warn("TCP socket listen: %s", strerror_(-ret));
-		close(fd);
-		return ret;
-	}
-
 	return fd;
 }
 
-- 
2.54.0