Add chromeos::NetworkStateManager

chromeos/network/network_state_handler.cc

+// 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.
+
+#include "chromeos/network/network_state_handler.h"
+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "base/values.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_device_client.h"
+#include "chromeos/dbus/shill_ipconfig_client.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/managed_state.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler_observer.h"
+#include "chromeos/network/shill_service_observer.h"
+#include "dbus/object_path.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace {
+
+void ErrorCallbackFunction(const std::string& error_name,
+                           const std::string& error_message) {
+  // TODO(stevenjb): Add error logging.
+  LOG(ERROR) << "Shill Error: " << error_name << " : " << error_message;
+}
+
+const base::ListValue* GetListValue(const std::string& key,
+                                    const base::Value& value) {
+  const base::ListValue* vlist = NULL;
+  if (!value.GetAsList(&vlist)) {
+    LOG(ERROR) << "Error parsing key as list: " << key;
+    return NULL;
+  }
+  return vlist;
+}
+
+}
+
+namespace chromeos {
+
+NetworkStateHandler::NetworkStateHandler()
+    : shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()),
+      weak_ptr_factory_(this) {
+}
+
+NetworkStateHandler::~NetworkStateHandler() {
+  STLDeleteContainerPointers(network_list_.begin(), network_list_.end());

[pneubeck (no reviews), 2012/10/26 08:17:46]

This line can be removed for scoped_vector

+  // Delete network service observers.
+  STLDeleteContainerPairSecondPointers(
+      observed_networks_.begin(), observed_networks_.end());
+  CHECK(shill_manager_ == DBusThreadManager::Get()->GetShillManagerClient());
+  shill_manager_->RemovePropertyChangedObserver(this);
+}

[pneubeck (no reviews), 2012/10/26 08:17:46]

The second STLDeleteContainerPointers(device_list_, ...) is missing.
That leads to a minor memory leak (during shutdown).
But it wouldn't be necessary and hadn't happen if we used scoped_vector :-)

[stevenjb, 2012/10/26 21:36:39]

Fixed.

