[Chromoting] The daemon process now starts the networking process and passes the host configuration…

remoting/host/daemon_process.cc

Index: remoting/host/daemon_process.cc
diff --git a/remoting/host/daemon_process.cc b/remoting/host/daemon_process.cc
index 2a08284eb4dae13847613ae9b7a590961b3e75ee..8e2081cdf1b97c155736c4784f34dbbe448def3c 100644
--- a/remoting/host/daemon_process.cc
+++ b/remoting/host/daemon_process.cc
@@ -6,22 +6,42 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/files/file_path_watcher.h"
+#include "base/file_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread.h"
+#include "remoting/host/branding.h"
+#include "remoting/host/chromoting_messages.h"
 
 namespace {
 
-const char kIpcThreadName[] = "Daemon process IPC";
+// The  command line switch name that can be used to specify the host
+// configuration file name.

[Wez, 2012/08/21 00:18:57]

nit: Suggest "The name of the command-line switch used to specify the host
configuration file to use."

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+const char kHostConfigSwitchName[] = "host-config";
+
+const FilePath::CharType kDefaultHostConfigFile[] =
+    FILE_PATH_LITERAL("host.json");
 
 }  // namespace
 
 namespace remoting {
 
 DaemonProcess::~DaemonProcess() {
+  CHECK_EQ(config_watcher_state_, kConfigWatcherStopped);
+}
+
+void DaemonProcess::OnChannelConnected() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  // Send the configuration to the network process.
+  Send(new ChromotingDaemonNetworkMsg_Configuration(config_));
 }
 
 bool DaemonProcess::OnMessageReceived(const IPC::Message& message) {
-  return true;
+  DCHECK(main_task_runner()->BelongsToCurrentThread());

[Wez, 2012/08/21 00:18:57]

nit: You don't need this check if all you're going to do is return false.

[alexeypa (please no reviews), 2012/08/23 23:53:01]

I'll add useful code to this method in the nearest future. Meanwhile this DCHECK
is a documentation describing thread affinity of the method.

+
+  return false;
 }
 
 DaemonProcess::DaemonProcess(

[Wez, 2012/08/21 00:18:57]

nit: Although this ordering matches that in the class definition, I think it's
clearer to put the ctor immediately before the dtor in the implementation.

[alexeypa (please no reviews), 2012/08/21 17:16:36]

There was a long thread in chrome-dev about this and the outcome was that the
order should match. At least majority of folks thought this is a better
practice. So I'll stick to matching order in .h and .cc even if I personally
think that grouping constructors and destructors in .cc makes more sense.

@@ -29,28 +49,131 @@ DaemonProcess::DaemonProcess(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     const base::Closure& stopped_callback)
     : Stoppable(main_task_runner, stopped_callback),
+      config_watcher_state_(kConfigWatcherRunning),
       main_task_runner_(main_task_runner),
       io_task_runner_(io_task_runner) {
   // Initialize on the same thread that will be used for shutting down.
   main_task_runner_->PostTask(
     FROM_HERE,
-    base::Bind(&DaemonProcess::Init, base::Unretained(this)));
+    base::Bind(&DaemonProcess::Initialize, base::Unretained(this)));

[Wez, 2012/08/21 00:18:57]

Is there any stronger guarantee that DaemonProcess still exists when this task
is processed than that we have to ping-pong a message off the IO thread before
we becomes "stopped"? That seems a little fragile to me.

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Making the object ref-countable (or passing scoped_ptr reference to it) merely
guarantees that posted tasks will run while the object is alive. It does not
guarantee that the object will be deleted only after it was properly shutdown
because the owner can still release the last reference too soon. This means that
CHECK in the destructor validating that the object is properly stopped is needed
in either case.

This CHECK in the destructor appears to be a strong enough guarantee that
posting to base::Unretained(this) is safe.

[Wez, 2012/08/30 20:36:04]

For a follow-up CL, I wonder if we could add a helper to Stoppable to get a
base::Unretained(this), to use in place of scary-looking Unretaineds, so I don't
have to keep asking this question? :P

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Maybe. I have a whole bunch of other important stuff in my to do list, so please
feel free to jump in and send me a review.

+}
+
+void DaemonProcess::Initialize() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  // Get the name of the host configuration file.
+  FilePath default_config_dir = remoting::GetConfigDir();
+  config_path_ = default_config_dir.Append(kDefaultHostConfigFile);
+

[Wez, 2012/08/21 00:18:57]

