Reimplements net::AddressList without struct addrinfo.

net/socket/tcp_client_socket_libevent.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_client_socket.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <netdb.h>
 #include <sys/socket.h>
@@ skipping 109 matching lines @@
 
 //-----------------------------------------------------------------------------
 
 TCPClientSocketLibevent::TCPClientSocketLibevent(
     const AddressList& addresses,
     net::NetLog* net_log,
     const net::NetLog::Source& source)
     : socket_(kInvalidSocket),
       bound_socket_(kInvalidSocket),
       addresses_(addresses),
-      current_ai_(NULL),
+      current_address_index_(static_cast<size_t>(-1)),
       read_watcher_(this),
       write_watcher_(this),
       next_connect_state_(CONNECT_STATE_NONE),
       connect_os_error_(0),
       net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)),
       previously_disconnected_(false),
       use_tcp_fastopen_(false),
       tcp_fastopen_connected_(false),
       num_bytes_read_(0) {
   scoped_refptr<NetLog::EventParameters> params;
@@ skipping 15 matching lines @@
 
   int error = SetupSocket(socket);
   if (error)
     return MapSystemError(error);
 
   socket_ = socket;
 
   // This is to make GetPeerAddress() work. It's up to the caller ensure
   // that |address_| contains a reasonable address for this
   // socket. (i.e. at least match IPv4 vs IPv6!).
-  current_ai_ = addresses_.head();
+  current_address_index_ = 0;
   use_history_.set_was_ever_connected();
 
   return OK;
 }
 
 int TCPClientSocketLibevent::Bind(const IPEndPoint& address) {
-  if (current_ai_ != NULL || bind_address_.get()) {
+  if (current_address_index_ < addresses_.size() || bind_address_.get()) {

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

If you do my recommendation to change index to an "int", this will need to be
expanded to check >= 0 as well.

     // Cannot bind the socket if we are already bound 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|.
   int error = CreateSocket(address.GetFamily(), &bound_socket_);
   if (error)
     return MapSystemError(error);
 
-  if (HANDLE_EINTR(bind(bound_socket_, addr, addr_len))) {
+  if (HANDLE_EINTR(bind(bound_socket_, storage.addr, storage.addr_len))) {
     error = errno;
     if (HANDLE_EINTR(close(bound_socket_)) < 0)
       PLOG(ERROR) << "close";
     bound_socket_ = kInvalidSocket;
     return MapSystemError(error);
   }
 
   bind_address_.reset(new IPEndPoint(address));
 
   return 0;
@@ skipping 11 matching lines @@
 
   DCHECK(!waiting_connect());
 
   net_log_.BeginEvent(
       NetLog::TYPE_TCP_CONNECT,
       make_scoped_refptr(new AddressListNetLogParam(addresses_)));
 
   // 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) {
     // Synchronous operation not supported.
     DCHECK(!callback.is_null());
     write_callback_ = callback;
   } else {
     LogConnectCompletion(rv);
   }
 
@@ skipping 19 matching lines @@
         LOG(DFATAL) << "bad state";
         rv = ERR_UNEXPECTED;
         break;
     }
   } while (rv != ERR_IO_PENDING && next_connect_state_ != CONNECT_STATE_NONE);
 
   return rv;
 }
 
 int TCPClientSocketLibevent::DoConnect() {
-  DCHECK(current_ai_);
+  DCHECK_GE(current_address_index_, 0u);

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

nit: trivially evaluates to true since index is unsigned. If you follow my
recommendation to make it signed though, this will become necessary.

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

True. This is a leftover of my first attempt with signed int.

+  DCHECK_LT(current_address_index_, addresses_.size());
+  DCHECK_EQ(0, connect_os_error_);
 
-  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,
                       make_scoped_refptr(new NetLogStringParameter(
-                          "address", NetAddressToStringWithPort(current_ai_))));
+                          "address",
+                          endpoint.ToString())));
 
   next_connect_state_ = CONNECT_STATE_CONNECT_COMPLETE;
 
   if (bound_socket_ != kInvalidSocket) {
     DCHECK(bind_address_.get());
     socket_ = bound_socket_;
     bound_socket_ = kInvalidSocket;
   } else {
     // Create a non-blocking socket.
-    connect_os_error_ = CreateSocket(current_ai_->ai_family, &socket_);
+    connect_os_error_ = CreateSocket(endpoint.GetFamily(), &socket_);
     if (connect_os_error_)
       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 (HANDLE_EINTR(bind(socket_, addr, addr_len)))
+      if (HANDLE_EINTR(bind(socket_, storage.addr, storage.addr_len)))
         return MapSystemError(errno);
     }
   }
 
   // Connect the socket.
   if (!use_tcp_fastopen_) {
+    SockaddrStorage storage;
+    if (!endpoint.ToSockAddr(storage.addr, &storage.addr_len))

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

nit: This is duplicated in two locations. How about extracting this to the top
and always converting endpoint to a sockaddr.

+      return ERR_INVALID_ARGUMENT;
+
     connect_start_time_ = base::TimeTicks::Now();
-    if (!HANDLE_EINTR(connect(socket_, current_ai_->ai_addr,
-                              static_cast<int>(current_ai_->ai_addrlen)))) {
+    if (!HANDLE_EINTR(connect(socket_, storage.addr, storage.addr_len))) {
       // Connected without waiting!
       return OK;
     }
   } else {
     // With TCP FastOpen, we pretend that the socket is connected.
     DCHECK(!tcp_fastopen_connected_);
     return OK;
   }
 
   // Check if the connect() failed synchronously.
@@ skipping 27 matching lines @@
     connect_time_micros_ = base::TimeTicks::Now() - connect_start_time_;
     write_socket_watcher_.StopWatchingFileDescriptor();
     use_history_.set_was_ever_connected();
     return OK;  // Done!
   }
 
   // Close whatever partially connected socket we currently have.
   DoDisconnect();
 
   // Try to fall back to the next address in the list.
