From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id EBB105A026D for ; Thu, 3 Aug 2023 00:47:59 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691016478; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Lkgb0Tnrf+iPfeDp++Ei0uGuSNEI+OVhcj8hY2qaLR0=; b=bIKBBI31gRuAm0i1mYqDW1sglK6szGfLvKGqcdY0ODAr6mtcw9CSxoy8poIzrzGfG8Lh+Y aC24EkBs8c7amFqbqQsR9Kbr6i3Yz36r7AAmuzSnQbztfZeUPBXD3ROrJwew+tqOUZUsJr BVBiB8hVNmfQVk+d2eH+rOH8mXInupU= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-441-jPeZgYuPOtuG9Qf8R8cdmA-1; Wed, 02 Aug 2023 18:47:54 -0400 X-MC-Unique: jPeZgYuPOtuG9Qf8R8cdmA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8E805104D513; Wed, 2 Aug 2023 22:47:53 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CD08040D2839; Wed, 2 Aug 2023 22:47:52 +0000 (UTC) Date: Thu, 3 Aug 2023 00:47:50 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 02/17] netlink: Split nl_addr() into separate operation functions Message-ID: <20230803004750.4e2feb37@elisabeth> In-Reply-To: <20230724060936.952659-3-david@gibson.dropbear.id.au> References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-3-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 5AJ44DP2MAFP6BJWYYRFD5AGDWZDTD62 X-Message-ID-Hash: 5AJ44DP2MAFP6BJWYYRFD5AGDWZDTD62 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, 24 Jul 2023 16:09:21 +1000 David Gibson wrote: > 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 > --- > 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); I guess this should be c->ifi6 (also in 17/17). -- Stefano