[PATCH 02/17] netlink: Split nl_addr() into separate operation functions

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

nl_addr() can perform three quite different operations based on the 'op'
parameter, each of which uses a different subset of the parameters.  Split
them up into a function for each operation.  This does use more lines of
code, but the overlap wasn't that great, and the separated logic is much
easier to follow.

It's also clearer in the callers what we expect the netlink operations to
do, and what information it uses.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c    |  12 ++-
 netlink.c | 232 ++++++++++++++++++++++++++++++++----------------------
 netlink.h |   6 +-
 pasta.c   |  17 ++--
 4 files changed, 159 insertions(+), 108 deletions(-)

diff --git a/conf.c b/conf.c
index 2ff9e2a..2057028 100644
--- a/conf.c
+++ b/conf.c
@@ -650,10 +650,8 @@ static unsigned int conf_ip4(unsigned int ifi,
 	if (IN4_IS_ADDR_UNSPECIFIED(&ip4->gw))
 		nl_route(NL_GET, ifi, 0, AF_INET, &ip4->gw);
 
-	if (IN4_IS_ADDR_UNSPECIFIED(&ip4->addr)) {
-		nl_addr(NL_GET, ifi, 0, AF_INET,
-			&ip4->addr, &ip4->prefix_len, NULL);
-	}
+	if (IN4_IS_ADDR_UNSPECIFIED(&ip4->addr))
+		nl_addr_get(ifi, AF_INET, &ip4->addr, &ip4->prefix_len, NULL);
 
 	if (!ip4->prefix_len) {
 		in_addr_t addr = ntohl(ip4->addr.s_addr);
@@ -703,9 +701,9 @@ static unsigned int conf_ip6(unsigned int ifi,
 	if (IN6_IS_ADDR_UNSPECIFIED(&ip6->gw))
 		nl_route(NL_GET, ifi, 0, AF_INET6, &ip6->gw);
 
-	nl_addr(NL_GET, ifi, 0, AF_INET6,
-		IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) ? &ip6->addr : NULL,
-		&prefix_len, &ip6->addr_ll);
+	nl_addr_get(ifi, AF_INET6,
+		    IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) ? &ip6->addr : NULL,
+		    &prefix_len, &ip6->addr_ll);
 
 	memcpy(&ip6->addr_seen, &ip6->addr, sizeof(ip6->addr));
 	memcpy(&ip6->addr_ll_seen, &ip6->addr_ll, sizeof(ip6->addr_ll));
diff --git a/netlink.c b/netlink.c
index 4b1f75e..269d738 100644
--- a/netlink.c
+++ b/netlink.c
@@ -334,17 +334,76 @@ next:
 }
 
 /**
- * nl_addr() - Get/set/copy IP addresses for given interface and address family
- * @op:		Requested operation
+ * nl_addr_get() - Get IP address for given interface and address family
  * @ifi:	Interface index in outer network namespace
- * @ifi_ns:	Interface index in target namespace for NL_SET, NL_DUP
  * @af:		Address family
- * @addr:	Global address to fill on NL_GET, to set on NL_SET
- * @prefix_len:	Mask or prefix length, set or fetched (for IPv4)
- * @addr_l:	Link-scoped address to fill on NL_GET
+ * @addr:	Global address to fill
+ * @prefix_len:	Mask or prefix length, to fill (for IPv4)
+ * @addr_l:	Link-scoped address to fill (for IPv6)
+ */
+void nl_addr_get(unsigned int ifi, sa_family_t af, void *addr,
+		 int *prefix_len, void *addr_l)
+{
+	struct req_t {
+		struct nlmsghdr nlh;
+		struct ifaddrmsg ifa;
+	} req = {
+		.nlh.nlmsg_type    = RTM_GETADDR,
+		.nlh.nlmsg_flags   = NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP,
+		.nlh.nlmsg_len     = sizeof(req),
+		.nlh.nlmsg_seq     = nl_seq++,
+
+		.ifa.ifa_family    = af,
+		.ifa.ifa_index     = ifi,
+	};
+	struct nlmsghdr *nh;
+	char buf[NLBUFSIZ];
+	ssize_t n;
+
+	if ((n = nl_req(0, buf, &req, req.nlh.nlmsg_len)) < 0)
+		return;
+
+	for (nh = (struct nlmsghdr *)buf;
+	     NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
+	     nh = NLMSG_NEXT(nh, n)) {
+		struct ifaddrmsg *ifa = (struct ifaddrmsg *)NLMSG_DATA(nh);
+		struct rtattr *rta;
+		size_t na;
+
+		if (nh->nlmsg_type != RTM_NEWADDR)
+			continue;
+
+		if (ifa->ifa_index != ifi)
+			continue;
+
+		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
+		     rta = RTA_NEXT(rta, na)) {
+			if (rta->rta_type != IFA_ADDRESS)
+				continue;
+
+			if (af == AF_INET) {
+				memcpy(addr, RTA_DATA(rta), RTA_PAYLOAD(rta));
+				*prefix_len = ifa->ifa_prefixlen;
+			} else if (af == AF_INET6 && addr &&
+				   ifa->ifa_scope == RT_SCOPE_UNIVERSE) {
+				memcpy(addr, RTA_DATA(rta), RTA_PAYLOAD(rta));
+			}
+
+			if (addr_l &&
+			    af == AF_INET6 && ifa->ifa_scope == RT_SCOPE_LINK)
+				memcpy(addr_l, RTA_DATA(rta), RTA_PAYLOAD(rta));
+		}
+	}
+}
+
+/**
+ * nl_add_set() - Set IP addresses for given interface and address family
+ * @ifi:	Interface index
+ * @af:		Address family
+ * @addr:	Global address to set
+ * @prefix_len:	Mask or prefix length to set
  */
