Merge changes I3250bdb9,I20f6e5e1,I70c96a09,I88f77bd4,If6cefa51, ...
* changes:
ethernet: rename methods in EthernetNetworkFactory
ethernet: move registerNetworkOffer into its own function
ethernet: provider to keep track of requestId
ethernet: return result when removing interface
ethernet: enable testCallbacks_forServerModeInterfaces on CF
ethernet: expand server mode callbacks test
ethernet: add test for removing interface while in server mode
diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
index c4ea9ae..cedffe1 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.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 @@
}
@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 @@
private boolean mLinkUp;
private int mLegacyType;
private LinkProperties mLinkProperties = new LinkProperties();
- private Set<NetworkRequest> mRequests = new ArraySet<>();
+ private final Set<Integer> mRequestIds = new ArraySet<>();
private volatile @Nullable IpClientManager mIpClient;
private @NonNull NetworkCapabilities mCapabilities;
@@ -401,7 +409,7 @@
// 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 @@
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 @@
+ "transport type.");
}
- private static NetworkScore getBestNetworkScore() {
+ private static NetworkScore getNetworkScore() {
return new NetworkScore.Builder().build();
}
@@ -465,9 +477,7 @@
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 @@
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 @@
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 458d225..1748612 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.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.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 @@
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 @@
}
}
- // 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 @@
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()
+ }
}