Apply suggestions from code review

Co-authored-by: Dan Wang <[email protected]>

src/runtime/rpc/rpc_address.cpp

@@ -27,9 +27,7 @@
 #include "runtime/rpc/rpc_address.h"
 
 #include <arpa/inet.h>
-#include <errno.h>
 #include <ifaddrs.h>
-#include <netdb.h>
 #include <netinet/in.h>
 #include <string.h>
 #include <sys/socket.h>
@@ -39,11 +37,32 @@
 #include "utils/fixed_size_buffer_pool.h"
 #include "utils/fmt_logging.h"
 #include "utils/ports.h"
-#include "utils/safe_strerror_posix.h"
 #include "utils/string_conv.h"
 #include "utils/strings.h"
 
 namespace dsn {
+/*static*/
+error_s rpc_address::GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
+{
+    addrinfo *res = nullptr;
+    const int rc = getaddrinfo(hostname.c_str(), nullptr, &hints, &res);
+    const int err = errno; // preserving the errno from the getaddrinfo() call
+    AddrInfo result(res, ::freeaddrinfo);
+    if (rc != 0) {
+        if (rc == EAI_SYSTEM) {
+            LOG_ERROR(
+                "getaddrinfo failed, name = {}, err = {}", hostname, utils::safe_strerror(rc));
+            return error_s::make(ERR_NETWORK_FAILURE, utils::safe_strerror(err));
+        }
+        LOG_ERROR("getaddrinfo failed, name = {}, err = {}", hostname, gai_strerror(rc));
+        return error_s::make(ERR_NETWORK_FAILURE, gai_strerror(rc));
+    }
+
+    if (info != nullptr) {
+        info->swap(result);
+    }
+    return error_s::ok();
+}
 
 const rpc_address rpc_address::s_invalid_address;
 
@@ -54,23 +73,16 @@ uint32_t rpc_address::ipv4_from_host(const char *name)
     memset(&hints, 0, sizeof(hints));
     hints.ai_family = AF_INET;
     hints.ai_socktype = SOCK_STREAM;
-    addrinfo *res = nullptr;
-
-    const int rc = getaddrinfo(name, nullptr, &hints, &res);
-    const int err = errno; // preserving the errno from the getaddrinfo() call
-
-    if (rc != 0) {
-        if (rc == EAI_SYSTEM) {
-            LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, utils::safe_strerror(err));
-        }
-        LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, gai_strerror(err));
+    AddrInfo result;
+    if (dsn_unlikely(!GetAddrInfo(name, hints, &result).is_ok())) {
         return 0;
     }
-
-    CHECK(res->ai_family == AF_INET, "");
-    struct sockaddr_in *ipv4 = (struct sockaddr_in *)res->ai_addr;
+    CHECK_EQ(result.get()->ai_family, AF_INET);
+    auto *ipv4 = reinterpret_cast<struct sockaddr_in *>(result.get()->ai_addr);
     // converts from network byte order to host byte order
-    return (uint32_t)ntohl(ipv4->sin_addr.s_addr);
+    uint32_t iphost = ntohl(ipv4->sin_addr.s_addr);
+    freeaddrinfo(result.get());
+    return iphost;
 }
 
 /*static*/

src/runtime/rpc/rpc_address.h

@@ -27,6 +27,8 @@
 #pragma once
 
 #include <arpa/inet.h> // IWYU pragma: keep
+#include <errno.h>
+#include <netdb.h>
 #include <cstddef>
 #include <cstdint>
 // IWYU pragma: no_include <experimental/string_view>
@@ -35,6 +37,8 @@
 #include <string>
 #include <string_view>
 
+#include "utils/errors.h"
+#include "utils/safe_strerror_posix.h"
 #include "utils/fmt_utils.h"
 
 namespace apache {
@@ -54,6 +58,8 @@ USER_DEFINED_ENUM_FORMATTER(dsn_host_type_t)
 
 namespace dsn {
 
+using AddrInfo = std::unique_ptr<addrinfo, std::function<void(addrinfo *)>>;
+
 class rpc_group_address;
 
 class rpc_address
@@ -64,6 +70,7 @@ class rpc_address
     static bool is_site_local_address(uint32_t ip_net);
     static uint32_t ipv4_from_host(const char *hostname);
     static uint32_t ipv4_from_network_interface(const char *network_interface);
+    static error_s GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info);
 
     ~rpc_address();
 

src/runtime/rpc/rpc_host_port.cpp

@@ -39,30 +39,6 @@ namespace dsn {
 
 const host_port host_port::s_invalid_host_port;
 
-namespace {
-
-using AddrInfo = std::unique_ptr<addrinfo, std::function<void(addrinfo *)>>;
-
-error_s GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
-{
-    addrinfo *res = nullptr;
-    const int rc = getaddrinfo(hostname.c_str(), nullptr, &hints, &res);
-    const int err = errno; // preserving the errno from the getaddrinfo() call
-    AddrInfo result(res, ::freeaddrinfo);
-    if (rc != 0) {
-        if (rc == EAI_SYSTEM) {
-            return error_s::make(ERR_NETWORK_FAILURE, utils::safe_strerror(err));
-        }
-        return error_s::make(ERR_NETWORK_FAILURE, gai_strerror(rc));
-    }
-
-    if (info != nullptr) {
-        info->swap(result);
-    }
-    return error_s::ok();
-}
-}
-
 host_port::host_port(std::string host, uint16_t port)
     : _host(std::move(host)), _port(port), _type(HOST_TYPE_IPV4)
 {
@@ -100,11 +76,7 @@ bool host_port::from_string(const std::string &s)
         return false;
     }
 
-    struct addrinfo hints;
-    memset(&hints, 0, sizeof(hints));
-    hints.ai_family = AF_INET;
-    hints.ai_socktype = SOCK_STREAM;
-    if (dsn_unlikely(!GetAddrInfo(_host, hints, nullptr))) {
+    if (dsn_unlikely(rpc_address::ipv4_from_host(_host.c_str()) == 0)) {
         return false;
     }
 
@@ -194,7 +166,7 @@ error_s host_port::resolve_addresses(std::vector<rpc_address> &addresses) const
     hints.ai_family = AF_INET;
     hints.ai_socktype = SOCK_STREAM;
     AddrInfo result;
-    RETURN_NOT_OK(GetAddrInfo(_host, hints, &result));
+    RETURN_NOT_OK(rpc_address::GetAddrInfo(_host, hints, &result));
 
     // DNS may return the same host multiple times. We want to return only the unique
     // addresses, but in the same order as DNS returned them. To do so, we keep track
@@ -212,6 +184,7 @@ error_s host_port::resolve_addresses(std::vector<rpc_address> &addresses) const
         }
     }
 
+    freeaddrinfo(result.get());
     if (result_addresses.empty()) {
         return error_s::make(dsn::ERR_NETWORK_FAILURE,
                              fmt::format("can not resolve host_port {}.", to_string()));