Add an ONC property for the third-party VPN provider extension ID

chromeos/network/network_state.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.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_type_pattern.h"
 #include "chromeos/network/network_util.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "chromeos/network/shill_property_util.h"
 #include "components/device_event_log/device_event_log.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
@@ skipping 142 matching lines @@
       // Warning: The DictionaryValue returned from
       // ReadDictionaryFromJson/JSONParser is an optimized derived class that
       // doesn't allow releasing ownership of nested values. A Swap in the wrong
       // order leads to memory access errors.
       proxy_config_.MergeDictionary(proxy_config_dict.get());
     } else {
       NET_LOG(ERROR) << "Failed to parse " << path() << "." << key;
     }
     return true;
   } else if (key == shill::kProviderProperty) {
+    third_party_vpn_provider_extension_id_.clear();

[pneubeck (no reviews), 2015/03/18 19:35:29]

I found the reason why I said we must be cautious with the clear()s here.
See the documentation of this function:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...

If you return false, the state _must not_ have changed.
Please move this clear into the
 if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
case

Then there's still one corner-case left if vpn_provider_type_ changed to 3rd
party and fails in line 187.

[bartfab (slow), 2015/03/18 20:02:14]

Fixed, including the corner case.

     const base::DictionaryValue* dict;
-    std::string provider_type;
     if (!value.GetAsDictionary(&dict) ||
         !dict->GetStringWithoutPathExpansion(shill::kTypeProperty,
-                                             &provider_type)) {
+                                             &vpn_provider_type_)) {
+      NET_LOG(ERROR) << "Failed to parse " << path() << "." << key;
       return false;
     }
 
-    if (provider_type != shill::kProviderThirdPartyVpn) {
-      // If the network uses the built-in OpenVPN and L2TP support, set the
-      // provider extension ID to an empty string.
-      vpn_provider_extension_id_.clear();
+    if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
       return true;
-    }
 
     // If the network uses a third-party VPN provider, copy over the provider's
     // extension ID, which is held in |shill::kHostProperty|.
-    return dict->GetStringWithoutPathExpansion(shill::kHostProperty,
-                                               &vpn_provider_extension_id_);
+    if (!dict->GetStringWithoutPathExpansion(
+            shill::kHostProperty, &third_party_vpn_provider_extension_id_)) {
+      NET_LOG(ERROR) << "Failed to parse " << path() << "." << key;
+      return false;
+    }
+
+    return true;
   }
   return false;
 }
 
 bool NetworkState::InitialPropertiesReceived(
     const base::DictionaryValue& properties) {
   NET_LOG(EVENT) << "InitialPropertiesReceived: " << path() << ": " << name()
                  << " State: " << connection_state_ << " Visible: " << visible_;
   if (!properties.HasKey(shill::kTypeProperty)) {
     NET_LOG(ERROR) << "NetworkState has no type: "
@@ skipping 27 matching lines @@
   dictionary->SetStringWithoutPathExpansion(shill::kProfileProperty,
                                             profile_path());
 
   if (visible()) {
     if (!error().empty())
       dictionary->SetStringWithoutPathExpansion(shill::kErrorProperty, error());
     dictionary->SetStringWithoutPathExpansion(shill::kStateProperty,
                                               connection_state());
   }
 
+  // VPN properties.
+  if (NetworkTypePattern::VPN().MatchesType(type())) {
+    scoped_ptr<base::DictionaryValue> provider_property(

[pneubeck (no reviews), 2015/03/18 19:35:29]

nit: drop a comment before this line, like:

"Shill is sending VPN provider properties in a nested dictionary (but expects to
receive a flat dictionary). |dictionary| must have the same structure."

[bartfab (slow), 2015/03/18 20:02:14]

Done.

+        new base::DictionaryValue);
+    provider_property->SetStringWithoutPathExpansion(shill::kTypeProperty,
+                                                     vpn_provider_type_);
+    if (vpn_provider_type_ == shill::kProviderThirdPartyVpn) {
+      provider_property->SetStringWithoutPathExpansion(
+          shill::kHostProperty, third_party_vpn_provider_extension_id_);
+    }
+    dictionary->SetWithoutPathExpansion(shill::kProviderProperty,
+                                        provider_property.release());
+  }
+
   // Wireless properties
   if (!NetworkTypePattern::Wireless().MatchesType(type()))
     return;
 
   if (visible()) {
     dictionary->SetBooleanWithoutPathExpansion(shill::kConnectableProperty,
                                                connectable());
     dictionary->SetIntegerWithoutPathExpansion(shill::kSignalStrengthProperty,
                                                signal_strength());
   }
@@ skipping 152 matching lines @@
   return IsCaptivePortalState(shill_properties, false /* log */);
 }
 
 // static
 bool NetworkState::ErrorIsValid(const std::string& error) {
   // Shill uses "Unknown" to indicate an unset or cleared error state.
   return !error.empty() && error != kErrorUnknown;
 }
 
 }  // namespace chromeos