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

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<NetworkRequest> mRequests = new ArraySet<>();
+        private final Set<Integer> 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 (!mRequestIds.remove(request.requestId)) {
-                if (mRequests.isEmpty()) {
+                    // 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(),
+                registerNetworkOffer();
-                        new NetworkCapabilities(capabilities), cmd -> mHandler.post(cmd),
-                        mNetworkOfferCallback);
             }
         }
 
@@ -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(),
+                registerNetworkOffer();
-                        new NetworkCapabilities(mCapabilities), (cmd) -> mHandler.post(cmd),
-                        mNetworkOfferCallback);
             }
 
             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,

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
+    private fun assumeNoInterfaceForTetheringAvailable() {
-    // EthernetTetheringTest), so it should be moved to testutils.
+        // Interfaces that have configured NetworkCapabilities will never be used for tethering,
-    private fun isAdbOverNetwork(): Boolean {
+        // see aosp/2123900.
-        // If adb TCP port opened, this test may running by adb over network.
+        try {
-        return (SystemProperties.getInt("persist.adb.tcp.port", -1) > -1 ||
+            // assumeException does not exist.
-                SystemProperties.getInt("service.adb.tcp.port", -1) > -1)
+            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.
+        // do not run this test if an interface that can be used for tethering already exists.
-        assumeFalse(isAdbOverNetwork())
+        assumeNoInterfaceForTetheringAvailable()
+
+        val iface = createInterface()
+        requestTetheredInterface().expectOnAvailable()
 
         val listener = EthernetStateListener()
         addInterfaceStateListener(listener)
-
+        // TODO(b/236895792): THIS IS A BUG! Existing server mode interfaces are not reported when
-        // it is possible that a physical interface is present, so it is not guaranteed that iface
+        // an InterfaceStateListener is registered.
-        // will be put into server mode. This should not matter for the test though. Calling
+        // Note: using eventuallyExpect as there may be other interfaces present.
-        // createInterface() makes sure we have at least one interface available.
+        // listener.eventuallyExpect(iface, STATE_LINK_UP, ROLE_SERVER)
-        val iface = createInterface()
-        val cb = requestTetheredInterface()
-        val ifaceName = cb.expectOnAvailable()
-        listener.eventuallyExpect(ifaceName, 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()
+    }
 }