+
+void NetworkStateHandler::Init() {
+  shill_manager_->GetProperties(
+      base::Bind(&NetworkStateHandler::ManagerPropertiesCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+  shill_manager_->AddPropertyChangedObserver(this);
+}
+
+void NetworkStateHandler::AddObserver(NetworkStateHandlerObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void NetworkStateHandler::RemoveObserver(
+    NetworkStateHandlerObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool NetworkStateHandler::TechnologyAvailable(
+    const std::string& technology) const {
+  return available_technologies_.find(technology) !=
+      available_technologies_.end();
+}
+
+bool NetworkStateHandler::TechnologyEnabled(
+    const std::string& technology) const {
+  return enabled_technologies_.find(technology) !=
+      enabled_technologies_.end();
+}
+
+void NetworkStateHandler::SetTechnologyEnabled(const std::string& technology,
+                                               bool enabled) {
+  if (enabled) {
+    shill_manager_->EnableTechnology(technology,
+                                     base::Bind(&base::DoNothing),
+                                     base::Bind(&ErrorCallbackFunction));
+  } else {
+    shill_manager_->DisableTechnology(technology,
+                                      base::Bind(&base::DoNothing),
+                                      base::Bind(&ErrorCallbackFunction));
+  }
+}
+
+const DeviceState* NetworkStateHandler::GetDeviceState(
+    const std::string& path) const {
+  return GetModifiableDeviceState(path);
+}
+
+const DeviceState* NetworkStateHandler::GetDeviceStateByType(
+    const std::string& type) const {
+  for (DeviceStateList::const_iterator iter = device_list_.begin();
+       iter != device_list_.end(); ++iter) {
+    const DeviceState* device = *iter;
+    if (device->type() == type)
+      return device;
+  }
+  return NULL;
+}
+
+const NetworkState* NetworkStateHandler::GetNetworkState(
+    const std::string& path) const {
+  return GetModifiableNetworkState(path);
+}
+
+const NetworkState* NetworkStateHandler::ActiveNetwork() const {
+  if (network_list_.empty())
+    return NULL;
+  const NetworkState* network = network_list_.front();
+  if (!network->IsConnectedState())
+    return NULL;
+  return network;
+}
+
+const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
+    const std::string& type) const {
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (!network->IsConnectedState())
+      break;  // Connected networks are listed first.

[gauravsh, 2012/10/26 01:15:01]

I think we should not make this assumption. It ties too much into how shill
deals with service lists. I'd rather have us keep a list of connected services
separately, or something similar and update it in response to property changed
events from shill if optimization is a concern here.

[pneubeck (no reviews), 2012/10/26 08:17:46]

Optimization shouldn't be a concern if we don't have evidence for its need, i.e
measured data.
Robustness, readability and decoupled code should be more important.

In this case, I think keeping the one list is the simplest solution. And I agree
with Gaurav that we shouldn't rely on the lists order if not necessary:

if (network->type() == type && network->IsConnectedState())
  return network;

[stevenjb, 2012/10/26 21:36:39]

This list *can* be quite long, O(100), and I prefer to avoid O(N) code in UI
accessors.
This entire code is tightly bound to Shill behavior, so I think the optimization
is valid.
I definitely want to avoid multiple lists; maintaining them in NetworkLibrary
has been a nightmare.

[gauravsh, 2012/10/27 00:26:33]

To be fair, the problem with NetworkLibrary is that that there is no single
place/entity responsible for doing this management. It's fairly ad-hoc.

One of the goals of this re-factor is to allow us to easily change Shill's API
contract going forward. In that regard, mirroring Shill's behavior with regards
to how service state is handled here is problematic.

+    if (network->type() == type)
+      return network;
+  }
+  return NULL;
+}
+
+const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
+    const std::string& type) const {
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (network->IsConnectedState())
+      continue;
+    if (!network->IsConnectingState())
+      break;  // Connected and connecting networks are listed first.
+    if (network->type() == type ||
+        (type.empty() && type != flimflam::kTypeEthernet)) {
+      return network;
+    }
+  }
+  return NULL;
+}
+
+std::string NetworkStateHandler::HardwareAddress(
+    const std::string& type) const {
+  std::string result;
+  const NetworkState* network = ConnectedNetworkByType(type);
+  if (network) {
+    const DeviceState* device = GetDeviceState(network->device_path());
+    if (device)
+      result = device->mac_address();
+  }
+  StringToUpperASCII(&result);
+  return result;
+}
+
+std::string NetworkStateHandler::HardwareAddressFormatted(
+    const std::string& type) const {
+  std::string address = HardwareAddress(type);

[gauravsh, 2012/10/26 01:15:01]

This should have an associated unit test.

+  if (address.size() % 2 != 0)
+    return address;
+  std::string result;
+  for (size_t i = 0; i < address.size(); ++i) {

[gauravsh, 2012/10/26 01:15:01]

You may consider changing this to i+=2, and also push_back(address[i+1]). This
way you don't need the div by 2 check below. It would make the code look clearer
IMO.

[stevenjb, 2012/10/26 21:36:39]

I copied this from NetworkLibrary; I'm not 100% certain I follow your
suggestion. We could optimize and test this, although it doesn't feel high
priority to me.

[gauravsh, 2012/10/27 00:26:33]

The optimization was just a suggestion to improve readability, not performance.
However, we are required to have unit tests for all non-trivial methods. 

+    if ((i != 0) && (i % 2 == 0))
+      result.push_back(':');
+    result.push_back(address[i]);
+  }
+  return result;
+}
+
+const NetworkStateHandler::NetworkStateList&
+NetworkStateHandler::GetNetworkList() const {

[gauravsh, 2012/10/26 01:15:01]

See my comment in .h about the potentially confusing semantics of this method.

+  RequestScan();
+  return network_list_;
+}
+
+void NetworkStateHandler::OnPropertyChanged(const std::string& key,
+                                            const base::Value& value) {
+  if (ManagerPropertyChanged(key, value))
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkManagerChanged(key));
+}
+
+//------------------------------------------------------------------------------
+// Private methods
+
+DeviceState* NetworkStateHandler::GetModifiableDeviceState(
+    const std::string& path) const {
+  for (DeviceStateList::const_iterator iter = device_list_.begin();
+       iter != device_list_.end(); ++iter) {
+    DeviceState* device = *iter;
+    if (device->path() == path)
+      return device;
+  }
+  return NULL;
+}
+
+NetworkState* NetworkStateHandler::GetModifiableNetworkState(
+    const std::string& path) const {
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    NetworkState* network = *iter;
+    if (network->path() == path)
+      return network;
+  }
+  return NULL;
+}
+
+void NetworkStateHandler::ManagerPropertiesCallback(
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get Manager properties:" << call_status;
+    return;
+  }
+  bool notify = false;
+  for (base::DictionaryValue::Iterator iter(properties);
+       iter.HasNext(); iter.Advance()) {
+    notify |= ManagerPropertyChanged(iter.key(), iter.value());
+  }
+  if (notify) {
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkManagerChanged(std::string()));
+  }
+}
+
+bool NetworkStateHandler::ManagerPropertyChanged(const std::string& key,
+                                                 const base::Value& value) {
+  bool notify_manager_changed = false;
+  if (key == flimflam::kServicesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
+      // Do not send an UpdateManagerChanged notification to observers.
+      // An UpdateNetworkList notification will be sent when the service list
+      // updates have completed.
+    }
+  } else if (key == flimflam::kDevicesProperty) {
+    const ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateManagedList(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
+      notify_manager_changed = true;
+    }
+  } else if (key == flimflam::kAvailableTechnologiesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist ) {
+      UpdateAvailableTechnologies(*vlist);
+      notify_manager_changed = true;
+    }
+  } else if (key == flimflam::kEnabledTechnologiesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateEnabledTechnologies(*vlist);
+      notify_manager_changed = true;
+    }
+  }
+  return notify_manager_changed;
+}
+
+NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
+    ManagedState::ManagedType type) {
+  // Use reinterpret_cast here to avoid copying the list to a list of identical
+  // base class pointers.
+  switch(type) {
+    case ManagedState::MANAGED_TYPE_NETWORK:
+      return reinterpret_cast<ManagedStateList*>(&network_list_);

[gauravsh, 2012/10/26 01:15:01]

This makes me iffy too. I wouldn't be surprised if this failed in mysterious
ways. There are 2 places ManagedStateList is used - UpdateManagedList and
GetPropertiesCallback.

It looks to me that managing the list of state like this (c.f. the code for
UpdateManagedList below) adds a substantial amount of complexity of the class.
Wouldn't it be better to have a separate ManagedStateList class - and have the
StateHandler object have 2 members of that - one for managing the list of device
states, and the other for network states.

This way a substantial bit of implementation logic to deal with these containers
can be moved out of this class. And it would also make it easy to write unit
tests for the core state management logic.

[stevenjb, 2012/10/26 21:36:39]

This has been refactored.

+    case ManagedState::MANAGED_TYPE_DEVICE:
+      return reinterpret_cast<ManagedStateList*>(&device_list_);
+  }
+  return NULL;
+}
+
+void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
+                                            const base::ListValue& entries) {
+  ManagedStateList* managed_list = GetManagedList(type);
+  VLOG(2) << "UpdateManagedList: " << type;
+  // Create a map of existing entries.
+  std::map<std::string, ManagedState*> managed_map;
+  for (ManagedStateList::iterator iter = managed_list->begin();
+       iter != managed_list->end(); ++iter) {
+    ManagedState* managed = *iter;
+    managed_map[managed->path()] = managed;
+  }
+  // Clear the list (pointers are owned by managed_map).
+  managed_list->clear();

[pneubeck (no reviews), 2012/10/26 08:17:46]

This will be weak_clear() for scoped_vector, so there is no reason to not use
it.

[stevenjb, 2012/10/26 21:36:39]

Personally I think that as soon as you use weak_clear(), you shouldn't be using
ScopedVector; I'd rather do the explicit deletes :)

+  // Updates managed_list and request updates for new entries.
+  for (base::ListValue::const_iterator iter = entries.begin();
+       iter != entries.end(); ++iter) {
+    std::string path;
+    (*iter)->GetAsString(&path);
+    if (path.empty())
+      continue;
+    std::map<std::string, ManagedState*>::iterator found =
+        managed_map.find(path);
+    bool request_properties = false;
+    if (found == managed_map.end()) {
+      request_properties = true;
+      managed_list->push_back(ManagedState::Create(type, path));
+    } else {
+      ManagedState* managed = found->second;
+      managed_list->push_back(managed);
+      managed_map.erase(found);
+    }
+    if (request_properties) {
+      ++pending_updates_[type];
+      RequestProperties(type, path);
+    }
+  }
+  // Delete any remaning entries in managed_map.
+  STLDeleteContainerPairSecondPointers(managed_map.begin(), managed_map.end());
+}
+
+void NetworkStateHandler::UpdateObservedNetworkServices() {
+  // Watch any connected or connecting networks.
+  ShillServiceObserverMap new_observed;
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (!network->IsConnectedState() && !network->IsConnectingState())
+      break;  // Connected and connecting networks are listed first.
+    const std::string& path = network->path();
+    ShillServiceObserverMap::iterator iter = observed_networks_.find(path);
+    if (iter != observed_networks_.end()) {
+      new_observed[path] = observed_networks_[path];
+    } else {
+      new_observed[path] = new ShillServiceObserver(
+          path, base::Bind(&NetworkStateHandler::NetworkServicePropertyChanged,
+                           weak_ptr_factory_.GetWeakPtr()));
+    }
+    observed_networks_.erase(path);
+  }
+  VLOG(2) << "UpdateObservedNetworkServices, new observed: "
+          << new_observed.size();
+  // Delete network service observers still in observed_networks_.
+  STLDeleteContainerPairSecondPointers(
+      observed_networks_.begin(), observed_networks_.end());
+  observed_networks_.swap(new_observed);
+  // Notify observers that the list of networks has changed.
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                    NetworkListChanged(network_list_));
+  // Notify observers if the active network has changed.
+  const NetworkState* new_active_network =
+      network_list_.empty() ? NULL : network_list_.front();
+  if (new_active_network->path() != active_network_path_) {
+    active_network_path_ = new_active_network->path();
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      ActiveNetworkChanged(new_active_network));
+  }
+}
+
+void NetworkStateHandler::UpdateAvailableTechnologies(
+    const base::ListValue& technologies) {
+  available_technologies_.clear();
+  for (base::ListValue::const_iterator iter = technologies.begin();
+       iter != technologies.end(); ++iter) {
+    std::string technology;
+    (*iter)->GetAsString(&technology);
+    if (technology.empty())

[gauravsh, 2012/10/26 01:15:01]

Should this log a warning?

[stevenjb, 2012/10/26 21:36:39]

Should never happen and wouldn't hurt anything, I'll make it a DCHECK instead.

+      continue;
+    available_technologies_.insert(technology);
+  }
+}
+
+void NetworkStateHandler::UpdateEnabledTechnologies(
+    const base::ListValue& technologies) {
+  enabled_technologies_.clear();
+  for (base::ListValue::const_iterator iter = technologies.begin();
+       iter != technologies.end(); ++iter) {
+    std::string technology;
+    (*iter)->GetAsString(&technology);
+    if (technology.empty())

[gauravsh, 2012/10/26 01:15:01]

Should this log a warning?

[stevenjb, 2012/10/26 21:36:39]

Same as above.

+      continue;
+    enabled_technologies_.insert(technology);
+  }
+}
+
+void NetworkStateHandler::RequestProperties(ManagedState::ManagedType type,
+                                            const std::string& path) {
+  switch(type) {
+    case ManagedState::MANAGED_TYPE_NETWORK:
+      DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
+          dbus::ObjectPath(path),
+          base::Bind(&NetworkStateHandler::GetPropertiesCallback,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     type, path));
+      break;
+    case ManagedState::MANAGED_TYPE_DEVICE:
+      DBusThreadManager::Get()->GetShillDeviceClient()->GetProperties(
+          dbus::ObjectPath(path),
+          base::Bind(&NetworkStateHandler::GetPropertiesCallback,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     type, path));
+      break;
+  }
+}
+
+void NetworkStateHandler::GetPropertiesCallback(
+    ManagedState::ManagedType type,
+    const std::string& path,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  VLOG(2) << "GetPropertiesCallback: " << type << " : " << path;
+  --pending_updates_[type];
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get properties for: " << path
+               << ": " << call_status;
+    return;
+  }
+  bool found = false;
+  ManagedStateList* list = GetManagedList(type);
+  for (ManagedStateList::iterator iter = list->begin();
+       iter != list->end(); ++iter) {
+    ManagedState* managed = *iter;
+    if (managed->path() == path) {
+      ParseProperties(managed, properties);
+      found = true;
+      break;
+    }
+  }
+  if (!found)
+    LOG(ERROR) << "GetPropertiesCallback: " << path << " Not found!";
+  // Notify observers only when all updates for that type have completed.
+  if (type == ManagedState::MANAGED_TYPE_NETWORK &&
+      pending_updates_[type] == 0) {
+    UpdateObservedNetworkServices();
+  }
+}
+
+void NetworkStateHandler::ParseProperties(
+    ManagedState* managed,
+    const base::DictionaryValue& properties) {
+  for (base::DictionaryValue::Iterator iter(properties);
+       iter.HasNext(); iter.Advance()) {
+    bool success = false;
+    // Handle IPConfig here since it requires additional complexity that is
+    // better handled here than in NetworkState::PropertyChanged (e.g.
+    // observer notification).
+    if (iter.key() == shill::kIPConfigProperty) {
+      DCHECK(managed->managed_type() == ManagedState::MANAGED_TYPE_NETWORK);
+      std::string ip_config_path;
+      if (iter.value().GetAsString(&ip_config_path)) {
+        DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(
+            dbus::ObjectPath(ip_config_path),
+            base::Bind(&NetworkStateHandler::GetIPConfigCallback,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       managed->path()));
+        success = true;
+      }
+    } else {
+      success = managed->PropertyChanged(iter.key(), iter.value());
+    }
+    if (!success)
+      LOG(ERROR) << "Error getting value for key: " << iter.key();
+  }
+}
+
+void NetworkStateHandler::NetworkServicePropertyChanged(
+    const std::string& path,
+    const std::string& key,
+    const base::Value& value) {
+  NetworkState* network = GetModifiableNetworkState(path);
+  if (!network)
+    return;
+  if (network->PropertyChanged(key, value)) {
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkServicePropertyChanged(network, key));
+    if (network == network_list_.front() && key == flimflam::kStateProperty) {

[gauravsh, 2012/10/26 01:15:01]

Is there a guarantee that the manager update with a new list of services would
have already been processed (i.e. network_list_ updates) such that this check
succeeds when it should?

This is the kind of race scenario that I am worried about. We shouldn't rely on
order of networks in network_list to keep track of the active/connected service
and instead manage them separately.

[stevenjb, 2012/10/26 21:36:39]

I think that separate management would add the kind complexity I am trying to
avoid.

If the order of network_list_ changes, then we will call ActiveNetworkChanged(),
which any client would also need to observe.

Shill processes requests in-order, so it should either complete the service
requests before sending an update, or vice-versa. If that were not true, there
is no way we could consistently track state (or it would be extremely
complicated).

[gauravsh, 2012/10/27 00:26:33]

Since dbus signals are handled asynchronously, this still depends on the race
between which signal is handled first. I don't think managing the active network
explicitly will add to the complexity. The bigger problem is that false
positives on active network changes is disruptive to NCN - as it impacts
existing connections. We should minimize that.

+      FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                        ActiveNetworkStateChanged(network));

[gauravsh, 2012/10/26 01:15:01]

Where do you notify the observes when we transition from have an active network
-> no active network?

[stevenjb, 2012/10/26 21:36:39]

In UpdateObservedNetworkServices() (now NotifyNetworkServiceObservers())

+    }
+  }
+}
+
+void NetworkStateHandler::GetIPConfigCallback(
+    const std::string& service_path,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties)  {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get IP properties for: " << service_path;
+    return;
+  }
+  std::string ip_address;
+  if (!properties.GetStringWithoutPathExpansion(flimflam::kAddressProperty,
+                                                &ip_address)) {
+    LOG(ERROR) << "Failed to get IP Address property for: " << service_path;
+    return;
+  }
+  NetworkState* network = GetModifiableNetworkState(service_path);
+  if (!network)
+    return;
+  network->set_ip_address(ip_address);
+  FOR_EACH_OBSERVER(
+      NetworkStateHandlerObserver, observers_,
+      NetworkServicePropertyChanged(network, flimflam::kAddressProperty));
+}
+
+void NetworkStateHandler::RequestScan() const {
+  shill_manager_->RequestScan("",
+                              base::Bind(&base::DoNothing),
+                              base::Bind(&ErrorCallbackFunction));
+}
+
+}  // namespace chromeos