From ed41dfeb44fa117db968b675cea138bfad0e0693 Mon Sep 17 00:00:00 2001 From: James Mattis Date: Mon, 28 Feb 2022 14:08:12 -0800 Subject: [PATCH] Ethernet validate if iface is tracked on handler Removing synchronous validation of whether a particular interface is tracked or not and instead relying on asynchronous validation and callbacks. An interface can be in the midst of being provisioned and checking if it is tracked sychronously before provisioning is complete will erroneously throw an error for a call that would have been successful when executed on the ethernet handler thread. Bug: 210487893 Bug: 210485380 Test: atest EthernetServiceTests Change-Id: Ib70312a240cab412a54ca7f598893aa9b1e108fd --- .../ethernet/EthernetNetworkFactory.java | 26 ++++++++++--------- .../server/ethernet/EthernetServiceImpl.java | 13 +++------- .../ethernet/EthernetNetworkFactoryTest.java | 14 ++++++++++ .../ethernet/EthernetServiceImplTest.java | 24 ----------------- 4 files changed, 31 insertions(+), 46 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index f266386024..8ce27a64b1 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -220,20 +220,17 @@ public class EthernetNetworkFactory extends NetworkFactory { @NonNull final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, @Nullable final IEthernetNetworkManagementListener listener) { - enforceInterfaceIsTracked(ifaceName); + if (!hasInterface(ifaceName)) { + maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); + return; + } + final NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName); iface.updateInterface(ipConfig, capabilities, listener); mTrackingInterfaces.put(ifaceName, iface); updateCapabilityFilter(); } - private void enforceInterfaceIsTracked(@NonNull final String ifaceName) { - if (!hasInterface(ifaceName)) { - throw new UnsupportedOperationException( - "Interface with name " + ifaceName + " is not being tracked."); - } - } - private static NetworkCapabilities mixInCapabilities(NetworkCapabilities nc, NetworkCapabilities addedNc) { final NetworkCapabilities.Builder builder = new NetworkCapabilities.Builder(nc); @@ -272,10 +269,8 @@ public class EthernetNetworkFactory extends NetworkFactory { @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up, @Nullable final IEthernetNetworkManagementListener listener) { - if (!mTrackingInterfaces.containsKey(ifaceName)) { - maybeSendNetworkManagementCallback(listener, null, - new EthernetNetworkManagementException( - ifaceName + " can't be updated as it is not available.")); + if (!hasInterface(ifaceName)) { + maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); return false; } @@ -287,6 +282,13 @@ public class EthernetNetworkFactory extends NetworkFactory { return iface.updateLinkState(up, listener); } + private void maybeSendNetworkManagementCallbackForUntracked( + String ifaceName, IEthernetNetworkManagementListener listener) { + maybeSendNetworkManagementCallback(listener, null, + new EthernetNetworkManagementException( + ifaceName + " can't be updated as it is not available.")); + } + @VisibleForTesting protected boolean hasInterface(String ifaceName) { return mTrackingInterfaces.containsKey(ifaceName); diff --git a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java index fed500f367..f80f6a05b8 100644 --- a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java +++ b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java @@ -66,12 +66,6 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { methodName + " is only available on automotive devices."); } - private void enforceInterfaceIsTracked(final @NonNull String iface) { - if(!mTracker.isTrackingInterface(iface)) { - throw new UnsupportedOperationException("The given iface is not currently tracked."); - } - } - private boolean checkUseRestrictedNetworksPermission() { return PermissionUtils.checkAnyPermissionOf(mContext, android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS); @@ -220,13 +214,12 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { */ private void validateNetworkManagementState(@NonNull final String iface, final @NonNull String methodName) { + Objects.requireNonNull(iface, "Pass a non-null iface."); + Objects.requireNonNull(methodName, "Pass a non-null methodName."); + enforceAutomotiveDevice(methodName); enforceNetworkManagementPermission(); logIfEthernetNotStarted(); - - Objects.requireNonNull(iface, "Pass a non-null iface."); - Objects.requireNonNull(methodName, "Pass a non-null methodName."); - enforceInterfaceIsTracked(iface); } @Override diff --git a/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 33831a56d5..61425bf171 100644 --- a/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -763,4 +763,18 @@ public class EthernetNetworkFactoryTest { eq(capabilities), any(), any(), any(), any()); verifyRestart(ipConfiguration); } + + @Test + public void testUpdateInterfaceForNonExistingInterface() throws Exception { + initEthernetNetworkFactory(); + // No interface exists due to not calling createAndVerifyProvisionedInterface(...). + final NetworkCapabilities capabilities = createDefaultFilterCaps(); + final IpConfiguration ipConfiguration = createStaticIpConfig(); + final TestNetworkManagementListener listener = new TestNetworkManagementListener(); + + mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener); + + verifyNoStopOrStart(); + assertFailedListener(listener, "can't be updated as it is not available"); + } } diff --git a/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java b/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java index 28297537ea..e74a5a366c 100644 --- a/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java +++ b/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java @@ -156,30 +156,6 @@ public class EthernetServiceImplTest { }); } - @Test - public void testUpdateConfigurationRejectsWithUntrackedIface() { - shouldTrackIface(TEST_IFACE, false); - assertThrows(UnsupportedOperationException.class, () -> { - mEthernetServiceImpl.updateConfiguration(TEST_IFACE, UPDATE_REQUEST, NULL_LISTENER); - }); - } - - @Test - public void testConnectNetworkRejectsWithUntrackedIface() { - shouldTrackIface(TEST_IFACE, false); - assertThrows(UnsupportedOperationException.class, () -> { - mEthernetServiceImpl.connectNetwork(TEST_IFACE, NULL_LISTENER); - }); - } - - @Test - public void testDisconnectNetworkRejectsWithUntrackedIface() { - shouldTrackIface(TEST_IFACE, false); - assertThrows(UnsupportedOperationException.class, () -> { - mEthernetServiceImpl.disconnectNetwork(TEST_IFACE, NULL_LISTENER); - }); - } - private void denyManageEthPermission() { doThrow(new SecurityException("")).when(mContext) .enforceCallingOrSelfPermission(