Restart the host when the third party auth certificate changes

remoting/host/linux/certificate_watcher.h

+// 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.
+
+#ifndef REMOTING_HOST_LINUX_CERTIFICATE_WATCHER_H_
+#define REMOTING_HOST_LINUX_CERTIFICATE_WATCHER_H_
+
+#include "base/files/file_path.h"
+#include "base/files/file_path_watcher.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
+

[Sergey Ulanov, 2016/03/30 21:02:45]

remove this empty line

[Yuwei, 2016/03/31 17:40:04]

Done.

+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/host/host_status_monitor.h"
+#include "remoting/host/host_status_observer.h"
+
+namespace remoting {
+
+// This class watches the NSS database and kills the host when a change of the
+// database is detected. The runner script will restart the host when the host
+// is killed then the new host will capture any new changes of the database.
+//
+// Acceptable false positives will be caused by desktop sessions and other
+// external programs.
+//
+// Implements HostStatusObserver to defer restart action when the host is
+// connected to a client.
+class CertificateWatcher : public remoting::HostStatusObserver {
+ public:
+  CertificateWatcher(int delay, const base::Closure& restart_action);
+  CertificateWatcher(int delay,

[Sergey Ulanov, 2016/03/30 21:02:45]

Does the delay really need to be passed to the constructor? If it's useful only
for tests then add set_delay_for_tests().

[Yuwei, 2016/03/30 21:37:00]

So may just have a constant inside the CertificateWatcher class?

[Yuwei, 2016/03/31 17:40:04]

Done.

+                     const base::Closure& restart_action,
+                     const base::Closure& restart_deferred_action,

[Sergey Ulanov, 2016/03/30 21:02:45]

This parameter is used only for tests. Do you really need it?

[Yuwei, 2016/03/30 21:37:00]

In fact this constructor is basically for tests only...

+                     const base::FilePath& watch_path);

[Sergey Ulanov, 2016/03/30 21:02:45]

This argument is used for tests only. Replace it with set_watch_path_for_tests
method. Then you wouldn't need overloaded constructor.

[Yuwei, 2016/03/30 21:37:00]

okay

[Yuwei, 2016/03/31 17:40:04]

Done.

+
+  // Constructs watcher with given impl. Will take ownership.
+  ~CertificateWatcher() override;
+
+

[Sergey Ulanov, 2016/03/30 21:02:45]

remove extra empty lines

+
+  // Starts at current thread.
+  // Add itself to the monitor as an observer if the monitor is not null.
+  void Start(base::WeakPtr<HostStatusMonitor> monitor);
+
+  // Starts on specified thread
+  void StartOn(scoped_refptr<AutoThreadTaskRunner> runner,

[Sergey Ulanov, 2016/03/30 21:02:45]

Why do you need this method?

[Yuwei, 2016/03/30 21:37:00]

For most of the cases you can simply run watcher on the main thread... Do you
suggest that it's the caller's responsibility to run runner->PostTask(&Start...)
instead of the callee's?

[Yuwei, 2016/03/31 17:40:05]

Removed

+               base::WeakPtr<HostStatusMonitor> monitor);
+
+  void Stop();
+
+  // Marks inhibit_restart_scheduled_ flag without calling the restart action
+  void Inhibit();
+
+  // Leaves inhibit mode and schedule restart action if
+  // inhibit_restart_scheduled_ is marked in previous inhibit mode.
+  void Uninhibit();

[Sergey Ulanov, 2016/03/30 21:02:45]

Do you still need this?

[Yuwei, 2016/03/30 21:37:00]

Basically only observer's functions are calling these functions, but these
functions do exactly what their names say and doesn't take unnecessary arguments
like jid. I feel like keeping these functions may make the test code look
clearer...

[Yuwei, 2016/03/31 17:40:05]

Changed to SetInhibit()

+
+  // HostStatusObserver interface:

[Sergey Ulanov, 2016/03/30 21:02:45]

s/:/./

[Yuwei, 2016/03/31 17:40:05]

Done.

+
+  // inhibits CertificateWatcher when the client connects

[Sergey Ulanov, 2016/03/30 21:02:45]

Don't need comments for interface overrides..

[Yuwei, 2016/03/31 17:40:04]

Done.

+  void OnClientConnected(const std::string& jid) override;
+
+  // uninhibits CertificateWatcher when the client disconnects
+  void OnClientDisconnected(const std::string& jid) override;
+
+ private:
+  bool inhibit_mode_ = false;
+
+  // true if a restart is scheduled in inhibit mode.
+  bool inhibit_restart_scheduled_ = false;
+
+  // The time to wait to restart when it is scheduled.
+  int delay_;

[Sergey Ulanov, 2016/03/30 21:02:45]

Use base::TimeDelta for time-delta values.

[Yuwei, 2016/03/31 17:40:04]

Done.

+
+  // reference to the monitor
+  base::WeakPtr<HostStatusMonitor> monitor_;

[Sergey Ulanov, 2016/03/30 21:02:45]

Move this and all other values passed to the constructor, so that they are first
in the list.

[Yuwei, 2016/03/31 17:40:04]

Done.

+
+  // called when a restart is scheduled.
+  base::Closure restart_action_;
+
+  // called when the certificate is updated during inhibit mode.
+  // This is just for notification and it shouldn't restart the host.
+  base::Closure restart_deferred_action_;
+
+  // The file watcher to watch the certificate.

[Sergey Ulanov, 2016/03/30 21:02:45]

the watcher watches certs directory, which may contain multiple certificates.

[Yuwei, 2016/03/31 17:40:04]

Changed comments...

+  scoped_ptr<base::FilePathWatcher> file_watcher_;
+
+  // Timer to delay the restart action.
+  scoped_ptr<base::DelayTimer> restart_timer_;
+
+  // path of the NSS files/directories.
+  base::FilePath nss_watch_path_;

[Sergey Ulanov, 2016/03/30 21:02:44]

Move this above the |file_watcher_|, so the order of definition matches the
logical order in which these fields are initialized/used.

[Yuwei, 2016/03/31 17:40:04]

Done.

+
+  // called when the certificate get updated.

[Sergey Ulanov, 2016/03/30 21:02:45]

nit: Suggest rewarding: "Callback passed to |file_watcher_|." Also please notice
capital "C". Comments should start with upper-case letter.

[Yuwei, 2016/03/31 17:40:04]

Done.

+  void OnNSSUpdate(const base::FilePath& path, bool error);

[Sergey Ulanov, 2016/03/30 21:02:45]

Functions should be defined before data memebers:
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[Sergey Ulanov, 2016/03/30 21:02:45]

Suggest renaming to OnCertDirectoryChanged(). "NSSUpdate" makes little sense.

[Yuwei, 2016/03/31 17:40:05]

Done.

+
+  // called when the timer ticks.

[Sergey Ulanov, 2016/03/30 21:02:45]

This comment doesn't add anything. Maybe reword it "Called by |restart_timer_|
when it's time to restart the host."

[Yuwei, 2016/03/31 17:40:04]

Done.

+  void OnTimer();
+
+  DISALLOW_COPY_AND_ASSIGN(CertificateWatcher);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_LINUX_CERTIFICATE_WATCHER_H_