Offline state detection for linux, using new D-Bus library.

net/base/network_change_notifier_linux.cc

Index: net/base/network_change_notifier_linux.cc
diff --git a/net/base/network_change_notifier_linux.cc b/net/base/network_change_notifier_linux.cc
index 45ee139fc28b38e0d1c4eb1c1706199307788238..54d9862d152713e1ef8c6c5ecbe4b8be4ce2ad1a 100644
--- a/net/base/network_change_notifier_linux.cc
+++ b/net/base/network_change_notifier_linux.cc
@@ -8,13 +8,20 @@
 #include <sys/socket.h>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/callback_old.h"
 #include "base/compiler_specific.h"
 #include "base/eintr_wrapper.h"
 #include "base/file_util.h"
 #include "base/files/file_path_watcher.h"
+#include "base/memory/weak_ptr.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/synchronization/lock.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_change_notifier_netlink_linux.h"
 
@@ -26,6 +33,182 @@ namespace {
 
 const int kInvalidSocket = -1;
 
+const char kNetworkManagerServiceName[] = "org.freedesktop.NetworkManager";
+const char kNetworkManagerPath[] = "/org/freedesktop/NetworkManager";
+const char kNetworkManagerInterface[] = "org.freedesktop.NetworkManager";
+
+// http://projects.gnome.org/NetworkManager/developers/spec-08.html#type-NM_STATE
+enum {
+  NM_LEGACY_STATE_UNKNOWN = 0,
+  NM_LEGACY_STATE_ASLEEP = 1,
+  NM_LEGACY_STATE_CONNECTING = 2,
+  NM_LEGACY_STATE_CONNECTED = 3,
+  NM_LEGACY_STATE_DISCONNECTED = 4
+};
+
+// http://projects.gnome.org/NetworkManager/developers/migrating-to-09/spec.html#type-NM_STATE
+enum {
+  NM_STATE_UNKNOWN = 0,
+  NM_STATE_ASLEEP = 10,
+  NM_STATE_DISCONNECTED = 20,
+  NM_STATE_DISCONNECTING = 30,
+  NM_STATE_CONNECTING = 40,
+  NM_STATE_CONNECTED_LOCAL = 50,
+  NM_STATE_CONNECTED_SITE = 60,
+  NM_STATE_CONNECTED_GLOBAL = 70
+};
+
+class NetworkManagerApi : public base::SupportsWeakPtr<NetworkManagerApi> {

[satorux1, 2011/10/12 22:28:43]

Please write a class comment.

BTW, I think WeakPtrFactory is slightly preferable if you don't need to expose a
weak pointer, but I don't have a strong opinion on this.

[adamk, 2011/10/13 01:46:17]

Added a comment and used WeakPtrFactory.

+ public:
+  typedef base::Callback<void(void)> NotificationCallback;
+  explicit NetworkManagerApi(NotificationCallback notification_callback);
+  ~NetworkManagerApi();
+
+  void Init();
+  bool IsCurrentlyOffline();

[satorux1, 2011/10/12 22:28:43]

Function comments are missing. One line comments would suffix for these. Please
check other places too.

[adamk, 2011/10/13 01:46:17]

I've added comments for each of these (and a few others below), but I don't
think every method needs a comment.  Let me know if there are other places where
you think explanation is warranted. 

[satorux1, 2011/10/13 06:58:05]

Thank you for adding comments. Sorry for being nit-picky. It was from our C++
style guide.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it. 

[adamk, 2011/10/13 18:14:06]

Understood, I guess my point is that there are some methods that don't need
comments (e.g.,
NetworkChangeNotifierLinux::Thread::NotifyObserversOfOnlineStateChange, with its
verbose name and one-line body).

+
+ private:
+  enum OnlineState {
+    UNINITIALIZED = -1,
+    OFFLINE = 0,
+    ONLINE = 1
+  };
+
+  void NotifyObserversOfOnlineStateChange() {
+    notification_callback_.Run();
+  }
+  void OnStateChanged(dbus::Signal* signal);
+  void OnConnected(const std::string&, const std::string&, bool success) {
+    if (!success)
+      LOG(WARNING) << "Failed to set up offline state detection";

[satorux1, 2011/10/12 22:28:43]

LOG_IF(WARNING, !success) is a bit more succinct.

[adamk, 2011/10/13 01:46:17]

Done.

+  }
+
+  static uint32 GetCurrentState(dbus::ObjectProxy* proxy);
+  static OnlineState TranslateState(uint32 state);
+
+  OnlineState online_state_;
+  base::Lock online_state_lock_;
+  base::ConditionVariable initial_state_cv_;

[satorux1, 2011/10/12 22:28:43]

I guess you can use WaitedEvent instead?

[satorux1, 2011/10/12 22:29:50]

I meant WaitableEvent in base/synchronization

[adamk, 2011/10/13 01:46:17]

Hmm, it's not obvious to me that it's a win to use a WaitableEvent, given that I
already have a mutex guarding online_state_.  Let me know if you feel if you
feel it's important to use WaitableEvent for clarity.

[satorux1, 2011/10/13 06:58:05]

That's a good point. Generally WaitableEvent is simpler than ConditionVariable +
Lock, but you are right that the lock is already there, so use of WaitableEvent
wouldn't be a win. You could remove while(...) around Wait() but it's not a big 
deal.

[adamk, 2011/10/13 18:14:06]

Yeah, I guess there's no good reason _not_ to use a WaitableEvent, so I'll just
do that.

+
+  NotificationCallback notification_callback_;
+
+  scoped_refptr<dbus::Bus> system_bus_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkManagerApi);
+};
+
+NetworkManagerApi::NetworkManagerApi(NotificationCallback notification_callback)
+    : online_state_(UNINITIALIZED),
+      initial_state_cv_(&online_state_lock_),
+      notification_callback_(notification_callback) {
+}
+
+NetworkManagerApi::~NetworkManagerApi() {
+}
+
+void NetworkManagerApi::Init() {

[satorux1, 2011/10/12 22:28:43]

What thread is the function running on? I assume it's not on UI thread

Maybe you might want to add something like DCHECK(CurrentlyOn(...));

[adamk, 2011/10/13 01:46:17]

It's run on the NetworkChangeNotifier's helper thread (see
NetworkChangeNotifierLinux::Thread::Init() for the call).
I've added a DCHECK that it's an IO thread, and stored the thread ID for
DCHECKs.  Let me know if I'm going overboard.

[satorux1, 2011/10/13 06:58:05]

Thanks. I think it'd be nice to add a comment about it, as it's not obvious
here.

[adamk, 2011/10/13 18:14:06]

I added a comment in the declaration saying how to use it, it's really up to the
caller (NetworkChangeNotifierLinux::Thread) to ensure that it actually runs on
the right thread.

+  dbus::Bus::Options options;
+  options.bus_type = dbus::Bus::SYSTEM;
+  options.connection_type = dbus::Bus::PRIVATE;
+  system_bus_ = new dbus::Bus(options);

[satorux1, 2011/10/12 22:28:43]

I suppose the bus works on the current thread because the message loop of the
current thread is MessageLoopForIO.

[adamk, 2011/10/13 01:46:17]

Yup, added a DCHECK for this (as noted above).

+
+  dbus::ObjectProxy* proxy =
+      system_bus_->GetObjectProxy(kNetworkManagerServiceName,
+                                  kNetworkManagerPath);
+  OnlineState online_state = TranslateState(GetCurrentState(proxy));
+  {
+    base::AutoLock lock(online_state_lock_);
+    online_state_ = online_state;
+    initial_state_cv_.Signal();
+  }
+
+  proxy->ConnectToSignal(
+      kNetworkManagerInterface,
+      "StateChanged",
+      base::Bind(&NetworkManagerApi::OnStateChanged, AsWeakPtr()),
+      base::Bind(&NetworkManagerApi::OnConnected, AsWeakPtr()));
+}
+
+void NetworkManagerApi::OnStateChanged(dbus::Signal* signal) {
+  DCHECK(signal);
+  dbus::MessageReader reader(signal);
+  uint32 state;
+  if (!reader.PopUint32(&state)) {
+    LOG(WARNING) << "Unexpected response for NetworkManager State request: "
+                 << signal->ToString();
+    return;
+  }
+  OnlineState new_online_state = TranslateState(state);
+  {
+    base::AutoLock lock(online_state_lock_);
+    if (new_online_state != online_state_)
+      online_state_ = new_online_state;
+    else
+      return;
+  }
+  // Can't notify while we hold the lock
+  NotifyObserversOfOnlineStateChange();
+}
+
+uint32 NetworkManagerApi::GetCurrentState(dbus::ObjectProxy* proxy) {
+  dbus::MethodCall method_call(DBUS_INTERFACE_PROPERTIES, "Get");
+  dbus::MessageWriter builder(&method_call);
+  builder.AppendString(kNetworkManagerInterface);
+  builder.AppendString("State");
+  scoped_ptr<dbus::Response> response(
+      proxy->CallMethodAndBlock(&method_call,
+                                dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));

[satorux1, 2011/10/12 22:28:43]

Not sure if it's ok to do a blocking method call here. What thread is the
function running on?

[adamk, 2011/10/13 01:46:17]

Again, the helper thread.  I wouldn't mind making this async if that'd make you
less worried.

[satorux1, 2011/10/13 06:58:05]

If it's in a dedicated thread, a blocking call would be fine. I was just
worrying if the code was using a shared thread like FILE and could other tasks.
You might want to add a comment why it's fine to do a blocking call here.

[adamk, 2011/10/13 18:14:06]

Added a comment.

+  if (!response.get())
+    return NM_STATE_UNKNOWN;
+
+  dbus::MessageReader reader(response.get());
+  uint32 state;
+  if (!reader.PopUint32(&state)) {
+    LOG(WARNING) << "Unexpected response for NetworkManager State request: "
+                 << response->ToString();
+    return NM_STATE_UNKNOWN;
+  }
+
+  return state;
+}
+
+NetworkManagerApi::OnlineState NetworkManagerApi::TranslateState(uint32 state) {
+  switch (state) {
+    case NM_LEGACY_STATE_CONNECTED:
+    case NM_STATE_CONNECTED_SITE:
+    case NM_STATE_CONNECTED_GLOBAL:
+      // Definitely connected
+      return ONLINE;
+    case NM_LEGACY_STATE_DISCONNECTED:
+    case NM_STATE_DISCONNECTED:
+      // Definitely disconnected
+      return OFFLINE;
+    case NM_STATE_CONNECTED_LOCAL:
+      // Local networking only; I'm treating this as offline (keybuk)
+      return OFFLINE;
+    case NM_LEGACY_STATE_CONNECTING:
+    case NM_STATE_DISCONNECTING:
+    case NM_STATE_CONNECTING:
+      // In-flight change to connection status currently underway
+      return OFFLINE;
+    case NM_LEGACY_STATE_ASLEEP:
+    case NM_STATE_ASLEEP:
+      // Networking disabled or no devices on system
+      return OFFLINE;
+    default:
+      // Unknown status
+      return ONLINE;
+  }
+}
+
+bool NetworkManagerApi::IsCurrentlyOffline() {
+  base::AutoLock lock(online_state_lock_);
+  while (online_state_ == UNINITIALIZED) {
+    initial_state_cv_.Wait();
+  }
+  return online_state_ == OFFLINE;
+}
+
 class DNSWatchDelegate : public FilePathWatcher::Delegate {
  public:
   explicit DNSWatchDelegate(Callback0::Type* callback)
@@ -61,6 +244,10 @@ class NetworkChangeNotifierLinux::Thread
   virtual void OnFileCanReadWithoutBlocking(int fd);
   virtual void OnFileCanWriteWithoutBlocking(int /* fd */);
 
+  bool IsCurrentlyOffline() {
+    return network_manager_api_.IsCurrentlyOffline();
+  }
+
  protected:
   // base::Thread
   virtual void Init();
@@ -75,6 +262,10 @@ class NetworkChangeNotifierLinux::Thread
     NetworkChangeNotifier::NotifyObserversOfDNSChange();
   }
 
