Restart the host when the third party auth certificate changes

remoting/host/linux/certificate_watcher.cc

+// Copyright 2016 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 "certificate_watcher.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
+
+namespace remoting {
+
+// Delay time to restart the host when a change of certificate is detected.
+// This is to repeating restarts when continuous writes to the database occur.

[Sergey Ulanov, 2016/03/31 22:36:15]

s/to repeating/to avoid repeating/

[Yuwei, 2016/04/01 23:41:02]

Done.

+const int kRestartDelayInSecond = 2;
+
+// Full Path: $HOME/.pki/nssdb
+const base::BasePathKey kNssBasePath = base::DIR_HOME;

[Sergey Ulanov, 2016/03/31 22:36:15]

Don't need this. Just pass base::DIR_HOME when calling PathService::Get().

[Yuwei, 2016/04/01 23:41:02]

Done.

+const char kNssCertDirectoryPath[] = ".pki/nssdb";
+
+CertificateWatcher::CertificateWatcher(base::WeakPtr<HostStatusMonitor> monitor,
+                                       const base::Closure& restart_action)
+    : monitor_(monitor),
+      delay_(base::TimeDelta::FromSeconds(kRestartDelayInSecond)),
+      restart_action_(restart_action),
+      restart_deferred_action_(base::Bind(&base::DoNothing)) {
+  base::PathService::Get(kNssBasePath, &nss_watch_path_);

[Sergey Ulanov, 2016/03/31 22:36:15]

Verify that you get correct result from here. If we don't then I think we want
to log the error and keep the watcher disabled.

[Yuwei, 2016/04/01 23:41:02]

Done.

+  nss_watch_path_ = nss_watch_path_.AppendASCII(

[Sergey Ulanov, 2016/03/31 22:36:15]

call it watch_path_ instead of nss_watch_path_

[Yuwei, 2016/04/01 23:41:02]

Done.

+      std::string(kNssCertDirectoryPath));

[Sergey Ulanov, 2016/03/31 22:36:15]

Don't need std::string(). Compiler will convert it implicitly.

[Yuwei, 2016/04/01 23:41:02]

Done.

+  if (monitor_) {
+    monitor_->AddStatusObserver(this);
+  }
+}
+
+void CertificateWatcher::OnCertDirectoryChanged(const base::FilePath& path,
+                                                bool error) {
+  if (error || path != nss_watch_path_) {
+    LOG(WARNING) << "Unexpected file update callback. "
+                 << "Path: " << path.MaybeAsASCII() << "; "
+                 << "Error: " << error;
+    return;
+  }
+
+  restart_timer_->Reset();
+
+  if (inhibit_mode_) {
+    restart_deferred_action_.Run();

[Sergey Ulanov, 2016/03/31 22:36:15]

Why do you need this?

[Yuwei, 2016/03/31 23:08:33]

For testing reason. We need some way to quit the message loop if the restart
action is deferred...

[Sergey Ulanov, 2016/04/01 17:49:23]

Just post a delayed Quit task on that message loop with delay slightly larger
than |delay_|?

[Yuwei, 2016/04/01 18:28:04]

That's time dependent and sounds a little bit flaky? I am not sure how long it
takes for the file watcher to notice the changes and how precise the delayed
timer will be... Or we can quit the message loop with delay say 2*|delay_|?

[Sergey Ulanov, 2016/04/01 20:11:00]

The test can work something like this:
 1. OnClientConnected();
 2. Update watch path.
 3. Post quit delayed task for delay_ + 0.1s
 4. message_loop_.Run();
 5. Verify that the restart closure wasn't called.
 6. OnClientDisconnected();
 7. If the restart closure still hasn't been called then message_loop_.Run()
which quits when it's called.

I don't think this will be flaky. Even if the watcher doesn't notice the change
in step 4 it should notice it in 7. The test may sometimes still succeed when
restart is not inhibited properly, but I think that's acceptable.

[Yuwei, 2016/04/01 20:53:22]

Okay... Sounds reasonable

[Yuwei, 2016/04/01 23:41:02]

Done.

+  }
+}
+
+void CertificateWatcher::OnTimer() {
+  if (inhibit_mode_) {
+    restart_timer_->Reset();

[Sergey Ulanov, 2016/03/31 22:36:15]

I don't think you want to rest the timer here. It doesn't make sense to keep the
timer running while we are waiting the user to disconnect. Instead I suggest you
add a boolean flag that would indicate that restart is pending. So here you will
have
  restart_pending_ = true;

[Yuwei, 2016/03/31 23:08:33]

I guess I misunderstood your last comment about always starting the timer in
inhibit mode.

So we still want to use a flag to mark a pending restart but we should defer it
at the beginning of OnTimer instead of at the end of OnUpdate, right?

[Sergey Ulanov, 2016/04/01 17:49:23]

Starting/resetting the timer makes sense only in response to file change
notifications. I.e. you shouldn't be calling timer_->Reset() anywhere except
from OnCertDirectoryChanged()

[Yuwei, 2016/04/01 18:28:04]

Okay...

[Yuwei, 2016/04/01 23:41:02]

Done.

+    return;
+  }
+  VLOG(1) << "NSS certificate was updated. Calling restart...";
+  restart_action_.Run();
+}
+
+CertificateWatcher::~CertificateWatcher() {
+  if (monitor_) {
+    monitor_->RemoveStatusObserver(this);
+  }
+
+  VLOG(1) << "Stopped watching certificate changes.";
+}
+
+void CertificateWatcher::Start() {
+  file_watcher_.reset(new base::FilePathWatcher());
+  file_watcher_->Watch(
+      nss_watch_path_, true,
+      base::Bind(&CertificateWatcher::OnCertDirectoryChanged,
+                 base::Unretained(this)));
+
+  auto raw_timer =

[Sergey Ulanov, 2016/03/31 22:36:15]

Don't need this variable:
  restart_timer.reset(new base::DelayTimer(...));

[Yuwei, 2016/04/01 23:41:02]

Done.

+      new base::DelayTimer(FROM_HERE,
+                           delay_,

[Sergey Ulanov, 2016/03/31 22:36:15]

Please clang-format this code. It will move this line to the previous one.

[Yuwei, 2016/04/01 23:41:02]

Done.

+                           this,
+                           &CertificateWatcher::OnTimer);
+  restart_timer_.reset(raw_timer);
+
+  VLOG(1) << "Started watching certificate changes.";
+}
+
+void CertificateWatcher::SetInhibit(bool inhibit) { inhibit_mode_ = inhibit; }

[Sergey Ulanov, 2016/03/31 22:36:15]

Don't need this method. Just set the variable in OnClientConnected() and
OnClientDisconnected()

[Yuwei, 2016/04/01 23:41:02]

Done.

+
+void CertificateWatcher::OnClientConnected(const std::string& jid) {
+  SetInhibit(true);
+}
+
+void CertificateWatcher::OnClientDisconnected(const std::string& jid) {
+  SetInhibit(false);
+}

[Sergey Ulanov, 2016/03/31 22:36:15]

if (restart_pending_) {
  restart_action_.Run();
}

[Yuwei, 2016/04/01 23:41:02]

Done.

+
+void CertificateWatcher::SetDelay(const base::TimeDelta& delay) {

[Sergey Ulanov, 2016/03/31 22:36:15]

Call it SetDelayForTests.

[Yuwei, 2016/04/01 23:41:03]

Done.

+  delay_ = delay;
+}
+
+void CertificateWatcher::SetWatchPath(const base::FilePath& watch_path) {
+  nss_watch_path_ = watch_path;
+}
+
+void CertificateWatcher::SetRestartDeferredAction(

[Sergey Ulanov, 2016/03/31 22:36:15]

Why do you need this?

[Yuwei, 2016/03/31 23:08:33]

See above... For quitting the message loop

[Yuwei, 2016/04/01 23:41:02]

Removed.

+    const base::Closure& restart_action) {
+  restart_deferred_action_ = restart_action;
+}
+
+}  // namespace remoting