Clean up Machine implementation for issue #4159.

include/tscpp/util/TextView.h

@@ -1265,6 +1265,32 @@ namespace literals
   constexpr ts::TextView operator"" _tv(const char *s, size_t n) { return {s, n}; }
 } // namespace literals
 
+/** Functor for STL containers that need caseless comparisons of standard string types.
+ *
+ * For example a @c std::set of strings with caseless comparison would be
+ *
+ * @code
+ * std::set<std::string, ts::caseless_compare> strings;
+ * @endcode
+ */
+struct caseless_compare {
+  bool
+  operator()(std::string_view const &lhs, std::string_view const &rhs) const
+  {
+    return strcasecmp(lhs, rhs);
+  }
+  bool
+  operator()(TextView const &lhs, TextView const &rhs) const
+  {
+    return strcasecmp(lhs, rhs);
+  }
+  bool
+  operator()(std::string const &lhs, std::string const &rhs) const
+  {
+    return strcasecmp(lhs, rhs);
+  }
+};
+
 } // namespace ts
 
 namespace std

iocore/utils/I_Machine.h

@@ -33,9 +33,10 @@
 #include "tscore/ink_inet.h"
 #include "tscore/ink_uuid.h"
 
-#include <unordered_map>
 #include <unordered_set>
 #include <memory>
