From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH 02/17] netlink: Split nl_addr() into separate operation functions
Date: Thu, 3 Aug 2023 12:11:13 +1000 [thread overview]
Message-ID: <ZMsMweIv05mMQbzC@zatzit> (raw)
In-Reply-To: <20230803004750.4e2feb37@elisabeth>
[-- Attachment #1: Type: text/plain, Size: 14612 bytes --]
On Thu, Aug 03, 2023 at 12:47:50AM +0200, Stefano Brivio wrote:
> On Mon, 24 Jul 2023 16:09:21 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <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);
>
> I guess this should be c->ifi6 (also in 17/17).
Oops, yes. Fixed.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-08-03 2:27 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-24 6:09 [PATCH 00/17] netlink fixes and cleanups David Gibson
2023-07-24 6:09 ` [PATCH 01/17] netlink: Split up functionality if nl_link() David Gibson
2023-08-02 22:47 ` Stefano Brivio
2023-08-03 2:09 ` David Gibson
2023-08-03 4:29 ` David Gibson
2023-08-03 5:39 ` David Gibson
2023-08-03 5:40 ` Stefano Brivio
2023-07-24 6:09 ` [PATCH 02/17] netlink: Split nl_addr() into separate operation functions David Gibson
2023-08-02 22:47 ` Stefano Brivio
2023-08-03 2:11 ` David Gibson [this message]
2023-07-24 6:09 ` [PATCH 03/17] netlink: Split nl_route() " David Gibson
2023-08-02 22:47 ` Stefano Brivio
2023-08-03 2:18 ` David Gibson
2023-07-24 6:09 ` [PATCH 04/17] netlink: Use struct in_addr for IPv4 addresses, not bare uint32_t David Gibson
2023-07-24 6:09 ` [PATCH 05/17] netlink: Explicitly pass netlink sockets to operations David Gibson
2023-07-24 6:09 ` [PATCH 06/17] netlink: Make nl_*_dup() use a separate datagram for each request David Gibson
2023-07-24 6:09 ` [PATCH 07/17] netlink: Start sequence number from 1 instead of 0 David Gibson
2023-07-24 6:09 ` [PATCH 08/17] netlink: Treat send() or recv() errors as fatal David Gibson
2023-08-02 22:47 ` Stefano Brivio
2023-08-03 2:19 ` David Gibson
2023-07-24 6:09 ` [PATCH 09/17] netlink: Fill in netlink header fields from nl_req() David Gibson
2023-07-24 6:09 ` [PATCH 10/17] netlink: Add nl_do() helper for simple operations with error checking David Gibson
2023-08-02 22:48 ` Stefano Brivio
2023-08-03 2:24 ` David Gibson
2023-07-24 6:09 ` [PATCH 11/17] netlink: Clearer reasoning about the netlink response buffer size David Gibson
2023-08-02 22:48 ` Stefano Brivio
2023-08-03 2:22 ` David Gibson
2023-07-24 6:09 ` [PATCH 12/17] netlink: Split nl_req() to allow processing multiple response datagrams David Gibson
2023-07-24 6:09 ` [PATCH 13/17] netlink: Add nl_foreach_oftype to filter response message types David Gibson
2023-07-24 6:09 ` [PATCH 14/17] netlink: Propagate errors for "set" operations David Gibson
2023-07-24 6:09 ` [PATCH 15/17] netlink: Always process all responses to a netlink request David Gibson
2023-07-24 6:09 ` [PATCH 16/17] netlink: Propagate errors for "dump" operations David Gibson
2023-07-24 6:09 ` [PATCH 17/17] netlink: Propagate errors for "dup" operations David Gibson
2023-08-02 22:48 ` Stefano Brivio
2023-08-03 2:26 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZMsMweIv05mMQbzC@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).