From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 429365A026D for ; Thu, 3 Aug 2023 04:27:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1691029621; bh=1c38zgp46alAYO1U6WtiMDadOYtOxqNowH3aBzOrCEA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SUC4uG0HIh+pRHA3S5W9l8dRjyfZkws3BtJVd57l4eUkX0XRYiMmOiJkyrj1gySSo BBmapEROUAMF43ogC8uBcEK4aj8hz4mjS7PHlsmz3QpSmXKXnox9D0prSUw3BLCFSO TK/5rrIxiQ8w/zdbJdJDCj84wFCe14z9dcGQT1+c= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RGXmP4cQDz4wxW; Thu, 3 Aug 2023 12:27:01 +1000 (AEST) Date: Thu, 3 Aug 2023 12:18:30 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 03/17] netlink: Split nl_route() into separate operation functions Message-ID: References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-4-david@gibson.dropbear.id.au> <20230803004740.0f271388@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lmfYHYuCyd8O3TZL" Content-Disposition: inline In-Reply-To: <20230803004740.0f271388@elisabeth> Message-ID-Hash: O27DIHG3H7JZ2645DOUSIELAGVNKVCJM X-Message-ID-Hash: O27DIHG3H7JZ2645DOUSIELAGVNKVCJM X-MailFrom: dgibson@gandalf.ozlabs.org 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: --lmfYHYuCyd8O3TZL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 03, 2023 at 12:47:40AM +0200, Stefano Brivio wrote: > On Mon, 24 Jul 2023 16:09:22 +1000 > David Gibson wrote: >=20 > > nl_route() can perform 3 quite different operations based on the 'op' > > parameter. Split this into separate functions for each one. This requ= ires > > more lines of code, but makes the internal logic of each operation much > > easier to follow. > >=20 > > Signed-off-by: David Gibson > > --- > > conf.c | 4 +- > > netlink.c | 238 ++++++++++++++++++++++++++++++++++-------------------- > > netlink.h | 11 +-- > > pasta.c | 16 ++-- > > 4 files changed, 164 insertions(+), 105 deletions(-) > >=20 > > 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, > > } > > =20 > > 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); > > =20 > > 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, > > } > > =20 > > 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); > > =20 > > 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) > > } > > =20 > > /** > > - * nl_route() - Get/set/copy routes for given interface and address fa= mily > > - * @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 addr= ess 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 =3D { > > + .nlh.nlmsg_type =3D RTM_GETROUTE, > > + .nlh.nlmsg_len =3D sizeof(req), > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_DUMP, > > + .nlh.nlmsg_seq =3D nl_seq++, > > + > > + .rtm.rtm_family =3D af, > > + .rtm.rtm_table =3D RT_TABLE_MAIN, > > + .rtm.rtm_scope =3D RT_SCOPE_UNIVERSE, > > + .rtm.rtm_type =3D RTN_UNICAST, > > + > > + .rta.rta_type =3D RTA_OIF, > > + .rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)), > > + .ifi =3D ifi, > > + }; > > + struct nlmsghdr *nh; > > + char buf[NLBUFSIZ]; > > + ssize_t n; > > + > > + if ((n =3D nl_req(0, buf, &req, req.nlh.nlmsg_len)) < 0) > > + return; > > + > > + for (nh =3D (struct nlmsghdr *)buf; > > + NLMSG_OK(nh, n) && nh->nlmsg_type !=3D NLMSG_DONE; > > + nh =3D NLMSG_NEXT(nh, n)) { > > + struct rtmsg *rtm =3D (struct rtmsg *)NLMSG_DATA(nh); > > + struct rtattr *rta; > > + size_t na; > > + > > + if (nh->nlmsg_type !=3D RTM_NEWROUTE) > > + continue; > > + > > + if (rtm->rtm_dst_len) > > + continue; > > + > > + for (rta =3D RTM_RTA(rtm), na =3D RTM_PAYLOAD(nh); RTA_OK(rta, na); > > + rta =3D RTA_NEXT(rta, na)) { > > + if (rta->rta_type !=3D RTA_GATEWAY) > > + continue; > > + > > + memcpy(gw, RTA_DATA(rta), RTA_PAYLOAD(rta)); > > + return; > > + } > > + } > > +} > > + > > +/** > > + * nl_route_set_def() - Set default route for given interface and addr= ess 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 =3D { > > - .nlh.nlmsg_type =3D op =3D=3D NL_SET ? RTM_NEWROUTE : RTM_GETROUTE, > > - .nlh.nlmsg_flags =3D NLM_F_REQUEST, > > + .nlh.nlmsg_type =3D RTM_NEWROUTE, > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_ACK | > > + NLM_F_CREATE | NLM_F_EXCL, > > .nlh.nlmsg_seq =3D nl_seq++, > > =20 > > .rtm.rtm_family =3D af, > > .rtm.rtm_table =3D RT_TABLE_MAIN, > > .rtm.rtm_scope =3D RT_SCOPE_UNIVERSE, > > .rtm.rtm_type =3D RTN_UNICAST, > > + .rtm.rtm_protocol =3D RTPROT_BOOT, > > =20 > > .rta.rta_type =3D RTA_OIF, > > .rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)), > > - .ifi =3D op =3D=3D NL_SET ? ifi_ns : ifi, > > + .ifi =3D ifi, > > }; > > - unsigned dup_routes =3D 0; > > - ssize_t n, nlmsgs_size; > > - struct nlmsghdr *nh; > > - struct rtattr *rta; > > char buf[NLBUFSIZ]; > > - struct rtmsg *rtm; > > - size_t na; > > - > > - if (op =3D=3D NL_SET) { > > - if (af =3D=3D AF_INET6) { > > - size_t rta_len =3D RTA_LENGTH(sizeof(req.set.r6.d)); > > =20 > > - req.nlh.nlmsg_len =3D offsetof(struct req_t, set.r6) > > - + sizeof(req.set.r6); > > + if (af =3D=3D AF_INET6) { > > + size_t rta_len =3D RTA_LENGTH(sizeof(req.set.r6.d)); > > =20 > > - req.set.r6.rta_dst.rta_type =3D RTA_DST; > > - req.set.r6.rta_dst.rta_len =3D rta_len; > > + req.nlh.nlmsg_len =3D offsetof(struct req_t, set.r6) > > + + sizeof(req.set.r6); > > =20 > > - memcpy(&req.set.r6.a, gw, sizeof(req.set.r6.a)); > > - req.set.r6.rta_gw.rta_type =3D RTA_GATEWAY; > > - req.set.r6.rta_gw.rta_len =3D rta_len; > > - } else { > > - size_t rta_len =3D RTA_LENGTH(sizeof(req.set.r4.d)); > > + req.set.r6.rta_dst.rta_type =3D RTA_DST; > > + req.set.r6.rta_dst.rta_len =3D rta_len; > > =20 > > - req.nlh.nlmsg_len =3D 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 =3D RTA_GATEWAY; > > + req.set.r6.rta_gw.rta_len =3D rta_len; > > + } else { > > + size_t rta_len =3D RTA_LENGTH(sizeof(req.set.r4.d)); > > =20 > > - req.set.r4.rta_dst.rta_type =3D RTA_DST; > > - req.set.r4.rta_dst.rta_len =3D rta_len; > > + req.nlh.nlmsg_len =3D offsetof(struct req_t, set.r4) > > + + sizeof(req.set.r4); > > =20 > > - req.set.r4.a =3D *(uint32_t *)gw; > > - req.set.r4.rta_gw.rta_type =3D RTA_GATEWAY; > > - req.set.r4.rta_gw.rta_len =3D rta_len; > > - } > > + req.set.r4.rta_dst.rta_type =3D RTA_DST; > > + req.set.r4.rta_dst.rta_len =3D rta_len; > > =20 > > - req.rtm.rtm_protocol =3D RTPROT_BOOT; > > - req.nlh.nlmsg_flags |=3D NLM_F_ACK | NLM_F_EXCL | NLM_F_CREATE; > > - } else { > > - req.nlh.nlmsg_len =3D offsetof(struct req_t, set.r6); > > - req.nlh.nlmsg_flags |=3D NLM_F_DUMP; > > + req.set.r4.a =3D *(uint32_t *)gw; > > + req.set.r4.rta_gw.rta_type =3D RTA_GATEWAY; > > + req.set.r4.rta_gw.rta_len =3D rta_len; > > } > > =20 > > - if ((n =3D nl_req(op =3D=3D NL_SET, buf, &req, req.nlh.nlmsg_len)) < = 0) > > - return; > > + nl_req(1, buf, &req, req.nlh.nlmsg_len); > > +} > > =20 > > - if (op =3D=3D 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 a= f) > > +{ > > + struct req_t { > > + struct nlmsghdr nlh; > > + struct rtmsg rtm; > > + struct rtattr rta; > > + unsigned int ifi; > > + } req =3D { > > + .nlh.nlmsg_type =3D RTM_GETROUTE, > > + .nlh.nlmsg_len =3D sizeof(req), > > + .nlh.nlmsg_flags =3D NLM_F_REQUEST | NLM_F_DUMP, > > + .nlh.nlmsg_seq =3D nl_seq++, > > + > > + .rtm.rtm_family =3D af, > > + .rtm.rtm_table =3D RT_TABLE_MAIN, > > + .rtm.rtm_scope =3D RT_SCOPE_UNIVERSE, > > + .rtm.rtm_type =3D RTN_UNICAST, > > + > > + .rta.rta_type =3D RTA_OIF, > > + .rta.rta_len =3D RTA_LENGTH(sizeof(unsigned int)), > > + .ifi =3D ifi, > > + }; > > + char buf[NLBUFSIZ], resp[NLBUFSIZ]; > > + unsigned dup_routes =3D 0; > > + ssize_t n, nlmsgs_size; > > + struct nlmsghdr *nh; > > + unsigned i; > > + > > + if ((n =3D nl_req(0, buf, &req, req.nlh.nlmsg_len)) < 0) > > return; > > =20 > > - nh =3D (struct nlmsghdr *)buf; > > nlmsgs_size =3D n; > > =20 > > - for ( ; NLMSG_OK(nh, n); nh =3D NLMSG_NEXT(nh, n)) { > > - if (nh->nlmsg_type !=3D RTM_NEWROUTE) > > - goto next; > > - > > - if (op =3D=3D NL_DUP) { > > - nh->nlmsg_seq =3D nl_seq++; > > - nh->nlmsg_pid =3D 0; > > - nh->nlmsg_flags &=3D ~NLM_F_DUMP_FILTERED; > > - nh->nlmsg_flags |=3D NLM_F_REQUEST | NLM_F_ACK | > > - NLM_F_CREATE; > > - dup_routes++; > > - } > > + for (nh =3D (struct nlmsghdr *)buf; > > + NLMSG_OK(nh, n) && nh->nlmsg_type !=3D NLMSG_DONE; > > + nh =3D NLMSG_NEXT(nh, n)) { > > + struct rtmsg *rtm =3D (struct rtmsg *)NLMSG_DATA(nh); > > + struct rtattr *rta; > > + size_t na; > > =20 > > - rtm =3D (struct rtmsg *)NLMSG_DATA(nh); > > - if (op =3D=3D NL_GET && rtm->rtm_dst_len) > > + if (nh->nlmsg_type !=3D RTM_NEWROUTE) > > continue; > > =20 > > + nh->nlmsg_seq =3D nl_seq++; > > + nh->nlmsg_pid =3D 0; > > + nh->nlmsg_flags &=3D ~NLM_F_DUMP_FILTERED; > > + nh->nlmsg_flags |=3D NLM_F_REQUEST | NLM_F_ACK | > > + NLM_F_CREATE; > > + dup_routes++; > > + > > for (rta =3D RTM_RTA(rtm), na =3D RTM_PAYLOAD(nh); RTA_OK(rta, na); > > rta =3D RTA_NEXT(rta, na)) { > > - if (op =3D=3D NL_GET) { > > - if (rta->rta_type !=3D RTA_GATEWAY) > > - continue; > > - > > - memcpy(gw, RTA_DATA(rta), RTA_PAYLOAD(rta)); > > - return; > > - } > > - > > - if (op =3D=3D NL_DUP && rta->rta_type =3D=3D RTA_OIF) > > + if (rta->rta_type =3D=3D RTA_OIF) > > *(unsigned int *)RTA_DATA(rta) =3D ifi_ns; > > } > > - > > -next: > > - if (nh->nlmsg_type =3D=3D NLMSG_DONE) > > - break; > > } > > =20 > > - if (op =3D=3D NL_DUP) { > > - char resp[NLBUFSIZ]; > > - unsigned i; > > - > > - nh =3D (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 =3D 0; i < dup_routes; i++) > > - nl_req(1, resp, nh, nlmsgs_size); > > - } > > + nh =3D (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. > > + */ >=20 > Or: >=20 > /* 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. > */ >=20 > (can also be "fixed" in 6/17). Huh... I just used M-q to wrap these in emacs, and it turns out the default fill-column value is 70, not 80. Fixed up my config and adjusted this accordingly. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --lmfYHYuCyd8O3TZL Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmTLDm8ACgkQzQJF27ox 2GddHRAAgvCCsCOfc6R9J9EqrYW3zQCABPmX1IEsX/ya1lD3P+L2YMwtYuFZGqUE MB37FlGLZriqaNkIg+6pR3BAJeT/EbilPyBLytzdgBxH22E8yUaHHtGEDcV76PwB vW6LNoCs271Xz8HhAnHy1HDP16lvQMCryu2EkMt/CPZCV3iN4dwUlONzQrIYLsJu xLC9VEcP9V2Qy7NoR9b7TfNw4DE5LGp1NuaBHeZYtUsmi1RFlBc6tzaD56+I6Ico EH+OxQ8aVdJBUqI/8COcHa/bkTTaN53nHoCuLDVGVP9M+2YpUan3AMZEVuEtHcmE ccz2lEh7zS8w/b/ChhJ17Glwg+l3nMfIC9siN65VmzqyhP3NrcbWITLRXztBHQKh uZv96d4a2iw3Grx+ImpxjyQnU9cOnYJgVcJgrtWu4GQq5fjsDPGC+0WH+7hD16y6 AEXKfxHgyAGBe61a6W0u7U4Cf90RW8TbgRybl5pTaW0auCfsJxJLWPRXf+TBt/sN 0dvpHbcJcVpiZjkd9M/y78UTZrUex4oUMMjjI7T2H3ZUwa5VEgPfF/UXVgR4c59W Rdf1FwiYyW14nzooqvhu3WjKha5nZqDJe+pVpAqm08IMrjM6+Pg2E3HNCcQX7z5m BGBTKAKnLNCih9WbgTr8ZRc1At0H3fvLRhmXJ24XJWEaW/lXSqo= =OrKd -----END PGP SIGNATURE----- --lmfYHYuCyd8O3TZL--