-void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
-	     sa_family_t af, void *addr, int *prefix_len, void *addr_l)
+void nl_addr_set(unsigned int ifi, sa_family_t af, void *addr, int prefix_len)
 {
 	struct req_t {
 		struct nlmsghdr nlh;
@@ -364,125 +423,112 @@ void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 			} a6;
 		} set;
 	} req = {
-		.nlh.nlmsg_type    = op == NL_SET ? RTM_NEWADDR : RTM_GETADDR,
-		.nlh.nlmsg_flags   = NLM_F_REQUEST,
+		.nlh.nlmsg_type    = RTM_NEWADDR,
+		.nlh.nlmsg_flags   = NLM_F_REQUEST | NLM_F_ACK |
+				     NLM_F_CREATE | NLM_F_EXCL,
 		.nlh.nlmsg_len     = NLMSG_LENGTH(sizeof(struct ifaddrmsg)),
 		.nlh.nlmsg_seq     = nl_seq++,
 
 		.ifa.ifa_family    = af,
-		.ifa.ifa_index     = op == NL_SET ? ifi_ns : ifi,
-		.ifa.ifa_prefixlen = op == NL_SET ? *prefix_len : 0,
+		.ifa.ifa_index     = ifi,
+		.ifa.ifa_prefixlen = prefix_len,
+		.ifa.ifa_scope	   = RT_SCOPE_UNIVERSE,
 	};
-	ssize_t n, nlmsgs_size;
-	struct ifaddrmsg *ifa;
-	struct nlmsghdr *nh;
-	struct rtattr *rta;
 	char buf[NLBUFSIZ];
-	size_t na;
 
