Use Name not SSID in CopyIdentifyingProperties

chromeos/network/shill_property_util.cc

 // Copyright 2013 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/shill_property_util.h"
 
 #include "base/i18n/icu_encoding_detection.h"
 #include "base/i18n/icu_string_conversions.h"
 #include "base/json/json_writer.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 162 matching lines @@
   base::DictionaryValue ui_data_dict;
   ui_data.FillDictionary(&ui_data_dict);
   std::string ui_data_blob;
   base::JSONWriter::Write(&ui_data_dict, &ui_data_blob);
   shill_dictionary->SetStringWithoutPathExpansion(flimflam::kUIDataProperty,
                                                   ui_data_blob);
 }
 
 bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
                                base::DictionaryValue* dest) {
-  bool success = true;
-
   // GUID is optional.
   CopyStringFromDictionary(service_properties, flimflam::kGuidProperty, dest);
 
   std::string type;
   service_properties.GetStringWithoutPathExpansion(flimflam::kTypeProperty,
                                                    &type);
-  success &= !type.empty();
+  if (type.empty()) {
+    NET_LOG_ERROR("CopyIdentifyingProperties", "Empty Type");
+    return false;
+  }
   dest->SetStringWithoutPathExpansion(flimflam::kTypeProperty, type);
   if (type == flimflam::kTypeWifi) {
-    success &= CopyStringFromDictionary(
-        service_properties, flimflam::kSecurityProperty, dest);
-    success &= CopyStringFromDictionary(
-        service_properties, flimflam::kSSIDProperty, dest);
-    success &= CopyStringFromDictionary(
-        service_properties, flimflam::kModeProperty, dest);
+    if (!CopyStringFromDictionary(
+            service_properties, flimflam::kNameProperty, dest)) {

[pneubeck (no reviews), 2013/09/13 07:15:10]

I think that this change makes it even worse:
From all previous discussions with Paul, a Wifi service is identified by
Security+SSID+Mode.
This is also what is documented here:
https://cs.corp.google.com/#chromeos_public/src/platform/shill/doc/manager-ap...
and implemented here:
https://cs.corp.google.com/#chromeos_public/src/platform/shill/wifi_provider....

The properties that we set are ok. But, it seems that we ran again into the
issue that Shill doesn't set kSSIDProperty reliably.
This problem should as well be fixed if we change everything to HexSSID.

A temporary fix could be to copy service_properties.kNameProperty to
dest.kSSIDProperty, if service_properties.kSSIDProperty is empty/unset.

+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Name");
+      return false;
+    }
+    if (!CopyStringFromDictionary(
+            service_properties, flimflam::kSecurityProperty, dest)) {
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Security");
+      return false;
+    }
+    if (!CopyStringFromDictionary(
+            service_properties, flimflam::kModeProperty, dest)) {
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Mode");
+      return false;
+    }
   } else if (type == flimflam::kTypeVPN) {
-    success &= CopyStringFromDictionary(
-        service_properties, flimflam::kNameProperty, dest);
+    if (!CopyStringFromDictionary(
+            service_properties, flimflam::kNameProperty, dest)) {
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Name");
+      return false;
+    }
     // VPN Provider values are read from the "Provider" dictionary, but written
     // with the keys "Provider.Type" and "Provider.Host".
     const base::DictionaryValue* provider_properties = NULL;
     if (!service_properties.GetDictionaryWithoutPathExpansion(
              flimflam::kProviderProperty, &provider_properties)) {
-      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing VPN provider dict");
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing VPN Provider dict");
       return false;
     }
     std::string vpn_provider_type;
     provider_properties->GetStringWithoutPathExpansion(flimflam::kTypeProperty,
                                                        &vpn_provider_type);
-    success &= !vpn_provider_type.empty();
+    if (vpn_provider_type.empty()) {
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Provider Type");
+      return false;
+    }
     dest->SetStringWithoutPathExpansion(flimflam::kProviderTypeProperty,
                                         vpn_provider_type);
 
     std::string vpn_provider_host;
     provider_properties->GetStringWithoutPathExpansion(flimflam::kHostProperty,
                                                        &vpn_provider_host);
-    success &= !vpn_provider_host.empty();
+    if (vpn_provider_host.empty()) {
+      NET_LOG_ERROR("CopyIdentifyingProperties", "Missing Provider Host");
+      return false;
+    }
     dest->SetStringWithoutPathExpansion(flimflam::kProviderHostProperty,
                                         vpn_provider_host);
   } else if (type == flimflam::kTypeEthernet ||
              type == shill::kTypeEthernetEap) {
     // Ethernet and EthernetEAP don't have any additional identifying
     // properties.
   } else {
     NOTREACHED() << "Unsupported network type " << type;
-    success = false;
+    NET_LOG_ERROR("CopyIdentifyingProperties", "Bad Type: " + type);
+    return false;
   }
-  if (!success)
-    NET_LOG_ERROR("CopyIdentifyingProperties", "Missing required properties");
-  return success;
+  return true;
 }
 
 }  // namespace shill_property_util
 
 namespace {
 
 const char kPatternDefault[] = "PatternDefault";
 const char kPatternEthernet[] = "PatternEthernet";
 const char kPatternWireless[] = "PatternWireless";
 const char kPatternMobile[] = "PatternMobile";
@@ skipping 120 matching lines @@
     if (!str.empty())
       str += "|";
     str += shill_type_to_flag[i].shill_network_type;
   }
   return str;
 }
 
 NetworkTypePattern::NetworkTypePattern(int pattern) : pattern_(pattern) {}
 
 }  // namespace chromeos