+#include <string_view>
+#include <tscpp/util/TextView.h>
 
 /**
   The Machine is a simple place holder for the hostname and the ip
@@ -52,21 +53,14 @@
 
  */
 struct Machine {
-  typedef Machine self; ///< Self reference type.
-
-  char *hostname;   // name of the internet host
-  int hostname_len; // size of the string pointed to by hostname
+  using self_type = Machine; ///< Self reference type.
 
   IpEndpoint ip;  ///< Preferred IP address of the host (network order)
   IpEndpoint ip4; ///< IPv4 address if present.
   IpEndpoint ip6; ///< IPv6 address if present.
 
-  ip_text_buffer ip_string; // IP address of the host as a string.
-  int ip_string_len;
-
-  char ip_hex_string[TS_IP6_SIZE * 2 + 1]; ///< IP address as hex string
-  int ip_hex_string_len;
-
+  std::string host_name;
+  std::string ip_hex_string; ///< IP address as hex string
   ATSUuid uuid;
 
   ~Machine();
@@ -77,22 +71,22 @@ struct Machine {
       @note This must be called before called @c instance so that the
       singleton is not @em inadvertently default initialized.
   */
-  static self *init(char const *name     = nullptr, ///< Host name of the machine.
-                    sockaddr const *addr = nullptr  ///< Primary IP address of the machine.
+  static self_type *init(char const *name     = nullptr, ///< Host name of the machine.
+                         sockaddr const *addr = nullptr  ///< Primary IP address of the machine.
   );
   /// @return The global instance of this class.
-  static self *instance();
-  bool is_self(const char *name);
-  bool is_self(const char *name, int name_len);
-  bool is_self(const IpAddr *ipaddr);
+  static self_type *instance();
+  bool is_self(std::string_view name);
+  bool is_self(std::string const &name);
+  bool is_self(IpAddr const &ipaddr);
   bool is_self(struct sockaddr const *addr);
-  void insert_id(char *id);
-  void insert_id(IpAddr *ipaddr);
+  void insert_id(char const *id);
+  void insert_id(IpAddr const &ipaddr);
 
 protected:
   Machine(char const *hostname, sockaddr const *addr);
 
-  static self *_instance; ///< Singleton for the class.
-  std::unordered_set<std::string> machine_id_strings;
-  std::unordered_map<std::string, IpAddr *> machine_id_ipaddrs;
+  static self_type *_instance; ///< Singleton for the class.
+  std::unordered_set<std::string, std::hash<std::string>, ts::caseless_compare> machine_id_strings;
+  std::unordered_set<IpAddr, IpAddr::Hasher> machine_id_ipaddrs;
 };

iocore/utils/Machine.cc

@@ -31,21 +31,6 @@
 #include <ifaddrs.h>
 #endif
 
-static void
-make_to_lower_case(const char *name, int name_len, char *lower_case_name, int buf_len)
-{
-  int i;
-
-  if (name_len > (buf_len - 1)) {
-    name_len = buf_len - 1;
-  }
-
-  for (i = 0; i < name_len; i++) {
-    lower_case_name[i] = ParseRules::ink_tolower(name[i]);
-  }
-  lower_case_name[i] = '\0';
-}
-
 // Singleton
 Machine *Machine::_instance = nullptr;
 
@@ -65,29 +50,23 @@ Machine::init(char const *name, sockaddr const *ip)
 }
 
 Machine::Machine(char const *the_hostname, sockaddr const *addr)
-  : hostname(nullptr), hostname_len(0), ip_string_len(0), ip_hex_string_len(0)
 {
-  char localhost[1024];
-  char ip_strbuf[INET6_ADDRSTRLEN];
   int status; // return for system calls.
-
-  ip_string[0]     = 0;
-  ip_hex_string[0] = 0;
-  ink_zero(ip);
-  ink_zero(ip4);
-  ink_zero(ip6);
+  ip_text_buffer ip_strbuf;
+  char localhost[1024];
 
   uuid.initialize(TS_UUID_V4);
   ink_release_assert(nullptr != uuid.getString()); // The Process UUID must be available on startup
 
-  localhost[sizeof(localhost) - 1] = 0; // ensure termination.
-
   if (!ats_is_ip(addr)) {
     if (!the_hostname) {
-      ink_release_assert(!gethostname(localhost, sizeof(localhost) - 1));
-      the_hostname = localhost;
+      // @c gethostname has a broken interface - there's no way to determine the actual size of
+      // the host name explicitly - the error case doesn't return the size. The standards based
+      // limit is 63, or 255 for a FQDN.
+      auto result = gethostname(localhost, sizeof(localhost));
+      ink_release_assert(result == 0);
+      host_name.assign(localhost, result);
     }
-    hostname = ats_strdup(the_hostname);
 
 #if HAVE_IFADDRS_H
     ifaddrs *ifa_addrs = nullptr;
@@ -98,7 +77,7 @@ Machine::Machine(char const *the_hostname, sockaddr const *addr)
     // you would expect. On a normal system with just two interfaces and
     // one address / interface the return count is 120. Stack space is
     // cheap so it's best to go big.
-    static const int N_REQ = 1024;
+    static constexpr int N_REQ = 1024;
     ifconf conf;
     ifreq req[N_REQ];
     if (0 <= s) {
@@ -111,7 +90,8 @@ Machine::Machine(char const *the_hostname, sockaddr const *addr)
 #endif
 
     if (0 != status) {
-      Warning("Unable to determine local host '%s' address information - %s", hostname, strerror(errno));
+      Warning("Unable to determine local host '%.*s' address information - %s", int(host_name.size()), host_name.data(),
+              strerror(errno));
     } else {
       // Loop through the interface addresses and prefer by type.
       enum {
@@ -172,8 +152,7 @@ Machine::Machine(char const *the_hostname, sockaddr const *addr)
           if (spot_type != LL && getnameinfo(ifip, ats_ip_size(ifip), localhost, sizeof(localhost) - 1, nullptr, 0, 0) == 0) {
             insert_id(localhost);
           }
-          IpAddr *ipaddr = new IpAddr(ifip);
-          insert_id(ipaddr);
+          insert_id(IpAddr(ifip));
           if (ats_is_ip4(ifip)) {
             if (spot_type > ip4_type) {
               ats_ip_copy(&ip4, ifip);
@@ -216,86 +195,53 @@ Machine::Machine(char const *the_hostname, sockaddr const *addr)
       ip_text_buffer ipbuff;
       Warning("Failed to find hostname for address '%s' - %s", ats_ip_ntop(addr, ipbuff, sizeof(ipbuff)), gai_strerror(status));
     } else {
-      hostname = ats_strdup(localhost);
+      host_name.assign(localhost);
     }
   }
 
-  hostname_len = hostname ? strlen(hostname) : 0;
-
-  ats_ip_ntop(&ip.sa, ip_string, sizeof(ip_string));
-  ip_string_len     = strlen(ip_string);
-  ip_hex_string_len = ats_ip_to_hex(&ip.sa, ip_hex_string, sizeof(ip_hex_string));
+  char hex_buff[TS_IP6_SIZE * 2 + 1];
+  ats_ip_to_hex(&ip.sa, hex_buff, sizeof(hex_buff));
+  ip_hex_string.assign(hex_buff);
 }
 
-Machine::~Machine()
-{
-  ats_free(hostname);
-  for (auto &machine_id_ipaddr : machine_id_ipaddrs) {
-    delete machine_id_ipaddr.second;
-  }
-}
+Machine::~Machine() {}
 
 bool
-Machine::is_self(const char *name)
+Machine::is_self(std::string const &name)
 {
-  return is_self(name, strlen(name));
+  return machine_id_strings.find(name) != machine_id_strings.end();
 }
 
 bool
-Machine::is_self(const char *name, int name_len)
+Machine::is_self(std::string_view name)
 {
-  char lower_case_name[TS_MAX_HOST_NAME_LEN + 1] = {0};
-
-  if (name_len == 0) {
-    return false;
-  }
-
-  make_to_lower_case(name, name_len, lower_case_name, sizeof(lower_case_name));
-
-  return machine_id_strings.find(lower_case_name) != machine_id_strings.end();
+  return this->is_self(std::string(name));
 }
 
 bool
-Machine::is_self(const IpAddr *ipaddr)
+Machine::is_self(IpAddr const &ipaddr)
 {
-  char string_value[INET6_ADDRSTRLEN + 1] = {0};
-
-  if (ipaddr == nullptr) {
-    return false;
-  }
-  ipaddr->toString(string_value, sizeof(string_value));
-  return machine_id_ipaddrs.find(string_value) != machine_id_ipaddrs.end();
+  return machine_id_ipaddrs.end() != machine_id_ipaddrs.find(ipaddr);
 }
 
 bool
 Machine::is_self(struct sockaddr const *addr)
 {
-  char string_value[INET6_ADDRSTRLEN + 1] = {0};
-
-  if (addr == nullptr) {
-    return false;
-  }
-  ats_ip_ntop(addr, string_value, sizeof(string_value));
-  return machine_id_ipaddrs.find(string_value) != machine_id_ipaddrs.end();
+  return machine_id_ipaddrs.find(IpAddr(addr)) != machine_id_ipaddrs.end();
 }
 
 void
-Machine::insert_id(char *id)
+Machine::insert_id(char const *id)
 {
-  char lower_case_name[TS_MAX_HOST_NAME_LEN + 1] = {0};
-
-  make_to_lower_case(id, strlen(id), lower_case_name, sizeof(lower_case_name));
-  machine_id_strings.emplace(lower_case_name);
+  machine_id_strings.emplace(id);
 }
 
 void
-Machine::insert_id(IpAddr *ipaddr)
+Machine::insert_id(IpAddr const &ipaddr)
 {
-  char string_value[INET6_ADDRSTRLEN + 1] = {0};
+  ip_text_buffer buff;
 
-  if (ipaddr != nullptr) {
-    ipaddr->toString(string_value, sizeof(string_value));
-    machine_id_strings.emplace(string_value);
-    machine_id_ipaddrs.emplace(string_value, ipaddr);
-  }
+  ipaddr.toString(buff, sizeof(buff));
+  machine_id_strings.emplace(buff);
+  machine_id_ipaddrs.emplace(ipaddr);
 }

proxy/ParentSelection.cc

@@ -394,7 +394,7 @@ ParentRecord::PreProcessParents(const char *val, const int line_num, char *buf,
       ink_assert(length < sizeof(fqdn));
       memset(fqdn, 0, sizeof(fqdn));
       strncpy(fqdn, token, length);
-      if (self_detect && machine->is_self(fqdn)) {
+      if (self_detect && machine->is_self(std::string_view(fqdn))) {
         if (self_detect == 1) {
           Debug("parent_select", "token: %s, matches this machine.  Removing self from parent list at line %d", fqdn, line_num);
           token = strtok_r(nullptr, PARENT_DELIMITERS, &savePtr);
@@ -405,7 +405,7 @@ ParentRecord::PreProcessParents(const char *val, const int line_num, char *buf,
         }
       }
     } else {
-      if (self_detect && machine->is_self(token)) {
+      if (self_detect && machine->is_self(std::string_view(token))) {
         if (self_detect == 1) {
           Debug("parent_select", "token: %s, matches this machine.  Removing self from parent list at line %d", token, line_num);
           token = strtok_r(nullptr, PARENT_DELIMITERS, &savePtr);

proxy/http/HttpTransactHeaders.cc

@@ -1008,7 +1008,7 @@ HttpTransactHeaders::add_forwarded_field_to_request(HttpTransact::State *s, HTTP
       hdr << "by=_" << m.uuid.getString();
     }
 
-    if (optSet[HttpForwarded::BY_IP] and (m.ip_string_len > 0)) {
+    if (optSet[HttpForwarded::BY_IP] and m.ip.isValid()) {
       if (hdr.size()) {
         hdr << ';';
       }

proxy/http/remap/Makefile.am

@@ -133,14 +133,14 @@ test_NextHopStrategyFactory_CPPFLAGS = \
 	@YAMLCPP_INCLUDES@
 
 test_NextHopStrategyFactory_LDADD = \
-  $(top_builddir)/src/tscpp/util/libtscpputil.la \
   $(top_builddir)/src/tscore/libtscore.la \
   $(top_builddir)/proxy/hdrs/libhdrs.a \
   $(top_builddir)/iocore/eventsystem/libinkevent.a \
   $(top_builddir)/lib/records/librecords_p.a \
   $(top_builddir)/proxy/logging/liblogging.a \
   $(top_builddir)/mgmt/libmgmt_p.la \
   $(top_builddir)/iocore/utils/libinkutils.a \
+  $(top_builddir)/src/tscpp/util/libtscpputil.la \
 	@YAMLCPP_LIBS@ \
 	@HWLOC_LIBS@
 
@@ -164,27 +164,27 @@ test_NextHopRoundRobin_CPPFLAGS = \
 	@YAMLCPP_INCLUDES@
 
 test_NextHopRoundRobin_LDADD = \
-  $(top_builddir)/src/tscpp/util/libtscpputil.la \
   $(top_builddir)/src/tscore/libtscore.la \
   $(top_builddir)/proxy/hdrs/libhdrs.a \
   $(top_builddir)/iocore/eventsystem/libinkevent.a \
   $(top_builddir)/lib/records/librecords_p.a \
   $(top_builddir)/proxy/logging/liblogging.a \
   $(top_builddir)/mgmt/libmgmt_p.la \
   $(top_builddir)/iocore/utils/libinkutils.a \
+  $(top_builddir)/src/tscpp/util/libtscpputil.la \
 	@YAMLCPP_LIBS@ \
 	@HWLOC_LIBS@
 
-test_NextHopRoundRobin_LDFLAGS = $(AM_LDFLAGS) -L$(top_builddir)/src/tscore/.libs -ltscore
+test_NextHopRoundRobin_LDFLAGS = $(AM_LDFLAGS) -L$(top_builddir)/src/tscore/.libs -ltscore -L$(top_builddir)/src/tscpp/util/.libs -ltscpputil
 
 test_NextHopRoundRobin_SOURCES = \
+	unit-tests/test_NextHopRoundRobin.cc \
+	unit-tests/nexthop_test_stubs.cc \
 	NextHopSelectionStrategy.cc \
 	NextHopStrategyFactory.cc \
 	NextHopRoundRobin.cc \
 	NextHopConsistentHash.cc \
-	NextHopHealthStatus.cc \
-	unit-tests/test_NextHopRoundRobin.cc \
-	unit-tests/nexthop_test_stubs.cc
+	NextHopHealthStatus.cc
 
 test_NextHopConsistentHash_CPPFLAGS = \
 	$(AM_CPPFLAGS) \
@@ -195,14 +195,14 @@ test_NextHopConsistentHash_CPPFLAGS = \
 	@YAMLCPP_INCLUDES@
 
 test_NextHopConsistentHash_LDADD = \
-  $(top_builddir)/src/tscpp/util/libtscpputil.la \
   $(top_builddir)/src/tscore/libtscore.la \
   $(top_builddir)/proxy/hdrs/libhdrs.a \
   $(top_builddir)/iocore/eventsystem/libinkevent.a \
   $(top_builddir)/lib/records/librecords_p.a \
   $(top_builddir)/proxy/logging/liblogging.a \
   $(top_builddir)/mgmt/libmgmt_p.la \
   $(top_builddir)/iocore/utils/libinkutils.a \
+  $(top_builddir)/src/tscpp/util/libtscpputil.la \
 	@YAMLCPP_LIBS@ \
 	@HWLOC_LIBS@
 

proxy/http/remap/NextHopConsistentHash.cc

@@ -292,7 +292,7 @@ NextHopConsistentHash::findNextHop(TSHttpTxn txnp, void *ih, time_t now)
         result->first_choice_status = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
         // if peering and the selected host is myself, change rings and search for an upstream
         // parent.
-        if (ring_mode == NH_PEERING_RING && machine->is_self(pRec->hostname.c_str())) {
+        if (ring_mode == NH_PEERING_RING && machine->is_self(pRec->hostname)) {
           // switch to the upstream ring.
           cur_ring = 1;
           continue;