From f34945bf6f7e1db2b4361f4938f5d2a35bc23707 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 22 Jun 2022 17:21:30 -0700 Subject: [PATCH 1/7] ethernet: add test for removing interface while in server mode Bug: 236892130 Test: atest EthernetManagerTest#testRemoveInterface_whileInServerMode Change-Id: I3ca71cd87301da8e96d31f7632b69d318ee97d4a --- .../android/net/cts/EthernetManagerTest.kt | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index 458d225954..20d4f78eb5 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -68,14 +68,15 @@ import com.android.testutils.TestableNetworkCallback import com.android.testutils.runAsShell import com.android.testutils.waitForIdle import org.junit.After -import org.junit.Assume.assumeTrue import org.junit.Assume.assumeFalse +import org.junit.Assume.assumeTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.net.Inet6Address import java.util.concurrent.CompletableFuture import java.util.concurrent.ExecutionException +import java.util.concurrent.TimeoutException import java.util.concurrent.TimeUnit import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -458,6 +459,21 @@ class EthernetManagerTest { } } + private fun assumeNoInterfaceForTetheringAvailable() { + // Interfaces that have configured NetworkCapabilities will never be used for tethering, + // see aosp/2123900. + try { + // assumeException does not exist. + requestTetheredInterface().expectOnAvailable() + // interface used for tethering is available, throw an assumption error. + assumeTrue(false) + } catch (e: TimeoutException) { + // do nothing -- the TimeoutException indicates that no interface is available for + // tethering. + releaseTetheredInterface() + } + } + // TODO: this function is now used in two places (EthernetManagerTest and // EthernetTetheringTest), so it should be moved to testutils. private fun isAdbOverNetwork(): Boolean { @@ -469,6 +485,7 @@ class EthernetManagerTest { @Test fun testCallbacks_forServerModeInterfaces() { // do not run this test when adb might be connected over ethernet. + // TODO: Consider using assumeNoInterfaceForTetheringAvailable() instead, so this runs on CF assumeFalse(isAdbOverNetwork()) val listener = EthernetStateListener() @@ -651,4 +668,26 @@ class EthernetManagerTest { iface.setCarrierEnabled(false) cb.eventuallyExpectLost() } + + @Test + fun testRemoveInterface_whileInServerMode() { + assumeNoInterfaceForTetheringAvailable() + + val listener = EthernetStateListener() + addInterfaceStateListener(listener) + + val iface = createInterface() + val ifaceName = requestTetheredInterface().expectOnAvailable() + + assertEquals(iface.name, ifaceName) + listener.eventuallyExpect(iface, STATE_LINK_UP, ROLE_SERVER) + + removeInterface(iface) + + // Note: removeInterface already verifies that a STATE_ABSENT, ROLE_NONE callback is + // received, but it can't hurt to explicitly check for it. + listener.expectCallback(iface, STATE_ABSENT, ROLE_NONE) + releaseTetheredInterface() + listener.assertNoCallback() + } } From 43fa51ca2e1918b3e138547373dd3731215d41cd Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 22 Jun 2022 19:10:51 -0700 Subject: [PATCH 2/7] ethernet: expand server mode callbacks test Bug: 236895792 Test: atest EthernetManagerTest Change-Id: Ib74fc0689592ebce7247d5f36a60b68ede6713a9 --- .../android/net/cts/EthernetManagerTest.kt | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index 20d4f78eb5..a951b993b7 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -207,12 +207,8 @@ class EthernetManagerTest { fun eventuallyExpect(expected: CallbackEntry) = events.poll(TIMEOUT_MS) { it == expected } - fun eventuallyExpect(interfaceName: String, state: Int, role: Int) { - assertNotNull(eventuallyExpect(createChangeEvent(interfaceName, state, role))) - } - fun eventuallyExpect(iface: EthernetTestInterface, state: Int, role: Int) { - eventuallyExpect(iface.name, state, role) + assertNotNull(eventuallyExpect(createChangeEvent(iface.name, state, role))) } fun assertNoCallback() { @@ -488,19 +484,25 @@ class EthernetManagerTest { // TODO: Consider using assumeNoInterfaceForTetheringAvailable() instead, so this runs on CF assumeFalse(isAdbOverNetwork()) + val iface = createInterface() + requestTetheredInterface().expectOnAvailable() + val listener = EthernetStateListener() addInterfaceStateListener(listener) - - // it is possible that a physical interface is present, so it is not guaranteed that iface - // will be put into server mode. This should not matter for the test though. Calling - // createInterface() makes sure we have at least one interface available. - val iface = createInterface() - val cb = requestTetheredInterface() - val ifaceName = cb.expectOnAvailable() - listener.eventuallyExpect(ifaceName, STATE_LINK_UP, ROLE_SERVER) + // TODO(b/236895792): THIS IS A BUG! Existing server mode interfaces are not reported when + // an InterfaceStateListener is registered. + // Note: using eventuallyExpect as there may be other interfaces present. + // listener.eventuallyExpect(iface, STATE_LINK_UP, ROLE_SERVER) releaseTetheredInterface() - listener.eventuallyExpect(ifaceName, STATE_LINK_UP, ROLE_CLIENT) + listener.eventuallyExpect(iface, STATE_LINK_UP, ROLE_CLIENT) + + requestTetheredInterface().expectOnAvailable() + // This should be changed to expectCallback, once b/236895792 is fixed. + listener.eventuallyExpect(iface, STATE_LINK_UP, ROLE_SERVER) + + releaseTetheredInterface() + listener.expectCallback(iface, STATE_LINK_UP, ROLE_CLIENT) } /** From 525c30205c2c5563a80a645eb529396e5a95c163 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 22 Jun 2022 19:13:11 -0700 Subject: [PATCH 3/7] ethernet: enable testCallbacks_forServerModeInterfaces on CF testCallbacks_forServerModeInterfaces was previously skipped on CF as isAdbOverNetwork() was true. The existing interface on CF has configured NetworkCapabilities which excludes it from being used in tethering. Using assumeNoInterfaceForTetheringAvailable() is therefore a better assumption check for this test. Test: atest EthernetManagerTest Change-Id: If6cefa519b32d041c8dccc0a58acb3028eefd878 --- .../src/android/net/cts/EthernetManagerTest.kt | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index a951b993b7..1748612e8e 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -49,7 +49,6 @@ import android.os.Build import android.os.Handler import android.os.Looper import android.os.OutcomeReceiver -import android.os.SystemProperties import android.platform.test.annotations.AppModeFull import android.util.ArraySet import androidx.test.platform.app.InstrumentationRegistry @@ -68,7 +67,6 @@ import com.android.testutils.TestableNetworkCallback import com.android.testutils.runAsShell import com.android.testutils.waitForIdle import org.junit.After -import org.junit.Assume.assumeFalse import org.junit.Assume.assumeTrue import org.junit.Before import org.junit.Test @@ -470,19 +468,10 @@ class EthernetManagerTest { } } - // TODO: this function is now used in two places (EthernetManagerTest and - // EthernetTetheringTest), so it should be moved to testutils. - private fun isAdbOverNetwork(): Boolean { - // If adb TCP port opened, this test may running by adb over network. - return (SystemProperties.getInt("persist.adb.tcp.port", -1) > -1 || - SystemProperties.getInt("service.adb.tcp.port", -1) > -1) - } - @Test fun testCallbacks_forServerModeInterfaces() { - // do not run this test when adb might be connected over ethernet. - // TODO: Consider using assumeNoInterfaceForTetheringAvailable() instead, so this runs on CF - assumeFalse(isAdbOverNetwork()) + // do not run this test if an interface that can be used for tethering already exists. + assumeNoInterfaceForTetheringAvailable() val iface = createInterface() requestTetheredInterface().expectOnAvailable() From dcf73e897316e2fde3d951f1a56b2b819e98b338 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 21 Jun 2022 14:05:53 -0700 Subject: [PATCH 4/7] ethernet: return result when removing interface This is not used yet, but is consistent with the other EthernetNetworkFactory APIs and will come in handy soon as logging and error handling is expanded. Also removing NetworkProvider from the class documentation as EthernetNetworkFactory does not inherit from it. This change was suggested in aosp/2059590. Test: TH Change-Id: I88f77bd461d50d5df73164560bc5a8395e584d05 --- .../server/ethernet/EthernetNetworkFactory.java | 12 ++++++++++-- 1 file changed, 10 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 c4ea9ae8b0..d5ec6040e8 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -60,7 +60,9 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; /** - * {@link NetworkProvider} that manages NetworkOffers for Ethernet networks. + * Class that manages NetworkOffers for Ethernet networks. + * + * TODO: this class should be merged into EthernetTracker. */ public class EthernetNetworkFactory { private final static String TAG = EthernetNetworkFactory.class.getSimpleName(); @@ -221,11 +223,17 @@ public class EthernetNetworkFactory { } @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - protected void removeInterface(String interfaceName) { + protected boolean removeInterface(String interfaceName) { NetworkInterfaceState iface = mTrackingInterfaces.remove(interfaceName); if (iface != null) { iface.destroy(); + return true; } + // 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"); + return false; } /** Returns true if state has been modified */ From b9f73c8f6789151b268e2e344b31b7d733195959 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 21 Jun 2022 14:15:47 -0700 Subject: [PATCH 5/7] ethernet: provider to keep track of requestId Keeping track of request ids instead of the entire NetworkRequest is slightly more efficient. Per suggestion in aosp/2059590. Test: TH Change-Id: I70c96a09d8f011d7b59852f2e9798154ee91c6d8 --- .../server/ethernet/EthernetNetworkFactory.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index d5ec6040e8..9834b18c32 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -300,7 +300,7 @@ public class EthernetNetworkFactory { private boolean mLinkUp; private int mLegacyType; private LinkProperties mLinkProperties = new LinkProperties(); - private Set mRequests = new ArraySet<>(); + private final Set mRequestIds = new ArraySet<>(); private volatile @Nullable IpClientManager mIpClient; private @NonNull NetworkCapabilities mCapabilities; @@ -409,7 +409,7 @@ public class EthernetNetworkFactory { // existing requests. // ConnectivityService filters requests for us based on the NetworkCapabilities // passed in the registerNetworkOffer() call. - mRequests.add(request); + mRequestIds.add(request.requestId); // if the network is already started, this is a no-op. start(); } @@ -420,8 +420,12 @@ public class EthernetNetworkFactory { Log.d(TAG, String.format("%s: onNetworkUnneeded for request: %s", name, request)); } - mRequests.remove(request); - if (mRequests.isEmpty()) { + if (!mRequestIds.remove(request.requestId)) { + // This can only happen if onNetworkNeeded was not called for a request or if + // the requestId changed. Both should *never* happen. + Log.wtf(TAG, "onNetworkUnneeded called for unknown request"); + } + if (mRequestIds.isEmpty()) { // not currently serving any requests, stop the network. stop(); } @@ -671,7 +675,7 @@ public class EthernetNetworkFactory { public void destroy() { mNetworkProvider.unregisterNetworkOffer(mNetworkOfferCallback); stop(); - mRequests.clear(); + mRequestIds.clear(); } private static void provisionIpClient(@NonNull final IpClientManager ipClient, From 6ae808eead95f8aa9cf3f7b3d1fd8fa6bba0fd09 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 21 Jun 2022 14:20:27 -0700 Subject: [PATCH 6/7] ethernet: move registerNetworkOffer into its own function Doing this makes the code much more readable. Per suggestion in aosp/2059590. Test: TH Change-Id: I20f6e5e10bae717ddd3d6cbc62b6b187854dbcd3 --- .../server/ethernet/EthernetNetworkFactory.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 9834b18c32..5c22771810 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -477,9 +477,7 @@ public class EthernetNetworkFactory { if (mLinkUp) { // registering a new network offer will update the existing one, not install a // new one. - mNetworkProvider.registerNetworkOffer(getBestNetworkScore(), - new NetworkCapabilities(capabilities), cmd -> mHandler.post(cmd), - mNetworkOfferCallback); + registerNetworkOffer(); } } @@ -646,9 +644,7 @@ public class EthernetNetworkFactory { EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, name, null); } else { // was down, goes up // register network offer - mNetworkProvider.registerNetworkOffer(getBestNetworkScore(), - new NetworkCapabilities(mCapabilities), (cmd) -> mHandler.post(cmd), - mNetworkOfferCallback); + registerNetworkOffer(); } return true; @@ -672,6 +668,12 @@ public class EthernetNetworkFactory { mLinkProperties.clear(); } + private void registerNetworkOffer() { + mNetworkProvider.registerNetworkOffer(getBestNetworkScore(), + new NetworkCapabilities(mCapabilities), cmd -> mHandler.post(cmd), + mNetworkOfferCallback); + } + public void destroy() { mNetworkProvider.unregisterNetworkOffer(mNetworkOfferCallback); stop(); From bb8b44e127d3447d8275b852ceef82b7070b227d Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 21 Jun 2022 14:21:36 -0700 Subject: [PATCH 7/7] ethernet: rename methods in EthernetNetworkFactory For clarity, renaming the following functions: getBestNetworkScore() -> getNetworkScore() destroy() -> unregisterNetworkOfferAndStop() This is more inline with their behavior and the other functions in EthernetNetworkFactory. Test: TH Change-Id: I3250bdb91e15433ce89354bed46d011f634f3da5 --- .../server/ethernet/EthernetNetworkFactory.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 5c22771810..cedffe15cf 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -226,7 +226,7 @@ public class EthernetNetworkFactory { protected boolean removeInterface(String interfaceName) { NetworkInterfaceState iface = mTrackingInterfaces.remove(interfaceName); if (iface != null) { - iface.destroy(); + iface.unregisterNetworkOfferAndStop(); return true; } // TODO(b/236892130): if an interface is currently in server mode, it may not be properly @@ -466,7 +466,7 @@ public class EthernetNetworkFactory { + "transport type."); } - private static NetworkScore getBestNetworkScore() { + private static NetworkScore getNetworkScore() { return new NetworkScore.Builder().build(); } @@ -639,7 +639,7 @@ public class EthernetNetworkFactory { if (!up) { // was up, goes down // retract network offer and stop IpClient. - destroy(); + unregisterNetworkOfferAndStop(); // If only setting the interface down, send a callback to signal completion. EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, name, null); } else { // was down, goes up @@ -669,12 +669,12 @@ public class EthernetNetworkFactory { } private void registerNetworkOffer() { - mNetworkProvider.registerNetworkOffer(getBestNetworkScore(), + mNetworkProvider.registerNetworkOffer(getNetworkScore(), new NetworkCapabilities(mCapabilities), cmd -> mHandler.post(cmd), mNetworkOfferCallback); } - public void destroy() { + public void unregisterNetworkOfferAndStop() { mNetworkProvider.unregisterNetworkOffer(mNetworkOfferCallback); stop(); mRequestIds.clear();