-	if (op == NL_SET) {
-		if (af == AF_INET6) {
-			size_t rta_len = RTA_LENGTH(sizeof(req.set.a6.l));
+	if (af == AF_INET6) {
+		size_t rta_len = RTA_LENGTH(sizeof(req.set.a6.l));
 
-			/* By default, strictly speaking, it's duplicated */
-			req.ifa.ifa_flags = IFA_F_NODAD;
+		/* By default, strictly speaking, it's duplicated */
+		req.ifa.ifa_flags = IFA_F_NODAD;
 
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.a6)
-				+ sizeof(req.set.a6);
+		req.nlh.nlmsg_len = offsetof(struct req_t, set.a6)
+			+ sizeof(req.set.a6);
 
-			memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l));
-			req.set.a6.rta_l.rta_len = rta_len;
-			req.set.a4.rta_l.rta_type = IFA_LOCAL;
-			memcpy(&req.set.a6.a, addr, sizeof(req.set.a6.a));
-			req.set.a6.rta_a.rta_len = rta_len;
-			req.set.a6.rta_a.rta_type = IFA_ADDRESS;
-		} else {
-			size_t rta_len = RTA_LENGTH(sizeof(req.set.a4.l));
-
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.a4)
-				+ sizeof(req.set.a4);
+		memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l));
+		req.set.a6.rta_l.rta_len = rta_len;
+		req.set.a4.rta_l.rta_type = IFA_LOCAL;
+		memcpy(&req.set.a6.a, addr, sizeof(req.set.a6.a));
+		req.set.a6.rta_a.rta_len = rta_len;
+		req.set.a6.rta_a.rta_type = IFA_ADDRESS;
+	} else {
+		size_t rta_len = RTA_LENGTH(sizeof(req.set.a4.l));
 
-			req.set.a4.l = req.set.a4.a = *(uint32_t *)addr;
-			req.set.a4.rta_l.rta_len = rta_len;
-			req.set.a4.rta_l.rta_type = IFA_LOCAL;
-			req.set.a4.rta_a.rta_len = rta_len;
-			req.set.a4.rta_a.rta_type = IFA_ADDRESS;
-		}
+		req.nlh.nlmsg_len = offsetof(struct req_t, set.a4)
+			+ sizeof(req.set.a4);
 
-		req.ifa.ifa_scope = RT_SCOPE_UNIVERSE;
-		req.nlh.nlmsg_flags |= NLM_F_CREATE | NLM_F_ACK | NLM_F_EXCL;
-	} else {
-		req.nlh.nlmsg_flags |= NLM_F_DUMP;
+		req.set.a4.l = req.set.a4.a = *(uint32_t *)addr;
+		req.set.a4.rta_l.rta_len = rta_len;
+		req.set.a4.rta_l.rta_type = IFA_LOCAL;
+		req.set.a4.rta_a.rta_len = rta_len;
+		req.set.a4.rta_a.rta_type = IFA_ADDRESS;
 	}
 
-	if ((n = nl_req(op == NL_SET, buf, &req, req.nlh.nlmsg_len)) < 0)
-		return;
+	nl_req(1, buf, &req, req.nlh.nlmsg_len);
+}
 
