Use setProperties for IP Config.

chromeos/network/network_configuration_handler.cc

Index: chromeos/network/network_configuration_handler.cc
diff --git a/chromeos/network/network_configuration_handler.cc b/chromeos/network/network_configuration_handler.cc
index 1ad09d80d046c05d4a67930f1b46324ac74da084..85a79c703d9ee69b37fd397bc887d552524e84db 100644
--- a/chromeos/network/network_configuration_handler.cc
+++ b/chromeos/network/network_configuration_handler.cc
@@ -20,6 +20,7 @@
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/network_device_handler.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -34,8 +35,8 @@ namespace {
 // Strip surrounding "" from keys (if present).
 std::string StripQuotations(const std::string& in_str) {
   size_t len = in_str.length();
-  if (len >= 2 && in_str[0] == '"' && in_str[len-1] == '"')
-    return in_str.substr(1, len-2);
+  if (len >= 2 && in_str[0] == '"' && in_str[len - 1] == '"')
+    return in_str.substr(1, len - 2);
   return in_str;
 }
 
@@ -44,8 +45,8 @@ void InvokeErrorCallback(const std::string& service_path,
                          const std::string& error_name) {
   std::string error_msg = "Config Error: " + error_name;
   NET_LOG_ERROR(error_msg, service_path);
-  network_handler::RunErrorCallback(
-      error_callback, service_path, error_name, error_msg);
+  network_handler::RunErrorCallback(error_callback, service_path, error_name,
+                                    error_msg);
 }
 
 void GetPropertiesCallback(
@@ -57,8 +58,7 @@ void GetPropertiesCallback(
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     // Because network services are added and removed frequently, we will see
     // failures regularly, so don't log these.
-    network_handler::RunErrorCallback(error_callback,
-                                      service_path,
+    network_handler::RunErrorCallback(error_callback, service_path,
                                       network_handler::kDBusFailedError,
                                       network_handler::kDBusFailedErrorMessage);
     return;
@@ -82,23 +82,29 @@ void SetNetworkProfileErrorCallback(
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
   network_handler::ShillErrorCallbackFunction(
-      "Config.SetNetworkProfile Failed: " + profile_path,
-      service_path, error_callback,
-      dbus_error_name, dbus_error_message);
+      "Config.SetNetworkProfile Failed: " + profile_path, service_path,
+      error_callback, dbus_error_name, dbus_error_message);
 }
 
 void LogConfigProperties(const std::string& desc,
                          const std::string& path,
                          const base::DictionaryValue& properties) {
-  for (base::DictionaryValue::Iterator iter(properties);
-       !iter.IsAtEnd(); iter.Advance()) {
+  for (base::DictionaryValue::Iterator iter(properties); !iter.IsAtEnd();
+       iter.Advance()) {
     std::string v = "******";
     if (!shill_property_util::IsPassphraseKey(iter.key()))
       base::JSONWriter::Write(&iter.value(), &v);
-    NET_LOG_DEBUG(desc,  path + "." + iter.key() + "=" + v);
+    NET_LOG_DEBUG(desc, path + "." + iter.key() + "=" + v);
   }
 }
 
+bool PropertyRequiresIPConfigRefresh(const std::string& key) {
+  return key == shill::kStaticIPAddressProperty ||
+         key == shill::kStaticIPPrefixlenProperty ||
+         key == shill::kStaticIPGatewayProperty ||
+         key == shill::kStaticIPNameServersProperty;
+}
+
 }  // namespace
 
 // Helper class to request from Shill the profile entries associated with a
@@ -119,12 +125,12 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
         guid_(guid),
         source_(source),
         callback_(callback),
-        error_callback_(error_callback) {
-  }
+        error_callback_(error_callback) {}
 
   void Run() {
-    DBusThreadManager::Get()->GetShillServiceClient()->
-        GetLoadableProfileEntries(
+    DBusThreadManager::Get()
+        ->GetShillServiceClient()
+        ->GetLoadableProfileEntries(
             dbus::ObjectPath(service_path_),
             base::Bind(&ProfileEntryDeleter::GetProfileEntriesToDeleteCallback,
                        AsWeakPtr()));
@@ -135,46 +141,49 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
       DBusMethodCallStatus call_status,
       const base::DictionaryValue& profile_entries) {
     if (call_status != DBUS_METHOD_CALL_SUCCESS) {
-      InvokeErrorCallback(
-          service_path_, error_callback_, "GetLoadableProfileEntriesFailed");
+      InvokeErrorCallback(service_path_, error_callback_,
+                          "GetLoadableProfileEntriesFailed");
       // ProfileEntryDeleterCompleted will delete this.
       owner_->ProfileEntryDeleterCompleted(service_path_, guid_, source_,
                                            false /* failed */);
       return;
     }
 
-    for (base::DictionaryValue::Iterator iter(profile_entries);
-         !iter.IsAtEnd(); iter.Advance()) {
+    for (base::DictionaryValue::Iterator iter(profile_entries); !iter.IsAtEnd();
+         iter.Advance()) {
       std::string profile_path = StripQuotations(iter.key());
       std::string entry_path;
       iter.value().GetAsString(&entry_path);
       if (profile_path.empty() || entry_path.empty()) {
-        NET_LOG_ERROR("Failed to parse Profile Entry", base::StringPrintf(
-            "%s: %s", profile_path.c_str(), entry_path.c_str()));
+        NET_LOG_ERROR("Failed to parse Profile Entry",
+                      base::StringPrintf("%s: %s", profile_path.c_str(),
+                                         entry_path.c_str()));
         continue;
       }
       if (profile_delete_entries_.count(profile_path) != 0) {
-        NET_LOG_ERROR("Multiple Profile Entries", base::StringPrintf(
-            "%s: %s", profile_path.c_str(), entry_path.c_str()));
+        NET_LOG_ERROR("Multiple Profile Entries",
+                      base::StringPrintf("%s: %s", profile_path.c_str(),
+                                         entry_path.c_str()));
         continue;
       }
-      NET_LOG_DEBUG("Delete Profile Entry", base::StringPrintf(
-          "%s: %s", profile_path.c_str(), entry_path.c_str()));
+      NET_LOG_DEBUG("Delete Profile Entry",
+                    base::StringPrintf("%s: %s", profile_path.c_str(),
+                                       entry_path.c_str()));
       profile_delete_entries_[profile_path] = entry_path;
       DBusThreadManager::Get()->GetShillProfileClient()->DeleteEntry(
-          dbus::ObjectPath(profile_path),
-          entry_path,
+          dbus::ObjectPath(profile_path), entry_path,
           base::Bind(&ProfileEntryDeleter::ProfileEntryDeletedCallback,
                      AsWeakPtr(), profile_path, entry_path),
-          base::Bind(&ProfileEntryDeleter::ShillErrorCallback,
-                     AsWeakPtr(), profile_path, entry_path));
+          base::Bind(&ProfileEntryDeleter::ShillErrorCallback, AsWeakPtr(),
+                     profile_path, entry_path));
     }
   }
 
   void ProfileEntryDeletedCallback(const std::string& profile_path,
                                    const std::string& entry) {
-    NET_LOG_DEBUG("Profile Entry Deleted", base::StringPrintf(
-        "%s: %s", profile_path.c_str(), entry.c_str()));
+    NET_LOG_DEBUG(
+        "Profile Entry Deleted",
+        base::StringPrintf("%s: %s", profile_path.c_str(), entry.c_str()));
     profile_delete_entries_.erase(profile_path);
     if (!profile_delete_entries_.empty())
       return;
@@ -232,8 +241,8 @@ void NetworkConfigurationHandler::GetProperties(
   NET_LOG_USER("GetProperties", service_path);
   DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
       dbus::ObjectPath(service_path),
-      base::Bind(&GetPropertiesCallback,
-                 callback, error_callback, service_path));
+      base::Bind(&GetPropertiesCallback, callback, error_callback,
+                 service_path));
 }
 
 void NetworkConfigurationHandler::SetProperties(
@@ -248,16 +257,48 @@ void NetworkConfigurationHandler::SetProperties(
     return;
   }
   NET_LOG_USER("SetProperties", service_path);
-  LogConfigProperties("SetProperty", service_path, properties);
 
+  // Copy |properties| to |properties_to_set| then iterate over |properties| and
+  // move any NULL properties to |properties_to_clear|, removing them from
+  // |properties_to_set|.
+  scoped_ptr<base::DictionaryValue> properties_to_set(properties.DeepCopy());
+  std::vector<std::string> properties_to_clear;
+  for (base::DictionaryValue::Iterator iter(properties); !iter.IsAtEnd();
+       iter.Advance()) {
+    const std::string& key = iter.key();
+    const base::Value& value = iter.value();
+    if (value.IsType(base::Value::TYPE_NULL)) {
+      properties_to_set->RemoveWithoutPathExpansion(key, nullptr);
+      properties_to_clear.push_back(key);
+    } else if (value.IsType(base::Value::TYPE_DICTIONARY)) {

[pneubeck (no reviews), 2014/12/05 09:47:46]

isn't this rather dangerous if you want to support partial updates? If ever an
empty dictionary ends up in a setProperties dictionary by mistake, the settings
will be removed.

the code that you changed in onc_to_shill translation sets NULL and not an empty
dict. Is this change then necessary at all?

[stevenjb, 2015/01/08 00:44:39]

Removed (for now at least) since we are using empty dictionaries for IP config.

+      // Also clear empty dictionaries.
+      const base::DictionaryValue* dictionary;
+      if (!value.GetAsDictionary(&dictionary) || dictionary->empty()) {

[pneubeck (no reviews), 2014/12/05 09:47:46]

this check for !GetAsDictionary after IsType(Dictionary) reads a bit confusing.
Could you instead replace IsType by value.GetAsDictionary ?

[stevenjb, 2015/01/08 00:44:39]

Acknowledged.

+        properties_to_set->RemoveWithoutPathExpansion(key, nullptr);
+        properties_to_clear.push_back(key);
+      }
+    }
+  }
+
+  // Copy input properties to pass to observers on completion.
   scoped_ptr<base::DictionaryValue> properties_copy(properties.DeepCopy());
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
-      dbus::ObjectPath(service_path), properties,
-      base::Bind(&NetworkConfigurationHandler::SetPropertiesSuccessCallback,
-                 AsWeakPtr(), service_path, base::Passed(&properties_copy),
-                 source, callback),
-      base::Bind(&NetworkConfigurationHandler::SetPropertiesErrorCallback,
-                 AsWeakPtr(), service_path, error_callback));
+
+  if (!properties_to_clear.empty()) {
+    // ClearProperties chains a Shill GetProperties request followed by a Shill
+    // ClearProperties request, so wait for those to complete before setting any
+    // properties.
+    ClearProperties(

[pneubeck (no reviews), 2014/12/05 09:47:46]

is this behavior covered in NCH unit tests already?
considering the complexity of this, it better should be.

[stevenjb, 2015/01/08 00:44:39]

Removed for now.

+        service_path, properties_to_clear,
+        base::Bind(&NetworkConfigurationHandler::SendSetPropertiesToShill,
+                   AsWeakPtr(), service_path, base::Passed(&properties_copy),
+                   base::Passed(&properties_to_set), source, callback,
+                   error_callback),
+        error_callback);
+  } else {
+    SendSetPropertiesToShill(service_path, properties_copy.Pass(),
+                             properties_to_set.Pass(), source, callback,
+                             error_callback);
+  }
 }
 
 void NetworkConfigurationHandler::ClearProperties(
@@ -271,17 +312,12 @@ void NetworkConfigurationHandler::ClearProperties(
     return;
   }
   NET_LOG_USER("ClearProperties", service_path);
-  for (std::vector<std::string>::const_iterator iter = names.begin();
-       iter != names.end(); ++iter) {
-    NET_LOG_DEBUG("ClearProperty", service_path + "." + *iter);
-  }
-  DBusThreadManager::Get()->GetShillServiceClient()->ClearProperties(
+  // Shill triggers an error if we clear properties that are unset, so
+  // first get the current properties.
+  DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
       dbus::ObjectPath(service_path),
-      names,
-      base::Bind(&NetworkConfigurationHandler::ClearPropertiesSuccessCallback,
-                 AsWeakPtr(), service_path, names, callback),
-      base::Bind(&NetworkConfigurationHandler::ClearPropertiesErrorCallback,
-                 AsWeakPtr(), service_path, error_callback));
+      base::Bind(&NetworkConfigurationHandler::ClearPropertiesGetCallback,
+                 AsWeakPtr(), service_path, names, callback, error_callback));
 }
 
 void NetworkConfigurationHandler::CreateConfiguration(
@@ -297,8 +333,7 @@ void NetworkConfigurationHandler::CreateConfiguration(
   if (NetworkTypePattern::Ethernet().MatchesType(type)) {
     InvokeErrorCallback(
         shill_property_util::GetNetworkIdFromProperties(properties),
-        error_callback,
-        "ConfigureServiceForProfile: Invalid type: " + type);
+        error_callback, "ConfigureServiceForProfile: Invalid type: " + type);
     return;
   }
 
@@ -328,8 +363,8 @@ void NetworkConfigurationHandler::RemoveConfiguration(
   // Service.Remove is not reliable. Instead, request the profile entries
   // for the service and remove each entry.
   if (ContainsKey(profile_entry_deleters_, service_path)) {
-    InvokeErrorCallback(
-        service_path, error_callback, "RemoveConfigurationInProgress");
+    InvokeErrorCallback(service_path, error_callback,
+                        "RemoveConfigurationInProgress");
     return;
   }
 
@@ -369,13 +404,15 @@ NetworkConfigurationHandler::NetworkConfigurationHandler()
 }
 
 NetworkConfigurationHandler::~NetworkConfigurationHandler() {
-  STLDeleteContainerPairSecondPointers(
-      profile_entry_deleters_.begin(), profile_entry_deleters_.end());
+  STLDeleteContainerPairSecondPointers(profile_entry_deleters_.begin(),
+                                       profile_entry_deleters_.end());
 }
 
 void NetworkConfigurationHandler::Init(
-    NetworkStateHandler* network_state_handler) {
+    NetworkStateHandler* network_state_handler,
+    NetworkDeviceHandler* network_device_handler) {
   network_state_handler_ = network_state_handler;
+  network_device_handler_ = network_device_handler;
 }
 
 void NetworkConfigurationHandler::RunCreateNetworkCallback(
@@ -424,9 +461,44 @@ void NetworkConfigurationHandler::SetNetworkProfileCompleted(
       OnConfigurationProfileChanged(service_path, profile_path, source));
 }
 
+void NetworkConfigurationHandler::SendSetPropertiesToShill(
+    const std::string& service_path,
+    scoped_ptr<base::DictionaryValue> changed_properties,
+    scoped_ptr<base::DictionaryValue> properties_to_set,
+    NetworkConfigurationObserver::Source source,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
+  if (properties_to_set->empty()) {
+    SetPropertiesSuccessCallback(service_path, changed_properties.Pass(),
+                                 source, callback);
+    return;
+  }
+
+  LogConfigProperties("SetProperty", service_path, *properties_to_set);
+  DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
+      dbus::ObjectPath(service_path), *properties_to_set,
+      base::Bind(&NetworkConfigurationHandler::SetPropertiesSuccessCallback,
+                 AsWeakPtr(), service_path, base::Passed(&changed_properties),
+                 source, callback),
+      base::Bind(&NetworkConfigurationHandler::SetPropertiesErrorCallback,
+                 AsWeakPtr(), service_path, error_callback));
+
+  // If we set any properties that might affect IP config, request an IP config
+  // refresh after calling SetProperties. Note: it is possible (but unlikely and
+  // harmless) for both Set and Clear to trigger an IP Config refresh.
+  for (base::DictionaryValue::Iterator iter(*properties_to_set);
+       !iter.IsAtEnd(); iter.Advance()) {
+    const std::string& key = iter.key();
+    if (PropertyRequiresIPConfigRefresh(key)) {
+      RequestRefreshIPConfigs(service_path);
+      break;
+    }
+  }
+}
+
 void NetworkConfigurationHandler::SetPropertiesSuccessCallback(
     const std::string& service_path,
-    scoped_ptr<base::DictionaryValue> set_properties,
+    scoped_ptr<base::DictionaryValue> changed_properties,
     NetworkConfigurationObserver::Source source,
     const base::Closure& callback) {
   if (!callback.is_null())
@@ -438,7 +510,7 @@ void NetworkConfigurationHandler::SetPropertiesSuccessCallback(
 
   FOR_EACH_OBSERVER(NetworkConfigurationObserver, observers_,
                     OnPropertiesSet(service_path, network_state->guid(),
-                                    *set_properties, source));
+                                    *changed_properties, source));
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
@@ -448,21 +520,62 @@ void NetworkConfigurationHandler::SetPropertiesErrorCallback(
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
   network_handler::ShillErrorCallbackFunction(
-      "Config.SetProperties Failed",
-      service_path, error_callback,
+      "Config.SetProperties Failed", service_path, error_callback,
       dbus_error_name, dbus_error_message);
   // Some properties may have changed so request an update regardless.
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
+void NetworkConfigurationHandler::ClearPropertiesGetCallback(
+    const std::string& service_path,
+    const std::vector<std::string>& names,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    network_handler::RunErrorCallback(error_callback, service_path,
+                                      network_handler::kDBusFailedError,
+                                      network_handler::kDBusFailedErrorMessage);
+    return;
+  }
+
+  // Clear only properties that are currently set. Also trigger an IP Configs
+  // refresh if necessary (see note in SendSetPropertiesToShill).
+  std::vector<std::string> properties_to_clear;
+  bool refresh_ip_configs = false;
+  for (const auto& key : names) {
+    if (properties.HasKey(key)) {
+      NET_LOG_DEBUG("ClearProperty", service_path + "." + key);
+      properties_to_clear.push_back(key);
+      if (PropertyRequiresIPConfigRefresh(key))
+        refresh_ip_configs = true;
+    } else {
+      NET_LOG_DEBUG("ClearProperty:Unset (Ignoring)", service_path + "." + key);
+    }
+  }
+
+  DBusThreadManager::Get()->GetShillServiceClient()->ClearProperties(
+      dbus::ObjectPath(service_path), properties_to_clear,
+      base::Bind(&NetworkConfigurationHandler::ClearPropertiesSuccessCallback,
+                 AsWeakPtr(), service_path, properties_to_clear, callback),
+      base::Bind(&NetworkConfigurationHandler::ClearPropertiesErrorCallback,
+                 AsWeakPtr(), service_path, error_callback));
+
+  // If we cleared any properties that might affect IP config, request an IP
+  // config refresh after calling ClearProperties.
+  if (refresh_ip_configs)
+    RequestRefreshIPConfigs(service_path);
+}
+
 void NetworkConfigurationHandler::ClearPropertiesSuccessCallback(
     const std::string& service_path,
     const std::vector<std::string>& names,
     const base::Closure& callback,
     const base::ListValue& result) {
   const std::string kClearPropertiesFailedError("Error.ClearPropertiesFailed");
-  DCHECK(names.size() == result.GetSize())
-      << "Incorrect result size from ClearProperties.";
+  if (names.size() != result.GetSize())
+    NET_LOG_ERROR("ClearProperties Result size != input", service_path);
 
   for (size_t i = 0; i < result.GetSize(); ++i) {
     bool success = false;
@@ -485,18 +598,31 @@ void NetworkConfigurationHandler::ClearPropertiesErrorCallback(
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
   network_handler::ShillErrorCallbackFunction(
-      "Config.ClearProperties Failed",
-      service_path, error_callback,
+      "Config.ClearProperties Failed", service_path, error_callback,
       dbus_error_name, dbus_error_message);
   // Some properties may have changed so request an update regardless.
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
+void NetworkConfigurationHandler::RequestRefreshIPConfigs(
+    const std::string& service_path) {
+  if (!network_device_handler_)
+    return;
+  const NetworkState* network_state =
+      network_state_handler_->GetNetworkState(service_path);
+  if (!network_state || network_state->device_path().empty())
+    return;
+  network_device_handler_->RequestRefreshIPConfigs(
+      network_state->device_path(), base::Bind(&base::DoNothing),
+      network_handler::ErrorCallback());
+}
+
 // static
 NetworkConfigurationHandler* NetworkConfigurationHandler::InitializeForTest(
-    NetworkStateHandler* network_state_handler) {
+    NetworkStateHandler* network_state_handler,
+    NetworkDeviceHandler* network_device_handler) {
   NetworkConfigurationHandler* handler = new NetworkConfigurationHandler();
-  handler->Init(network_state_handler);
+  handler->Init(network_state_handler, network_device_handler);
   return handler;
 }