Reporting of policy errors via host-offline-reason: part 1

remoting/host/policy_hack/policy_watcher.h

 // Copyright (c) 2012 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_POLICY_HACK_POLICY_WATCHER_H_
 #define REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_
 
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 class TimeDelta;
 class WaitableEvent;
 }  // namespace base
 
 namespace remoting {
 namespace policy_hack {
 
 // Watches for changes to the managed remote access host policies.
 // If StartWatching() has been called, then before this object can be deleted,
 // StopWatching() have completed (the provided |done| event must be signaled).
 class PolicyWatcher {
  public:
   // Called first with all policies, and subsequently with any changed policies.
   typedef base::Callback<void(scoped_ptr<base::DictionaryValue>)>
-      PolicyCallback;
+      PolicyUpdatedCallback;
+
+  // Called after detecting malformed policies

[Lambros, 2014/11/13 00:30:27]

nit: Period.

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Done.

+  typedef base::Callback<void()>
+      PolicyErrorCallback;

[Lambros, 2014/11/13 00:30:27]

nit: Fits on one line.

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Done.

 
   explicit PolicyWatcher(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   virtual ~PolicyWatcher();
 
-  // This guarantees that the |policy_callback| is called at least once with
-  // the current policies.  After that, |policy_callback| will be called
-  // whenever a change to any policy is detected. It will then be called only
-  // with the changed policies.
-  virtual void StartWatching(const PolicyCallback& policy_callback);
+  // This guarantees that the |policy_updated_callback| is called at least once
+  // with the current policies.  After that, |policy_updated_callback| will be
+  // called whenever a change to any policy is detected. It will then be called
+  // only with the changed policies.
+  // |policy_error_callback| will be called when malformed policies are detected
+  // (i.e. wrong type of policy value, or unparseable files under
+  // /etc/opt/chrome/policies/managed) and indicates that we need to shut down
+  // to avoid the risk of running with incorrectly formulated policies.
+  virtual void StartWatching(
+      const PolicyUpdatedCallback& policy_updated_callback,
+      const PolicyErrorCallback& policy_error_callback);
 
   // Should be called after StartWatching() before the object is deleted. Calls
   // just wait for |done| to be signaled before deleting the object.
   virtual void StopWatching(base::WaitableEvent* done);
 
   // Implemented by each platform.  This message loop should be an IO message
   // loop.
   static PolicyWatcher* Create(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
@@ skipping 42 matching lines @@
   virtual void StopWatchingInternal() = 0;
   virtual void Reload() = 0;
 
   // Used to check if the class is on the right thread.
   bool OnPolicyWatcherThread() const;
 
   // Takes the policy dictionary from the OS specific store and extracts the
   // relevant policies.
   void UpdatePolicies(const base::DictionaryValue* new_policy);
 
+  // Signals policy error to whoever is watching this policy watcher

[Lambros, 2014/11/13 00:30:27]

s/whoever is watching this policy watcher/the registered PolicyErrorCallback/

is a bit clearer perhaps? Also, comments end with a period.

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Done.

+  void SignalPolicyError();
+
+  // Notes a transient policy error which shouldn't be reported

[Lambros, 2014/11/13 00:30:27]

I think I would document more fully what is going on here. Something like this:

Called whenever a transient error occurs during reading of policy files. This
will increment a counter, and will trigger a call to SignalPolicyError() only
after a threshold count is reached. The counter is reset whenever policy has
been successfully read.

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Done.

+  // to users of this PolicyWatcher unless
+  // |transient_policy_error_retry_counter_| has reached a threshold.
+  void SignalTransientPolicyError();
+
   // Used for time-based reloads in case something goes wrong with the
   // notification system.
   void ScheduleFallbackReloadTask();
   void ScheduleReloadTask(const base::TimeDelta& delay);
 
   // Returns a DictionaryValue containing the default values for each policy.
   const base::DictionaryValue& Defaults() const;
 
  private:
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
-  PolicyCallback policy_callback_;
+  PolicyUpdatedCallback policy_updated_callback_;
+  PolicyErrorCallback policy_error_callback_;
+  int transient_policy_error_retry_counter_;
 
   scoped_ptr<base::DictionaryValue> old_policies_;
   scoped_ptr<base::DictionaryValue> default_values_;
   scoped_ptr<base::DictionaryValue> bad_type_values_;
 
   // Allows us to cancel any inflight FileWatcher events or scheduled reloads.
   base::WeakPtrFactory<PolicyWatcher> weak_factory_;
 };
 
 }  // namespace policy_hack
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_POLICY_HACK_POLICY_WATCHER_H_