From 7fae9fc51ef929ca1d9c13bb1af84b8f532ca89a Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 21 Mar 2025 09:39:38 -0700 Subject: [PATCH 1/5] Prevent mixing mutable and immutable lists As per errorprone feedback. Test: TH Change-Id: I4da5673551e4c8c4b3fea0aa65d34649126aaff8 --- service-t/src/com/android/server/ethernet/EthernetTracker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index def701b0b7..8f000a3795 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -1037,7 +1037,7 @@ public class EthernetTracker { } capabilities.add(capability); } - return capabilities; + return Collections.unmodifiableList(capabilities); } private static int parseTransportType(String transportString) { From 515af4b1b05cb6683fda7f390ae743d2dc07a8bf Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 21 Mar 2025 10:04:35 -0700 Subject: [PATCH 2/5] Do not support configuring test interfaces in config.xml Test interfaces are currently suffixed with an ever increasing number, so naming is not predictable. Also, given that the config.xml must be present at build time makes it not particularly useful for testing. Test: TH Change-Id: I48d0b0f98fdd90fa5ef50cc62b6b7bf08a55af1f --- service-t/src/com/android/server/ethernet/EthernetTracker.java | 3 +-- 1 file changed, 1 insertion(+), 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 8f000a3795..4d3a653d07 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -771,8 +771,7 @@ public class EthernetTracker { // the default network. To avoid breaking existing device configurations, this // change is gated by the SDK level. if (SdkLevel.isAtLeastB() && config.mCaps.isEmpty()) { - boolean isTestIface = config.mIface.matches(TEST_IFACE_REGEXP); - nc = createDefaultNetworkCapabilities(isTestIface, config.mTransport); + nc = createDefaultNetworkCapabilities(false /* isTestIface */, config.mTransport); } else { nc = createNetworkCapabilities(config.mCaps, config.mTransport).build(); } From f0e0e2819d0ee8684f47cd85ee54b3c70050790d Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 21 Mar 2025 10:43:48 -0700 Subject: [PATCH 3/5] Rename EthernetTrackerConfig to EthernetConfigParser Because that's what it really is. Test: TH Change-Id: I3806750a87042b4ae8e816f00388d6393346d0a8 --- .../server/ethernet/EthernetTracker.java | 8 ++++---- .../server/ethernet/EthernetTrackerTest.java | 20 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 4d3a653d07..f19f443cfa 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -758,7 +758,7 @@ public class EthernetTracker { * ;[Network Capabilities];[IP config];[Override Transport]} */ private void parseEthernetConfig(String configString) { - final EthernetTrackerConfig config = new EthernetTrackerConfig(configString); + final EthernetConfigParser config = new EthernetConfigParser(configString); NetworkCapabilities nc; // Starting with Android B (API level 36), we provide default NetworkCapabilities // for Ethernet interfaces when no explicit capabilities are specified in the @@ -1011,7 +1011,7 @@ public class EthernetTracker { } @VisibleForTesting - static class EthernetTrackerConfig { + static class EthernetConfigParser { final String mIface; final List mCaps; final String mIpConfig; @@ -1067,8 +1067,8 @@ public class EthernetTracker { } } - EthernetTrackerConfig(String configString) { - Objects.requireNonNull(configString, "EthernetTrackerConfig requires non-null config"); + EthernetConfigParser(String configString) { + Objects.requireNonNull(configString, "EthernetConfigParser requires non-null config"); final String[] tokens = configString.split(";", /* limit of tokens */ 4); mIface = tokens[0]; mCaps = tokens.length > 1 ? parseCapabilities(tokens[1]) : Collections.emptyList(); diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index 6ce7dbc26a..03fb505c10 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -46,7 +46,7 @@ import android.os.RemoteException; import androidx.test.filters.SmallTest; -import com.android.server.ethernet.EthernetTracker.EthernetTrackerConfig; +import com.android.server.ethernet.EthernetTracker.EthernetConfigParser; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRunner; import com.android.testutils.HandlerUtils; @@ -291,20 +291,20 @@ public class EthernetTrackerTest { final String ipConfig = ""; final String configString = String.join(";", TEST_IFACE, configCapabilities, ipConfig, configTransports); - final EthernetTrackerConfig config = new EthernetTrackerConfig(configString); + final EthernetConfigParser config = new EthernetConfigParser(configString); assertEquals( expectedNetworkCapabilities, EthernetTracker.createNetworkCapabilities(config.mCaps, config.mTransport).build()); } @Test - public void testCreateEthernetTrackerConfigReturnsCorrectValue() { + public void testCreateEthernetConfigParserReturnsCorrectValue() { final String capabilities = "2,4,6,8"; final String ipConfig = "3"; final String transport = "1"; final String configString = String.join(";", TEST_IFACE, capabilities, ipConfig, transport); - final EthernetTrackerConfig config = new EthernetTrackerConfig(configString); + final EthernetConfigParser config = new EthernetConfigParser(configString); assertEquals(TEST_IFACE, config.mIface); assertThat(config.mCaps).containsExactly(2, 4, 6, 8); @@ -313,30 +313,30 @@ public class EthernetTrackerTest { } @Test - public void testCreateEthernetTrackerConfig_withInvalidTransport() { + public void testCreateEthernetConfigParser_withInvalidTransport() { final String capabilities = "2"; final String ipConfig = "3"; final String transport = "100"; // Invalid transport type final String configString = String.join(";", TEST_IFACE, capabilities, ipConfig, transport); - final EthernetTrackerConfig config = new EthernetTrackerConfig(configString); + final EthernetConfigParser config = new EthernetConfigParser(configString); assertEquals(NetworkCapabilities.TRANSPORT_ETHERNET, config.mTransport); } @Test - public void testCreateEthernetTrackerConfig_withDisallowedTransport() { + public void testCreateEthernetConfigParser_withDisallowedTransport() { final String capabilities = "2"; final String ipConfig = "3"; final String transport = "4"; // TRANSPORT_VPN is not allowed final String configString = String.join(";", TEST_IFACE, capabilities, ipConfig, transport); - final EthernetTrackerConfig config = new EthernetTrackerConfig(configString); + final EthernetConfigParser config = new EthernetConfigParser(configString); assertEquals(NetworkCapabilities.TRANSPORT_ETHERNET, config.mTransport); } @Test - public void testCreateEthernetTrackerConfigThrowsNpeWithNullInput() { - assertThrows(NullPointerException.class, () -> new EthernetTrackerConfig(null)); + public void testCreateEthernetConfigParserThrowsNpeWithNullInput() { + assertThrows(NullPointerException.class, () -> new EthernetConfigParser(null)); } @Test From 269bb274826160b248f50b195acd9fb0e99a525c Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 21 Mar 2025 11:13:16 -0700 Subject: [PATCH 4/5] Pass nullable values to parse functions So the default value does not have to be repeated as null is handled the same as "". Test: TH Change-Id: I1354ec5305af2ab74109f313318576dd56d96cef --- .../src/com/android/server/ethernet/EthernetTracker.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index f19f443cfa..53a899ea5e 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -1017,7 +1017,7 @@ public class EthernetTracker { final String mIpConfig; final int mTransport; - private static List parseCapabilities(String capabilitiesString) { + private static List parseCapabilities(@Nullable String capabilitiesString) { if (TextUtils.isEmpty(capabilitiesString)) { return Collections.emptyList(); } @@ -1039,7 +1039,7 @@ public class EthernetTracker { return Collections.unmodifiableList(capabilities); } - private static int parseTransportType(String transportString) { + private static int parseTransportType(@Nullable String transportString) { if (TextUtils.isEmpty(transportString)) { return TRANSPORT_ETHERNET; } @@ -1071,9 +1071,9 @@ public class EthernetTracker { Objects.requireNonNull(configString, "EthernetConfigParser requires non-null config"); final String[] tokens = configString.split(";", /* limit of tokens */ 4); mIface = tokens[0]; - mCaps = tokens.length > 1 ? parseCapabilities(tokens[1]) : Collections.emptyList(); + mCaps = parseCapabilities(tokens.length > 1 ? tokens[1] : null); mIpConfig = tokens.length > 2 && !TextUtils.isEmpty(tokens[2]) ? tokens[2] : null; - mTransport = tokens.length > 3 ? parseTransportType(tokens[3]) : TRANSPORT_ETHERNET; + mTransport = parseTransportType(tokens.length > 3 ? tokens[3] : null); } } } From afa7ee8bfc9fddd2bcce8c33c6b6ac2d76aae98e Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 21 Mar 2025 13:05:49 -0700 Subject: [PATCH 5/5] Move isAtLeastB to dependencies So it is testable. Test: TH Change-Id: Ie63a3583c2e9d327cf47effc2c9352e2da03c7c0 --- .../src/com/android/server/ethernet/EthernetTracker.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 53a899ea5e..1c453cb4e8 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -170,6 +170,10 @@ public class EthernetTracker { return resources.get().getStringArray( com.android.connectivity.resources.R.array.config_ethernet_interfaces); } + + public boolean isAtLeastB() { + return SdkLevel.isAtLeastB(); + } } private class EthernetNetlinkMonitor extends NetlinkMonitor { @@ -770,7 +774,7 @@ public class EthernetTracker { // capabilities) to prevent certain Ethernet interfaces from becoming // the default network. To avoid breaking existing device configurations, this // change is gated by the SDK level. - if (SdkLevel.isAtLeastB() && config.mCaps.isEmpty()) { + if (mDeps.isAtLeastB() && config.mCaps.isEmpty()) { nc = createDefaultNetworkCapabilities(false /* isTestIface */, config.mTransport); } else { nc = createNetworkCapabilities(config.mCaps, config.mTransport).build();