From 8a61566dd8464c54c0b587febe3b334abba7a434 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 26 Mar 2021 18:23:35 +0800 Subject: [PATCH] common/pick_addr: refactor pick_address.cc and ipaddr.cc * do not replicate the same logic in IPv4 and IPv6 paths * use helpers returning bool for filtering the candidate addresses for better readability Signed-off-by: Kefu Chai (cherry picked from commit 52785d5a3607b2f2ee6d41069d18a154b3eb5d45) Conflicts: src/common/ipaddr.cc src/common/pick_address.cc: trivial resolution --- src/common/ipaddr.cc | 118 ++++++------------------ src/common/pick_address.cc | 182 +++++++++++++++++++------------------ src/include/ipaddr.h | 15 ++- src/test/test_ipaddr.cc | 70 ++++++-------- 4 files changed, 157 insertions(+), 228 deletions(-) diff --git a/src/common/ipaddr.cc b/src/common/ipaddr.cc index 0abf7f20ca71f..bd11cbfc1808f 100644 --- a/src/common/ipaddr.cc +++ b/src/common/ipaddr.cc @@ -3,7 +3,6 @@ #include #include #include -#include #if defined(__FreeBSD__) #include #include @@ -29,54 +28,23 @@ void netmask_ipv4(const struct in_addr *addr, out->s_addr = addr->s_addr & mask; } - -static bool match_numa_node(const string& if_name, int numa_node) +bool matches_ipv4_in_subnet(const struct ifaddrs& addrs, + const struct sockaddr_in* net, + unsigned int prefix_len) { -#ifdef WITH_SEASTAR - return true; -#else - int if_node = -1; - int r = get_iface_numa_node(if_name, &if_node); - if (r < 0) { + if (addrs.ifa_addr == nullptr) return false; - } - return if_node == numa_node; -#endif -} - -const struct ifaddrs *find_ipv4_in_subnet(const struct ifaddrs *addrs, - const struct sockaddr_in *net, - unsigned int prefix_len, - int numa_node) { - struct in_addr want, temp; + if (addrs.ifa_addr->sa_family != net->sin_family) + return false; + struct in_addr want; netmask_ipv4(&net->sin_addr, prefix_len, &want); - for (; addrs != NULL; addrs = addrs->ifa_next) { - - if (addrs->ifa_addr == NULL) - continue; - - if (strcmp(addrs->ifa_name, "lo") == 0 || boost::starts_with(addrs->ifa_name, "lo:")) - continue; - - if (numa_node >= 0 && !match_numa_node(addrs->ifa_name, numa_node)) - continue; - - if (addrs->ifa_addr->sa_family != net->sin_family) - continue; - - struct in_addr *cur = &((struct sockaddr_in*)addrs->ifa_addr)->sin_addr; - netmask_ipv4(cur, prefix_len, &temp); - - if (temp.s_addr == want.s_addr) { - return addrs; - } - } - - return NULL; + struct in_addr *cur = &((struct sockaddr_in*)addrs.ifa_addr)->sin_addr; + struct in_addr temp; + netmask_ipv4(cur, prefix_len, &temp); + return temp.s_addr == want.s_addr; } - void netmask_ipv6(const struct in6_addr *addr, unsigned int prefix_len, struct in6_addr *out) { @@ -90,59 +58,25 @@ void netmask_ipv6(const struct in6_addr *addr, memset(out->s6_addr+prefix_len/8+1, 0, 16-prefix_len/8-1); } +bool matches_ipv6_in_subnet(const struct ifaddrs& addrs, + const struct sockaddr_in6* net, + unsigned int prefix_len) +{ + if (addrs.ifa_addr == nullptr) + return false; -const struct ifaddrs *find_ipv6_in_subnet(const struct ifaddrs *addrs, - const struct sockaddr_in6 *net, - unsigned int prefix_len, - int numa_node) { - struct in6_addr want, temp; - + if (addrs.ifa_addr->sa_family != net->sin6_family) + return false; + struct in6_addr want; netmask_ipv6(&net->sin6_addr, prefix_len, &want); - for (; addrs != NULL; addrs = addrs->ifa_next) { - - if (addrs->ifa_addr == NULL) - continue; - - if (strcmp(addrs->ifa_name, "lo") == 0 || boost::starts_with(addrs->ifa_name, "lo:")) - continue; - - if (numa_node >= 0 && !match_numa_node(addrs->ifa_name, numa_node)) - continue; - - if (addrs->ifa_addr->sa_family != net->sin6_family) - continue; - - struct in6_addr *cur = &((struct sockaddr_in6*)addrs->ifa_addr)->sin6_addr; - if (IN6_IS_ADDR_LINKLOCAL(cur)) - continue; - netmask_ipv6(cur, prefix_len, &temp); - - if (IN6_ARE_ADDR_EQUAL(&temp, &want)) - return addrs; - } - - return NULL; -} - - -const struct ifaddrs *find_ip_in_subnet(const struct ifaddrs *addrs, - const struct sockaddr *net, - unsigned int prefix_len, - int numa_node) { - switch (net->sa_family) { - case AF_INET: - return find_ipv4_in_subnet(addrs, (struct sockaddr_in*)net, prefix_len, - numa_node); - - case AF_INET6: - return find_ipv6_in_subnet(addrs, (struct sockaddr_in6*)net, prefix_len, - numa_node); - } - - return NULL; + struct in6_addr temp; + struct in6_addr *cur = &((struct sockaddr_in6*)addrs.ifa_addr)->sin6_addr; + if (IN6_IS_ADDR_LINKLOCAL(cur)) + return false; + netmask_ipv6(cur, prefix_len, &temp); + return IN6_ARE_ADDR_EQUAL(&temp, &want); } - bool parse_network(const char *s, struct sockaddr_storage *network, unsigned int *prefix_len) { char *slash = strchr((char*)s, '/'); if (!slash) { diff --git a/src/common/pick_address.cc b/src/common/pick_address.cc index 8f8fd4fbb5c2c..a369980ffddd4 100644 --- a/src/common/pick_address.cc +++ b/src/common/pick_address.cc @@ -35,6 +35,75 @@ #define dout_subsys ceph_subsys_ +namespace { + +bool matches_with_name(const ifaddrs& ifa, const std::string& if_name) +{ + return if_name.compare(ifa.ifa_name) == 0; +} + +bool is_addr_allowed(const ifaddrs& ifa) +{ + if (strcmp(ifa.ifa_name, "lo") == 0) { + return false; + } + if (boost::starts_with(ifa.ifa_name, "lo:")) { + return false; + } + return true; +} + +bool matches_with_net(const ifaddrs& ifa, + const sockaddr* net, + unsigned int prefix_len, + unsigned ipv) +{ + switch (net->sa_family) { + case AF_INET: + if (ipv & CEPH_PICK_ADDRESS_IPV4) { + return matches_ipv4_in_subnet(ifa, (struct sockaddr_in*)net, prefix_len); + } + break; + case AF_INET6: + if (ipv & CEPH_PICK_ADDRESS_IPV6) { + return matches_ipv6_in_subnet(ifa, (struct sockaddr_in6*)net, prefix_len); + } + break; + } + return false; +} + +bool matches_with_net(CephContext *cct, + const ifaddrs& ifa, + const std::string& s, + unsigned ipv) +{ + struct sockaddr_storage net; + unsigned int prefix_len; + if (!parse_network(s.c_str(), &net, &prefix_len)) { + lderr(cct) << "unable to parse network: " << s << dendl; + exit(1); + } + return matches_with_net(ifa, (sockaddr*)&net, prefix_len, ipv); +} + +bool matches_with_numa_node(const ifaddrs& ifa, int numa_node) +{ +#if defined(WITH_SEASTAR) || defined(_WIN32) + return true; +#else + if (numa_node < 0) { + return true; + } + int if_node = -1; + int r = get_iface_numa_node(ifa.ifa_name, &if_node); + if (r < 0) { + return false; + } + return if_node == numa_node; +#endif +} +} const struct sockaddr *find_ip_in_subnet_list( CephContext *cct, const struct ifaddrs *ifa, @@ -43,86 +112,32 @@ const struct sockaddr *find_ip_in_subnet_list( const std::string &interfaces, int numa_node) { - std::list nets; - get_str_list(networks, nets); - std::list ifs; - get_str_list(interfaces, ifs); - - // filter interfaces by name - const struct ifaddrs *filtered = nullptr; - if (ifs.empty()) { - filtered = ifa; - } else { - if (nets.empty()) { + const auto ifs = get_str_list(interfaces); + const auto nets = get_str_list(networks); + if (!ifs.empty() && nets.empty()) { lderr(cct) << "interface names specified but not network names" << dendl; exit(1); - } - const struct ifaddrs *t = ifa; - struct ifaddrs *head = 0; - while (t) { - bool match = false; - for (auto& i : ifs) { - if (strcmp(i.c_str(), t->ifa_name) == 0) { - match = true; - break; - } - } - if (match) { - struct ifaddrs *n = new ifaddrs; - memcpy(n, t, sizeof(*t)); - n->ifa_next = head; - head = n; - } - t = t->ifa_next; - } - if (!head) { - lderr(cct) << "no interfaces matching " << ifs << dendl; - exit(1); - } - filtered = head; } - struct sockaddr *r = nullptr; - for (auto& s : nets) { - struct sockaddr_storage net; - unsigned int prefix_len; - - if (!parse_network(s.c_str(), &net, &prefix_len)) { - lderr(cct) << "unable to parse network: " << s << dendl; - exit(1); + for (const auto* addr = ifa; addr != nullptr; addr = addr->ifa_next) { + if (!is_addr_allowed(*addr)) { + continue; } - - switch (net.ss_family) { - case AF_INET: - if (!(ipv & CEPH_PICK_ADDRESS_IPV4)) { - continue; - } - break; - case AF_INET6: - if (!(ipv & CEPH_PICK_ADDRESS_IPV6)) { - continue; - } - break; - } - - const struct ifaddrs *found = find_ip_in_subnet( - filtered, - (struct sockaddr *) &net, prefix_len, numa_node); - if (found) { - r = found->ifa_addr; - break; - } - } - - if (filtered != ifa) { - while (filtered) { - struct ifaddrs *t = filtered->ifa_next; - delete filtered; - filtered = t; + if ((ifs.empty() || + std::any_of(std::begin(ifs), std::end(ifs), + [&](const auto& if_name) { + return matches_with_name(*addr, if_name); + })) && + (nets.empty() || + std::any_of(std::begin(nets), std::end(nets), + [&](const auto& net) { + return matches_with_net(cct, *addr, net, ipv); + })) && + matches_with_numa_node(*addr, numa_node)) { + return addr->ifa_addr; } } - - return r; + return nullptr; } #ifndef WITH_SEASTAR @@ -464,20 +479,15 @@ std::string pick_iface(CephContext *cct, const struct sockaddr_storage &network) lderr(cct) << "unable to fetch interfaces and addresses: " << err << dendl; return {}; } - - const unsigned int prefix_len = max(sizeof(in_addr::s_addr), sizeof(in6_addr::s6_addr)) * CHAR_BIT; - const struct ifaddrs *found = find_ip_in_subnet( - ifa, - (const struct sockaddr *) &network, prefix_len); - - std::string result; - if (found) { - result = found->ifa_name; + auto free_ifa = make_scope_guard([ifa] { freeifaddrs(ifa); }); + const unsigned int prefix_len = std::max(sizeof(in_addr::s_addr), sizeof(in6_addr::s6_addr)) * CHAR_BIT; + for (auto addr = ifa; addr != nullptr; addr = addr->ifa_next) { + if (matches_with_net(*ifa, (const struct sockaddr *) &network, prefix_len, + CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6)) { + return addr->ifa_name; + } } - - freeifaddrs(ifa); - - return result; + return {}; } diff --git a/src/include/ipaddr.h b/src/include/ipaddr.h index e8bed82920af3..bf06cfc93642a 100644 --- a/src/include/ipaddr.h +++ b/src/include/ipaddr.h @@ -4,15 +4,14 @@ class entity_addr_t; /* - * Find an IP address that is in the wanted subnet. - * - * If there are multiple matches, the first one is returned; this order - * is system-dependent and should not be relied on. + * Check if an IP address that is in the wanted subnet. */ -const struct ifaddrs *find_ip_in_subnet(const struct ifaddrs *addrs, - const struct sockaddr *net, - unsigned int prefix_len, - int numa_node = -1); +bool matches_ipv4_in_subnet(const struct ifaddrs& addrs, + const struct sockaddr_in* net, + unsigned int prefix_len); +bool matches_ipv6_in_subnet(const struct ifaddrs& addrs, + const struct sockaddr_in6* net, + unsigned int prefix_len); /* * Validate and parse IPv4 or IPv6 network diff --git a/src/test/test_ipaddr.cc b/src/test/test_ipaddr.cc index d904a6819fffd..efd736193389f 100644 --- a/src/test/test_ipaddr.cc +++ b/src/test/test_ipaddr.cc @@ -38,7 +38,6 @@ TEST(CommonIPAddr, TestNotFound) struct sockaddr_in a_one; struct sockaddr_in6 a_two; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -53,9 +52,8 @@ TEST(CommonIPAddr, TestNotFound) ipv4(&a_one, "10.11.12.13"); ipv6(&a_two, "2001:1234:5678:90ab::cdef"); ipv4(&net, "10.11.234.56"); - - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 24); - ASSERT_EQ(0, result); + ASSERT_FALSE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 24)); + ASSERT_FALSE(matches_ipv6_in_subnet(two, (struct sockaddr_in6*)&net, 24)); } TEST(CommonIPAddr, TestV4_Simple) @@ -64,7 +62,6 @@ TEST(CommonIPAddr, TestV4_Simple) struct sockaddr_in a_one; struct sockaddr_in6 a_two; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -80,8 +77,8 @@ TEST(CommonIPAddr, TestV4_Simple) ipv6(&a_two, "2001:1234:5678:90ab::cdef"); ipv4(&net, "10.11.12.42"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 24); - ASSERT_EQ((struct sockaddr*)&a_one, result->ifa_addr); + ASSERT_TRUE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 24)); + ASSERT_FALSE(matches_ipv4_in_subnet(two, (struct sockaddr_in*)&net, 24)); } TEST(CommonIPAddr, TestV4_Prefix25) @@ -90,7 +87,6 @@ TEST(CommonIPAddr, TestV4_Prefix25) struct sockaddr_in a_one; struct sockaddr_in a_two; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -106,8 +102,8 @@ TEST(CommonIPAddr, TestV4_Prefix25) ipv4(&a_two, "10.11.12.129"); ipv4(&net, "10.11.12.128"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 25); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 25)); + ASSERT_TRUE(matches_ipv4_in_subnet(two, (struct sockaddr_in*)&net, 25)); } TEST(CommonIPAddr, TestV4_Prefix16) @@ -116,7 +112,6 @@ TEST(CommonIPAddr, TestV4_Prefix16) struct sockaddr_in a_one; struct sockaddr_in a_two; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -132,8 +127,8 @@ TEST(CommonIPAddr, TestV4_Prefix16) ipv4(&a_two, "10.2.1.123"); ipv4(&net, "10.2.0.0"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 16); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 16)); + ASSERT_TRUE(matches_ipv4_in_subnet(two, (struct sockaddr_in*)&net, 16)); } TEST(CommonIPAddr, TestV4_PrefixTooLong) @@ -141,7 +136,6 @@ TEST(CommonIPAddr, TestV4_PrefixTooLong) struct ifaddrs one; struct sockaddr_in a_one; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -152,8 +146,7 @@ TEST(CommonIPAddr, TestV4_PrefixTooLong) ipv4(&a_one, "10.11.12.13"); ipv4(&net, "10.11.12.12"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 42); - ASSERT_EQ(0, result); + ASSERT_FALSE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 42)); } TEST(CommonIPAddr, TestV4_PrefixZero) @@ -162,7 +155,6 @@ TEST(CommonIPAddr, TestV4_PrefixZero) struct sockaddr_in6 a_one; struct sockaddr_in a_two; struct sockaddr_in net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -178,8 +170,8 @@ TEST(CommonIPAddr, TestV4_PrefixZero) ipv4(&a_two, "10.1.2.3"); ipv4(&net, "255.0.1.2"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 0); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv4_in_subnet(one, (struct sockaddr_in*)&net, 0)); + ASSERT_TRUE(matches_ipv4_in_subnet(two, (struct sockaddr_in*)&net, 0)); } static char lo[] = "lo"; @@ -191,10 +183,6 @@ TEST(CommonIPAddr, TestV4_SkipLoopback) struct sockaddr_in a_one; struct sockaddr_in a_two; struct sockaddr_in a_three; - struct sockaddr_in net; - const struct ifaddrs *result; - - memset(&net, 0, sizeof(net)); one.ifa_next = &two; one.ifa_addr = (struct sockaddr*)&a_one; @@ -211,10 +199,12 @@ TEST(CommonIPAddr, TestV4_SkipLoopback) ipv4(&a_one, "127.0.0.1"); ipv4(&a_two, "127.0.0.1"); ipv4(&a_three, "10.1.2.3"); - ipv4(&net, "255.0.0.0"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 0); - ASSERT_EQ((struct sockaddr*)&a_three, result->ifa_addr); + const struct sockaddr *result = + find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one, + CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6, + "10.0.0.0/8", ""); + ASSERT_EQ((struct sockaddr*)&a_three, result); } TEST(CommonIPAddr, TestV6_Simple) @@ -223,7 +213,6 @@ TEST(CommonIPAddr, TestV6_Simple) struct sockaddr_in a_one; struct sockaddr_in6 a_two; struct sockaddr_in6 net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -239,8 +228,8 @@ TEST(CommonIPAddr, TestV6_Simple) ipv6(&a_two, "2001:1234:5678:90ab::cdef"); ipv6(&net, "2001:1234:5678:90ab::dead:beef"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 64); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv6_in_subnet(one, (struct sockaddr_in6*)&net, 64)); + ASSERT_TRUE(matches_ipv6_in_subnet(two, (struct sockaddr_in6*)&net, 64)); } TEST(CommonIPAddr, TestV6_Prefix57) @@ -249,7 +238,6 @@ TEST(CommonIPAddr, TestV6_Prefix57) struct sockaddr_in6 a_one; struct sockaddr_in6 a_two; struct sockaddr_in6 net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -265,8 +253,8 @@ TEST(CommonIPAddr, TestV6_Prefix57) ipv6(&a_two, "2001:1234:5678:90ab::cdef"); ipv6(&net, "2001:1234:5678:90ab::dead:beef"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 57); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv6_in_subnet(one, (struct sockaddr_in6*)&net, 57)); + ASSERT_TRUE(matches_ipv6_in_subnet(two, (struct sockaddr_in6*)&net, 57)); } TEST(CommonIPAddr, TestV6_PrefixTooLong) @@ -274,7 +262,6 @@ TEST(CommonIPAddr, TestV6_PrefixTooLong) struct ifaddrs one; struct sockaddr_in6 a_one; struct sockaddr_in6 net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -285,8 +272,7 @@ TEST(CommonIPAddr, TestV6_PrefixTooLong) ipv6(&a_one, "2001:1234:5678:900F::cdef"); ipv6(&net, "2001:1234:5678:900F::cdee"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 9000); - ASSERT_EQ(0, result); + ASSERT_FALSE(matches_ipv6_in_subnet(one, (struct sockaddr_in6*)&net, 9000)); } TEST(CommonIPAddr, TestV6_PrefixZero) @@ -295,7 +281,6 @@ TEST(CommonIPAddr, TestV6_PrefixZero) struct sockaddr_in a_one; struct sockaddr_in6 a_two; struct sockaddr_in6 net; - const struct ifaddrs *result; one.ifa_next = &two; one.ifa_addr = (struct sockaddr*)&a_one; @@ -309,8 +294,8 @@ TEST(CommonIPAddr, TestV6_PrefixZero) ipv6(&a_two, "2001:f00b::1"); ipv6(&net, "ff00::1"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 0); - ASSERT_EQ((struct sockaddr*)&a_two, result->ifa_addr); + ASSERT_FALSE(matches_ipv6_in_subnet(one, (struct sockaddr_in6*)&net, 0)); + ASSERT_TRUE(matches_ipv6_in_subnet(two, (struct sockaddr_in6*)&net, 0)); } TEST(CommonIPAddr, TestV6_SkipLoopback) @@ -320,7 +305,6 @@ TEST(CommonIPAddr, TestV6_SkipLoopback) struct sockaddr_in6 a_two; struct sockaddr_in6 a_three; struct sockaddr_in6 net; - const struct ifaddrs *result; memset(&net, 0, sizeof(net)); @@ -341,8 +325,11 @@ TEST(CommonIPAddr, TestV6_SkipLoopback) ipv6(&a_three, "2001:1234:5678:90ab::beef"); ipv6(&net, "ff00::1"); - result = find_ip_in_subnet(&one, (struct sockaddr*)&net, 0); - ASSERT_EQ((struct sockaddr*)&a_three, result->ifa_addr); + const struct sockaddr *result = + find_ip_in_subnet_list(nullptr, (struct ifaddrs*)&one, + CEPH_PICK_ADDRESS_IPV4 | CEPH_PICK_ADDRESS_IPV6, + "2001:1234:5678:90ab::0/64", ""); + ASSERT_EQ((struct sockaddr*)&a_three, result); } TEST(CommonIPAddr, ParseNetwork_Empty) @@ -994,4 +981,3 @@ TEST(pick_address, ipv4_ipv6_enabled2) ASSERT_EQ(-1, r); } } - -- 2.39.5