nit: No need for this blank line.

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(kHostConfigSwitchName)) {
+    config_path_ = command_line->GetSwitchValuePath(kHostConfigSwitchName);
+  }
+
+  // Initialize the config file watcher on the I/O thread.
+  io_task_runner()->PostTask(
+    FROM_HERE,
+    base::Bind(&DaemonProcess::StartConfigWatcher, base::Unretained(this)));

[Wez, 2012/08/21 00:18:57]

Similarly, what is the guarantee that DaemonProcess still exist when this task
is processed?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

CHECK in the destructor seems to be it.

+
+  // Launch the process.
+  LaunchNetworkProcess();
+}
+
+void DaemonProcess::DoStop() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  if (config_watcher_state_ == kConfigWatcherRunning) {
+    config_watcher_state_ = kConfigWatcherStopping;
+    io_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&DaemonProcess::StopConfigWatcher, base::Unretained(this)));
+  }

[Wez, 2012/08/21 00:18:57]

I think you need to clear the reload timer here, otherwise the timer may go off
after the stopped task has been posted, and cause a new task to be posted to
this object that won't be processed until the stopped task has deleted it?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

It is done in DaemonProcess::StopConfigWatcher.

+
+  // Wait until the configuration file watcher has been fully stopped.
+  if (config_watcher_state_ == kConfigWatcherStopped)
+    CompleteStopping();
+}
+
+void DaemonProcess::ConfigUpdated(const FilePath& path, bool error) {
+  DCHECK(io_task_runner()->BelongsToCurrentThread());
+
+  // Call ReloadConfig() after a short delay, so that we will not try to read
+  // the updated configuration file before it has been completely written.
+  // If the writer moves the new configuration file into place atomically,
+  // this delay may not be necessary.
+  if (!error && config_path_ == path)
+    config_updated_timer_->Reset();
 }
 
-void DaemonProcess::Init() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
+void DaemonProcess::ReloadConfig() {
+  DCHECK(io_task_runner()->BelongsToCurrentThread());
 
-  if (!LaunchNetworkProcess()) {
-    LOG(ERROR) << "Failed to launch the networking process.";
+  std::string config;
+  if (!file_util::ReadFileToString(config_path_, &config)) {
+    LOG(ERROR) << "Failed to read '" << config_path_.value() << "'";
     Stop();
     return;
   }
+
+  main_task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonProcess::SendConfig, base::Unretained(this), config));
 }
 
-void DaemonProcess::DoStop() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
+void DaemonProcess::SendConfig(const std::string& config) {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  if (config_ != config) {
+    config_ = config;
+    Send(new ChromotingDaemonNetworkMsg_Configuration(config_));
+  }
+}
+
+void DaemonProcess::StartConfigWatcher() {
+  DCHECK(io_task_runner()->BelongsToCurrentThread());
+  DCHECK(config_updated_timer_.get() == NULL);
+  DCHECK(config_watcher_.get() == NULL);
+
+  // Create the timer that will be used for dealyed-reading the configuration

[Wez, 2012/08/21 00:18:57]

typo: delayed

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  // file.
+  config_updated_timer_.reset(new base::DelayTimer<DaemonProcess>(
+      FROM_HERE, base::TimeDelta::FromSeconds(2), this,
+      &DaemonProcess::ReloadConfig));
+
+  // Start watching the configuration file.
+  config_watcher_.reset(new base::files::FilePathWatcher());
+  if (!config_watcher_->Watch(config_path_,
+                              base::Bind(&DaemonProcess::ConfigUpdated,
+                                         base::Unretained(this)))) {
+    LOG(ERROR) << "Couldn't watch file '" << config_path_.value() << "'";
+    Stop();
+    return;
+  }
+
+  // Force reloading of the configuration file at least once.
+  ReloadConfig();
+}
+
+void DaemonProcess::StopConfigWatcher() {
+  DCHECK(io_task_runner()->BelongsToCurrentThread());
+  DCHECK(config_updated_timer_.get() != NULL);
+  DCHECK(config_watcher_.get() != NULL);
+
+  config_updated_timer_.reset(NULL);
+  config_watcher_.reset(NULL);
+
+  main_task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonProcess::StopConfigWatcherCompleted,
+                 base::Unretained(this)));
+}
+
+void DaemonProcess::StopConfigWatcherCompleted() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+  DCHECK(config_watcher_state_ == kConfigWatcherStopping);
 
-  CompleteStopping();
+  config_watcher_state_ = kConfigWatcherStopped;
+  Stop();
 }
 
 }  // namespace remoting