From 52785d5a3607b2f2ee6d41069d18a154b3eb5d45 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 --- src/common/ipaddr.cc | 118 ++++++------------------ src/common/pick_address.cc | 181 ++++++++++++++++++++----------------- src/include/ipaddr.h | 15 ++- src/test/test_ipaddr.cc | 70 ++++++-------- 4 files changed, 157 insertions(+), 227 deletions(-) diff --git a/src/common/ipaddr.cc b/src/common/ipaddr.cc index 5eddc9b1628..8c5da54b92f 100644 --- a/src/common/ipaddr.cc +++ b/src/common/ipaddr.cc @@ -5,7 +5,6 @@ #include #include #include -#include #if defined(__FreeBSD__) #include #include @@ -33,54 +32,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) { -#if defined(WITH_SEASTAR) || defined(_WIN32) - 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) { @@ -94,59 +62,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 a2b72371eec..a17b6a349dc 100644 --- a/src/common/pick_address.cc +++ b/src/common/pick_address.cc @@ -19,6 +19,7 @@ #include #include +#include #include #include "include/ipaddr.h" @@ -37,6 +38,75 @@ using std::string; using std::vector; +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, @@ -45,86 +115,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); - } - - 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; + for (const auto* addr = ifa; addr != nullptr; addr = addr->ifa_next) { + if (!is_addr_allowed(*addr)) { + continue; } - } - - 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 @@ -467,20 +483,15 @@ std::string pick_iface(CephContext *cct, const struct sockaddr_storage &network) lderr(cct) << "unable to fetch interfaces and addresses: " << err << dendl; return {}; } - + 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; - const struct ifaddrs *found = find_ip_in_subnet( - ifa, - (const struct sockaddr *) &network, prefix_len); - - std::string result; - if (found) { - result = found->ifa_name; + 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 e8bed82920a..bf06cfc9364 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 748ecf05eb8..3eccd5be42e 100644 --- a/src/test/test_ipaddr.cc +++ b/src/test/test_ipaddr.cc @@ -39,7 +39,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)); @@ -54,9 +53,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) @@ -65,7 +63,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)); @@ -81,8 +78,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) @@ -91,7 +88,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)); @@ -107,8 +103,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) @@ -117,7 +113,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)); @@ -133,8 +128,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) @@ -142,7 +137,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)); @@ -153,8 +147,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) @@ -163,7 +156,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)); @@ -179,8 +171,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"; @@ -192,10 +184,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; @@ -212,10 +200,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) @@ -224,7 +214,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)); @@ -240,8 +229,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) @@ -250,7 +239,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)); @@ -266,8 +254,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) @@ -275,7 +263,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)); @@ -286,8 +273,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) @@ -296,7 +282,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; @@ -310,8 +295,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) @@ -321,7 +306,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)); @@ -342,8 +326,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) @@ -997,4 +984,3 @@ TEST(pick_address, ipv4_ipv6_enabled2) ASSERT_EQ(-1, r); } } - -- 2.39.5