diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index c4ea9ae8b0..cedffe15cf 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(); + iface.unregisterNetworkOfferAndStop(); + 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 */ @@ -292,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; @@ -401,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(); } @@ -412,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(); } @@ -454,7 +466,7 @@ public class EthernetNetworkFactory { + "transport type."); } - private static NetworkScore getBestNetworkScore() { + private static NetworkScore getNetworkScore() { return new NetworkScore.Builder().build(); } @@ -465,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(); } } @@ -629,14 +639,12 @@ 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 // register network offer - mNetworkProvider.registerNetworkOffer(getBestNetworkScore(), - new NetworkCapabilities(mCapabilities), (cmd) -> mHandler.post(cmd), - mNetworkOfferCallback); + registerNetworkOffer(); } return true; @@ -660,10 +668,16 @@ public class EthernetNetworkFactory { mLinkProperties.clear(); } - public void destroy() { + private void registerNetworkOffer() { + mNetworkProvider.registerNetworkOffer(getNetworkScore(), + new NetworkCapabilities(mCapabilities), cmd -> mHandler.post(cmd), + mNetworkOfferCallback); + } + + public void unregisterNetworkOfferAndStop() { mNetworkProvider.unregisterNetworkOffer(mNetworkOfferCallback); stop(); - mRequests.clear(); + mRequestIds.clear(); } private static void provisionIpClient(@NonNull final IpClientManager ipClient, diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index 458d225954..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 @@ -69,13 +68,13 @@ 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.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 @@ -206,12 +205,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() { @@ -458,32 +453,45 @@ 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) + 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() + } } @Test fun testCallbacks_forServerModeInterfaces() { - // do not run this test when adb might be connected over ethernet. - assumeFalse(isAdbOverNetwork()) + // do not run this test if an interface that can be used for tethering already exists. + assumeNoInterfaceForTetheringAvailable() + + 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) } /** @@ -651,4 +659,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() + } }