Reimplements net::AddressList without struct addrinfo.

net/socket/tcp_client_socket_win.cc

Index: net/socket/tcp_client_socket_win.cc
diff --git a/net/socket/tcp_client_socket_win.cc b/net/socket/tcp_client_socket_win.cc
index 28b0eeffbb3fbf0e8b207fd7dbc41f1120fcf756..d6918e6e7a6095e6a41bde0cf367e5c551eb0d42 100644
--- a/net/socket/tcp_client_socket_win.cc
+++ b/net/socket/tcp_client_socket_win.cc
@@ -20,7 +20,6 @@
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
 #include "net/base/network_change_notifier.h"
-#include "net/base/sys_addrinfo.h"
 #include "net/base/winsock_init.h"
 #include "net/base/winsock_util.h"
 
@@ -317,7 +316,7 @@ TCPClientSocketWin::TCPClientSocketWin(const AddressList& addresses,
     : socket_(INVALID_SOCKET),
       bound_socket_(INVALID_SOCKET),
       addresses_(addresses),
-      current_ai_(NULL),
+      current_address_index_(static_cast<size_t>(-1)),
       waiting_read_(false),
       waiting_write_(false),
       next_connect_state_(CONNECT_STATE_NONE),
@@ -349,22 +348,20 @@ int TCPClientSocketWin::AdoptSocket(SOCKET socket) {
 
   core_ = new Core(this);
 
-  current_ai_ = addresses_.head();
+  current_address_index_ = 0;
   use_history_.set_was_ever_connected();
 
   return OK;
 }
 
 int TCPClientSocketWin::Bind(const IPEndPoint& address) {
-  if (current_ai_ != NULL || bind_address_.get()) {
+  if (current_address_index_ < addresses_.size() || bind_address_.get()) {
     // Cannot bind the socket if we are already connected or connecting.
     return ERR_UNEXPECTED;
   }
 
-  sockaddr_storage addr_storage;
-  sockaddr* addr = reinterpret_cast<struct sockaddr*>(&addr_storage);
-  size_t addr_len = sizeof(addr_storage);
-  if (!address.ToSockAddr(addr, &addr_len))
+  SockaddrStorage storage;
+  if (!address.ToSockAddr(storage.addr, &storage.addr_len))
     return ERR_INVALID_ARGUMENT;
 
   // Create |bound_socket_| and try to bound it to |address|.
@@ -372,7 +369,7 @@ int TCPClientSocketWin::Bind(const IPEndPoint& address) {
   if (error)
     return MapSystemError(error);
 
-  if (bind(bound_socket_, addr, addr_len)) {
+  if (bind(bound_socket_, storage.addr, storage.addr_len)) {
     error = errno;
     if (closesocket(bound_socket_) < 0)
       PLOG(ERROR) << "closesocket";
@@ -402,7 +399,7 @@ int TCPClientSocketWin::Connect(const CompletionCallback& callback) {
   // We will try to connect to each address in addresses_. Start with the
   // first one in the list.
   next_connect_state_ = CONNECT_STATE_CONNECT;
-  current_ai_ = addresses_.head();
+  current_address_index_ = 0;
 
   int rv = DoConnectLoop(OK);
   if (rv == ERR_IO_PENDING) {
@@ -443,18 +440,19 @@ int TCPClientSocketWin::DoConnectLoop(int result) {
 }
 
 int TCPClientSocketWin::DoConnect() {
-  const struct addrinfo* ai = current_ai_;
-  DCHECK(ai);
+  DCHECK_LT(current_address_index_, addresses_.size());
   DCHECK_EQ(0, connect_os_error_);
 
+  const IPEndPoint& endpoint = addresses_[current_address_index_];
+
   if (previously_disconnected_) {
     use_history_.Reset();
     previously_disconnected_ = false;
   }
 
   net_log_.BeginEvent(NetLog::TYPE_TCP_CONNECT_ATTEMPT,
-                      new NetLogStringParameter(
-                          "address", NetAddressToStringWithPort(current_ai_)));
+                      new NetLogStringParameter("address",
+                                                endpoint.ToString()));
 
   next_connect_state_ = CONNECT_STATE_CONNECT_COMPLETE;
 
@@ -463,17 +461,15 @@ int TCPClientSocketWin::DoConnect() {
     socket_ = bound_socket_;
     bound_socket_ = INVALID_SOCKET;
   } else {
-    connect_os_error_ = CreateSocket(ai->ai_family, &socket_);
+    connect_os_error_ = CreateSocket(endpoint.GetFamily(), &socket_);

[eroman, 2012/05/04 01:08:41]

Side comment: When we refactor GetFamily() to return an enum rather than int we
need to be very careful about callsites like this, since they won't result in a
compile break. Might be worth changing the name while doing the refactor to
catch this.

[szym, 2012/05/04 02:38:29]

Yup. To avoid this pitfall when converting RequestPriority order I changed that
enum to a class to catch all call sites.

     if (connect_os_error_ != 0)
       return MapSystemError(connect_os_error_);
 
     if (bind_address_.get()) {
-      sockaddr_storage addr_storage;
-      sockaddr* addr = reinterpret_cast<struct sockaddr*>(&addr_storage);
-      size_t addr_len = sizeof(addr_storage);
-      if (!bind_address_->ToSockAddr(addr, &addr_len))
+      SockaddrStorage storage;
+      if (!bind_address_->ToSockAddr(storage.addr, &storage.addr_len))
         return ERR_INVALID_ARGUMENT;
-      if (bind(socket_, addr, addr_len))
+      if (bind(socket_, storage.addr, storage.addr_len))
         return MapSystemError(errno);
     }
   }
@@ -484,8 +480,11 @@ int TCPClientSocketWin::DoConnect() {
   // Our connect() and recv() calls require that the socket be non-blocking.
   WSAEventSelect(socket_, core_->read_overlapped_.hEvent, FD_CONNECT);
 
+  SockaddrStorage storage;
+  if (!endpoint.ToSockAddr(storage.addr, &storage.addr_len))
+    return ERR_INVALID_ARGUMENT;

[eroman, 2012/05/04 01:08:41]

I wander if these failure are possible? When building AddressList we should be
very explicit about what values are put in. I guess I need to read ToSockAddr to
see the failure cases.

Would be nice if this function didn't have any failures to check, and perhaps
just had a crash assertion if something really wonky happened.

[szym, 2012/05/04 02:38:29]

Agreed that ToSockAddr should ideally not return false. But even if we ignore
non-INET* junk, IPEndPoint could be in one of three states: empty, INET, INET6.
IPEndPoint().ToSockAddr(...) ?

   connect_start_time_ = base::TimeTicks::Now();
-  if (!connect(socket_, ai->ai_addr, static_cast<int>(ai->ai_addrlen))) {
+  if (!connect(socket_, storage.addr, storage.addr_len)) {
     // Connected without waiting!
     //
     // The MSDN page for connect says:
@@ -532,9 +531,9 @@ int TCPClientSocketWin::DoConnectComplete(int result) {
   DoDisconnect();
 
   // Try to fall back to the next address in the list.
-  if (current_ai_->ai_next) {
+  if (current_address_index_ + 1 < addresses_.size()) {
     next_connect_state_ = CONNECT_STATE_CONNECT;
-    current_ai_ = current_ai_->ai_next;
+    ++current_address_index_;
     return OK;
   }
 
@@ -544,7 +543,7 @@ int TCPClientSocketWin::DoConnectComplete(int result) {
 
 void TCPClientSocketWin::Disconnect() {
   DoDisconnect();
-  current_ai_ = NULL;
+  current_address_index_ = addresses_.size();

[eroman, 2012/05/04 01:08:41]

Same comment as earlier, I wander if this is actually necessary.

The only case I can see where this matters if if someone calls GetPeerAddress()
after disconnecting the socket.

However as this code stands, it will already early return based on not being
connected.

[szym, 2012/05/04 02:38:29]

Agreed. Will fix.

[eroman, 2012/05/04 19:37:36]

Sorry, I suggest leaving this in place, as a reset to -1.

 }
 
 void TCPClientSocketWin::DoDisconnect() {
@@ -629,7 +628,7 @@ int TCPClientSocketWin::GetPeerAddress(AddressList* address) const {
   DCHECK(address);
   if (!IsConnected())
     return ERR_SOCKET_NOT_CONNECTED;
-  *address = AddressList::CreateByCopyingFirstAddress(current_ai_);
+  *address = AddressList(addresses_[current_address_index_]);
   return OK;
 }