-  if (current_ai_->ai_next) {
+  if (current_address_index_ + 1 < addresses_.size()) {

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

The reason I don't like current_address_index being unsigned and initialized to
-1, is code like this can pass unexpectedly (i.e. -1 + 1 = 0, which is then less
than addresses_.size()). Believe it or not I have seen bugs of this sort before
so I am always cautious about unsigned arithmetic.

     next_connect_state_ = CONNECT_STATE_CONNECT;
-    current_ai_ = current_ai_->ai_next;
+    ++current_address_index_;
     return OK;
   }
 
   // Otherwise there is nothing to fall back to, so give up.
   return result;
 }
 
 void TCPClientSocketLibevent::Disconnect() {
   DCHECK(CalledOnValidThread());
 
   DoDisconnect();
-  current_ai_ = NULL;
+  current_address_index_ = addresses_.size();

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

This seems unnecessary... I would just delete it if it isn't clear that it is
needed. (I suspect before this was an attempt to make sure the pointer isn't
accidentally used after it may have been released?)

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

I was concerned that it was used to distinguish sockets that were disconnected,
but it seems there are other checks for that.

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

Actually sorry, I changed my mind!

I think it would be best to leave this as a reset to -1 (same as initial value)
to be safe.

It would be more appropriate to change that in a follow up CL, since it would be
a shame to have unnecessary fallout from this refactor.

Sorry for the extra work! (You can disregard this if you like).

 }
 
 void TCPClientSocketLibevent::DoDisconnect() {
   if (socket_ == kInvalidSocket)
     return;
 
   bool ok = read_socket_watcher_.StopWatchingFileDescriptor();
   DCHECK(ok);
   ok = write_socket_watcher_.StopWatchingFileDescriptor();
   DCHECK(ok);
   if (HANDLE_EINTR(close(socket_)) < 0)
     PLOG(ERROR) << "close";
   socket_ = kInvalidSocket;
   previously_disconnected_ = true;
 }
 
 bool TCPClientSocketLibevent::IsConnected() const {
   DCHECK(CalledOnValidThread());
 
   if (socket_ == kInvalidSocket || waiting_connect())
     return false;
 
   if (use_tcp_fastopen_ && !tcp_fastopen_connected_) {
     // With TCP FastOpen, we pretend that the socket is connected.
     // This allows GetPeerAddress() to return current_ai_ as the peer
     // address.  Since we don't fail over to the next address if
     // sendto() fails, current_ai_ is the only possible peer address.
-    CHECK(current_ai_);
+    CHECK_LT(current_address_index_, addresses_.size());
     return true;
   }
 
   // Check if connection is alive.
   char c;
   int rv = HANDLE_EINTR(recv(socket_, &c, 1, MSG_PEEK));
   if (rv == 0)
     return false;
   if (rv == -1 && errno != EAGAIN && errno != EWOULDBLOCK)
     return false;
@@ skipping 95 matching lines @@
 
   write_buf_ = buf;
   write_buf_len_ = buf_len;
   write_callback_ = callback;
   return ERR_IO_PENDING;
 }
 
 int TCPClientSocketLibevent::InternalWrite(IOBuffer* buf, int buf_len) {
   int nwrite;
   if (use_tcp_fastopen_ && !tcp_fastopen_connected_) {
+    SockaddrStorage storage;
+    if (!addresses_[current_address_index_].ToSockAddr(storage.addr,

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

I have seen this in a couple places now. I think it can be simplified. We should
only ever call this once.

Here is how:

Inside of DoConnect() save the sockaddr to a member variable called
current_sockaddr_. Everywhere else that needs the sockaddr for current address
would use that.

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

Not sure. |current_sockaddr_| would need to be a SockaddrStorage and it'd only
get used in two places.

On the other hand, not sure if really need to check ToSockAddr result here,
since non-INET* should already be filtered out in AddressList.

+                                                       &storage.addr_len)) {
+      return ERR_INVALID_ARGUMENT;
+    }
+
     // We have a limited amount of data to send in the SYN packet.
     int kMaxFastOpenSendLength = 1420;
 
     buf_len = std::min(kMaxFastOpenSendLength, buf_len);
 
     int flags = 0x20000000;  // Magic flag to enable TCP_FASTOPEN
     nwrite = HANDLE_EINTR(sendto(socket_,
                                  buf->data(),
                                  buf_len,
                                  flags,
-                                 current_ai_->ai_addr,
-                                 static_cast<int>(current_ai_->ai_addrlen)));
+                                 storage.addr,
+                                 storage.addr_len));
     tcp_fastopen_connected_ = true;
 
     if (nwrite < 0) {
       // Non-blocking mode is returning EINPROGRESS rather than EAGAIN.
       if (errno == EINPROGRESS)
          errno = EAGAIN;
 
       // Unlike "normal" nonblocking sockets, the data is already queued,
       // so tell the app that we've consumed it.
       return buf_len;
@@ skipping 24 matching lines @@
 
 void TCPClientSocketLibevent::LogConnectCompletion(int net_error) {
   if (net_error == OK)
     UpdateConnectionTypeHistograms(CONNECTION_ANY);
 
   if (net_error != OK) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_TCP_CONNECT, net_error);
     return;
   }
 
