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.133.124]) by passt.top (Postfix) with ESMTP id 8C5505A026D for ; Thu, 3 Aug 2023 00:47:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691016464; 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=ih8kZl1qLVbn3RCV36ZFMxMSB9mgX0sml76g7Qvh4lQ=; b=iESfWqoBKGLS1FUNPDbY5A7yM3ZB53dy1cSr8+EJnYAwN0fOLNnq3+aIgbPBR/WyPaSBnC ID+98UN1DcRid8y5Ly1wBiQ7Eiq1JbN8OJg8N07zR3CfNR3QEo0ntmsmnNF3Dc1K/8ld60 3Vhc4doOsJHCaoewisEh/0zv9pD+Efc= 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-436-Ht7iKIkAMQqo74QjMskG_g-1; Wed, 02 Aug 2023 18:47:43 -0400 X-MC-Unique: Ht7iKIkAMQqo74QjMskG_g-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C468583DE37; Wed, 2 Aug 2023 22:47:42 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 26CED401DA9; Wed, 2 Aug 2023 22:47:41 +0000 (UTC) Date: Thu, 3 Aug 2023 00:47:40 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 03/17] netlink: Split nl_route() into separate operation functions Message-ID: <20230803004740.0f271388@elisabeth> In-Reply-To: <20230724060936.952659-4-david@gibson.dropbear.id.au> References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-4-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: Q6E3HL35LXWRXWOYHHP2YJACV3QXGTCR X-Message-ID-Hash: Q6E3HL35LXWRXWOYHHP2YJACV3QXGTCR 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:22 +1000 David Gibson wrote: > nl_route() can perform 3 quite different operations based on the 'op' > parameter. Split this into separate functions for each one. This requires > more lines of code, but makes the internal logic of each operation much > easier to follow. > > Signed-off-by: David Gibson > --- > conf.c | 4 +- > netlink.c | 238 ++++++++++++++++++++++++++++++++++-------------------- > netlink.h | 11 +-- > pasta.c | 16 ++-- > 4 files changed, 164 insertions(+), 105 deletions(-) > > diff --git a/conf.c b/conf.c > index 2057028..66958d4 100644 > --- a/conf.c > +++ b/conf.c > @@ -648,7 +648,7 @@ 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); > + nl_route_get_def(ifi, AF_INET, &ip4->gw); > > if (IN4_IS_ADDR_UNSPECIFIED(&ip4->addr)) > nl_addr_get(ifi, AF_INET, &ip4->addr, &ip4->prefix_len, NULL); > @@ -699,7 +699,7 @@ 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_route_get_def(ifi, AF_INET6, &ip6->gw); > > nl_addr_get(ifi, AF_INET6, > IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) ? &ip6->addr : NULL, > diff --git a/netlink.c b/netlink.c > index 269d738..346eb3a 100644 > --- a/netlink.c > +++ b/netlink.c > @@ -185,15 +185,71 @@ unsigned int nl_get_ext_if(sa_family_t af) > } > > /** > - * nl_route() - Get/set/copy routes for given interface and address family > - * @op: Requested operation > - * @ifi: Interface index in outer network namespace > - * @ifi_ns: Interface index in target namespace for NL_SET, NL_DUP > + * nl_route_get_def() - Get default route for given interface and address family > + * @ifi: Interface index > + * @af: Address family > + * @gw: Default gateway to fill on NL_GET > + */ > +void nl_route_get_def(unsigned int ifi, sa_family_t af, void *gw) > +{ > + struct req_t { > + struct nlmsghdr nlh; > + struct rtmsg rtm; > + struct rtattr rta; > + unsigned int ifi; > + } req = { > + .nlh.nlmsg_type = RTM_GETROUTE, > + .nlh.nlmsg_len = sizeof(req), > + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP, > + .nlh.nlmsg_seq = nl_seq++, > + > + .rtm.rtm_family = af, > + .rtm.rtm_table = RT_TABLE_MAIN, > + .rtm.rtm_scope = RT_SCOPE_UNIVERSE, > + .rtm.rtm_type = RTN_UNICAST, > + > + .rta.rta_type = RTA_OIF, > + .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), > + .ifi = 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 rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh); > + struct rtattr *rta; > + size_t na; > + > + if (nh->nlmsg_type != RTM_NEWROUTE) > + continue; > + > + if (rtm->rtm_dst_len) > + continue; > + > + for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na); > + rta = RTA_NEXT(rta, na)) { > + if (rta->rta_type != RTA_GATEWAY) > + continue; > + > + memcpy(gw, RTA_DATA(rta), RTA_PAYLOAD(rta)); > + return; > + } > + } > +} > + > +/** > + * nl_route_set_def() - Set default route for given interface and address family > + * @ifi: Interface index in target namespace > * @af: Address family > - * @gw: Default gateway to fill on NL_GET, to set on NL_SET > + * @gw: Default gateway to set > */ > -void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns, > - sa_family_t af, void *gw) > +void nl_route_set_def(unsigned int ifi, sa_family_t af, void *gw) > { > struct req_t { > struct nlmsghdr nlh; > @@ -215,122 +271,126 @@ void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns, > } r4; > } set; > } req = { > - .nlh.nlmsg_type = op == NL_SET ? RTM_NEWROUTE : RTM_GETROUTE, > - .nlh.nlmsg_flags = NLM_F_REQUEST, > + .nlh.nlmsg_type = RTM_NEWROUTE, > + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | > + NLM_F_CREATE | NLM_F_EXCL, > .nlh.nlmsg_seq = nl_seq++, > > .rtm.rtm_family = af, > .rtm.rtm_table = RT_TABLE_MAIN, > .rtm.rtm_scope = RT_SCOPE_UNIVERSE, > .rtm.rtm_type = RTN_UNICAST, > + .rtm.rtm_protocol = RTPROT_BOOT, > > .rta.rta_type = RTA_OIF, > .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), > - .ifi = op == NL_SET ? ifi_ns : ifi, > + .ifi = ifi, > }; > - unsigned dup_routes = 0; > - ssize_t n, nlmsgs_size; > - struct nlmsghdr *nh; > - struct rtattr *rta; > char buf[NLBUFSIZ]; > - struct rtmsg *rtm; > - size_t na; > - > - if (op == NL_SET) { > - if (af == AF_INET6) { > - size_t rta_len = RTA_LENGTH(sizeof(req.set.r6.d)); > > - req.nlh.nlmsg_len = offsetof(struct req_t, set.r6) > - + sizeof(req.set.r6); > + if (af == AF_INET6) { > + size_t rta_len = RTA_LENGTH(sizeof(req.set.r6.d)); > > - req.set.r6.rta_dst.rta_type = RTA_DST; > - req.set.r6.rta_dst.rta_len = rta_len; > + req.nlh.nlmsg_len = offsetof(struct req_t, set.r6) > + + sizeof(req.set.r6); > > - memcpy(&req.set.r6.a, gw, sizeof(req.set.r6.a)); > - req.set.r6.rta_gw.rta_type = RTA_GATEWAY; > - req.set.r6.rta_gw.rta_len = rta_len; > - } else { > - size_t rta_len = RTA_LENGTH(sizeof(req.set.r4.d)); > + req.set.r6.rta_dst.rta_type = RTA_DST; > + req.set.r6.rta_dst.rta_len = rta_len; > > - req.nlh.nlmsg_len = offsetof(struct req_t, set.r4) > - + sizeof(req.set.r4); > + memcpy(&req.set.r6.a, gw, sizeof(req.set.r6.a)); > + req.set.r6.rta_gw.rta_type = RTA_GATEWAY; > + req.set.r6.rta_gw.rta_len = rta_len; > + } else { > + size_t rta_len = RTA_LENGTH(sizeof(req.set.r4.d)); > > - req.set.r4.rta_dst.rta_type = RTA_DST; > - req.set.r4.rta_dst.rta_len = rta_len; > + req.nlh.nlmsg_len = offsetof(struct req_t, set.r4) > + + sizeof(req.set.r4); > > - req.set.r4.a = *(uint32_t *)gw; > - req.set.r4.rta_gw.rta_type = RTA_GATEWAY; > - req.set.r4.rta_gw.rta_len = rta_len; > - } > + req.set.r4.rta_dst.rta_type = RTA_DST; > + req.set.r4.rta_dst.rta_len = rta_len; > > - req.rtm.rtm_protocol = RTPROT_BOOT; > - req.nlh.nlmsg_flags |= NLM_F_ACK | NLM_F_EXCL | NLM_F_CREATE; > - } else { > - req.nlh.nlmsg_len = offsetof(struct req_t, set.r6); > - req.nlh.nlmsg_flags |= NLM_F_DUMP; > + req.set.r4.a = *(uint32_t *)gw; > + req.set.r4.rta_gw.rta_type = RTA_GATEWAY; > + req.set.r4.rta_gw.rta_len = rta_len; > } > > - 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_route_dup() - Copy routes 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 > + */ > +void nl_route_dup(unsigned int ifi, unsigned int ifi_ns, sa_family_t af) > +{ > + struct req_t { > + struct nlmsghdr nlh; > + struct rtmsg rtm; > + struct rtattr rta; > + unsigned int ifi; > + } req = { > + .nlh.nlmsg_type = RTM_GETROUTE, > + .nlh.nlmsg_len = sizeof(req), > + .nlh.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP, > + .nlh.nlmsg_seq = nl_seq++, > + > + .rtm.rtm_family = af, > + .rtm.rtm_table = RT_TABLE_MAIN, > + .rtm.rtm_scope = RT_SCOPE_UNIVERSE, > + .rtm.rtm_type = RTN_UNICAST, > + > + .rta.rta_type = RTA_OIF, > + .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), > + .ifi = ifi, > + }; > + char buf[NLBUFSIZ], resp[NLBUFSIZ]; > + unsigned dup_routes = 0; > + ssize_t n, nlmsgs_size; > + struct nlmsghdr *nh; > + unsigned i; > + > + if ((n = nl_req(0, buf, &req, req.nlh.nlmsg_len)) < 0) > return; > > - nh = (struct nlmsghdr *)buf; > nlmsgs_size = n; > > - for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) { > - if (nh->nlmsg_type != RTM_NEWROUTE) > - goto next; > - > - 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; > - dup_routes++; > - } > + for (nh = (struct nlmsghdr *)buf; > + NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE; > + nh = NLMSG_NEXT(nh, n)) { > + struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh); > + struct rtattr *rta; > + size_t na; > > - rtm = (struct rtmsg *)NLMSG_DATA(nh); > - if (op == NL_GET && rtm->rtm_dst_len) > + if (nh->nlmsg_type != RTM_NEWROUTE) > continue; > > + 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; > + dup_routes++; > + > for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na); > rta = RTA_NEXT(rta, na)) { > - if (op == NL_GET) { > - if (rta->rta_type != RTA_GATEWAY) > - continue; > - > - memcpy(gw, RTA_DATA(rta), RTA_PAYLOAD(rta)); > - return; > - } > - > - if (op == NL_DUP && rta->rta_type == RTA_OIF) > + if (rta->rta_type == RTA_OIF) > *(unsigned int *)RTA_DATA(rta) = ifi_ns; > } > - > -next: > - if (nh->nlmsg_type == NLMSG_DONE) > - break; > } > > - if (op == NL_DUP) { > - char resp[NLBUFSIZ]; > - unsigned i; > - > - nh = (struct nlmsghdr *)buf; > - /* Routes might have dependencies between each other, and the > - * kernel processes RTM_NEWROUTE messages sequentially. For n > - * valid routes, we might need to send up to n requests to get > - * all of them inserted. Routes that have been already inserted > - * won't cause the whole request to fail, so we can simply > - * repeat the whole request. This approach avoids the need to > - * calculate dependencies: let the kernel do that. > - */ > - for (i = 0; i < dup_routes; i++) > - nl_req(1, resp, nh, nlmsgs_size); > - } > + nh = (struct nlmsghdr *)buf; > + /* Routes might have dependencies between each other, and the > + * kernel processes RTM_NEWROUTE messages sequentially. For n > + * valid routes, we might need to send up to n requests to get > + * all of them inserted. Routes that have been already > + * inserted won't cause the whole request to fail, so we can > + * simply repeat the whole request. This approach avoids the > + * need to calculate dependencies: let the kernel do that. > + */ Or: /* Routes might have dependencies between each other, and the kernel * processes RTM_NEWROUTE messages sequentially. For n valid routes, we * might need to send up to n requests to get all of them inserted. * Routes that have been already inserted won't cause the whole request * to fail, so we can simply repeat the whole request. This approach * avoids the need to calculate dependencies: let the kernel do that. */ (can also be "fixed" in 6/17). -- Stefano