ONC: Add IPAddressConfigType and NameServersConfigType

chromeos/network/onc/onc_translator_onc_to_shill.cc

Index: chromeos/network/onc/onc_translator_onc_to_shill.cc
diff --git a/chromeos/network/onc/onc_translator_onc_to_shill.cc b/chromeos/network/onc/onc_translator_onc_to_shill.cc
index 8e30af55446f6889b6c35922c488bf602039aae8..ed8812de24aa7cd7aa6dc750b4dbcc121d0780bb 100644
--- a/chromeos/network/onc/onc_translator_onc_to_shill.cc
+++ b/chromeos/network/onc/onc_translator_onc_to_shill.cc
@@ -257,6 +257,22 @@ void LocalTranslator::TranslateNetworkConfiguration() {
   if (type == ::onc::network_type::kVPN)
     CopyFieldFromONCToShill(::onc::network_config::kName, shill::kNameProperty);
 
+  // If both IPAddressConfigType and NameServersConfigType are set to DHCP,

[pneubeck (no reviews), 2015/01/06 10:57:46]

Just to clarify that we understand what this translation code will do:

If in a partial update either IPAddressConfigType or NameServersConfigType is
_not_ set, then we will not clear the StaticIPConfig property and thus not
change to the respective DHCP value.

Similarly, if in a partial update the ONC StaticIPConfig dict contains _only_
the NameServers or only the IPAddress+RoutingPrefix+Gateway, then exactly these
values will be translated and the others will be reset to DHCP by Shill.

[stevenjb, 2015/01/06 17:12:30]

If *neither* Type is set, then StaticIPConfig will be unchanged.

If *both* Types are set to DHCP, then the property will be cleared.

If *either* Type is set to Static then we rely on the validator to ensure that a
StaticIPConfig object exists (with a TODO to ensure that the correct properties
are set).

I suppose to be robust we should specify an empty dictionary if either is set to
DHCP. I will add that (and try to clarify the comments).

I think that I agree that we should separate the NameServers property in the ONC
spec, but I'd rather see that done in a follow-up at this point.

+  // provide an empty StaticIPConfig property so that any existing properties
+  // will be cleared. Note: if either is set to Static, then a StaticIPConfig
+  // object will be provided which will specify any static properties (and any
+  // unspecified properties will be cleared).
+  std::string ip_address_config_type, name_servers_config_type;
+  onc_object_->GetStringWithoutPathExpansion(
+      ::onc::network_config::kIPAddressConfigType, &ip_address_config_type);
+  onc_object_->GetStringWithoutPathExpansion(
+      ::onc::network_config::kNameServersConfigType, &name_servers_config_type);
+  if ((ip_address_config_type == ::onc::network_config::kIPConfigTypeDHCP) &&
+      (name_servers_config_type == ::onc::network_config::kIPConfigTypeDHCP)) {
+    shill_dictionary_->SetWithoutPathExpansion(shill::kStaticIPConfigProperty,
+                                               base::Value::CreateNullValue());

[pneubeck (no reviews), 2015/01/04 11:18:01]

can't we just set an empty dictionary? i thought that was what Paul said, that
it would clear the property?

[stevenjb, 2015/01/05 21:16:19]

Setting this to an empty Dictionary means changing a lot of test expectations
(adding a StaticIPConfig entry with a {} value). I think this is a tiny bit more
clear, but it is extra code, so I could be persuaded to always set
StaticIPConfig to an empty dictionary and change the expectations.

[pneubeck (no reviews), 2015/01/06 10:57:46]

Shouldn't it affect exactly the test cases that you modified in this CL?
I would just hope that it could also lead to a simplification of
https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
.

[stevenjb, 2015/01/06 17:12:30]

I think I misunderstood. The else clause I added should make this a little more
clear I hope.

+  }
+
   CopyFieldsAccordingToSignature();
 }