-  struct sockaddr_storage source_address;
-  socklen_t addrlen = sizeof(source_address);
-  int rv = getsockname(
-      socket_, reinterpret_cast<struct sockaddr*>(&source_address), &addrlen);
+  SockaddrStorage storage;
+  int rv = getsockname(socket_, storage.addr, &storage.addr_len);
   if (rv != 0) {
     PLOG(ERROR) << "getsockname() [rv: " << rv << "] error: ";
     NOTREACHED();
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_TCP_CONNECT, rv);
     return;
   }
 
   const std::string source_address_str =
-      NetAddressToStringWithPort(
-          reinterpret_cast<const struct sockaddr*>(&source_address),
-          sizeof(source_address));
+      NetAddressToStringWithPort(storage.addr, storage.addr_len);
   net_log_.EndEvent(NetLog::TYPE_TCP_CONNECT,
                     make_scoped_refptr(new NetLogStringParameter(
                         "source address",
                         source_address_str)));
 }
 
 void TCPClientSocketLibevent::DoReadCallback(int rv) {
   DCHECK_NE(rv, ERR_IO_PENDING);
   DCHECK(!read_callback_.is_null());
 
@@ skipping 88 matching lines @@
     write_socket_watcher_.StopWatchingFileDescriptor();
     DoWriteCallback(result);
   }
 }
 
 int TCPClientSocketLibevent::GetPeerAddress(AddressList* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
   if (!IsConnected())
     return ERR_SOCKET_NOT_CONNECTED;
-  *address = AddressList::CreateByCopyingFirstAddress(current_ai_);
+  *address = AddressList(addresses_[current_address_index_]);
   return OK;
 }
 
 int TCPClientSocketLibevent::GetLocalAddress(IPEndPoint* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
   if (!IsConnected())
     return ERR_SOCKET_NOT_CONNECTED;
 
-  struct sockaddr_storage addr_storage;
-  socklen_t addr_len = sizeof(addr_storage);
-  struct sockaddr* addr = reinterpret_cast<struct sockaddr*>(&addr_storage);
-  if (getsockname(socket_, addr, &addr_len))
+  SockaddrStorage storage;
+  if (getsockname(socket_, storage.addr, &storage.addr_len))
     return MapSystemError(errno);
-  if (!address->FromSockAddr(addr, addr_len))
+  if (!address->FromSockAddr(storage.addr, storage.addr_len))
     return ERR_FAILED;
 
   return OK;
 }
 
 const BoundNetLog& TCPClientSocketLibevent::NetLog() const {
   return net_log_;
 }
 
 void TCPClientSocketLibevent::SetSubresourceSpeculation() {
@@ skipping 18 matching lines @@
 
 base::TimeDelta TCPClientSocketLibevent::GetConnectTimeMicros() const {
   return connect_time_micros_;
 }
 
 NextProto TCPClientSocketLibevent::GetNegotiatedProtocol() const {
   return kProtoUnknown;
 }
 
 }  // namespace net