diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 1b9f720210..c0ecc1a2b0 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -52,6 +52,8 @@ import com.android.net.module.util.InterfaceParams; import com.android.server.connectivity.ConnectivityResources; import java.io.FileDescriptor; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -126,6 +128,17 @@ public class EthernetNetworkFactory { mContext.getSystemService(ConnectivityManager.class).registerNetworkProvider(mProvider); } + /** Returns an unordered(!) list of EthernetPort objects tracked by this factory. */ + public List getEthernetPorts() { + // Note that while mTrackingInterfaces is a ConcurrentHashMap, it is only ever modified on + // the handler thread. + final List ports = new ArrayList<>(mTrackingInterfaces.size()); + for (NetworkInterfaceState iface : mTrackingInterfaces.values()) { + ports.add(iface.getPort()); + } + return ports; + } + /** * Returns an array of available interface names. The array is sorted: unrestricted interfaces * goes first, then sorted by name. @@ -144,8 +157,8 @@ public class EthernetNetworkFactory { .toArray(String[]::new); } - @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - protected void addInterface(EthernetPort port, IpConfiguration ipConfig, + /** Add an interface to the factory. */ + public void addInterface(EthernetPort port, IpConfiguration ipConfig, NetworkCapabilities capabilities) { final String ifaceName = port.getInterfaceName(); final String hwAddress = port.getMacAddress().toString(); @@ -205,9 +218,9 @@ public class EthernetNetworkFactory { return; } - @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - protected boolean removeInterface(String interfaceName) { - NetworkInterfaceState iface = mTrackingInterfaces.remove(interfaceName); + /** Removes the interface from the factory and returns whether the interface was tracked */ + public boolean removeInterface(EthernetPort port) { + NetworkInterfaceState iface = mTrackingInterfaces.remove(port.getInterfaceName()); if (iface != null) { iface.unregisterNetworkOfferAndStop(); return true; @@ -215,13 +228,13 @@ public class EthernetNetworkFactory { // TODO(b/236892130): if an interface is currently in server mode, it may not be properly // removed. // TODO: when false is returned, do not send a STATE_ABSENT callback. - Log.w(TAG, interfaceName + " is not tracked and cannot be removed"); + Log.w(TAG, "removeInterface() failed because port is not tracked " + port); return false; } /** Returns true if state has been modified */ - @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up) { + public boolean updateInterfaceLinkState(EthernetPort port, boolean up) { + final String ifaceName = port.getInterfaceName(); if (!hasInterface(ifaceName)) { return false; } diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 11912eaa0b..b772d6f8ec 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -233,12 +233,12 @@ public class EthernetTracker { maybeTrackInterface(port); } Log.i(TAG, "interfaceLinkStateChanged: " + port + ", up: " + linkUp); - updateInterfaceState(ifname, linkUp); + updateInterfaceState(port, linkUp); } private void onDelLink(EthernetPort port) { Log.i(TAG, "onInterfaceRemoved: " + port); - stopTrackingInterface(port.getInterfaceName()); + stopTrackingInterface(port); } private void processRtNetlinkLinkMessage(RtNetlinkLinkMessage msg) { @@ -457,12 +457,11 @@ public class EthernetTracker { return mFactory.hasInterface(iface); } - private List getAllInterfaces() { - final ArrayList interfaces = new ArrayList<>( - List.of(mFactory.getInterfacesSorted(/* includeRestricted */ true))); - + /** Returns an unordered(!) list of tracked EthernetPort objects. */ + private List getAllInterfaces() { + final List interfaces = new ArrayList<>(mFactory.getEthernetPorts()); if (mTetheringInterfaceMode == INTERFACE_MODE_SERVER && mTetheringInterface != null) { - interfaces.add(mTetheringInterface.getInterfaceName()); + interfaces.add(mTetheringInterface); } return interfaces; } @@ -534,9 +533,10 @@ public class EthernetTracker { } else { removeTestData(); // remove all test interfaces - for (String iface : getAllInterfaces()) { + for (EthernetPort port : getAllInterfaces()) { + final String iface = port.getInterfaceName(); if (shouldTrackInterface(iface)) continue; - stopTrackingInterface(iface); + stopTrackingInterface(port); } } }); @@ -610,7 +610,7 @@ public class EthernetTracker { Log.d(TAG, "Setting tethering interface mode to " + mode); mTetheringInterfaceMode = mode; if (mTetheringInterface != null) { - removeInterface(mTetheringInterface.getInterfaceName()); + removeInterface(mTetheringInterface); addInterface(mTetheringInterface); // when this broadcast is sent, any calls to notifyTetheredInterfaceAvailable or // notifyTetheredInterfaceUnavailable have already happened @@ -649,13 +649,14 @@ public class EthernetTracker { return INTERFACE_MODE_CLIENT; } - private void removeInterface(String iface) { - mFactory.removeInterface(iface); - maybeUpdateServerModeInterfaceState(iface, false); + private void removeInterface(EthernetPort port) { + mFactory.removeInterface(port); + maybeUpdateServerModeInterfaceState(port.getInterfaceName(), false); } - private void stopTrackingInterface(String iface) { - removeInterface(iface); + private void stopTrackingInterface(EthernetPort port) { + removeInterface(port); + final String iface = port.getInterfaceName(); if (mTetheringInterface != null && iface.equals(mTetheringInterface.getInterfaceName())) { mTetheringInterface = null; mTetheringInterfaceHwAddr = null; @@ -712,7 +713,7 @@ public class EthernetTracker { // no need to send an interface state change as this is not a true "state change". The // callers (maybeTrackInterface() and setTetheringInterfaceMode()) already broadcast the // state change. - mFactory.updateInterfaceLinkState(iface, true); + mFactory.updateInterfaceLinkState(port, true); } } @@ -735,7 +736,8 @@ public class EthernetTracker { cb.onResult(iface); } - private void updateInterfaceState(String iface, boolean up) { + private void updateInterfaceState(EthernetPort port, boolean up) { + final String iface = port.getInterfaceName(); final int mode = getInterfaceMode(iface); if (mode == INTERFACE_MODE_SERVER) { // TODO: support tracking link state for interfaces in server mode. @@ -743,7 +745,7 @@ public class EthernetTracker { } // If updateInterfaceLinkState returns false, the interface is already in the correct state. - if (mFactory.updateInterfaceLinkState(iface, up)) { + if (mFactory.updateInterfaceLinkState(port, up)) { broadcastInterfaceStateChange(iface); } } @@ -864,8 +866,8 @@ public class EthernetTracker { if (mIsEthernetEnabled == enabled) return; mIsEthernetEnabled = enabled; - for (String iface : getAllInterfaces()) { - setInterfaceUpState(iface, enabled); + for (EthernetPort port : getAllInterfaces()) { + setInterfaceUpState(port.getInterfaceName(), enabled); } broadcastEthernetStateChange(mIsEthernetEnabled); }); diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 843e194a9d..74b02367c0 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -234,7 +234,7 @@ public class EthernetNetworkFactoryTest { final IpConfiguration ipConfig = createDefaultIpConfig(); mNetFactory.addInterface(port, ipConfig, createInterfaceCapsBuilder(transportType).build()); - assertTrue(mNetFactory.updateInterfaceLinkState(port.getInterfaceName(), true)); + assertTrue(mNetFactory.updateInterfaceLinkState(port, true)); ArgumentCaptor captor = ArgumentCaptor.forClass( NetworkOfferCallback.class); @@ -260,7 +260,7 @@ public class EthernetNetworkFactoryTest { final int expectedLegacyType) throws Exception { createAndVerifyProvisionedInterface(TEST_IFACE, transportType, expectedLegacyType); - mNetFactory.removeInterface(TEST_IFACE.getInterfaceName()); + mNetFactory.removeInterface(TEST_IFACE); } private void createAndVerifyProvisionedInterface( @@ -287,8 +287,7 @@ public class EthernetNetworkFactoryTest { createInterfaceUndergoingProvisioning(TEST_IFACE); // verify that the IpClient gets shut down when interface state changes to down. - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE.getInterfaceName(), false /* up */); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */); assertTrue(ret); verify(mIpClient).shutdown(); @@ -299,8 +298,7 @@ public class EthernetNetworkFactoryTest { initEthernetNetworkFactory(); // if interface was never added, link state cannot be updated. - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE.getInterfaceName(), true /* up */); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */); assertFalse(ret); verifyNoStopOrStart(); @@ -410,7 +408,7 @@ public class EthernetNetworkFactoryTest { private IpClientCallbacks getStaleIpClientCallbacks() throws Exception { createAndVerifyProvisionedInterface(TEST_IFACE); final IpClientCallbacks staleIpClientCallbacks = mIpClientCallbacks; - mNetFactory.removeInterface(TEST_IFACE.getInterfaceName()); + mNetFactory.removeInterface(TEST_IFACE); verifyStop(); assertNotSame(mIpClientCallbacks, staleIpClientCallbacks); return staleIpClientCallbacks; @@ -543,7 +541,7 @@ public class EthernetNetworkFactoryTest { public void testOnNetworkNeededOnStaleNetworkOffer() throws Exception { initEthernetNetworkFactory(); createAndVerifyProvisionedInterface(TEST_IFACE); - mNetFactory.updateInterfaceLinkState(TEST_IFACE.getInterfaceName(), false); + mNetFactory.updateInterfaceLinkState(TEST_IFACE, false); verify(mNetworkProvider).unregisterNetworkOffer(mNetworkOfferCallback); // It is possible that even after a network offer is unregistered, CS still sends it // onNetworkNeeded() callbacks. diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index 5013d78c31..7b6985e522 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -24,6 +24,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -77,7 +78,7 @@ public class EthernetTrackerTest { public void setUp() throws RemoteException { MockitoAnnotations.initMocks(this); initMockResources(); - doReturn(false).when(mFactory).updateInterfaceLinkState(anyString(), anyBoolean()); + doReturn(false).when(mFactory).updateInterfaceLinkState(any(), anyBoolean()); doReturn(new String[0]).when(mNetd).interfaceGetList(); doReturn(new String[0]).when(mFactory).getInterfacesSorted(anyBoolean()); mHandlerThread = new HandlerThread(THREAD_NAME);