Use the new forwarder2's Daemon implementation in device_forwarder.

tools/android/forwarder2/host_forwarder_main.cc

Index: tools/android/forwarder2/host_forwarder_main.cc
diff --git a/tools/android/forwarder2/host_forwarder_main.cc b/tools/android/forwarder2/host_forwarder_main.cc
index dff0c8f42e975da0e7ca4dd67a7cc174cf844d75..59c0ba6a253bdc3d1378defea256d69bb88ab46b 100644
--- a/tools/android/forwarder2/host_forwarder_main.cc
+++ b/tools/android/forwarder2/host_forwarder_main.cc
@@ -4,14 +4,15 @@
 
 #include <errno.h>
 #include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
 #include <unistd.h>
 
-#include <vector>
+#include <cstdio>
+#include <cstring>
 #include <string>
+#include <vector>
 
 #include "base/command_line.h"
+#include "base/compiler_specific.h"
 #include "base/eintr_wrapper.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
@@ -29,26 +30,36 @@
 #include "tools/android/forwarder2/pipe_notifier.h"
 #include "tools/android/forwarder2/socket.h"
 
-using base::StringToInt;
-
 namespace forwarder2 {
 namespace {
 
+const char kLogFilePath[] = "/tmp/host_forwarder_log";
 const char kPIDFilePath[] = "/tmp/host_forwarder_pid";
-const char kCommandSocketPath[] = "host_forwarder_command_socket";
-const char kWelcomeMessage[] = "forwarder2";
+const char kDaemonIdentifier[] = "chrome_host_forwarder_daemon";
+
+const char kKillServerCommand[] = "kill-server";
+const char kForwardCommand[] = "forward";
+
 const int kBufSize = 256;
 
-// Need to be global to be able to accessed from the signal handler.
-PipeNotifier* g_notifier;
+// Needs to be global to be able to be accessed from the signal handler.
+PipeNotifier* g_notifier = NULL;
+
+// Lets the daemon fetch the exit notifier file descriptor.
+int GetExitNotifierFD() {
+  DCHECK(g_notifier);
+  return g_notifier->receiver_fd();
+}
 
 void KillHandler(int signal_number) {
+  char buf[kBufSize];
   if (signal_number != SIGTERM && signal_number != SIGINT) {
-    char buf[kBufSize];
     snprintf(buf, sizeof(buf), "Ignoring unexpected signal %d.", signal_number);
     SIGNAL_SAFE_LOG(WARNING, buf);
     return;
   }
+  snprintf(buf, sizeof(buf), "Received signal %d.", signal_number);
+  SIGNAL_SAFE_LOG(WARNING, buf);
   static int s_kill_handler_count = 0;
   CHECK(g_notifier);
   // If for some reason the forwarder get stuck in any socket waiting forever,
@@ -59,34 +70,6 @@ void KillHandler(int signal_number) {
     exit(1);
 }
 
-enum {
-  kConnectSingleTry = 1,
-  kConnectNoIdleTime = 0,
-};
-
-scoped_ptr<Socket> ConnectToDaemon(int tries_count, int idle_time_msec) {
-  for (int i = 0; i < tries_count; ++i) {
-    scoped_ptr<Socket> socket(new Socket());
-    if (!socket->ConnectUnix(kCommandSocketPath, true)) {
-      if (idle_time_msec)
-        usleep(idle_time_msec * 1000);
-      continue;
-    }
-    char buf[sizeof(kWelcomeMessage)];
-    memset(buf, 0, sizeof(buf));
-    if (socket->Read(buf, sizeof(buf)) < 0) {
-      perror("read");
-      continue;
-    }
-    if (strcmp(buf, kWelcomeMessage)) {
-      LOG(ERROR) << "Unexpected message read from daemon: " << buf;
-      break;
-    }
-    return socket.Pass();
-  }
-  return scoped_ptr<Socket>(NULL);
-}
-
 // Format of |command|:
 //   <ADB port>:<Device port>[:<Forward to port>:<Forward to address>].
 bool ParseForwardCommand(const std::string& command,
@@ -98,12 +81,12 @@ bool ParseForwardCommand(const std::string& command,
   base::SplitString(command, ':', &command_pieces);
 
   if (command_pieces.size() < 2 ||
-      !StringToInt(command_pieces[0], adb_port) ||
-      !StringToInt(command_pieces[1], device_port))
+      !base::StringToInt(command_pieces[0], adb_port) ||
+      !base::StringToInt(command_pieces[1], device_port))
     return false;
 
   if (command_pieces.size() > 2) {
-    if (!StringToInt(command_pieces[2], forward_to_port))
+    if (!base::StringToInt(command_pieces[2], forward_to_port))
       return false;
     if (command_pieces.size() > 3)
       *forward_to_host = command_pieces[3];
@@ -121,44 +104,30 @@ bool IsForwardCommandValid(const std::string& command) {
       command, &adb_port, &device_port, &forward_to_host, &forward_to_port);
 }
 
-bool DaemonHandler() {
-  LOG(INFO) << "Starting host process daemon (pid=" << getpid() << ")";
-  DCHECK(!g_notifier);
-  g_notifier = new PipeNotifier();
-
-  const int notifier_fd = g_notifier->receiver_fd();
-  Socket command_socket;
-  if (!command_socket.BindUnix(kCommandSocketPath, true)) {
-    LOG(ERROR) << "Could not bind Unix Domain Socket";
-    return false;
-  }
-  command_socket.set_exit_notifier_fd(notifier_fd);
+class DaemonDelegate : public Daemon::ServerDelegate {

[felipeg, 2012/11/15 15:54:54]

One suggestion would be:
make a notifier object be owned by the DaemonDelegate class.
On the Init method of this class you just change the g_notifier pointer.
The g_notifier pointer still needs to be global to be accessed from the signal
handler, but the real object is owned by the DaemonDelegate class.

Also, you can keep the CHECK(!g_notifier) ond the Init() as you have right now.
With this we can be sure that there is only one call to the Init() function of
DaemonDelegate, and also there is no chance of memory leak since the notifier
object is owned.

[Philippe, 2012/11/15 16:25:19]

See my reply to your comment line 117.

+ public:
+  DaemonDelegate() : has_failed_(false) {}
 
-  signal(SIGTERM, KillHandler);
-  signal(SIGINT, KillHandler);
+  bool has_failed() const { return has_failed_; }
 
-  ScopedVector<HostController> controllers;
-  int failed_count = 0;
+  // Daemon::ServerDelegate:
+  virtual void Init() OVERRIDE {
+    LOG(INFO) << "Starting host process daemon (pid=" << getpid() << ")";
+    DCHECK(!g_notifier);

[felipeg, 2012/11/15 15:54:54]

may need a lock ?
Or add a comment saying this class is not thread safe.

[Philippe, 2012/11/15 16:25:19]

I think it would look a little weird to say that this (private) class (which we
fully control since we are the only client) is not thread-safe in a purely
single-thread context.

What do you think?

[felipeg, 2012/11/15 18:40:58]

I agree.
But a comment will be nice for my future me to remember that when I make a
change that may use this class in multiple places I have to remember that this
is not thread safe :)
But it is nip tic, your call

[Philippe, 2012/11/19 10:22:14]

I wish I could help you here but I don't think this will in that specific case
:) I even think that this comment could be misleading since it could make the
reader think that OnClientConnected() for instance is called on a new thread.
The only client dealing with this class is Daemon therefore you shouldn't have
to worry too much here (you can't reuse it).
If you can think of a non-misleading comment here though I would be glad to add
it. I just think that "this class is not thread-safe" doesn't make a lot of
sense here.

What do you think?

+    g_notifier = new PipeNotifier();

[felipeg, 2012/11/15 15:54:54]

you are initializing a global var in the init method of a class.
If you have more than one object of this class things will leak.
Can you think of a better way to make sure our global var is properly
initialized only once ?
I know this was caused by my initial code where we had the global static method
for initializing this.

+    signal(SIGTERM, KillHandler);
+    signal(SIGINT, KillHandler);
+  }
 
-  for (;;) {
-    Socket client_socket;
-    if (!command_socket.Accept(&client_socket)) {
-      if (command_socket.exited())
-        return true;
-      PError("Accept()");
-      return false;
-    }
-    if (!client_socket.Write(kWelcomeMessage, sizeof(kWelcomeMessage))) {
-      PError("Write()");
-      continue;
-    }
+  virtual void OnClientConnected(scoped_ptr<Socket> client_socket) OVERRIDE {
     char buf[kBufSize];
-    const int bytes_read = client_socket.Read(buf, sizeof(buf));
+    const int bytes_read = client_socket->Read(buf, sizeof(buf));
     if (bytes_read <= 0) {
-      if (client_socket.exited())
-        break;
+      if (client_socket->exited())
+        return;
       PError("Read()");
-      ++failed_count;
+      has_failed_ = true;
+      return;
     }
     const std::string command(buf, bytes_read);
     int adb_port = 0;
@@ -168,41 +137,81 @@ bool DaemonHandler() {
     const bool succeeded = ParseForwardCommand(
         command, &adb_port, &device_port, &forward_to_host, &forward_to_port);
     if (!succeeded) {
-      ++failed_count;
-      client_socket.WriteString(
+      has_failed_ = true;
+      client_socket->WriteString(
           base::StringPrintf("ERROR: Could not parse forward command '%s'",
                              command.c_str()));
-      continue;
+      return;
     }
     scoped_ptr<HostController> host_controller(
         new HostController(device_port, forward_to_host, forward_to_port,
-                           adb_port, notifier_fd));
+                           adb_port, g_notifier->receiver_fd()));

[felipeg, 2012/11/15 15:54:54]

Maybe use GetExitNotifierFD() for consistency ?
also maybe add a DCHECK(g_notifier)

[Philippe, 2012/11/15 16:25:19]

Sure. The DCHECK is in GetExitNotifierFD().

     if (!host_controller->Connect()) {
-      ++failed_count;
-      client_socket.WriteString("ERROR: Connection to device failed.");
-      continue;
+      has_failed_ = true;
+      client_socket->WriteString("ERROR: Connection to device failed.");
+      return;
     }
     // Get the current allocated port.
     device_port = host_controller->device_port();
     LOG(INFO) << "Forwarding device port " << device_port << " to host "
               << forward_to_host << ":" << forward_to_port;
-    if (!client_socket.WriteString(
+    if (!client_socket->WriteString(
         base::StringPrintf("%d:%d", device_port, forward_to_port))) {
-      ++failed_count;
-      continue;
+      has_failed_ = true;
+      return;
     }
     host_controller->Start();
-    controllers.push_back(host_controller.release());
+    controllers_.push_back(host_controller.release());
   }
-  for (int i = 0; i < controllers.size(); ++i)
-    controllers[i]->Join();
 
-  if (controllers.size() == 0) {
-    LOG(ERROR) << "No forwarder servers could be started. Exiting.";
-    return false;
+  virtual void OnServerExited() OVERRIDE {
+    for (int i = 0; i < controllers_.size(); ++i)
+      controllers_[i]->Join();
+    if (controllers_.size() == 0) {
+      LOG(ERROR) << "No forwarder servers could be started. Exiting.";
+      has_failed_ = true;
+    }
+  }
+
+ private:
+  ScopedVector<HostController> controllers_;
+  bool has_failed_;
+
+  DISALLOW_COPY_AND_ASSIGN(DaemonDelegate);
+};
+
+class ClientDelegate : public Daemon::ClientDelegate {
+ public:
+  ClientDelegate(const std::string& forward_command)
+      : forward_command_(forward_command),
+        has_failed_(false) {
   }
-  return true;
-}
+
+  bool has_failed() const { return has_failed_; }
+
+  // Daemon::ClientDelegate:
+  virtual void OnDaemonReady(Socket* daemon_socket) OVERRIDE {
+    // Send the forward command to the daemon.
+    CHECK(daemon_socket->WriteString(forward_command_));
+    char buf[kBufSize];
+    const int bytes_read = daemon_socket->Read(
+        buf, sizeof(buf) - 1 /* leave space for null terminator */);
+    CHECK_GT(bytes_read, 0);
+    DCHECK(bytes_read < sizeof(buf));
+    buf[bytes_read] = 0;
+    base::StringPiece msg(buf, bytes_read);
+    if (msg.starts_with("ERROR")) {
+      LOG(ERROR) << msg;
+      has_failed_ = true;
+      return;
+    }
+    printf("%s\n", buf);
+  }
+
+ private:
+  const std::string forward_command_;
+  bool has_failed_;
+};
 
 void PrintUsage(const char* program_name) {
   LOG(ERROR) << program_name << " adb_port:from_port:to_port:to_host\n"
@@ -215,63 +224,36 @@ int RunHostForwarder(int argc, char** argv) {
     return 1;
   }
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  std::string command;
+  const char* command = NULL;
   int adb_port = 0;
   if (argc != 2) {
     PrintUsage(argv[0]);
     return 1;
   }
-  if (!strcmp(argv[1], "kill-server")) {
-    command = "kill-server";
+  if (!strcmp(argv[1], kKillServerCommand)) {
+    command = kKillServerCommand;
   } else {
-    command = "forward";
+    command = kForwardCommand;
     if (!IsForwardCommandValid(argv[1])) {
       PrintUsage(argv[0]);
       return 1;
     }
   }
 
-  Daemon daemon(kPIDFilePath);
+  ClientDelegate client_delegate(argv[1]);
+  DaemonDelegate daemon_delegate;

[felipeg, 2012/11/15 15:54:54]

I don't see you calling daemon_delegate->Init() which initializes the
g_exit_notifier
and you call GetExitNotifierFD below.

[Philippe, 2012/11/15 16:25:19]

Init() is called by the Daemon. See daemon.cc:273.

+  Daemon daemon(
+      kLogFilePath, kPIDFilePath, kDaemonIdentifier, &client_delegate,
+      &daemon_delegate, &GetExitNotifierFD);
 
-  if (command == "kill-server")
+  if (command == kKillServerCommand)
     return !daemon.Kill();
 
-  bool is_daemon = false;
-  scoped_ptr<Socket> daemon_socket = ConnectToDaemon(
-      kConnectSingleTry, kConnectNoIdleTime);
-  if (!daemon_socket) {
-    if (!daemon.Spawn(&is_daemon))
-      return 1;
-  }
-
-  if (is_daemon)
-    return !DaemonHandler();
-
-  if (!daemon_socket) {
-    const int kTries = 10;
-    const int kIdleTimeMsec = 10;
-    daemon_socket = ConnectToDaemon(kTries, kIdleTimeMsec);
-    if (!daemon_socket) {
-      LOG(ERROR) << "Could not connect to daemon.";
-      return 1;
-    }
-  }
-
-  // Send the forward command to the daemon.
-  CHECK(daemon_socket->Write(argv[1], strlen(argv[1])));
-  char buf[kBufSize];
-  const int bytes_read = daemon_socket->Read(
-      buf, sizeof(buf) - 1 /* leave space for null terminator */);
-  CHECK_GT(bytes_read, 0);
-  DCHECK(bytes_read < sizeof(buf));
-  buf[bytes_read] = 0;
-  base::StringPiece msg(buf, bytes_read);
-  if (msg.starts_with("ERROR")) {
-    LOG(ERROR) << msg;
+  DCHECK(command == kForwardCommand);
+  if (!daemon.SpawnIfNeeded())
     return 1;
-  }
-  printf("%s\n", buf);
-  return 0;
+
+  return client_delegate.has_failed() || daemon_delegate.has_failed();
 }
 
 }  // namespace