-	if (op == NL_SET)
+/**
+ * nl_addr_dup() - Copy IP addresses for given interface and address family
+ * @ifi:	Interface index in outer network namespace
+ * @ifi_ns:	Interface index in target namespace
+ * @af:		Address family
+ */
+void nl_addr_dup(unsigned int ifi, unsigned int ifi_ns, sa_family_t af)
+{
+	struct req_t {
+		struct nlmsghdr nlh;
+		struct ifaddrmsg ifa;
+	} req = {
+		.nlh.nlmsg_type    = RTM_GETADDR,
+		.nlh.nlmsg_flags   = NLM_F_REQUEST | NLM_F_DUMP,
+		.nlh.nlmsg_len     = sizeof(req),
+		.nlh.nlmsg_seq     = nl_seq++,
+
+		.ifa.ifa_family    = af,
+		.ifa.ifa_index     = ifi,
+		.ifa.ifa_prefixlen = 0,
+	};
+	char buf[NLBUFSIZ], resp[NLBUFSIZ];
+	ssize_t n, nlmsgs_size;
+	struct nlmsghdr *nh;
+
+	if ((n = nl_req(0, buf, &req, sizeof(req))) < 0)
 		return;
 
-	nh = (struct nlmsghdr *)buf;
 	nlmsgs_size = n;
 
-	for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
+	for (nh = (struct nlmsghdr *)buf;
+	     NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
+	     nh = NLMSG_NEXT(nh, n)) {
+		struct ifaddrmsg *ifa;
+		struct rtattr *rta;
+		size_t na;
+
 		if (nh->nlmsg_type != RTM_NEWADDR)
-			goto next;
+			continue;
 
-		if (op == NL_DUP) {
-			nh->nlmsg_seq = nl_seq++;
-			nh->nlmsg_pid = 0;
-			nh->nlmsg_flags &= ~NLM_F_DUMP_FILTERED;
-			nh->nlmsg_flags |= NLM_F_REQUEST | NLM_F_ACK |
-					   NLM_F_CREATE;
-		}
+		nh->nlmsg_seq = nl_seq++;
+		nh->nlmsg_pid = 0;
+		nh->nlmsg_flags &= ~NLM_F_DUMP_FILTERED;
+		nh->nlmsg_flags |= NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE;
 
 		ifa = (struct ifaddrmsg *)NLMSG_DATA(nh);
 
-		if (op == NL_DUP && (ifa->ifa_scope == RT_SCOPE_LINK ||
-				     ifa->ifa_index != ifi)) {
+		if (ifa->ifa_scope == RT_SCOPE_LINK || ifa->ifa_index != ifi) {
 			ifa->ifa_family = AF_UNSPEC;
-			goto next;
+			continue;
 		}
 
-		if (ifa->ifa_index != ifi)
-			goto next;
-
-		if (op == NL_DUP)
-			ifa->ifa_index = ifi_ns;
+		ifa->ifa_index = ifi_ns;
 
 		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
-			if (op == NL_DUP && rta->rta_type == IFA_LABEL)
+			if (rta->rta_type == IFA_LABEL)
 				rta->rta_type = IFA_UNSPEC;
-
-			if (op == NL_DUP || rta->rta_type != IFA_ADDRESS)
-				continue;
-
-			if (af == AF_INET && addr && !*(uint32_t *)addr) {
-				memcpy(addr, RTA_DATA(rta), RTA_PAYLOAD(rta));
-				*prefix_len = ifa->ifa_prefixlen;
-			} else if (af == AF_INET6 && addr &&
-				 ifa->ifa_scope == RT_SCOPE_UNIVERSE &&
-				 IN6_IS_ADDR_UNSPECIFIED(addr)) {
-				memcpy(addr, RTA_DATA(rta), RTA_PAYLOAD(rta));
-			}
-
-			if (addr_l &&
-			    af == AF_INET6 && ifa->ifa_scope == RT_SCOPE_LINK &&
-			    IN6_IS_ADDR_UNSPECIFIED(addr_l))
-				memcpy(addr_l, RTA_DATA(rta), RTA_PAYLOAD(rta));
 		}
-next:
-		if (nh->nlmsg_type == NLMSG_DONE)
-			break;
 	}
 
-	if (op == NL_DUP) {
-		char resp[NLBUFSIZ];
-
-		nh = (struct nlmsghdr *)buf;
-		nl_req(1, resp, nh, nlmsgs_size);
-	}
+	nl_req(1, resp, buf, nlmsgs_size);
 }
 
 /**
diff --git a/netlink.h b/netlink.h
index 980ac44..5ac972d 100644
--- a/netlink.h
+++ b/netlink.h
@@ -16,8 +16,10 @@ void nl_sock_init(const struct ctx *c, bool ns);
 unsigned int nl_get_ext_if(sa_family_t af);
 void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
 	      sa_family_t af, void *gw);
-void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
-	     sa_family_t af, void *addr, int *prefix_len, void *addr_l);
+void nl_addr_get(unsigned int ifi, sa_family_t af, void *addr,
+		 int *prefix_len, void *addr_l);
+void nl_addr_set(unsigned int ifi, sa_family_t af, void *addr, int prefix_len);
+void nl_addr_dup(unsigned int ifi, unsigned int ifi_ns, sa_family_t af);
 void nl_link_get_mac(int ns, unsigned int ifi, void *mac);
 void nl_link_set_mac(int ns, unsigned int ifi, void *mac);
 void nl_link_up(int ns, unsigned int ifi, int mtu);
diff --git a/pasta.c b/pasta.c
index 3b5537d..1a8f09c 100644
--- a/pasta.c
+++ b/pasta.c
@@ -282,21 +282,26 @@ void pasta_ns_conf(struct ctx *c)
 
 	if (c->pasta_conf_ns) {
 		enum nl_op op_routes = c->no_copy_routes ? NL_SET : NL_DUP;
-		enum nl_op op_addrs =  c->no_copy_addrs  ? NL_SET : NL_DUP;
 
 		nl_link_up(1, c->pasta_ifi, c->mtu);
 
 		if (c->ifi4) {
-			nl_addr(op_addrs, c->ifi4, c->pasta_ifi, AF_INET,
-				&c->ip4.addr, &c->ip4.prefix_len, NULL);
+			if (c->no_copy_addrs)
+				nl_addr_set(c->pasta_ifi, AF_INET, 
+					    &c->ip4.addr, c->ip4.prefix_len);
+			else
+				nl_addr_dup(c->ifi4, c->pasta_ifi, AF_INET);
+
 			nl_route(op_routes, c->ifi4, c->pasta_ifi, AF_INET,
 				 &c->ip4.gw);
 		}
 
 		if (c->ifi6) {
-			int prefix_len = 64;
-			nl_addr(op_addrs, c->ifi6, c->pasta_ifi, AF_INET6,
-				&c->ip6.addr, &prefix_len, NULL);
+			if (c->no_copy_addrs)
+				nl_addr_set(c->pasta_ifi, AF_INET6, &c->ip6.addr, 64);
+			else
+				nl_addr_dup(c->ifi4, c->pasta_ifi, AF_INET6);
+
 			nl_route(op_routes, c->ifi6, c->pasta_ifi, AF_INET6,
 				 &c->ip6.gw);
 		}
-- 
@@ -282,21 +282,26 @@ void pasta_ns_conf(struct ctx *c)
 
 	if (c->pasta_conf_ns) {
 		enum nl_op op_routes = c->no_copy_routes ? NL_SET : NL_DUP;
-		enum nl_op op_addrs =  c->no_copy_addrs  ? NL_SET : NL_DUP;
 
 		nl_link_up(1, c->pasta_ifi, c->mtu);
 
 		if (c->ifi4) {
-			nl_addr(op_addrs, c->ifi4, c->pasta_ifi, AF_INET,
-				&c->ip4.addr, &c->ip4.prefix_len, NULL);
+			if (c->no_copy_addrs)
+				nl_addr_set(c->pasta_ifi, AF_INET, 
+					    &c->ip4.addr, c->ip4.prefix_len);
+			else
+				nl_addr_dup(c->ifi4, c->pasta_ifi, AF_INET);
+
 			nl_route(op_routes, c->ifi4, c->pasta_ifi, AF_INET,
 				 &c->ip4.gw);
 		}
 
 		if (c->ifi6) {
-			int prefix_len = 64;
-			nl_addr(op_addrs, c->ifi6, c->pasta_ifi, AF_INET6,
-				&c->ip6.addr, &prefix_len, NULL);
+			if (c->no_copy_addrs)
+				nl_addr_set(c->pasta_ifi, AF_INET6, &c->ip6.addr, 64);
+			else
+				nl_addr_dup(c->ifi4, c->pasta_ifi, AF_INET6);
+
 			nl_route(op_routes, c->ifi6, c->pasta_ifi, AF_INET6,
 				 &c->ip6.gw);
 		}
-- 
2.41.0