Miscelaneous cleanups to AddressList to make it harder to mis-use.

 - Removed all destructive non-const member functions -- these were dangerous since if you called them without first making a copy of the AddressList, it could mutate earlier copies.

 - Made AddressList::Data::head const, so new code added to AddressList cannot inadvertently introduce such dangerous mutations (won't compile).

 - Moved the non-trivial constructors and assign methods into factory methods (for added readability)

 - Removed the bool parameter from Copy (for added readability).

Review URL: http://codereview.chromium.org/6880302

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@85090 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 55 insertions, 52 deletions

diff --git a/net/base/address_list.cc b/net/base/address_list.cc
index 1804736..70b94b2 100644
--- a/net/base/address_list.cc
+++ b/net/base/address_list.cc
@@ -22,22 +22,16 @@ char* do_strdup(const char* src) {
 #endif
 }
 
-// Assign the port for all addresses in the list.
-void SetPortRecursive(struct addrinfo* info, int port) {
-  uint16* port_field = GetPortFieldFromAddrinfo(info);
-  if (port_field)
-    *port_field = htons(port);
-
-  // Assign recursively.
-  if (info->ai_next)
-    SetPortRecursive(info->ai_next, port);
-}
-
 }  // namespace
 
 struct AddressList::Data : public base::RefCountedThreadSafe<Data> {
   Data(struct addrinfo* ai, bool is_system_created);
-  struct addrinfo* head;
+
+  // This variable is const since it should NOT be mutated. Since
+  // Data is reference counted, this |head| could be shared by multiple
+  // instances of AddressList, hence we need to be careful not to mutate
+  // it from any one instance.
+  const struct addrinfo * const head;
 
   // Indicates which free function to use for |head|.
   bool is_system_created;
@@ -51,8 +45,18 @@ struct AddressList::Data : public base::RefCountedThreadSafe<Data> {
 AddressList::AddressList() {
 }
 
-AddressList::AddressList(const IPAddressNumber& address, int port,
-                         bool canonicalize_name) {
+// static
+AddressList AddressList::CreateFromIPAddress(
+      const IPAddressNumber& address,
+      uint16 port) {
+  return CreateFromIPAddressWithCname(address, port, false);
+}
+
+// static
+AddressList AddressList::CreateFromIPAddressWithCname(
+    const IPAddressNumber& address,
+    uint16 port,
+    bool canonicalize_name) {
   struct addrinfo* ai = new addrinfo;
   memset(ai, 0, sizeof(addrinfo));
   ai->ai_socktype = SOCK_STREAM;
@@ -94,8 +98,8 @@ AddressList::AddressList(const IPAddressNumber& address, int port,
     std::string name = NetAddressToString(ai);
     ai->ai_canonname = do_strdup(name.c_str());
   }
-  data_ = new Data(ai, false /*is_system_created*/);
-  SetPort(port);
+  SetPortForAllAddrinfos(ai, port);
+  return AddressList(new Data(ai, false /*is_system_created*/));
 }
 
 AddressList::AddressList(const AddressList& addresslist)
@@ -110,24 +114,30 @@ AddressList& AddressList::operator=(const AddressList& addresslist) {
   return *this;
 }
 
-void AddressList::Adopt(struct addrinfo* head) {
-  data_ = new Data(head, true /*is_system_created*/);
+// static
+AddressList AddressList::CreateByAdoptingFromSystem(struct addrinfo* head) {
+  return AddressList(new Data(head, true /*is_system_created*/));
 }
 
-void AddressList::Copy(const struct addrinfo* head, bool recursive) {
-  data_ = new Data(CreateCopyOfAddrinfo(head, recursive),
-                   false /*is_system_created*/);
+// static
+AddressList AddressList::CreateByCopying(const struct addrinfo* head) {
+  return AddressList(new Data(CreateCopyOfAddrinfo(head, true /*recursive*/),
+                              false /*is_system_created*/));
+}
+
+// static
+AddressList AddressList::CreateByCopyingFirstAddress(
+    const struct addrinfo* head) {
+  return AddressList(new Data(CreateCopyOfAddrinfo(head, false /*recursive*/),
+                              false /*is_system_created*/));
 }
 
 void AddressList::Append(const struct addrinfo* head) {
   DCHECK(head);
-  struct addrinfo* new_head;
-  if (data_->is_system_created) {
-    new_head = CreateCopyOfAddrinfo(data_->head, true);
-    data_ = new Data(new_head, false /*is_system_created*/);
-  } else {
-    new_head = data_->head;
-  }
+  // Always create a copy, since the Data might be shared across instances.
+  struct addrinfo* new_head = CreateCopyOfAddrinfo(data_->head, true);
+  data_ = new Data(new_head, false /*is_system_created*/);
+
   // Find the end of current linked list and append new data there.
   struct addrinfo* copy_ptr = new_head;
   while (copy_ptr->ai_next)
@@ -146,25 +156,18 @@ void AddressList::Append(const struct addrinfo* head) {
   }
 }
 
-void AddressList::SetPort(int port) {
-  SetPortRecursive(data_->head, port);
+void AddressList::SetPort(uint16 port) {
+  // NOTE: we need to be careful not to mutate the reference-counted data,
+  // since it might be shared by other AddressLists.
+  struct addrinfo* head = CreateCopyOfAddrinfo(data_->head, true);
+  SetPortForAllAddrinfos(head, port);
+  data_ = new Data(head, false /*is_system_created*/);
 }
 
-int AddressList::GetPort() const {
+uint16 AddressList::GetPort() const {
   return GetPortFromAddrinfo(data_->head);
 }
 
-void AddressList::SetFrom(const AddressList& src, int port) {
-  if (src.GetPort() == port) {
-    // We can reference the data from |src| directly.
-    *this = src;
-  } else {
-    // Otherwise we need to make a copy in order to change the port number.
-    Copy(src.head(), true);
-    SetPort(port);
-  }
-}
-
 bool AddressList::GetCanonicalName(std::string* canonical_name) const {
   DCHECK(canonical_name);
   if (!data_ || !data_->head->ai_canonname)
@@ -173,10 +176,6 @@ bool AddressList::GetCanonicalName(std::string* canonical_name) const {
   return true;
 }
 
-void AddressList::Reset() {
-  data_ = NULL;
-}
-
 const struct addrinfo* AddressList::head() const {
   if (!data_)
     return NULL;
@@ -186,7 +185,7 @@ const struct addrinfo* AddressList::head() const {
 AddressList::AddressList(Data* data) : data_(data) {}
 
 // static
-AddressList* AddressList::CreateAddressListFromSockaddr(
+AddressList AddressList::CreateFromSockaddr(
     const struct sockaddr* address,
     socklen_t address_length,
     int socket_type,
@@ -223,7 +222,7 @@ AddressList* AddressList::CreateAddressListFromSockaddr(
   ai->ai_addrlen = address_length;
   ai->ai_addr = reinterpret_cast<struct sockaddr*>(new char[address_length]);
   memcpy(ai->ai_addr, address, address_length);
-  return new AddressList(new Data(ai, false /*is_system_created*/));
+  return AddressList(new Data(ai, false /*is_system_created*/));
 }
 
 
@@ -233,12 +232,16 @@ AddressList::Data::Data(struct addrinfo* ai, bool is_system_created)
 }
 
 AddressList::Data::~Data() {
-  // Call either freeaddrinfo(head), or FreeCopyOfAddrinfo(head), depending on
-  // who created the data.
+  // Casting away the const is safe, since upon destruction we know that
+  // no one holds a reference to the data any more.
+  struct addrinfo* mutable_head = const_cast<struct addrinfo*>(head);
+
+  // Call either freeaddrinfo(head), or FreeMyAddrinfo(head), depending who
+  // created the data.
   if (is_system_created)
-    freeaddrinfo(head);
+    freeaddrinfo(mutable_head);
   else
-    FreeCopyOfAddrinfo(head);
+    FreeCopyOfAddrinfo(mutable_head);
 }
 
 }  // namespace net