From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top
Subject: Re: [PATCH 02/17] netlink: Split nl_addr() into separate operation functions
Date: Thu, 3 Aug 2023 00:47:50 +0200 [thread overview]
Message-ID: <20230803004750.4e2feb37@elisabeth> (raw)
In-Reply-To: <20230724060936.952659-3-david@gibson.dropbear.id.au>
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).
--
Stefano
next prev parent reply other threads:[~2023-08-02 22:47 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 [this message]
2023-08-03 2:11 ` David Gibson
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=20230803004750.4e2feb37@elisabeth \
--to=sbrivio@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
/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).