[PATCH v2 12/12] icmp: Dedicated functions for starting and closing ping sequences

[David Gibson <david@gibson.dropbear.id.au>]

ICMP sockets are cleaned up on a timeout implemented in icmp_timer_one(),
and the logic to do that cleanup is open coded in that function.  Similarly
new sockets are opened when we discover we don't have an existing one in
icmp_tap_handler(), and again the logic is open-coded.

That's not the worst thing, but it's a bit cleaner to have dedicated
functions for the creation and destruction of ping sockets.  This will also
make things a bit easier for future changes we have in mind.

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

diff --git a/icmp.c b/icmp.c
index 129a7f1..d669351 100644
--- a/icmp.c
+++ b/icmp.c
@@ -132,6 +132,70 @@ unexpected:
 	warn("%s: Unexpected packet on ping socket", pname);
 }
 
+/**
+ * icmp_ping_close() - Close out and cleanup a ping sequence
+ * @c:		Execution context
+ * @id_map:	id map entry of the sequence to close
+ */
+static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_map)
+{
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
+	close(id_map->sock);
+	id_map->sock = -1;
+	id_map->seq = -1;
+}
+
+/**
+ * icmp_ping_new() - Prepare a new ping socket for a new id
+ * @c:		Execution context
+ * @id_map:	id map entry of the sequence to open
+ * @af:		Address family, AF_INET or AF_INET6
+ * @id:		ICMP id for the new sequence
+ *
+ * Return: Newly opened ping socket fd, or -1 on failure
+ */
+static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *id_map,
+			 int af, uint16_t id)
+{
+	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
+	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
+	union icmp_epoll_ref iref = { .id = id };
+	const void *bind_addr;
+	const char *bind_if;
+	int s;
+
+	if (af == AF_INET) {
+		bind_addr = &c->ip4.addr_out;
+		bind_if = c->ip4.ifname_out;
+	} else {
+		bind_addr = &c->ip6.addr_out;
+		bind_if = c->ip6.ifname_out;
+	}
+
+	s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
+
+	if (s < 0) {
+		warn("Cannot open \"ping\" socket. You might need to:");
+		warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
+		warn("...echo requests/replies will fail.");
+		goto cancel;
+	}
+
+	if (s > FD_REF_MAX)
+		goto cancel;
+
+	id_map->sock = s;
+
+	debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
+
+	return s;
+
+cancel:
+	if (s >= 0)
+		close(s);
+	return -1;
+}
+
 /**
  * icmp_tap_handler() - Handle packets from tap
  * @c:		Execution context
@@ -148,7 +212,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		     const void *saddr, const void *daddr,
 		     const struct pool *p, const struct timespec *now)
 {
-	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
 	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	union {
 		struct sockaddr sa;
@@ -200,37 +263,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		ASSERT(0);
 	}
 
-	if ((s = id_map->sock) < 0) {
-		union icmp_epoll_ref iref = { .id = id };
-		const void *bind_addr;
-		const char *bind_if;
-
-		if (af == AF_INET) {
-			bind_addr = &c->ip4.addr_out;
-			bind_if = c->ip4.ifname_out;
-		} else {
-			bind_addr = &c->ip6.addr_out;
-			bind_if = c->ip6.ifname_out;
-		}
-
-		s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
-
-		if (s < 0) {
-			warn("Cannot open \"ping\" socket. You might need to:");
-			warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
-			warn("...echo requests/replies will fail.");
-			return 1;
-		}
-
-		if (s > FD_REF_MAX) {
-			close(s);
+	if ((s = id_map->sock) < 0)
+		if ((s = icmp_ping_new(c, id_map, af, id)) < 0)
 			return 1;
-		}
-
-		id_map->sock = s;
-
-		debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
-	}
 
 	id_map->ts = now->tv_sec;
 
@@ -257,10 +292,7 @@ static void icmp_timer_one(const struct ctx *c, struct icmp_id_sock *id_map,
 	if (id_map->sock < 0 || now->tv_sec - id_map->ts <= ICMP_ECHO_TIMEOUT)
 		return;
 
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
-	close(id_map->sock);
-	id_map->sock = -1;
-	id_map->seq = -1;
+	icmp_ping_close(c, id_map);
 }
 
 /**
-- 
@@ -132,6 +132,70 @@ unexpected:
 	warn("%s: Unexpected packet on ping socket", pname);
 }
 
+/**
+ * icmp_ping_close() - Close out and cleanup a ping sequence
+ * @c:		Execution context
+ * @id_map:	id map entry of the sequence to close
+ */
+static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_map)
+{
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
+	close(id_map->sock);
+	id_map->sock = -1;
+	id_map->seq = -1;
+}
+
+/**
+ * icmp_ping_new() - Prepare a new ping socket for a new id
+ * @c:		Execution context
+ * @id_map:	id map entry of the sequence to open
+ * @af:		Address family, AF_INET or AF_INET6
+ * @id:		ICMP id for the new sequence
+ *
+ * Return: Newly opened ping socket fd, or -1 on failure
+ */
+static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *id_map,
+			 int af, uint16_t id)
+{
+	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
+	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
+	union icmp_epoll_ref iref = { .id = id };
+	const void *bind_addr;
+	const char *bind_if;
+	int s;
+
+	if (af == AF_INET) {
+		bind_addr = &c->ip4.addr_out;
+		bind_if = c->ip4.ifname_out;
+	} else {
+		bind_addr = &c->ip6.addr_out;
+		bind_if = c->ip6.ifname_out;
+	}
+
+	s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
+
+	if (s < 0) {
+		warn("Cannot open \"ping\" socket. You might need to:");
+		warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
+		warn("...echo requests/replies will fail.");
+		goto cancel;
+	}
+
+	if (s > FD_REF_MAX)
+		goto cancel;
+
+	id_map->sock = s;
+
+	debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
+
+	return s;
+
+cancel:
+	if (s >= 0)
+		close(s);
+	return -1;
+}
+
 /**
  * icmp_tap_handler() - Handle packets from tap
  * @c:		Execution context
@@ -148,7 +212,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		     const void *saddr, const void *daddr,
 		     const struct pool *p, const struct timespec *now)
 {
-	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
 	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	union {
 		struct sockaddr sa;
@@ -200,37 +263,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		ASSERT(0);
 	}
 
-	if ((s = id_map->sock) < 0) {
-		union icmp_epoll_ref iref = { .id = id };
-		const void *bind_addr;
-		const char *bind_if;
-
-		if (af == AF_INET) {
-			bind_addr = &c->ip4.addr_out;
-			bind_if = c->ip4.ifname_out;
-		} else {
-			bind_addr = &c->ip6.addr_out;
-			bind_if = c->ip6.ifname_out;
-		}
-
-		s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
-
-		if (s < 0) {
-			warn("Cannot open \"ping\" socket. You might need to:");
-			warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
-			warn("...echo requests/replies will fail.");
-			return 1;
-		}
-
-		if (s > FD_REF_MAX) {
-			close(s);
+	if ((s = id_map->sock) < 0)
+		if ((s = icmp_ping_new(c, id_map, af, id)) < 0)
 			return 1;
-		}
-
-		id_map->sock = s;
-
-		debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
-	}
 
 	id_map->ts = now->tv_sec;
 
@@ -257,10 +292,7 @@ static void icmp_timer_one(const struct ctx *c, struct icmp_id_sock *id_map,
 	if (id_map->sock < 0 || now->tv_sec - id_map->ts <= ICMP_ECHO_TIMEOUT)
 		return;
 
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
-	close(id_map->sock);
-	id_map->sock = -1;
-	id_map->seq = -1;
+	icmp_ping_close(c, id_map);
 }
 
 /**
-- 
2.43.0