Add support for dynamically allocated port in Forwarder2.

Dynamically allocated port is necessary for running the net_unittests.


Also fixes a bug in Socket::Connect() where we forgot to wait for the notifier_fd using a select, and it was causing the forwarder to be stuck when the connection didn't complete (it did not have a timeout).

BUG=146502

Review URL: https://chromiumcodereview.appspot.com/10957043

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

8 files changed, 152 insertions, 40 deletions

diff --git a/tools/android/forwarder2/device_controller.cc b/tools/android/forwarder2/device_controller.cc
index 3a2ceba..c7acca3 100644
--- a/tools/android/forwarder2/device_controller.cc
+++ b/tools/android/forwarder2/device_controller.cc
@@ -81,10 +81,19 @@ bool DeviceController::Start(const std::string& adb_unix_socket) {
           // Remove deletes the listener object.
           listeners_.Remove(port);
         }
-        // Listener object will be deleted by the CleanUpDeadListeners method.
-        DeviceListener* new_listener = new DeviceListener(socket.Pass(), port);
-        listeners_.AddWithID(new_listener, port);
+        scoped_ptr<DeviceListener> new_listener(
+            new DeviceListener(socket.Pass(), port));
+        if (!new_listener->BindListenerSocket())
+          continue;
         new_listener->Start();
+        // |port| can be zero, to allow dynamically allocated port, so instead,
+        // we call DeviceListener::listener_port() to retrieve the currently
+        // allocated port to this new listener, which has been set by the
+        // BindListenerSocket() method in case of success.
+        const int listener_port = new_listener->listener_port();
+        // |new_listener| is now owned by listeners_ map.
+        listeners_.AddWithID(new_listener.release(), listener_port);
+        printf("Forwarding device port %d to host.\n", listener_port);
         break;
       }
       case command::DATA_CONNECTION:
diff --git a/tools/android/forwarder2/device_listener.cc b/tools/android/forwarder2/device_listener.cc
index d4575b9..fdabc70 100644
--- a/tools/android/forwarder2/device_listener.cc
+++ b/tools/android/forwarder2/device_listener.cc
@@ -27,7 +27,7 @@ namespace forwarder2 {
 DeviceListener::DeviceListener(scoped_ptr<Socket> adb_control_socket, int port)
     : adb_control_socket_(adb_control_socket.Pass()),
       listener_port_(port),
-      is_alive_(true),
+      is_alive_(false),
       must_exit_(false) {
   CHECK(adb_control_socket_.get());
   adb_control_socket_->set_exit_notifier_fd(exit_notifier_.receiver_fd());
@@ -152,20 +152,25 @@ void DeviceListener::RunInternal() {
 bool DeviceListener::BindListenerSocket() {
   bool success = listener_socket_.BindTcp("", listener_port_);
   if (success) {
+    // In case the |listener_port_| was zero, GetPort() will return the
+    // currently (non-zero) allocated port for this socket.
+    listener_port_ = listener_socket_.GetPort();
     SendCommand(command::BIND_SUCCESS,
                 listener_port_,
                 adb_control_socket_.get());
+    is_alive_ = true;
   } else {
     LOG(ERROR) << "Device could not bind and listen to local port.";
     SendCommand(command::BIND_ERROR,
                 listener_port_,
                 adb_control_socket_.get());
+    adb_control_socket_->Close();
   }
   return success;
 }
 
 void DeviceListener::Run() {
-  if (BindListenerSocket())
+  if (is_alive_)
     RunInternal();
   adb_control_socket_->Close();
   listener_socket_.Close();
diff --git a/tools/android/forwarder2/device_listener.h b/tools/android/forwarder2/device_listener.h
index f496f81..44d37aa 100644
--- a/tools/android/forwarder2/device_listener.h
+++ b/tools/android/forwarder2/device_listener.h
@@ -28,20 +28,23 @@ class DeviceListener : public Thread {
 
   bool SetAdbDataSocket(scoped_ptr<Socket> adb_data_socket);
 
-  // |is_alive_| is set only on creation and written once when Run() terminates.
-  // So even in case of a race condition, the worst that could happen is for the
-  // main thread to see the listener alive when it isn't. And also, this is not
-  // a problem since the main thread checks the liveliness of the listeners in a
-  // loop.
+  bool BindListenerSocket();
+
+  // |is_alive_| is set only on BindAndListenSocket and written once when Run()
+  // terminates.  So even in case of a race condition, the worst that could
+  // happen is for the main thread to see the listener alive when it isn't. And
+  // also, this is not a problem since the main thread checks the liveliness of
+  // the listeners in a loop.
   bool is_alive() const { return is_alive_; }
   void ForceExit();
 
+  int listener_port() const { return listener_port_; }
+
  protected:
   // Thread:
   virtual void Run() OVERRIDE;
 
  private:
-  bool BindListenerSocket();
   void RunInternal();
 
   // Must be called after successfully acquired mutex.
@@ -58,7 +61,7 @@ class DeviceListener : public Thread {
   // the forwarder. Ownership transferred to the Forwarder.
   scoped_ptr<Socket> adb_data_socket_;
 
-  const int listener_port_;
+  int listener_port_;
   pthread_mutex_t adb_data_socket_mutex_;
   pthread_cond_t adb_data_socket_cond_;
   bool is_alive_;
diff --git a/tools/android/forwarder2/host_controller.cc b/tools/android/forwarder2/host_controller.cc
index 6c04398..0507c11 100644
--- a/tools/android/forwarder2/host_controller.cc
+++ b/tools/android/forwarder2/host_controller.cc
@@ -23,7 +23,8 @@ HostController::HostController(int device_port,
       forward_to_host_(forward_to_host),
       forward_to_host_port_(forward_to_host_port),
       adb_port_(adb_port),
-      exit_notifier_fd_(exit_notifier_fd) {
+      exit_notifier_fd_(exit_notifier_fd),
+      ready_(false) {
   adb_control_socket_.set_exit_notifier_fd(exit_notifier_fd);
 }
 
@@ -60,19 +61,31 @@ void HostController::StartForwarder(
   forwarder->Start();
 }
 
-void HostController::Run() {
+bool HostController::Connect() {
   if (!adb_control_socket_.ConnectTcp("", adb_port_)) {
     LOG(ERROR) << "Could not Connect HostController socket on port: "
                << adb_port_;
-    return;
+    return false;
   }
   // Send the command to the device start listening to the "device_forward_port"
   SendCommand(command::LISTEN, device_port_, &adb_control_socket_);
-  if (!ReceivedCommand(command::BIND_SUCCESS, &adb_control_socket_)) {
+  int device_port_allocated;
+  command::Type command;
+  if (!ReadCommand(&adb_control_socket_, &device_port_allocated, &command) ||
+      command != command::BIND_SUCCESS) {
     LOG(ERROR) << "Device binding error using port " << device_port_;
     adb_control_socket_.Close();
-    return;
+    return false;
   }
+  // When doing dynamically allocation of port, we get the port from the
+  // BIND_SUCCESS command we received above.
+  device_port_ = device_port_allocated;
+  ready_ = true;
+  return true;
+}
+
+void HostController::Run() {
+  CHECK(ready_) << "HostController not ready. Must call Connect() first.";
   while (true) {
     if (!ReceivedCommand(command::ACCEPT_SUCCESS,
                          &adb_control_socket_)) {
@@ -82,6 +95,7 @@ void HostController::Run() {
     }
     // Try to connect to host server.
     scoped_ptr<Socket> host_server_data_socket(new Socket);
+    host_server_data_socket->set_exit_notifier_fd(exit_notifier_fd_);
     if (!host_server_data_socket->ConnectTcp(
             forward_to_host_, forward_to_host_port_)) {
       LOG(ERROR) << "Could not Connect HostServerData socket on port: "
diff --git a/tools/android/forwarder2/host_controller.h b/tools/android/forwarder2/host_controller.h
index bfec575..b4409eb 100644
--- a/tools/android/forwarder2/host_controller.h
+++ b/tools/android/forwarder2/host_controller.h
@@ -19,6 +19,7 @@ class HostProxy;
 
 class HostController : public Thread {
  public:
+  // If |device_port| is zero, it dynamically allocates a port.
   HostController(int device_port,
                  const std::string& forward_to_host,
                  int forward_to_host_port,
@@ -26,6 +27,15 @@ class HostController : public Thread {
                  int exit_notifier_fd);
   virtual ~HostController();
 
+  // Connects to the device forwarder app and sets the |device_port_| to the
+  // dynamically allocated port (when the provided |device_port| is zero).
+  // Returns true on success.  Clients must call Connect() before calling
+  // Start().
+  bool Connect();
+
+  // Gets the current device allocated port. Must be called after Connect().
+  int device_port() const { return device_port_; }
+
  protected:
   // Thread:
   virtual void Run() OVERRIDE;
@@ -34,7 +44,7 @@ class HostController : public Thread {
   void StartForwarder(scoped_ptr<Socket> host_server_data_socket_ptr);
 
   Socket adb_control_socket_;
-  const int device_port_;
+  int device_port_;
   const std::string forward_to_host_;
   const int forward_to_host_port_;
   const int adb_port_;
@@ -42,6 +52,8 @@ class HostController : public Thread {
   // Used to notify the controller to exit.
   const int exit_notifier_fd_;
 
+  bool ready_;
+
   DISALLOW_COPY_AND_ASSIGN(HostController);
 };
 
diff --git a/tools/android/forwarder2/host_forwarder_main.cc b/tools/android/forwarder2/host_forwarder_main.cc
index 2125d4f..1b3bf5d 100644
--- a/tools/android/forwarder2/host_forwarder_main.cc
+++ b/tools/android/forwarder2/host_forwarder_main.cc
@@ -32,8 +32,14 @@ const int kDefaultAdbPort = 3000;
 forwarder2::PipeNotifier* g_notifier;
 
 void KillHandler(int /* unused */) {
+  static int s_kill_handler_count = 0;
   CHECK(g_notifier);
-  g_notifier->Notify();
+  // If for some reason the forwarder get stuck in any socket waiting forever,
+  // we can send a SIGKILL or SIGINT three times to force it die
+  // (non-nicely). This is useful when debugging.
+  ++s_kill_handler_count;
+  if (!g_notifier->Notify() || s_kill_handler_count > 2)
+    exit(-1);
 }
 
 // Format of arg: <Device port>[:<Forward to port>:<Forward to address>]
@@ -46,14 +52,6 @@ bool ParseForwardArg(const std::string& arg,
   if (arg_pieces.size() == 0 || !StringToInt(arg_pieces[0], device_port))
     return false;
 
-  if (*device_port == 0) {
-    // TODO(felipeg): handle the case where we want to dynamically allocate the
-    // port. Althogh the Socket already supports it, we have to
-    // communicate the port from one forwarder to the other.
-    LOG(ERROR) << "Dynamically allocate the port is not yet implemented.";
-    return false;
-  }
-
   if (arg_pieces.size() > 1) {
     if (!StringToInt(arg_pieces[1], forward_to_port))
       return false;
@@ -62,9 +60,6 @@ bool ParseForwardArg(const std::string& arg,
   } else {
     *forward_to_port = *device_port;
   }
-
-  printf("Forwarding device port %d to host %d:%s\n",
-         *device_port, *forward_to_port, forward_to_host->c_str());
   return true;
 }
 
@@ -108,14 +103,21 @@ int main(int argc, char** argv) {
                         &device_port,
                         &forward_to_host,
                         &forward_to_port)) {
-      HostController* host_controller(
+      scoped_ptr<HostController> host_controller(
           new HostController(device_port,
                              forward_to_host,
                              forward_to_port,
                              adb_port,
                              g_notifier->receiver_fd()));
+      if (!host_controller->Connect())
+        continue;
       host_controller->Start();
-      controllers.push_back(host_controller);
+      // Get the current allocated port.
+      device_port = host_controller->device_port();
+      printf("Forwarding device port %d to host %d:%s\n",
+             device_port, forward_to_port, forward_to_host.c_str());
+
+      controllers.push_back(host_controller.release());
     } else {
       printf("Couldn't start forwarder server for port spec: %s\n",
              forward_args[i].c_str());
diff --git a/tools/android/forwarder2/socket.cc b/tools/android/forwarder2/socket.cc
index 7133500..898afc3d 100644
--- a/tools/android/forwarder2/socket.cc
+++ b/tools/android/forwarder2/socket.cc
@@ -33,6 +33,11 @@
   } while (false);
 
 
+namespace {
+const int kNoTimeout = -1;
+const int kConnectTimeOut = 10;  // Seconds.
+}  // namespace
+
 namespace forwarder2 {
 
 bool Socket::BindUnix(const std::string& path, bool abstract) {
@@ -175,12 +180,35 @@ bool Socket::BindAndListen() {
     SetSocketError();
     return false;
   }
+  if (port_ == 0) {
+    SockAddr addr;
+    memset(&addr, 0, sizeof(addr));
+    socklen_t addrlen = 0;
+    sockaddr* addr_ptr = NULL;
+    uint16* port_ptr = NULL;
+    if (family_ == AF_INET) {
+      addr_ptr = reinterpret_cast<sockaddr*>(&addr.addr4);
+      port_ptr = &addr.addr4.sin_port;
+      addrlen = sizeof(addr.addr4);
+    } else if (family_ == AF_INET6) {
+      addr_ptr = reinterpret_cast<sockaddr*>(&addr.addr6);
+      port_ptr = &addr.addr6.sin6_port;
+      addrlen = sizeof(addr.addr6);
+    }
+    errno = 0;
+    if (getsockname(socket_, addr_ptr, &addrlen) != 0) {
+      LOG(ERROR) << "getsockname error: " << safe_strerror(errno);;
+      SetSocketError();
+      return false;
+    }
+    port_ = ntohs(*port_ptr);
+  }
   return true;
 }
 
 bool Socket::Accept(Socket* new_socket) {
   DCHECK(new_socket != NULL);
-  if (!WaitForEvent()) {
+  if (!WaitForEvent(READ, kNoTimeout)) {
     SetSocketError();
     return false;
   }
@@ -197,8 +225,16 @@ bool Socket::Accept(Socket* new_socket) {
 }
 
 bool Socket::Connect() {
+  // Set non-block because we use select.
+  fcntl(socket_, F_SETFL, fcntl(socket_, F_GETFL) | O_NONBLOCK);
   errno = 0;
-  if (HANDLE_EINTR(connect(socket_, addr_ptr_, addr_len_)) < 0) {
+  if (HANDLE_EINTR(connect(socket_, addr_ptr_, addr_len_)) < 0 &&
+      errno != EINPROGRESS) {
+    SetSocketError();
+    return false;
+  }
+  // Wait for connection to complete, or receive a notification.
+  if (!WaitForEvent(WRITE, kConnectTimeOut)) {
     SetSocketError();
     return false;
   }
@@ -234,6 +270,14 @@ bool Socket::Resolve(const std::string& host) {
   return true;
 }
 
+int Socket::GetPort() {
+  if (family_ != AF_INET && family_ != AF_INET6) {
+    LOG(ERROR) << "Can't call GetPort() on an unix domain socket.";
+    return 0;
+  }
+  return port_;
+}
+
 bool Socket::IsFdInSet(const fd_set& fds) const {
   if (IsClosed())
     return false;
@@ -266,7 +310,7 @@ void Socket::SetSocketError() {
 }
 
 int Socket::Read(char* buffer, size_t buffer_size) {
-  if (!WaitForEvent()) {
+  if (!WaitForEvent(READ, kNoTimeout)) {
     SetSocketError();
     return 0;
   }
@@ -298,15 +342,28 @@ int Socket::WriteNumBytes(const char* buffer, size_t num_bytes) {
   return bytes_written;
 }
 
-bool Socket::WaitForEvent() const {
+bool Socket::WaitForEvent(EventType type, int timeout_secs) const {
   if (exit_notifier_fd_ == -1 || socket_ == -1)
     return true;
   const int nfds = std::max(socket_, exit_notifier_fd_) + 1;
   fd_set read_fds;
+  fd_set write_fds;
   FD_ZERO(&read_fds);
-  FD_SET(socket_, &read_fds);
+  FD_ZERO(&write_fds);
+  if (type == READ)
+    FD_SET(socket_, &read_fds);
+  else
+    FD_SET(socket_, &write_fds);
   FD_SET(exit_notifier_fd_, &read_fds);
-  if (HANDLE_EINTR(select(nfds, &read_fds, NULL, NULL, NULL)) <= 0)
+
+  timeval tv = {};
+  timeval* tv_ptr = NULL;
+  if (timeout_secs > 0) {
+    tv.tv_sec = timeout_secs;
+    tv.tv_usec = 0;
+    tv_ptr = &tv;
+  }
+  if (HANDLE_EINTR(select(nfds, &read_fds, &write_fds, NULL, tv_ptr)) <= 0)
     return false;
   return !FD_ISSET(exit_notifier_fd_, &read_fds);
 }
diff --git a/tools/android/forwarder2/socket.h b/tools/android/forwarder2/socket.h
index 01955ca..15a04b0 100644
--- a/tools/android/forwarder2/socket.h
+++ b/tools/android/forwarder2/socket.h
@@ -37,6 +37,9 @@ class Socket {
 
   bool Accept(Socket* new_socket);
 
+  // Returns the port allocated to this socket or zero on error.
+  int GetPort();
+
   bool IsFdInSet(const fd_set& fds) const;
   bool AddFdToSet(fd_set* fds) const;
 
@@ -96,10 +99,17 @@ class Socket {
   bool InitSocketInternal();
   void SetSocketError();
 
+  enum EventType {
+    READ,
+    WRITE
+  };
+
   // Waits until either the Socket or the |exit_notifier_fd_| has received a
   // read event (accept or read). Returns false iff an exit notification was
-  // received.
-  bool WaitForEvent() const;
+  // received.  If |read| is false, it waits until Socket is ready to write,
+  // instead.  If |timeout_secs| is a non-negative value, it sets the timeout,
+  // for the select operation.
+  bool WaitForEvent(EventType type, int timeout_secs) const;
 
   int socket_;
   int port_;