Provide IPConfigs in networkingPrivate.GetProperties

chromeos/network/network_configuration_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_configuration_handler.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/device_state.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/shill_property_util.h"
 #include "dbus/object_path.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 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);
   return in_str;
 }
 
 void InvokeErrorCallback(const std::string& service_path,
                          const network_handler::ErrorCallback& error_callback,
                          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);
 }
 
-void GetPropertiesCallback(
-    const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback,
-    const std::string& service_path,
-    DBusMethodCallStatus call_status,
-    const base::DictionaryValue& properties) {
-  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::kDBusFailedError,
-                                      network_handler::kDBusFailedErrorMessage);
-    return;
-  }
-  if (callback.is_null())
-    return;
-
-  // Get the correct name from WifiHex if necessary.
-  scoped_ptr<base::DictionaryValue> properties_copy(properties.DeepCopy());
-  std::string name =
-      shill_property_util::GetNameFromProperties(service_path, properties);
-  if (!name.empty())
-    properties_copy->SetStringWithoutPathExpansion(shill::kNameProperty, name);
-  callback.Run(service_path, *properties_copy.get());
-}
-
 void SetNetworkProfileErrorCallback(
     const std::string& service_path,
     const std::string& profile_path,
     const network_handler::ErrorCallback& error_callback,
     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);
@@ skipping 114 matching lines @@
   std::map<std::string, std::string> profile_delete_entries_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileEntryDeleter);
 };
 
 // NetworkConfigurationHandler
 
 void NetworkConfigurationHandler::GetProperties(
     const std::string& service_path,
     const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback) const {
+    const network_handler::ErrorCallback& error_callback) {
   DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
       dbus::ObjectPath(service_path),
-      base::Bind(&GetPropertiesCallback,
-                 callback, error_callback, service_path));
+      base::Bind(&NetworkConfigurationHandler::GetPropertiesCallback,
+                 AsWeakPtr(), callback, error_callback, service_path));
 }
 
 void NetworkConfigurationHandler::SetProperties(
     const std::string& service_path,
     const base::DictionaryValue& properties,
     const base::Closure& callback,
     const network_handler::ErrorCallback& error_callback) {
   if (properties.empty()) {
     if (!callback.is_null())
       callback.Run();
@@ skipping 136 matching lines @@
 
 void NetworkConfigurationHandler::ProfileEntryDeleterCompleted(
     const std::string& service_path) {
   std::map<std::string, ProfileEntryDeleter*>::iterator iter =
       profile_entry_deleters_.find(service_path);
   DCHECK(iter != profile_entry_deleters_.end());
   delete iter->second;
   profile_entry_deleters_.erase(iter);
 }
 
+void NetworkConfigurationHandler::GetPropertiesCallback(
+    const network_handler::DictionaryResultCallback& callback,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& service_path,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  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::kDBusFailedError,
+                                      network_handler::kDBusFailedErrorMessage);
+    return;
+  }
+  if (callback.is_null())
+    return;
+
+  // Get the correct name from WifiHex if necessary.
+  scoped_ptr<base::DictionaryValue> properties_copy(properties.DeepCopy());
+  std::string name =
+      shill_property_util::GetNameFromProperties(service_path, properties);
+  if (!name.empty())
+    properties_copy->SetStringWithoutPathExpansion(shill::kNameProperty, name);
+
+  // Get the IPConfig properties from the device and store them in "IPConfigs"
+  // (plural) in the properties dictionary.
+  std::string device;
+  properties_copy->GetStringWithoutPathExpansion(
+      shill::kDeviceProperty, &device);
+  const DeviceState* device_state =
+      network_state_handler_->GetDeviceState(device);

[pneubeck (no reviews), 2014/05/05 21:09:28]

Just some thought, not saying that it's really a problem:

If you get the ipconfigs from StateHandler's cache, might this lead to timing
issues?
Namely, if the properties received here are more up-to-date than the device
state, e.g. before and after connecting. This might lead to
ConnectionState=connected but no available ipconfigs.

[stevenjb, 2014/05/06 01:15:11]

So, I thought about this some, and realized a few things:
1) We shouldn't do this here, we should do it in ManagedNCH.
2) We should only do this when State == Connected
3) Because Shill handles all requests in-order, the only edge case would occur
if the service changed state to Connected in the same frame that GetProperties
was requested. Otherwise Chrome would have already received the IPConfigs
changed signal and requested the IPConfigs, which would have been processed by
the time we got to this code. However, there will always be an edge case because
State may have changed to !Connected by the time we get here, in which case a
fresh request would produce the wrong result. Thus, I think we should stick with
the edge case that is less work :)

[pneubeck (no reviews), 2014/05/06 07:31:17]

sg

+  if (device_state) {
+    // Convert IPConfig dictionary to ListValue
+    base::ListValue* ip_configs = new base::ListValue;
+    for (base::DictionaryValue::Iterator iter(device_state->ip_configs());
+         !iter.IsAtEnd(); iter.Advance()) {
+      ip_configs->Append(iter.value().DeepCopy());
+    }
+    properties_copy->SetWithoutPathExpansion(
+        shill::kIPConfigsProperty, ip_configs);

[pneubeck (no reviews), 2014/05/05 21:09:28]

do we want to document that Shill doesn't use this property for Services but
only for Devices?

[stevenjb, 2014/05/06 01:15:11]

Done. (elaborated comment above)

+  } else {
+    NET_LOG_ERROR("GetPropertiesCallback: no device: " + device, service_path);
+  }
+  callback.Run(service_path, *properties_copy.get());

[pneubeck (no reviews), 2014/05/05 21:09:28]

.get() shouldn't be necessary

[stevenjb, 2014/05/06 01:15:11]

Done.

+}
+
 void NetworkConfigurationHandler::SetPropertiesSuccessCallback(
     const std::string& service_path,
     const base::Closure& callback) {
   if (!callback.is_null())
     callback.Run();
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
 void NetworkConfigurationHandler::SetPropertiesErrorCallback(
     const std::string& service_path,
@@ skipping 47 matching lines @@
 
 // static
 NetworkConfigurationHandler* NetworkConfigurationHandler::InitializeForTest(
     NetworkStateHandler* network_state_handler) {
   NetworkConfigurationHandler* handler = new NetworkConfigurationHandler();
   handler->Init(network_state_handler);
   return handler;
 }
 
 }  // namespace chromeos