From 92077d1530112f7af0330ca70148c96e36047b08 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:02:07 -0700 Subject: [PATCH 1/7] Add getEthernetPorts and use it in getAllInterfaces This CL adds getEthernetPorts() to EthernetNetworkFactory which returns an unsorted list of EthernetPorts tracked by the factory (including restricted interfaces). This method is then used inside getAllInterfaces(), which in turn is called by setIncludeTestInterfaces() and setEthernetEnabled(). setIncludeTestInterfaces() is not used in production code, so the interface order should not matter. However, with this change setEthernetEnabled() will update the administrative state of the network interfaces in unspecified order. It is hard to conceive that this could break anything, given that when tracking multiple interfaces, link state changes (which trigger the same InterfaceStateCallback) can already naturally occur in any order. Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: Iaf6810ec2fd784c16a9a09662282e81bc2070e37 --- .../server/ethernet/EthernetNetworkFactory.java | 13 +++++++++++++ .../android/server/ethernet/EthernetTracker.java | 16 ++++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 1b9f720210..14193dfd56 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. diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 11912eaa0b..1460b23512 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -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,7 +533,8 @@ 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); } @@ -864,8 +864,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); }); From 88fdbf3093add71eca307bc75eea7a2c54de04ec Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:11:17 -0700 Subject: [PATCH 2/7] Pass EthernetPort to stopTrackingInterface Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: Ia96d35041cbfedb3fd4f67d43bb7238bfdd86d40 --- .../src/com/android/server/ethernet/EthernetTracker.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 1460b23512..ab4396584a 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -238,7 +238,7 @@ public class EthernetTracker { private void onDelLink(EthernetPort port) { Log.i(TAG, "onInterfaceRemoved: " + port); - stopTrackingInterface(port.getInterfaceName()); + stopTrackingInterface(port); } private void processRtNetlinkLinkMessage(RtNetlinkLinkMessage msg) { @@ -536,7 +536,7 @@ public class EthernetTracker { for (EthernetPort port : getAllInterfaces()) { final String iface = port.getInterfaceName(); if (shouldTrackInterface(iface)) continue; - stopTrackingInterface(iface); + stopTrackingInterface(port); } } }); @@ -654,7 +654,8 @@ public class EthernetTracker { maybeUpdateServerModeInterfaceState(iface, false); } - private void stopTrackingInterface(String iface) { + private void stopTrackingInterface(EthernetPort port) { + final String iface = port.getInterfaceName(); removeInterface(iface); if (mTetheringInterface != null && iface.equals(mTetheringInterface.getInterfaceName())) { mTetheringInterface = null; From d5159a9c5ffb57aef21e206b7a1df57b03ccb6a8 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:14:08 -0700 Subject: [PATCH 3/7] Pass EthernetPort to removeInterface Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I4fd6658e95d65c65dad95b0a28f6878e4f5119fd --- .../src/com/android/server/ethernet/EthernetTracker.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index ab4396584a..7d581f8697 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -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,14 +649,15 @@ public class EthernetTracker { return INTERFACE_MODE_CLIENT; } - private void removeInterface(String iface) { + private void removeInterface(EthernetPort port) { + final String iface = port.getInterfaceName(); mFactory.removeInterface(iface); maybeUpdateServerModeInterfaceState(iface, false); } private void stopTrackingInterface(EthernetPort port) { + removeInterface(port); final String iface = port.getInterfaceName(); - removeInterface(iface); if (mTetheringInterface != null && iface.equals(mTetheringInterface.getInterfaceName())) { mTetheringInterface = null; mTetheringInterfaceHwAddr = null; From 345876e1192eec418cd505e61620505b83944387 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:17:22 -0700 Subject: [PATCH 4/7] Pass EthernetPort to EthernetNetworkFactory#removeInterface Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: Iba1dfdba7f249013da0946bfe9738f110f95391e --- .../android/server/ethernet/EthernetNetworkFactory.java | 8 ++++---- .../src/com/android/server/ethernet/EthernetTracker.java | 5 ++--- .../server/ethernet/EthernetNetworkFactoryTest.java | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 14193dfd56..16652ca291 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -218,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; @@ -228,7 +228,7 @@ 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; } diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 7d581f8697..aeabc30780 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -650,9 +650,8 @@ public class EthernetTracker { } private void removeInterface(EthernetPort port) { - final String iface = port.getInterfaceName(); - mFactory.removeInterface(iface); - maybeUpdateServerModeInterfaceState(iface, false); + mFactory.removeInterface(port); + maybeUpdateServerModeInterfaceState(port.getInterfaceName(), false); } private void stopTrackingInterface(EthernetPort port) { diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 843e194a9d..292803183c 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -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( @@ -410,7 +410,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; From 8191dd61c3ab91e4c66a337d31029511668ef5d5 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:42:18 -0700 Subject: [PATCH 5/7] Remove spurious VisibleForTesting and change visibility to public The visibility modifiers in this file are all over the place. In general, I would like to get to private / public where possible so the code becomes more readable. Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I2fb00abfd903083e0320fe33eeff2d40302440c7 --- .../com/android/server/ethernet/EthernetNetworkFactory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 16652ca291..2c8fb895dc 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -157,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(); From ff317e73efb28c08305154d482503e4dd40d446e Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:47:19 -0700 Subject: [PATCH 6/7] Pass EthernetPort to updateInterfaceState Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: Id0c9cc4fd054f4e7096e0bf45ee3ffb5a9085698 --- .../src/com/android/server/ethernet/EthernetTracker.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index aeabc30780..419a09bfaf 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -233,7 +233,7 @@ public class EthernetTracker { maybeTrackInterface(port); } Log.i(TAG, "interfaceLinkStateChanged: " + port + ", up: " + linkUp); - updateInterfaceState(ifname, linkUp); + updateInterfaceState(port, linkUp); } private void onDelLink(EthernetPort port) { @@ -736,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. From db32c1bbdc548e79b04e658718350ff8bdf57e0f Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 5 Jun 2025 14:55:48 -0700 Subject: [PATCH 7/7] Pass EthernetPort to EthernetNetworkFactory#updateInterfaceLinkState Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I7e78be87824e29eab90074b1a2263e0cbd779021 --- .../server/ethernet/EthernetNetworkFactory.java | 4 ++-- .../com/android/server/ethernet/EthernetTracker.java | 4 ++-- .../server/ethernet/EthernetNetworkFactoryTest.java | 10 ++++------ .../android/server/ethernet/EthernetTrackerTest.java | 3 ++- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 2c8fb895dc..c0ecc1a2b0 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -233,8 +233,8 @@ public class EthernetNetworkFactory { } /** 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 419a09bfaf..b772d6f8ec 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -713,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); } } @@ -745,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); } } diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 292803183c..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); @@ -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(); @@ -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);