From 45d9eff5e4faf9f2fe91a48ec71e7147c589e771 Mon Sep 17 00:00:00 2001
From: Kefu Chai <tchaikov@gmail.com>
Date: Thu, 14 Oct 2021 07:51:31 +0800
Subject: [PATCH] common/pick_address: refactor pick_addresses()

* consolidate the logic handling CEPH_PICK_ADDRESS_PREFER_IPV4 using
  std::sort(). this might be overkill. but it helps to explain
  what CEPH_PICK_ADDRESS_PREFER_IPV4 is for, and helps to dedup
  the code to order the addresses.
* let fill_in_one_address() return an optional<entity_addrvec_t>.
  more readable this way
* early return if the required address is not found, instead of
  checking variables like ipv4_r
* rename fill_in_one_address() to get_one_address() to reflect
  the change of the function's return value's type

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
---
 src/common/pick_address.cc | 79 ++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 46 deletions(-)

diff --git a/src/common/pick_address.cc b/src/common/pick_address.cc
index 1b7831c060634..0ab19292582b5 100644
--- a/src/common/pick_address.cc
+++ b/src/common/pick_address.cc
@@ -294,13 +294,12 @@ void pick_addresses(CephContext *cct, int needs)
 }
 #endif	// !WITH_SEASTAR
 
-static int fill_in_one_address(
+static std::optional<entity_addr_t> get_one_address(
   CephContext *cct,
   const struct ifaddrs *ifa,
   unsigned ipv,
   const string &networks,
   const string &interfaces,
-  entity_addrvec_t *addrs,
   int numa_node = -1)
 {
   const struct sockaddr *found = find_ip_in_subnet_list(cct, ifa, ipv,
@@ -318,7 +317,7 @@ static int fill_in_one_address(
     }
     lderr(cct) << "unable to find any " << ip_type << " address in networks '"
                << networks << "' interfaces '" << interfaces << "'" << dendl;
-    return -1;
+    return {};
   }
 
   char buf[INET6_ADDRSTRLEN];
@@ -334,16 +333,15 @@ static int fill_in_one_address(
 		    NI_NUMERICHOST);
   if (err != 0) {
     lderr(cct) << "unable to convert chosen address to string: " << gai_strerror(err) << dendl;
-    return -1;
+    return {};
   }
 
   entity_addr_t addr;
-  bool r = addr.parse(buf);
-  if (!r) {
-    return -1;
+  if (addr.parse(buf)) {
+    return addr;
+  } else {
+    return {};
   }
-  addrs->v.push_back(addr);
-  return 0;
 }
 
 int pick_addresses(
@@ -417,31 +415,19 @@ int pick_addresses(
   }
   if (addr.is_blank_ip() &&
       !networks.empty()) {
-    int ipv4_r = !(ipv & CEPH_PICK_ADDRESS_IPV4) ? 0 : -1;
-    int ipv6_r = !(ipv & CEPH_PICK_ADDRESS_IPV6) ? 0 : -1;
     // note: pass in ipv to filter the matching addresses
-    if ((ipv & CEPH_PICK_ADDRESS_IPV4) &&
-	(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
-      ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
-				   networks, interfaces,
-				   addrs,
-				   preferred_numa_node);
-    }
-    if (ipv & CEPH_PICK_ADDRESS_IPV6) {
-      ipv6_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV6,
-				   networks, interfaces,
-				   addrs,
-				   preferred_numa_node);
-    }
-    if ((ipv & CEPH_PICK_ADDRESS_IPV4) &&
-	!(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
-      ipv4_r = fill_in_one_address(cct, ifa, CEPH_PICK_ADDRESS_IPV4,
-				   networks, interfaces,
-				   addrs,
-				   preferred_numa_node);
-    }
-    if (ipv4_r < 0 || ipv6_r < 0) {
-      return -1;
+    for (auto pick_mask :  {CEPH_PICK_ADDRESS_IPV4, CEPH_PICK_ADDRESS_IPV6}) {
+      if (ipv & pick_mask) {
+        auto ip_addr = get_one_address(cct, ifa, pick_mask,
+                                       networks, interfaces,
+                                       preferred_numa_node);
+        if (ip_addr) {
+          addrs->v.push_back(*ip_addr);
+        } else {
+          // picked but not found
+          return -1;
+        }
+      }
     }
   }
 
@@ -450,22 +436,23 @@ int pick_addresses(
   // ipv4 and/or ipv6?
   if (addrs->v.empty()) {
     addr.set_type(entity_addr_t::TYPE_MSGR2);
-    if ((ipv & CEPH_PICK_ADDRESS_IPV4) &&
-	(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
-      addr.set_family(AF_INET);
-      addrs->v.push_back(addr);
-    }
-    if (ipv & CEPH_PICK_ADDRESS_IPV6) {
-      addr.set_family(AF_INET6);
-      addrs->v.push_back(addr);
-    }
-    if ((ipv & CEPH_PICK_ADDRESS_IPV4) &&
-	!(flags & CEPH_PICK_ADDRESS_PREFER_IPV4)) {
-      addr.set_family(AF_INET);
-      addrs->v.push_back(addr);
+    for (auto pick_mask : {CEPH_PICK_ADDRESS_IPV4, CEPH_PICK_ADDRESS_IPV6}) {
+      if (ipv & pick_mask) {
+        addr.set_family(pick_mask == CEPH_PICK_ADDRESS_IPV4 ? AF_INET : AF_INET6);
+        addrs->v.push_back(addr);
+      }
     }
   }
 
+  std::sort(addrs->v.begin(), addrs->v.end(),
+	    [flags] (entity_addr_t& lhs, entity_addr_t& rhs) {
+	      if (flags & CEPH_PICK_ADDRESS_PREFER_IPV4) {
+		return lhs.is_ipv4() && rhs.is_ipv6();
+	      } else {
+		return lhs.is_ipv6() && rhs.is_ipv4();
+	      }
+	    });
+
   // msgr2 or legacy or both?
   if (msgrv == (CEPH_PICK_ADDRESS_MSGR1 | CEPH_PICK_ADDRESS_MSGR2)) {
     vector<entity_addr_t> v;
-- 
2.39.5