+  static void NotifyObserversOfOnlineStateChange() {
+    NetworkChangeNotifier::NotifyObserversOfOnlineStateChange();
+  }
+
   // Starts listening for netlink messages.  Also handles the messages if there
   // are any available on the netlink socket.
   void ListenForNotifications();
@@ -96,13 +287,19 @@ class NetworkChangeNotifierLinux::Thread
   scoped_ptr<base::files::FilePathWatcher> hosts_file_watcher_;
   scoped_refptr<DNSWatchDelegate> file_watcher_delegate_;
 
+  // Used to detect online/offline state changes.
+  NetworkManagerApi network_manager_api_;
+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
 NetworkChangeNotifierLinux::Thread::Thread()
     : base::Thread("NetworkChangeNotifier"),
       netlink_fd_(kInvalidSocket),
-      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
+      network_manager_api_(
+          base::Bind(&NetworkChangeNotifierLinux::Thread
+                     ::NotifyObserversOfOnlineStateChange)) {
 }
 
 NetworkChangeNotifierLinux::Thread::~Thread() {}
@@ -127,6 +324,8 @@ void NetworkChangeNotifierLinux::Thread::Init() {
     return;
   }
   ListenForNotifications();
+
+  network_manager_api_.Init();
 }
 
 void NetworkChangeNotifierLinux::Thread::CleanUp() {
@@ -218,8 +417,7 @@ NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() {
 }
 
 bool NetworkChangeNotifierLinux::IsCurrentlyOffline() const {
-  // TODO(eroman): http://crbug.com/53473
-  return false;
+  return notifier_thread_->IsCurrentlyOffline();
 }
 
 }  // namespace net