Merge "Enable and disable usb IpServer according to ACTION_USB_STATE"

Tethering/src/com/android/networkstack/tethering/Tethering.java

@@ -181,12 +181,16 @@ public class Tethering {
         public final IpServer ipServer;
         public int lastState;
         public int lastError;
+        // This field only valid for TETHERING_USB and TETHERING_NCM.
+        // TODO: Change this from boolean to int for extension.
+        public final boolean isNcm;
 
-        TetherState(IpServer ipServer) {
+        TetherState(IpServer ipServer, boolean isNcm) {
             this.ipServer = ipServer;
             // Assume all state machines start out available and with no errors.
             lastState = IpServer.STATE_AVAILABLE;
             lastError = TETHER_ERROR_NO_ERROR;
+            this.isNcm = isNcm;
         }
 
         public boolean isCurrentlyServing() {
@@ -522,9 +526,11 @@ public class Tethering {
 
     // This method needs to exist because TETHERING_BLUETOOTH and TETHERING_WIGIG can't use
     // enableIpServing.
-    private void startOrStopIpServer(final String iface, boolean enabled) {
+    private void processInterfaceStateChange(final String iface, boolean enabled) {
-        // TODO: do not listen to USB interface state changes. USB tethering is driven only by
+        // Do not listen to USB interface state changes or USB interface add/removes. USB tethering
-        // USB_ACTION broadcasts.
+        // is driven only by USB_ACTION broadcasts.
+        final int type = ifaceNameToType(iface);
+        if (type == TETHERING_USB || type == TETHERING_NCM) return;
 
         if (enabled) {
             ensureIpServerStarted(iface);
@@ -548,7 +554,7 @@ public class Tethering {
             return;
         }
 
-        startOrStopIpServer(iface, up);
+        processInterfaceStateChange(iface, up);
     }
 
     void interfaceLinkStateChanged(String iface, boolean up) {
@@ -576,12 +582,12 @@ public class Tethering {
 
     void interfaceAdded(String iface) {
         if (VDBG) Log.d(TAG, "interfaceAdded " + iface);
-        startOrStopIpServer(iface, true /* enabled */);
+        processInterfaceStateChange(iface, true /* enabled */);
     }
 
     void interfaceRemoved(String iface) {
         if (VDBG) Log.d(TAG, "interfaceRemoved " + iface);
-        startOrStopIpServer(iface, false /* enabled */);
+        processInterfaceStateChange(iface, false /* enabled */);
     }
 
     void startTethering(final TetheringRequestParcel request, final IIntResultListener listener) {
@@ -894,7 +900,7 @@ public class Tethering {
                 : IpServer.STATE_TETHERED;
     }
 
-    private int getRequestedUsbType(boolean forNcmFunction) {
+    private int getServedUsbType(boolean forNcmFunction) {
         // TETHERING_NCM is only used if the device does not use NCM for regular USB tethering.
         if (forNcmFunction && !mConfig.isUsingNcm()) return TETHERING_NCM;
 
@@ -1036,11 +1042,11 @@ public class Tethering {
         private void handleUsbAction(Intent intent) {
             final boolean usbConnected = intent.getBooleanExtra(USB_CONNECTED, false);
             final boolean usbConfigured = intent.getBooleanExtra(USB_CONFIGURED, false);
-            final boolean rndisEnabled = intent.getBooleanExtra(USB_FUNCTION_RNDIS, false);
+            final boolean usbRndis = intent.getBooleanExtra(USB_FUNCTION_RNDIS, false);
-            final boolean ncmEnabled = intent.getBooleanExtra(USB_FUNCTION_NCM, false);
+            final boolean usbNcm = intent.getBooleanExtra(USB_FUNCTION_NCM, false);
 
             mLog.i(String.format("USB bcast connected:%s configured:%s rndis:%s ncm:%s",
-                    usbConnected, usbConfigured, rndisEnabled, ncmEnabled));
+                    usbConnected, usbConfigured, usbRndis, usbNcm));
 
             // There are three types of ACTION_USB_STATE:
             //
@@ -1057,18 +1063,45 @@ public class Tethering {
             //       functions are ready to use.
             //
             // For more explanation, see b/62552150 .
-            if (!usbConnected && (mRndisEnabled || mNcmEnabled)) {
+            boolean rndisEnabled = usbConfigured && usbRndis;
-                // Turn off tethering if it was enabled and there is a disconnect.
+            boolean ncmEnabled = usbConfigured && usbNcm;
-                disableUsbIpServing(TETHERING_USB);
+            if (!usbConnected) {
-                mEntitlementMgr.stopProvisioningIfNeeded(TETHERING_USB);
+                // Don't stop provisioning if function is disabled but usb is still connected. The
-            } else if (usbConfigured && rndisEnabled) {
+                // function may be disable/enable to handle ip conflict condition (disabling the
-                // Tether if rndis is enabled and usb is configured.
+                // function is necessary to ensure the connected device sees a disconnect).
-                enableUsbIpServing(false /* isNcm */);
+                // Normally the provisioning should be stopped by stopTethering(int)
-            } else if (usbConfigured && ncmEnabled) {
+                maybeStopUsbProvisioning();
-                enableUsbIpServing(true /* isNcm */);
+                rndisEnabled = false;
+                ncmEnabled = false;
+            }
+
+            if (mRndisEnabled != rndisEnabled) {
+                changeUsbIpServing(rndisEnabled, false /* forNcmFunction */);
+                mRndisEnabled = rndisEnabled;
+            }
+
+            if (mNcmEnabled != ncmEnabled) {
+                changeUsbIpServing(ncmEnabled, true /* forNcmFunction */);
+                mNcmEnabled = ncmEnabled;
+            }
+        }
+
+        private void changeUsbIpServing(boolean enable, boolean forNcmFunction) {
+            if (enable) {
+                // enable ip serving if function is enabled and usb is configured.
+                enableUsbIpServing(forNcmFunction);
+            } else {
+                disableUsbIpServing(forNcmFunction);
+            }
+        }
+
+        private void maybeStopUsbProvisioning() {
+            for (int i = 0; i < mTetherStates.size(); i++) {
+                final int type = mTetherStates.valueAt(i).ipServer.interfaceType();
+                if (type == TETHERING_USB || type == TETHERING_NCM) {
+                    mEntitlementMgr.stopProvisioningIfNeeded(type);
+                }
             }
-            mRndisEnabled = usbConfigured && rndisEnabled;
-            mNcmEnabled = usbConfigured && ncmEnabled;
         }
 
         private void handleWifiApAction(Intent intent) {
@@ -1216,7 +1249,12 @@ public class Tethering {
     }
 
     private void enableIpServing(int tetheringType, String ifname, int ipServingMode) {
-        ensureIpServerStarted(ifname, tetheringType);
+        enableIpServing(tetheringType, ifname, ipServingMode, false /* isNcm */);
+    }
+
+    private void enableIpServing(int tetheringType, String ifname, int ipServingMode,
+            boolean isNcm) {
+        ensureIpServerStarted(ifname, tetheringType, isNcm);
         changeInterfaceState(ifname, ipServingMode);
     }
 
@@ -1289,15 +1327,22 @@ public class Tethering {
         }
     }
 
-    // TODO: Consider renaming to something more accurate in its description.
+    // TODO: Pass TetheringRequest into this method. The code can look at the existing requests
+    // to see which one matches the function that was enabled. That will tell the code what
+    // tethering type was requested, without having to guess it from the configuration.
     // This method:
     //     - allows requesting either tethering or local hotspot serving states
-    //     - handles both enabling and disabling serving states
     //     - only tethers the first matching interface in listInterfaces()
     //       order of a given type
-    private void enableUsbIpServing(boolean isNcm) {
+    private void enableUsbIpServing(boolean forNcmFunction) {
-        final int interfaceType = getRequestedUsbType(isNcm);
+        // Note: TetheringConfiguration#isUsingNcm can change between the call to
-        final int requestedState = getRequestedState(interfaceType);
+        // startTethering(TETHERING_USB) and the ACTION_USB_STATE broadcast. If the USB tethering
+        // function changes from NCM to RNDIS, this can lead to Tethering starting NCM tethering
+        // as local-only. But if this happens, the SettingsObserver will call stopTetheringInternal
+        // for both TETHERING_USB and TETHERING_NCM, so the local-only NCM interface will be
+        // stopped immediately.
+        final int tetheringType = getServedUsbType(forNcmFunction);
+        final int requestedState = getRequestedState(tetheringType);
         String[] ifaces = null;
         try {
             ifaces = mNetd.interfaceGetList();
@@ -1306,49 +1351,28 @@ public class Tethering {
             return;
         }
 
-        String chosenIface = null;
         if (ifaces != null) {
             for (String iface : ifaces) {
-                if (ifaceNameToType(iface) == interfaceType) {
+                if (ifaceNameToType(iface) == tetheringType) {
-                    chosenIface = iface;
+                    enableIpServing(tetheringType, iface, requestedState, forNcmFunction);
-                    break;
-                }
-            }
-        }
-
-        if (chosenIface == null) {
-            Log.e(TAG, "could not find iface of type " + interfaceType);
                     return;
                 }
-
-        changeInterfaceState(chosenIface, requestedState);
-    }
-
-    private void disableUsbIpServing(int interfaceType) {
-        String[] ifaces = null;
-        try {
-            ifaces = mNetd.interfaceGetList();
-        } catch (RemoteException | ServiceSpecificException e) {
-            mLog.e("Cannot disableUsbIpServing due to error listing Interfaces" + e);
-            return;
-        }
-
-        String chosenIface = null;
-        if (ifaces != null) {
-            for (String iface : ifaces) {
-                if (ifaceNameToType(iface) == interfaceType) {
-                    chosenIface = iface;
-                    break;
-                }
             }
         }
 
-        if (chosenIface == null) {
+        mLog.e("could not enable IpServer for function " + (forNcmFunction ? "NCM" : "RNDIS"));
-            Log.e(TAG, "could not find iface of type " + interfaceType);
-            return;
     }
 
-        changeInterfaceState(chosenIface, IpServer.STATE_AVAILABLE);
+    private void disableUsbIpServing(boolean forNcmFunction) {
+        for (int i = 0; i < mTetherStates.size(); i++) {
+            final TetherState state = mTetherStates.valueAt(i);
+            final int type = state.ipServer.interfaceType();
+            if (type != TETHERING_USB && type != TETHERING_NCM) continue;
+
+            if (state.isNcm == forNcmFunction) {
+                ensureIpServerStopped(state.ipServer.interfaceName());
+            }
+        }
     }
 
     private void changeInterfaceState(String ifname, int requestedState) {
@@ -2545,10 +2569,10 @@ public class Tethering {
             return;
         }
 
-        ensureIpServerStarted(iface, interfaceType);
+        ensureIpServerStarted(iface, interfaceType, false /* isNcm */);
     }
 
-    private void ensureIpServerStarted(final String iface, int interfaceType) {
+    private void ensureIpServerStarted(final String iface, int interfaceType, boolean isNcm) {
         // If we have already started a TISM for this interface, skip.
         if (mTetherStates.containsKey(iface)) {
             mLog.log("active iface (" + iface + ") reported as added, ignoring");
@@ -2560,7 +2584,7 @@ public class Tethering {
                 new IpServer(iface, mLooper, interfaceType, mLog, mNetd, mBpfCoordinator,
                              makeControlCallback(), mConfig.enableLegacyDhcpServer,
                              mConfig.isBpfOffloadEnabled(), mPrivateAddressCoordinator,
-                             mDeps.getIpServerDependencies()));
+                             mDeps.getIpServerDependencies()), isNcm);
         mTetherStates.put(iface, tetherState);
         tetherState.ipServer.start();
     }

Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java

@@ -234,6 +234,8 @@ public class TetheringTest {
     private static final int WIFI_NETID = 101;
     private static final int DUN_NETID = 102;
 
+    private static final int TETHER_USB_RNDIS_NCM_FUNCTIONS = 2;
+
     private static final int DHCPSERVER_START_TIMEOUT_MS = 1000;
 
     @Mock private ApplicationInfo mApplicationInfo;
@@ -738,7 +740,16 @@ public class TetheringTest {
         mLooper.dispatchAll();
     }
 
+    // enableType:
+    // No function enabled            = -1
+    // TETHER_USB_RNDIS_FUNCTION      = 0
+    // TETHER_USB_NCM_FUNCTIONS       = 1
+    // TETHER_USB_RNDIS_NCM_FUNCTIONS = 2
     private boolean tetherUsbFunctionMatches(int function, int enabledType) {
+        if (enabledType < 0) return false;
+
+        if (enabledType == TETHER_USB_RNDIS_NCM_FUNCTIONS) return function < enabledType;
+
         return function == enabledType;
     }
 
@@ -822,8 +833,6 @@ public class TetheringTest {
         mTethering.startTethering(createTetheringRequestParcel(TETHERING_NCM), null);
         mLooper.dispatchAll();
         verify(mUsbManager, times(1)).setCurrentFunctions(UsbManager.FUNCTION_NCM);
-
-        mTethering.interfaceStatusChanged(TEST_NCM_IFNAME, true);
     }
 
     private void prepareUsbTethering() {
@@ -1903,7 +1912,6 @@ public class TetheringTest {
     private void runStopUSBTethering() {
         mTethering.stopTethering(TETHERING_USB);
         mLooper.dispatchAll();
-        mTethering.interfaceRemoved(TEST_RNDIS_IFNAME);
         sendUsbBroadcast(true, true, -1 /* function */);
         mLooper.dispatchAll();
         verify(mUsbManager).setCurrentFunctions(UsbManager.FUNCTION_NONE);
@@ -2087,7 +2095,7 @@ public class TetheringTest {
         setDataSaverEnabled(true);
         // Verify that tethering should be disabled.
         verify(mUsbManager, times(1)).setCurrentFunctions(UsbManager.FUNCTION_NONE);
-        mTethering.interfaceRemoved(TEST_RNDIS_IFNAME);
+        sendUsbBroadcast(true, true, -1 /* function */);
         mLooper.dispatchAll();
         assertEquals(mTethering.getTetheredIfaces(), new String[0]);
         reset(mUsbManager, mIPv6TetheringCoordinator);
@@ -2350,14 +2358,14 @@ public class TetheringTest {
         final String ipv4Address = ifaceConfigCaptor.getValue().ipv4Addr;
         verify(mDhcpServer, timeout(DHCPSERVER_START_TIMEOUT_MS).times(1)).startWithCallbacks(
                 any(), any());
-        reset(mNetd, mUsbManager);
+        reset(mUsbManager);
 
         // Cause a prefix conflict by assigning a /30 out of the downstream's /24 to the upstream.
         updateV4Upstream(new LinkAddress(InetAddresses.parseNumericAddress(ipv4Address), 30),
                 wifiNetwork, TEST_WIFI_IFNAME, TRANSPORT_WIFI);
         // verify turn off usb tethering
         verify(mUsbManager).setCurrentFunctions(UsbManager.FUNCTION_NONE);
-        mTethering.interfaceRemoved(TEST_RNDIS_IFNAME);
+        sendUsbBroadcast(true, true, -1 /* function */);
         mLooper.dispatchAll();
         // verify restart usb tethering
         verify(mUsbManager).setCurrentFunctions(UsbManager.FUNCTION_RNDIS);
@@ -2398,7 +2406,7 @@ public class TetheringTest {
         verify(mUsbManager).setCurrentFunctions(UsbManager.FUNCTION_NONE);
         // verify turn off ethernet tethering
         verify(mockRequest).release();
-        mTethering.interfaceRemoved(TEST_RNDIS_IFNAME);
+        sendUsbBroadcast(true, true, -1 /* function */);
         ethCallback.onUnavailable();
         mLooper.dispatchAll();
         // verify restart usb tethering