chromeos: clear not supported devices on new MAC address source

Clear not supported devices set on new Ethernet MAC address source.

BUG=b:143695438
TEST=unittests
TEST=manual verify on arcada board that after changing setting
     non-supported 'builtin_adapter_mac', it's still possible to set
     another supported value and MAC will change.

Change-Id: I8bd5ef6c4833ffbc789b1c052724c5072523c4ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893886
Commit-Queue: Oleh Lamzin <lamzin@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713001}

chromeos/dbus/shill/fake_shill_device_client.cc

@@ -414,7 +414,7 @@ void FakeShillDeviceClient::SetUsbEthernetMacAddressSource(
           device_path.value());
   if (error_name_iter !=
           set_usb_ethernet_mac_address_source_error_names_.end() &&
-      !error_name_iter->first.empty()) {
+      !error_name_iter->second.empty()) {
     PostError(error_name_iter->second, error_callback);
     return;
   }

chromeos/network/network_device_handler_impl.cc

@@ -334,8 +334,13 @@ void NetworkDeviceHandlerImpl::SetMACAddressRandomizationEnabled(
 
 void NetworkDeviceHandlerImpl::SetUsbEthernetMacAddressSource(
     const std::string& source) {
+  if (source == usb_ethernet_mac_address_source_) {
+    return;
+  }
+
   usb_ethernet_mac_address_source_ = source;
   usb_ethernet_mac_address_source_needs_update_ = true;
+  mac_address_change_not_supported_.clear();
   ApplyUsbEthernetMacAddressSourceToShill();
 }
 
@@ -569,18 +574,20 @@ void NetworkDeviceHandlerImpl::ApplyUsbEthernetMacAddressSourceToShill() {
           weak_ptr_factory_.GetWeakPtr(),
           primary_enabled_usb_ethernet_device_path_,
           primary_enabled_usb_ethernet_device_state->mac_address(),
-          network_handler::ErrorCallback()));
+          usb_ethernet_mac_address_source_, network_handler::ErrorCallback()));
 }
 
 void NetworkDeviceHandlerImpl::OnSetUsbEthernetMacAddressSourceError(
     const std::string& device_path,
     const std::string& device_mac_address,
+    const std::string& mac_address_source,
     const network_handler::ErrorCallback& error_callback,
     const std::string& shill_error_name,
     const std::string& shill_error_message) {
   HandleShillCallFailure(device_path, error_callback, shill_error_name,
                          shill_error_message);
-  if (shill_error_name == NetworkDeviceHandler::kErrorNotSupported) {
+  if (shill_error_name == NetworkDeviceHandler::kErrorNotSupported &&
+      mac_address_source == usb_ethernet_mac_address_source_) {
     mac_address_change_not_supported_.insert(device_mac_address);
     ApplyUsbEthernetMacAddressSourceToShill();
   }

chromeos/network/network_device_handler_impl.h

@@ -152,9 +152,14 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkDeviceHandlerImpl
   // specified yet.
   void ApplyUsbEthernetMacAddressSourceToShill();
 
+  // Callback to be called on MAC address source change request failure.
+  // The request was called on device with |device_path| path and
+  // |device_mac_address| MAC address to change MAC address source to the new
+  // |mac_address_source| value.
   void OnSetUsbEthernetMacAddressSourceError(
       const std::string& device_path,
       const std::string& device_mac_address,
+      const std::string& mac_address_source,
       const network_handler::ErrorCallback& error_callback,
       const std::string& shill_error_name,
       const std::string& shill_error_message);
@@ -188,8 +193,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkDeviceHandlerImpl
   std::string usb_ethernet_mac_address_source_;
   bool usb_ethernet_mac_address_source_needs_update_ = false;
   std::string primary_enabled_usb_ethernet_device_path_;
-  // Set of device's MAC addresses that do not support MAC address change. Use
-  // MAC address as unique device identifier, because link name can change.
+  // Set of device's MAC addresses that do not support MAC address source change
+  // to |usb_ethernet_mac_address_source_|. Use MAC address as unique device
+  // identifier, because link name can change.
   std::unordered_set<std::string> mac_address_change_not_supported_;
 
   base::WeakPtrFactory<NetworkDeviceHandlerImpl> weak_ptr_factory_{this};

chromeos/network/network_device_handler_unittest.cc

@@ -293,6 +293,47 @@ TEST_F(NetworkDeviceHandlerTest,
       "usb_adapter_mac"));
 }
 
+TEST_F(NetworkDeviceHandlerTest, UsbEthernetMacAddressSourceNotSupported) {
+  ShillDeviceClient::TestInterface* device_test =
+      fake_device_client_->GetTestInterface();
+
+  constexpr char kSourceToOverride[] = "source_to_override";
+  constexpr char kUsbEthernetDevicePath[] = "usb_ethernet_device1";
+  device_test->AddDevice(kUsbEthernetDevicePath, shill::kTypeEthernet, "eth1");
+  device_test->SetDeviceProperty(
+      kUsbEthernetDevicePath, shill::kDeviceBusTypeProperty,
+      base::Value(shill::kDeviceBusTypeUsb), /*notify_changed=*/true);
+  device_test->SetDeviceProperty(kUsbEthernetDevicePath, shill::kLinkUpProperty,
+                                 base::Value(true),
+                                 /*notify_changed=*/true);
+  device_test->SetDeviceProperty(kUsbEthernetDevicePath,
+                                 shill::kUsbEthernetMacAddressSourceProperty,
+                                 base::Value(kSourceToOverride),
+                                 /*notify_changed=*/true);
+  device_test->SetUsbEthernetMacAddressSourceError(kUsbEthernetDevicePath,
+                                                   "not-supported");
+
+  network_device_handler_->SetUsbEthernetMacAddressSource("some_source1");
+  base::RunLoop().RunUntilIdle();
+
+  // Expect to do not change MAC address source property, because eth1 does not
+  // support |some_source1|.
+  ASSERT_NO_FATAL_FAILURE(ExpectDeviceProperty(
+      kUsbEthernetDevicePath, shill::kUsbEthernetMacAddressSourceProperty,
+      kSourceToOverride));
+
+  constexpr char kSource2[] = "some_source2";
+  device_test->SetUsbEthernetMacAddressSourceError(kUsbEthernetDevicePath, "");
+  network_device_handler_->SetUsbEthernetMacAddressSource(kSource2);
+  base::RunLoop().RunUntilIdle();
+
+  // Expect to change MAC address source property, because eth1 supports
+  // |some_source2|.
+  ASSERT_NO_FATAL_FAILURE(ExpectDeviceProperty(
+      kUsbEthernetDevicePath, shill::kUsbEthernetMacAddressSourceProperty,
+      kSource2));
+}
+
 TEST_F(NetworkDeviceHandlerTest, UsbEthernetMacAddressSource) {
   ShillDeviceClient::TestInterface* device_test =
       fake_device_client_->GetTestInterface();